Drop the Property trait entirely
#652
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
It turns out that the `Sha256 = Self::_Sha256` requirement on `FromStrKey` is not sufficient for `FromStrKey::_Sha256` to implement all the traits that `Miniscript::Sha256` does. It further turns out that sometimes, for `#[derive(Debug)]` to work, Rust requires this. So just copy the bounds over explicitly.
Running `cargo +nightly clippy --all-features` finds a couple things in the compiler. Fix these. One is a subtle change to our `OrdF64` type: this is a wrapper type to add `Ord` to f64, panicking in the case that we hit NaN. Previously our `Ord` impl would panic but our `PartialOrd` impl would not. Now they both do.
The `Property` trait was supposed to provide a "general recursive property of Miniscript" definition. In practice this allowed a tiny bit of reused code between the different hash fragments and the different time fragments, while preventing the use of constfns and resulting in ugly APIs which would sometimes take unused parameters. It will also prevent returning distinct error types in future, which is why we are removing it now. While we're at it, rename all the `from_` constructors to not have `from_` in them. In practice there is almost no situation where you would want to generalize over different `Property`s and over the next few commits we will delete this trait entirely.
See previous commit message for justification. In this case we can also change the signature of every method that returned a Result<Self, ErrorKind> to just return Self, since malleability computations don't error out. Worst case they just mark the final miniscript as being malleable.
Most of these functions are not called from anywhere, and the ones that are called are only called from tests. It may be worth revisiting whether they ought to be pub. For now I just left them as pub since they were publicly visible before. See earlier commit message for justification.
This arguably belongs in the previous commit but it's invasive and entirely mechanical so I'm putting it in its own commit.
Reveals a couple interesting things -- the `and_n` constructor was never used, though it was implemented (and_n is a composite of andor and 0). Also we call CompilerExtData::sanity_checks even though this function is not defined and just defaults to a no-op.
It still feels like there is more to remove because we have this `type_check_common` method which sometimes takes an error-returning closure and sometimes takes an infallible one. But this at lesat gets rid of the error paths that were purely caused by Property. Again, in a separate commit to make reviewing easier.
This trait is now only used on the `Type` type. Drop the trait, rename the `from_` constructors, and remove unused parameters and type parameters. Most of the methods can now be constfns. Will do this in the next commit since it's a bit annoying (mainly: can't use ?, have to be explicit about early returns).
This is a mechanical/annoying change so I did it in its own commit. But the upshot is that you can do all the type construction in a const context now.
This was one last set of unused error paths that was hard to remove until I'd removed the Property trait from Type. But now that's done so I can do it.
apoelstra
force-pushed
the
2024-03--no-property
branch
from
9f4ff67 to
e971e77
Compare
sanket1729
approved these changes
Mar 6, 2024
There was a problem hiding this comment.
ACK e971e77. Looks a lot cleaner. Thanks for these changes. It's nice that we no longer have the artificial error paths because the parent trait had the definition to encompass all possible implementations.
| impl Property for Correctness { | ||
| fn sanity_checks(&self) { | ||
| /// Confirm invariants of the correctness checker. | ||
| pub fn sanity_checks(&self) { |
There was a problem hiding this comment.
Aside: I feel we should not make this pub. rust-miniscript should guarantee that these hold. We should check these internally for our soundness checks.
I saw in few downstream crates where they called ms.sanity_check() frequently. This gives me C++ vibes where users call isValid before calling functions on struct.
There was a problem hiding this comment.
Sounds good! I'll do this in a followup.
There was a problem hiding this comment.
Actually I'll just file an issue. There are a lot of sanity_check methods in the code, many of which are not called from within the library, and at least one has a doc example suggesting to use it to check "whether all spend paths are accessible in the Bitcoin network".
So we need to re-assess all these functions and see if they can be pulled into the type system somehow so you simply can't create the objects without running the (non-pub) methods.
heap-coder
added a commit
to heap-coder/rust-miniscript
that referenced
this pull request
Sep 27, 2025
…irely
e971e77d64527d8f70bad690500ea12a51cfdf08 types: remove error path from sub-fragment parameter to threshold() (Andrew Poelstra)
d4dc2267e40f2a2253fa0bf4ef2b67b223533d3a types: make a ton of constructors const (Andrew Poelstra)
fdea7f642e77be14cbaaf8c275f4e8bca316460f types: drop `Property` trait entirely (Andrew Poelstra)
b25023db3a54b4034339bdbea315263ba867326d compiler: remove a bunch of unused error paths (Andrew Poelstra)
eb854aacc5eeab55477b8fa9e7f659ea176c6a81 compiler: stop using Property trait in CompilerExtData (Andrew Poelstra)
602fd29ba4d7e701a97888a0f41e7fffe7e54d01 types: remove all Result returns for ExtData (Andrew Poelstra)
b6c3ca859b56a9cbe0ccdee1d42d5cb2bbdd3b92 types: don't implement Property for ExtData. (Andrew Poelstra)
87294ff7d350c4e9d64956a4d85b93b78d4e2ebb types: don't implement Property for Malleability (Andrew Poelstra)
dd0978d547e45e0c61702c8c91de97df6f79494a types: don't implement Property for Correctness (Andrew Poelstra)
962038034c607c3690acc450f5dcdea39bcca94e clippy: fix a couple nits (Andrew Poelstra)
3c1c896ace7134015ede5ab8ba580c38e6dc14f6 blanket_traits: remove crate:: in a bunch of places (Andrew Poelstra)
9280609aa3500f43798f34b4f3995f335e67c21e blanket_traits: add some more bounds (Andrew Poelstra)
Pull request description:
Something of a followup to #649, which specifically removes the `type_check` method from `Property`.
Basically, this whole trait is confused. Originally it was supposed to express "a recursively defined property of a Miniscript" which could be defined by a couple constructors and then automatically computed for any Miniscript. In fact, the trait required a separate constructor for every single Miniscript fragment so nothing was "automatic" except sometimes reusing code between the different hash fragments and between `older`/`after`.
Furthermore, every implementor of this trait had slightly different requirements, meaning that there were `unreachable!()`s throughout the code, functions that were never called (e.g. `Type::from_sha256` passing through to `Correctness::from_sha256` and `Malleability::from_sha256` when all three of those types implemented the function by calling `from_hash`, which *also* was defined by `Type::from_hash` calling `Correctness::from_hash` and `Malleability::from_hash`. So much boilerplate) and many instances of methods being implemented but ignoring arguments, or (worse) being generic over `Pk`/`Ctx` and ignoring those types entirely, or returning `Result` but always returing the `Ok` variant.
So basically, there was no value in being generic over the trait and the trait wasn't even a generalization of any of the types that implemented it.
**Adding to this** having the trait prevents us from having constfns in our type checking code, which sucks because we have a lot of common fixed fragments. It also prevents us from returning different error types from different parts of the typechecker, which is my specific goal in removing the trait.
This PR has a lot of commits but most of them are mechanical and/or small.
ACKs for top commit:
sanket1729:
ACK e971e77d64527d8f70bad690500ea12a51cfdf08. Looks a lot cleaner. Thanks for these changes. It's nice that we no longer have the artificial error paths because the parent trait had the definition to encompass all possible implementations.
Tree-SHA512: 38c254caf938e5c3f84c1f0007e0bfc93180e678aef3968cb8f16ad1544722a0f7cfda2c014056446f0091b198edb12c91c0424df64f68c31f5d316fac28425c
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.
Something of a followup to #649, which specifically removes the
type_checkmethod fromProperty.Basically, this whole trait is confused. Originally it was supposed to express "a recursively defined property of a Miniscript" which could be defined by a couple constructors and then automatically computed for any Miniscript. In fact, the trait required a separate constructor for every single Miniscript fragment so nothing was "automatic" except sometimes reusing code between the different hash fragments and between
older/after.Furthermore, every implementor of this trait had slightly different requirements, meaning that there were
unreachable!()s throughout the code, functions that were never called (e.g.Type::from_sha256passing through toCorrectness::from_sha256andMalleability::from_sha256when all three of those types implemented the function by callingfrom_hash, which also was defined byType::from_hashcallingCorrectness::from_hashandMalleability::from_hash. So much boilerplate) and many instances of methods being implemented but ignoring arguments, or (worse) being generic overPk/Ctxand ignoring those types entirely, or returningResultbut always returing theOkvariant.So basically, there was no value in being generic over the trait and the trait wasn't even a generalization of any of the types that implemented it.
Adding to this having the trait prevents us from having constfns in our type checking code, which sucks because we have a lot of common fixed fragments. It also prevents us from returning different error types from different parts of the typechecker, which is my specific goal in removing the trait.
This PR has a lot of commits but most of them are mechanical and/or small.