From ba5f712ba5c8fb18cab77ff5c8e741b619e7ac3c Mon Sep 17 00:00:00 2001 From: elnafateh Date: Mon, 10 Aug 2026 15:52:46 +0100 Subject: [PATCH 1/5] Fix BOLT11 DuplicatePayment triggering on-chain fallback in unified payments Error::DuplicatePayment is now terminal in UnifiedPayment::send, preventing a duplicate Lightning payment from falling back to an on-chain payment. --- src/payment/unified.rs | 36 +++++++++++---- tests/integration_tests_rust.rs | 78 +++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 9 deletions(-) diff --git a/src/payment/unified.rs b/src/payment/unified.rs index cb5117414..61eecf313 100644 --- a/src/payment/unified.rs +++ b/src/payment/unified.rs @@ -287,9 +287,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) } @@ -304,14 +317,19 @@ 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 }); + }, + 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); + }, + 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 fd247f74c..dd6b139a5 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -3417,6 +3417,84 @@ async fn unified_send_receive_bip21_uri() { assert_eq!(node_b.list_balances().total_lightning_balance_sats, 200_000); } +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn unified_send_bolt11_duplicate_payment_no_onchain_fallback() { + // Regression test for https://github.com/lightningdevkit/ldk-node/issues/1033 + // + // Sending a unified BIP21 payment that resolves to BOLT11 should return + // Error::DuplicatePayment on retry, not fall back to the on-chain method. + + 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 address_a = node_a.onchain_payment().new_address().unwrap(); + let premined_sats = 5_000_000; + + premine_and_distribute_funds( + &bitcoind.client, + &electrsd.client, + vec![address_a], + Amount::from_sat(premined_sats), + ) + .await; + + node_a.sync_wallets().unwrap(); + open_channel(&node_a, &node_b, 4_000_000, true, &electrsd).await; + 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()); + + // Sleep until we broadcast a node announcement. + while node_b.status().latest_node_announcement_broadcast_timestamp.is_none() { + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + tokio::time::sleep(std::time::Duration::from_secs(1)).await; + + let expected_amount_sats = 100_000; + let expiry_sec = 4_000; + + // Receive a unified payment on node_b — this will produce a URI with BOLT12 offer + BOLT11 invoice. + let uri_str = node_b.unified_payment().receive(expected_amount_sats, "asdf", expiry_sec).unwrap(); + + // Strip the BOLT12 offer so the URI resolves to BOLT11 only (no BOLT12, no on-chain fallback). + let uri_str_bolt11_only = uri_str.split("&lno=").next().unwrap(); + + // First send: should succeed via BOLT11. + let first_result = node_a.unified_payment().send(uri_str_bolt11_only, None, None).await; + let first_payment_id = match first_result { + Ok(UnifiedPaymentResult::Bolt11 { payment_id }) => payment_id, + Ok(other) => panic!("Expected Bolt11 result on first send, got: {:?}", other), + Err(e) => panic!("Expected Bolt11 result on first send, got error: {:?}", e), + }; + expect_payment_successful_event!(node_a, Some(first_payment_id), None); + + // Second send with the same URI: should return DuplicatePayment, NOT fall back to on-chain. + let second_result = node_a.unified_payment().send(uri_str_bolt11_only, None, None).await; + match second_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 + ); + }, + Ok(other) => { + panic!("Expected DuplicatePayment error on retry, got: {:?}", other); + }, + Err(other) => { + panic!("Expected DuplicatePayment error on retry, got: {:?}", other); + }, + } +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn lsps2_client_service_integration() { do_lsps2_client_service_integration(true).await; From e0fea6e5374af8a9c111736fd8fa9f7c9f24501a Mon Sep 17 00:00:00 2001 From: elnafateh Date: Mon, 10 Aug 2026 17:10:49 +0100 Subject: [PATCH 2/5] Apply rustfmt to new unified payment test --- tests/integration_tests_rust.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index dd6b139a5..6f0fee8e2 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -3460,7 +3460,8 @@ async fn unified_send_bolt11_duplicate_payment_no_onchain_fallback() { let expiry_sec = 4_000; // Receive a unified payment on node_b — this will produce a URI with BOLT12 offer + BOLT11 invoice. - let uri_str = node_b.unified_payment().receive(expected_amount_sats, "asdf", expiry_sec).unwrap(); + let uri_str = + node_b.unified_payment().receive(expected_amount_sats, "asdf", expiry_sec).unwrap(); // Strip the BOLT12 offer so the URI resolves to BOLT11 only (no BOLT12, no on-chain fallback). let uri_str_bolt11_only = uri_str.split("&lno=").next().unwrap(); From 8261ca6d1805467cf0bc308430d69f75c833f136 Mon Sep 17 00:00:00 2001 From: elnafateh Date: Wed, 19 Aug 2026 15:12:00 +0100 Subject: [PATCH 3/5] Fix regression test to match upstream macro signature The expect_payment_successful_event! macro takes a bare PaymentId, not an Option. Adjust the #1033 regression test accordingly so it compiles against current upstream/main. --- tests/integration_tests_rust.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index 6f0fee8e2..08ba44cce 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -3473,7 +3473,7 @@ async fn unified_send_bolt11_duplicate_payment_no_onchain_fallback() { Ok(other) => panic!("Expected Bolt11 result on first send, got: {:?}", other), Err(e) => panic!("Expected Bolt11 result on first send, got error: {:?}", e), }; - expect_payment_successful_event!(node_a, Some(first_payment_id), None); + expect_payment_successful_event!(node_a, first_payment_id, None); // Second send with the same URI: should return DuplicatePayment, NOT fall back to on-chain. let second_result = node_a.unified_payment().send(uri_str_bolt11_only, None, None).await; From 30aff003fc24995db0286bfe029f5b73ab02244a Mon Sep 17 00:00:00 2001 From: elnafateh Date: Wed, 19 Aug 2026 17:36:07 +0100 Subject: [PATCH 4/5] Fix unified payment falling back to on-chain after PersistenceFailed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In `UnifiedPayment::send`, the BOLT11 leg's `bolt11_invoice.send` only returns `Err(PersistenceFailed)` *after* `pay_for_bolt11_invoice` has already succeeded and the Lightning payment is in-flight. The previous match treated every error (via `Err(e)`) as a fall-through to the next payment method, so a persistence failure after initiation would broadcast an on-chain transaction for the same URI — a duplicate payment. We now treat `Err(Error::PersistenceFailed)` on the BOLT11 leg as terminal, mirroring how `DuplicatePayment` is already handled, and abort the unified payment instead of falling back to on-chain. This is a regression hazard raised during review of the #1033 fix (PR #1038). It is pre-existing and orthogonal to #1033 (which only made `DuplicatePayment` terminal); tracked separately as the unified variant of the broader post-commit persistence hazard. Adds `unified_send_bolt11_persistence_failure_no_onchain_fallback`, which arms a failing payment-store write on a `KVStore`-backed node and asserts that `send` returns `PersistenceFailed` without recording any on-chain payment. --- src/payment/unified.rs | 10 ++ tests/integration_tests_rust.rs | 180 ++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+) diff --git a/src/payment/unified.rs b/src/payment/unified.rs index 61eecf313..668c43d36 100644 --- a/src/payment/unified.rs +++ b/src/payment/unified.rs @@ -323,10 +323,20 @@ impl UnifiedPayment { 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); }, diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index 08ba44cce..b721d880d 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -3417,6 +3417,186 @@ async fn unified_send_receive_bip21_uri() { assert_eq!(node_b.list_balances().total_lightning_balance_sats, 200_000); } +/// A [`KVStore`] that fails every `write` once `fail_writes` is set, while keeping +/// reads/list/remove operational so the node can still start and run. +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 and the node + // itself, defeating the test of the `PersistenceFailed` handling path. + 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, + ) + } +} + +// Regression test for the unified-payment `PersistenceFailed` double-payment hazard: when the +// BOLT11 leg initiates the Lightning payment but the subsequent payment-store write fails, the +// error must be terminal rather than falling through to the on-chain method. +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn unified_send_bolt11_persistence_failure_no_onchain_fallback() { + let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); + let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap()); + let chain_source = TestChainSource::Esplora(&electrsd); + + // Node B (receiver) uses the default store. + let (node_a, node_b) = setup_two_nodes(&chain_source, false, false); + + let address_a = node_a.onchain_payment().new_address().unwrap(); + let premined_sats = 5_000_000; + premine_and_distribute_funds( + &bitcoind.client, + &electrsd.client, + vec![address_a], + Amount::from_sat(premined_sats), + ) + .await; + + node_a.sync_wallets().unwrap(); + open_channel(&node_a, &node_b, 4_000_000, true, &electrsd).await; + 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()); + + while node_b.status().latest_node_announcement_broadcast_timestamp.is_none() { + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + tokio::time::sleep(std::time::Duration::from_secs(1)).await; + + let expected_amount_sats = 100_000; + let expiry_sec = 4_000; + + let uri_str = + node_b.unified_payment().receive(expected_amount_sats, "asdf", expiry_sec).unwrap(); + // Strip the BOLT12 offer so the URI resolves to BOLT11 only. + let uri_str_bolt11_only = uri_str.split("&lno=").next().unwrap(); + + // Node A (sender) runs on a store that fails writes, so the payment-store insert after + // `pay_for_bolt11_invoice` succeeds will surface as `PersistenceFailed`. + let mut config_a = random_config(); + setup_builder!(builder_a, config_a.node_config); + let mut sync_config = EsploraSyncConfig::default(); + sync_config.background_sync_config = None; + builder_a.set_chain_source_esplora(esplora_url.clone(), Some(sync_config.clone())); + let fail_writes = Arc::new(AtomicBool::new(false)); + let failing_store = PaymentFailingStore { + inner: Arc::new(InMemoryStore::new()), + fail_writes: Arc::clone(&fail_writes), + }; + let node_a_failing = + builder_a.build_with_store(config_a.node_entropy.into(), failing_store).unwrap(); + node_a_failing.start().unwrap(); + + // Fund and open a channel for the failing-store node too, so it can initiate Lightning. + let address_a_failing = node_a_failing.onchain_payment().new_address().unwrap(); + premine_and_distribute_funds( + &bitcoind.client, + &electrsd.client, + vec![address_a_failing], + Amount::from_sat(premined_sats), + ) + .await; + node_a_failing.sync_wallets().unwrap(); + node_a_failing + .connect( + node_b.node_id(), + node_b.listening_addresses().unwrap().first().unwrap().clone(), + false, + ) + .unwrap(); + open_channel(&node_a_failing, &node_b, 4_000_000, true, &electrsd).await; + generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await; + node_a_failing.sync_wallets().unwrap(); + // `node_b` is the shared counterparty for both channels; it must also observe the + // new funding tx's confirmations or it will never emit `ChannelReady` back. + node_b.sync_wallets().unwrap(); + expect_channel_ready_event!(node_a_failing, node_b.node_id()); + expect_channel_ready_event!(node_b, node_a_failing.node_id()); + + // Arm the failure, then send. The BOLT11 leg will initiate but the store write fails. + fail_writes.store(true, Ordering::Release); + + let result = node_a_failing.unified_payment().send(uri_str_bolt11_only, None, None).await; + match 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); + }, + Ok(other) => { + panic!("Expected PersistenceFailed error, got: {:?}", other); + }, + Err(other) => { + panic!("Expected PersistenceFailed error, got: {:?}", other); + }, + } + + // Confirm no on-chain payment was recorded for the unified amount. + let onchain_payments = node_a_failing.list_all_payments().into_iter().any(|p| { + matches!(p.kind, PaymentKind::Onchain { .. }) + && p.amount_msat == Some(expected_amount_sats as u64 * 1000) + }); + assert!( + !onchain_payments, + "An on-chain payment for the unified amount was broadcast despite PersistenceFailed" + ); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn unified_send_bolt11_duplicate_payment_no_onchain_fallback() { // Regression test for https://github.com/lightningdevkit/ldk-node/issues/1033 From 9c2d37ceaaa665c7f3d38f84c180f0af38e19e56 Mon Sep 17 00:00:00 2001 From: elnafateh Date: Thu, 20 Aug 2026 22:13:40 +0100 Subject: [PATCH 5/5] Fix unused mut warning in unified persistence fallback test --- tests/integration_tests_rust.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index b721d880d..998520e3e 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -3527,7 +3527,7 @@ async fn unified_send_bolt11_persistence_failure_no_onchain_fallback() { // Node A (sender) runs on a store that fails writes, so the payment-store insert after // `pay_for_bolt11_invoice` succeeds will surface as `PersistenceFailed`. - let mut config_a = random_config(); + let config_a = random_config(); setup_builder!(builder_a, config_a.node_config); let mut sync_config = EsploraSyncConfig::default(); sync_config.background_sync_config = None;