Merge pull request lightningdevkit#3313 from valentinewallace/2024-09-fix-offer-double-pay

Don't pay a duplicate BOLT 12 invoice if `ChannelManager` is stale

Author: TheBlueMatt

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() {

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);

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;

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());
+}
+

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<L: Logger>(
+		&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

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::*;
 

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;