miniscript: add an unit test for substitute_raw_pkh() #729
Conversation
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.
|
@pythcoiner, I believe #725 already solves this. Can you try building your test on top of #725? |
|
We should still keep this PR with the test. The test is valuable and thanks for your contribution and bug report. |
pythcoiner
force-pushed
the
substitute_raw_pkh
branch
from
0ffd43d
to
25f94f0
Compare
sure! |
pythcoiner
force-pushed
the
substitute_raw_pkh
branch
from
25f94f0
to
6ebf973
Compare
pythcoiner
changed the title
substitute_raw_pkh() recursive
|
rebased on top of #725 |
|
Running CI -- but probably going to close this because #725 already includes a fixed unit test for this function. Though if CI fails here, that would be really interesting. Thanks for noticing this and for writing the test though! |
|
Ok, CI is failing due to a new lint and a lack of pinning in this crate. That's not so interesting :). The new unit test passes, which is what we expected. |
|
#725 is in which should fix the problem. @pythcoiner would you like me to backport the fix to the latest-released 12.x? We aren't planning a new major release for a little while. |
up to you, i'm actually using my fork |
|
Given the choice, I'd rather not backport :). This function has been totally broken for years, and you are the first person to file a bug about it (curiously, only a few days after I stumbled into it myself, just reading the code). |
miniscript: add an unit test for substitute_raw_pkh()