Allow specifing a raw `TapLeafHash` in sighash computation by afilini · Pull Request #722 · rust-bitcoin/rust-bitcoin

afilini · 2021-11-25T10:52:54Z

Still need to add some tests but the code should be ready for review. Please let me know if you have better ideas for the enum naming.

Instead of always requiring the full raw script and leaf version, allow
just specifying a raw leaf hash to the sighash computation functions.

This is very useful when dealing with PSBTs, because the
PSBT_IN_TAP_BIP32_DERIVATION field only maps a public key to a leaf
hash, so a signer could just take it and produce a signature with it
rathern than having to jump through hoops to recover the full raw
script.

src/util/sighash.rs

Kixunil · 2021-11-25T18:41:47Z

Maybe TaprootSpendPath? Shorter but usnure if clear.

afilini · 2021-12-06T13:00:27Z

Renamed the struct and rebased on the lastest master

dr-orlovsky

Not sure if I am right, but better to overlook than underlook (pls see comment)

dr-orlovsky · 2021-12-06T13:22:10Z

src/util/sighash.rs

Am I right that this will ignore the parity flag and will result in an incorrect TapLeafHash for non-even cases?

I don't think the parity flag is encoded here, I think it's only in the control block that goes in the witness

sanket1729

How about just removing the existing ScriptPath? It has the slight downside of making the callee compute the leaf hash. But makes the code here cleaner.

Unrelated: I think we should really have some helper functions with minimum parameters for taproot_key_spend_sighash, and taproot_script_spend_sighash. Most users really don't care about LeafVersion, annex, and codeseparator.

afilini · 2021-12-18T15:41:26Z

I've done a few changes:

kept ScriptPath so that it can be used to easily compute the TapLeafHash, but removed the op_codeseparator value
added two taproot_key_spend_signature_hash() and taproot_script_spend_signature_hash() methods as shortcuts. Both lack the ability to provide an annex, since it's currently unused and could just confuse the users. The latter also assumes the default op_codeseparator value.
taproot_script_spend_signature_hash() takes a generic that implements Into<TapLeafHash> for the leaf hash, so it can be used directly with a pre-computed leaf hash, or with ScriptPath

apoelstra

ACK 5aa1d02fee2df825e66347abb179d7ef26330afa

Cleaner and more flexible!

sanket1729

cr ACK 5aa1d02fee2df825e66347abb179d7ef26330afa . There is a minor spell error, but it should not be merge blocking.

sanket1729 · 2021-12-23T18:53:15Z

src/util/sighash.rs

nit: enging should be spelled engine . Same error in the following line

RCasatta · 2021-12-24T08:29:02Z

@afilini, unfortunately, I cannot merge, there is an error on test test_sighashes_with_script_path_raw_hash after merging on master

failures:

---- util::sighash::tests::test_sighashes_with_script_path_raw_hash stdout ----
thread 'util::sighash::tests::test_sighashes_with_script_path_raw_hash' panicked at 'assertion failed: `(left == right)`
  left: `[214, 109, 229, 39, 74, 96, 64, 12, 123, 8, 200, 107, 166, 183, 241, 152, 244, 6, 96, 7, 158, 223, 83, 172, 168, 157, 42, 149, 1, 49, 127, 46]`,
 right: `[207, 245, 203, 179, 121, 42, 102, 162, 155, 146, 233, 196, 9, 51, 120, 145, 230, 162, 26, 232, 30, 49, 240, 43, 241, 165, 224, 76, 32, 214, 144, 226]`', src/util/sighash.rs:997:9

sanket1729 · 2021-12-24T10:33:05Z

@afilini after some investigation, I found that the Tapleafhash is not parsed as reverse in master, but is parsed as reverse in this PR.

I changed the hashes to parse in straight order in another test vector PR #695 because the BIP printed them in without reversing.

apoelstra · 2021-12-24T14:43:36Z

Great catch @RCasatta !!

RCasatta · 2021-12-27T09:04:22Z

Great catch @RCasatta !!

Thanks! But it was merit of the great tool https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/github-merge.py with testcmd as cargo test

apoelstra · 2021-12-27T14:15:59Z

TIL about testcmd :)

