expression: replace recursive Tree datatype with a non-recursive one
#780
Conversation
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.
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
a94ce8e to
8235d5f
Compare
|
Gentle ping @sanket1729 |
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.
| } | ||
|
|
||
| impl<'s, Pk: MiniscriptKey> TreeStack<'s, Pk> { | ||
| fn new() -> Self { Self { inner: Vec::with_capacity(128) } } |
There was a problem hiding this comment.
nit: Should capacity allocation be optimized to better match the expected average use case? For example, a tree with depth 4 might suffice for most scenarios. Allocating a fixed capacity of 128 for everything seems potentially excessive, especially if it goes unused in most cases.
There was a problem hiding this comment.
need not be addressed in this PR
There was a problem hiding this comment.
Yes, but we're talking about a savings of a kilobyte or so of heap space. We can maybe revisit this later as part of a "can we make this library work on constrained memory devices" project.
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.