From: 志宇 Date: Thu, 10 Sep 2026 10:56:17 +0000 (+0000) Subject: fix(chain): compute per-tx MTP at `confirmation_height - 1` (BIP-68) X-Git-Tag: pre-fold-backup~3 X-Git-Url: http://internal-gitweb-vhost/-script/src/example_cli/database/encode/struct.EncoderStringWriter.html?a=commitdiff_plain;h=d2e49cc5f59786e511c7426407c884505ecd7f7f;p=bdk fix(chain): compute per-tx MTP at `confirmation_height - 1` (BIP-68) `CanonicalTx::mtp` was the MTP *at* the confirmation height. That is not the value BIP-68 evaluates relative timelocks against: Bitcoin Core's `CalculateSequenceLocks` uses GetAncestor(std::max(nCoinHeight - 1, 0))->GetMedianTimePast() i.e. the MTP of the block *preceding* the coin's height. Compute MTP at `confirmation_height_upper_bound().saturating_sub(1)` (`saturating_sub` matching Core's `max(…, 0)`) and rename the field to `prev_mtp` on both `CanonicalTx` and `CanonicalTxOut`, so the name says which block the value belongs to and a stale caller fails to compile rather than silently reading a different height. `Canonical::tip_mtp` is unchanged — MTP(tip.height) is the correct BIP-113 reference for absolute timelocks. Spell out what `None` means on the renamed field, since it is easy to read as "MTP was off": it also covers an unconfirmed tx, a tx confirmed only transitively through a descendant's anchor (whose own confirmation height genuinely isn't known), and a window the chain cannot fully resolve. A confirmed tx can therefore be `None` even with MTP enabled, and a caller checking a timelock must not treat that as a satisfied lock. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NND8BhEmYTYdLRv4dbUUro --- diff --git a/crates/chain/src/canonical.rs b/crates/chain/src/canonical.rs index 02133dec..c9183885 100644 --- a/crates/chain/src/canonical.rs +++ b/crates/chain/src/canonical.rs @@ -38,7 +38,7 @@ use crate::{spk_txout::SpkTxOutIndex, Anchor, Balance, CanonicalViewTask, ChainP pub(crate) struct CanonicalEntry

{ pub(crate) tx: Arc, pub(crate) pos: P, - pub(crate) mtp: Option, + pub(crate) prev_mtp: Option, } /// A single canonical transaction with its position. @@ -59,11 +59,26 @@ pub struct CanonicalTx

