Fix unified payment falling back to on-chain after PersistenceFailed

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.

Co-Authored-By: Claude <noreply@anthropic.com>

Author: elnafateh

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);
 						},

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<InMemoryStore>,
+	fail_writes: Arc<AtomicBool>,
+}
+
+impl KVStore for PaymentFailingStore {
+	fn read(
+		&self, primary_namespace: &str, secondary_namespace: &str, key: &str,
+	) -> impl Future<Output = Result<Vec<u8>, 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<u8>,
+	) -> impl Future<Output = Result<(), lightning::io::Error>> + '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<Output = Result<(), lightning::io::Error>> + 'static + Send {
+		KVStore::remove(&*self.inner, primary_namespace, secondary_namespace, key, lazy)
+	}
+
+	fn list(
+		&self, primary_namespace: &str, secondary_namespace: &str,
+	) -> impl Future<Output = Result<Vec<String>, 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<PageToken>,
+	) -> impl Future<Output = Result<PaginatedListResponse, lightning::io::Error>> + '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