improve relative locktime API #2549

apoelstra · 2024-03-07T18:27:16Z

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

coveralls · 2024-03-07T18:29:30Z

Pull Request Test Coverage Report for Build 8382382642

Details

41 of 120 (34.17%) changed or added relevant lines in 4 files are covered.
4 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.2%) to 83.425%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bitcoin/src/blockdata/locktime/absolute.rs	2	5	40.0%
units/src/locktime/absolute.rs	5	12	41.67%
units/src/locktime/relative.rs	3	19	15.79%
bitcoin/src/blockdata/locktime/relative.rs	31	84	36.9%

Files with Coverage Reduction	New Missed Lines	%
bitcoin/src/blockdata/locktime/absolute.rs	1	82.82%
units/src/locktime/relative.rs	1	17.65%
bitcoin/src/blockdata/locktime/relative.rs	2	59.41%

Totals
Change from base Build 8377069484:	-0.2%
Covered Lines:	19041
Relevant Lines:	22824

💛 - Coveralls

apoelstra · 2024-03-07T18:55:16Z

CI failing because my doccomment links are broken. I should learn to check that locally..

apoelstra · 2024-03-07T23:08:04Z

Ready for review.

Kixunil · 2024-03-08T09:43:47Z

bitcoin/src/blockdata/locktime/relative.rs

@@ -40,6 +40,46 @@ pub enum LockTime {
 }


Oh, there was From<u16>, well, I don't really like that...

Given that this type is basically only used within the LockTime::Height enum variant, where it is very clear what a bare number is supposed to mean, I think it's fine. Same with Time , though the interpretation of the number as "512-second intervals" is maybe surprising.

But sure, I can drop the From impls in a new commit if they bother you. They are not very discoverable and gain little ergonomics over just using LockTime::from_height/from_time.

Kixunil · 2024-03-08T09:43:57Z

bitcoin/src/blockdata/locktime/relative.rs

+
+    /// Constructs a `LockTime` from `n`, expecting `n` to be a 16-bit count of blocks.
+    #[inline]
+    pub fn from_height(n: u16) -> Self { LockTime::Blocks(Height(n)) }


Could be const.

Kixunil · 2024-03-08T09:44:16Z

bitcoin/src/blockdata/locktime/relative.rs

+    /// This function is a little awkward to use, and users may wish to instead use
+    /// [`Self::from_seconds_floor`] or [`Self::from_seconds_ceil`].
+    #[inline]
+    pub fn from_512_second_intervals(intervals: u16) -> Self { LockTime::Time(Time(intervals)) }


Kixunil · 2024-03-08T09:45:06Z

bitcoin/src/blockdata/locktime/relative.rs

