Unify the two satisfaction algorithms#102
Merged
Merged
Conversation
sipa
force-pushed
the
202202_unify_sat
branch
from
cb6378d to
eeff02b
Compare
darosior
reviewed
Feb 21, 2022
Contributor
darosior
left a comment
darosior
left a comment
There was a problem hiding this comment.
That's great, it clarifies the API and simplifies the code. I don't think we should optimize for the malleable satisfaction so IMO the tradeoff is totally worth it.
A couple of comments on the tests. I'm going to test this after having rebased the Bitcoin Core satisfaction PR.
Contributor
|
Had to rebase on top of #101, so if you need to rebase: https://github.com/darosior/miniscript/tree/202202_unify_sat |
sipa
force-pushed
the
202202_unify_sat
branch
from
eeff02b to
59d9cb6
Compare
Owner
Author
|
Rebased on merged #101, and addressed comments. |
sipa
force-pushed
the
202202_unify_sat
branch
from
59d9cb6 to
b1a061e
Compare
darosior
reviewed
Feb 22, 2022
Contributor
darosior
left a comment
darosior
left a comment
There was a problem hiding this comment.
The unit test doesn't compile but passes once compiled. LGTM otherwise. The extended fuzz target also passes on my corpus (running it in background today).
sipa
force-pushed
the
202202_unify_sat
branch
from
b1a061e to
6fbd6e4
Compare
darosior
approved these changes
Feb 22, 2022
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 changes the malleable satisfaction algorithm to produce the same result as the non-malleable one, in case a non-malleable satisfaction exists. Further, any non-malleable result gains the guarantee that it is valid assuming a new ValidSatisfaction(), which is weaker than IsSane(), and crucially, doesn't require IsNonMalleable().
Lots of code simplifications follow.
The downside is that this makes the malleable satisfaction algorithm less aggressively pursue minimal witness size (because it's internally using the same priorities as the non-malleable one). On the other hand, it simplifies the code, and makes it more testable (as demonstrated by the few incorrect noncanonical/malleable annotations that were necessary to make this work).
I have some ideas about how to bring back an actual don't-care-about-malleability satisfier in a more testable way, but that can wait until after integration in Bitcoin Core.