From c7b80b85f750f24de09bdb7056113592707431e6 Mon Sep 17 00:00:00 2001 From: Bartok9 <259807879+Bartok9@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:52:52 -0400 Subject: [PATCH 1/2] fix(close_channel): error when no matching channel found Closes #1084 Previously close_channel and force_close_channel returned Ok(()) when no matching UserChannelId was found for the counterparty, silently succeeding without initiating a close. Now returns Err(ChannelClosingFailed), matching update_channel_config. AI-assisted: generated by Sera (Hermes Agent), verified manually. Tests compile; integration test requires bitcoind/electrs binaries (architecturally incompatible electrs binary in this cron environment, same failure affects all integration tests) Signed-off-by: Bartok9 <259807879+Bartok9@users.noreply.github.com> --- src/lib.rs | 6 +++-- tests/integration_tests_rust.rs | 43 ++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9ce5a273e7..165320b4f7 100644 --- a/src/lib.rs +++ b/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. diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index 5f4a95b7eb..6650754f55 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -43,7 +43,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; @@ -544,6 +544,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(); From 91975e92e0a389dafe2c8df34d4752de0881c999 Mon Sep 17 00:00:00 2001 From: Bartok9 <259807879+Bartok9@users.noreply.github.com> Date: Sun, 13 Sep 2026 09:13:44 -0400 Subject: [PATCH 2/2] fix: BOLT12 arm of UnifiedPayment::send returns DuplicatePayment/PersistenceFailed errors The BOLT12 arm of UnifiedPayment::send was using a .map_err() + if let Ok pattern that caused Critical errors (DuplicatePayment, PersistenceFailed) to fall through to the BOLT11/onchain payment methods. This could result in double-payment if: 1. A BOLT12 payment fails with PersistenceFailed (e.g., payment store insert fails after ChannelManager accepted the payment) 2. The code falls through to BOLT11 attempt 3. BOLT11 also fails with PersistenceFailed 4. The code falls through to on-chain broadcast 5. User is charged twice (once via Lightning, once via on-chain) This fix mirrors the BOLT11 arm fix in PR #1038. Both DuplicatePayment and PersistenceFailed are now terminal errors that abort the unified payment entirely. Fixes: #1060 --- src/payment/unified.rs | 46 +++++++++++--- tests/integration_tests_rust.rs | 104 +++++++++++++++++++++++++++++--- 2 files changed, 132 insertions(+), 18 deletions(-) diff --git a/src/payment/unified.rs b/src/payment/unified.rs index e10d57ba00..1acc4c56f6 100644 --- a/src/payment/unified.rs +++ b/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) => { diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index 6650754f55..1f7e0bc481 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; @@ -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, + 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) +} + // 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