+    /// Will return an error if the input cannot be encoded in 16 bits.
+    #[inline]
+    pub fn from_seconds_floor(seconds: u32) -> Result<Self, Error> {
+        Time::from_seconds_floor(seconds).map(LockTime::Time)


Can be const with more complicated code. I'm not sure if worth it.

Done in new commit. Wasn't too bad, I think it was worth it.

Kixunil · 2024-03-08T09:45:28Z

bitcoin/src/blockdata/locktime/relative.rs

+    /// Will return an error if the input cannot be encoded in 16 bits.
+    #[inline]
+    pub fn from_seconds_ceil(seconds: u32) -> Result<Self, Error> {
+        Time::from_seconds_ceil(seconds).map(LockTime::Time)


Complicated const again.

Done in new commit. Wasn't too bad, I think it was worth it.

Kixunil · 2024-03-08T09:48:37Z

bitcoin/src/blockdata/locktime/relative.rs

+            Ok(Time::from_512_second_intervals(interval))
+        } else {
+            Err(Error::IntegerOverflow(seconds))
+        }


Could be const too with a bit uglier code.

Done in new commit. Wasn't too bad, I think it was worth it.

Kixunil · 2024-03-08T09:48:51Z

bitcoin/src/blockdata/locktime/relative.rs

@@ -222,6 +262,10 @@ impl Height {
    /// The maximum relative block height.
    pub const MAX: Self = Height(u16::max_value());

+    /// Create a [`Height`] using a count of blocks.
+    #[inline]
+    pub fn from_height(blocks: u16) -> Self { Height(blocks) }


Trivial const.

Kixunil · 2024-03-08T09:50:35Z

bitcoin/src/blockdata/locktime/relative.rs

+    ///
+    /// This method will **not** round-trip with [`Self::to_consensus_u32`], because relative
+    /// locktimes only use some bits of the underlying `u32` value and discard the rest. If
+    /// you want to preserve the full value, you should use the [`Sequence`] type instead.


Honestly, this looks like a foot gun. Is it really needed? Just decoding Sequence and converting that is less error-prone and more explicit.

Yeah, I considered this. I could go either way on it. I'm not sure that making users go through Sequence makes this flow any less footgunny.

But I can remove this if you feel strongly about it.

I'd maybe like to add a method to script to encode a Sequence number directly into script, if there isn't one, so that users would never need a u32.

bitcoin/src/blockdata/locktime/relative.rs

Kixunil · 2024-03-08T09:53:52Z

bitcoin/src/blockdata/locktime/relative.rs

+        match *self {
+            LockTime::Blocks(_) => true,
+            LockTime::Time(_) => false,
+        }


Could just use matches!

tcharding

ACK 8e43684

bitcoin/src/blockdata/locktime/relative.rs

apoelstra · 2024-03-10T13:26:58Z

In one of the resolved conversations it was noted that this doesn't implement ArbitraryOrd. I opened this issue to track this #2566

tcharding · 2024-03-13T01:44:00Z

Needs rebase mate.

bitcoin/src/blockdata/locktime/relative.rs

tcharding · 2024-03-18T20:32:45Z

Wow, super bad process by me. I previously ack'ed this PR and now I've given a 9-comment review. Therefore, every comment is a can-do-in-followup-PR

tcharding

ACK b72cc8c

apoelstra · 2024-03-19T16:51:29Z

Addressed all comments except the one about the ZERO constant.

tcharding

ACK b72cc8c

tcharding · 2024-03-19T20:49:14Z

Thanks man!

tcharding · 2024-03-20T22:12:45Z

Needs rebase again bro

apoelstra · 2024-03-20T22:16:51Z

Done.

tcharding

Error getter can be done later, and the failure to add it is a review fail by me for not noticing earlier. I"m planning on going over all the errors ... again ... next release.

In commit 2799ecdf relative locktime: add "obvious" constructors the last line of the commit log is stale, from_consensus change is in a later PR.

ACK d3fe9ac

tcharding · 2024-03-21T01:56:47Z

bitcoin/src/blockdata/locktime/relative.rs

+/// Error returned when a sequence number is parsed as a lock time, but its
+/// "disable" flag is set.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct DisabledLockTimeError(u32);


This is a leaf error type so we should probably provide a getter if I remembered correctly and that is the policy we settled upon.

…cktimes

Adds constructors to allow directly creating locktimes from time or block counts; adds a flooring constructor to Time to match the ceiling one; adds an explicit constructor to Height since the From<u16> was not very discoverable.

…imes and Sequence

This gives a way to determine whether a CSV will pass, given a sequence number, in a type-safe way where you can't get the two things backward.

Copy these from absolute::LockTime. While we are at it, make the functions in absolute::LockTime const.

apoelstra · 2024-03-21T22:10:54Z

Rebased on master, added an accessor to the error type, and fixed the commit message.

tcharding

ACK 04715e3

tcharding · 2024-03-22T04:11:33Z

@sanket1729 have you got time to review this guy please? I'd love to have a play with the rust-miniscript bitcoin upgrade some more once this merges - it would be great to be able to get rid of RelLockTime with this release.

apoelstra · 2024-03-22T14:19:44Z

@tcharding I don't think we can get rid of RelLockTime. We still need the a wrapper that implements Ord and we still need some other parsing-related properties that relative::LockTime doesn't have. Same reason we still have AbsLockTime even though we've made some improvements to absolute::LockTime here.

So really all this PR will do for us is simplify the implementation of RelLockTime.

tcharding · 2024-03-22T18:51:01Z

The Ord stuff is handled by #2581. What parsing stuff? After this PR goes in relative::LockTime and Sequence are interoperable so to speak, so RelLockTime(Sequence) seems like it should be able to go away?

Oooh, I see AbsLockTIme has an additional invariant (can't be 0) so it has to stay even with ArbitraryOrd, bother. No real benefit removing one and not the other.

rust-miniscript resists my attempts to improve it ... again :)

tcharding · 2024-03-22T18:51:39Z

Sorry for the noise @sanket1729

sanket1729

ACK 04715e3

apoelstra · 2024-03-22T20:32:18Z

Oooh, I see AbsLockTIme has an additional invariant (can't be 0) so it has to stay even with ArbitraryOrd, bother. No real benefit removing one and not the other.

Oh that's right! RelLockTime has the same invariant.

I knew there was something dumb in Miniscript that necessitated these types..

apoelstra · 2024-03-22T20:33:39Z

rust-miniscript resists my attempts to improve it ... again :)

Well, rust-miniscript does have a few APIs (satisfaction/PSBT) that interact with the "real" types that come with rust-bitcoin. These are currently a bit awkward and weird and will definitely be improved by this PR.

github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Mar 7, 2024

apoelstra force-pushed the 2024-03--relative-locktime branch from f7717c8 to cbf749c Compare March 7, 2024 22:01

Kixunil reviewed Mar 8, 2024

View reviewed changes

apoelstra force-pushed the 2024-03--relative-locktime branch from cbf749c to 8e43684 Compare March 9, 2024 13:47

tcharding reviewed Mar 9, 2024

View reviewed changes

bitcoin/src/blockdata/locktime/relative.rs Show resolved Hide resolved

tcharding added this to the 0.32.0 milestone Mar 11, 2024

apoelstra force-pushed the 2024-03--relative-locktime branch from 8e43684 to b72cc8c Compare March 18, 2024 13:27

github-actions bot added the C-units PRs modifying the units crate label Mar 18, 2024

tcharding reviewed Mar 18, 2024

View reviewed changes

tcharding previously approved these changes Mar 18, 2024

View reviewed changes

tcharding added the P-high High priority label Mar 18, 2024

tcharding mentioned this pull request Mar 19, 2024

Should LockTime types live in units #2611

Closed

apoelstra mentioned this pull request Mar 19, 2024

Single patch of changes #2613

Draft

apoelstra dismissed tcharding’s stale review via 2f1bd57 March 19, 2024 16:51

apoelstra force-pushed the 2024-03--relative-locktime branch from b72cc8c to 2f1bd57 Compare March 19, 2024 16:51

tcharding previously approved these changes Mar 19, 2024

View reviewed changes

apoelstra dismissed tcharding’s stale review via d3fe9ac March 20, 2024 22:16

apoelstra force-pushed the 2024-03--relative-locktime branch from 2f1bd57 to d3fe9ac Compare March 20, 2024 22:16

tcharding previously approved these changes Mar 21, 2024

View reviewed changes

locktimes: run cargo fmt

2ff5085

apoelstra added 9 commits March 21, 2024 22:07

relative locktime: copy comments and PartialOrd impl from absolute lo…

f02b1da

…cktimes

relative locktime: add "obvious" constructors

f27e675

Adds constructors to allow directly creating locktimes from time or block counts; adds a flooring constructor to Time to match the ceiling one; adds an explicit constructor to Height since the From<u16> was not very discoverable.

relative locktime: constify a bunch of constructors

ac968e0

relative locktime: add consensus encode/decode functions

5c8fb5c

relative locktime: add conversions to/from sequence

0ed2691

relative locktime: use From/TryFrom to convert between relative lockt…

319e102

…imes and Sequence

relative locktime: add is_implied_by method for sequences

c2f87c7

This gives a way to determine whether a CSV will pass, given a sequence number, in a type-safe way where you can't get the two things backward.

relative locktime: introduce is_* methods to check units

878b865

Copy these from absolute::LockTime. While we are at it, make the functions in absolute::LockTime const.

absolute: make is_* methods uniform with the ones from relative

04715e3

apoelstra dismissed tcharding’s stale review via 04715e3 March 21, 2024 22:10

apoelstra force-pushed the 2024-03--relative-locktime branch from d3fe9ac to 04715e3 Compare March 21, 2024 22:10

tcharding approved these changes Mar 21, 2024

View reviewed changes

sanket1729 approved these changes Mar 22, 2024

View reviewed changes

apoelstra merged commit 3851441 into rust-bitcoin:master Mar 22, 2024
187 checks passed

apoelstra deleted the 2024-03--relative-locktime branch March 22, 2024 21:33

improve relative locktime API #2549

improve relative locktime API #2549

Conversation

apoelstra commented Mar 7, 2024

coveralls commented Mar 7, 2024 • edited

Pull Request Test Coverage Report for Build 8382382642

Details

💛 - Coveralls

apoelstra commented Mar 7, 2024

apoelstra commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

apoelstra commented Mar 10, 2024

tcharding commented Mar 13, 2024

tcharding commented Mar 18, 2024

tcharding left a comment

Choose a reason for hiding this comment

apoelstra commented Mar 19, 2024

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Mar 19, 2024

tcharding commented Mar 20, 2024

apoelstra commented Mar 20, 2024

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Mar 21, 2024

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Mar 22, 2024

apoelstra commented Mar 22, 2024

tcharding commented Mar 22, 2024

tcharding commented Mar 22, 2024

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra commented Mar 22, 2024

apoelstra commented Mar 22, 2024

coveralls commented Mar 7, 2024 •

edited