Several locktime improvements #654

apoelstra · 2024-03-06T20:44:54Z

This PR:

Changes user-facing "check locktime" functions to use bitcoin::relative::LockTime rather than Sequence numbers.
Introduces a new RelLockTime type analogous to AbsLockTime which is used to provide a locktime type that implements Ord.
In both types, validate that the locktimes are legal for use in Miniscript during creation, rather than during typechecking or validity checks.
In many places, replace manual code with the new is_implied_by method from rust-bitcoin.

This might be a little hard to review because it messes with locktime code which is so easy to get backward. But it appears that there are pretty extensive unit tests which caught several iterations of this where I did get them backward. Going forward things should be much easier to maintain because we have stronger types and an encapsulated is_implied_by method.

The next PR will do a similar thing with thresholds to validate the 0 < k <= n invariant when thresholds are created, avoiding the repeated and inconsistent post-hoc error checking logic.

Fixes #270

tcharding

ACK 9b3d45d

tcharding · 2024-03-07T00:53:18Z

Gotta be happy with the new is_implied_by, feels good to get it right huh. Was a bit sad to see the error code at the top of the file \me shakes head, bangs table, and mumbles "have I taught you nothing" :)

apoelstra · 2024-03-07T19:27:15Z

Was a bit sad to see the error code at the top of the file \me shakes head, bangs table, and mumbles "have I taught you nothing" :)

In my later PRs there will be errors.rs modules, but for such short files with so few errors, it seemed fine to inline them :).

And if you want to feel disappointment, you should see the existing logic in this crate ;) there are stringly-typed errors everywhere and even places where we call .to_string on errors and then embed the strings in new errors!

sanket1729

ACK 1f8af49.

The error variant can be fixed in future commit. Does clippy have a lint that checks that all error variants are constructed and used in atleast one place?

sanket1729 · 2024-03-07T20:19:50Z

src/policy/compiler.rs

