Avoid generating unpayable routes due to balance restrictions#2312
Merged
TheBlueMatt merged 10 commits intolightningdevkit:mainfrom Jun 7, 2023
Merged
Avoid generating unpayable routes due to balance restrictions#2312TheBlueMatt merged 10 commits intolightningdevkit:mainfrom
TheBlueMatt merged 10 commits intolightningdevkit:mainfrom
Conversation
While its nice to be able to push an HTLC which spends balance that is removed in our local commitment transaction but awaiting an RAA from our peer for final removal its by no means a critical feature. Because peers should really be sending RAAs quickly after we send a commitment, this should be an exceedingly rare case, and we already don't expose this as available balance when routing, so this isn't even made available when sending, only forwarding. Note that `test_pending_claimed_htlc_no_balance_underflow` is removed as it tested a case which was only possible because of this and now is no longer possible.
In the coming commits we redo our next-HTLC-available logic which requires some minor test changes for tests which relied on calculating routes which were not usable. Here we do a minor prefactor to simplify a test which now no longer requires later changes.
When calculating the amount available to send for the next HTLC, if we over-count we may create routes which are not actually usable. Historically this has been an issue, which we resolve over a few commits. Here we include the cost of the commitment transaction fee in our calculation, subtracting the commitment tx fee cost from the available as we do in `send_payment`. We also add some testing when sending to ensure that send failures are accounted for in our balance calculations. This commit is based on original work by Gleb Naumenko <naumenko.gs@gmail.com> and modified by Matt Corallo <git@bluematt.me>.
When calculating the amount available to send for the next HTLC, if we over-count we may create routes which are not actually usable. Historically this has been an issue, which we resolve over a few commits. Here we consider the number of in-flight HTLCs which we are allowed to push towards a counterparty at once, setting the available balance to zero if we cannot push any further HTLCs. We also add some testing when sending to ensure that send failures are accounted for in our balance calculations.
TheBlueMatt
force-pushed
the
2023-05-next-htlc-min-max
branch
from
40f3f28 to
a3246d1
Compare
TheBlueMatt
force-pushed
the
2023-05-next-htlc-min-max
branch
from
a3246d1 to
cf8ce3b
Compare
Codecov ReportPatch coverage:
❗ 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 #2312 +/- ##
==========================================
+ Coverage 90.81% 91.45% +0.64%
==========================================
Files 104 104
Lines 53009 64073 +11064
Branches 53009 64073 +11064
==========================================
+ Hits 48139 58600 +10461
- Misses 4870 5473 +603
☔ View full report in Codecov by Sentry. |
wpaulino
reviewed
May 25, 2023
dunxen
reviewed
Jun 2, 2023
dunxen
reviewed
Jun 5, 2023
TheBlueMatt
force-pushed
the
2023-05-next-htlc-min-max
branch
from
885af49 to
559e4be
Compare
alecchendev
reviewed
Jun 6, 2023
TheBlueMatt
force-pushed
the
2023-05-next-htlc-min-max
branch
from
559e4be to
d297ca0
Compare
When calculating the amount available to send for the next HTLC, if we over-count we may create routes which are not actually usable. Historically this has been an issue, which we resolve over a few commits. Here we consider whether one additional HTLC's commitment tx fees would result in the counterparty's commitment tx fees being greater than the reserve we've picked for them and, if so, limit our next HTLC value to only include dust HTLCs. We also add some testing when sending to ensure that send failures are accounted for in our balance calculations. This, and the previous few commits, fixes lightningdevkit#1126.
In the coming commits, in order to ensure all routes we generate are usable, we'll start calculating the next-HTLC minimum for our channels and using it in the router. Here we set this up by adding an always-0 field for it in `ChannelDetails` and use it when routing.
When calculating the amount available to send for the next HTLC, if we over-count we may create routes which are not actually usable. Historically this has been an issue, which we resolve over a few commits. Here we consider how much adding one additional (dust) HTLC would impact our total dust exposure, setting the new next-HTLC-minimum field to require HTLCs be non-dust if required or set our next-HTLC maximum if we cannot send a dust HTLC but do have some additional exposure remaining. We also add some testing when sending to ensure that send failures are accounted for in our balance calculations. Fixes lightningdevkit#2252.
Now that the value available to send is expected to match the success or failure of sending exactly, we should assert this in the `chanmon_consistency` fuzzer. In the next commit we'll actually rip the checks out of `send_htlc` which will make this a somewhat less useful test, however fuzzing on this specific commit can help to reveal bugs.
Now that the `get_available_balances` min/max bounds are exact, we can stop doing all the explicit checks in `send_htlc` entirely, instead comparing against the `get_available_balances` bounds and failing if the amount is out of those bounds. This breaks support for sending amounts below the dust limit if there is some amount of dust exposure remaining before we hit our cap, however we will no longer generate such routes anyway.
TheBlueMatt
force-pushed
the
2023-05-next-htlc-min-max
branch
from
d297ca0 to
4bc3a79
Compare
Collaborator
Author
|
Squashed without further changes: |
wpaulino
approved these changes
Jun 7, 2023
dunxen
approved these changes
Jun 7, 2023
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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 calculating the amount available to send for the next HTLC, if
we over-count we may create routes which are not actually usable.
Historically this has been an issue, constructing routes that aren't payable and leading to spurious payment failures.
Here we fix that issue across several commits, ultimately removing the entire can-we-send complexity in
send_htlcentirely.Fixes #1126.
Fixes #2252.