Skip to content

Update send_payment_along_path to take its args as struct #2370

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 24, 2023
Merged

Update send_payment_along_path to take its args as struct #2370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 23 additions & 24 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ use crate::ln::onion_utils::HTLCFailReason;
use crate::ln::msgs::{ChannelMessageHandler, DecodeError, LightningError};
#[cfg(test)]
use crate::ln::outbound_payment;
use crate::ln::outbound_payment::{OutboundPayments, PaymentAttempts, PendingOutboundPayment};
use crate::ln::outbound_payment::{OutboundPayments, PaymentAttempts, PendingOutboundPayment, SendAlongPathArgs};
use crate::ln::wire::Encode;
use crate::sign::{EntropySource, KeysManager, NodeSigner, Recipient, SignerProvider, ChannelSigner, WriteableEcdsaChannelSigner};
use crate::util::config::{UserConfig, ChannelConfig, ChannelConfigUpdate};
Expand Down Expand Up @@ -2992,10 +2992,17 @@ where
#[cfg(test)]
pub(crate) fn test_send_payment_along_path(&self, path: &Path, payment_hash: &PaymentHash, recipient_onion: RecipientOnionFields, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>, session_priv_bytes: [u8; 32]) -> Result<(), APIError> {
let _lck = self.total_consistency_lock.read().unwrap();
self.send_payment_along_path(path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv_bytes)
self.send_payment_along_path(SendAlongPathArgs {
path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage,
session_priv_bytes
})
}

fn send_payment_along_path(&self, path: &Path, payment_hash: &PaymentHash, recipient_onion: RecipientOnionFields, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>, session_priv_bytes: [u8; 32]) -> Result<(), APIError> {
fn send_payment_along_path(&self, args: SendAlongPathArgs) -> Result<(), APIError> {
let SendAlongPathArgs {
path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage,
session_priv_bytes
} = args;
// The top-level caller should hold the total_consistency_lock read lock.
debug_assert!(self.total_consistency_lock.try_write().is_err());

Expand Down Expand Up @@ -3125,9 +3132,9 @@ where
let best_block_height = self.best_block.read().unwrap().height();
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
self.pending_outbound_payments
.send_payment_with_route(route, payment_hash, recipient_onion, payment_id, &self.entropy_source, &self.node_signer, best_block_height,
|path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv))
.send_payment_with_route(route, payment_hash, recipient_onion, payment_id,
&self.entropy_source, &self.node_signer, best_block_height,
|args| self.send_payment_along_path(args))
}

/// Similar to [`ChannelManager::send_payment_with_route`], but will automatically find a route based on
Expand All @@ -3139,18 +3146,16 @@ where
.send_payment(payment_hash, recipient_onion, payment_id, retry_strategy, route_params,
&self.router, self.list_usable_channels(), || self.compute_inflight_htlcs(),
&self.entropy_source, &self.node_signer, best_block_height, &self.logger,
&self.pending_events,
|path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv))
&self.pending_events, |args| self.send_payment_along_path(args))
}

#[cfg(test)]
pub(super) fn test_send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
let best_block_height = self.best_block.read().unwrap().height();
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
self.pending_outbound_payments.test_send_payment_internal(route, payment_hash, recipient_onion, keysend_preimage, payment_id, recv_value_msat, onion_session_privs, &self.node_signer, best_block_height,
|path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv))
self.pending_outbound_payments.test_send_payment_internal(route, payment_hash, recipient_onion,
keysend_preimage, payment_id, recv_value_msat, onion_session_privs, &self.node_signer,
best_block_height, |args| self.send_payment_along_path(args))
}

#[cfg(test)]
Expand Down Expand Up @@ -3204,9 +3209,7 @@ where
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
self.pending_outbound_payments.send_spontaneous_payment_with_route(
route, payment_preimage, recipient_onion, payment_id, &self.entropy_source,
&self.node_signer, best_block_height,
|path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv))
&self.node_signer, best_block_height, |args| self.send_payment_along_path(args))
}

