TransactionBuilder doesn't support varied sighash types in multisig signatures

[dthorpe]

Is this a known issue?

If a transaction contains a multisignature script and the hash type used for each signature is not the same across all signatures in the multisig set, TransactionBuilder.fromTransaction(tx).build() will rewrite the hash type bytes of all the signatures to match the hash type of the last signature. This will cause signature verification to fail later for the signatures whose hash type byte was altered.

Example use case:

1. Alice and Bob enter into a 2 of 2 multisig address. Funds are sent to the multisig address.
2. Bob constructs a transaction spending from the 2 of 2 multisig address. The tx has a complete set of outputs but is missing other inputs (to be provided and signed by others later). Bob signs this tx with SIGHASH_ALL so that he can tell if the tx was altered in transit when he gets it back later.
3. Bob sends the partially signed tx to Alice and asks Alice to sign input 0 with SIGHASH_ALL | SIGHASH_ANYONECANPAY using her private key. Alice signs and returns the updated tx to Bob.
4. Bob receives the tx from Alice and verifies the signatures. Alice's signature passing verification means Alice signed the tx with the correct private key to spend from the 2 of 2 address. Bob's signature passing verification means that Alice (or intermediate parties) did not modify the inputs or outputs of the transaction, other than to sign them.
5. Later, Bob adds inputs to the tx to spend from Carol's address. Carol signs with SIGHASH_ALL. Bob re-signs the tx with SIGHASH_ALL, since Bob's original signature is invalidated by the changes to the inputs. Alice's original signature is not affected by changes to inputs. The tx is now fully signed and ready for broadcast to the blockchain.

This is not a blocking issue for my use case as I have figured out a workaround.

If this is not a known issue and/or you need more detail I can put together code examples.

[dcousens]

TransactionBuilder.fromTransaction(tx).build() will rewrite the hash type bytes of all the signatures to match the hash type of the last signature

Yes, we assume the same SIGHASH for all signatures on a particular scriptSig.

You're right that it should be possible to mix them up.
Thanks for the issue submission @dthorpe, if you wanted to help, test cases/fixtures are incredibly useful.

[dthorpe]

Any preference for how to implement support for heterogenous hash types in multisig?

The most direct route would be to change TransactionBuilder.inputs[].signatures[] from an array of signature byte arrays to an array of { hashType, sigbytes }. However, this would clearly break existing code that manipulates input.signatures[].

A less disruptive route would be to capture the hash types into a new array property on the input object, sibling to the existing signatures array property. input.hashType[i] is the hash type discovered with input.signatures[i]. This would break less code up front, but existing code that manipulates input.signatures[] without knowledge of input.hashTypes[] is likely to still munge things up.

Either way, capturing the signature hash types as found in the source tx and reconstructing them in build() would at least make TransactionBuilder.fromTransaction(tx).build() non-destructive.

Your thoughts?

[dcousens]

A less disruptive route would be to capture the hash types into a new array property on the input object, sibling to the existing signatures array property. input.hashType[i] is the hash type discovered with input.signatures[i].

On first thought, this seems like the best option.
But you raise a valid point:

This would break less code up front, but existing code that manipulates input.signatures[] without knowledge of input.hashTypes[] is likely to still munge things up.

Now I'm not so sure.
On the one hand, input.signatures was an internal storage anyway, and shouldn't be used by most people; but we never really stipulated that either so, on the other hand, it could break applications subtly.

I think the input.hashTypes[] is the best solution, as it won't break code that needed only the signatures, and was likely storing the first hash type themselves, and assuming it never changed.

[dcousens]

Targeting this issue for the 2.x.y release, with the preliminary change s/hashType/hashTypes (and an array used) being done as a breaking change in 2.0.0.

[dcousens]

@dthorpe by the by, why does Bob provide his signature in the transaction?

You mention:

Bob's signature passing verification means that Alice (or intermediate parties) did not modify the inputs or outputs of the transaction, other than to sign them.

Which makes sense, but Bob could just check that the hashForSignature provided before giving the transaction to Alice is the same as the hashForSignature on receipt of Alices signature addition?

Or was the point that you didn't need to maintain state (aka, remembering that hash).