expression: replace recursive Tree datatype with a non-recursive one
#780
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
apoelstra
force-pushed
the
2024-11--expression-4
branch
from
515974a to
a94ce8e
Compare
|
I don't understand this doc failure... it's claiming that the |
Our current expression API has a `to_null_threshold` method, which works reasonably well but requires the caller translate the returned threshold by accessing individual children. We will later want to change the Tree represntation to make individual child access inefficient. To do this, encapsulate the threshold construction into a new verify_threshold. This previously was not done because our messy error structures made it very difficult to manage. But our error structures are less messy so it's possible now, though a bit of a hack. (We map everything to the global Error structure. Later we will do pretty-much the same thing but with ParseError in place of Error, which will be more elegant.)
This removes the ability to randomly access children of tree nodes. You can get them in order using the `children` iterator, and get them recursively using the `TreeLike` iterator methods. (In the next commits we will specialize these a bit, providing a `pre_order_iter` and `rtl_post_order_iter` which let you efficiently skip over subtrees. We will need these to parse Taproot expression trees and they don't fit into the `TreeLike` trait, at least not efficiently.)
This significantly speeds up and simplifies tree parsing, at the cost of having a more complicated API (but we mostly addressed the API question in the previous commits). This completely eliminates recursion for the Tree data type, including in the Drop impl. Big diff but there are only two "real" changes -- expression/mod.rs is substantially rewritten of course since we replace the core datatype, and Tr::from_tree is substantially rewritten since doing so was the point of this change. The rest of the changes are mechanically changing the signature of expression::FromTree::from_tree everywhere.
apoelstra
force-pushed
the
2024-11--expression-4
branch
from
a94ce8e to
8235d5f
Compare
|
Gentle ping @sanket1729 |
sanket1729
previously approved these changes
Dec 11, 2024
There was a problem hiding this comment.
ACK 8235d5f
Sorry for the delay in reviewing this PR. I wanted to set aside a dedicated two-hour block to ensure a thorough review. The taproot code is much cleaner and the previous one.
|
I will need to tweak the fuzztest commit with a workaround for nix-community/crate2nix#373 Sorry for the delay -- it took me quite a while to track down that bug. |
This bumps the local Cargo.toml version to 13, which will be the next release (since we've made many breaking changes), and in the fuzz test adds an explicit dependency on miniscript 12 from crates.io, as `old_miniscript`. Adds a single fuzztest which attempt to parse descriptors with both master and 12, to make sure they're the same.
apoelstra
force-pushed
the
2024-11--expression-4
branch
from
8235d5f to
6c8c34b
Compare
heap-coder
added a commit
to heap-coder/rust-miniscript
that referenced
this pull request
Sep 27, 2025
… `Tree` datatype with a non-recursive one
6c8c34b347f9e3d531c9a0b771e6fa607a5c8e9b descriptor: add fuzz test comparing to published rust-miniscript 12 (Andrew Poelstra)
ebae0ef9b7ac6d36a35c1d3ed80cc561d96760dd miniscript: remove unused Result from extdata type_check (Andrew Poelstra)
d05a034376dfd081c766a5965e72c3ba8056d28e expression: rewrite Tree module to no longer use a recursive data type (Andrew Poelstra)
616b6b6cf2b55cf4fe9230ac07c57ab4c249b9cb expression: hide all Tree fields and encapsulate API (Andrew Poelstra)
4137a5953c2abedb1834ac6ab0103e022fd1e256 expression: encapsulate threshold parsing (Andrew Poelstra)
Pull request description:
Replaces the recursive `expression::Tree` datatype with a nonrecursive one. Exposes a limited API consisting of operations which can be done efficiently (mainly, in-order iteration) and which are necessary for parsing trees and converting them to other types.
As a side effect this simplifies/unifies some more code and provides better error messages, in particular for threshold parsing. But that isn't a focus of this PR and I haven't quantified the changes.
This is the last of the "expression" PRs. I have followups which go in two directions: (1) eliminating more recursion and recursive datatypes, and (2) improving the TapTree API, which I found I needed this new expression API to do cleanly.
Will post benchmarks once I have them.
ACKs for top commit:
sanket1729:
utACK 6c8c34b347f9e3d531c9a0b771e6fa607a5c8e9b
Tree-SHA512: ee64f1aa5fdc3917b6561713249fb04a39baa30c43090705cd899ec88bc828fc6227ae6ee91f58b22ba02dd4a2026d8240156382fb27cbfb3a904599d629974a
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces the recursive
expression::Treedatatype with a nonrecursive one. Exposes a limited API consisting of operations which can be done efficiently (mainly, in-order iteration) and which are necessary for parsing trees and converting them to other types.As a side effect this simplifies/unifies some more code and provides better error messages, in particular for threshold parsing. But that isn't a focus of this PR and I haven't quantified the changes.
This is the last of the "expression" PRs. I have followups which go in two directions: (1) eliminating more recursion and recursive datatypes, and (2) improving the TapTree API, which I found I needed this new expression API to do cleanly.
Will post benchmarks once I have them.