Explicitly error on non-standard sighash types #233
Conversation
|
An alternate strategy would be to just try and parse the signature with the "sighash byte" removed and fail early if that doesn't work. |
|
Yes, can try this approach. Alternatively we could check the hashtype is standard: what do you think about adding a new method to |
We already do that in the current finalizer code. Maybe this catches all errors as it unlikely that we form another valid der formatted secp signature if we remove one byte. Treating unknown flags as all must have a backward compatibility good reason, that I can't think of right now. I think for this case, it is useful to have a method like from_u32_standard. |
Yep, this is consensus behaviour (but not standard!).
Will PR it upstream, should be trivial. |
|
Yeah, bitcoin core has a |
|
Done here rust-bitcoin/rust-bitcoin#573
Yes. Bitcoin-Core has |
|
Should we close this PR? |
|
iirc i was waiting for a release of rust-bitcoin. Will check and close if needed |
|
No, yeah i'm waiting for a rust-bicoin release since #233 (comment) |
|
Is the recent 0.26.2 release sufficient? |
darosior
force-pushed
the
finalizer_check_sig
branch
from
ab37293
to
c289fdf
Compare
|
Yes, just pushed an update that leverages the new |
darosior
changed the title
This moves from silently accepting non-standard sighash types with u32 to explicitly Erroring in such a case. We should never be managing signatures with such sighash types anyways, as they would not relay on today's Bitcoin network. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
darosior
force-pushed
the
finalizer_check_sig
branch
from
c289fdf
to
b8432e1
Compare
| @@ -757,7 +757,8 @@ where | |||
| F: FnOnce(&bitcoin::PublicKey, BitcoinSig) -> bool, | |||
| { | |||
| if let Some((sighash_byte, sig)) = sigser.split_last() { | |||
| let sighashtype = bitcoin::SigHashType::from_u32(*sighash_byte as u32); | |||
| let sighashtype = bitcoin::SigHashType::from_u32_standard(*sighash_byte as u32) | |||
There was a problem hiding this comment.
I wonder if the interpreter should use the from_u32_consensus. It is used to interpret the transactions that are already present in the blockchain. The main use is for block explorers which should accept non-standard transactions
There was a problem hiding this comment.
Hmm, but i thought we shouldn't be parsing invalid-by-standardness Script? Or is it called by parse_insane_*?
There was a problem hiding this comment.
The goal of parse_insane was to parse non-safe miniscripts or miniscripts containing duplicate keys or miniscripts possibly exceeding resource limits while satisfaction.
parse_insane does not currently support parsing non-standard scripts, so what you are doing in this PR seems correct. Having a non-standard miniscript is out of scope for now. This is can be addressed later(while we address #165)
There was a problem hiding this comment.
utACk b8432e1 . We should not waste review cycles for non-standard support because
- It is a low-priority feature
- It is currently inconsistently supported/partially implemented currently.
|
ACK b8432e1 |
I forgot to append the ANYONECANPAY flag with my boutique signer for hand-testing and spent some time before realising we were actually treating the last byte of the sig as a valid SIGHASH_ALL in the finalizer !
I believe it's cleaner to error early on too short signatures.
EDIT (made after the comments below): actually if i was just using SIGHASH_ALL i could have messed up my sighash type and not notifying it. Even though checking this is on the user of the lib, i believe it would be much nicer to not silently accept invalid-by-standardness signatures in rust-miniscript.