Include an outbound_payment flag in MaybeTimeoutClaimableHTLC
#2618
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
TheBlueMatt
force-pushed
the
2023-09-sensible-balances
branch
from
6921d46
to
c78aa46
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2618 +/- ##
==========================================
+ Coverage 88.69% 89.01% +0.31%
==========================================
Files 113 112 -1
Lines 89475 86299 -3176
Branches 89475 86299 -3176
==========================================
- Hits 79359 76817 -2542
+ Misses 7867 7252 -615
+ Partials 2249 2230 -19 ☔ View full report in Codecov by Sentry. |
|
Tagging this as 0.0.117 as we need to do something here, though that could also be reverting #2476. |
|
Okay this really isnt sufficient, I worked on more details but its an increasingly huge patchset. Slipping to 118 and reverting 2476. |
|
Pushed some wip stuff that i think is needed to fully switch over here. |
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).
…ClaimableOnChannelClose` 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
TheBlueMatt
force-pushed
the
2023-09-sensible-balances
branch
from
eb18aac
to
3bc8048
Compare
|
Should also address #2738 here. |
|
Hey @TheBlueMatt, could you allow edits by maintainers for this PR so I can take this over? :) |
|
Superseded by #3212. |
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.
When the user is fetching their current balances after forwarding a payment (before it clears), they'll see a
MaybePreimageClaimableHTLCand aMaybeTimeoutClaimableHTLCbut if they sum up their balance usingBalance::claimable_amount_satoshisneither 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
MaybeTimeoutClaimableHTLCas 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 anoutbound_paymentflag toMaybeTimeoutClaimableHTLC. This allows us to only include forwarded payments inclaimable_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.