Skip to content
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

Remove AvailableBalances::balance_msat #3243

Merged

Conversation

dunxen
Copy link
Contributor

@dunxen dunxen commented Aug 15, 2024

We can now (re)remove this field after the merging of #3212.

Context from original (reverted) PR authored by @wvanlint (#2476):

This PR removes the AvailableBalances::balance_msat field.

After a discussion on Discord, it was mentioned ChannelMonitor::get_claimable_balances provides a more straightforward approach to the balance of a channel, which satisfies most use cases. The computation of AvailableBalances::balance_msat is complex and originally had a different purpose that is not applicable anymore.

Its computation takes into account holding cell HTLCs and fulfills inbound-claimed HTLCs since https://github.com/lightningdevkit/rust-lightning/pull/1268/files. However, the root cause of the underflow that was the motivation of that PR does not exist anymore. At that time, the amount in send_htlc was verified against the CommitmentStats (and this commitment transaction resolves the pending HTLC in the InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_)) state). Currently, we're verifying against AvailableBalances::next_outbound_htlc_limit_msat which does not resolve such pending HTLCs. I copied and reran the test in #1268 from the main branch and it is not possible to send an HTLC back anymore and cause an underflow (sending the return payment fails with Cannot send more than our next-HTLC maximum). This is likely why the test has been removed.

@dunxen dunxen force-pushed the 2024-08-reremove-balancemsat branch from 31a40a2 to 64dd5d1 Compare August 15, 2024 08:26
@dunxen dunxen marked this pull request as ready for review August 15, 2024 08:26
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.62%. Comparing base (eba329c) to head (59f1689).
Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3243      +/-   ##
==========================================
- Coverage   89.63%   89.62%   -0.01%     
==========================================
  Files         126      126              
  Lines      102166   102154      -12     
  Branches   102166   102154      -12     
==========================================
- Hits        91572    91555      -17     
- Misses       7869     7874       +5     
  Partials     2725     2725              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
Copy link
Collaborator

Can we mark these as #[deprecated] and then remove them in the next release?

@dunxen
Copy link
Contributor Author

dunxen commented Aug 15, 2024

Can we mark these as #[deprecated] and then remove them in the next release?

Yeah, that'll be fair.

@dunxen
Copy link
Contributor Author

dunxen commented Aug 16, 2024

I'll leave this one open until 125 and then open a separate deprecation PR.

@dunxen dunxen marked this pull request as draft August 16, 2024 06:10
@dunxen dunxen force-pushed the 2024-08-reremove-balancemsat branch from 64dd5d1 to 890caa2 Compare August 21, 2024 13:30
@TheBlueMatt
Copy link
Collaborator

Can probably un-draft + rebase.

The ChannelMonitor::get_claimable_balances and ChainMonitor::get_claimable_balances
methods provide a more straightforward approach to the balance of a channel, which
satisfies most use cases. The computation of AvailableBalances::balance_msat is
complex and originally had a different purpose that is not applicable
anymore.

Co-authored-by: Willem Van Lint <[email protected]>
@dunxen dunxen force-pushed the 2024-08-reremove-balancemsat branch from 890caa2 to 59f1689 Compare September 9, 2024 17:37
@dunxen dunxen marked this pull request as ready for review September 9, 2024 17:37
@TheBlueMatt TheBlueMatt merged commit f7cc40e into lightningdevkit:main Sep 13, 2024
17 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants