Taproot-related 0.28.0 RC2 fixes by dr-orlovsky · Pull Request #922 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2022-03-30T09:35:54Z

I m collecting here different fixes which are really useful downstream while I am playing with Taproot implementation in my projects.

Once I will have everything downstream working I will remove this PR draft status.

LeafInfo structure is a useful form of representing leaf script information (script, leaf version and merkle proof).

Previously used depth and script tuple missed information about the leaf version. All three comprises already existing type `LeafInfo` which was made public in previous commits.

sanket1729 · 2022-03-30T20:12:06Z

@dr-orlovsky, are any of these really really needed? I don't think there is anything that you cannot do downstream. This is not a bug in RC. These are clearly new features that we can add later. I am saying this because these are non-trivial changes to review. I disagree with some of them (make an API for first, into_hidden_hash) and it might take some time to resolve.

I know it might long for the next release, but as @tcharding said we can do another taproot fixes release soon after. This release does not need to perfect, but it needs to happen soon. If we don't release this before Friday, we will have to wait another 7-10 before the release.

sanket1729

I have concept NACK for a few commits here. Just went through the first six commits. It will take more time for me to review this carefully.
I think we should remove this from RC fix label, as clearly there is no bug being fixed here.

sanket1729 · 2022-03-30T19:58:30Z

src/util/psbt/map/output.rs

 impl TapTree {
+    /// Convenience method to retrieve number of script leafs in the tree.
+    #[inline]
+    pub fn scripts_count(&self) -> usize {


I think we should implement ExactSizeIterator instead of Iterator

sanket1729 · 2022-03-30T20:14:51Z

src/util/taproot.rs

-    pub(crate) ver: LeafVersion,
-    // The merkle proof(hashing partners) to get this node
-    pub(crate) merkle_branch: TaprootMerkleBranch,
+pub struct LeafInfo {


I am not sure this is easy to review. I agree that this might be good to have, but I will need to review whether allowing setting member fields from the user can break weird invariants in the builder.

What if the user creates Merkle proofs for two different leaves that are not a part of the same tree. In general, even tough making something pub is simple change, making it's members public requires new reasoning and re-reviewing all previous code.

Yes, better to provide an accesor methods.

sanket1729 · 2022-03-30T20:16:03Z

src/util/taproot.rs

-    // Compute a leaf hash for the given leaf
-    fn hash(&self) -> sha256::Hash {
+    /// Compute a leaf hash for the given leaf
+    pub fn hash(&self) -> sha256::Hash {


Earlier this was okay to return sha256::Hash because it was internal used now. Now this should return TapLeafHash

Fully agree

sanket1729 · 2022-03-30T20:37:34Z

src/util/taproot.rs


+impl TapBranchHash {
+    /// Computes branch hash given two hashes of the nodes underneath it.
+    pub fn from_node_hashes(a: sha256::Hash, b: sha256::Hash) -> TapBranchHash {


This is unsafe, we have a taproot hashjes type system(LeafHash/NodeHash) for a reason. This code can be mis-used. As such I don't see any reason why this code needs to be public.

There are three main hashes in taptree hash type system:

TapLeafHash, which is for the script leaf,

TapBranchHash, which is for the script branch,

sha256::Hash, when we do not known what it is (branch or a leaf, i.e. hidden nodes).

I am following that convention here. And of course, we can have a type alias for sha256::Hash, like TapNodeHash.

But I do not see how this breaks the existing type system in taproot module.

tcharding · 2022-03-30T21:26:31Z

In your mind @dr-orlovsky can we do a 0.28.0 released by Miami? (Probably Tuesday the 5th to give us one day to do a quick 0.28.1 if needed.)

dr-orlovsky · 2022-03-30T21:35:39Z

I have removed RC label. I am up for the release ASAP.

sanket1729 · 2022-03-30T21:39:29Z

I have removed RC label. I am up for the release ASAP.

Thanks a lot

dr-orlovsky · 2022-03-31T13:34:42Z

Spit into multiple PRs addressing comments from there: #924, #925, #926, #927

4cdff06 Add convenience method TapTree:to_builder (Dr Maxim Orlovsky) a12e7c7 Implement From<TapTree> for TaprootBuilder (Dr Maxim Orlovsky) 410412f Rename TapTree::from_builder (Dr Maxim Orlovsky) 2192737 Rename TapTree::into_builder (Dr Maxim Orlovsky) f9d8d0d Make TapTree::node_info public (Dr Maxim Orlovsky) Pull request description: These are trivial fixes from extracted from now closed #922 ACKs for top commit: Kixunil: ACK 4cdff06 sanket1729: ACK 4cdff06 apoelstra: ACK 4cdff06 Tree-SHA512: 6132e8c214edc6f199a5550309daf4ed5035f24f545c793d6396c393bd2f55940dc418af62aed9aff25c0c90b74ee384ace986f7201db4018c6fd52710006126

dr-orlovsky added 3 commits March 30, 2022 11:33

TapTree::script_count method

bf6f9ed

TapTree::as_inner method

af80265

Make TapTree::node_info public

7402a70

dr-orlovsky added the RC fix label Mar 30, 2022

dr-orlovsky added this to the 0.28.0 milestone Mar 30, 2022

dr-orlovsky added 9 commits March 30, 2022 11:54

Make taproot LeafInfo public

3096acb

LeafInfo structure is a useful form of representing leaf script information (script, leaf version and merkle proof).

TapTree::first_leaf accessor method

c846372

Fix taproot LeadInfo docs

b5cb390

Require taproot tree depth argument always to be u8

1a6c642

Trait TapNodeHash marking all forms of taptree nodes

9137003

Make TapTreeIterator use LeafInfo

7dec117

Previously used depth and script tuple missed information about the leaf version. All three comprises already existing type `LeafInfo` which was made public in previous commits.

Replace redundant TapTreeIter with core::slice::Iter

984133e

Abstract tap branch hash computing into a dedicated method

068f6be

Remove repeated tap branch hash computing logic

3e5cc1c

sanket1729 reviewed Mar 30, 2022

View reviewed changes

dr-orlovsky removed the RC fix label Mar 30, 2022

dr-orlovsky modified the milestones: 0.28.0, 0.29.0 Mar 30, 2022

dr-orlovsky closed this Mar 31, 2022

dr-orlovsky mentioned this pull request Mar 31, 2022

Trivial improvements for TapTree type #927

Merged

cryptoquick mentioned this pull request Apr 25, 2023

Bump dependencies BP-WG/bitcoin_foundation#18

Closed

Conversation

dr-orlovsky commented Mar 30, 2022

Uh oh!

sanket1729 commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 left a comment • 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!

sanket1729 Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 30, 2022

Uh oh!

dr-orlovsky commented Mar 30, 2022

Uh oh!

sanket1729 commented Mar 30, 2022

Uh oh!

dr-orlovsky commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sanket1729 commented Mar 30, 2022 •

edited

Loading

sanket1729 left a comment •

edited

Loading