Eliminate more recursion, and Liftable impl for Terminal #725
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
There were accidental newlines in some error messages.
We will want to start rejecting duplicate keys in policy. Currently they are rejected in the compiler and when parsing (sane) Miniscripts and when calling `is_valid` on concrete policies, but NOT when lifting insane Miniscripts or when parsing concrete or semantic policies. Meanwhile, mixing timelocks is checked in all the above places EXCEPT when parsing concrete or semantic policies. And of course, neither is checked when directly constructing Miniscripts or policies. It's all very inconsistent. My eventual goal is to use the same set of "sane" checks everywhere. To do this, I will need to embed a set of checks into Miniscript objects, and then later do the same with Policy objects (which will need to stop being "bare recursive structs" and start having more structure, same as Miniscript).
…ebase We have several algorithms throughout the codebase which "translate" a recursive object by running a post-order iterator over it, building a modified copy node-by-node. We frequently do this by iterating over an Arc structure, pushing each created node onto a stack, and then using the `child_indices` member of the `PostOrderIterItem` struct to index into the stack. We copy elements out of the stack using Arc::clone and then push a new element. The result is that for an object with N nodes, we construct a stack with N elements, call Arc::clone N-1 times, and often we need to bend over backward to turn &self into an Arc<Self> before starting. There is a much more efficient way: with a post-order iterator, each node appears directly after its children. So we can just pop children off of the stack, construct the new node, and push that onto the stack. As long as we always pop all of the children, our stack will never grow beyond the depth of the object in question, and we can avoid some Arc::clones. Using a right-to-left iterator means that we can call .pop() in the "natural" way rather than having to muck about reordering the children. This commit converts every single use of post_order_iter in the library to use this new algorithm. In the case of Miniscript::substitute_raw_pkh, the old algorithm was actually completely wrong. The next commit adds a unit test.
…minal It doesn't really make conceptual sense that Terminal would be Liftable. Terminal itself has no meaning; it isn't even typechecked. The recursive algorithm also repeats a lot of checks unnecessarily. Better to just call lift_check() once at the start of a Miniscript lift.
sanket1729
approved these changes
Aug 26, 2024
| } | ||
|
|
||
| #[test] | ||
| fn mixed_timelocks() { |
Member
There was a problem hiding this comment.
I am surprised that this did not exist before. Anyways more tests are always welcome.
Member
Author
There was a problem hiding this comment.
Possibly one existed somewhere. I grepped for the timelock-specific errors and found nothing. But there are a ton of tests that just assert .unwrap_err() and don't check more specifically.
And I'm pretty confident that there is no test that covers all three separate checks (in Miniscript, Semantic and Concrete).
Member
|
Thanks for adding the tests. |
heap-coder
added a commit
to heap-coder/rust-miniscript
that referenced
this pull request
Sep 27, 2025
… Liftable impl for Terminal
8e2d90c50105d03fdc3e52c18d99c488886aa410 policy: make lifting algorithm non-recursive, remove Liftable for Terminal (Andrew Poelstra)
44e05502cd23955c537e0f0e0cad9d28e0e24086 miniscript: add unit test for substitute_raw_pkh (Andrew Poelstra)
e31d52b2fbe9e1b3826a67f6785afebe851d2265 iter: introduce right-to-left post-order iterator, use throughout codebase (Andrew Poelstra)
5da250a708c22310eccfab84863ad72732286357 miniscript: add unit test for repeated pubkeys (Andrew Poelstra)
7bb9b04c41b929a85adad4e59216f7ec1ef5577d miniscript: add unit test for timelock mixing (Andrew Poelstra)
ff0509fcfa2d58007bd0691b791a760baeea10aa policy: stop using duplicate keys in unit tests (Andrew Poelstra)
1b49a3940b329e5e43d49fb534dc6393afad26f3 types: clean up some error messages (Andrew Poelstra)
Pull request description:
This PR does a few things which are mostly related:
* Adds unit tests and does a couple cleanups.
* Introduces a new right-to-left post-order iterator and uses that to eliminate a bunch of Arc::clones and extra memory usage from existing non-recursive algorithms
* Fixes a bug in `Miniscript::substitute_raw_pkh` where the function completely didn't work (this fell out of the right-to-left change)
* Converts `Miniscript::lift` to be non-recursive
* Eleminates `Terminal::lift` because this impl is conceptually confused (`Terminal` as a type should never be used except to construct `Miniscript`s; they are not typechecked or sanity-checked and have no capacity to store type information). It was originally implemented as a convenience to help the `Miniscript` implementation but we don't need it now
It also stops using duplicate keys in a couple unit tests. Right now our "sanity checking" logic for things like duplicate keys is super ad-hoc and inconsistent. For example, the duplicate-key check is implemented three times (in miniscript::analyzable, policy::semantic and policy::concrete) and the check is actually done in somewhat-random places. This PR doesn't change that, but since I was touching unit tests I figured I'd clean it up.
ACKs for top commit:
sanket1729:
ACK 8e2d90c50105d03fdc3e52c18d99c488886aa410
Tree-SHA512: 55edb0b8ac055215d4f733b663a8b5279aa53d360f17bf9034be00f9d30f0329eaa58faae34446c4f739241ee53b7b4d36127f4cc2dacf7c9e49a57feab292f0
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.
This PR does a few things which are mostly related:
Miniscript::substitute_raw_pkhwhere the function completely didn't work (this fell out of the right-to-left change)Miniscript::liftto be non-recursiveTerminal::liftbecause this impl is conceptually confused (Terminalas a type should never be used except to constructMiniscripts; they are not typechecked or sanity-checked and have no capacity to store type information). It was originally implemented as a convenience to help theMiniscriptimplementation but we don't need it nowIt also stops using duplicate keys in a couple unit tests. Right now our "sanity checking" logic for things like duplicate keys is super ad-hoc and inconsistent. For example, the duplicate-key check is implemented three times (in miniscript::analyzable, policy::semantic and policy::concrete) and the check is actually done in somewhat-random places. This PR doesn't change that, but since I was touching unit tests I figured I'd clean it up.