{ pub txid: Txid, /// The full transaction. pub tx: Arc, - /// The median-time-past at the confirmation height, if computed. - /// - /// This is `Some` only when the transaction is confirmed and MTP computation was - /// enabled via [`CanonicalViewTask::with_mtp`](crate::CanonicalViewTask::with_mtp). - pub mtp: Option, + /// The median-time-past of the block *preceding* the confirmation height, if computed. + /// + /// This is the MTP at `confirmation_height - 1` (saturating at genesis), which is the + /// reference value used to evaluate BIP-68 relative timelocks on spends of this + /// transaction's outputs — matching Bitcoin Core's `CalculateSequenceLocks`. It is *not* + /// the MTP at the confirmation height itself. + /// + /// `None` means the value is unknown, which happens whenever any of these hold: + /// + /// - MTP computation was not enabled — see + /// [`CanonicalViewTask::with_mtp`](crate::CanonicalViewTask::with_mtp). + /// - The transaction is not confirmed. + /// - The transaction is confirmed only *transitively* — through a descendant's anchor, so + /// [`pos`](Self::pos) is [`Confirmed`](crate::ChainPosition::Confirmed) with + /// `transitively: Some(_)`. Its own confirmation height genuinely isn't known. + /// - The chain could not resolve the full 11-block window the MTP medians over. + /// + /// So a confirmed transaction can have `None` here even with MTP enabled; do not read + /// `None` as "MTP was off", and do not treat it as an absent or satisfied timelock. + pub prev_mtp: Option, } impl Ord for CanonicalTx

{ @@ -98,11 +113,17 @@ pub struct CanonicalTxOut

{ pub spent_by: Option<(P, Txid)>, /// Whether this output is on a coinbase transaction. pub is_on_coinbase: bool, - /// The median-time-past at the confirmation height, if computed. + /// The median-time-past of the block *preceding* the confirmation height, if computed. + /// + /// This is the MTP at `confirmation_height - 1` (saturating at genesis), which is the + /// reference value used to evaluate BIP-68 relative timelocks on spends of this output — + /// matching Bitcoin Core's `CalculateSequenceLocks`. It is *not* the MTP at the + /// confirmation height itself. /// - /// This is `Some` only when the output's transaction is confirmed and MTP computation - /// was enabled via [`CanonicalViewTask::with_mtp`](crate::CanonicalViewTask::with_mtp). - pub mtp: Option, + /// `None` means unknown, not "no timelock" — see + /// [`CanonicalTx::prev_mtp`](crate::CanonicalTx::prev_mtp) for the cases that produce it, + /// which include an output whose transaction is confirmed only transitively. + pub prev_mtp: Option, } impl Ord for CanonicalTxOut

{ @@ -270,7 +291,7 @@ impl Canonical { pos: entry.pos.clone(), txid, tx: entry.tx.clone(), - mtp: entry.mtp, + prev_mtp: entry.prev_mtp, }) } @@ -297,7 +318,7 @@ impl Canonical { txout: txout.clone(), spent_by, is_on_coinbase: entry.tx.is_coinbase(), - mtp: entry.mtp, + prev_mtp: entry.prev_mtp, }) } @@ -331,7 +352,7 @@ impl Canonical { pos: entry.pos.clone(), txid, tx: entry.tx.clone(), - mtp: entry.mtp, + prev_mtp: entry.prev_mtp, } }) } diff --git a/crates/chain/src/canonical_task.rs b/crates/chain/src/canonical_task.rs index 5ee4782a..46b99c3d 100644 --- a/crates/chain/src/canonical_task.rs +++ b/crates/chain/src/canonical_task.rs @@ -341,7 +341,7 @@ impl<'g, A: Anchor, B: ToBlockHash> CanonicalTask<'g, A, B> { canonical_entry.insert(CanonicalEntry { tx: tx.clone(), pos: this_reason, - mtp: None, + prev_mtp: None, }); Some(this_txid) }, @@ -596,10 +596,13 @@ mod tests { ChainPosition::Confirmed { .. } )); - // Per-tx MTP at height 5: median of timestamps at heights 0..=5 - // Timestamps: 100, 200, 300, 400, 500, 600 → sorted, median = ts[len/2] = ts[3] = 400 + // Per-tx `prev_mtp` for a tx confirmed at height 5: the MTP of the block *preceding* + // the confirmation height (BIP-68), i.e. MTP at height 4 = median of timestamps at + // heights 0..=4. + // Timestamps: 100, 200, 300, 400, 500 → sorted, median = ts[len/2] = ts[2] = 300. + // (Was 400 when the buggy code medianed heights 0..=5 at the confirmation height itself.) let canon_tx = view.txs().next().unwrap(); - assert_eq!(canon_tx.mtp, Some(400)); + assert_eq!(canon_tx.prev_mtp, Some(300)); // MTP at tip height (11): median of timestamps at heights 1..=11 // Timestamps: 200, 300, 400, 500, 600, 700, 800, 900, 1000, 1100, 1200 diff --git a/crates/chain/src/canonical_view_task.rs b/crates/chain/src/canonical_view_task.rs index 9cb46672..2c864ab0 100644 --- a/crates/chain/src/canonical_view_task.rs +++ b/crates/chain/src/canonical_view_task.rs @@ -110,10 +110,12 @@ impl<'g, A: Anchor, B> CanonicalViewTask<'g, A, B> { impl<'g, A: Anchor, B: ToBlockHash + ToBlockTime> CanonicalViewTask<'g, A, B> { /// Enable MTP (median-time-past) computation. /// - /// When enabled, the task will fetch additional blocks needed to compute MTP values - /// for each confirmed height and the tip height. The resulting [`CanonicalView`] will - /// have per-tx MTP on [`CanonicalTx::mtp`](crate::CanonicalTx::mtp) and the tip MTP - /// accessible via [`tip_mtp()`](crate::Canonical::tip_mtp). + /// When enabled, the task will fetch additional blocks needed to compute MTP values. + /// For each confirmed tx it computes the MTP of the block *preceding* the confirmation + /// height (per BIP-68, matching Bitcoin Core's `CalculateSequenceLocks`), exposed on + /// [`CanonicalTx::prev_mtp`](crate::CanonicalTx::prev_mtp). For the tip it computes + /// MTP(tip.height) (per BIP-113), accessible via + /// [`tip_mtp()`](crate::Canonical::tip_mtp). pub fn with_mtp(mut self) -> Self { self.extract_time = Some(B::to_blocktime); self @@ -166,10 +168,15 @@ impl<'g, A: Anchor, B: ToBlockHash> ChainTask for CanonicalViewTask<'g, A, B> } // Collect all MTP heights needed (an 11-block window per target height). + // + // For the tip we need MTP(tip.height) (BIP-113). For each confirmed tx we need + // the MTP of the block *preceding* the confirmation height (BIP-68), so we push + // `confirmation_height - 1` (saturating at genesis) — the same value read in + // `finish`. let mut mtp_heights = Vec::::new(); mtp_heights.push(self.tip.height); for anchor in self.direct_anchors.values() { - mtp_heights.push(anchor.confirmation_height_upper_bound()); + mtp_heights.push(anchor.confirmation_height_upper_bound().saturating_sub(1)); } let required: Vec = mtp_heights .into_iter() @@ -242,9 +249,16 @@ impl<'g, A: Anchor, B: ToBlockHash> ChainTask for CanonicalViewTask<'g, A, B> anchor: anchor.clone(), transitively: None, }; - let mtp = extract_time - .and_then(|f| compute_mtp_at(anchor.confirmation_height_upper_bound(), f)); - view_txs.insert(*txid, CanonicalEntry { tx, pos, mtp }); + // BIP-68 evaluates relative timelocks against the MTP of the block *before* the + // coin's height (Core's `std::max(nCoinHeight - 1, 0)`), so compute MTP at + // `confirmation_height - 1`, saturating to genesis for coins at height 0. + let prev_mtp = extract_time.and_then(|f| { + compute_mtp_at( + anchor.confirmation_height_upper_bound().saturating_sub(1), + f, + ) + }); + view_txs.insert(*txid, CanonicalEntry { tx, pos, prev_mtp }); continue; } @@ -295,8 +309,11 @@ impl<'g, A: Anchor, B: ToBlockHash> ChainTask for CanonicalViewTask<'g, A, B> CanonicalEntry { tx: tx.clone(), pos, - // No MTP since we do not know confirmation height. - mtp: None, + // No MTP: this stage handles txs without a direct anchor, so even when the + // position is `Confirmed` (transitively, via a descendant's anchor) this + // tx's own confirmation height is unknown and there is no block to median + // around. + prev_mtp: None, }, ); } diff --git a/crates/chain/src/local_chain.rs b/crates/chain/src/local_chain.rs index e8636539..0ff942ff 100644 --- a/crates/chain/src/local_chain.rs +++ b/crates/chain/src/local_chain.rs @@ -474,9 +474,14 @@ where /// /// Requires `D: ToBlockTime` (e.g. `LocalChain

