Remove redundant code computing tap hashes#926
Remove redundant code computing tap hashes#926sanket1729 merged 2 commits intorust-bitcoin:masterfrom BP-WG:taproot/taphash
Conversation
|
|
||
| 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, I like having it be public. It seems like a generally useful thing to do.
There was a problem hiding this comment.
Should this parameter be TapBranchHash instead?
There was a problem hiding this comment.
I see that for leaves it is TapLeafHash and for nodes, it is TapBranchHash. Maybe not worth having it here
There was a problem hiding this comment.
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
- Add type alias
pub type TapNodeHash = sha256::Hashand use it in API - Use
impl Into<TapNodeHash>in arguments which are generic over specific hash type - Do
From<TapBranch|LeafHash> for TapNodeHashconvertors.
But even if we decide to do so, this should be a scope of separate (quite large) PR.
There was a problem hiding this comment.
Sorry to be nit-picky here. I would like this to be left and right instead of a and b.
| eng.input(&a); | ||
| }; | ||
| TapBranchHash::from_engine(eng) | ||
| } |
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
I like this, but I won't hold up the PR for it.
| let hash = TapBranchHash::from_node_hashes(a.hash, b.hash); | ||
| Ok(Self { | ||
| hash: sha256::Hash::from_engine(eng), | ||
| hash: sha256::Hash::from_inner(hash.into_inner()), |
There was a problem hiding this comment.
Is this a 'normal' thing to do, convert between the various sha256 hash types? Should be be providing conversion functions over in bitcoin_hashes?
There was a problem hiding this comment.
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.
sanket1729
left a comment
There was a problem hiding this comment.
sorta ACK 1b28375. Left one question, that I am unsure about
|
|
||
| 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 { |
There was a problem hiding this comment.
Should this parameter be TapBranchHash instead?
|
|
||
| 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 { |
There was a problem hiding this comment.
I see that for leaves it is TapLeafHash and for nodes, it is TapBranchHash. Maybe not worth having it here
No description provided.