-
Notifications
You must be signed in to change notification settings - Fork 672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SigHashType: add a method to error on non-standard hashtypes #573
Conversation
Super nit, but a hashtype is not specific to a transaction but a signature. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concept ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should note that this being applicable only to pre-tapscript ECDSA signatures and not for Schnorr Sigs. This can also be addressed when we actually implement Taproot into rust-bitcoin.
Yes, as these rules actually become Consensus rules with BIP341. |
I think we may be mixing two things here. The sighash appears in two contexts when dealing with bitcoin transactions.
I think what we want is SighashType from u8? |
I believe so (see
I believe @apoelstra wanted to mimic Bitcoin-Core behaviour in 38b2cac |
Signed-off-by: Antoine Poinsot <darosior@protonmail.com> Co-Authored-by: sanket1729 <sanket1729@gmail.com>
2db2b49
to
196030b
Compare
Ok, i think i either responded or adressed all the review comments. Thanks everyone! Added another documentation commit after a nice deep dive with @sanket1729 about the rationale of masking with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACk 196030b. Left a minor nit about adding Hash
196030b
to
fe66d3b
Compare
Right now, any sighash type could be parsed without error, which matches consensus rules. However most of them would be invalid by standardness, so it's a bit footgun-y (even more so for pre-signed transactions protocols for which standardness is critical). This adds `from_u32_standard()`, which takes care to error if we are passed an invalid-by-current-policy-rules SIGHASH type. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
fe66d3b
to
bf98d9f
Compare
…sensus Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
bc6f54f
to
e36f3a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK e36f3a3
0x81 => Ok(SigHashType::AllPlusAnyoneCanPay), | ||
0x82 => Ok(SigHashType::NonePlusAnyoneCanPay), | ||
0x83 => Ok(SigHashType::SinglePlusAnyoneCanPay), | ||
_ => Err(NonStandardSigHashType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. in the PSBT API we'd include the input that was considered invalid in the error, but I don't think we are consistent about it. It's probably the right thing to do for new code though. I'm sorry I only see this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK e36f3a3 .
This was challeging than adding a simple function :) , but it was good learning experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack e36f3a3
Right now, any sighash type could be parsed without error, which matches
consensus rules. However most of them would be invalid by standardness,
so it's a bit footgun-y (even more so for pre-signed transactions
protocols for which standardness is critical).
This adds
from_u32_standard()
, which takes care to error if we arepassed an invalid-by-current-policy-rules SIGHASH type.
(Happy to bikeshed the method name)