From 8c356d865819910bcde314b60837806a1bf69a56 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 12 Sep 2024 13:18:03 -0400 Subject: [PATCH 1/3] Test utils: use full import path in reload_node macro. --- lightning/src/ln/functional_test_utils.rs | 4 ++-- lightning/src/ln/monitor_tests.rs | 1 - lightning/src/ln/priv_short_conf_tests.rs | 1 - lightning/src/ln/reorg_tests.rs | 1 - 4 files changed, 2 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 7e011e65efd..1c5af0d8f06 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1144,8 +1144,8 @@ macro_rules! reload_node { ($node: expr, $new_config: expr, $chanman_encoded: expr, $monitors_encoded: expr, $persister: ident, $new_chain_monitor: ident, $new_channelmanager: ident) => { let chanman_encoded = $chanman_encoded; - $persister = test_utils::TestPersister::new(); - $new_chain_monitor = test_utils::TestChainMonitor::new(Some($node.chain_source), $node.tx_broadcaster.clone(), $node.logger, $node.fee_estimator, &$persister, &$node.keys_manager); + $persister = $crate::util::test_utils::TestPersister::new(); + $new_chain_monitor = $crate::util::test_utils::TestChainMonitor::new(Some($node.chain_source), $node.tx_broadcaster.clone(), $node.logger, $node.fee_estimator, &$persister, &$node.keys_manager); $node.chain_monitor = &$new_chain_monitor; $new_channelmanager = _reload_node(&$node, $new_config, &chanman_encoded, $monitors_encoded); diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 1e774ca98ab..947ec4a1304 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -23,7 +23,6 @@ use crate::ln::msgs::ChannelMessageHandler; use crate::crypto::utils::sign; use crate::util::ser::Writeable; use crate::util::scid_utils::block_from_scid; -use crate::util::test_utils; use bitcoin::{Amount, PublicKey, ScriptBuf, Transaction, TxIn, TxOut, Witness}; use bitcoin::locktime::absolute::LockTime; diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs index 248f2dfb0ac..22854952c58 100644 --- a/lightning/src/ln/priv_short_conf_tests.rs +++ b/lightning/src/ln/priv_short_conf_tests.rs @@ -24,7 +24,6 @@ use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ChannelUpdat use crate::ln::wire::Encode; use crate::util::config::{UserConfig, MaxDustHTLCExposure}; use crate::util::ser::Writeable; -use crate::util::test_utils; use crate::prelude::*; diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index b81eda30985..5d4f1540fb9 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -17,7 +17,6 @@ use crate::events::{Event, MessageSendEventsProvider, ClosureReason, HTLCDestina use crate::ln::msgs::{ChannelMessageHandler, Init}; use crate::ln::types::ChannelId; use crate::sign::OutputSpender; -use crate::util::test_utils; use crate::util::ser::Writeable; use crate::util::string::UntrustedString; From 3c2da4147cf485a66317d455791c09d142579d31 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 16 Sep 2024 12:11:59 -0400 Subject: [PATCH 2/3] Move monitor<>outbound_payments startup htlc syncing code. Move the code that ensures that HTLCs locked into ChannelMonitors are synchronized with the ChannelManager's OutboundPayments store to the outbound_payments module. This is useful both because ChannelManager::read is very long/confusing method, so it's nice to encapsulate some of its functionality, and because we need to fix an existing bug in this logic where we may risk double-paying an offer due to outbound_payments being stale on startup. See the next commit for this bugfix. --- lightning/src/ln/channelmanager.rs | 34 ++++----------------------- lightning/src/ln/outbound_payment.rs | 35 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 03164e04fb5..c690ca9f506 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -61,7 +61,7 @@ use crate::ln::onion_utils::{HTLCFailReason, INVALID_ONION_BLINDING}; use crate::ln::msgs::{ChannelMessageHandler, DecodeError, LightningError}; #[cfg(test)] use crate::ln::outbound_payment; -use crate::ln::outbound_payment::{OutboundPayments, PaymentAttempts, PendingOutboundPayment, RetryableInvoiceRequest, SendAlongPathArgs, StaleExpiration}; +use crate::ln::outbound_payment::{OutboundPayments, PendingOutboundPayment, RetryableInvoiceRequest, SendAlongPathArgs, StaleExpiration}; use crate::ln::wire::Encode; use crate::offers::invoice::{Bolt12Invoice, DEFAULT_RELATIVE_EXPIRY, DerivedSigningPubkey, ExplicitSigningPubkey, InvoiceBuilder, UnsignedBolt12Invoice}; use crate::offers::invoice_error::InvoiceError; @@ -12569,37 +12569,11 @@ where return Err(DecodeError::InvalidValue); } - let path_amt = path.final_value_msat(); let mut session_priv_bytes = [0; 32]; session_priv_bytes[..].copy_from_slice(&session_priv[..]); - match pending_outbounds.pending_outbound_payments.lock().unwrap().entry(payment_id) { - hash_map::Entry::Occupied(mut entry) => { - let newly_added = entry.get_mut().insert(session_priv_bytes, &path); - log_info!(logger, "{} a pending payment path for {} msat for session priv {} on an existing pending payment with payment hash {}", - if newly_added { "Added" } else { "Had" }, path_amt, log_bytes!(session_priv_bytes), htlc.payment_hash); - }, - hash_map::Entry::Vacant(entry) => { - let path_fee = path.fee_msat(); - entry.insert(PendingOutboundPayment::Retryable { - retry_strategy: None, - attempts: PaymentAttempts::new(), - payment_params: None, - session_privs: hash_set_from_iter([session_priv_bytes]), - payment_hash: htlc.payment_hash, - payment_secret: None, // only used for retries, and we'll never retry on startup - payment_metadata: None, // only used for retries, and we'll never retry on startup - keysend_preimage: None, // only used for retries, and we'll never retry on startup - custom_tlvs: Vec::new(), // only used for retries, and we'll never retry on startup - pending_amt_msat: path_amt, - pending_fee_msat: Some(path_fee), - total_msat: path_amt, - starting_block_height: best_block_height, - remaining_max_total_routing_fee_msat: None, // only used for retries, and we'll never retry on startup - }); - log_info!(logger, "Added a pending payment for {} msat with payment hash {} for path with session priv {}", - path_amt, &htlc.payment_hash, log_bytes!(session_priv_bytes)); - } - } + pending_outbounds.insert_from_monitor_on_startup( + payment_id, htlc.payment_hash, session_priv_bytes, &path, best_block_height, logger + ); } } for (htlc_source, (htlc, preimage_opt)) in monitor.get_all_current_outbound_htlcs() { diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 7a850889d4c..b3b6348a62d 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -2121,6 +2121,41 @@ impl OutboundPayments { self.awaiting_invoice.store(false, Ordering::Release); invoice_requests } + + pub(super) fn insert_from_monitor_on_startup( + &self, payment_id: PaymentId, payment_hash: PaymentHash, session_priv_bytes: [u8; 32], + path: &Path, best_block_height: u32, logger: L + ) { + let path_amt = path.final_value_msat(); + match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { + hash_map::Entry::Occupied(mut entry) => { + let newly_added = entry.get_mut().insert(session_priv_bytes, &path); + log_info!(logger, "{} a pending payment path for {} msat for session priv {} on an existing pending payment with payment hash {}", + if newly_added { "Added" } else { "Had" }, path_amt, log_bytes!(session_priv_bytes), payment_hash); + }, + hash_map::Entry::Vacant(entry) => { + let path_fee = path.fee_msat(); + entry.insert(PendingOutboundPayment::Retryable { + retry_strategy: None, + attempts: PaymentAttempts::new(), + payment_params: None, + session_privs: hash_set_from_iter([session_priv_bytes]), + payment_hash, + payment_secret: None, // only used for retries, and we'll never retry on startup + payment_metadata: None, // only used for retries, and we'll never retry on startup + keysend_preimage: None, // only used for retries, and we'll never retry on startup + custom_tlvs: Vec::new(), // only used for retries, and we'll never retry on startup + pending_amt_msat: path_amt, + pending_fee_msat: Some(path_fee), + total_msat: path_amt, + starting_block_height: best_block_height, + remaining_max_total_routing_fee_msat: None, // only used for retries, and we'll never retry on startup + }); + log_info!(logger, "Added a pending payment for {} msat with payment hash {} for path with session priv {}", + path_amt, payment_hash, log_bytes!(session_priv_bytes)); + } + } + } } /// Returns whether a payment with the given [`PaymentHash`] and [`PaymentId`] is, in fact, a From fbb3ab2704232a9aa993435b8d3e0867fa0c615b Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 16 Sep 2024 12:26:16 -0400 Subject: [PATCH 3/3] Fix bug where we double-pay an offer due to stale manager This fixes the following bug: - An outbound payment is AwaitingInvoice - We receive an invoice and lock the HTLCs into the relevant ChannelMonitors - The monitors are successfully persisted, but the ChannelManager fails to persist, so the outbound payment remains AwaitingInvoice - We restart, causing the channels to close due to a stale ChannelManager - We receive a duplicate invoice, and attempt to pay it again due to the payment still being AwaitingInvoice in the stale ChannelManager After the fix for this, we will notice that the payment is already locked into the monitor on startup and transition the incorrectly-AwaitingInvoice payment to Retryable, which prevents double-paying on duplicate invoice receipt. --- lightning/src/ln/offers_tests.rs | 92 +++++++++++++++++++++++++++- lightning/src/ln/outbound_payment.rs | 47 +++++++++++--- 2 files changed, 128 insertions(+), 11 deletions(-) diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index 3b9317e4674..87934d202e4 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -47,7 +47,7 @@ use crate::blinded_path::IntroductionNode; use crate::blinded_path::message::BlindedMessagePath; use crate::blinded_path::payment::{Bolt12OfferContext, Bolt12RefundContext, PaymentContext}; use crate::blinded_path::message::{MessageContext, OffersContext}; -use crate::events::{Event, MessageSendEventsProvider, PaymentFailureReason, PaymentPurpose}; +use crate::events::{ClosureReason, Event, MessageSendEventsProvider, PaymentFailureReason, PaymentPurpose}; use crate::ln::channelmanager::{Bolt12PaymentError, MAX_SHORT_LIVED_RELATIVE_EXPIRY, PaymentId, RecentPaymentDetails, Retry, self}; use crate::ln::features::Bolt12InvoiceFeatures; use crate::ln::functional_test_utils::*; @@ -64,6 +64,7 @@ use crate::onion_message::offers::OffersMessage; use crate::onion_message::packet::ParsedOnionMessageContents; use crate::routing::gossip::{NodeAlias, NodeId}; use crate::sign::{NodeSigner, Recipient}; +use crate::util::ser::Writeable; use crate::prelude::*; @@ -2253,3 +2254,92 @@ fn fails_paying_invoice_with_unknown_required_features() { _ => panic!("Expected Event::PaymentFailed with reason"), } } + +#[test] +fn no_double_pay_with_stale_channelmanager() { + // This tests the following bug: + // - An outbound payment is AwaitingInvoice + // - We receive an invoice and lock the HTLCs into the relevant ChannelMonitors + // - The monitors are successfully persisted, but the ChannelManager fails to persist, so the + // payment remains AwaitingInvoice + // - We restart, causing the channels to close due to a stale ChannelManager + // - We receive a duplicate invoice, and attempt to pay it again due to the payment still being + // AwaitingInvoice in the stale ChannelManager + // After the fix for this, we will notice that the payment is already locked into the monitors on + // startup and transition the incorrectly-AwaitingInvoice payment to Retryable, which prevents + // double-paying on duplicate invoice receipt. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let persister; + let chain_monitor; + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let alice_deserialized; + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + let chan_id_0 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000).2; + let chan_id_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000).2; + + let alice_id = nodes[0].node.get_our_node_id(); + let bob_id = nodes[1].node.get_our_node_id(); + + let amt_msat = nodes[0].node.list_usable_channels()[0].next_outbound_htlc_limit_msat + 1; // Force MPP + let offer = nodes[1].node + .create_offer_builder(None).unwrap() + .clear_paths() + .amount_msats(amt_msat) + .build().unwrap(); + assert_eq!(offer.signing_pubkey(), Some(bob_id)); + assert!(offer.paths().is_empty()); + + let payment_id = PaymentId([1; 32]); + nodes[0].node.pay_for_offer(&offer, None, None, None, payment_id, Retry::Attempts(0), None).unwrap(); + expect_recent_payment!(nodes[0], RecentPaymentDetails::AwaitingInvoice, payment_id); + + let invreq_om = nodes[0].onion_messenger.next_onion_message_for_peer(bob_id).unwrap(); + nodes[1].onion_messenger.handle_onion_message(alice_id, &invreq_om); + + // Save the manager while the payment is in state AwaitingInvoice so we can reload it later. + let alice_chan_manager_serialized = nodes[0].node.encode(); + + let invoice_om = nodes[1].onion_messenger.next_onion_message_for_peer(alice_id).unwrap(); + nodes[0].onion_messenger.handle_onion_message(bob_id, &invoice_om); + let payment_hash = extract_invoice(&nodes[0], &invoice_om).0.payment_hash(); + + let expected_route: &[&[&Node]] = &[&[&nodes[1]], &[&nodes[1]]]; + let mut events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 2); + check_added_monitors!(nodes[0], 2); + + let ev = remove_first_msg_event_to_node(&bob_id, &mut events); + let args = PassAlongPathArgs::new(&nodes[0], expected_route[0], amt_msat, payment_hash, ev) + .without_clearing_recipient_events(); + do_pass_along_path(args); + + let ev = remove_first_msg_event_to_node(&bob_id, &mut events); + let args = PassAlongPathArgs::new(&nodes[0], expected_route[0], amt_msat, payment_hash, ev) + .without_clearing_recipient_events(); + do_pass_along_path(args); + + expect_recent_payment!(nodes[0], RecentPaymentDetails::Pending, payment_id); + match get_event!(nodes[1], Event::PaymentClaimable) { + Event::PaymentClaimable { .. } => {}, + _ => panic!("No Event::PaymentClaimable"), + } + + // Reload with the stale manager and check that receiving the invoice again won't result in a + // duplicate payment attempt. + let monitor_0 = get_monitor!(nodes[0], chan_id_0).encode(); + let monitor_1 = get_monitor!(nodes[0], chan_id_1).encode(); + reload_node!(nodes[0], &alice_chan_manager_serialized, &[&monitor_0, &monitor_1], persister, chain_monitor, alice_deserialized); + // The stale manager results in closing the channels. + check_closed_event!(nodes[0], 2, ClosureReason::OutdatedChannelManager, [bob_id, bob_id], 10_000_000); + check_added_monitors!(nodes[0], 2); + + // Alice receives a duplicate invoice, but the payment should be transitioned to Retryable by now. + nodes[0].onion_messenger.handle_onion_message(bob_id, &invoice_om); + // Previously, Alice would've attempted to pay the invoice a 2nd time. In this test case, this 2nd + // attempt would have resulted in a PaymentFailed event here, since the only channels between + // Alice and Bob is closed. Since no 2nd attempt should be made, check that no events are + // generated in response to the duplicate invoice. + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); +} + diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index b3b6348a62d..709e8341593 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -2127,15 +2127,11 @@ impl OutboundPayments { path: &Path, best_block_height: u32, logger: L ) { let path_amt = path.final_value_msat(); - match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { - hash_map::Entry::Occupied(mut entry) => { - let newly_added = entry.get_mut().insert(session_priv_bytes, &path); - log_info!(logger, "{} a pending payment path for {} msat for session priv {} on an existing pending payment with payment hash {}", - if newly_added { "Added" } else { "Had" }, path_amt, log_bytes!(session_priv_bytes), payment_hash); - }, - hash_map::Entry::Vacant(entry) => { - let path_fee = path.fee_msat(); - entry.insert(PendingOutboundPayment::Retryable { + let path_fee = path.fee_msat(); + + macro_rules! new_retryable { + () => { + PendingOutboundPayment::Retryable { retry_strategy: None, attempts: PaymentAttempts::new(), payment_params: None, @@ -2150,7 +2146,38 @@ impl OutboundPayments { total_msat: path_amt, starting_block_height: best_block_height, remaining_max_total_routing_fee_msat: None, // only used for retries, and we'll never retry on startup - }); + } + } + } + + match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { + hash_map::Entry::Occupied(mut entry) => { + let newly_added = match entry.get() { + PendingOutboundPayment::AwaitingInvoice { .. } | + PendingOutboundPayment::InvoiceReceived { .. } | + PendingOutboundPayment::StaticInvoiceReceived { .. } => + { + // If we've reached this point, it means we initiated a payment to a BOLT 12 invoice and + // locked the htlc(s) into the `ChannelMonitor`(s), but failed to persist the + // `ChannelManager` after transitioning from this state to `Retryable` prior to shutdown. + // Therefore, we need to move this payment to `Retryable` now to avoid double-paying if + // the recipient sends a duplicate invoice or release_held_htlc onion message. + *entry.get_mut() = new_retryable!(); + true + }, + PendingOutboundPayment::Legacy { .. } | + PendingOutboundPayment::Retryable { .. } | + PendingOutboundPayment::Fulfilled { .. } | + PendingOutboundPayment::Abandoned { .. } => + { + entry.get_mut().insert(session_priv_bytes, &path) + } + }; + log_info!(logger, "{} a pending payment path for {} msat for session priv {} on an existing pending payment with payment hash {}", + if newly_added { "Added" } else { "Had" }, path_amt, log_bytes!(session_priv_bytes), payment_hash); + }, + hash_map::Entry::Vacant(entry) => { + entry.insert(new_retryable!()); log_info!(logger, "Added a pending payment for {} msat with payment hash {} for path with session priv {}", path_amt, payment_hash, log_bytes!(session_priv_bytes)); }