Skip to content

PaymentBandwidth uses HtlcView #1462

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

GeorgeTsagk
Copy link
Member

Description

When AuxTrafficShaper.PaymentBandwidth is called asynchonously we could previously report the same bandwidth for 2 different HTLCs that are validating whether they can be added to the channel. This could lead to false-positives and payment failures as the latter HTLC should be checked against the correct bandwidth.

Instead of always consulting the commitment blob on what the current asset balance is, we now use the existing helper ComputeView to get a more precise representation of the asset balance, which is enabled by the new argument lnwallet.HtlcView.

@GeorgeTsagk GeorgeTsagk requested a review from guggero April 8, 2025 18:23
@GeorgeTsagk GeorgeTsagk self-assigned this Apr 8, 2025
@GeorgeTsagk GeorgeTsagk changed the title Aux bandwidth htlcview PaymentBandwidth uses HtlcView Apr 8, 2025
@coveralls
Copy link

coveralls commented Apr 8, 2025

Pull Request Test Coverage Report for Build 14516181401

Details

  • 0 of 91 (0.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on aux-bandwidth-htlcview at 28.549%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapchannel/aux_funding_controller.go 0 1 0.0%
server.go 0 2 0.0%
tapchannel/commitment.go 0 7 0.0%
tapchannel/aux_traffic_shaper.go 0 81 0.0%
Totals Coverage Status
Change from base Build 14492457954: 28.5%
Covered Lines: 26242
Relevant Lines: 91918

💛 - Coveralls

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise this looks good to me. But IMO we'd want a bit more certainty around whether this really works in all cases.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I think the code is correct now for our use case.
Now it's just code style and reviewer friendliness that can be improved somewhat.

@GeorgeTsagk GeorgeTsagk force-pushed the aux-bandwidth-htlcview branch 2 times, most recently from 98260c9 to 8e9b450 Compare April 17, 2025 12:41
@GeorgeTsagk GeorgeTsagk requested a review from guggero April 17, 2025 12:44
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

To make `PaymentBandwidth` more readable, we separate the main codepaths
into helper functions. One is used when we're directly comparing asset
units to each other, and the other one is the path that calculates an
msat amount based on an RFQ quote.
We now make use of the AuxHtlcView to calculate a more precise value of
our local balance. This is using the ComputeView helper in order to
manually process HTLC adds from our side, providing us with the correct
value of local balance. We also add the DecodedView as a return value of
the helper function, but this will only be used in the next commit for
verbose logging.
We add a trace log which includes a pretty print of the HTLCs that are
part of our local update log.
@GeorgeTsagk GeorgeTsagk force-pushed the aux-bandwidth-htlcview branch from 8e9b450 to 3182e35 Compare April 17, 2025 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants