Accuracy fixes and differentiating sources for Balance
#3212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3212 +/- ##
==========================================
- Coverage 89.75% 89.75% -0.01%
==========================================
Files 122 122
Lines 101921 102174 +253
Branches 101921 102174 +253
==========================================
+ Hits 91484 91702 +218
- Misses 7755 7784 +29
- Partials 2682 2688 +6 ☔ View full report in Codecov by Sentry. |
dunxen
force-pushed
the
2024-07-sensible-balances
branch
from
deafbd4
to
551f487
Compare
TheBlueMatt
reviewed
Aug 5, 2024
| @@ -2365,6 +2380,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> { | |||
| } else { | |||
| outbound_forwarded_htlc_rounded_msat += rounded_value_msat; | |||
| } | |||
| expected_tx_value_sats = expected_tx_value_sats.saturating_sub((htlc.amount_msat + 999) / 1000); | |||
There was a problem hiding this comment.
Looks like this is unused? IIRC I had some grand ambition to be able to sum up all the values in the Balance and get out the channel's total value, except we don't support tracking the counterparty's balance so there is no way to do that.
Comment on lines
639
to
640
| /// This value will not be included in `amount_satoshis` and any representable (quotient | ||
| /// when divided by 1000 without the remainder) value will go toward fees |
There was a problem hiding this comment.
This look like words that are missing a conclusion?
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| #[cfg_attr(test, derive(PartialOrd, Ord))] | ||
| pub enum BalanceSource { | ||
| /// The channel was force closed. |
There was a problem hiding this comment.
It looks like we have enough info to differentiate between counterparty and holder force-closures, so we probably should, no?
There was a problem hiding this comment.
I did originally have that distinction and had a couple issues with the tests for some reason so reverted to this, but I'm sure it's a simple fix and nice to have, so I'll include it again.
Comment on lines
638
to
682
| outbound_payment_htlc_rounded_msat: u64, | ||
| /// The amount of millisatoshis which has been burned to fees from HTLCs which are outbound | ||
| /// from us and are related to a forwarded HTLC. This is the sum of the millisatoshis part | ||
| /// of all HTLCs which are otherwise represented by [`Balance::MaybeTimeoutClaimableHTLC`] | ||
| /// with their [`Balance::MaybeTimeoutClaimableHTLC::outbound_payment`] flag *not* set, as | ||
| /// well as any dust HTLCs which would otherwise be represented the same. | ||
| outbound_forwarded_htlc_rounded_msat: u64, | ||
| /// The amount of millisatoshis which has been burned to fees from HTLCs which are inbound | ||
| /// to us and for which we know the preimage. This is the sum of the millisatoshis part of | ||
| /// all HTLCs which would be represented by [`Balance::ContentiousClaimable`] on channel | ||
| /// close, but who's current value is included in | ||
| /// [`Balance::ClaimableOnChannelClose::amount_satoshis`], as well as any dust HTLCs which | ||
| /// would otherwise be represented the same. | ||
| inbound_claiming_htlc_rounded_msat: u64, | ||
| /// The amount of millisatoshis which has been burned to fees from HTLCs which are inbound | ||
| /// to us and for which we do not know the preimage. This is the sum of the millisatoshis | ||
| /// part of all HTLCs which would be represented by [`Balance::MaybePreimageClaimableHTLC`] | ||
| /// on channel close, as well as any dust HTLCs which would otherwise be represented the | ||
| /// same. | ||
| inbound_htlc_rounded_msat: u64, |
There was a problem hiding this comment.
Looks like these aren't really covered by our tests. Should we update the existing monitor tests to use non-round HTLC amounts so that we can calculate this?
dunxen
force-pushed
the
2024-07-sensible-balances
branch
from
551f487
to
0d83579
Compare
When the user is fetching their current balances after forwarding a payment (before it clears), they'll see a `MaybePreimageClaimableHTLC` and a `MaybeTimeoutClaimableHTLC` but if they sum up their balance using `Balance::claimable_amount_satoshis` neither will be included. Obviously, exactly one of the two balances should be included - one of the two resolutions should happen in our favor. This causes our visible balance to fluctuate up and down by the full value of any HTLCs we're in the middle of forwarding, which is incredibly confusing to see. If we want to stop the fluctuations, we need to pick one of the two balances to include. The obvious candidate is `MaybeTimeoutClaimableHTLC` as it is the lower of the two, and represents our balance without the fee we'd receive from the forward. Sadly, if we always include it, we'll end up also including any HTLCs which we've sent but which haven't yet been claimed by their recipient, which is the wrong behavior. Luckily, we have access to the `Option<HTLCSource>` while walking HTLCs, which allows us to add an `outbound_payment` flag to `MaybeTimeoutClaimableHTLC`. This allows us to only include forwarded payments in `claimable_amount_satoshis`. Sadly, even with this in place our balance still fluctuates by the changes in the commitment transaction fees we have to pay during forwarding, but addressing that is left for later.
There's no reason to `use` a module within that module to refer to that module...
These don't really belong in `channel` as they're now used in other parts of the codebase.
`Balance::ClaimableOnChannelClose` excludes the commitment transaction fee, which makes it hard to use for current balance calculation. Here we add it, setting the value to zero for inbound channels (i.e. ones for which we don't pay the fee).
If we're gonna push users towards using `Balance` to determine their current balances, we really need to provide more information, including msat balances. Here we add rounded-out msat balances to the pre-close balance information
Introduce the `BalanceSource` enum to differentiate between force-close, coop-close, and HTLCs in `Balance::ClaimableAwaitingConfirmations`.
dunxen
force-pushed
the
2024-07-sensible-balances
branch
from
0d83579
to
d6c540d
Compare
TheBlueMatt
approved these changes
Aug 13, 2024
valentinewallace
approved these changes
Aug 14, 2024
valentinewallace
merged commit 5ab40b2
into
lightningdevkit:main
15 of 19 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR supersedes #2618 and includes a fix for #2738.
After this PR is merged, #2476 should be included again (it had been reverted prior to a previous release).
From 2618:
Fixes #2738.