Node RPC: check for more than one pubkey when signing multisig

[pinheadmz]

fixes #554

thanks to @nodar-chkuaselidze for finding the bug.

I wrote the test in http-test.js but I'd like to move it to rpc-test.js once #550 gets merged

[codecov-io]

Codecov Report

Merging #555 into master will increase coverage by 0.45%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
+ Coverage   52.88%   53.34%   +0.45%     
==========================================
  Files         104      104              
  Lines       27700    27699       -1     
  Branches     4748     4748              
==========================================
+ Hits        14649    14775     +126     
+ Misses      13051    12924     -127

Impacted Files	Coverage Δ
lib/node/rpc.js	29.43% <ø> (+6.12%)	⬆️
lib/script/script.js	76.99% <0%> (+0.12%)	⬆️
lib/primitives/tx.js	82.93% <0%> (+0.28%)	⬆️
lib/mempool/mempool.js	43.71% <0%> (+0.45%)	⬆️
lib/primitives/mtx.js	62.23% <0%> (+1.74%)	⬆️
lib/primitives/keyring.js	73.55% <0%> (+2.47%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f24a9b6...ad12709. Read the comment docs.

[sandu-c]

you might want to keep this break if you want to apply a different SIGHASH flag per input. (currently there seems to be only one SIGHASH type supported being applied to all inputs signatures per signature request, per input).

If you do want different SIGHASH types per input then you'll need this break in place and you'll need to resend the tx to be signed as many times as the nr. of inputs to be signed.

If we remove the break line, we can atomically sign multiple inputs with the same SIGASH type, but we loose the flexibility of allowing multiple distinct SIGHASH signatures per input

[pinheadmz]

Looking at Bitcoin Core signrawtransaction, only one SIGHASH parameter is accepted by the RPC call which, I assume, applies to all inputs in the tx. In our function, SIGHASH is handled here: https://github.com/bcoin-org/bcoin/blob/master/lib/node/rpc.js#L1969-L1986 So I think that we are OK for those.

However you are right that removing the break means a private key can only be used with one scriptand witness type in a transaction because of the map. I'm not sure how Bitcoin Core would handle that case but here it would fail if the same private key was used differently in two different inputs of the same transaction:

Failure case with different script's:
input 0: Multisig Alice + Bob
input 1: Multisig Alice + Charlie

Failure case with different witness's:
input 0: P2SH Alice (legacy BIP16 script hash, multisig or not)
input 1: P2WSH Alice (SegWit BIP141 script hash, mutisig or not)

[haxwell]

What is the expected behavior here? Should we allow the same private key [to be] used differently in two different inputs of the same transaction? That seems to be the outstanding question on this PR.