8000 Remove redundant code computing tap hashes by dr-orlovsky · Pull Request #926 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove redundant code computing tap hashes #926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 1, 2022
Merged

Remove redundant code computing tap hashes #926

merged 2 commits into from
Apr 1, 2022

Conversation

dr-orlovsky
Copy link
Collaborator

No description provided.

@dr-orlovsky dr-orlovsky added minor API Change This PR should get a minor version bump code quality Makes code easier to understand and less likely to lead to problems labels Mar 31, 2022
@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Mar 31, 2022
dr-orlovsky
dr-orlovsky commented Mar 31, 2022
View reviewed changes
src/util/taproot.rs
@@ -131,6 +131,21 @@ impl TapLeafHash {
}
}

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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referring to the previous discussion on whether this function should be public: https://github.com/rust-bitcoin/rust-bitcoin/pull/922/files#r838943645

I am using this function downstream in OP_RETURN taproot commitment code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like having it be public. It seems like a generally useful thing to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this parameter be TapBranchHash instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that for leaves it is TapLeafHash and for nodes, it is TapBranchHash. Maybe not worth having it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are three hash types involved in taproot trees: TapBranchHash, TapLeafHash and sha256::Hash for hidden branches/leaves or for parameters generic over branch/leaves. To solve the abiguity we have the following options

  1. Add type alias pub type TapNodeHash = sha256::Hash and use it in API
  2. Use impl Into<TapNodeHash> in arguments which are generic over specific hash type
  3. Do From<TapBranch|LeafHash> for TapNodeHash convertors.

But even if we decide to do so, this should be a scope of separate (quite large) PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be nit-picky here. I would like this to be left and right instead of a and b.

@dr-orlovsky dr-orlovsky mentioned this pull request Mar 31, 2022
Closed
@tcharding tcharding changed the title Remove redundand code computing tap hashes Remove redundant code computing tap hashes Mar 31, 2022
tcharding
tcharding reviewed Mar 31, 2022
View reviewed changes
src/util/taproot.rs
eng.input(&a);
};
TapBranchHash::from_engine(eng)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps:

        let (min, max) = if a < b { (a, b) } else { (b, a) };

        let mut eng = TapBranchHash::engine();

        eng.input(&min);
        eng.input(&max);

        TapBranchHash::from_engine(eng)
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, but I won't hold up the PR for it.

src/util/taproot.rs
Ok(Self {
hash: sha256::Hash::from_engine(eng),
hash: sha256::Hash::from_inner(hash.into_inner()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a 'normal' thing to do, convert between the various sha256 hash types? Should be be providing conversion functions over in bitcoin_hashes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably provide a conveience method for "wrapped hashes" to their generic form (and vice-versa), yeah.

Sometimes hash conversions reflect perversions in Bitcoin, and these should appear perversely in our code :) but I don't think this is one of them.

In any case, for purposes of this PR the explicit conversion is fine.

apoelstra
apoelstra approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f3ebfd6

sanket1729
sanket1729 approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member
@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorta ACK 1b28375. Left one question, that I am unsure about

src/util/taproot.rs
@@ -131,6 +131,21 @@ impl TapLeafHash {
}
}

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this parameter be TapBranchHash instead?

src/util/taproot.rs
@@ -131,6 +131,21 @@ impl TapLeafHash {
}
}

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that for leaves it is TapLeafHash and for nodes, it is TapBranchHash. Maybe not worth having it here

@sanket1729 sanket1729 merged commit 7fa8ce0 into rust-bitcoin:master Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanket1729 sanket1729 sanket1729 approved these changes

@tcharding tcharding tcharding left review comments

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
code quality Makes code easier to understand and less likely to lead to problems minor API Change This PR should get a minor version bump
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dr-orlovsky @apoelstra @sanket1729 @tcharding
0