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..eb5d3b1ec 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, @@ -391,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. @@ -406,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), @@ -1343,9 +1357,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(()) } @@ -1434,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 } @@ -1471,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();