Add new type for sequence #1093

nlanson · 2022-07-11T03:33:21Z

Created a new type for txin sequence field with methods to create sequences with relative time locks from block height or time units.

nlanson · 2022-07-11T03:33:53Z

I also thought about having a type for a relative time locks which a sequence can be created with, but thought it would be better to add it after #994.

pub enum RelativeTimeLock {
    BlockBased(u16),
    TimeBased(u16)
}

impl Sequence {
    // ...

    pub fn from_timelock(timelock: RelativeTimeLock) -> Self {
        //...
    }

    pub fn timelock(&self) -> Option<RelativeTimeLock> {
        //...
    }

}

tcharding

Nice work, I just left a few comments from what I have learned recently. I'll leave the finer API design review to others more knowledgeable.

src/blockdata/constants.rs

src/blockdata/transaction.rs

tcharding · 2022-07-11T04:41:28Z

src/blockdata/transaction.rs

+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
+pub struct Sequence(pub u32);


If this type is to abstract over nSequence do we want the inner int to be private, wait for others to comment though, don't go just off my review.

I also thought the value doesn't need to be public but made it public following #1082 (comment).

However, I do see the benefits of having the value public where users might want to do arithmetic with the value for any reason.

Sequence has no invariants so I think public is fine.

tcharding · 2022-07-11T04:43:30Z

src/blockdata/transaction.rs