@@ -1308,14 +1297,16 @@ mod tests {
    #[test]
    fn compile_timelocks() {
        // artificially create a policy that is problematic and try to compile
+        /*


Creating an issue to fix this.

I don't remember commenting out this code. I wonder if some unknown-to-me vim binding did it.

Oh, no, I remember now. The point is that this policy used to cause an error when you compiled it, but now you just can't construct it.

I should delete this test entirely.

Oh, no, I misunderstood the test. It's about mixing timelock types, not about invalid timelocks.

I should actually just uncomment the test :). It should still work.

sanket1729 · 2024-03-07T20:27:25Z

src/interpreter/stack.rs

        } else {
-            Some(Err(Error::RelativeLocktimeNotMet(n.to_consensus_u32())))
+            // BIP 112: if the tx locktime has the disable flag set, fail CSV.
+            Some(Err(Error::RelativeLocktimeNotMet(*n)))


RelativeLocktimeDisabled variant should be returned here.

Good catch. I'll just re-do the PR since this is a quick thing to fix.

apoelstra · 2024-03-07T21:42:22Z

@sanket1729 annoyingly no, Clippy, does not have a lint. But you can locally change pub on the error types to pub(crate) and then you'll get rustc warnings, so you can manually lint this way.

Simplifies the API and reduces copying.

These are unused since PR rust-bitcoin#13. We don't even use the term "strong" anymore in Miniscript.

We have the type `AbsLockTime` whose purpose is to provide an ordering for locktimes. This type is converted from strings and script values, but we don't check validity during construction. Instead we have multiple places where validity is manually checked. Fix this. Note that this is an API-breaking change for people who are manually constructing policies and Miniscripts since it removes a few ways to unconditionally convert u32s into locktimes.

The "age" from the transaction is actually just a sequence number. Whether it is interpreted as an "age" depends on various other factors, as described in BIP112. In the next commits we will start moving much of our `Sequence` usage to `relative::LockTime` and it will be helpful to have clearer names.

…relative locktimes Eliminates some error logic. Also replaces manual implementations of `is_implied_by` in a couple places. This introduces a couple unwrap()s which will be removed in a later commit, and which occur because there is a mismatch between the `Sequence` type used in `satisfy::Satisfaction` and `bitcoin::relative::LockTime` (and we can't switch this `Sequence` because we depend on `Ord`, which will need a new newtype, which should wait for another commit). This *also* deletes a couple lines in the PSBT code which cites BIP 112 saying that if the sequence number passed to OP_CSV has the disable flag set, to automatically pass `check_older`. With this code change, it is no longer possible to produce such a situation, so we drop the check and the citation. I believe this is correct because no Miniscript-generated call to OP_CSV should have the disable flag set. I believe the Miniscript spec is silent on this and more-or-less says you can pass any crap you want as the argument to `older`, but I am taking a stand here and think we should Miniscripts with the disable flag set.

Just like AbsLockTime, this gives us a locktime which implements Ord and which cannot be constructed unless it meets the invariants specified by Miniscript.

Gets rid of a couple unwraps from the last commit :)

This changes the signature of `at_age` to take a `relative::LockTime` to be symmetric with `at_lock_time`. But because `relative::LockTime` has no constructors this is kinda annoying to use. See rust-bitcoin/rust-bitcoin#2547 Anyway for now you can just construct a RelLockTime and tack .into() onto it, which is fine as a workaround. We may want to update docs for it.

This gets rid of all the new conversions and unwraps :)

In general, the type from the transaction should be `Sequence` while the type we are comparing to should be `RelLockTime` or `relative::LockTime`. Having different types here should help us avoid getting these comparisons backward.

apoelstra · 2024-03-07T22:00:28Z

Addressed both comments. Also changed Locktime to LockTime in the interpreter errors, since this was the only place that this spelling occurred (in rust-bitcoin we standardized on LockTime).

There are some unused variants in src/lib.rs Error type but I'll get those in a later PR. Eventually I want to delete this because we've moved away from the "library has a single giant global error type" model in rust-bitcoin.

sanket1729

ACK 422b83c

04715e3 absolute: make is_* methods uniform with the ones from relative (Andrew Poelstra) 878b865 relative locktime: introduce is_* methods to check units (Andrew Poelstra) c2f87c7 relative locktime: add is_implied_by method for sequences (Andrew Poelstra) 319e102 relative locktime: use From/TryFrom to convert between relative locktimes and Sequence (Andrew Poelstra) 0ed2691 relative locktime: add conversions to/from sequence (Andrew Poelstra) 5c8fb5c relative locktime: add consensus encode/decode functions (Andrew Poelstra) ac968e0 relative locktime: constify a bunch of constructors (Andrew Poelstra) f27e675 relative locktime: add "obvious" constructors (Andrew Poelstra) f02b1da relative locktime: copy comments and PartialOrd impl from absolute locktimes (Andrew Poelstra) 2ff5085 locktimes: run cargo fmt (Andrew Poelstra) Pull request description: While implementing rust-bitcoin/rust-miniscript#654 I ran into a number of limitations of the `relative::LockTime` API. This fixes these by * Copying a ton of functions from `absolute::LockTime` to `relative::LockTime`, adjusting comments and functionality accordingly. * Adding conversion functions to/from `Sequence` numbers, as well as a method to check whether a locktime is satisfied by a given sequence number. Fixes #2547 Fixes #2545 Fixes #2540 ACKs for top commit: tcharding: ACK 04715e3 sanket1729: ACK 04715e3 Tree-SHA512: 70740eaa3a83dc1e7312b99e907ccdcef4eeb6191ae881d81712707ad6fb949c4e28183ab6f9258c6cde1ef8fdd5dc6476439e705a9e02a939b7832430a608d4

apoelstra force-pushed the 2024-03--locktimes branch 2 times, most recently from 7137293 to 2093123 Compare March 6, 2024 20:53

apoelstra mentioned this pull request Mar 6, 2024

relative::LockTime should be convertable to Sequence and/or have a consensus_u32 encoder rust-bitcoin/rust-bitcoin#2545

Closed

apoelstra force-pushed the 2024-03--locktimes branch from 2093123 to 9b3d45d Compare March 6, 2024 21:36

tcharding approved these changes Mar 7, 2024

View reviewed changes

apoelstra mentioned this pull request Mar 7, 2024

improve relative locktime API rust-bitcoin/rust-bitcoin#2549

Merged

sanket1729 previously approved these changes Mar 7, 2024

View reviewed changes

sanket1729 mentioned this pull request Mar 7, 2024

Creating an issue to fix this. #655

Closed

apoelstra added 10 commits March 7, 2024 21:45

types: replace closure with iterator

2f66778

Simplifies the API and reduces copying.

types: drop unused "strongness" errors

60d7c98

These are unused since PR rust-bitcoin#13. We don't even use the term "strong" anymore in Miniscript.

introduce RelLockTime type

fc6bfa8

Just like AbsLockTime, this gives us a locktime which implements Ord and which cannot be constructed unless it meets the invariants specified by Miniscript.

miniscript: use RelLockTime in Satisfaction

a7a1b9a

Gets rid of a couple unwraps from the last commit :)

miniscript: use RelLockTime in Terminal

fc42e48

This gets rid of all the new conversions and unwraps :)

apoelstra dismissed sanket1729’s stale review via 422b83c March 7, 2024 21:59

apoelstra force-pushed the 2024-03--locktimes branch from 9b3d45d to 422b83c Compare March 7, 2024 21:59

sanket1729 approved these changes Mar 7, 2024

View reviewed changes

apoelstra merged commit b68bf13 into rust-bitcoin:master Mar 8, 2024
16 checks passed

apoelstra deleted the 2024-03--locktimes branch March 8, 2024 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several locktime improvements #654

Several locktime improvements #654

apoelstra commented Mar 6, 2024 •

edited

Loading

tcharding left a comment

tcharding commented Mar 7, 2024

apoelstra commented Mar 7, 2024

sanket1729 left a comment

sanket1729 Mar 7, 2024

apoelstra Mar 7, 2024

apoelstra Mar 7, 2024

apoelstra Mar 7, 2024

sanket1729 Mar 7, 2024

apoelstra Mar 7, 2024

apoelstra commented Mar 7, 2024

apoelstra commented Mar 7, 2024

sanket1729 left a comment

Several locktime improvements #654

Several locktime improvements #654

Conversation

apoelstra commented Mar 6, 2024 • edited Loading

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Mar 7, 2024

apoelstra commented Mar 7, 2024

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 Mar 7, 2024

Choose a reason for hiding this comment

apoelstra Mar 7, 2024

Choose a reason for hiding this comment

apoelstra Mar 7, 2024

Choose a reason for hiding this comment

apoelstra Mar 7, 2024

Choose a reason for hiding this comment

sanket1729 Mar 7, 2024

Choose a reason for hiding this comment

apoelstra Mar 7, 2024

Choose a reason for hiding this comment

apoelstra commented Mar 7, 2024

apoelstra commented Mar 7, 2024

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra commented Mar 6, 2024 •

edited

Loading