Unify `TapLeafHash` and `TapBranchHash` into `TapNodeHash` while tree construction #1479

sanket1729 · 2022-12-16T09:42:38Z

This does not completely fix #1393 but is a simple starter. Unfortunately, we still need hash_new_type for TapNodeHash because 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_script to deal with single leaf case cleanly. As a nice side effect, this removes the ugly uses of sha256::Hash that I had used to represent both TapLeafHash and TapBranchHash

Does not fix

Exporting TapTweakHash. This requires some changes to macros we use.
Safer usage of TapNodeHash by not making it hash_new_type and 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 not hash_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 TapBranchHash instead of TapNodeHash.

Kixunil · 2022-12-16T13:45:26Z

Oh, yeah, I wanted to open #1481 before but didn't have time and forgot. It should completely resolve the issue you found.

Kixunil

Have to run so not a deep review but looks mostly fine.

Kixunil · 2022-12-16T13:51:43Z

bitcoin/src/taproot.rs

@@ -622,16 +630,14 @@ impl ScriptLeaf {
 }


Shouldn't ScriptLeaf::leaf_hash return TapNodeHash? Do we even need TapLeafHash for anything now?

We need TapLeafHash in taproot sign APIs. https://docs.rs/bitcoin/latest/bitcoin/util/sighash/struct.SighashCache.html#method.taproot_signature_hash

Oh, right, in that case we should also have a trivial impl From<TapLeafhash> for TapNodehash.

apoelstra · 2022-12-17T16:20:22Z

This is really cool! It's a lot cleaner, both conceptually and code-wise, than I'd feared from the discussion in #1393.

apoelstra · 2022-12-17T16:39:26Z

bitcoin/src/taproot.rs

