miniscript: rewrite satisfier to be non-recursive #895
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
This is a very large file with lots of different things in it and poor privacy boundaries. We want to break it up into modules. Start by creating a new directory.
I would like to make the satisfaction logic non-recursive. We have a post order iterator on Miniscript but not on Terminal. So we must use Miniscript in our API functions. (This also is more conceptually correct.)
For every fragment, we are going to create a short helper function which computes the satisfaction and dissatisfaction for that fragment. We create these together because in general satisfactions and dissatisfactions are defined in terms of each other: * Several satisfactions (e.g. `or_b`) require dissatisfactions to create * Satisfaction of `and_v` requires the satisfaction of its V child to create When we un-recursify these functions we will therefore need to compute both satisfactions and dissatisfactions together. (The result will be less efficient than the recursive version, since it will compute both sats and dissats for every fragment rather than computing just the data that's needed. But it should be easier to follow.) The next commit will do this for several more fragments. But I am doing it for pk_k in a commit by itself so you can see the process.
Probably this should just be squashed into the previous commit.
Again, candidate for squash.
Member
Author
|
This is ready to review but I will eventually add a commit which addresses the "FIXME find a test case" comment for the logic change in the last commit. |
Member
Author
|
Hrpmh. Apparently I am using a FromIterator impl from 1.79. |
This rewrites the main satisfy function, which I apologize for, and it's not even a simple code move because I combined satisfy_ and dissatisfy_helper, and had to change the code to pop from the stack rather than doing recursive calls. Furthermore, I suspect that this code will be slower in some cases, though unlikelily in a way that anybody would ever notice, because it computes both the satisfaction and dissatisfaction for every single node, not just the ones that are needed. (Note: it was always the case that, when satisfying, that a satisfaction was attempted for every single node, and that a dissatisfaction would be attempted for most nodes contained in any disjunction. So probably satisfaction is not meaningfully slowed, and importantly, we aren't making any more calls to the satisfier object, which might be an API to some slow USB device.) HOWEVER, the resulting code is much smaller and cleaner. I found a bug, I think (noted with a FIXME to add a test). I deleted over twice as much code as I added, and in particular eliminated tons of extremely noisy generics related to mall/non-mall function pointers. (The next commits will remove even more generics.)
apoelstra
force-pushed
the
2026-02/satisfy-non-recursive
branch
from
8780041 to
2b9c724
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Currently the
Miniscript::satisfyfunction is implemented by the functionSatisfaction::satisfy_helperwhich is mutually recursive with the functionSatisfaction::dissatisfy_helper. There are a few issues with this:So I rewrote the whole function. I haven't benchmarked but I suspect the new one is a little slower, though it is asymptotically the same. The slowness comes from using heap-based rather than stack-based recursion, and also because the old algorithm could sometimes skip computing dissatisfactions while this one never does. I think the increase in code clarity justifies the loss of performance (if there is one).
See the last commit message for more details.