Skip to content

ChannelManager Payment Retries #1916

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

Merged
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 6 additions & 0 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3250,6 +3250,12 @@ 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.logger,
|path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv));

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
117 changes: 115 additions & 2 deletions lightning/src/ln/outbound_payment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};

use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use crate::ln::channelmanager::{HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, MIN_HTLC_RELAY_HOLDING_CELL_MILLIS, PaymentId};
use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, MIN_HTLC_RELAY_HOLDING_CELL_MILLIS, PaymentId};
use crate::ln::msgs::DecodeError;
use crate::ln::onion_utils::HTLCFailReason;
use crate::routing::router::{PaymentParameters, Route, RouteHop, RouteParameters, RoutePath};
use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, RoutePath, Router};
use crate::util::errors::APIError;
use crate::util::events;
use crate::util::logger::Logger;
Expand Down Expand Up @@ -237,6 +237,16 @@ impl Retry {
}
}

#[cfg(feature = "std")]
pub(super) fn has_expired(route_params: &RouteParameters) -> bool {
if let Some(expiry_time) = route_params.payment_params.expiry_time {
if let Ok(elapsed) = std::time::SystemTime::UNIX_EPOCH.elapsed() {
return elapsed > core::time::Duration::from_secs(expiry_time)
}
}
false
}

pub(crate) type PaymentAttempts = PaymentAttemptsUsingTime<ConfiguredTime>;

/// Storing minimal payment attempts information required for determining if a outbound payment can
Expand Down Expand Up @@ -411,6 +421,109 @@ impl OutboundPayments {
}
}

pub(super) fn check_retry_payments<R: Deref, ES: Deref, NS: Deref, SP, IH, FH, L: Deref>(
&self, router: &R, first_hops: FH, inflight_htlcs: IH, entropy_source: &ES, node_signer: &NS,
best_block_height: u32, logger: &L, send_payment_along_path: SP,
)
where
R::Target: Router,
ES::Target: EntropySource,
NS::Target: NodeSigner,
SP: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
u32, PaymentId, &Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>,
IH: Fn() -> InFlightHtlcs,
FH: Fn() -> Vec<ChannelDetails>,
L::Target: Logger,
{
loop {
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
let mut retry_id_route_params = None;
for (pmt_id, pmt) in outbounds.iter_mut() {
if pmt.is_retryable_now() {
if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, route_params: Some(params), .. } = pmt {
if pending_amt_msat < total_msat {
retry_id_route_params = Some((*pmt_id, params.clone()));
pmt.increment_attempts();
break
}
}
}
}
if let Some((payment_id, route_params)) = retry_id_route_params {
core::mem::drop(outbounds);
if let Err(e) = self.pay_internal(payment_id, route_params, router, first_hops(), inflight_htlcs(), entropy_source, node_signer, best_block_height, &send_payment_along_path) {
log_trace!(logger, "Errored retrying payment: {:?}", e);
}
} else { break }
}
}

fn pay_internal<R: Deref, NS: Deref, ES: Deref, F>(
&self, payment_id: PaymentId, route_params: RouteParameters, router: &R,
first_hops: Vec<ChannelDetails>, inflight_htlcs: InFlightHtlcs, entropy_source: &ES,
node_signer: &NS, best_block_height: u32, send_payment_along_path: &F
) -> Result<(), PaymentSendFailure>
where
R::Target: Router,
ES::Target: EntropySource,
NS::Target: NodeSigner,
F: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
u32, PaymentId, &Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
{
#[cfg(feature = "std")] {
if has_expired(&route_params) {
return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
err: format!("Invoice expired for payment id {}", log_bytes!(payment_id.0)),
}))
}
}

let route = router.find_route(
&node_signer.get_node_id(Recipient::Node).unwrap(), &route_params,
Some(&first_hops.iter().collect::<Vec<_>>()), &inflight_htlcs
).map_err(|e| PaymentSendFailure::ParameterError(APIError::APIMisuseError {
err: format!("Failed to find a route for payment {}: {:?}", log_bytes!(payment_id.0), e), // TODO: add APIError::RouteNotFound
}))?;

let res = self.retry_payment_with_route(&route, payment_id, entropy_source, node_signer, best_block_height, send_payment_along_path);
match res {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its kinda confusing to match res twice, can we not do it in one match and mem::drop(outbounds) before calling pay? eg in the current code it looks like we spuriously increment_attempts in the failed_paths_retry.is_none() case, which the comment talks about being due to monitor updates being done async.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch, fixed this

Err(PaymentSendFailure::AllFailedResendSafe(_)) => {
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
if let Some(payment) = outbounds.get_mut(&payment_id) {
let retryable = payment.is_retryable_now();
if retryable {
payment.increment_attempts();
} else { return res }
} else { return res }
core::mem::drop(outbounds);
self.pay_internal(payment_id, route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, send_payment_along_path)
},
Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, .. }) => {
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
if let Some(payment) = outbounds.get_mut(&payment_id) {
let retryable = payment.is_retryable_now();
if retryable {
payment.increment_attempts();
} else { return Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, payment_id }) }
} else { return Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, payment_id }) }
core::mem::drop(outbounds);

// Some paths were sent, even if we failed to send the full MPP value our recipient may
// misbehave and claim the funds, at which point we have to consider the payment sent, so
// return `Ok()` here, ignoring any retry errors.
let _ = self.pay_internal(payment_id, retry, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, send_payment_along_path);
Ok(())
},
Err(PaymentSendFailure::PartialFailure { failed_paths_retry: None, .. }) => {
// This may happen if we send a payment and some paths fail, but only due to a temporary
// monitor failure or the like, implying they're really in-flight, but we haven't sent the
// initial HTLC-Add messages yet.
Ok(())
},
res => res,
}
}

pub(super) fn retry_payment_with_route<ES: Deref, NS: Deref, F>(
&self, route: &Route, payment_id: PaymentId, entropy_source: &ES, node_signer: &NS, best_block_height: u32,
send_payment_along_path: F
Expand Down