miniscript: rewrite satisfier to be non-recursive by apoelstra · Pull Request #895 · rust-bitcoin/rust-miniscript

apoelstra · 2026-02-01T13:48:50Z

Currently the Miniscript::satisfy function is implemented by the function Satisfaction::satisfy_helper which is mutually recursive with the function Satisfaction::dissatisfy_helper. There are a few issues with this:

Rust does not handle recursion well, and this may stack overflow.
We need to fish several parameters (closures for computing min/thresh based on whether we allow malleability; the Taproot leaf hash which might be garbage for non-Taproot outputs; a boolean indicating whether we are hassig) through every single recursive call, which results in a lot of noise and makes the code very hard to maintain. (In particular, I want to fix the Taproot leaf hash thing, but without rewriting the satisfier, this requires changing several dozen call sites.)
The logic is very hard to follow and mixes somewhat-complicated recursion logic with actual satisfaction logic, which might be wrong (there is one logic change I made related to timelocks; I have a FIXME to find a test case for this because I am pretty sure the old logic was wrong.)

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.

apoelstra · 2026-02-01T13:49:15Z

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.

apoelstra · 2026-02-01T13:57:53Z

Hrpmh. Apparently I am using a FromIterator impl from 1.79.

apoelstra

On 2b9c724 successfully ran local tests

trevarj · 2026-03-21T07:58:25Z

+            Self::thresh
+        };
+
+        let mut stack = vec![];


The slowness comes from using heap-based rather than stack-based recursion

Maybe a simple with_capacity(SWEET_SPOT) could give a quick improvement if this is an issue, but I'm not sure.

Yeah, not a bad idea. I added with_capacity(128). As I said above, I haven't benchmarked this (it's a little tricky/annoying to benchmark satisfaction in a realistic way).

apoelstra · 2026-04-13T18:48:02Z

Rebased after #915

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.

apoelstra · 2026-04-13T21:46:21Z

Rebased after #920 and #921.

trevarj · 2026-04-14T04:31:16Z

Can't provide much for a review. Just admiring the cleanup!

tcharding · 2026-04-21T05:15:33Z

ACK 14a79f2

I can't help with the last patch because its over my head. The first 5 look good.

tcharding · 2026-04-21T22:51:44Z

How do you want to move forward with this @apoelstra? Wait for Sanket or want me and/or @trevarj to dig into something in particular that would give you confidence in our review? (Assuming Trevar wants to do that.)

apoelstra · 2026-04-22T14:09:13Z

Hi @tcharding. I reached out to @sanket1729 out of band and he said it's okay if we push forward on this without him.

I would appreciate a cursory review from you and/or @trevarj. You don't need to verify correctness of the rewrite. Hopefully the tests are thorough enough to catch any mistakes. Miniscript 12 is the "reference version" and we have pretty thorough fuzztests against it, although I don't think we're fuzzing the satisfier.

In parallel to this project we should try to improve those fuzztests -- in particular, finding a way to improve the satisfier. I think we want to have a "fuzz library" similar to what exists in rust-simplicity, which is able to create fuzzer-guided complete transaction chains.

trevarj

Did another review and tried to study sat_dissat.rs harder. Few doc nits found.

trevarj · 2026-04-22T06:12:51Z

+        }
+    }
+
+    /// The (dissatisfaction, satisfaction) pair for a `multi` fragment.


Suggested change

/// The (dissatisfaction, satisfaction) pair for a `multi` fragment.

/// The (dissatisfaction, satisfaction) pair for a `multi_a` fragment.

Can you indicate which commit you are talking about? Github provides no hint except 4 lines of diff.

Appears to be 14a79f2

Yes, sorry, I did this review on c31154d but it seems that it was introduced in 14a79f2

trevarj · 2026-04-22T06:13:18Z

+        )
+    }
+
+    /// The (dissatisfaction, satisfaction) pair for a `ripemd256` fragment.


Suggested change

/// The (dissatisfaction, satisfaction) pair for a `ripemd256` fragment.

/// The (dissatisfaction, satisfaction) pair for a `sha256` fragment.

trevarj · 2026-04-22T06:13:38Z

+        )
+    }
+
+    /// The (dissatisfaction, satisfaction) pair for a `ripemd256` fragment.


Suggested change

/// The (dissatisfaction, satisfaction) pair for a `ripemd256` fragment.

/// The (dissatisfaction, satisfaction) pair for a `hash256` fragment.

trevarj · 2026-04-22T14:12:12Z

+                    // FIXME existing code sets timelocks to None instead of propagating them,
+                    // in the dissat case. This dates to the original plan module and was not
+                    // comment on. https://github.com/rust-bitcoin/rust-miniscript/pull/592
+                    // Was likely a mistake. Need to find a test case that distinguishes before


Will we follow up on this test?

Staring at this for a long time wondering which dissatisfaction actually has a timelock that isn't IMPOSSIBLE.

Let me follow up on this. Now that we have good LLMs I should be able to gin up a test case.

Staring at this for a long time wondering which dissatisfaction actually has a timelock that isn't IMPOSSIBLE.

For example, to dissatisfy an and_v(<timelock>, <something else>) you need to satisfy the timelock but dissatisfy the <something else>.

This may be the only example.

