fix/unified payment bolt12 duplicate fallback

src/lib.rs

@@ -2123,9 +2123,11 @@ impl Node {
 			// dropping it. This lets `channel_reestablish` drive the recovery flow, which is
 			// especially important against LND peers that don't always handle force-closure
 			// error messages correctly.
-		}
 
-		Ok(())
+			Ok(())
+		} else {
+			Err(Error::ChannelClosingFailed)
+		}
 	}
 
 	/// Update the config for a previously opened channel.

src/payment/unified.rs

@@ -291,19 +291,47 @@ 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)
-					}
-					.map_err(|e| {
-						log_error!(self.logger, "Failed to send BOLT12 offer: {:?}. This is part of a unified payment. Falling back to the BOLT11 invoice.", e);
-						e
-					});
+					};
 
-					if let Ok(payment_id) = payment_result {
-						return Ok(UnifiedPaymentResult::Bolt12 { payment_id });
+					match payment_result {
+						Ok(payment_id) => {
+							return Ok(UnifiedPaymentResult::Bolt12 { payment_id });
+						},
+						// A duplicate payment already exists, so falling back to the
+						// BOLT11 invoice would pay the same offer a second time.
+						Err(Error::DuplicatePayment) => {
+							log_error!(self.logger, "Failed to send BOLT12 offer: 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 BOLT11 invoice 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 BOLT12 offer: 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 BOLT12 offer: {:?}. This is part of a unified payment. Falling back to the BOLT11 invoice.", e);
+						},
 					}
 				},
 				PaymentMethod::LightningBolt11(invoice) => {

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;
@@ -43,7 +44,7 @@ use ldk_node::payment::{
 	ConfirmationStatus, PayerProofOptions, PaymentDetails, PaymentDirection, PaymentKind,
 	PaymentStatus, TransactionType, UnifiedPaymentResult,
 };
-use ldk_node::{BuildError, Builder, Event, Node, NodeError, ReserveType};
+use ldk_node::{BuildError, Builder, Event, Node, NodeError, ReserveType, UserChannelId};
 use lightning::ln::channelmanager::PaymentId;
 use lightning::routing::gossip::{NodeAlias, NodeId};
 use lightning::routing::router::RouteParametersConfig;
@@ -297,6 +298,91 @@ impl PaginatedKVStore for WalletPersistGatedStore {
 	}
 }
 
+/// 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<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, 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<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,
+		)
+	}
+}
+
+/// 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<AtomicBool>,
+) -> (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)
+}
+
 // LDK invokes the sync `SignerProvider::get_shutdown_scriptpubkey` callback on a runtime worker
 // thread while holding channel locks when a node accepts (or opens) a channel. If deriving the
 // shutdown script waits on wallet persistence, a contended wallet store wedges the event handler
@@ -544,6 +630,47 @@ async fn peer_removed_when_counterparty_force_closes_last_channel() {
 	);
 }
 
+/// Regression test for issue #1084: `Node::close_channel` and
+/// `Node::force_close_channel` returned `Ok(())` when the supplied
+/// `UserChannelId` did not match any channel for the counterparty, silently
+/// succeeding without initiating a close.
+#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+async fn close_unknown_user_channel_id_errors() {
+	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();
+	premine_and_distribute_funds(
+		&bitcoind.client,
+		&electrsd.client,
+		vec![address_a],
+		Amount::from_sat(5_000_000),
+	)
+	.await;
+	node_a.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());
+	let unknown_user_channel_id = UserChannelId(user_channel_id_a.0 ^ 1);
+
+	assert_eq!(
+		node_a.close_channel(&unknown_user_channel_id, node_b.node_id()),
+		Err(NodeError::ChannelClosingFailed)
+	);
+
+	// force_close_channel shares close_channel_internal and should also error.
+	assert_eq!(
+		node_a.force_close_channel(&unknown_user_channel_id, node_b.node_id(), None),
+		Err(NodeError::ChannelClosingFailed)
+	);
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
 async fn channel_full_cycle_0conf() {
 	let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();