Add AuxHtlcValidator #10434
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.
| 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 
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.
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.
The validateAddHtlc helper is also called from the public function MayAddOutgoingHTLC which is used from places like the pathfinding logic. In those call sites we already perform bandwidth related checks for the purpose of pathfinding, so it is redundant to do it again within this helper. We use the aux bandwidth check only when truly adding the HTLC to the channel state.
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.
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.
GeorgeTsagk force-pushed the extra-aux-band-check branch from aea7673 to a3ebf57 Compare 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.
This is a subtle and important detail: The aux HTLC validator must use the same HTLC view that will be used when creating the remote commitment transaction. When we add an outgoing HTLC, we're creating a new remote commitment which only includes remote updates up to remoteACKedIndex (the last remote update we've ACKed in our local commitment). Previously, the validator used the full remote log index, including unACKed settles/fails. This caused an inconsistency: the validator would see remote settles and skip the corresponding adds (validating we have enough balance), but later FetchLeavesFromView would not see those same settles (since they're not ACKed yet), leading to balance underflows when it tried to account for adds without their settles.
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 ?
lnwallet/channel.go Outdated
| // pending HTLCs that haven't been committed yet. This | ||
| // provides the validator with the most accurate state. | ||
| view := lc.fetchHTLCView( | ||
| lc.updateLogs.Remote.logIndex, |
There was a problem hiding this comment.
I wonder why for the taproot assets side we do not need the guarantee that the remote ACKED the log entry of the remote log?
| return newAuxHtlcView(lc.fetchHTLCView( | ||
| lc.updateLogs.Remote.logIndex, lc.updateLogs.Local.logIndex, | ||
| )) | ||
| nextHeight := lc.commitChains.Local.tip().height + 1 |
There was a problem hiding this comment.
why is this nextHeight needed ?
| fundingBlob := dbChan.CustomBlob | ||
| commitmentBlob := dbChan.LocalCommitment.CustomBlob | ||
| | ||
| // Fetch the peer's public key. |
There was a problem hiding this comment.
Nit: its not really fetching but converting to a proper format
| // In order to avoid unnecessary validations of the aux bandwidth that | ||
| // may be costly to perform, let's skip unless this is the final check | ||
| // before adding the HTLC to the channel. | ||
| if !finalCheck { |
There was a problem hiding this comment.
instead of this boolean call it after validateHTLC in addHTLC ?
| // pending HTLCs that haven't been committed yet. This | ||
| // provides the validator with the most accurate state. | ||
| commitChain := lc.commitChains.Local | ||
| remoteIndex := commitChain.tail().messageIndices.Remote |
There was a problem hiding this comment.
Nit: Thats the remoteACK why not call it like this ?
| lc.updateLogs.Local.logIndex, | ||
| ) | ||
| | ||
| nextHeight := lc.commitChains.Local.tip().height + 1 |
There was a problem hiding this comment.
not quite sure why we need this height ?
| | ||
| // Get the current available balance for the link | ||
| // bandwidth check. This is needed for the reserve | ||
| // validation in the traffic shaper. We use NoBuffer |
There was a problem hiding this comment.
why do we need an extra reserve check on the traffic shaper level ? It should be done by LND ?
| linkBandwidth, _ := lc.availableBalance(NoBuffer) | ||
| | ||
| return validator( | ||
| pd.Amount, linkBandwidth, pd.CustomRecords, |
There was a problem hiding this comment.
does this pd.Amount hold the sathosi value of the HTLC or the asset value ?
3 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.