Fix signature hash returned for sighash single bug #860
Conversation
|
You could use this line of code as a citation: https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1647 But this is a well-known bit of Bitcoin folklore and doesn't really have a citation. |
7bae60f to
6cd167c
Compare
|
That gave me a name for a const definition which I wanted to add and the words I needed for the commit logs. Thanks! |
6cd167c to
4b96698
Compare
4b96698 to
f86baae
Compare
|
Sorry for the force push. The unit test uses |
There was a problem hiding this comment.
One nit, please let me know @apoelstra are we ok with the use of explicit unwrap in the non-test code in this case. If we are ok, ACK f86baae69dcece93f545b02a17a6c9015cdbf92e
src/blockdata/transaction.rs
Outdated
| @@ -410,12 +409,24 @@ impl Transaction { | |||
| script_pubkey: &Script, | |||
| sighash_u32: u32 | |||
| ) -> SigHash { | |||
| if self.is_invalid_use_of_sighash_single(sighash_u32, input_index) { | |||
| return SigHash::from_slice(&UINT256_ONE).unwrap(); // Parse won't panic for const slice. | |||
There was a problem hiding this comment.
Nit: we usually try to avoid any unwraps AFAIR:
| return SigHash::from_slice(&UINT256_ONE).unwrap(); // Parse won't panic for const slice. | |
| return SigHash::from_slice(&UINT256_ONE).expect("const-size array"). |
There was a problem hiding this comment.
Might as well fix it since we need to rebase anyway, but no, I don't particularly care either way.
Typically we do not put a whitespace character before a `:` when using explicit types.
Move helper function to above the test that uses it. Refactor only, no logic changes.
When signing a transaction will result in the sighash single bug being exploitable we should return the 1 array (equivalent to 1 as a uint256) as the signature hash. Currently we are using the correct array value but are re-hashing it, instead we should directly return it.
When signing a transaction will result in the sighash single bug being exploitable we should return the 'one array' (equivalent to 1 as a uint256) as the signature hash. Add a unit test to verify we return uint256 1 value when use of SIGHASH_SINGLE is invalid.
f86baae to
d1abfd9
Compare
|
Force push includes:
|
|
Somehow I didn't notice, when reviewing this, that it completely removes the sighash-single bug logic from |
|
That was by design, right? The initial problem was that having the sighash single bug handled in |
|
@tcharding the issue is that I guess, this was the situation before this PR and continues to be the situation after this PR, so no harm done. But a better solution would be for |
Fix up the logic that handles correctly returning the special array 1,0,0,...,0 for signature hash when the sighash single bug is exploitable i.e., when signing a transaction with SIGHASH_SINGLE for an input index that does not have a corresponding transaction output of the same index.
Resolves: #817