Payment Retries in ChannelManager #1096
Conversation
valentinewallace
force-pushed
the
2021-09-mpp-retries
branch
from
6ba5c5e to
c9df739
Compare
Codecov Report
@@ Coverage Diff @@
## main #1096 +/- ##
==========================================
+ Coverage 90.73% 91.14% +0.41%
==========================================
Files 65 65
Lines 34318 37177 +2859
==========================================
+ Hits 31137 33884 +2747
- Misses 3181 3293 +112
Continue to review full report at Codecov.
|
lightning/src/ln/channelmanager.rs
Outdated
| } | ||
|
|
||
| impl PendingOutboundPayment { | ||
| fn remove(&mut self, session_priv: &[u8; 32]) -> bool { |
There was a problem hiding this comment.
Heh, match the hashset api, slick, i like it.
lightning/src/ln/channelmanager.rs
Outdated
| } | ||
| } else { | ||
| // XXX they could retry with a bogus amt/hash/secret/payment_id here and we wouldn't catch it atm | ||
| // should we keep payments in pending_outbound_payments until they succeed/expire/manually removed? |
There was a problem hiding this comment.
Hmm, I was kinda figuring we'd require users just call send_payment again, though I admit it would be kinda slick to let them just call the same method here. I guess we could do something like time out pending all-paths-failed payments after, eg, 3 blocks or so? Do you have an opinion @jkczyz?
There was a problem hiding this comment.
Yeah, I figured once a payment has failed (and thus ChannelManager has been cleaned up), retrying would just involve calling send_payment again as is currently done in #1059. So the retry interface would be specific to retrying a path failure for MPP.
Originally, I hoped it could be a single interface across any send/retry scenario. See conversation here: #1053 (comment). But that seemed to not be ideal.
If we could have a single call for any retry scenario, that would be nice. It mostly just prevents us from needing to switch on all_paths_failed.
There was a problem hiding this comment.
Yea, I was thinking we'd have separate interfaces, but maybe the memory usage is not so bad? Like, we'd have a bunch of memory used for failed payments, but we could time-box it. For high-volume scenarios it could be a real concern, but we'd clean up any memory (a) after the payment succeeds, even across a different path, or (b) after some time. Maybe we can figure out how to address the DoS later?
There was a problem hiding this comment.
We could parameterize send_payment further by the invoice expiry, and remove pending outbounds at min(invoice_expiry, 3 blocks) maybe?
In any case, given this week's a bit busy, I'm in favor of punting complete payment retries to follow-up
There was a problem hiding this comment.
In any case, given this week's a bit busy, I'm in favor of punting complete payment retries to follow-up
Well we either have to handle it here or in #1059, both for 0.0.102. I suppose we can punt "tightly limit expiry time" to a later release, even if we do expire here?
There was a problem hiding this comment.
Oh, I meant punt complete payment retries via the retry_payment API. I think #1059 already does complete payment retries, just via send_payment
There was a problem hiding this comment.
Oh, I meant punt complete payment retries via the retry_payment API. I think #1059 already does complete payment retries, just via send_payment
Right, my point was its probably ~the same amount of code to handle switching between retry_payment and send_payment in #1059 as it is to just call retry_payment instead of send_payment and not remove the payment entry until a payment succeeds in this PR :)
There was a problem hiding this comment.
> not remove the payment entry until a payment succeeds in this PR :)
Hmm, so when I keep failed payments, then retry a payment with all_paths_failed, it fails due to we didn't have a corresponding inbound payment (on the receiver end). Gotta make it symmetrical (i.e. keep failed payments on receiver end as well), I suppose?
Actually, this seems like it could be an issue for #1059 as well iiuc
EDIT: disregard, should never retry if rejected_by_dest anyway
valentinewallace
force-pushed
the
2021-09-mpp-retries
branch
from
c9df739 to
2723fe4
Compare
lightning/src/ln/channelmanager.rs
Outdated
| (1, pending_outbound_payments, required), | ||
| (1, pending_outbound_payments_compat_2, required), | ||
| (3, pending_outbound_payments, required), |
There was a problem hiding this comment.
Will changing the order break the format?
There was a problem hiding this comment.
IIUC, if e.g. in 0.0.102 we change the hashmap serialization format that corresponds to the 1 tlv type here, 0.0.101 clients will fail to deserialize it (hope that answers)
valentinewallace
force-pushed
the
2021-09-mpp-retries
branch
4 times, most recently
from
e5654b8 to
c4ac620
Compare
Leftover from previous PR Jeff feedback. Useful in upcoming commits as we'll expose this to users for payment retries
Used in upcoming commits for retries
We want to reuse send_payment internal functions for retries, so some need to now be parameterized by PaymentId to avoid generating a new PaymentId on retry
Retrying a partial payment means send_payment_internal needs to be parameterized by a total payment amount, else 'HTLC values do not match' errors
valentinewallace
force-pushed
the
2021-09-mpp-retries
branch
2 times, most recently
from
58afd58 to
977dbf8
Compare
valentinewallace
force-pushed
the
2021-09-mpp-retries
branch
from
977dbf8 to
24c2144
Compare
valentinewallace
force-pushed
the
2021-09-mpp-retries
branch
2 times, most recently
from
98a225e to
8460a97
Compare
This is because we want the ability to retry completely failed payments. Upcoming commits will remove these payments on timeout to prevent DoS issues Also test that this removal allows retrying single-path payments
valentinewallace
force-pushed
the
2021-09-mpp-retries
branch
from
8460a97 to
3e6297a
Compare
Seeking concept ACKs on the new
pending_outbound_paymentsformat/new enum @jkczyz @TheBlueMattAlso left a comment on an open question about retrying payments that have been completely removed.
TODOs