From 10bd39bb10ffecce81c0c04436870d76a621ebc0 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Wed, 13 Sep 2023 19:31:03 +0300 Subject: [PATCH 1/2] feat: allow self payment This PR solves issue #2462. If we asked to pay an invoice that we generated ourselves. We generate PaymentSent and PaymentClaimable event and mark the payment as fulfilled in our set of outbound payments. This PR is important because we realized users can easily screw up self payments when they implement it themselves. See here: https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-June/003983.html --- lightning/src/ln/channelmanager.rs | 105 +++++++++++++++++++++++++-- lightning/src/ln/outbound_payment.rs | 54 +++++++++++--- 2 files changed, 143 insertions(+), 16 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 11413db612a..06e22bd4c35 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -677,6 +677,7 @@ struct ClaimablePayment { purpose: events::PaymentPurpose, onion_fields: Option, htlcs: Vec, + amount_msat: Option, } /// Information about claimable or being-claimed payments @@ -697,6 +698,17 @@ struct ClaimablePayments { pending_claiming_payments: HashMap, } +/// Information about self claimable payments. +struct ClaimableSelfPayments { + claimable_payments: HashMap +} + +struct ClaimableSelfPayment { + purpose: events::PaymentPurpose, + amount_msat: u64, + payment_id: PaymentId, +} + /// Events which we process internally but cannot be processed immediately at the generation site /// usually because we're running pre-full-init. They are handled immediately once we detect we are /// running normally, and specifically must be processed before any other non-background @@ -1267,6 +1279,12 @@ where /// See `ChannelManager` struct-level documentation for lock order requirements. claimable_payments: Mutex, + /// The set of self payments which are claimable. See [`ClaimableSelfPayments`] + /// individual field docs for more info. + /// + /// See `ChannelManager` struct-level documentation for lock order requirements. + claimable_self_payments: Mutex, + /// The set of outbound SCID aliases across all our channels, including unconfirmed channels /// and some closed channels which reached a usable state prior to being closed. This is used /// only to avoid duplicates, and is not persisted explicitly to disk, but rebuilt from the @@ -2468,6 +2486,7 @@ where pending_outbound_payments: OutboundPayments::new(), forward_htlcs: Mutex::new(new_hash_map()), claimable_payments: Mutex::new(ClaimablePayments { claimable_payments: new_hash_map(), pending_claiming_payments: new_hash_map() }), + claimable_self_payments: Mutex::new(ClaimableSelfPayments { claimable_payments: new_hash_map() }), pending_intercepted_htlcs: Mutex::new(new_hash_map()), outpoint_to_peer: Mutex::new(new_hash_map()), short_to_chan_info: FairRwLock::new(new_hash_map()), @@ -3549,11 +3568,38 @@ where pub fn send_payment(&self, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry) -> Result<(), RetryableSendFailure> { let best_block_height = self.best_block.read().unwrap().height; let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); + let mut preimage: Option = None; + let mut payment_secret = PaymentSecret([0; 32]); + let mut is_self_pay = false; + if let Some(secret) = recipient_onion.payment_secret { + payment_secret = secret; + if let Payee::Clear{ node_id, .. } = route_params.payment_params.payee { + let is_phantom_payee = match self.node_signer.get_node_id(Recipient::PhantomNode) { + Ok(phantom_node_id) => node_id == phantom_node_id, + Err(_) => false, + }; + if node_id == self.get_our_node_id() || is_phantom_payee { + let payment_data = msgs::FinalOnionHopData{ payment_secret, total_msat: route_params.final_value_msat }; + preimage = inbound_payment::verify(payment_hash, &payment_data, self.highest_seen_timestamp.load(Ordering::Acquire) as u64, &self.inbound_payment_key, &self.logger).map_err(|_| RetryableSendFailure::RecipientRejected)?.0; + is_self_pay = true; + } + } + } self.pending_outbound_payments - .send_payment(payment_hash, recipient_onion, payment_id, retry_strategy, route_params, + .send_payment(payment_hash, recipient_onion.clone(), payment_id, retry_strategy, route_params.clone(), &self.router, self.list_usable_channels(), || self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height, &self.logger, - &self.pending_events, |args| self.send_payment_along_path(args)) + &self.pending_events, |args| self.send_payment_along_path(args))?; + + if is_self_pay { + let mut claimable_self_payments = self.claimable_self_payments.lock().unwrap(); + let purpose = events::PaymentPurpose::InvoicePayment { payment_preimage: preimage, payment_secret }; + claimable_self_payments.claimable_payments.insert(payment_hash, ClaimableSelfPayment{ purpose: purpose.clone(), amount_msat: route_params.final_value_msat, payment_id }); + let mut pending_events = self.pending_events.lock().unwrap(); + pending_events.push_back((events::Event::PaymentClaimable { receiver_node_id: Some(self.get_our_node_id()), payment_hash, onion_fields: Some(recipient_onion), amount_msat: route_params.final_value_msat, counterparty_skimmed_fee_msat: 0, purpose, via_channel_id: None, via_user_channel_id: None, claim_deadline: None }, None)); + } + + Ok(()) } #[cfg(test)] @@ -4602,7 +4648,7 @@ where .or_insert_with(|| { committed_to_claimable = true; ClaimablePayment { - purpose: $purpose.clone(), htlcs: Vec::new(), onion_fields: None, + purpose: $purpose.clone(), htlcs: Vec::new(), onion_fields: None, amount_msat: None, } }); if $purpose != claimable_payment.purpose { @@ -5446,10 +5492,32 @@ where let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); + // This handles fulfilling and claiming of Self payment. + { + let mut claimable_self_payments = self.claimable_self_payments.lock().unwrap(); + if let Some(payment) = claimable_self_payments.claimable_payments.remove(&payment_hash) { + let mut pending_outbounds_lock = self.pending_outbound_payments.pending_outbound_payments.lock().unwrap(); + let out_payment = pending_outbounds_lock.get_mut(&payment.payment_id).unwrap(); + out_payment.mark_fulfilled(); + let mut pending_events_lock = self.pending_events.lock().unwrap(); + pending_events_lock.push_back((Event::PaymentSent { payment_id: Some(payment.payment_id), payment_preimage, + payment_hash, fee_paid_msat: None }, None)); + pending_events_lock.push_back((Event::PaymentClaimed { receiver_node_id: None, payment_hash, + amount_msat: payment.amount_msat, purpose: payment.purpose, htlcs: vec![], sender_intended_total_msat: None }, None)); + return; + } + } + let mut sources = { let mut claimable_payments = self.claimable_payments.lock().unwrap(); if let Some(payment) = claimable_payments.claimable_payments.remove(&payment_hash) { let mut receiver_node_id = self.our_network_pubkey; + if payment.htlcs.is_empty() { + let mut pending_events_lock = self.pending_events.lock().unwrap(); + pending_events_lock.push_back((Event::PaymentClaimed { receiver_node_id: Some(receiver_node_id), payment_hash, + amount_msat: payment.amount_msat.unwrap(), purpose: payment.purpose, htlcs: vec![], sender_intended_total_msat: None }, None)); + return; + } for htlc in payment.htlcs.iter() { if htlc.prev_hop.phantom_shared_secret.is_some() { let phantom_pubkey = self.node_signer.get_node_id(Recipient::PhantomNode) @@ -10111,6 +10179,7 @@ where let pending_inbound_payments = self.pending_inbound_payments.lock().unwrap(); let claimable_payments = self.claimable_payments.lock().unwrap(); + let claimable_self_payments = self.claimable_self_payments.lock().unwrap(); let pending_outbound_payments = self.pending_outbound_payments.pending_outbound_payments.lock().unwrap(); let mut htlc_purposes: Vec<&events::PaymentPurpose> = Vec::new(); @@ -10126,6 +10195,14 @@ where htlc_onion_fields.push(&payment.onion_fields); } + (claimable_self_payments.claimable_payments.len() as u64).write(writer)?; + for (payment_hash, payment) in claimable_self_payments.claimable_payments.iter() { + payment_hash.write(writer)?; + payment.purpose.write(writer)?; + payment.amount_msat.write(writer)?; + payment.payment_id.write(writer)?; + } + let mut monitor_update_blocked_actions_per_peer = None; let mut peer_states = Vec::new(); for (_, peer_state_mutex) in per_peer_state.iter() { @@ -10639,6 +10716,21 @@ where claimable_htlcs_list.push((payment_hash, previous_hops)); } + let claimable_self_payment_count: u64 = Readable::read(reader)?; + let mut claimable_self_payments = hash_map_with_capacity(claimable_self_payment_count as usize); + for _ in 0..claimable_self_payment_count { + //read each payment details and add an entry to the map of claimable payments. + let payment_hash = Readable::read(reader)?; + let purpose = Readable::read(reader)?; + let amount_msat = Readable::read(reader)?; + let payment_id = Readable::read(reader)?; + claimable_self_payments.insert(payment_hash, ClaimableSelfPayment { + purpose, + amount_msat, + payment_id, + }); + } + let peer_state_from_chans = |channel_by_id| { PeerState { channel_by_id, @@ -11064,14 +11156,14 @@ where purposes.into_iter().zip(onion_fields.into_iter().zip(claimable_htlcs_list.into_iter())) { let existing_payment = claimable_payments.insert(payment_hash, ClaimablePayment { - purpose, htlcs, onion_fields: onion, + purpose, htlcs, onion_fields: onion, amount_msat: None, }); if existing_payment.is_some() { return Err(DecodeError::InvalidValue); } } } else { for (purpose, (payment_hash, htlcs)) in purposes.into_iter().zip(claimable_htlcs_list.into_iter()) { let existing_payment = claimable_payments.insert(payment_hash, ClaimablePayment { - purpose, htlcs, onion_fields: None, + purpose, htlcs, onion_fields: None, amount_msat: None, }); if existing_payment.is_some() { return Err(DecodeError::InvalidValue); } } @@ -11105,7 +11197,7 @@ where events::PaymentPurpose::SpontaneousPayment(*payment_preimage), }; claimable_payments.insert(payment_hash, ClaimablePayment { - purpose, htlcs, onion_fields: None, + purpose, htlcs, onion_fields: None, amount_msat: None, }); } } @@ -11272,6 +11364,7 @@ where forward_htlcs: Mutex::new(forward_htlcs), claimable_payments: Mutex::new(ClaimablePayments { claimable_payments, pending_claiming_payments: pending_claiming_payments.unwrap() }), + claimable_self_payments: Mutex::new(ClaimableSelfPayments { claimable_payments: claimable_self_payments }), outbound_scid_aliases: Mutex::new(outbound_scid_aliases), outpoint_to_peer: Mutex::new(outpoint_to_peer), short_to_chan_info: FairRwLock::new(short_to_chan_info), diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index b05d6f3f729..b3abf885fba 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -19,7 +19,7 @@ use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret}; use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, PaymentId}; use crate::ln::onion_utils::{DecodedOnionFailure, HTLCFailReason}; use crate::offers::invoice::Bolt12Invoice; -use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router}; +use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router, Payee}; use crate::util::errors::APIError; use crate::util::logger::Logger; use crate::util::time::Time; @@ -170,7 +170,7 @@ impl PendingOutboundPayment { } } - fn mark_fulfilled(&mut self) { + pub fn mark_fulfilled(&mut self) { let mut session_privs = new_hash_set(); core::mem::swap(&mut session_privs, match self { PendingOutboundPayment::Legacy { session_privs } | @@ -421,6 +421,8 @@ pub enum RetryableSendFailure { /// [`Event::PaymentSent`]: crate::events::Event::PaymentSent /// [`Event::PaymentFailed`]: crate::events::Event::PaymentFailed DuplicatePayment, + /// The intended recipient rejected our payment. + RecipientRejected, } /// If a payment fails to send with [`ChannelManager::send_payment_with_route`], it can be in one @@ -907,16 +909,48 @@ impl OutboundPayments { } } - let mut route = router.find_route_with_id( - &node_signer.get_node_id(Recipient::Node).unwrap(), &route_params, + let payer = node_signer.get_node_id(Recipient::Node).unwrap(); + let route = match router.find_route_with_id( + &payer,&route_params, Some(&first_hops.iter().collect::>()), inflight_htlcs(), payment_hash, payment_id, - ).map_err(|_| { - log_error!(logger, "Failed to find route for payment with id {} and hash {}", - payment_id, payment_hash); - RetryableSendFailure::RouteNotFound - })?; - + ) { + Ok(res) => Some(res), + Err(_) => { + // The following code handles self payments. + if let Payee::Clear{node_id, .. } = route_params.payment_params.payee { + let is_phantom_payee = match node_signer.get_node_id(Recipient::PhantomNode) { + Ok(phantom_node_id) => node_id == phantom_node_id, + Err(_) => false, + }; + if node_id == payer || is_phantom_payee { + let dummy_route = Route { + paths: vec![Path { + hops: vec![], + blinded_tail: None, + }], + route_params: Some(route_params.clone()), + }; + + let _ = self.add_new_pending_payment(payment_hash, + recipient_onion.clone(), payment_id, keysend_preimage, &dummy_route, Some(retry_strategy), + Some(route_params.payment_params.clone()), entropy_source, best_block_height) + .map_err(|_| { + log_error!(logger, "Payment with id {} is already pending. New payment had payment hash {}", + payment_id, payment_hash); + RetryableSendFailure::DuplicatePayment + })?; + return Ok(()); + } + } + None + } + }; + if route.is_none() { + log_error!(logger, "Failed to find route for payment with id {} and hash {}", payment_id, payment_hash); + return Err(RetryableSendFailure::RouteNotFound); + } + let mut route = route.unwrap(); if route.route_params.as_ref() != Some(&route_params) { debug_assert!(false, "Routers are expected to return a Route which includes the requested RouteParameters"); From 2880c552964139134d22a021661220de8f9f9ee8 Mon Sep 17 00:00:00 2001 From: Vladimir Fomene Date: Thu, 21 Sep 2023 15:10:53 +0300 Subject: [PATCH 2/2] test: add test for self payment --- lightning/src/ln/payment_tests.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 09c022a4c87..06621cdb116 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -1552,6 +1552,37 @@ fn preflight_probes_yield_event_and_skip() { assert!(!nodes[0].node.has_pending_payments()); } +#[test] +fn test_self_payment() { + let chanmon_cfg = create_chanmon_cfgs(1); + let node_cfg = create_node_cfgs(1, &chanmon_cfg); + let node_chanmgr = create_node_chanmgrs(1, &node_cfg, &[None, None]); + let nodes = create_network(1, &node_cfg, &node_chanmgr); + let (payment_hash, payment_secret) = nodes[0].node.create_inbound_payment(Some(1000), 60, None).unwrap(); + let payment_params = PaymentParameters::from_node_id(nodes[0].node.get_our_node_id(), TEST_FINAL_CLTV); + let route_params = RouteParameters { + payment_params, + final_value_msat: 100000, + max_total_routing_fee_msat: None, + }; + let res = nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(0)); + assert!(res.is_ok()); + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + assert!(matches!(events[0], Event::PaymentClaimable { .. })); + let pending_payments = nodes[0].node.list_recent_payments(); + assert_eq!(pending_payments.len(), 1); + let payment_preimage = nodes[0].node.get_payment_preimage(payment_hash, payment_secret).unwrap(); + assert!(matches!(pending_payments[0], RecentPaymentDetails::Pending { .. })); + nodes[0].node.claim_funds(payment_preimage); + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2); + assert!(matches!(events[0], Event::PaymentSent { .. })); + assert!(matches!(events[1], Event::PaymentClaimed { .. })); + let pending_payments = nodes[0].node.list_recent_payments(); + assert!(matches!(pending_payments[0], RecentPaymentDetails::Fulfilled { .. })); +} + #[test] fn claimed_send_payment_idempotent() { // Tests that `send_payment` (and friends) are (reasonably) idempotent.