Commit 8ab549a
committed
Merge #895: miniscript: rewrite satisfier to be non-recursive
a662453 satisfy: add regression test for timelock tracking in or_i satisfaction (Andrew Poelstra)
c491ae5 miniscript/satisfy: rewrite satifier to be non-recursive (Andrew Poelstra)
ffbf415 miniscript/satisfy: move multi/multi_a into submodule (Andrew Poelstra)
3214b70 miniscript/satisfy: move the rest of the terminals into their own module (Andrew Poelstra)
fe0d153 miniscript/satisfy: move pk_k logic into helper function (Andrew Poelstra)
e372b12 miniscript/satisfy: use Miniscript instead of Terminal in API (Andrew Poelstra)
171bd33 rename miniscript/satisfy.rs to miniscript/satisfy/mod.rs (Andrew Poelstra)
Pull request description:
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:
1. Rust does not handle recursion well, and this may stack overflow.
2. 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.)
3. 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.
ACKs for top commit:
trevarj:
ACK a662453
apoelstra:
On a662453 successfully ran local tests
tcharding:
ACK a662453
Tree-SHA512: b017fa5774b9bb4fc1a2d985e40c0fb41bf9cb055a53dded12cb2392effab046dc727f7d0558b74a6034db1cd1dce45e995d6d00f1382def677a6c5b01c9e5f93 files changed
Lines changed: 615 additions & 661 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
464 | 464 | | |
465 | 465 | | |
466 | 466 | | |
467 | | - | |
| 467 | + | |
468 | 468 | | |
469 | 469 | | |
470 | 470 | | |
| |||
482 | 482 | | |
483 | 483 | | |
484 | 484 | | |
485 | | - | |
| 485 | + | |
486 | 486 | | |
487 | 487 | | |
488 | 488 | | |
| |||
511 | 511 | | |
512 | 512 | | |
513 | 513 | | |
514 | | - | |
| 514 | + | |
515 | 515 | | |
516 | 516 | | |
517 | 517 | | |
| |||
527 | 527 | | |
528 | 528 | | |
529 | 529 | | |
530 | | - | |
| 530 | + | |
531 | 531 | | |
532 | 532 | | |
533 | 533 | | |
| |||
0 commit comments