From f4b1c1be3b51904ddde4ea0de5856dd1e9e41529 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 12 Sep 2024 15:21:21 -0400 Subject: [PATCH] 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 monitor is successfully persisted, but the ChannelManager fails to persist, so the outbound payment remains AwaitingInvoice - We restart, causing the channel 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/channelmanager.rs | 6 ++- lightning/src/ln/offers_tests.rs | 73 +++++++++++++++++++++++++++- lightning/src/ln/outbound_payment.rs | 34 +++++++++++++ 3 files changed, 111 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 5f8dc1e5541..367397a75b8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -12425,7 +12425,11 @@ where 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); + let newly_added = entry + .get_mut() + .insert_from_monitor_on_startup( + session_priv_bytes, &path, htlc.payment_hash, best_block_height + ); 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); }, diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index 3b9317e4674..7c5d51f2aa2 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,73 @@ 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 ChannelMonitor + // - The monitor is successfully persisted, but the ChannelManager fails to persist, so the + // payment remains AwaitingInvoice + // - We restart, causing the channel 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. + 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 = 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 offer = nodes[1].node + .create_offer_builder(None).unwrap() + .clear_paths() + .amount_msats(10_000_000) + .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 (invoice, _) = extract_invoice(&nodes[0], &invoice_om); + route_bolt12_payment(&nodes[0], &[&nodes[1]], &invoice); + 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 = get_monitor!(nodes[0], chan_id).encode(); + reload_node!(nodes[0], &alice_chan_manager_serialized, &[&monitor], persister, chain_monitor, alice_deserialized); + // The stale manager results in closing the channel. + check_closed_event!(nodes[0], 1, ClosureReason::OutdatedChannelManager, [bob_id], 10_000_000); + check_added_monitors!(nodes[0], 1); + + // 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 we would've attempted to pay the invoice a 2nd time, resulting in a PaymentFailed + // event here. + 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 ca0d7c17d99..ef20ef99e82 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -282,6 +282,40 @@ impl PendingOutboundPayment { insert_res } + pub(super) fn insert_from_monitor_on_startup( + &mut self, session_priv: [u8; 32], path: &Path, payment_hash: PaymentHash, + best_block_height: u32 + ) -> bool { + match self { + PendingOutboundPayment::AwaitingInvoice { .. } | + PendingOutboundPayment::InvoiceReceived { .. } => { + // 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. + *self = Self::Retryable { + retry_strategy: None, + attempts: PaymentAttempts::new(), + payment_params: None, + session_privs: hash_set_from_iter([session_priv]), + payment_hash, + payment_secret: None, + payment_metadata: None, + keysend_preimage: None, + custom_tlvs: Vec::new(), + pending_amt_msat: path.final_value_msat(), + pending_fee_msat: Some(path.fee_msat()), + total_msat: path.final_value_msat(), + starting_block_height: best_block_height, + remaining_max_total_routing_fee_msat: None, + }; + } + _ => {} + } + self.insert(session_priv, path) + } + pub(super) fn remaining_parts(&self) -> usize { match self { PendingOutboundPayment::Legacy { session_privs } |