feat: track in-flight amount for pending payments#4366
feat: track in-flight amount for pending payments#4366shruti2522 wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 I see @joostjager was un-assigned. |
shruti2522
force-pushed
the
feat/pending-balance
branch
from
4c7a57d to
0c440b6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4366 +/- ##
==========================================
- Coverage 86.54% 86.00% -0.55%
==========================================
Files 158 156 -2
Lines 103166 102606 -560
Branches 103166 102606 -560
==========================================
- Hits 89287 88243 -1044
- Misses 11456 11853 +397
- Partials 2423 2510 +87
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
wpaulino
left a comment
wpaulino
left a comment
There was a problem hiding this comment.
Please include the description of the changes in the commit message as well
| /// Amount (in msat) currently locked in HTLCs. | ||
| /// | ||
| /// `total_msat - inflight_msat` gives the amount waiting to be retried | ||
| /// Reserve both from spendable balance. |
There was a problem hiding this comment.
This line is not very clear to me, can you rephrase it?
|
|
||
| let payment_amt = 100_000; | ||
| let (payment_preimage, _payment_hash, _, payment_id) = | ||
| route_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_amt); |
There was a problem hiding this comment.
Let's route another payment but still only claim one so we can assert the in-flight amount of the remaining payment
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
✅ Added second reviewer: @joostjager |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I'm not convinced this API is quite sufficient for what we need. We really want a single, consistent view of users' current balance. The problem with fetching information from two separate sources (here ChannelMonitor(s) + ChannelManager) is that we end up racing retries across them - calling them both in succession may double- or under-count pending payments. I'm not quite sure with the right answer is, maybe we need to list HTLCs in pending payments and have some utility that removes duplicated entries, or maybe we need to include total payment amounts in the ChannelMonitor balance list and have some logic to compare the pending payments list to remove payments that have partially MPP-timed-out.
|
We'll have to explore it a bit, but I think the second option is compelling - we already have payment info in the monitors (I hope enough) so communicating that back may suffice. The only case we'd maybe want to worry about is whether we over-state the in-flight amount for payments where we've given up retrying but there are stuck MPP parts. In that case we'll need the pending payment info like you exposed here, but may need more information about the retry state. |
4 participants
fix #3374
adds
inflight_msattoRecentPaymentDetails::Pendingto expose the amount currently locked in HTLCs. this allows wallets to avoid balance flicker during payment retries by distinguishing between inflight funds and funds waiting to retry