Instead of always requiring the full raw script and leaf version, allow just specifying a raw leaf hash to the sighash computation functions. This is very useful when dealing with PSBTs, because the `PSBT_IN_TAP_BIP32_DERIVATION` field only maps a public key to a leaf hash, so a signer could just take it and produce a signature with it rathern than having to jump through hoops to recover the full raw script.

afilini · 2021-12-27T15:18:34Z

Fixed and rebased :)

sanket1729

Tested locally. ACK 2959e04. Reviewed range-diff with 5aa1d02

apoelstra

ACK 2959e04

Also checked the range-diff.

When fuzzing my new non-recursive tree parsing logic, I noticed that we were deviating from the released 12.0 in the way that we display l:0. This is an ambiguous fragment which can be displayed either as l:0 or u:0. In our released code we use u:0 so stick with that. This was unintentially changed as part of rust-bitcoin#722. Change it back. While we are at it add a regression test for rust-bitcoin/rust-miniscript#735

…things 1259375 miniscript: make display prefer 'u' over 'l' in the fragment l:0 (Andrew Poelstra) 67fdc50 descriptor: reject strings of the form "tr(<key>,)" (Andrew Poelstra) 00cac40 descriptor: add unit test demonstrating sanity-checking behavior in <= 12.x (Andrew Poelstra) Pull request description: This PR has three changes which are mostly unrelated except that they were all found when fuzzing my "rewrite expression parser to be nonrecursive" branch against 12.x. * First is a unit test demonstrating rust-bitcoin#734. It doesn't fix anything, just tests the current behavior. * Second is a fix for rust-bitcoin#736 (backported in rust-bitcoin#735). * Third tweaks the new `Display` code from rust-bitcoin#722 to change how the ambiguous `l:0`/`u:0` is serialized, to match 12.x. ACKs for top commit: sanket1729: ACK 1259375 Tree-SHA512: 921d65a1efd49bda0f9db488a2854b004e14518f584d832497a9dbc13a845ceec99544375385570c6ac42d4985277e8dcbb3aa8654de93235cf9067ba601f91d

RCasatta added API break This PR requires a version bump for the next release and removed API break This PR requires a version bump for the next release labels Nov 25, 2021

RCasatta reviewed Nov 25, 2021

View reviewed changes

src/util/sighash.rs Outdated Show resolved Hide resolved

afilini force-pushed the taproot-sighash-given-leaf-hash branch from eaaa02e to 35a25ca Compare December 6, 2021 13:00

afilini force-pushed the taproot-sighash-given-leaf-hash branch from 35a25ca to 656d732 Compare December 6, 2021 13:01

dr-orlovsky reviewed Dec 6, 2021

View reviewed changes

sanket1729 reviewed Dec 12, 2021

View reviewed changes

afilini force-pushed the taproot-sighash-given-leaf-hash branch from 656d732 to 5aa1d02 Compare December 18, 2021 15:40

apoelstra previously approved these changes Dec 22, 2021

View reviewed changes

sanket1729 previously approved these changes Dec 23, 2021

View reviewed changes

afilini dismissed stale reviews from sanket1729 and apoelstra via 2959e04 December 27, 2021 15:18

afilini force-pushed the taproot-sighash-given-leaf-hash branch from 5aa1d02 to 2959e04 Compare December 27, 2021 15:18

sanket1729 approved these changes Dec 27, 2021

View reviewed changes

apoelstra approved these changes Dec 28, 2021

View reviewed changes

RCasatta merged commit e511670 into rust-bitcoin:master Dec 29, 2021

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Conversation

afilini commented Nov 25, 2021

Uh oh!

Uh oh!

Kixunil commented Nov 25, 2021

Uh oh!

afilini commented Dec 6, 2021

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

afilini Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

afilini commented Dec 18, 2021

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RCasatta commented Dec 24, 2021

Uh oh!

sanket1729 commented Dec 24, 2021

Uh oh!

apoelstra commented Dec 24, 2021

Uh oh!

RCasatta commented Dec 27, 2021

Uh oh!

apoelstra commented Dec 27, 2021

Uh oh!

afilini commented Dec 27, 2021

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sanket1729 Dec 23, 2021 •

edited

Loading