/// Similar to [`ChannelManager::send_spontaneous_payment`], but will automatically find a route
Expand All @@ -3222,9 +3225,7 @@ where
self.pending_outbound_payments.send_spontaneous_payment(payment_preimage, recipient_onion,
payment_id, retry_strategy, route_params, &self.router, self.list_usable_channels(),
|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height,
&self.logger, &self.pending_events,
|path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv))
&self.logger, &self.pending_events, |args| self.send_payment_along_path(args))
}

/// Send a payment that is probing the given route for liquidity. We calculate the
Expand All @@ -3233,9 +3234,9 @@ where
pub fn send_probe(&self, path: Path) -> Result<(PaymentHash, PaymentId), PaymentSendFailure> {
let best_block_height = self.best_block.read().unwrap().height();
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
self.pending_outbound_payments.send_probe(path, self.probing_cookie_secret, &self.entropy_source, &self.node_signer, best_block_height,
|path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv))
self.pending_outbound_payments.send_probe(path, self.probing_cookie_secret,
&self.entropy_source, &self.node_signer, best_block_height,
|args| self.send_payment_along_path(args))
}

/// Returns whether a payment with the given [`PaymentHash`] and [`PaymentId`] is, in fact, a
Expand Down Expand Up @@ -4039,9 +4040,7 @@ where
let best_block_height = self.best_block.read().unwrap().height();
self.pending_outbound_payments.check_retry_payments(&self.router, || self.list_usable_channels(),
|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height,
&self.pending_events, &self.logger,
|path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_hash, recipient_onion, total_value, cur_height, payment_id, keysend_preimage, session_priv));
&self.pending_events, &self.logger, |args| self.send_payment_along_path(args));

for (htlc_source, payment_hash, failure_reason, destination) in failed_forwards.drain(..) {
self.fail_htlc_backwards_internal(&htlc_source, &payment_hash, &failure_reason, destination);
Expand Down
77 changes: 38 additions & 39 deletions lightning/src/ln/outbound_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,18 @@ impl RecipientOnionFields {
}
}

/// Arguments for [`super::channelmanager::ChannelManager::send_payment_along_path`].
pub(super) struct SendAlongPathArgs<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Not so sure about the location of this struct. i'd think its supposed to be more co-located with where its used but i also think channelmanager.rs is also too large.

what is the current understanding around outbound_payment.rs? are we intending to move most/all outbound payment / routing / orchestration logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The outbound_payment module came about so we could remove code from channelmanager. So, anything exclusive to outbound payments (such as this struct) could reasonably live there IMO. I believe we've moved most if not all of it already.

pub path: &'a Path,
pub payment_hash: &'a PaymentHash,
pub recipient_onion: RecipientOnionFields,
pub total_value: u64,
pub cur_height: u32,
pub payment_id: PaymentId,
pub keysend_preimage: &'a Option<PaymentPreimage>,
pub session_priv_bytes: [u8; 32],
}

