diff --git a/src/payment/unified.rs b/src/payment/unified.rs index e10d57ba0..cdbfa7e7b 100644 --- a/src/payment/unified.rs +++ b/src/payment/unified.rs @@ -291,9 +291,22 @@ impl UnifiedPayment { let payment_result = if let Ok(hrn) = HumanReadableName::from_encoded(uri_str) { let hrn = maybe_wrap(hrn.clone()); - self.bolt12_payment.send_using_amount_inner(&offer, amount_msat.unwrap_or(0), None, None, route_parameters, Some(hrn)) + self.bolt12_payment.send_using_amount_inner( + &offer, + amount_msat.unwrap_or(0), + None, + None, + route_parameters, + Some(hrn), + ) } else if let Some(amount_msat) = amount_msat { - self.bolt12_payment.send_using_amount(&offer, amount_msat, None, None, route_parameters) + self.bolt12_payment.send_using_amount( + &offer, + amount_msat, + None, + None, + route_parameters, + ) } else { self.bolt12_payment.send(&offer, None, None, route_parameters) } @@ -308,14 +321,29 @@ impl UnifiedPayment { }, PaymentMethod::LightningBolt11(invoice) => { let invoice = maybe_wrap(invoice.clone()); - let payment_result = self.bolt11_invoice.send(&invoice, route_parameters) - .map_err(|e| { + let payment_result = self.bolt11_invoice.send(&invoice, route_parameters); + + match payment_result { + Ok(payment_id) => { + return Ok(UnifiedPaymentResult::Bolt11 { payment_id }); + }, + // A duplicate payment already exists, so falling back to the + // on-chain method would pay the same invoice a second time. + Err(Error::DuplicatePayment) => { + log_error!(self.logger, "Failed to send BOLT11 invoice: DuplicatePayment. This is part of a unified payment. Aborting to avoid duplicate payment."); + return Err(Error::DuplicatePayment); + }, + // A persistence failure may occur after the Lightning payment has + // already been initiated with the ChannelManager. Falling back to + // the on-chain method in that case would double-pay, so we abort + // instead of proceeding to the next payment method. + Err(Error::PersistenceFailed) => { + log_error!(self.logger, "Failed to send BOLT11 invoice: PersistenceFailed. This is part of a unified payment. Aborting to avoid a potential duplicate payment."); + return Err(Error::PersistenceFailed); + }, + Err(e) => { log_error!(self.logger, "Failed to send BOLT11 invoice: {:?}. This is part of a unified payment. Falling back to the on-chain transaction.", e); - e - }); - - if let Ok(payment_id) = payment_result { - return Ok(UnifiedPaymentResult::Bolt11 { payment_id }); + }, } }, PaymentMethod::OnChain(address) => { diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index 0dad32d6a..4cc70e874 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -22,15 +22,16 @@ use common::logging::{ init_log_logger, validate_log_entry, CollectingLogWriter, MultiNodeLogger, TestLogWriter, }; use common::{ - bump_fee_and_broadcast, distribute_funds_unconfirmed, do_channel_full_cycle, - expect_channel_pending_event, expect_channel_ready_event, expect_channel_ready_events, - expect_event, expect_payment_claimable_event, expect_payment_received_event, - expect_payment_successful_event, expect_splice_negotiated_event, generate_blocks_and_wait, - generate_listening_addresses, invalidate_blocks, open_channel, open_channel_no_wait, - open_channel_push_amt, open_channel_with_all, premine_and_distribute_funds, premine_blocks, - prepare_rbf, random_chain_source, random_config, setup_bitcoind_and_electrsd, setup_builder, - setup_node, setup_two_nodes, splice_in_with_all, wait_for_block, wait_for_tx, InMemoryStore, - NodePaymentExt, TestChainSource, TestConfig, TestStoreType, TestSyncStore, + bump_fee_and_broadcast, configure_chain_source, distribute_funds_unconfirmed, + do_channel_full_cycle, expect_channel_pending_event, expect_channel_ready_event, + expect_channel_ready_events, expect_event, expect_payment_claimable_event, + expect_payment_received_event, expect_payment_successful_event, expect_splice_negotiated_event, + generate_blocks_and_wait, generate_listening_addresses, invalidate_blocks, open_channel, + open_channel_no_wait, open_channel_push_amt, open_channel_with_all, + premine_and_distribute_funds, premine_blocks, prepare_rbf, random_chain_source, random_config, + setup_bitcoind_and_electrsd, setup_builder, setup_node, setup_two_nodes, splice_in_with_all, + wait_for_block, wait_for_tx, InMemoryStore, NodePaymentExt, TestChainSource, TestConfig, + TestNode, TestStoreType, TestSyncStore, }; use electrsd::corepc_node::{self, Node as BitcoinD}; use electrsd::ElectrsD; @@ -3315,12 +3316,99 @@ async fn unified_receive_rejects_msat_overflow() { ); } +/// A [`KVStore`] that fails every `write` to the payments namespace once `fail_writes` is set, +/// while keeping everything else operational. Used to arm a `PersistenceFailed` regression case +/// on top of an otherwise-normal node, without needing a dedicated node/channel fixture. +struct PaymentFailingStore { + inner: Arc, + fail_writes: Arc, +} + +impl KVStore for PaymentFailingStore { + fn read( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, + ) -> impl Future, lightning::io::Error>> + 'static + Send { + KVStore::read(&*self.inner, primary_namespace, secondary_namespace, key) + } + + fn write( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: Vec, + ) -> impl Future> + 'static + Send { + let inner = Arc::clone(&self.inner); + let fail_writes = Arc::clone(&self.fail_writes); + let primary_namespace = primary_namespace.to_string(); + let secondary_namespace = secondary_namespace.to_string(); + let key = key.to_string(); + async move { + // Only fail payment-store writes. Failing every write (e.g. channel monitor + // updates) would crash the background processor, defeating the test. + if fail_writes.load(Ordering::Acquire) && primary_namespace == "payments" { + return Err(lightning::io::Error::new( + lightning::io::ErrorKind::Other, + "injected payment persistence failure", + )); + } + KVStore::write(&*inner, &primary_namespace, &secondary_namespace, &key, buf).await + } + } + + fn remove( + &self, primary_namespace: &str, secondary_namespace: &str, key: &str, lazy: bool, + ) -> impl Future> + 'static + Send { + KVStore::remove(&*self.inner, primary_namespace, secondary_namespace, key, lazy) + } + + fn list( + &self, primary_namespace: &str, secondary_namespace: &str, + ) -> impl Future, lightning::io::Error>> + 'static + Send { + KVStore::list(&*self.inner, primary_namespace, secondary_namespace) + } +} + +impl PaginatedKVStore for PaymentFailingStore { + fn list_paginated( + &self, primary_namespace: &str, secondary_namespace: &str, page_token: Option, + ) -> impl Future> + 'static + Send + { + PaginatedKVStore::list_paginated( + &*self.inner, + primary_namespace, + secondary_namespace, + page_token, + ) + } +} + +/// Builds `node_a` on a [`PaymentFailingStore`] the caller can arm later via `fail_writes`, and +/// `node_b` on the default store — otherwise identical to `setup_two_nodes`. Lets a single test +/// flow cover the `PersistenceFailed` fallback hazard on top of the fixture it already needs for +/// the normal unified-payment paths, instead of duplicating that fixture in a standalone test. +fn setup_two_nodes_with_failing_store_a( + chain_source: &TestChainSource, fail_writes: Arc, +) -> (TestNode, TestNode) { + let config_a = random_config(); + setup_builder!(builder_a, config_a.node_config); + configure_chain_source(chain_source, &mut builder_a, &config_a); + builder_a.set_async_payments_role(config_a.async_payments_role).unwrap(); + let failing_store = PaymentFailingStore { inner: Arc::new(InMemoryStore::new()), fail_writes }; + let node_a = builder_a.build_with_store(config_a.node_entropy.into(), failing_store).unwrap(); + node_a.start().unwrap(); + + let mut config_b = random_config(); + config_b.node_config.manually_handle_unknown_bolt11_payments = true; + let node_b = setup_node(chain_source, config_b); + + (node_a, node_b) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn unified_send_receive_bip21_uri() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); let chain_source = random_chain_source(&bitcoind, &electrsd); - let (node_a, node_b) = setup_two_nodes(&chain_source, false, false); + let fail_writes = Arc::new(AtomicBool::new(false)); + let (node_a, node_b) = + setup_two_nodes_with_failing_store_a(&chain_source, Arc::clone(&fail_writes)); let address_a = node_a.onchain_payment().new_address().unwrap(); let premined_sats = 5_000_000; @@ -3395,6 +3483,22 @@ async fn unified_send_receive_bip21_uri() { }; expect_payment_successful_event!(node_a, invoice_payment_id, None); + // Regression test for https://github.com/lightningdevkit/ldk-node/issues/1033: retrying + // the same BOLT11 invoice must return DuplicatePayment, not fall back to on-chain. + let duplicate_result = node_a.unified_payment().send(uri_str_without_offer, None, None).await; + match duplicate_result { + Err(NodeError::DuplicatePayment) => { + // Expected — this is the fix for #1033. + }, + Ok(UnifiedPaymentResult::Onchain { txid }) => { + panic!( + "Regression: duplicate BOLT11 payment fell back to on-chain. txid={}. See #1033", + txid + ); + }, + other => panic!("Expected DuplicatePayment error on retry, got: {:?}", other), + } + let expect_onchain_amount_sats = 800_000; let onchain_uni_payment = node_b.unified_payment().receive(expect_onchain_amount_sats, "asdf", 4_000).unwrap(); @@ -3425,6 +3529,36 @@ async fn unified_send_receive_bip21_uri() { assert_eq!(node_b.list_balances().total_onchain_balance_sats, 800_000); assert_eq!(node_b.list_balances().total_lightning_balance_sats, 200_000); + + // Regression test: a payment-store persistence failure on the BOLT11 leg — which can occur + // after the Lightning payment has already been initiated with the ChannelManager — must also + // abort rather than fall back to on-chain. Arm node_a's store and send a fresh BOLT11-only + // URI to isolate the BOLT11 leg from the BOLT12/on-chain legs already exercised above. + fail_writes.store(true, Ordering::Release); + + let fresh_amount_sats = 50_000; + let fresh_uri = node_b.unified_payment().receive(fresh_amount_sats, "asdf", 4_000).unwrap(); + let fresh_uri_bolt11_only = fresh_uri.split("&lno=").next().unwrap(); + + let persistence_result = node_a.unified_payment().send(fresh_uri_bolt11_only, None, None).await; + match persistence_result { + Err(NodeError::PersistenceFailed) => { + // Expected — the unified payment must abort, not fall back to on-chain. + }, + Ok(UnifiedPaymentResult::Onchain { txid }) => { + panic!("Regression: PersistenceFailed fell back to on-chain. txid={}", txid); + }, + other => panic!("Expected PersistenceFailed error, got: {:?}", other), + } + + let onchain_payments = node_a.list_all_payments().into_iter().any(|p| { + matches!(p.kind, PaymentKind::Onchain { .. }) + && p.amount_msat == Some(fresh_amount_sats as u64 * 1000) + }); + assert!( + !onchain_payments, + "An on-chain payment for the fresh amount was broadcast despite PersistenceFailed" + ); } #[tokio::test(flavor = "multi_thread", worker_threads = 1)]