Fail satisfy when resource limits exceed #189
Merged
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
sanket1729
force-pushed
the
satisfy_fail
branch
from
fbb351d to
dd3d78a
Compare
Closed
sanket1729
force-pushed
the
satisfy_fail
branch
2 times, most recently
from
ef367e3 to
8776e7b
Compare
apoelstra
reviewed
Nov 23, 2020
| honggfuzz = { version = "0.5", optional = true } | ||
| afl = { version = "0.8", optional = true } | ||
| regex = { version = "1.4"} | ||
| miniscript = { path = "..", features = ["fuzztarget", "compiler"] } |
Member
There was a problem hiding this comment.
Why are we changing this? The fuzzer won't be able to come up with hash preimages or signatures without the fuzztarget on.
Member
Author
There was a problem hiding this comment.
What does the fuzztarget feature do in rust-bitcoin? I think the only use we have in rust-miniscript is pass through for rust-bitcoin.
Member
Author
There was a problem hiding this comment.
I think I disabled them because I was not able to come up with secp Signature required for satisfier.
sanket1729
force-pushed
the
satisfy_fail
branch
from
8776e7b to
dbc27ff
Compare
sanket1729
force-pushed
the
satisfy_fail
branch
from
dbc27ff to
70b987f
Compare
apoelstra
approved these changes
Nov 23, 2020
darosior
reviewed
Nov 24, 2020
Comment on lines
+324
to
+325
| if witness_size(witness) > MAX_STANDARD_P2WSH_SCRIPT_SIZE { | ||
| return Err(ScriptContextError::MaxScriptSigSizeExceeded); |
Contributor
There was a problem hiding this comment.
Hmm shouldn't it return a MaxWitnessSizeExceeded Error instead ?
apoelstra
added a commit
to apoelstra/rust-miniscript
that referenced
this pull request
Oct 20, 2025
This is a weird function. Its role is to double-check a witness after it has been produced to ensure that it falls within limits. If you are working with a sane miniscript this is impossible to fail, and if you're working with an insane miniscript then the checks are too strong -- it verifies standardness rules rather than consensus. This was introduced in rust-bitcoin#189 without much discussion. It had no tests then and has no tests now -- everything continues to pass. I am removing it for now. I will later replace it with an API where you call Plan::validate if you want to do checks like this. If the user wants to do something oddball like "parse a miniscript without checking satisfaction limits, then produce a satisfaction and see if the result is nonetheless within limits" the workflow will be: 1. Parse an insane Miniscript 2. Produce a Plan 3. Run Plan::validate with tighter validation params than were used in step 1. This gives the user a fair bit more flexibility. It is an awkward and hard to discover workflow, but IMO it's better than we have now.
apoelstra
added a commit
to apoelstra/rust-miniscript
that referenced
this pull request
Oct 26, 2025
This is a weird function. Its role is to double-check a witness after it has been produced to ensure that it falls within limits. If you are working with a sane miniscript this is impossible to fail, and if you're working with an insane miniscript then the checks are too strong -- it verifies standardness rules rather than consensus. This was introduced in rust-bitcoin#189 without much discussion. It had no tests then and has no tests now -- everything continues to pass. I am removing it for now. I will later replace it with an API where you call Plan::validate if you want to do checks like this. If the user wants to do something oddball like "parse a miniscript without checking satisfaction limits, then produce a satisfaction and see if the result is nonetheless within limits" the workflow will be: 1. Parse an insane Miniscript 2. Produce a Plan 3. Run Plan::validate with tighter validation params than were used in step 1. This gives the user a fair bit more flexibility. It is an awkward and hard to discover workflow, but IMO it's better than we have now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Satisfy can fail if the resource limits are exceeded