+    }
+}
+
+impl From<u32> for Sequence {


I think all the discussion on #994 about the naming of to_consensus_u32 and from_consensus applies to sequence as well.

I'm not a big fan of the naming but I've replaced From<u32> with the above two functions.

@tcharding I actually think it doesn't. :) Sequence is a number, it's also publicly represented as a number, has no different imaginable encodings... The entire purpose of this newtype is to avoid mixing it with other integers. There's also implicit constructor Sequence just because the field is pub.

That being said, I dislike From<u32> for Sequence, From<Sequence> for u32 is OK though. Also if people prefer consistency I'm fine with having to_consensus/from_consensus.

I'll leave this one open for discussion. I personally think it's clearer to be using the Sequence constructor + From<Sequence> for u32 since there is no special encoding for sequence numbers.

src/blockdata/transaction.rs

tcharding · 2022-07-11T06:04:17Z

Sweet mate, cheers. We can probably learn a thing or two by using this patch in miniscript like I do in rust-bitcoin/rust-miniscript#408, I can have a go but I'll wait for you first so as not to tread on your toes.

nlanson · 2022-07-11T06:08:15Z

Sweet mate, cheers. We can probably learn a thing or two by using this patch in miniscript like I do in rust-bitcoin/rust-miniscript#408, I can have a go but I'll wait for you first so as not to tread on your toes.

Sounds like a good way to figure out what APIs are useful and not but I'm not familiar with mini-script so happy for you to have a go at it.

Kixunil

Light review, looks nice so far!

Kixunil · 2022-07-11T11:13:22Z

src/blockdata/transaction.rs

+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
+pub struct Sequence(pub u32);


Sequence has no invariants so I think public is fine.

src/blockdata/constants.rs

src/blockdata/transaction.rs

src/util/psbt/mod.rs

src/blockdata/transaction.rs

Kixunil · 2022-07-11T11:28:26Z

Also maybe Sequence::ZERO? Even if zero isn't popular, the constant is common on these kinds of newtypes.

nlanson · 2022-07-12T01:32:25Z

Thanks all, looking a lot cleaner now.

I've made Sequence::RBF_SIGNAL public so it can be used in the PSBT tests, but now that it is public, naming seems a little off. Any ideas?

tcharding

A couple of observations.

src/blockdata/transaction.rs

Kixunil · 2022-07-12T09:43:00Z

Could you add #[inline] on all public functions?

Kixunil

Almost ACK, would like to see public method naming resolved and also Default reconsidered.

src/blockdata/transaction.rs

Kixunil · 2022-07-12T19:54:09Z

src/blockdata/transaction.rs

+
+    /// Returns `true` if the sequence has a relative locktime.
+    #[inline]
+    pub fn is_relative_lock_time(&self) -> bool {


is vs has (in doc), not sure what is better - native speaker check?

I think @nlanson is a native English speaker but I'll chime in with 'is'. Also s/locktime/lock time/ since we use undersore separation in is_relative_lock_time

Kixunil · 2022-07-12T19:58:43Z

src/blockdata/transaction.rs

+
+    /// Create a relative lock-time using time units where each unit is equivalent to 512 seconds.
+    #[inline]
+    pub fn from_timeunits(units: u16) -> Self {


Uff, this will be confusing. Maybe something like from_512_seconds_multiples?
Maybe consider adding from_seconds_rounded, from_seconds_floor, from_seconds_ceiling each dividing by 512 and applying appropriate operation.

Explaining in the docs that finer granularity is not supported by Bitcoin would be also helpful.

I've renamed from_timeunits() to from_512_second_intervals() and added from_seconds_floor and from_seconds_ceiling but have them returning Result<Self, RelativeLockTimeError> due to the max possible seconds being larger that u16 and conversion from u32 to u16 being fallible.

Also, if I am to add the RelativeTimeLock enum in this PR, these methods would be implemented on the enum instead of Sequence and Sequence will instead have pub fn from_lock_time(lock_time: RelativeLockTime) -> Sequence

src/blockdata/transaction.rs

tcharding · 2022-07-13T00:15:06Z

You've got a bunch of trailing whitespace issues in this PR @nlanson, we have a git pre-commit hook to help catch that (it does make committing slower though). Check out the Githooks section of the readme if you want to use it.

We are currently working on two new timelock APIs - `LockTime`: rust-bitcoin/rust-bitcoin#994 - `Sequence`: rust-bitcoin/rust-bitcoin#1093 This PR demonstrates how we would use those APIs here in miniscript. Includes improvements that are not yet in 1093. To test this out locally you can grab the `WIP-sequence-and-lock-time` branch from my tree. You'll have to have `rust-bitcoincore-rpc` locally and patch it to use local `rust-bitcoin`. This PR is not intended as a merge candidate.

tcharding · 2022-07-13T02:16:25Z

Hi @nlanson, I've hacked around in miniscript on top of this PR and the LockTime PR (#994). Its all working and is pretty nice if I don't say so myself. I've pushed a draft PR demonstrating use of both PRs (including some additions to the Sequence API): rust-bitcoin/rust-miniscript#446

The Sequence improvements are in comit 3b4fb42e WIP: sequence suggestions on a branch called WIP-sequence-and-lock-time on my tree. I'll paste a diff here for folks to see without checking out the branch. By no means do you have to use my suggestions verbatim :)

diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs
index b990744a..dace0958 100644
--- a/src/blockdata/transaction.rs
+++ b/src/blockdata/transaction.rs
@@ -243,10 +243,13 @@ impl Sequence {
pub const RBF_SIGNAL: Self = Sequence(0xFFFFFFFE);
/// Zero value sequence
pub const ZERO: Self = Sequence(0);
+
/// BIP-68 relative time lock disable flag mask
const DISABLE_FLAG_MASK: u32 = 0x80000000;
/// BIP-68 relative time lock type flag mask
const LOCK_TYPE_MASK: u32 = 0x00400000;
+    /// Allows getting the least significant 16 bits representing the CSV value.
+    const VALUE_MASK: u32 = 0x0000FFFF;

/// Retuns `true` if the sequence number is 0xffffffff.
#[inline]
@@ -279,6 +282,19 @@ impl Sequence {
self.is_relative_lock_time() & (self.0 & Sequence::LOCK_TYPE_MASK > 0)
}

+    /// Returns true if both sequence numbers are timelocks (i.e. enabled) and both are the same
+    /// type i.e., both height based or both time based.
+    pub fn is_same_type_locked(&self, other: Sequence) -> bool {
+        if self.is_time_locked() && other.is_time_locked() {
+            return true;
+        }
+        if self.is_height_locked() && other.is_height_locked() {
+            return true;
+        }
+        // Either one is disabled or they are different lock types.
+        false
+    }
+
/// Create a relative lock-time using block height.
#[inline]
pub fn from_height(height: u16) -> Self {
@@ -297,6 +313,24 @@ impl Sequence {
!self.is_final()
}

+    /// Returns the sixteen least significant bits representing a timelock value.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if [`Sequence`] is disabled.
+    pub fn value(&self) -> Result<u16, SequenceError> {
+        if !self.is_relative_lock_time() {
+            return Err(SequenceError::Disabled);
+        }
+        Ok(self.value_unchecked())
+    }
+
+    /// Returns the sixteen least significant bits representing a timelock value. Caller is expected
+    /// to have checked that the values are comparable by first calling [`is_same_unit_timelock`].
+    pub fn value_unchecked(&self) -> u16 {
+        (self.0 & Sequence::VALUE_MASK) as u16
+    }
+
/// Create a sequence from a u32 value.
#[inline]
pub fn from_consensus(n: u32) -> Self {
@@ -322,6 +356,42 @@ impl From<Sequence> for u32 {
}
}

+impl fmt::Display for Sequence {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.0)
+    }
+}
+
+/// Catchall type for errors that relate to sequences.
+#[derive(Debug, Clone, PartialEq, Eq)]
+#[non_exhaustive]
+pub enum SequenceError {
+    /// Attempted to use sequence but it is disabled.
+    Disabled,
+}
+
+impl fmt::Display for SequenceError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use self::SequenceError::*;
+
+        match *self {
+            Disabled => write!(f, "attempted to use sequence but it is disabled"),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
+impl std::error::Error for SequenceError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use self::SequenceError::*;
+
+        match *self {
+            Disabled => None,
+        }
+    }
+}
+
/// Bitcoin transaction output.
///
/// Defines new coins to be created as a result of the transaction,
diff --git a/src/lib.rs b/src/lib.rs
index 2bb103fa..a67af7b0 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -102,6 +102,7 @@ pub use crate::blockdata::block::Block;
pub use crate::blockdata::block::BlockHeader;
pub use crate::blockdata::locktime::{self, LockTime, PackedLockTime};
pub use crate::blockdata::script::Script;
+pub use crate::blockdata::transaction::Sequence;
pub use crate::blockdata::transaction::Transaction;
pub use crate::blockdata::transaction::TxIn;
pub use crate::blockdata::transaction::TxOut;

(The error could be a struct instead of an enum?)

nlanson · 2022-07-13T02:25:21Z

accidentally closed on force push

Kixunil

Found some more things to polish, not critical though.

src/blockdata/transaction.rs

Kixunil · 2022-07-14T12:14:14Z

src/blockdata/transaction.rs

+/// Bitcoin transaction input sequencing number.
+/// 
+/// The sequence field is used for:
+/// - Indicating whether [BIP-65] absolute lock-times are enabled.


Just realized this is confusing because lock times are ancient and the behavior was implemented before BIPs existed. BIP-65 is somewhat related but not the entire specification.

I suggest removing the reference from that phrase and perhaps put below something like "Note that transactions spending an output with OP_CHECKLOCKTIMEVERIFY MUST NOT use Sequence::MAX for the corresponding input. (BIP-65)."

Also "absolute" is not common terminology, so maybe write "Indicating whether the absolute lock-time (specified in lock_time field of Transaction) is enabled." Singular because there's only one lock time for transaction.

src/blockdata/transaction.rs

nlanson · 2022-07-15T02:27:14Z

RE: Sequence::ENABLE_RBF_NO_LOCKTIME and Sequence::ENABLE_LOCKTIME_NO_RBF

I'm finding the names misleading because Sequence::ENABLE_RBF_NO_LOCKTIME has absolute lock-time enabled and Sequence::ENABLE_LOCKTIME_NO_RBF has relative lock-time disabled. There is no consistency in which lock-time is enabled/disabled with the constants.

I have got what is enabled/disabled written in both docs but still not sure if this is enough.
However, I think the confusion will be lessened by the RelativeLockTime enum that will be added later on.

tcharding · 2022-07-15T05:37:54Z

What! Confusion surrounding timelock terminology - outrageous :)

nlanson · 2022-07-15T11:45:08Z

added absolute and relative prefixes in docs where they were missing

Kixunil

ACK cd5874c

The nit can be fixed later.

src/blockdata/transaction.rs

apoelstra · 2022-07-15T20:43:18Z

This is really great! I have been ignoring this PR because there's been a ton of active discussion. Now that I look at the result, it's really ambitious and (I think) correct.

I had previously argued that we couldn't newtype Sequence since it does so many things that it's basically a raw u32 ... but I guess I've been proven wrong :)

apoelstra

ACK cd5874c

nlanson · 2022-07-16T00:48:06Z

fixed nits.

Kixunil

ACK e34bc53

tcharding

ACK e34bc53

apoelstra

ACK e34bc53

e34bc53 Add new type for sequence (Noah Lanson) Pull request description: #1082 Created a new type for txin sequence field with methods to create sequences with relative time locks from block height or time units. ACKs for top commit: Kixunil: ACK e34bc53 tcharding: ACK e34bc53 apoelstra: ACK e34bc53 Tree-SHA512: 6605349d0312cc36ef9a4632f954e59265b3ba5cfd437aa88a37672fe479688aa4a3eff474902f8cc55848efe55caf3f09f321b3a62417842bfc3ec365c40688

darosior

Nice. Thanks for working on that, that'll be helpful for me downstream! :)

darosior · 2022-10-13T11:30:39Z

src/blockdata/transaction.rs

+    /// Returns `true` if the sequence has a relative lock-time.
+    #[inline]
+    pub fn is_relative_lock_time(&self) -> bool {
+        self.0 & Sequence::LOCK_TIME_DISABLE_FLAG_MASK == 0


Any reason for ruling out 0 block (or time) relative timelocks?`

It doesn't rule them out. 0 & x == 0 holds for all x.

I misread the & for a && 🤦. That wouldn't even make sense as it would always be false. Thanks!

That wouldn't even compile since they are not bools. :)

darosior · 2022-10-13T11:31:59Z

src/blockdata/transaction.rs

+
+    /// Create a relative lock-time using block height.
+    #[inline]
+    pub fn from_height(height: u16) -> Self {


nlanson force-pushed the sequence branch from a4ff7c0 to 801d6c6 Compare July 11, 2022 03:43

tcharding reviewed Jul 11, 2022

View reviewed changes

nlanson force-pushed the sequence branch from 801d6c6 to f7e24a5 Compare July 11, 2022 05:59

Kixunil reviewed Jul 11, 2022

View reviewed changes

nlanson force-pushed the sequence branch 2 times, most recently from 2c56750 to 97b5afa Compare July 12, 2022 01:18

nlanson marked this pull request as ready for review July 12, 2022 01:29

tcharding reviewed Jul 12, 2022

View reviewed changes

src/blockdata/transaction.rs Show resolved Hide resolved

src/blockdata/transaction.rs Show resolved Hide resolved

nlanson force-pushed the sequence branch 4 times, most recently from 9083777 to 478d513 Compare July 12, 2022 10:31

Kixunil reviewed Jul 12, 2022

View reviewed changes

tcharding mentioned this pull request Jul 13, 2022

Add a LockTime type #994

Merged

tcharding mentioned this pull request Jul 13, 2022

WIP: Demonstrate the new timelock APIs rust-bitcoin/rust-miniscript#446

Closed

nlanson closed this Jul 13, 2022

nlanson force-pushed the sequence branch from 478d513 to 97c680d Compare July 13, 2022 02:24

nlanson reopened this Jul 13, 2022

nlanson force-pushed the sequence branch 2 times, most recently from 026bdef to 7002299 Compare July 13, 2022 02:41

Kixunil reviewed Jul 14, 2022

View reviewed changes

nlanson force-pushed the sequence branch 2 times, most recently from 5cecaec to 7c1565d Compare July 15, 2022 02:17

nlanson force-pushed the sequence branch from 7c1565d to cd5874c Compare July 15, 2022 11:40

Kixunil previously approved these changes Jul 15, 2022

View reviewed changes

src/blockdata/transaction.rs Outdated Show resolved Hide resolved

apoelstra reviewed Jul 15, 2022

View reviewed changes

src/blockdata/transaction.rs Outdated Show resolved Hide resolved

apoelstra previously approved these changes Jul 15, 2022

View reviewed changes

nlanson dismissed stale reviews from apoelstra and Kixunil via e34bc53 July 16, 2022 00:47

nlanson force-pushed the sequence branch from cd5874c to e34bc53 Compare July 16, 2022 00:47

Add new type for sequence

e34bc53

Kixunil approved these changes Jul 16, 2022

View reviewed changes

tcharding approved these changes Jul 17, 2022

View reviewed changes

apoelstra approved these changes Jul 17, 2022

View reviewed changes

apoelstra merged commit 5d9f564 into rust-bitcoin:master Jul 17, 2022

nlanson deleted the sequence branch July 18, 2022 07:54

0xB10C mentioned this pull request Jul 19, 2022

Add BIP152 (Compact Blocks) structures #1088

Merged

nlanson mentioned this pull request Jul 21, 2022

Make sequence a newtype #1082

Closed

nlanson mentioned this pull request Jul 28, 2022

RelativeLockTime type #1140

Closed

Kixunil added enhancement API break This PR requires a version bump for the next release 1.0 Issues and PRs required or helping to stabilize the API labels Aug 1, 2022

Kixunil added this to the 0.29.0 milestone Aug 1, 2022

darosior reviewed Oct 13, 2022

View reviewed changes

Add new type for sequence #1093

Add new type for sequence #1093

Conversation

nlanson commented Jul 11, 2022 • edited

nlanson commented Jul 11, 2022

tcharding left a 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

nlanson Jul 12, 2022 • edited

Choose a reason for hiding this comment

tcharding commented Jul 11, 2022

nlanson commented Jul 11, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Jul 11, 2022

nlanson commented Jul 12, 2022

tcharding left a comment • edited

Choose a reason for hiding this comment

Kixunil commented Jul 12, 2022

Kixunil left a 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

nlanson Jul 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Jul 13, 2022

tcharding commented Jul 13, 2022

nlanson commented Jul 13, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlanson commented Jul 15, 2022 • edited

tcharding commented Jul 15, 2022

nlanson commented Jul 15, 2022

Kixunil left a comment

Choose a reason for hiding this comment

apoelstra commented Jul 15, 2022

apoelstra left a comment

Choose a reason for hiding this comment

nlanson commented Jul 16, 2022

Kixunil left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

darosior left a 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

nlanson commented Jul 11, 2022 •

edited

nlanson Jul 12, 2022 •

edited

tcharding left a comment •

edited

nlanson Jul 13, 2022 •

edited

nlanson commented Jul 15, 2022 •

edited