diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b87c2ebf3b..5c69a1a714 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/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 7e011e65ef..1c5af0d8f0 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 1e774ca98a..947ec4a130 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/offers_tests.rs b/lightning/src/ln/offers_tests.rs index b8e6fca5bb..a9805cfedf 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 7a850889d4..709e834159 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -2121,6 +2121,68 @@ 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(); + let path_fee = path.fee_msat(); + + macro_rules! new_retryable { + () => { + 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 + } + } + } + + 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)); + } + } + } } /// Returns whether a payment with the given [`PaymentHash`] and [`PaymentId`] is, in fact, a diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs index 248f2dfb0a..22854952c5 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 b81eda3098..5d4f1540fb 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;