Unify TapLeafHash and TapBranchHash into TapNodeHash while tree construction#1479
Unify TapLeafHash and TapBranchHash into TapNodeHash while tree construction#1479sanket1729 merged 2 commits intorust-bitcoin:masterfrom
TapLeafHash and TapBranchHash into TapNodeHash while tree construction#1479Conversation
|
Oh, yeah, I wanted to open #1481 before but didn't have time and forgot. It should completely resolve the issue you found. |
Kixunil
left a comment
There was a problem hiding this comment.
Have to run so not a deep review but looks mostly fine.
bitcoin/src/taproot.rs
Outdated
There was a problem hiding this comment.
Shouldn't ScriptLeaf::leaf_hash return TapNodeHash? Do we even need TapLeafHash for anything now?
There was a problem hiding this comment.
We need TapLeafHash in taproot sign APIs. https://docs.rs/bitcoin/latest/bitcoin/util/sighash/struct.SighashCache.html#method.taproot_signature_hash
There was a problem hiding this comment.
Oh, right, in that case we should also have a trivial impl From<TapLeafhash> for TapNodehash.
|
This is really cool! It's a lot cleaner, both conceptually and code-wise, than I'd feared from the discussion in #1393. |
bitcoin/src/taproot.rs
Outdated
There was a problem hiding this comment.
In e03b9457111848cb26f3764b2d9cdc9d9ea21534:
Can you add a comment here highlighting that you're actually casting between two hash types here, since it's maybe hard to see in the from_inner(X.into_inner()) form, explaining that this is because leaves of a taptree use a different hash prefix than the internal nodes, but the Merkle tree logic does not care about the distinction. Maybe also link to the BIP and/or to #1393.
I think a regular comment is fine, no need to doccomment this, since it's an internal implementation detail.
There was a problem hiding this comment.
Also, I agree with Kix's suggestion below that we add a conversion method from TapLeafHash to TabBranchHash, so this line can be simplified to TapLeafHash::from_script(script, ver).into().
There was a problem hiding this comment.
Since there's From now we might as well use it here?
bitcoin/src/taproot.rs
Outdated
There was a problem hiding this comment.
In 6609dae247e9ac63c2a1d357183c93a55e4fb1af:
This doccomment should be updated to say that it might be tagged either with TapBranch or with TapLeaf; or maybe it should not mention this at all and just say "tagged hash used in taproot trees; see BIP-340 for tagging rules"
108959b to
27c049d
Compare
|
Rebased and ready for review |
Kixunil
left a comment
There was a problem hiding this comment.
Mostly nits, just one somewhat important thing.
bitcoin/src/taproot.rs
Outdated
There was a problem hiding this comment.
Could you try splitting "This hash type..." in these two into a separate paragraph to improve the doc?
bitcoin/src/taproot.rs
Outdated
There was a problem hiding this comment.
Since there's From now we might as well use it here?
bitcoin/src/taproot.rs
Outdated
There was a problem hiding this comment.
This should accept TapNodeHash. Or am I missing something? (Only this one is blocking.)
There was a problem hiding this comment.
Duh, I swear all instances of sha256::Hash in this PR, but looks like I screwed up in rebasing somewhere
4bfad2f to
c6e4454
Compare
sanket1729
left a comment
There was a problem hiding this comment.
Ready for review again
bitcoin/src/taproot.rs
Outdated
There was a problem hiding this comment.
A couple formatting changes got sneaked in here. Don't think it should be an issue
This cleans up some ugly code where we had to use sha256::Hash for combining TapLeafHash and TapBranchHash
c6e4454 to
f39cd88
Compare
| sha256t_hash_newtype!(TapSighashHash, TapSighashTag, MIDSTATE_TAPSIGHASH, 64, | ||
| doc="Taproot-tagged hash for the taproot signature hash", false | ||
| doc="Taproot-tagged hash with tag \"TapSighash\". | ||
| This hash type is used for computing taproot signature hash.", false |
There was a problem hiding this comment.
I think there should be an empty line, but since I'm too tired to check myself, I will ignore this nit. We can fix later.
This does not completely fix #1393 but is a simple starter. Unfortunately, we still need hash_new_type for
TapNodeHashbecause is exported as the Merkle root in taproot psbt. This requires serialization and display/from_str methods.This also adds a method 1)
TapNodeHash::from_scriptto deal with single leaf case cleanly. As a nice side effect, this removes the ugly uses ofsha256::Hashthat I had used to represent bothTapLeafHashandTapBranchHashDoes not fix
TapTweakHash. This requires some changes to macros we use.TapNodeHashby not making ithash_new_typeand having guarded constructors. As mentioned above, this is required for psbts and sharing Merkle roots. Perhaps, we might need a new type while constructing trees that is nothash_new_type, and convert it to another type that represents the final Merkle root.My overall feeling is that this is best addressed with more examples than changing and complicating existing code.
I don't feel strongly about the rename, so can go back if
TapBranchHashinstead ofTapNodeHash.