Drop the `Property` trait entirely #652

apoelstra · 2024-03-06T03:47:06Z

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.

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.

sanket1729

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.

sanket1729 · 2024-03-06T06:57:31Z

src/miniscript/types/correctness.rs

-impl Property for Correctness {
-    fn sanity_checks(&self) {
+    /// Confirm invariants of the correctness checker.
+    pub fn sanity_checks(&self) {


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.

Sounds good! I'll do this in a followup.

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.

apoelstra added 12 commits March 5, 2024 21:29

blanket_traits: remove crate:: in a bunch of places

3c1c896

types: remove all Result returns for ExtData

602fd29

This arguably belongs in the previous commit but it's invasive and entirely mechanical so I'm putting it in its own commit.

types: make a ton of constructors const

d4dc226

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.

types: remove error path from sub-fragment parameter to threshold()

e971e77

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 March 6, 2024 04:00

sanket1729 approved these changes Mar 6, 2024

View reviewed changes

apoelstra merged commit dcbef52 into rust-bitcoin:master Mar 6, 2024
16 checks passed

apoelstra deleted the 2024-03--no-property branch March 6, 2024 13:45

apoelstra mentioned this pull request Mar 6, 2024

sanity_check methods throughout the codebase shouldn't be part of the API #653

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop the `Property` trait entirely #652

Drop the `Property` trait entirely #652

apoelstra commented Mar 6, 2024 •

edited

Loading

sanket1729 left a comment

sanket1729 Mar 6, 2024

apoelstra Mar 6, 2024

apoelstra Mar 6, 2024

Drop the Property trait entirely #652

Drop the Property trait entirely #652

Conversation

apoelstra commented Mar 6, 2024 • edited Loading

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 Mar 6, 2024

Choose a reason for hiding this comment

apoelstra Mar 6, 2024

Choose a reason for hiding this comment

apoelstra Mar 6, 2024

Choose a reason for hiding this comment

Drop the `Property` trait entirely #652

Drop the `Property` trait entirely #652

apoelstra commented Mar 6, 2024 •

edited

Loading