pub(super) struct OutboundPayments {
pub(super) pending_outbound_payments: Mutex<HashMap<PaymentId, PendingOutboundPayment>>,
pub(super) retry_lock: Mutex<()>,
Expand All @@ -499,8 +511,7 @@ impl OutboundPayments {
NS::Target: NodeSigner,
L::Target: Logger,
IH: Fn() -> InFlightHtlcs,
SP: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>,
SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
self.send_payment_internal(payment_id, payment_hash, recipient_onion, None, retry_strategy,
route_params, router, first_hops, &compute_inflight_htlcs, entropy_source, node_signer,
Expand All @@ -515,8 +526,7 @@ impl OutboundPayments {
where
ES::Target: EntropySource,
NS::Target: NodeSigner,
F: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
F: Fn(SendAlongPathArgs) -> Result<(), APIError>
{
let onion_session_privs = self.add_new_pending_payment(payment_hash, recipient_onion.clone(), payment_id, None, route, None, None, entropy_source, best_block_height)?;
self.pay_route_internal(route, payment_hash, recipient_onion, None, payment_id, None,
Expand All @@ -537,8 +547,7 @@ impl OutboundPayments {
NS::Target: NodeSigner,
L::Target: Logger,
IH: Fn() -> InFlightHtlcs,
SP: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>,
SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
let preimage = payment_preimage
.unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes()));
Expand All @@ -557,8 +566,7 @@ impl OutboundPayments {
where
ES::Target: EntropySource,
NS::Target: NodeSigner,
F: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
F: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
let preimage = payment_preimage
.unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes()));
Expand Down Expand Up @@ -587,8 +595,7 @@ impl OutboundPayments {
R::Target: Router,
ES::Target: EntropySource,
NS::Target: NodeSigner,
SP: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>,
SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
IH: Fn() -> InFlightHtlcs,
FH: Fn() -> Vec<ChannelDetails>,
L::Target: Logger,
Expand Down Expand Up @@ -658,8 +665,7 @@ impl OutboundPayments {
NS::Target: NodeSigner,
L::Target: Logger,
IH: Fn() -> InFlightHtlcs,
SP: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
#[cfg(feature = "std")] {
if has_expired(&route_params) {
Expand Down Expand Up @@ -699,8 +705,7 @@ impl OutboundPayments {
NS::Target: NodeSigner,
L::Target: Logger,
IH: Fn() -> InFlightHtlcs,
SP: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
#[cfg(feature = "std")] {
if has_expired(&route_params) {
Expand Down Expand Up @@ -821,8 +826,7 @@ impl OutboundPayments {
NS::Target: NodeSigner,
L::Target: Logger,
IH: Fn() -> InFlightHtlcs,
SP: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
match err {
PaymentSendFailure::AllFailedResendSafe(errs) => {
Expand Down Expand Up @@ -894,8 +898,7 @@ impl OutboundPayments {
where
ES::Target: EntropySource,
NS::Target: NodeSigner,
F: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
F: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
let payment_id = PaymentId(entropy_source.get_secure_random_bytes());

Expand Down Expand Up @@ -989,8 +992,7 @@ impl OutboundPayments {
) -> Result<(), PaymentSendFailure>
where
NS::Target: NodeSigner,
F: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
F: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
if route.paths.len() < 1 {
return Err(PaymentSendFailure::ParameterError(APIError::InvalidRoute{err: "There must be at least one path to send over".to_owned()}));
Expand Down Expand Up @@ -1031,9 +1033,11 @@ impl OutboundPayments {
let cur_height = best_block_height + 1;
let mut results = Vec::new();
debug_assert_eq!(route.paths.len(), onion_session_privs.len());
for (path, session_priv) in route.paths.iter().zip(onion_session_privs.into_iter()) {
let mut path_res = send_payment_along_path(&path, &payment_hash, recipient_onion.clone(),
total_value, cur_height, payment_id, &keysend_preimage, session_priv);
for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.into_iter()) {
let mut path_res = send_payment_along_path(SendAlongPathArgs {
path: &path, payment_hash: &payment_hash, recipient_onion: recipient_onion.clone(),
total_value, cur_height, payment_id, keysend_preimage: &keysend_preimage, session_priv_bytes
});
match path_res {
Ok(_) => {},
Err(APIError::MonitorUpdateInProgress) => {
Expand All @@ -1044,7 +1048,7 @@ impl OutboundPayments {
Err(_) => {
let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
if let Some(payment) = pending_outbounds.get_mut(&payment_id) {
let removed = payment.remove(&session_priv, Some(path));
let removed = payment.remove(&session_priv_bytes, Some(path));
debug_assert!(removed, "This can't happen as the payment has an entry for this path added by callers");
} else {
debug_assert!(false, "This can't happen as the payment was added by callers");
Expand Down Expand Up @@ -1098,8 +1102,7 @@ impl OutboundPayments {
) -> Result<(), PaymentSendFailure>
where
NS::Target: NodeSigner,
F: Fn(&Path, &PaymentHash, RecipientOnionFields, u64, u32, PaymentId,
&Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
F: Fn(SendAlongPathArgs) -> Result<(), APIError>,
{
self.pay_route_internal(route, payment_hash, recipient_onion, keysend_preimage, payment_id,
recv_value_msat, onion_session_privs, node_signer, best_block_height,
Expand Down Expand Up @@ -1480,8 +1483,8 @@ mod tests {
&&keys_manager, 0).unwrap();
outbound_payments.retry_payment_internal(
PaymentHash([0; 32]), PaymentId([0; 32]), expired_route_params, &&router, vec![],
&|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
&pending_events, &|_, _, _, _, _, _, _, _| Ok(()));
&|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, &pending_events,
&|_| Ok(()));
let events = pending_events.lock().unwrap();
assert_eq!(events.len(), 1);
if let Event::PaymentFailed { ref reason, .. } = events[0].0 {
Expand All @@ -1491,8 +1494,7 @@ mod tests {
let err = outbound_payments.send_payment(
PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(), PaymentId([0; 32]),
Retry::Attempts(0), expired_route_params, &&router, vec![], || InFlightHtlcs::new(),
&&keys_manager, &&keys_manager, 0, &&logger,
&pending_events, |_, _, _, _, _, _, _, _| Ok(())).unwrap_err();
&&keys_manager, &&keys_manager, 0, &&logger, &pending_events, |_| Ok(())).unwrap_err();
if let RetryableSendFailure::PaymentExpired = err { } else { panic!("Unexpected error"); }
}
}
Expand Down Expand Up @@ -1528,17 +1530,16 @@ mod tests {
&&keys_manager, 0).unwrap();
outbound_payments.retry_payment_internal(
PaymentHash([0; 32]), PaymentId([0; 32]), route_params, &&router, vec![],
&|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
&pending_events, &|_, _, _, _, _, _, _, _| Ok(()));
&|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, &pending_events,
&|_| Ok(()));
let events = pending_events.lock().unwrap();
assert_eq!(events.len(), 1);
if let Event::PaymentFailed { .. } = events[0].0 { } else { panic!("Unexpected event"); }
} else {
let err = outbound_payments.send_payment(
PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(), PaymentId([0; 32]),
Retry::Attempts(0), route_params, &&router, vec![], || InFlightHtlcs::new(),
&&keys_manager, &&keys_manager, 0, &&logger,
&pending_events, |_, _, _, _, _, _, _, _| Ok(())).unwrap_err();
&&keys_manager, &&keys_manager, 0, &&logger, &pending_events, |_| Ok(())).unwrap_err();
if let RetryableSendFailure::RouteNotFound = err {
} else { panic!("Unexpected error"); }
}
Expand Down Expand Up @@ -1587,8 +1588,7 @@ mod tests {
PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(), PaymentId([0; 32]),
Retry::Attempts(0), route_params.clone(), &&router, vec![], || InFlightHtlcs::new(),
&&keys_manager, &&keys_manager, 0, &&logger, &pending_events,
|_, _, _, _, _, _, _, _| Err(APIError::ChannelUnavailable { err: "test".to_owned() }))
.unwrap();
|_| Err(APIError::ChannelUnavailable { err: "test".to_owned() })).unwrap();
let mut events = pending_events.lock().unwrap();
assert_eq!(events.len(), 2);
if let Event::PaymentPathFailed {
Expand All @@ -1606,16 +1606,15 @@ mod tests {
PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(), PaymentId([0; 32]),
Retry::Attempts(0), route_params.clone(), &&router, vec![], || InFlightHtlcs::new(),
&&keys_manager, &&keys_manager, 0, &&logger, &pending_events,
|_, _, _, _, _, _, _, _| Err(APIError::MonitorUpdateInProgress)).unwrap();
|_| Err(APIError::MonitorUpdateInProgress)).unwrap();
assert_eq!(pending_events.lock().unwrap().len(), 0);

// Ensure that any other error will result in a PaymentPathFailed event but no blamed scid.
outbound_payments.send_payment(
PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(), PaymentId([1; 32]),
Retry::Attempts(0), route_params.clone(), &&router, vec![], || InFlightHtlcs::new(),
&&keys_manager, &&keys_manager, 0, &&logger, &pending_events,
|_, _, _, _, _, _, _, _| Err(APIError::APIMisuseError { err: "test".to_owned() }))
.unwrap();
|_| Err(APIError::APIMisuseError { err: "test".to_owned() })).unwrap();
let events = pending_events.lock().unwrap();
assert_eq!(events.len(), 2);
if let Event::PaymentPathFailed {
Expand Down