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

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;