From 33f1c2dcc84afd5103a5e80489b2ff2f39410018 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 1 Jul 2026 18:49:09 -0500 Subject: [PATCH 1/2] Preserve funding-payment confirmation state on late reclassification Funding broadcasts are classified into payment records off the broadcaster's queue, which can run after wallet sync has already recorded the transaction -- for instance when LDK re-broadcasts a still-pending funding transaction on restart. In that case the classification overwrote a record wallet sync had already advanced, downgrading a confirmed or graduated funding payment back to unconfirmed/pending. Merge only the classification and our contribution figures into an existing record, leaving the confirmation state that the wallet-sync events own in place. Raised by Codex in the review of #888. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/payment/store.rs | 115 +++++++++++++++++++++++++++++++++++++++++++ src/wallet/mod.rs | 28 +++++++++-- 2 files changed, 139 insertions(+), 4 deletions(-) diff --git a/src/payment/store.rs b/src/payment/store.rs index 160890895..5bd610361 100644 --- a/src/payment/store.rs +++ b/src/payment/store.rs @@ -710,6 +710,33 @@ impl PaymentDetailsUpdate { tx_type: None, } } + + /// Builds an update that merges a freshly-classified funding payment's classification + /// (`tx_type`), broadcast txid, and our contribution figures (amount/fee) into an existing + /// record, while leaving the top-level [`PaymentStatus`] and the on-chain + /// [`ConfirmationStatus`] untouched. + /// + /// Funding classification runs off the broadcaster queue and can land *after* wallet sync has + /// already advanced a record's confirmation state (e.g. when LDK re-broadcasts a still-pending + /// funding transaction on restart, or when the counterparty's broadcast is observed first). + /// Merging only the funding-specific fields keeps such a late classification from downgrading a + /// `Confirmed`/`Succeeded` payment back to `Unconfirmed`/`Pending`; the confirmation state is + /// owned by the wallet-sync events instead. + /// + /// The txid and figures are taken from the freshly broadcast (active) candidate. LDK only + /// re-broadcasts the active/confirmed funding candidate, so for an already-confirmed record + /// these equal what graduation stamped and the overwrite is a no-op; we rely on that invariant + /// rather than gating the txid/amount/fee merge on the stored confirmation state. + pub(crate) fn funding_reclassification(details: PaymentDetails) -> Self { + let mut update = Self::new(details.id); + update.amount_msat = Some(details.amount_msat); + update.fee_paid_msat = Some(details.fee_paid_msat); + if let PaymentKind::Onchain { txid, tx_type, .. } = details.kind { + update.txid = Some(txid); + update.tx_type = Some(tx_type); + } + update + } } impl From<&PaymentDetails> for PaymentDetailsUpdate { @@ -921,6 +948,94 @@ mod tests { assert_eq!(kind, PaymentKind::read(&mut &*kind.encode()).unwrap()); } + #[test] + fn funding_reclassification_does_not_downgrade_an_advanced_record() { + use bitcoin::hashes::Hash; + use std::str::FromStr; + + // A splice funding payment wallet sync has already advanced to Succeeded/Confirmed. + let txid = Txid::from_byte_array([7u8; 32]); + let id = PaymentId(txid.to_byte_array()); + let tx_type = Some(TransactionType::InteractiveFunding { + channels: vec![Channel { + counterparty_node_id: PublicKey::from_str( + "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", + ) + .unwrap(), + channel_id: ChannelId([3u8; 32]), + }], + }); + let advanced = PaymentDetails::new( + id, + PaymentKind::Onchain { + txid, + status: ConfirmationStatus::Confirmed { + block_hash: BlockHash::from_byte_array([8u8; 32]), + height: 100, + timestamp: 1, + }, + tx_type: tx_type.clone(), + }, + Some(2_000_000), + Some(999), + PaymentDirection::Outbound, + PaymentStatus::Succeeded, + ); + + // A fresh funding classification for the same payment is always Pending/Unconfirmed. + let fresh = PaymentDetails::new( + id, + PaymentKind::Onchain { txid, status: ConfirmationStatus::Unconfirmed, tx_type }, + Some(1_000_000), + Some(500), + PaymentDirection::Outbound, + PaymentStatus::Pending, + ); + + // The naive full update `insert_or_update` applied before the fix downgrades both the + // top-level status and the on-chain confirmation status — the bug Codex flagged. + let mut downgraded = advanced.clone(); + downgraded.update((&fresh).into()); + assert_eq!( + downgraded.status, + PaymentStatus::Pending, + "a full update from a fresh classification downgrades the top-level status", + ); + assert!( + matches!( + downgraded.kind, + PaymentKind::Onchain { status: ConfirmationStatus::Unconfirmed, .. } + ), + "a full update from a fresh classification downgrades the confirmation status", + ); + + // The narrowed reclassification update merges only the funding fields and preserves the + // advanced confirmation state that wallet sync owns. + let mut merged = advanced.clone(); + merged.update(PaymentDetailsUpdate::funding_reclassification(fresh)); + assert_eq!( + merged.status, + PaymentStatus::Succeeded, + "reclassification must not downgrade the top-level status", + ); + assert!( + matches!( + merged.kind, + PaymentKind::Onchain { + status: ConfirmationStatus::Confirmed { .. }, + tx_type: Some(TransactionType::InteractiveFunding { .. }), + .. + } + ), + "reclassification must preserve the confirmation status and keep the funding tx_type", + ); + // The contribution-derived figures from the fresh classification ARE merged in, replacing + // the existing record's: they are authoritative (the wallet can't recompute our share of a + // shared funding output), so the merge must carry them. + assert_eq!(merged.amount_msat, Some(1_000_000)); + assert_eq!(merged.fee_paid_msat, Some(500)); + } + #[derive(Clone, Debug, PartialEq, Eq)] struct LegacyBolt11JitKind { hash: PaymentHash, diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index ad4f8d45e..69e306de2 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -56,7 +56,8 @@ use persist::KVStoreWalletPersister; use crate::config::Config; use crate::fee_estimator::{ConfirmationTarget, FeeEstimator, OnchainFeeEstimator}; use crate::logger::{log_debug, log_error, log_info, log_trace, LdkLogger, Logger}; -use crate::payment::store::ConfirmationStatus; +use crate::payment::pending_payment_store::PendingPaymentDetailsUpdate; +use crate::payment::store::{ConfirmationStatus, PaymentDetailsUpdate}; use crate::payment::{ FundingTxCandidate, PaymentDetails, PaymentDirection, PaymentKind, PaymentStatus, PendingPaymentDetails, TransactionType, @@ -1343,9 +1344,28 @@ impl Wallet { async fn persist_funding_payment( &self, details: PaymentDetails, candidates: Vec, ) -> Result<(), Error> { - self.payment_store.insert_or_update(details.clone()).await?; - let pending = PendingPaymentDetails::new(details, Vec::new(), candidates); - self.pending_payment_store.insert_or_update(pending).await?; + if !self.payment_store.contains_key(&details.id) { + // First time we record this funding payment: store it and index it for graduation. + self.payment_store.insert_or_update(details.clone()).await?; + let pending = PendingPaymentDetails::new(details, Vec::new(), candidates); + self.pending_payment_store.insert_or_update(pending).await?; + } else { + // An earlier candidate or a racing wallet sync already recorded this payment. Merge only + // the classification (`tx_type`) and our contribution figures, which the wallet can't + // recompute; the confirmation state is owned by wallet-sync events, so a late + // classification must not move it (which would downgrade an already-Confirmed/Succeeded + // record). `update` is a no-op when the entry is absent, so the pending index is not + // re-created for a payment the graduation path already removed. + let update = PaymentDetailsUpdate::funding_reclassification(details); + let pending_update = PendingPaymentDetailsUpdate { + id: update.id, + payment_update: Some(update.clone()), + conflicting_txids: None, + candidates, + }; + self.payment_store.update(update).await?; + self.pending_payment_store.update(pending_update).await?; + } Ok(()) } From f01e83e4a4cfd94152939c913a16757a17c94273 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 1 Jul 2026 18:49:37 -0500 Subject: [PATCH 2/2] Revert reorged funding payments instead of duplicating them A funding payment's id is anchored to its first negotiated candidate, but once it confirms the record is stamped with the candidate that actually confirmed. After it graduates and its pending-store entry is removed, a later reorg event carrying the confirmed candidate's txid could no longer be resolved to the payment, so it was recorded as a duplicate generic on-chain payment rather than reverting the splice payment. RBF splices, whose confirmed candidate differs from the first, are the reachable case. Resolve such an event against the payment store by on-chain txid once the pending store no longer holds it, and revert the funding payment to pending so wallet sync re-graduates it when it reconfirms. Raised by Codex in the review of #888. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/wallet/mod.rs | 61 ++++++++++-- tests/integration_tests_rust.rs | 160 ++++++++++++++++++++++++++++++++ 2 files changed, 215 insertions(+), 6 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 69e306de2..eb5d3b1ec 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -392,11 +392,6 @@ impl Wallet { continue; }; - // Collect all conflict txids - let mut conflict_txids: Vec = - conflicts.iter().map(|(_, conflict_txid)| *conflict_txid).collect(); - - conflict_txids.push(txid); // The payment already exists in the store at this point: `bump_fee_rbf` updates // the payment store with the replacement txid before the next sync cycle, so we // can safely fetch it here. @@ -407,8 +402,26 @@ impl Wallet { ); let payment = self.payment_store.get(&payment_id).ok_or(Error::InvalidPaymentId)?; + + // A graduated funding payment is resolvable here only through + // `find_payment_by_txid`'s payment-store fallback. Revert it like the + // `TxUnconfirmed`/`TxDropped` arms instead of mirroring a non-`Pending` record + // into the pending store, which graduation's pending-only scan would reject. + if payment.status != PaymentStatus::Pending + && self.apply_funding_status_update( + payment_id, + txid, + ConfirmationStatus::Unconfirmed, + )? { + continue; + } + + // Collect all conflict txids + let mut conflict_txids: Vec = + conflicts.iter().map(|(_, conflict_txid)| *conflict_txid).collect(); + conflict_txids.push(txid); let pending_payment_details = - self.create_pending_payment_from_tx(payment, conflict_txids.clone()); + self.create_pending_payment_from_tx(payment, conflict_txids); self.runtime.block_on( self.pending_payment_store.insert_or_update(pending_payment_details), @@ -1454,6 +1467,34 @@ impl Wallet { return Some(replaced_details.details.id); } + // A funding payment graduates out of the pending store, after which only the payment store + // retains it — under its first-candidate-anchored id, but stamped with the confirmed + // candidate's txid. Map a later event (e.g. a reorg returning the confirmed candidate to the + // mempool) back to that funding payment so it is reverted in place rather than duplicated as + // a generic on-chain payment under the candidate's txid. Only one funding record carries a + // given confirmed txid (its id is anchored to the first candidate and reclassification + // merges into it), so the first match is unambiguous. + if let Some(funding) = self + .payment_store + .list_filter(|p| { + matches!( + p.kind, + PaymentKind::Onchain { + txid, + tx_type: + Some( + TransactionType::Funding { .. } + | TransactionType::InteractiveFunding { .. }, + ), + .. + } if txid == target_txid + ) + }) + .first() + { + return Some(funding.id); + } + None } @@ -1491,6 +1532,14 @@ impl Wallet { } } + // A reorg returning the transaction to the mempool reverts the payment to pending so wallet + // sync re-graduates it once it reconfirms. This also re-establishes the pending-store entry + // below (gated on `Pending`) that graduation removed; without it a graduated payment would + // be left `Succeeded` with an `Unconfirmed` kind and no way to re-graduate. + if matches!(confirmation_status, ConfirmationStatus::Unconfirmed) { + payment.status = PaymentStatus::Pending; + } + payment.kind = PaymentKind::Onchain { txid: event_txid, status: confirmation_status, tx_type }; self.runtime.block_on(self.payment_store.insert_or_update(payment.clone()))?; diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index c3c2f4262..dac73a1b7 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -1948,6 +1948,166 @@ async fn splice_payment_reorged_to_unconfirmed() { node_b.stop().unwrap(); } +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn rbf_splice_payment_reverts_after_deep_reorg() { + // A graduated RBF splice payment is anchored to the FIRST candidate's id but stamped with the + // CONFIRMED (RBF) candidate's txid. Graduation removes its pending-store entry, so a later deep + // reorg (deeper than ANTI_REORG_DELAY) that returns the confirmed candidate to the mempool must + // still map the event back to the original payment and revert it — not create a duplicate + // generic on-chain payment under the confirmed candidate's id. + + // Lower incrementalrelayfee so the RBF feerate bump is relayable (as run_rbf_splice_channel_test). + let bitcoind_exe = std::env::var("BITCOIND_EXE") + .ok() + .or_else(|| corepc_node::downloaded_exe_path().ok()) + .expect( + "you need to provide an env var BITCOIND_EXE or specify a bitcoind version feature", + ); + let mut bitcoind_conf = corepc_node::Conf::default(); + bitcoind_conf.network = "regtest"; + bitcoind_conf.args.push("-rest"); + bitcoind_conf.args.push("-incrementalrelayfee=0.00000100"); + let bitcoind = BitcoinD::with_conf(bitcoind_exe, &bitcoind_conf).unwrap(); + + let electrs_exe = std::env::var("ELECTRS_EXE") + .ok() + .or_else(electrsd::downloaded_exe_path) + .expect("you need to provide env var ELECTRS_EXE or specify an electrsd version feature"); + let mut electrsd_conf = electrsd::Conf::default(); + electrsd_conf.http_enabled = true; + electrsd_conf.network = "regtest"; + let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &electrsd_conf).unwrap(); + let chain_source = random_chain_source(&bitcoind, &electrsd); + + let (node_a, node_b) = setup_two_nodes(&chain_source, false, true, false); + + let address_a = node_a.onchain_payment().new_address().unwrap(); + let address_b = node_b.onchain_payment().new_address().unwrap(); + let premine_amount_sat = 5_000_000; + premine_and_distribute_funds( + &bitcoind.client, + &electrsd.client, + vec![address_a, address_b], + Amount::from_sat(premine_amount_sat), + ) + .await; + node_a.sync_wallets().unwrap(); + node_b.sync_wallets().unwrap(); + + open_channel(&node_a, &node_b, 4_000_000, false, &electrsd).await; + generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await; + node_a.sync_wallets().unwrap(); + node_b.sync_wallets().unwrap(); + let _user_channel_id_a = expect_channel_ready_event!(node_a, node_b.node_id()); + let user_channel_id_b = expect_channel_ready_event!(node_b, node_a.node_id()); + + // node_b splices in, then RBF-bumps it: the funding payment spans two candidates, its id + // anchored to the first (original) candidate's txid. + node_b.splice_in(&user_channel_id_b, node_a.node_id(), 1_000_000).unwrap(); + let original_txo = expect_splice_negotiated_event!(node_a, node_b.node_id()); + expect_splice_negotiated_event!(node_b, node_a.node_id()); + wait_for_tx(&electrsd.client, original_txo.txid).await; + wait_for_classified_funding_payment(&node_b, original_txo.txid).await; + node_a.sync_wallets().unwrap(); + node_b.sync_wallets().unwrap(); + + node_b.bump_channel_funding_fee(&user_channel_id_b, node_a.node_id()).unwrap(); + let rbf_txo = expect_splice_negotiated_event!(node_a, node_b.node_id()); + expect_splice_negotiated_event!(node_b, node_a.node_id()); + assert_ne!(original_txo, rbf_txo, "RBF should produce a different funding txo"); + wait_for_tx(&electrsd.client, rbf_txo.txid).await; + wait_for_classified_funding_payment(&node_b, rbf_txo.txid).await; + node_a.sync_wallets().unwrap(); + node_b.sync_wallets().unwrap(); + + // Confirm the RBF candidate and graduate it past ANTI_REORG_DELAY (6 confirmations), which + // removes the pending-store entry. + generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await; + node_a.sync_wallets().unwrap(); + node_b.sync_wallets().unwrap(); + expect_channel_ready_event!(node_a, node_b.node_id()); + expect_channel_ready_event!(node_b, node_a.node_id()); + + let payment_id = PaymentId(original_txo.txid.to_byte_array()); + let rbf_payment_id = PaymentId(rbf_txo.txid.to_byte_array()); + + // Graduated: anchored to the original candidate's id, stamped with the confirmed RBF + // candidate's txid, with no separate record under the RBF candidate's id. + let payment = node_b.payment(&payment_id).expect("splice payment graduated"); + assert_eq!(payment.status, PaymentStatus::Succeeded); + assert!(matches!( + payment.kind, + PaymentKind::Onchain { + txid, + status: ConfirmationStatus::Confirmed { .. }, + tx_type: Some(TransactionType::InteractiveFunding { .. }), + } if txid == rbf_txo.txid + )); + assert!( + node_b.payment(&rbf_payment_id).is_none(), + "the graduated splice payment must not be duplicated under the RBF candidate's id", + ); + + // Deep reorg (deeper than ANTI_REORG_DELAY): drop the 6 graduation blocks and build a longer, + // transaction-free chain, returning the confirmed RBF candidate to the mempool. + let original_height = + bitcoind.client.get_blockchain_info().expect("failed to get blockchain info").blocks; + invalidate_blocks(&bitcoind.client, 6); + let replacement_address = bitcoind.client.new_address().expect("failed to get new address"); + for _ in 0..7 { + let _res: serde_json::Value = bitcoind + .client + .call("generateblock", &[json!(replacement_address.to_string()), json!([])]) + .expect("failed to generate empty block"); + } + wait_for_block(&electrsd.client, original_height as usize + 1).await; + // Wait for the reorged-out RBF candidate to reappear in the mempool before syncing, so the sync + // reliably observes its TxUnconfirmed event rather than racing electrs's reindex. + wait_for_tx(&electrsd.client, rbf_txo.txid).await; + node_b.sync_wallets().unwrap(); + + // The reorg event for the confirmed RBF candidate's txid must map back to the original payment + // and revert it to Pending/Unconfirmed, rather than creating a duplicate generic on-chain + // payment under the RBF candidate's id. + assert!( + node_b.payment(&rbf_payment_id).is_none(), + "a reorged-out RBF splice must not produce a duplicate generic on-chain payment", + ); + let payment = node_b.payment(&payment_id).expect("splice payment still exists after the reorg"); + assert_eq!(payment.status, PaymentStatus::Pending); + assert!(matches!( + payment.kind, + PaymentKind::Onchain { + status: ConfirmationStatus::Unconfirmed, + tx_type: Some(TransactionType::InteractiveFunding { .. }), + .. + } + )); + + // The revert re-established the pending-store entry, so once the RBF candidate (still in the + // mempool) reconfirms past ANTI_REORG_DELAY the payment re-graduates to Succeeded in place — + // without leaving a duplicate behind. + generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await; + node_b.sync_wallets().unwrap(); + let payment = node_b.payment(&payment_id).expect("splice payment re-graduated"); + assert_eq!(payment.status, PaymentStatus::Succeeded); + assert!(matches!( + payment.kind, + PaymentKind::Onchain { + txid, + status: ConfirmationStatus::Confirmed { .. }, + tx_type: Some(TransactionType::InteractiveFunding { .. }), + } if txid == rbf_txo.txid + )); + assert!( + node_b.payment(&rbf_payment_id).is_none(), + "re-graduation must not create a duplicate payment under the RBF candidate's id", + ); + + node_a.stop().unwrap(); + node_b.stop().unwrap(); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn splice_in_rbf_joins_counterparty_splice() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();