TapTree iterator #901
Conversation
sanket1729
left a comment
There was a problem hiding this comment.
ACK, but CI is failing on 1.29. This is fairly small and hopefully non-controversial. We can sneak it in as @dr-orlovsky really needs it.
It might sense moving all the TapTree struct and methods inside the taproot module, this is not only related to psbt now. I don't want to scope this further, we can do it next breaking release.
We just need to add a |
|
@sanket1729 I'll do a separate PR once this got merged to move @ALL: CI is fixed |
There was a problem hiding this comment.
utACK e27f8ff.
Follow up notes (post release):
- Move
TapTreetoutil/taproot - Use the .node_info() method in the
psbt::Serializeimpl and in the iterator to removeunreachable!from the code here.
Would be great if you do that in this PR itself, but I would like to prioritize getting this in first.
tcharding
left a comment
There was a problem hiding this comment.
I don't have depth of knowledge in this area, please take my review as shallow only.
| /// Returns iterator for a taproot script tree, operating in DFS order over leaf depth and | ||
| /// leaf script pairs. | ||
| pub fn iter(&self) -> TapTreeIter { | ||
| self.into_iter() |
There was a problem hiding this comment.
Are we happy that this complies with Rust convention that iter should return an iterator that iterates over &T? I spent some time reading the docs for IntoIterator and I can't work out if the tuple (u8, &'tree Script) is in line with convention or not. Should it by (&u8, &'tree Script)?
There was a problem hiding this comment.
It can't be &u8, since u8 is not owned by the TapTree structure and is just retured from len() method which instantiate the value. You technically can't return a reference to it.
|
Would it also make sense to add an API for getting this info out of a TaprootSpendInfo? |
|
AFAIR |
If you can find a compelling use-case, sure. Technically speaking, this information is The implementation here might be complicated as it involves inferring a Tree from a BTreeMap of |
Implemented after @sanket1729 suggestion in #895 (comment)
Iterates all scripts present in TapTree in DFS order returning
(depth, script)pairs.I propose to have it as an RC fix since this functionality is really lacking and may be required for many wallets working with Taproot PSBT even outside of the scope where I originally needed it (OP_RETURN tweaks for TapTree described in #895)