8000 Improve segwit signature hash API by tcharding · Pull Request #1995 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Improve segwit signature hash API #1995

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
Merged
merged 1 commit into from
Aug 21, 2023
Merged

Improve segwit signature hash API #1995

merged 1 commit into from
Aug 21, 2023

Conversation

tcharding
Copy link
Member

The word "segwit" refers to segwit v0 and taproot but these functions are version specific. Add v0 into the function names.

This is similar to #1994, both based on recent post of mine to bitcoin dev mailing list.

@apoelstra
Copy link
Member

concept ACK. Though I wonder if we want to split these into wpkh and wsh variants rather than a single v0_segwit version. (See also #1932)

apoelstra
apoelstra previously approved these changes Aug 14, 2023
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 3a0579b78c705f5b25f61bb11cf88b3d333fcd93

@tcharding tcharding force-pushed the 08-14-sighash branch 2 times, most recently from 9054594 to 00dec50 Compare August 14, 2023 23:23
@tcharding
Copy link
Member Author
tcharding commented Aug 14, 2023 •
edited
Loading

I have a question, in the BIP-143 test vectors for wrapped p2wsh the script code is the witness script with cf pre-pended. I can't work out

  1. What that cf is
  2. Why we do not add it yet all our the sighash test vectors pass

I'd like to answer that because the script paramater to segwit_encoding_signing_data_to is called script_code so if looking at the bip and our code something is fishy.

@tcharding tcharding changed the title Add v0 to segwit version 0 signature hash functions Improve segwit signature hash API Aug 14, 2023
junderw
junderw reviewed Aug 14, 2023
View reviewed changes
bitcoin/src/crypto/sighash.rs Outdated
@@ -821,6 +843,7 @@ impl<R: Borrow<Transaction>> SighashCache<R> {
}

/// Computes the BIP143 sighash for any flag type.
#[deprecated(since = "0.30.0", note = "use segwit_v0_signature_hash instead")]
Copy link
Contributor

Choose a reason for hiding this comment

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

note needs to be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh woops, thanks mate!

@junderw
Copy link
Contributor
junderw commented Aug 14, 2023 •
edited
Loading

BIP-143

0xcf is 207, the number of bytes in the redeemScript. It is a VarInt with the encoded length.

The script code needs to be the the "witnessScript serialized as scripts inside CTxOut". Which is VarInt size + script bytes.

@apoelstra
Copy link
Member

@tcharding 0xcf is a length prefix and it is automatically added when consensus encoding byte vectors such as scripts.

1 similar comment
@apoelstra
Copy link
Member

@tcharding 0xcf is a length prefix and 8000 it is automatically added when consensus encoding byte vectors such as scripts.

@tcharding
Copy link
Member Author

ah BOOM! I've been bitten by that one before :)

@tcharding
Copy link
Member Author

Added some more docs.

@tcharding
Add p2wpkh and p2wsh signature hash functions
4300cf2 
The word "segwit" refers to segwit v0 and taproot but currently we have
`segwit_signature_hash` that is version specific (segwit v0).

- Rename `segwit_encode_signing_data_to` to
  `segwit_v0_encode_signing_data_to`
- Add `p2wpkh_signature_hash` and `p2wsh_signature_hash` functions

We keep the single encode function because the error handling is better
that way.

While we are at it test the bip-143 test vectors against all the
sighash types of wrapped p2wsh.
@apoelstra
Copy link
Member

looks good. Shitty that we have so many Results that can only trigger by somebody providing a bad input index, but I don't think there's any good way to fix that in Rust.

apoelstra
apoelstra approved these changes Aug 15, 2023
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 4300cf2

@tcharding
Copy link
Member Author

looks good. Shitty that we have so many Results that can only trigger by somebody providing a bad input index, but I don't think there's any good way to fix that in Rust.

I rekon the situation would be improved by an InputOutOfBoundsError return type for those functions, way more explicit than a SignError.

@apoelstra
Copy link
Member

Agreed. Maybe we should do a followup PR for this.

The we are already returning a sighash::Error type which is either "input out of bounds" or "io error". But the IO error case casen't can't happen in the functions that return a hash.

@tcharding
Copy link
Member Author

I'm hesitant to dig too deeply into errors because of the secp error POC but I'll have a gentle poke (in a separate PR).

@tcharding tcharding mentioned this pull request Aug 17, 2023
Merged
@stevenroose
Copy link
Collaborator

My first reaction when reading the OP was "eh again a naming discussion around segwit <> segwitv0". But as someone who has been using sighashes a lot lately, I really like this change. The scriptCode is one of the most confusing things about the segwit BIPs (see what I did there?). So this change hides that confusion concept and the argument naming is pretty clear.

ACK 4300cf2

@tcharding
Copy link
Member Author

BOOM! Reviews - I love it. Thanks bro.

@apoelstra apoelstra merged commit 5bf2117 into rust-bitcoin:master Aug 21, 2023
@tcharding tcharding deleted the 08-14-sighash branch August 22, 2023 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junderw junderw junderw left review comments

@stevenroose stevenroose stevenroose approved these changes

@apoelstra apoelstra apoelstra approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@tcharding @apoelstra @junderw @stevenroose
0