diff --git a/src/descriptor/policy.rs b/src/descriptor/policy.rs index a2085fe6..2ba9fd06 100644 --- a/src/descriptor/policy.rs +++ b/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 { - const BLOCKS_TIMELOCK_THRESHOLD: u32 = 500000000; + fn merge_nlocktime(a: u32, b: u32) -> Result { + 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 { + 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 { 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, _ => {} } diff --git a/src/error.rs b/src/error.rs index 8273e0e7..0df0e697 100644 --- a/src/error.rs +++ b/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 { diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 8621d192..ecf14dc3 100644 --- a/src/wallet/mod.rs +++ b/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, - (Some(rbf), _) if rbf >= 0xFFFFFFFE => return Err(Error::Generic("Cannot enable RBF with a nSequence >= 0xFFFFFFFE".into())), - (Some(rbf), _) => rbf, - (None, _) => 0xFFFFFFFF, + + // RBF with a specific value but that value is too high + (Some(tx_builder::RBFValue::Value(rbf)), _) if rbf >= 0xFFFFFFFE => { + return Err(Error::Generic( + "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()); diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs index aad24899..5cf6ffe8 100644 --- a/src/wallet/tx_builder.rs +++ b/src/wallet/tx_builder.rs @@ -85,7 +85,7 @@ pub struct TxBuilder, Ctx: TxBuilderC pub(crate) sighash: Option, pub(crate) ordering: TxOrdering, pub(crate) locktime: Option, - pub(crate) rbf: Option, + pub(crate) rbf: Option, pub(crate) version: Option, pub(crate) change_policy: ChangeSpendPolicy, pub(crate) force_non_witness_utxo: bool, @@ -451,8 +451,9 @@ impl> TxBuilder { /// Enable signaling RBF /// /// This will use the default nSequence value of `0xFFFFFFFD`. - pub fn enable_rbf(self) -> Self { - self.enable_rbf_with_sequence(0xFFFFFFFD) + pub fn enable_rbf(mut self) -> Self { + self.rbf = Some(RBFValue::Default); + self } /// Enable signaling RBF with a specific nSequence value @@ -463,7 +464,7 @@ impl> TxBuilder { /// 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 { diff --git a/src/wallet/utils.rs b/src/wallet/utils.rs index 7eee7a60..78a94199 100644 --- a/src/wallet/utils.rs +++ b/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> Satisfier for After { fn check_after(&self, n: u32) -> bool { if let Some(current_height) = self.current_height {