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.

Author: valentinewallace

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

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

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 } |