From 0ccc031702d9ae31a389b4fc094a081865c41a11 Mon Sep 17 00:00:00 2001 From: ChallengeDev210 Date: Thu, 28 Jul 2022 17:18:57 -0700 Subject: [PATCH 1/4] Return EmptyTree instead of OverCompleteTree when there are no scripts to add --- src/util/taproot.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/taproot.rs b/src/util/taproot.rs index dca69e6..aaa2b0e 100644 --- a/src/util/taproot.rs +++ b/src/util/taproot.rs @@ -392,7 +392,7 @@ impl TaprootBuilder { node_weights.push((Reverse(p), NodeInfo::new_leaf_with_ver(leaf, LeafVersion::TapScript))); } if node_weights.is_empty() { - return Err(TaprootBuilderError::IncompleteTree); + return Err(TaprootBuilderError::EmptyTree); } while node_weights.len() > 1 { // Combine the last two elements and insert a new node From bfcb64999f2552ae8c42b42aed6064fb6bb5b7d0 Mon Sep 17 00:00:00 2001 From: ChallengeDev210 Date: Thu, 28 Jul 2022 17:20:16 -0700 Subject: [PATCH 2/4] Update TaprootBuilder::finalize This commit does two things: 1) BUG FIX: We should have checked that the length of the branch is 1 before computing the spend_info on it. This was introduced in #936, but slipped past my review :( 2) Update the return type to return the consumed `self` value. This function can only error when there the tree building is not complete. Returning TaprootBuilderError causes issues in downstream projects that need to deal with error cases which cannot happen in this function --- src/util/taproot.rs | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/util/taproot.rs b/src/util/taproot.rs index aaa2b0e..90f7c82 100644 --- a/src/util/taproot.rs +++ b/src/util/taproot.rs @@ -199,7 +199,8 @@ impl TaprootSpendInfo { I: IntoIterator, C: secp256k1::Verification, { - TaprootBuilder::with_huffman_tree(script_weights)?.finalize(secp, internal_key) + let builder = TaprootBuilder::with_huffman_tree(script_weights)?; + Ok(builder.finalize(secp, internal_key).expect("Huffman Tree is always complete")) } /// Creates a new key spend with `internal_key` and `merkle_root`. Provide [`None`] for @@ -451,19 +452,23 @@ impl TaprootBuilder { /// Creates a [`TaprootSpendInfo`] with the given internal key. /// - // TODO: in a future breaking API change, this no longer needs to return result + /// Returns the unmodified builder as Err if the builder is not finalized. + /// See also [`TaprootBuilder::is_finalized`] pub fn finalize( mut self, secp: &Secp256k1, internal_key: UntweakedPublicKey, - ) -> Result { - match self.branch.pop() { - None => Ok(TaprootSpendInfo::new_key_spend(secp, internal_key, None)), - Some(Some(node)) => { - Ok(TaprootSpendInfo::from_node_info(secp, internal_key, node)) + ) -> Result { + match self.branch.len() { + 0 => Ok(TaprootSpendInfo::new_key_spend(secp, internal_key, None)), + 1 => { + if let Some(Some(node)) = self.branch.pop() { + Ok(TaprootSpendInfo::from_node_info(secp, internal_key, node)) + } else { + unreachable!("Size checked above. Builder guarantees the last element is Some") + } } - _ => Err(TaprootBuilderError::IncompleteTree), - + _ => Err(self), } } @@ -1013,8 +1018,6 @@ pub enum TaprootBuilderError { OverCompleteTree, /// Invalid taproot internal key. InvalidInternalKey(secp256k1::Error), - /// Called finalize on an incomplete tree. - IncompleteTree, /// Called finalize on a empty tree. EmptyTree, } @@ -1036,9 +1039,6 @@ impl fmt::Display for TaprootBuilderError { TaprootBuilderError::InvalidInternalKey(ref e) => { write_err!(f, "invalid internal x-only key"; e) } - TaprootBuilderError::IncompleteTree => { - write!(f, "Called finalize on an incomplete tree") - } TaprootBuilderError::EmptyTree => { write!(f, "Called finalize on an empty tree") } @@ -1057,7 +1057,6 @@ impl std::error::Error for TaprootBuilderError { InvalidMerkleTreeDepth(_) | NodeNotInDfsOrder | OverCompleteTree - | IncompleteTree | EmptyTree => None } } From b3e0468e4d58eefa3cab426c0ce8eba5df2d7c60 Mon Sep 17 00:00:00 2001 From: ChallengeDev210 Date: Thu, 28 Jul 2022 17:38:26 -0700 Subject: [PATCH 3/4] Add breaking test that allowed incomplete builders to be created --- src/util/taproot.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/taproot.rs b/src/util/taproot.rs index 90f7c82..17d1ac9 100644 --- a/src/util/taproot.rs +++ b/src/util/taproot.rs @@ -1340,6 +1340,9 @@ mod test { let builder = builder.add_leaf(2, b.clone()).unwrap(); let builder = builder.add_leaf(2, c.clone()).unwrap(); let builder = builder.add_leaf(3, d.clone()).unwrap(); + + // Trying to finalize an incomplete tree returns the Err(builder) + let builder = builder.finalize(&secp, internal_key).unwrap_err(); let builder = builder.add_leaf(3, e.clone()).unwrap(); let tree_info = builder.finalize(&secp, internal_key).unwrap(); From 24e259242f96808ab85ecaa34cac454acfa6c3c6 Mon Sep 17 00:00:00 2001 From: ChallengeDev210 Date: Thu, 28 Jul 2022 17:33:26 -0700 Subject: [PATCH 4/4] Rename is_finalized to is_finalizable I really liked the is_complete naming, but that was changed earlier in b0f3992db16cbfb571160039f59e0c426404b997 Was also suggested by Andrew https://github.com/rust-bitcoin/rust-bitcoin/pull/929#discussion_r850631207 --- src/util/psbt/map/output.rs | 2 +- src/util/psbt/serialize.rs | 2 +- src/util/taproot.rs | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/psbt/map/output.rs b/src/util/psbt/map/output.rs index 74d27d5..2545c1e 100644 --- a/src/util/psbt/map/output.rs +++ b/src/util/psbt/map/output.rs @@ -194,7 +194,7 @@ impl TryFrom for TapTree { /// A [`TapTree`] iff the `builder` is complete, otherwise return [`IncompleteTapTree`] /// error with the content of incomplete `builder` instance. fn try_from(builder: TaprootBuilder) -> Result { - if !builder.is_finalized() { + if !builder.is_finalizable() { Err(IncompleteTapTree::NotFinalized(builder)) } else if builder.has_hidden_nodes() { Err(IncompleteTapTree::HiddenParts(builder)) diff --git a/src/util/psbt/serialize.rs b/src/util/psbt/serialize.rs index 441728a..c10b98f 100644 --- a/src/util/psbt/serialize.rs +++ b/src/util/psbt/serialize.rs @@ -343,7 +343,7 @@ impl Deserialize for TapTree { builder = builder.add_leaf_with_ver(*depth, script, leaf_version) .map_err(|_| encode::Error::ParseFailed("Tree not in DFS order"))?; } - if builder.is_finalized() && !builder.has_hidden_nodes() { + if builder.is_finalizable() && !builder.has_hidden_nodes() { Ok(TapTree(builder)) } else { Err(encode::Error::ParseFailed("Incomplete taproot Tree")) diff --git a/src/util/taproot.rs b/src/util/taproot.rs index 17d1ac9..8658093 100644 --- a/src/util/taproot.rs +++ b/src/util/taproot.rs @@ -441,7 +441,7 @@ impl TaprootBuilder { } /// Checks if the builder has finalized building a tree. - pub fn is_finalized(&self) -> bool { + pub fn is_finalizable(&self) -> bool { self.branch.len() == 1 && self.branch[0].is_some() } @@ -452,8 +452,8 @@ impl TaprootBuilder { /// Creates a [`TaprootSpendInfo`] with the given internal key. /// - /// Returns the unmodified builder as Err if the builder is not finalized. - /// See also [`TaprootBuilder::is_finalized`] + /// Returns the unmodified builder as Err if the builder is not finalizable. + /// See also [`TaprootBuilder::is_finalizable`] pub fn finalize( mut self, secp: &Secp256k1,