Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SigHashType: add a method to error on non-standard hashtypes #573

Merged
merged 4 commits into from Feb 21, 2021
Merged

SigHashType: add a method to error on non-standard hashtypes #573

merged 4 commits into from Feb 21, 2021

Conversation

darosior
Copy link
Contributor

@darosior darosior commented Feb 18, 2021 •
edited

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.

(Happy to bikeshed the method name)

@darosior
doc: correct SigHashType doc comment
7f73d5f 
Super nit, but a hashtype is not specific to a transaction but a
signature.

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@darosior darosior mentioned this pull request Feb 18, 2021
Merged
sgeisler
sgeisler reviewed Feb 18, 2021
View reviewed changes
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concept ACK

src/blockdata/transaction.rs Outdated Show resolved Hide resolved
src/blockdata/transaction.rs Outdated Show resolved Hide resolved
src/blockdata/transaction.rs Outdated Show resolved Hide resolved
src/blockdata/transaction.rs Outdated Show resolved Hide resolved
sanket1729
sanket1729 reviewed Feb 18, 2021
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a 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.

src/blockdata/transaction.rs Outdated Show resolved Hide resolved
src/blockdata/transaction.rs Outdated Show resolved Hide resolved
@darosior
Copy link
Contributor Author

We should note that this being applicable only to pre-tapscript ECDSA signatures and not for Schnorr Sigs

Yes, as these rules actually become Consensus rules with BIP341.

@sanket1729
Copy link
Member

I think we may be mixing two things here. The sighash appears in two contexts when dealing with bitcoin transactions.

  1. It is serialized as 4 bytes u32 when used as a signature message in the ECDSA sig algorithm
  2. It is appended as a single byte when it is appended to the ECDSA sig to form a bitcoin signature.

I think what we want is SighashType from u8?

@darosior
Copy link
Contributor Author

I think what we want is SighashType from u8?

I believe so (see #rust-bitcoin), but it's a bit of a mess on bitcoind itself:

  • You can't have a >1byte SIGHASH type
  • But it's then treated as a signed integer
  • It's also serialized as an int32_t in the sighash

I believe @apoelstra wanted to mimic Bitcoin-Core behaviour in 38b2cac

@darosior @sanket1729
transaction: document why we mask sighash types with 0x9f
466f161 
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Co-Authored-by: sanket1729 <sanket1729@gmail.com>
@darosior
Copy link
Contributor Author

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 0x9f. This was apparently meant to be an extension mechanism in a precedent era of Bitcoin, at least from what we could get off the source code fossils :)

sanket1729
sanket1729 reviewed Feb 18, 2021
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a 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

src/blockdata/transaction.rs Show resolved Hide resolved
@darosior
transaction: add a method to err on non-standard types to SigHashType
bf98d9f 
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>
@darosior
transaction: deprecate SigHashType::from_u32 in favor of from_u32_con…
e36f3a3 
…sensus

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@sgeisler sgeisler added this to the 0.26.1 milestone Feb 19, 2021
sgeisler
sgeisler approved these changes Feb 19, 2021
View reviewed changes
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK e36f3a3

src/blockdata/transaction.rs
0x81 => Ok(SigHashType::AllPlusAnyoneCanPay),
0x82 => Ok(SigHashType::NonePlusAnyoneCanPay),
0x83 => Ok(SigHashType::SinglePlusAnyoneCanPay),
_ => Err(NonStandardSigHashType)
Copy link
Contributor

@sgeisler sgeisler Feb 19, 2021 •
edited

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.

sanket1729
sanket1729 approved these changes Feb 19, 2021
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a 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.

apoelstra
apoelstra approved these changes Feb 21, 2021
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack e36f3a3

@apoelstra apoelstra merged commit 2414c5b into rust-bitcoin:master Feb 21, 2021
@darosior darosior deleted the standard_sighash branch February 21, 2021 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpc dpc dpc left review comments

@apoelstra apoelstra apoelstra approved these changes

@sgeisler sgeisler sgeisler approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.26.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@darosior @sanket1729 @dpc @apoelstra @sgeisler