Add AuxHtlcValidator#10434
Conversation
Summary of ChangesHello @GeorgeTsagk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust mechanism to enhance the reliability of HTLC additions within Lightning channels. It addresses a potential race condition where payment bandwidth calculations could become outdated by implementing a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an AuxHtlcValidator to perform a final bandwidth check right before an HTLC is added to the channel state. This is a solid approach to prevent race conditions arising from stale bandwidth information during pathfinding. The implementation is clean and integrates well with the existing channel logic. My main feedback is the lack of unit tests for this new validation logic, which would be beneficial to ensure its correctness and cover edge cases. I've also included one minor suggestion for code simplification.
peer/brontide.go
Outdated
| peerBytes := p.IdentityKey().SerializeCompressed() | ||
| peer, err := route.NewVertexFromBytes(peerBytes) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create vertex from peer "+ | ||
| "pub key: %w", err) | ||
| } |
There was a problem hiding this comment.
This block can be simplified by using route.NewVertex. The current implementation serializes the public key to bytes, then route.NewVertexFromBytes parses it back to a public key for validation before converting it to a route.Vertex. Since p.IdentityKey() is guaranteed to return a valid *btcec.PublicKey, we can use route.NewVertex directly. This is slightly more efficient and makes the code cleaner by removing the unnecessary error handling.
| peerBytes := p.IdentityKey().SerializeCompressed() | |
| peer, err := route.NewVertexFromBytes(peerBytes) | |
| if err != nil { | |
| return fmt.Errorf("failed to create vertex from peer "+ | |
| "pub key: %w", err) | |
| } | |
| peer := route.NewVertex(p.IdentityKey()) |
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
My gut reaction: can we get rid of some of the extra calls that validate this elsewhere (eg: switch method calls into the link to check if an HTLC is ready for transit) if we're adding this additional layer of protection?
We definitely need more than 1 call sites per operation (forward / payment) as we need to first quickly gauge if enough funds exist to go ahead with the operation, then verify things one last time before committing it to the channel. The aux bandwidth calls though seem to be a bit intertwined. By adding the Will update PR soon |
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
283874d to
aea7673
Compare
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
aea7673 to
a3ebf57
Compare
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
a3ebf57 to
b29a104
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Interesting approach, this solves all your flakes on the tap side ?
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
b29a104 to
55d1579
Compare
|
@Roasbeef @ziggie1984 Would be good to prio review on this after the last round of fixes. This should help fix some recurrent itest flakes we get pummelled by in tapd. |
| return fn.Map(v.Updates.Remote, newAuxHtlcDescriptor) | ||
| } | ||
|
|
||
| // FetchLatestAuxHTLCView returns the latest HTLC view of the lightning channel |
There was a problem hiding this comment.
is this only returning the Taproot Assets HTLC or all HTLCs of a channel ?
There was a problem hiding this comment.
all HTLCs of the channel, wrapped inside the AuxHtlcDescriptor
There was a problem hiding this comment.
could you explain why we need to fetch the none AssetHTLCs as well tho, how are they treated in the limit calculation because they do not represent any assets or are they then just skipped in the taproot assests evaluation ?
There was a problem hiding this comment.
All HTLCs are included because ComputeView (in tapchannel/commitment.go) needs the full picture. It iterates through all updates and distinguishes asset vs non-asset HTLCs. Non-asset HTLCs are skipped for the asset balance calculation and placed into a separate nonAssetView (used downstream for non-asset allocations). Only HTLCs with asset custom records affect the computed local asset balance.
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
55d1579 to
7b88227
Compare
|
@claude review this |
|
Claude Code is working… I'll analyze this and get back to you. |
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Could you explain to me why we have the
// opts is the set of options that channel was initialized with.
opts *channelOpts`
where the new auxHtlcValidator lives and in
LightningChannel
we have like auxResolver or auxSigner on a different level, did we just change our design and now try to move everything into channelOpts or is there another reason.
The The |
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
7b88227 to
977cac6
Compare
lightninglabs-deploy
added
the
severity-critical
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
977cac6 to
b4e95a1
Compare
lightninglabs-deploy
added
severity-critical
|
@Roasbeef: review reminder |
peer/brontide.go
Outdated
| // The linkBandwidth is provided by the channel and represents | ||
| // the current available balance, which is used by the traffic | ||
| // shaper to ensure we don't dip below channel reserves. | ||
| bandwidth, err := ts.PaymentBandwidth( |
There was a problem hiding this comment.
Perhaps we should add a context.Context here? Then it can exit out safely if this hangs for w/e reason.
Non-blocking.
There was a problem hiding this comment.
Since you marked this as non-blocking: Let's leave it as is for the scope of this PR.
I'd be very happy to have a follow up switching all the aux interfaces to using the actor-ish model (with all hooks returning fn.Result immediately). In that version I think context is unnecessary.
github-project-automation
bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
Previously we'd perform aux bandwidth checks during path finding. This could lead to issues where multiple HTLCs where querying the same bandwidth but were not accounting for each other before being added to the commitment log. We now add a new validator function that will serve as the last point of checks before adding the HTLC to the commitment. During path finding HTLCs could query channel bandwidth asynchronously. At this new call site all HTLCs that are about to be added to the channel have been organised in sequence, so it's safe to query bandwdith again at this point as we're getting the actual up-to-date values. We remove the aux bandwidth check from the helper canSendHtlc, which was called from CheckHTLCTransit and CheckHTLCForward (both are methods of the htlcswitch). For forwards we now fail at the link level, following the introduction of the AuxHtlcValidator. For payments, we now may fail either at the pathfinding level, or at the link level. The htlcswitch may no longer fail for aux bandwidth checks. Finally, when fetching the latest htlc view (for bandwidth checks during pathfinding) we'd silently set the nextHeight of the view to the default zero value. We now make sure to set it to the correct nextHeight value.
When instantiating the lightning channel we now pass in the created HTLC validator. This validator simply performs a bandwidth check and errors out if that is insufficient.
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
b4e95a1 to
defb09e
Compare
lightninglabs-deploy
added
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
AnalysisThis PR touches two distinct CRITICAL packages: Expert review is warranted given the combination of wallet/channel logic changes alongside peer connection changes. To override, add a |
We add this constructor for an AuxHtlcDescriptor that allows setting some of the internal fields. This is useful for testing purposes for code external to this package that may need to extensively test the AuxHtlcView.
GeorgeTsagk
force-pushed
the
extra-aux-band-check
branch
from
defb09e to
8d30e7d
Compare
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
6 participants
Description
Updates the lightning channel to query the TrafficShaper bandwidth once more before adding the HTLC to the channel state. During pathfinding, the reported payment bandwidth could be stale, as it may have not accounted for HTLCs that have not yet been added to the channel state (i.e the aux htlc view).
By querying the aux bandwidth once more, right before the HTLC is added to the channel state, we ensure that no race condition can lead to unexpected failures due to insufficient balance.