`). /// - /// The resulting [`CanonicalView`] will have per-tx MTP on - /// [`CanonicalTx::mtp`](crate::CanonicalTx::mtp) and the tip MTP accessible via + /// For each confirmed transaction this computes the MTP of the block *preceding* the + /// confirmation height (the BIP-68 reference for relative timelocks), exposed on + /// [`CanonicalTx::prev_mtp`](crate::CanonicalTx::prev_mtp). For the tip it computes + /// MTP(tip.height) (the BIP-113 reference for absolute timelocks), accessible via /// [`tip_mtp()`](crate::Canonical::tip_mtp). + /// + /// See [`CanonicalViewTask::with_mtp`](crate::CanonicalViewTask::with_mtp) for the block + /// data this requires. pub fn canonicalize_with_mtp( &self, tx_graph: &TxGraph, diff --git a/crates/chain/tests/test_canonical_view_mtp.rs b/crates/chain/tests/test_canonical_view_mtp.rs index 70c92931..b6e6b499 100644 --- a/crates/chain/tests/test_canonical_view_mtp.rs +++ b/crates/chain/tests/test_canonical_view_mtp.rs @@ -42,6 +42,39 @@ fn chain_with_time_equal_to_height(tip_height: u32) -> LocalChain
{ LocalChain::from_blocks(blocks).expect("chain connects from genesis") } +/// Insert a transaction into `tx_graph` spending a distinct parent (identified by `seed`) and +/// anchor it at `height`. +/// +/// Returns the txid of the inserted transaction. +fn confirm_tx_at( + tx_graph: &mut TxGraph, + chain: &LocalChain
, + seed: u8, + height: u32, +) -> bitcoin::Txid { + let tx = Transaction { + input: vec![TxIn { + previous_output: OutPoint::new(bitcoin::Txid::from_byte_array([seed; 32]), 0), + ..Default::default() + }], + output: vec![TxOut { + value: Amount::from_sat(50_000), + script_pubkey: ScriptBuf::new(), + }], + ..new_tx(1) + }; + let txid = tx.compute_txid(); + let _ = tx_graph.insert_tx(tx); + let _ = tx_graph.insert_anchor( + txid, + ConfirmationBlockTime { + block_id: chain.get(height).unwrap().block_id(), + confirmation_time: 0, + }, + ); + txid +} + #[test] fn canonicalize_with_mtp_computes_median_time_past() { // A 13-block chain (heights 0..=12) where each block's time equals its height. @@ -76,11 +109,62 @@ fn canonicalize_with_mtp_computes_median_time_past() { // Tip is at height 12, so its MTP is the median of heights 2..=12, which is 7. assert_eq!(view.tip_mtp(), Some(7)); - // The transaction is confirmed at height 11, so its MTP is the median of heights 1..=11 == 6. + // The transaction is confirmed at height 11. Its `prev_mtp` is the MTP of the block + // *preceding* the confirmation height (BIP-68), i.e. MTP at height 10 = median of heights + // 0..=10 == 5. (Was 6 when the buggy code medianed heights 1..=11 at height 11 itself.) let canonical_tx = view .txs() .find(|c| c.txid == txid) .expect("tx is canonical"); assert!(matches!(canonical_tx.pos, ChainPosition::Confirmed { .. })); - assert_eq!(canonical_tx.mtp, Some(6)); + assert_eq!(canonical_tx.prev_mtp, Some(5)); +} + +#[test] +fn prev_mtp_saturates_for_low_confirmation_heights() { + // A 13-block chain (heights 0..=12) where each block's time equals its height. + let chain = chain_with_time_equal_to_height(12); + let mut tx_graph = TxGraph::default(); + + // Confirmed at genesis (height 0): `confirmation_height - 1` saturates to 0, matching + // Core's `std::max(nCoinHeight - 1, 0)`. prev_mtp = MTP(0) = median of heights 0..=0 == 0. + let genesis_txid = confirm_tx_at(&mut tx_graph, &chain, 1, 0); + + // Confirmed at height 3: prev_mtp = MTP(2) = median of heights 0..=2 == 1. The MTP window + // start (`2 - 10`) also saturates to 0. (The buggy code would have medianed heights 0..=3 + // at height 3 == 2.) + let low_txid = confirm_tx_at(&mut tx_graph, &chain, 2, 3); + + let view = chain.canonicalize_with_mtp(&tx_graph, chain.tip().block_id(), Default::default()); + + let genesis_tx = view + .txs() + .find(|c| c.txid == genesis_txid) + .expect("tx is canonical"); + assert_eq!(genesis_tx.prev_mtp, Some(0)); + + let low_tx = view + .txs() + .find(|c| c.txid == low_txid) + .expect("tx is canonical"); + assert_eq!(low_tx.prev_mtp, Some(1)); +} + +#[test] +fn prev_mtp_differs_from_tip_mtp() { + // A 13-block chain (heights 0..=12) where each block's time equals its height. + let chain = chain_with_time_equal_to_height(12); + let mut tx_graph = TxGraph::default(); + + // Confirmed at height 11, with the tip at height 12: prev_mtp = MTP(10) = 5, while + // tip_mtp = MTP(12) = 7. These are distinguishable, so `prev_mtp` (BIP-68, the block before + // the coin) is clearly not the same reference as `tip_mtp` (BIP-113, the tip). + let txid = confirm_tx_at(&mut tx_graph, &chain, 1, 11); + + let view = chain.canonicalize_with_mtp(&tx_graph, chain.tip().block_id(), Default::default()); + + let canonical_tx = view.txs().find(|c| c.txid == txid).expect("tx is canonical"); + assert_eq!(canonical_tx.prev_mtp, Some(5)); + assert_eq!(view.tip_mtp(), Some(7)); + assert_ne!(canonical_tx.prev_mtp, view.tip_mtp()); }