[wallet] Set the correct nSequence when RBF and OP_CSV are used

This commit also fixes the timelock comparing logic in the policy module, since
the rules are different for absolute (OP_CLTV) and relative (OP_CSV) timelocks.

Fixes #215

src/descriptor/policy.rs

@@ -66,7 +66,7 @@ use log::{debug, error, info, trace};
 
 use crate::descriptor::ExtractPolicy;
 use crate::wallet::signer::{SignerId, SignersContainer};
-use crate::wallet::utils::{descriptor_to_pk_ctx, SecpCtx};
+use crate::wallet::utils::{self, descriptor_to_pk_ctx, SecpCtx};
 
 use super::checksum::get_checksum;
 use super::error::Error;
@@ -462,10 +462,21 @@ pub struct Condition {
 }
 
 impl Condition {
-    fn merge_timelock(a: u32, b: u32) -> Result<u32, PolicyError> {
+    fn merge_nlocktime(a: u32, b: u32) -> Result<u32, PolicyError> {
-        const BLOCKS_TIMELOCK_THRESHOLD: u32 = 500000000;
+        if (a < utils::BLOCKS_TIMELOCK_THRESHOLD) != (b < utils::BLOCKS_TIMELOCK_THRESHOLD) {
+            Err(PolicyError::MixedTimelockUnits)
+        } else {
+            Ok(max(a, b))
+        }
+    }
 
-        if (a < BLOCKS_TIMELOCK_THRESHOLD) != (b < BLOCKS_TIMELOCK_THRESHOLD) {
+    fn merge_nsequence(a: u32, b: u32) -> Result<u32, PolicyError> {
+        let mask = utils::SEQUENCE_LOCKTIME_TYPE_FLAG | utils::SEQUENCE_LOCKTIME_MASK;
+
+        let a = a & mask;
+        let b = b & mask;
+
+        if (a < utils::SEQUENCE_LOCKTIME_TYPE_FLAG) != (b < utils::SEQUENCE_LOCKTIME_TYPE_FLAG) {
             Err(PolicyError::MixedTimelockUnits)
         } else {
             Ok(max(a, b))
@@ -474,13 +485,13 @@ impl Condition {
 
     pub(crate) fn merge(mut self, other: &Condition) -> Result<Self, PolicyError> {
         match (self.csv, other.csv) {
-            (Some(a), Some(b)) => self.csv = Some(Self::merge_timelock(a, b)?),
+            (Some(a), Some(b)) => self.csv = Some(Self::merge_nsequence(a, b)?),
             (None, any) => self.csv = any,
             _ => {}
         }
 
         match (self.timelock, other.timelock) {
-            (Some(a), Some(b)) => self.timelock = Some(Self::merge_timelock(a, b)?),
+            (Some(a), Some(b)) => self.timelock = Some(Self::merge_nlocktime(a, b)?),
             (None, any) => self.timelock = any,
             _ => {}
         }

src/error.rs

@@ -60,7 +60,7 @@ pub enum Error {
     TransactionNotFound,
     /// Happens when trying to bump a transaction that is already confirmed
     TransactionConfirmed,
-    /// Trying to replace a tx that has a sequence = `0xFFFFFFFF`
+    /// Trying to replace a tx that has a sequence >= `0xFFFFFFFE`
     IrreplaceableTransaction,
     /// When bumping a tx the fee rate requested is lower than required
     FeeRateTooLow {

src/wallet/mod.rs

@@ -63,7 +63,7 @@ pub use utils::IsDust;
 use address_validator::AddressValidator;
 use signer::{Signer, SignerId, SignerOrdering, SignersContainer};
 use tx_builder::{BumpFee, CreateTx, FeePolicy, TxBuilder, TxBuilderContext};
-use utils::{descriptor_to_pk_ctx, After, Older, SecpCtx};
+use utils::{check_nlocktime, check_nsequence_rbf, descriptor_to_pk_ctx, After, Older, SecpCtx};
 
 use crate::blockchain::{Blockchain, BlockchainMarker, OfflineBlockchain, Progress};
 use crate::database::{BatchDatabase, BatchOperations, DatabaseUtils};
@@ -330,19 +330,48 @@ where
         };
 
         let lock_time = match builder.locktime {
+            // No nLockTime, default to 0
             None => requirements.timelock.unwrap_or(0),
+            // Specific nLockTime required and we have no constraints, so just set to that value
             Some(x) if requirements.timelock.is_none() => x,
-            Some(x) if requirements.timelock.unwrap() <= x => x,
+            // Specific nLockTime required and it's compatible with the constraints
+            Some(x) if check_nlocktime(x, requirements.timelock.unwrap()) => x,
+            // Invalid nLockTime required
             Some(x) => return Err(Error::Generic(format!("TxBuilder requested timelock of `{}`, but at least `{}` is required to spend from this script", x, requirements.timelock.unwrap())))
         };
 
         let n_sequence = match (builder.rbf, requirements.csv) {
+            // No RBF or CSV but there's an nLockTime, so the nSequence cannot be final
+            (None, None) if lock_time != 0 => 0xFFFFFFFE,
+            // No RBF, CSV or nLockTime, make the transaction final
+            (None, None) => 0xFFFFFFFF,
+
+            // No RBF requested, use the value from CSV. Note that this value is by definition
+            // non-final, so even if a timelock is enabled this nSequence is fine, hence why we
+            // don't bother checking for it here. The same is true for all the other branches below
             (None, Some(csv)) => csv,
-            (Some(rbf), Some(csv)) if rbf < csv => return Err(Error::Generic(format!("Cannot enable RBF with nSequence `{}`, since at least `{}` is required to spend with OP_CSV", rbf, csv))),
+
-            (None, _) if requirements.timelock.is_some() => 0xFFFFFFFE,
+            // RBF with a specific value but that value is too high
-            (Some(rbf), _) if rbf >= 0xFFFFFFFE => return Err(Error::Generic("Cannot enable RBF with a nSequence >= 0xFFFFFFFE".into())),
+            (Some(tx_builder::RBFValue::Value(rbf)), _) if rbf >= 0xFFFFFFFE => {
-            (Some(rbf), _) => rbf,
+                return Err(Error::Generic(
-            (None, _) => 0xFFFFFFFF,
+                    "Cannot enable RBF with a nSequence >= 0xFFFFFFFE".into(),
+                ))
+            }
+            // RBF with a specific value requested, but the value is incompatible with CSV
+            (Some(tx_builder::RBFValue::Value(rbf)), Some(csv))
+                if !check_nsequence_rbf(rbf, csv) =>
+            {
+                return Err(Error::Generic(format!(
+                    "Cannot enable RBF with nSequence `{}` given a required OP_CSV of `{}`",
+                    rbf, csv
+                )))
+            }
+
+            // RBF enabled with the default value with CSV also enabled. CSV takes precedence
+            (Some(tx_builder::RBFValue::Default), Some(csv)) => csv,
+            // Valid RBF, either default or with a specific value. We ignore the `CSV` value
+            // because we've already checked it before
+            (Some(rbf), _) => rbf.get_value(),
         };
 
         let mut tx = Transaction {
@@ -1712,12 +1741,14 @@ mod test {
             )
             .unwrap();
 
-        assert_eq!(psbt.global.unsigned_tx.input[0].sequence, 0xFFFFFFFD);
+        // When CSV is enabled it takes precedence over the rbf value (unless forced by the user).
+        // It will be set to the OP_CSV value, in this case 6
+        assert_eq!(psbt.global.unsigned_tx.input[0].sequence, 6);
     }
 
     #[test]
     #[should_panic(
-        expected = "Cannot enable RBF with nSequence `3`, since at least `6` is required to spend with OP_CSV"
+        expected = "Cannot enable RBF with nSequence `3` given a required OP_CSV of `6`"
     )]
     fn test_create_tx_with_custom_rbf_csv() {
         let (wallet, _, _) = get_funded_wallet(get_test_single_sig_csv());

src/wallet/tx_builder.rs

@@ -85,7 +85,7 @@ pub struct TxBuilder<D: Database, Cs: CoinSelectionAlgorithm<D>, Ctx: TxBuilderC
     pub(crate) sighash: Option<SigHashType>,
     pub(crate) ordering: TxOrdering,
     pub(crate) locktime: Option<u32>,
-    pub(crate) rbf: Option<u32>,
+    pub(crate) rbf: Option<RBFValue>,
     pub(crate) version: Option<Version>,
     pub(crate) change_policy: ChangeSpendPolicy,
     pub(crate) force_non_witness_utxo: bool,
@@ -451,8 +451,9 @@ impl<D: Database, Cs: CoinSelectionAlgorithm<D>> TxBuilder<D, Cs, CreateTx> {
     /// Enable signaling RBF
     ///
     /// This will use the default nSequence value of `0xFFFFFFFD`.
-    pub fn enable_rbf(self) -> Self {
+    pub fn enable_rbf(mut self) -> Self {
-        self.enable_rbf_with_sequence(0xFFFFFFFD)
+        self.rbf = Some(RBFValue::Default);
+        self
     }
 
     /// Enable signaling RBF with a specific nSequence value
@@ -463,7 +464,7 @@ impl<D: Database, Cs: CoinSelectionAlgorithm<D>> TxBuilder<D, Cs, CreateTx> {
     /// If the `nsequence` is higher than `0xFFFFFFFD` an error will be thrown, since it would not
     /// be a valid nSequence to signal RBF.
     pub fn enable_rbf_with_sequence(mut self, nsequence: u32) -> Self {
-        self.rbf = Some(nsequence);
+        self.rbf = Some(RBFValue::Value(nsequence));
         self
     }
 }
@@ -545,6 +546,24 @@ impl Default for Version {
     }
 }
 
+/// RBF nSequence value
+///
+/// Has a default value of `0xFFFFFFFD`
+#[derive(Debug, Ord, PartialOrd, Eq, PartialEq, Hash, Clone, Copy)]
+pub(crate) enum RBFValue {
+    Default,
+    Value(u32),
+}
+
+impl RBFValue {
+    pub(crate) fn get_value(&self) -> u32 {
+        match self {
+            RBFValue::Default => 0xFFFFFFFD,
+            RBFValue::Value(v) => *v,
+        }
+    }
+}
+
 /// Policy regarding the use of change outputs when creating a transaction
 #[derive(Debug, Ord, PartialOrd, Eq, PartialEq, Hash, Clone, Copy)]
 pub enum ChangeSpendPolicy {

src/wallet/utils.rs

@@ -31,6 +31,18 @@ use miniscript::{MiniscriptKey, Satisfier, ToPublicKey};
 // De-facto standard "dust limit" (even though it should change based on the output type)
 const DUST_LIMIT_SATOSHI: u64 = 546;
 
+// MSB of the nSequence. If set there's no consensus-constraint, so it must be disabled when
+// spending using CSV in order to enforce CSV rules
+pub(crate) const SEQUENCE_LOCKTIME_DISABLE_FLAG: u32 = 1 << 31;
+// When nSequence is lower than this flag the timelock is interpreted as block-height-based,
+// otherwise it's time-based
+pub(crate) const SEQUENCE_LOCKTIME_TYPE_FLAG: u32 = 1 << 22;
+// Mask for the bits used to express the timelock
+pub(crate) const SEQUENCE_LOCKTIME_MASK: u32 = 0x0000FFFF;
+
+// Threshold for nLockTime to be considered a block-height-based timelock rather than time-based
+pub(crate) const BLOCKS_TIMELOCK_THRESHOLD: u32 = 500000000;
+
 /// Trait to check if a value is below the dust limit
 // we implement this trait to make sure we don't mess up the comparison with off-by-one like a <
 // instead of a <= etc. The constant value for the dust limit is not public on purpose, to
@@ -60,6 +72,49 @@ impl After {
     }
 }
 
+pub(crate) fn check_nsequence_rbf(rbf: u32, csv: u32) -> bool {
+    // This flag cannot be set in the nSequence when spending using OP_CSV
+    if rbf & SEQUENCE_LOCKTIME_DISABLE_FLAG != 0 {
+        return false;
+    }
+
+    // The nSequence value must be >= the O_CSV
+    if rbf < csv {
+        return false;
+    }
+
+    let mask = SEQUENCE_LOCKTIME_TYPE_FLAG | SEQUENCE_LOCKTIME_MASK;
+    let rbf = rbf & mask;
+    let csv = csv & mask;
+
+    // Both values should be represented in the same unit (either time-based or
+    // block-height based)
+    if (rbf < SEQUENCE_LOCKTIME_TYPE_FLAG) != (csv < SEQUENCE_LOCKTIME_TYPE_FLAG) {
+        return false;
+    }
+
+    // The value should be at least `csv`
+    if rbf < csv {
+        return false;
+    }
+
+    true
+}
+
+pub(crate) fn check_nlocktime(nlocktime: u32, required: u32) -> bool {
+    // Both values should be expressed in the same unit
+    if (nlocktime < BLOCKS_TIMELOCK_THRESHOLD) != (required < BLOCKS_TIMELOCK_THRESHOLD) {
+        return false;
+    }
+
+    // The value should be at least `required`
+    if nlocktime < required {
+        return false;
+    }
+
+    true
+}
+
 impl<ToPkCtx: Copy, Pk: MiniscriptKey + ToPublicKey<ToPkCtx>> Satisfier<ToPkCtx, Pk> for After {
     fn check_after(&self, n: u32) -> bool {
         if let Some(current_height) = self.current_height {