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/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 5f4a95b7eb..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; @@ -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, + 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 @@ -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();