+
+    /// Computes the [`TapNodeHash`] from a script and a leaf version.
+    pub fn from_script(script: &Script, ver: LeafVersion) -> TapNodeHash {
+        TapNodeHash::from_inner(TapLeafHash::from_script(script, ver).into_inner())


In e03b945:

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.

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().

Since there's From now we might as well use it here?

apoelstra · 2022-12-17T16:43:13Z

bitcoin/src/taproot.rs

-    doc="Taproot-tagged hash for tapscript Merkle tree branches", false
+sha256t_hash_newtype!(TapNodeHash, TapBranchTag, MIDSTATE_TAPBRANCH, 64,
+    doc="Taproot-tagged hash with tag \"TapBranch\". This hash type is used while constructing
+    a taproot tree.", false


In 6609dae:

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"

sanket1729 · 2023-01-09T19:04:01Z

Rebased and ready for review

Kixunil

Mostly nits, just one somewhat important thing.

Kixunil · 2023-01-09T20:03:22Z

bitcoin/src/taproot.rs

 );
 #[rustfmt::skip]
 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


Could you try splitting "This hash type..." in these two into a separate paragraph to improve the doc?

Kixunil · 2023-01-09T20:04:49Z

bitcoin/src/taproot.rs

+
+    /// Computes the [`TapNodeHash`] from a script and a leaf version.
+    pub fn from_script(script: &Script, ver: LeafVersion) -> TapNodeHash {
+        TapNodeHash::from_inner(TapLeafHash::from_script(script, ver).into_inner())


Since there's From now we might as well use it here?

Kixunil · 2023-01-09T20:09:31Z

bitcoin/src/taproot.rs

@@ -544,15 +556,14 @@ pub struct NodeInfo {
 impl NodeInfo {
    /// Creates a new [`NodeInfo`] with omitted/hidden info.
    pub fn new_hidden_node(hash: sha256::Hash) -> Self {


This should accept TapNodeHash. Or am I missing something? (Only this one is blocking.)

Duh, I swear all instances of sha256::Hash in this PR, but looks like I screwed up in rebasing somewhere

sanket1729

Ready for review again

sanket1729 · 2023-01-09T20:50:11Z

bitcoin/src/taproot.rs

@@ -1064,15 +1062,17 @@ impl fmt::Display for TaprootError {
                "Merkle Tree depth({}) must be less than {}",
                d, TAPROOT_CONTROL_MAX_NODE_COUNT
            ),
-            TaprootError::InvalidTaprootLeafVersion(v) =>
-                write!(f, "Leaf version({}) must have the least significant bit 0", v),
+            TaprootError::InvalidTaprootLeafVersion(v) => {


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

Kixunil

ACK f39cd88

Kixunil · 2023-01-09T21:23:18Z

bitcoin/src/taproot.rs

 );
 #[rustfmt::skip]
 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


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.

tcharding

ACK f39cd88

Kixunil reviewed Dec 16, 2022

View reviewed changes

apoelstra reviewed Dec 17, 2022

View reviewed changes

sanket1729 mentioned this pull request Jan 9, 2023

Implement support for Hidden nodes in taproot trees and Fix taproot serde bugs #1533

Merged

sanket1729 force-pushed the tap_updates branch 2 times, most recently from 108959b to 27c049d Compare January 9, 2023 19:03

Kixunil requested changes Jan 9, 2023

View reviewed changes

Rename TapBranchHash -> TapNodeHash

5ff2635

sanket1729 force-pushed the tap_updates branch 2 times, most recently from 4bfad2f to c6e4454 Compare January 9, 2023 20:48

sanket1729 commented Jan 9, 2023

View reviewed changes

Use TapNodeHash in NodeInfo

f39cd88

This cleans up some ugly code where we had to use sha256::Hash for combining TapLeafHash and TapBranchHash

sanket1729 force-pushed the tap_updates branch from c6e4454 to f39cd88 Compare January 9, 2023 21:01

Kixunil approved these changes Jan 9, 2023

View reviewed changes

tcharding approved these changes Jan 9, 2023

View reviewed changes

sanket1729 merged commit 8df0033 into rust-bitcoin:master Jan 11, 2023

sanket1729 mentioned this pull request Jan 11, 2023

Review fixups from "Unify TapLeafHash and TapBranchHash into TapNodeHash while tree construction" #1540

Closed

tcharding added the release notes mention label Feb 27, 2023

apoelstra mentioned this pull request Jan 4, 2024

Update rust-bitcoin to 0.31.0, and associated dependencies ElementsProject/rust-elements#188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify `TapLeafHash` and `TapBranchHash` into `TapNodeHash` while tree construction #1479

Unify `TapLeafHash` and `TapBranchHash` into `TapNodeHash` while tree construction #1479

sanket1729 commented Dec 16, 2022

Kixunil commented Dec 16, 2022 •

edited

Kixunil left a comment

Kixunil Dec 16, 2022

sanket1729 Dec 16, 2022

Kixunil Dec 16, 2022

apoelstra commented Dec 17, 2022

apoelstra Dec 17, 2022

apoelstra Dec 17, 2022

sanket1729 Jan 9, 2023

Kixunil Jan 9, 2023

apoelstra Dec 17, 2022

sanket1729 commented Jan 9, 2023

Kixunil left a comment

Kixunil Jan 9, 2023

Kixunil Jan 9, 2023

Kixunil Jan 9, 2023

sanket1729 Jan 9, 2023

sanket1729 left a comment

sanket1729 Jan 9, 2023

Kixunil left a comment

Kixunil Jan 9, 2023

tcharding left a comment

Unify TapLeafHash and TapBranchHash into TapNodeHash while tree construction #1479

Unify TapLeafHash and TapBranchHash into TapNodeHash while tree construction #1479

Conversation

sanket1729 commented Dec 16, 2022

Kixunil commented Dec 16, 2022 • edited

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Dec 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Jan 9, 2023

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Unify `TapLeafHash` and `TapBranchHash` into `TapNodeHash` while tree construction #1479

Unify `TapLeafHash` and `TapBranchHash` into `TapNodeHash` while tree construction #1479

Kixunil commented Dec 16, 2022 •

edited