Various cleanups, mostly around the compiler #828
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
|
Need to investigate why jj is not signing these commits. |
apoelstra
force-pushed
the
2025-05-compiler
branch
from
1959461 to
e228232
Compare
|
I had authored them with the wrong email address set. |
This was an oversight in rust-bitcoin#815.
Generally we try not to add special-case logic for weird tools, but this is a a really simple change and I think that jujutsu is going to become pretty popular among rust-bitcoin developers. This is some repeated code which looks like it should be factored out into a utility function, but we can't really do that because we need to figure out the workspace root before we can even locate utility functions.
apoelstra
force-pushed
the
2025-05-compiler
branch
from
e228232 to
bae6bf5
Compare
tcharding
approved these changes
Jun 20, 2025
|
Code review only, my ack has no weight in regards to semantic correctness of the changes. |
apoelstra
force-pushed
the
2025-05-compiler
branch
from
bae6bf5 to
5dff64e
Compare
We can cause panics in the compiler comparing OrdF64s which are NaN, which we can produce by parsing ors where every child has weight 0. Simply make it a parse error to have any zero-probability branches. In theory somebody might want an or where some fragments have positive probability and others are 0, indicating "maximally pessimize these". But it has never occurred to me to do this, and it's easy to simulate this by using 100s and 1s, say.
In the compiler we have a function which inserts the "cast closure" of an expression. It accepts the expression as a candidate as well as a large list of cast-wrapped variants of the expression. For each candidate it rejects it if it is consensus-invalid or has malleable satisfactions. In the original compiler (PR rust-bitcoin#29) it would decide whether an expression was malleable by checking ty.mall.non_malleable on the miniscript. It had the confusing comment "return malleable types directly" which I believe was supposed to mean "return early if the node is malleable". The comment also observes that "If a elem is malleable, all the casts to it are also going to be malleable" but the code doesn't actually use this fact anywhere. It always tries all the casts. But ok, whatever. Later in rust-bitcoin#97 (add context to Miniscript) we weakened the malleability check in a bizarre way -- now it checks that the node is malleable according to the type system AND that it is non-malleable according to the context system. That is, we whitelist or_i and d: as "acceptable malleability" in a legacy/bare context. This change, which was not commented on in the original PR, seems like it's just totally wrong. I suspect it was supposed to be an or: if the node is malleable according to the type system OR according to the contextual checks, then skip it. But I'm unsure. (The check is also wrong because the contextual check is wrong; see the last commit.) Anyway, after some fairly heavy fuzzing I was unable to find any instance where this check affects compiler output. I suspect that it's merely an optimization. So change it back to the stronger, simpler check and update the comment.
These can't be constfs but they're still useful.
This eliminates some expect()s and is plausibly faster.
…lerError The limitation that we have only binary and()s and or()s is really just a compiler limit; it's neither needed nor enforced when parsing policies, printing them, lifting them, etc etc. Maybe we _want_ to enforce this at the type level, but we don't now, and given that, these error variants are in the wrong place (and will get in the way of the refactors in the next commit).
apoelstra
force-pushed
the
2025-05-compiler
branch
from
5dff64e to
c317cd7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 PR is a grab back of prepatory commits for larger changes. It includes a couple of followups to #815 in the first commits. Then it cleans up some error types and paths.
I don't believe anything in here will be tricky or controversial, though there are API breaks because of the cleaned up error types.