types: make Malleability an enum - #1037
Conversation
| // The "s" property is only reported for a non-malleable | ||
| // expression, so the expected value only applies to those. | ||
| if non_mal { | ||
| assert_eq!(ms.requires_sig(), need_sig); |
There was a problem hiding this comment.
In c7a495d:
Rather than gating the check on non_mal, I think we should change the assertion to non_mal && need_sig. And we should promise that this method returns false for malleable script.
There was a problem hiding this comment.
Ah, your third commit adds such a test. We should do the same here.
There was a problem hiding this comment.
| /// begin with, so nothing is known for a malleable one, and `false` is the | ||
| /// assumption that keeps a caller from relying on a signature being | ||
| /// required. | ||
| pub fn requires_sig(&self) -> bool { self.ty.mall.signed().unwrap_or(false) } |
There was a problem hiding this comment.
In 53993fc:
In addition to updating the docs we should also deprecate this and rename it to non_malleable_and_requires_sig.
We could also implement it as matches!(self.ty.mall, Malleability:::NonMalleable { signed: true, .. }) I think. I dunno if that's any clearer than what you've written. Just suggesting it in case you like it.
There was a problem hiding this comment.
Agree, I've done that in a new refactor commit (4817382).
The "s", "f" and "e" properties of a fragment describe its satisfactions and dissatisfactions only if the fragment meets the non-malleability requirement of every fragment it is built from. Once a fragment is malleable, so is every fragment containing it, and nothing follows from those three properties: a malleable "f" fragment may be dissatisfiable without a signature, and a malleable "e" fragment may have several unconditional dissatisfactions. The type test vectors nevertheless pinned them for malleable expressions, which pinned a choice the type system never meant to promise. This drops them from the 9094 malleable vectors that carried one, leaving the "m" of a non-malleable expression as the gate for the other three. BIP379 is gaining the same clarification in bitcoin/bips#2267. Because `ms_test` compares the rendered type exactly, dropping the three properties from those vectors asserts that a malleable expression does not carry them, rather than merely leaving them unchecked. No expression changes its type: the properties are still computed as before, they are only not checked where they say nothing. Assisted-by: Claude Opus 5
The three malleability properties of a fragment only describe it if the
fragment is non-malleable to begin with, so carrying them for a malleable
one invites reading a promise into them that the type system does not
make. Encode that in the type:
pub enum Malleability {
Malleable,
NonMalleable { dissat: Dissat, signed: bool },
}
Every combinator already required each of its sub expressions to be
non-malleable, so a malleable sub expression short circuits the whole
fragment and the requirement that the BIP379 table lists for each
fragment becomes an early return rather than a conjunct buried in a
`non_malleable` field. No non-malleable expression changes type, and
nothing but the erased properties of a malleable expression changes at
all.
Two consequences worth naming:
- `requires_sig` returns false for a malleable expression, where it used
to return a value the type system did not stand behind. False is the
answer that keeps a caller from relying on a signature being required.
It is spelled as a single `matches!` on the enum rather than a defaulted
`Option`, so the conjunction it reports is visible at the definition.
The attribute tests assert it for every vector, expecting `non_mal &&
need_sig` rather than skipping the malleable ones, and the next commit
renames and deprecates the method.
- The threshold "e" rule now matches BIP379's "e=all are s" exactly,
without the extra condition of rust-bitcoin#1032: the requirement checked before it
already establishes that every sub expression is "e", so the two rules
coincide for every threshold that has the property at all.
`Malleability` is public, so this is a breaking change for anyone reading
its fields.
Assisted-by: Claude Opus 5
Since the previous commit the method answers "is this non-malleable, and does every satisfaction require a signature", returning false for a malleable expression because there is no signature property to report for one. The old name asks about the signature alone, which invites a caller to read a `false` as "no signature required" when it may equally mean "malleable, so nothing is promised". The new name states the conjunction the body checks. The rename is a hard break rather than a forwarding shim, deliberately. The previous commit changed what the method answers for a malleable expression, and a shim under the old name would keep downstream code compiling while quietly returning a different value, which is worse than a name that is simply gone. The new name is then deprecated straight away, because the conjunction is a wart rather than a property worth exposing: it exists so the crate has an honest word for what it checks in `validate` and when picking a satisfaction, and so the break lands on a name that says what it does. A caller who wants the property itself should read it off the malleability type, which is what the note says. Nothing is asked of downstream users twice, since the deprecated name is new and has no users yet. Mechanical otherwise: the definition and its six call sites, all within `src/miniscript`. `Malleability` is public and so is this method, so it is a break downstream users will see. While here, fix the `is_non_malleable` doc comment, which described the opposite of what the method returns. Assisted-by: Claude Opus 5
Covers both arms of the enum on the same fragment shape: an or_b of two hash fragments is valid but malleable, and renders as "B/du" with no signature requirement, while the same shape over two keys keeps its dissatisfaction and signature properties and renders as "B/duesm". That or_b was never signed, so on its own it only pins the erasure of the dissatisfaction property; its signature assertion held before this branch too. The third case is a fragment that did carry "s": and_b is signed if either child is, and the multi is, so `j:and_b(multi(2,A,B),s:or_i(older(1),older(4252898)))` rendered as "B/ndus" and answered true to `requires_sig` while still being malleable. It is the case that would notice the erasure being undone. Assisted-by: Claude Opus 5
8dc9335 to
dc60507
Compare
|
Thanks a lot for the review, I've addressed your comments! |
| #[deprecated( | ||
| since = "TBD", | ||
| note = "check the `signed` property of the malleability type instead" | ||
| )] |
There was a problem hiding this comment.
In 4817382:
This deprecation is in the wrong place and its message makes no sense. We should deprecate the requires_sig method, telling users to use non_malleable_and_requires_sig instead. And we should not deprecate the new method.
Did Claude do this (introducing a new method and immediately deprecating it)?
There was a problem hiding this comment.
Hmm, I might have misunderstood your earlier comment.
The requires_sig method did exist before this PR. And your comment stated: "In addition to updating the docs we should also deprecate this and rename it to non_malleable_and_requires_sig.".
Which is what I did (with Claude, yes).
But it sounds like you might have meant something like "introduce a new, fixed method called non_malleable_and_requires_sig"?
And yes, the deprecation comment definitely isn't my best work, happy to use a better wording if you have a suggestion.
|
Overall this looks good (other than the deprecation thing). ChatGPT 5.6 Sol points out that this slightly messes up our satisfaction algorithm. In particular, we pass If there is a signature, then we return This was already a crude check that would sometimes report timelock-checks as malleable when they weren't, and now it's even cruder, but I guess it's worth noting. We should maybe file a bug to "fix" the satisfier to keep better track of whether it's included a not-3rd-party-removable signature anywhere, and use that rather than this I would suggest that, at least, we add a regression test to this PR, and call it |
2 participants
This is an alternative approach to #1032, following the suggestion here: #1032 (comment).
With this PR we change the
types::Malleabilitytype to erase thef/e/sproperties if something is malleable, by turning it into an enum.The BIP will be updated to match this change. And, assuming there is agreement on the approach, #1032 would be closed.