I have constructed an example. Amusingly, while the old code would produce an invalid dissatisfaction, the new code produces no dissatisfaction at all, which is arguably no better. But fixing that is nontrivial and out of scope for this PR.

I will create a test case with a detailed comment analyzing this situation and add it to this PR.

the new code produces no dissatisfaction at all,

Actually I think it produces no dissatisfaction at all because of an unrelated bug (the comment "Dissatisfactions don't need to non-malleable." is false). Probably I need to fix that separately and backport it.

Ah, the "unrelated bug" is a combination of #734 which is fine to just leave alone for now, and a misunderstanding I had about malleability of dissatisfactions.

Again, candidate for squash.

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.)

Prior to this PR, when we compute the dissatisfaction for an or_i (which requires dissatisfying one of the child nodes), we would discard the timelock information for the child's dissatisfaction. This is wrong and can lead to defective plans, though apparently only in pathological cases. Add a regression test for this. Incidentally, this also provides a regression test for the "malleability for dissatisfactions doesn't matter" logic in the or_i branch of dissatisfy; previously you could replace Self::minimum_mall with min_fn and nothing would break.

apoelstra · 2026-04-22T18:25:53Z

@trevarj

addressed your copy/paste errors in comments
replaced a call to min_fn with Self::minimum_mall to match the old code; replaced the FIXME comment with a detailed comment justifying the use of minimum_mall
added a commit with a regression test that covers both the FIXME and the new minimum_mall thing

This should be good to go now.

trevarj

ACK a662453

apoelstra · 2026-04-23T02:19:51Z

On a662453 successfully ran local tests

tcharding · 2026-04-23T06:13:27Z

The module is called sat_dissat but the function is dissat_sat and returns (dissat, sat). Can we have them the same? And here there are around the other way

            // Satisfaction is more complicated.
            sat_dissat_vec.push((sub.sat_data, sub.dissat_data));

We could do a separate PR to change extra_props to use (dissait, sat) before or after this?

tcharding

ACK a662453

apoelstra · 2026-04-23T13:20:39Z

@tcharding sure, in my next PR I'll add a commit that swaps them.

Probably I should just make a utility struct for these. When writing and reviewing this code I repeatedly got the sat and dissat branches switched in my head.

apoelstra force-pushed the 2026-02/satisfy-non-recursive branch from 8780041 to 2b9c724 Compare February 1, 2026 13:59

apoelstra commented Feb 2, 2026

View reviewed changes

apoelstra mentioned this pull request Feb 2, 2026

Tap context: restore d: as unit under MINIMALIF? #894

Open

trevarj reviewed Mar 21, 2026

View reviewed changes

apoelstra mentioned this pull request Apr 13, 2026

refactor: remove SortedMultiVec and use Terminal::SortedMulti #915

Merged

apoelstra force-pushed the 2026-02/satisfy-non-recursive branch 2 times, most recently from a36dae0 to f9c911d Compare April 13, 2026 18:47

apoelstra force-pushed the 2026-02/satisfy-non-recursive branch from f9c911d to a609aec Compare April 13, 2026 19:30

apoelstra added 4 commits April 13, 2026 21:45

rename miniscript/satisfy.rs to miniscript/satisfy/mod.rs

171bd33

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.

miniscript/satisfy: use Miniscript instead of Terminal in API

e372b12

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.)

miniscript/satisfy: move the rest of the terminals into their own module

3214b70

Probably this should just be squashed into the previous commit.

apoelstra force-pushed the 2026-02/satisfy-non-recursive branch from a609aec to c31154d Compare April 13, 2026 21:45

apoelstra mentioned this pull request Apr 15, 2026

Prepare beta version #922

Closed

apoelstra mentioned this pull request Apr 21, 2026

Bitcoin 0.33 migration #924

Closed

trevarj reviewed Apr 22, 2026

View reviewed changes

apoelstra added 3 commits April 22, 2026 18:24

miniscript/satisfy: move multi/multi_a into submodule

ffbf415

Again, candidate for squash.

apoelstra force-pushed the 2026-02/satisfy-non-recursive branch from c31154d to a662453 Compare April 22, 2026 18:24

trevarj approved these changes Apr 22, 2026

View reviewed changes

tcharding approved these changes Apr 23, 2026

View reviewed changes

apoelstra merged commit 8ab549a into rust-bitcoin:master Apr 23, 2026
26 checks passed

apoelstra deleted the 2026-02/satisfy-non-recursive branch April 23, 2026 13:21

	/// The (dissatisfaction, satisfaction) pair for a `multi` fragment.
	/// The (dissatisfaction, satisfaction) pair for a `multi_a` fragment.

Conversation

apoelstra commented Feb 1, 2026

Uh oh!

apoelstra commented Feb 1, 2026

Uh oh!

apoelstra commented Feb 1, 2026

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 13, 2026

Uh oh!

apoelstra commented Apr 13, 2026

Uh oh!

trevarj commented Apr 14, 2026

Uh oh!

tcharding commented Apr 21, 2026

Uh oh!

tcharding commented Apr 21, 2026

Uh oh!

apoelstra commented Apr 22, 2026

Uh oh!

trevarj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 22, 2026

Uh oh!

trevarj left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 23, 2026

Uh oh!

tcharding commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Apr 23, 2026 •

edited

Loading