Fix funding-payment reclassification downgrade and deep-reorg duplication

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,

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<Txid> =
-						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<Txid> =
+						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<FundingTxCandidate>,
 	) -> 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()))?;