Support auto-retrying keysend payments in ChannelManager
#2002
Conversation
Codecov ReportBase: 90.94% // Head: 90.93% // Decreases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2002 +/- ##
==========================================
- Coverage 90.94% 90.93% -0.01%
==========================================
Files 99 99
Lines 52775 52806 +31
Branches 52775 52806 +31
==========================================
+ Hits 47994 48021 +27
- Misses 4781 4785 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Can be rebased, now. |
This sets us up for spontaneous payment retries in ChannelManager. Currently, retrying spontaneous payments is broken in InvoicePayer because it does not include the keysend preimage on retry.
valentinewallace
force-pushed
the
2023-01-keysend-retries
branch
from
40fdb4b
to
6b49af1
Compare
| first_hops: Vec<ChannelDetails>, inflight_htlcs: InFlightHtlcs, entropy_source: &ES, | ||
| node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F |
There was a problem hiding this comment.
Unrelated this PR, but may want to consider using the Payer abstraction here and throughout in the future to reduce the number of parameters. Could possibly simplify the tests by using a custom function for send_payment_along_path instead of needing to setup ChannelManager... or maybe not yet if retries are still triggered in ChannelManager?
There was a problem hiding this comment.
Retries upon payment failure are triggered in ChannelManager but they could always be triggered manually for unit testing. Using Payer to reduce the number of params sgtm 👍
| let (total_msat, payment_hash, payment_secret, keysend_preimage) = { | ||
| let mut outbounds = self.pending_outbound_payments.lock().unwrap(); | ||
| match outbounds.get_mut(&payment_id) { | ||
| Some(payment) => { | ||
| let res = match payment { | ||
| PendingOutboundPayment::Retryable { | ||
| total_msat, payment_hash, payment_secret, pending_amt_msat, .. | ||
| total_msat, payment_hash, keysend_preimage, payment_secret, pending_amt_msat, .. | ||
| } => { | ||
| let retry_amt_msat: u64 = route.paths.iter().map(|path| path.last().unwrap().fee_msat).sum(); | ||
| if retry_amt_msat + *pending_amt_msat > *total_msat * (100 + RETRY_OVERFLOW_PERCENTAGE) / 100 { | ||
| return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { | ||
| err: format!("retry_amt_msat of {} will put pending_amt_msat (currently: {}) more than 10% over total_payment_amt_msat of {}", retry_amt_msat, pending_amt_msat, total_msat).to_string() | ||
| })) | ||
| } | ||
| (*total_msat, *payment_hash, *payment_secret) | ||
| (*total_msat, *payment_hash, *payment_secret, *keysend_preimage) |
There was a problem hiding this comment.
Possible follow-up opportunity to improve code cleanliness here. The first pattern is the only one that doesn't return early. So some of the indentations can be reduced by getting rid of the res local variable (and thus moving up the match) and also the outer (total_msat, payment_hash, payment_secret, keysend_preimage) variables, though you'd have to drop the lock before calling pay_route_internal.
Supports sending spontaneous payments with a retry strategy as part of moving all payment retries into
ChannelManagerPartially addresses #1932.
Based on #1994