Eliminate more recursion, and Liftable impl for Terminal #725
Conversation
This file contains 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
| @@ -1408,6 +1408,39 @@ mod tests { | |||
| ms.translate_pk(&mut t).unwrap_err(); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn mixed_timelocks() { | |||
There was a problem hiding this comment.
I am surprised that this did not exist before. Anyways more tests are always welcome.
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).
|
Thanks for adding the tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.