Make NodeInfo API public #910
Conversation
|
It is a fairly small change, but don't want to add anything else to the already pending rust-bitcoin release. |
9549998 to
d840614
Compare
|
I think this is needed for this release |
There was a problem hiding this comment.
ACK d840614b1e71a8ad5f1c1702738003694eda610b
Regarding my nits, here and everywhere: I think the main challenge is to get ACKs, so all nits should be better addressed as a follow-up PR(s), if the author agrees with them - and not as a force-pushes discarding existing ACKs and taking reviewers time on non-important stuff once again.
src/util/taproot.rs
Outdated
| fn from_node_info<C: secp256k1::Verification>( | ||
| /// Compute [`TaprootSpendInfo`] from [`NodeInfo`], and internal key. | ||
| /// This is useful when you want to manually build a taproot tree wihtout | ||
| /// using [`TaprootBuilder`] |
There was a problem hiding this comment.
nit:
| /// using [`TaprootBuilder`] | |
| /// using [`TaprootBuilder`]. |
src/util/taproot.rs
Outdated
| /// Merkle Hash for this node | ||
| pub(crate) hash: sha256::Hash, | ||
| /// information about leaves inside this node | ||
| pub(crate) leaves: Vec<LeafInfo>, | ||
| } | ||
|
|
||
| impl NodeInfo { | ||
| // Create a new NodeInfo with omitted/hidden info | ||
| fn new_hidden(hash: sha256::Hash) -> Self { | ||
| /// Create a new [`NodeInfo`] with omitted/hidden info |
There was a problem hiding this comment.
nit
| /// Create a new [`NodeInfo`] with omitted/hidden info | |
| /// Creates a new [`NodeInfo`] with omitted/hidden info |
There was a problem hiding this comment.
nit: nit:
| /// Create a new [`NodeInfo`] with omitted/hidden info | |
| /// Creates a new [`NodeInfo`] with omitted/hidden info. |
src/util/taproot.rs
Outdated
| Self { | ||
| hash: hash, | ||
| leaves: vec![], | ||
| } | ||
| } | ||
|
|
||
| // Create a new leaf with NodeInfo | ||
| fn new_leaf_with_ver(script: Script, ver: LeafVersion) -> Self { | ||
| /// Create a new leaf [`NodeInfo`] with given [`Script`] and [`LeafVersion`] |
There was a problem hiding this comment.
nit
| /// Create a new leaf [`NodeInfo`] with given [`Script`] and [`LeafVersion`] | |
| /// Creates a new leaf [`NodeInfo`] with given [`Script`] and [`LeafVersion`] |
There was a problem hiding this comment.
nit: nit:
| /// Create a new leaf [`NodeInfo`] with given [`Script`] and [`LeafVersion`] | |
| /// Create a new leaf [`NodeInfo`] with given [`Script`] and [`LeafVersion`]. |
Phew, docs are hard to do meticulously :)
src/util/taproot.rs
Outdated
| let leaf = LeafInfo::new(script, ver); | ||
| Self { | ||
| hash: leaf.hash(), | ||
| leaves: vec![leaf], | ||
| } | ||
| } | ||
|
|
||
| // Combine two NodeInfo's to create a new parent | ||
| fn combine(a: Self, b: Self) -> Result<Self, TaprootBuilderError> { | ||
| /// Combine two [`NodeInfo`] to create a new parent |
There was a problem hiding this comment.
nit
| /// Combine two [`NodeInfo`] to create a new parent | |
| /// Combines two [`NodeInfo`] to create a new parent |
|
ACK |
|
This is good to have, but not mandatory! This can be emulated using the |
|
Well, it's quite trivial and requires just a single ACK more... |
I agree with you that all this re-acking is adding seemingly unnecessary work for reviewers. Idea: what if we pushed the burden onto the merger instead of the dev or reviewer? By that I mean, merger could decide that changes since ACK where trivial (e.g., docs, non-logic-changing refactor) and therefore the ACK is still valid? That way if reviewer gets to the PR and wants to re-ack, fine, if merger gets to the PR and sees nit-only fixes, then fine too. |
This is possible, but I think we should keep this to a bare minimum, comment fixes, variable renames etc. I don't think code move refactors fall here because that has a big diff. I would be okay in doing this if the diff is fairly small(< 10-15 lines) and no code changes. |
There was a problem hiding this comment.
The NodeInfo struct is now public but the fields are private and there are no getters/setters, are users able to use this struct as expected? I noticed that non of the util::taproot types are re-exported out of util, is that what we want? I am about to try and do the 'add taproot example' issue, perhaps I'll add a bunch of rustdoc examples to the taproot module to test the public API?
src/util/taproot.rs
Outdated
| /// Merkle Hash for this node | ||
| pub(crate) hash: sha256::Hash, | ||
| /// information about leaves inside this node | ||
| pub(crate) leaves: Vec<LeafInfo>, | ||
| } | ||
|
|
||
| impl NodeInfo { | ||
| // Create a new NodeInfo with omitted/hidden info | ||
| fn new_hidden(hash: sha256::Hash) -> Self { | ||
| /// Create a new [`NodeInfo`] with omitted/hidden info |
There was a problem hiding this comment.
nit: nit:
| /// Create a new [`NodeInfo`] with omitted/hidden info | |
| /// Creates a new [`NodeInfo`] with omitted/hidden info. |
src/util/taproot.rs
Outdated
| Self { | ||
| hash: hash, | ||
| leaves: vec![], | ||
| } | ||
| } | ||
|
|
||
| // Create a new leaf with NodeInfo | ||
| fn new_leaf_with_ver(script: Script, ver: LeafVersion) -> Self { | ||
| /// Create a new leaf [`NodeInfo`] with given [`Script`] and [`LeafVersion`] |
There was a problem hiding this comment.
nit: nit:
| /// Create a new leaf [`NodeInfo`] with given [`Script`] and [`LeafVersion`] | |
| /// Create a new leaf [`NodeInfo`] with given [`Script`] and [`LeafVersion`]. |
Phew, docs are hard to do meticulously :)
src/util/taproot.rs
Outdated
| let leaf = LeafInfo::new(script, ver); | ||
| Self { | ||
| hash: leaf.hash(), | ||
| leaves: vec![leaf], | ||
| } | ||
| } | ||
|
|
||
| // Combine two NodeInfo's to create a new parent | ||
| fn combine(a: Self, b: Self) -> Result<Self, TaprootBuilderError> { | ||
| /// Combine two [`NodeInfo`] to create a new parent |
This! @apoelstra do you think we can do this, would be very cool but also even more pressure not to get egg on our face :) I think we can handle it, what do you rekon? |
For example:
H_POINT+[
pk(A),
[
`1 OP_ADD 2 OP_EQUAL`,
0x51
]
]
Depends on rust-bitcoin/rust-bitcoin#910
|
Thanks! That's exactly what I wanted. I wasn't able to test directly with this PR branch because I'm also using rust-miniscript which isn't compatible with rust-bitcoin's master ( With this in place, minsc now allows to construct custom taproot trees like in the first example (previously only huffman trees were supported, like in the second). |
|
Legend @shesek, thank you. |
This allows users to create TaprootSpendInfo using NodeInfo. This offers an alternative to TaprootBuilder.
d840614 to
208eb65
Compare
I have only made the minimal things pub so that the above example in the PR description works. Thankfully, Shesek is able to confirm that this works.
I don't have super strong opinions about this. But I have a preference that these should be re-exported from util. If we have an established convention, I would be happy to adhere to it. So far, I see some things are re-exported out of
Yes, please. I will also make a point to add rustdoc examples for future APIs. They are really handy. |
|
Force pushed again addressing all the doc-comment nits. |
|
ACK all the docs stuff, I have more docs fixes for the |
|
@shesek I did a PR rust-bitcoin/rust-miniscript#324 that brings So, you can add this section to all of your projects: [patch.crates-io]
bitcoin = { git = "https://github.com/LNP-BP/rust-bitcoin", branch = "canary" }
miniscript = { git = "https://github.com/LNP-BP/rust-miniscript", branch = "canary" } |
For example:
H_POINT+[
pk(A),
[
`1 OP_ADD 2 OP_EQUAL`,
0x51
]
]
Depends on rust-bitcoin/rust-bitcoin#910
Reported by @shesek. Users might find it convenient to manually construct the tree using
NodeInfoAPI