Remove AvailableBalances::balance_msat #2476
Conversation
wvanlint
force-pushed
the
remove_balance_msat
branch
from
31b7419
to
124d2a9
Compare
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2476 +/- ##
==========================================
- Coverage 90.40% 90.40% -0.01%
==========================================
Files 106 106
Lines 56328 56321 -7
Branches 56328 56321 -7
==========================================
- Hits 50926 50916 -10
- Misses 5402 5405 +3
☔ View full report in Codecov by Sentry. |
|
Could you add a release note regarding serialization compatibility to the |
wvanlint
force-pushed
the
remove_balance_msat
branch
3 times, most recently
from
817548d
to
c1bb5d3
Compare
| /// The amount that would go to us if we close the channel, ignoring any on-chain fees. | ||
| pub balance_msat: u64, |
There was a problem hiding this comment.
If we remove this, is there still a way to calculate our balance with msat precision since ChannelMonitor::get_claimable_balances will be rounded to sats?
There was a problem hiding this comment.
Not currently. With regard to AvailableBalances::balance_msat, its computation was very specific to a use case that is not applicable anymore and its value is likely not what is expected. For introspection at msat precision, perhaps Channel::value_to_self_msat and the pending HTLCs can be exposed? For the latter, see #2442.
There was a problem hiding this comment.
perhaps Channel::value_to_self_msat
Hm, perhaps. Will take a look at #2442, thanks!
There was a problem hiding this comment.
Hmm, what would the use case be for getting a claimable balance in msat? on-chain doesn't have msats
There was a problem hiding this comment.
I was thinking just to get our channel balance in general, not necessarily on-chain
There was a problem hiding this comment.
Ah okay. It's nice to have a specific use case for a balance IMO, pretty vague term in lightning. Maybe something like that would be useful for accounting reasons, though 🤷♀️
There was a problem hiding this comment.
Yea it's probably not a big deal to not have it for now
|
Adjusted documentation and serialization compatibility. |
| /// The amount that would go to us if we close the channel, ignoring any on-chain fees. | ||
| pub balance_msat: u64, |
There was a problem hiding this comment.
Hmm, what would the use case be for getting a claimable balance in msat? on-chain doesn't have msats
wvanlint
force-pushed
the
remove_balance_msat
branch
2 times, most recently
from
fb4b56f
to
67b47bf
Compare
| @@ -7550,7 +7542,7 @@ impl Readable for ChannelDetails { | |||
| (10, channel_value_satoshis, required), | |||
| (12, unspendable_punishment_reserve, option), | |||
| (14, user_channel_id_low, required), | |||
| (16, balance_msat, required), | |||
| (16, _balance_msat, option), // Backwards compatibility for removed balance_msat field. | |||
There was a problem hiding this comment.
God rustc pisses me off for doing this - it just blindly picks a type (IIRC i64, so we're okay) and assumes that that's the type we want here. Its completely nonsense and results in code that does surprising things. Can you re-add the let _balance_msat: Option<u64> = _balance_msat below?
There was a problem hiding this comment.
Ah thanks for catching this! Re-added the type hint.
wvanlint
force-pushed
the
remove_balance_msat
branch
from
67b47bf
to
5184606
Compare
The ChannelMonitor::get_claimable_balances method 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.
wvanlint
force-pushed
the
remove_balance_msat
branch
from
5184606
to
ef5be58
Compare
This PR removes the
AvailableBalances::balance_msatfield.After a discussion on Discord, it was mentioned
ChannelMonitor::get_claimable_balancesprovides a more straightforward approach to the balance of a channel, which satisfies most use cases. The computation ofAvailableBalances::balance_msatis 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_htlcwas verified against the CommitmentStats (and this commitment transaction resolves the pending HTLC in theInboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_))state). Currently, we're verifying againstAvailableBalances::next_outbound_htlc_limit_msatwhich 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 withCannot send more than our next-HTLC maximum). This is likely why the test has been removed.