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.
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
Implement from arrays for TaprootMerkleBranch
#1613
Implement from arrays for TaprootMerkleBranch
#1613
Conversation
Oh, I meant |
Coo, will re-spin this PR. |
355e245
to
3d91ad5
Compare
Did so in #1621 |
From<ARRAY>
for TaprootMerkleBranch
for various array sizesTaprootMerkleBranch
Force push is re-write to use |
bitcoin/src/taproot.rs
Outdated
} | ||
} | ||
// Implement for all values [1, 128] inclusive. | ||
impl_try_from_array!(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't impl 0 but from_collection
allows it. So one of them is wrong, I don't have much time to figure out which one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialization from a zero sized array and initialization for an empty length are not exactly the same, right? Two thoughts:
- Seems like there is no reason to enable initialization from a zero sized array, if folks really want to do it they can use
Vec::new()
. - Currently we do not have an invariant that the inner vector is non-empty so initialization from an empty array is valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC Taproot rules disallow empty branch so we should have the invariant and should check it. But still didn't have time to double-check. In any case they should be consistent. I'm fine with disabling initialization from empty collections in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we should prevent it in from_collection
(and I checked, it is not currently prevented). Also agreed that this should be a separate commit, if not a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, added to my todo list. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3d91ad5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3d91ad5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remembered (and double checked) I use empty branch in real code and it works so it's clearly valid length.
The reason is TaprootMerkleBranch
doesn't contain the hash of the node that's being proven - it's not needed because the script is already right before control block.
Thus this needs to be implemented for 0 length.
The `TaprootMerkleBranch` contains a vector of `TapNodeHash`s, as such it can trivially be constructed from an array of the same type. Implement `From` for all array sizes 1 - 128 inclusive.
3d91ad5
to
118a593
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 118a593
Force pushed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 118a593
The
TaprootMerkleBranch
contains a vector ofTapNodeHash
s, as such it can trivially be constructed from an array of the same type.Implement
From
for all array sizes 1 - 128 inclusive.Fix: #1469