Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle self payments #2573

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 99 additions & 6 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,7 @@ struct ClaimablePayment {
purpose: events::PaymentPurpose,
onion_fields: Option<RecipientOnionFields>,
htlcs: Vec<ClaimableHTLC>,
amount_msat: Option<u64>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of the new field here?

}

/// Information about claimable or being-claimed payments
Expand All @@ -697,6 +698,17 @@ struct ClaimablePayments {
pending_claiming_payments: HashMap<PaymentHash, ClaimingPayment>,
}

/// Information about self claimable payments.
struct ClaimableSelfPayments {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need the struct indirection?

claimable_payments: HashMap<PaymentHash, ClaimableSelfPayment>
}

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
Expand Down Expand Up @@ -1267,6 +1279,12 @@ where
/// See `ChannelManager` struct-level documentation for lock order requirements.
claimable_payments: Mutex<ClaimablePayments>,

/// 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<ClaimableSelfPayments>,

/// 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
Expand Down Expand Up @@ -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()),
Expand Down Expand Up @@ -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<PaymentPreimage> = 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't fully decided if this is a self payment or not at this point - we call send_payment_internal which may call find_route_with_id and get a rebalance route rather than an actual send-to-self. Thus, we need to check with the outbound payments logic to see if its really a self-pay first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our current code in find_route_with_id will throw an error if you attempt to find a route to yourself, so I don't think you will get a rebalance route.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our router, yes, but we cannot rely on that as users can provide their own custom router.

}
}
}
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)]
Expand Down Expand Up @@ -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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When updating or inserting a ClaimablePayment instance, the amount_msat field is not updated. This omission could lead to inconsistencies in tracking and managing claimable payments. Ensure that amount_msat is correctly handled in all scenarios where ClaimablePayment is updated or inserted.

}
});
if $purpose != claimable_payment.purpose {
Expand Down Expand Up @@ -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() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand the reason for this now that we have a separate self-payment map.

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)
Expand Down Expand Up @@ -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();
Expand All @@ -10126,6 +10195,14 @@ where
htlc_onion_fields.push(&payment.onion_fields);
}

(claimable_self_payments.claimable_payments.len() as u64).write(writer)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New fields have to be written with TLVs, as written here it would mean upgraded LDK can't read previous versions of LDK's data (and vice versa).

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() {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Comment on lines +11159 to +11166
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handling of ClaimablePayment insertion during decoding does not account for the amount_msat field, potentially leading to incomplete data. Ensure that amount_msat is correctly handled during the decoding process to maintain data integrity.

});
if existing_payment.is_some() { return Err(DecodeError::InvalidValue); }
}
Expand Down Expand Up @@ -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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to previous comments, the insertion of ClaimablePayment for spontaneous payments does not include the amount_msat field, potentially affecting data integrity. Ensure that amount_msat is correctly handled for spontaneous payments to maintain consistency and integrity.

});
}
}
Expand Down Expand Up @@ -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),
Expand Down
54 changes: 44 additions & 10 deletions lightning/src/ln/outbound_payment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -170,7 +170,7 @@ impl PendingOutboundPayment {
}
}

fn mark_fulfilled(&mut self) {
pub fn mark_fulfilled(&mut self) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The visibility of the mark_fulfilled method is changed to public.

Changing the visibility of methods can have broader implications on how they are used throughout the codebase. It's essential to ensure that this method's public visibility does not allow for misuse or unintended side effects, especially since it marks payments as fulfilled which is a critical operation.

-    fn mark_fulfilled(&mut self) {
+    pub fn mark_fulfilled(&mut self) {

Consider adding documentation to clarify the intended use cases and any precautions that should be taken when calling this method.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
pub 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 } |
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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::<Vec<_>>()), 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");
Expand Down
31 changes: 31 additions & 0 deletions lightning/src/ln/payment_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading