Attributable failures#9888
Open
GeorgeTsagk wants to merge 10 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
GeorgeTsagk
force-pushed
the
attr-failures
branch
from
f713f3c to
6640a01
Compare
Collaborator
Author
|
Bumped lightning-onion and rebased on |
GeorgeTsagk
force-pushed
the
attr-failures
branch
from
6640a01 to
1dd8765
Compare
Collaborator
Author
|
The linter failure is only about the |
Collaborator
|
/gemini review |
![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 support for attributable failures, which is a significant and complex change to the error handling and encryption logic. The core refactoring to separate shared secret extraction from error encrypter creation is well-executed and improves the design.
I've found a few issues related to the implementation:
- An incorrect unit calculation for HTLC hold times, which deviates from the BOLT specification.
- A logic error in handling failures for introduction nodes in a blinded path, where the wrong message type is created and attribution data is discarded.
- A minor opportunity for code simplification in the new logging logic.
Overall, the changes are well-structured, but the identified issues, particularly the correctness ones, should be addressed. The PR description is clear about the work-in-progress nature, and this review should help in finalizing the implementation.
GeorgeTsagk
force-pushed
the
attr-failures
branch
from
1dd8765 to
1fc33bd
Compare
Collaborator
Author
|
Rebased |
GeorgeTsagk
force-pushed
the
attr-failures
branch
from
1fc33bd to
a5b9c2a
Compare
Collaborator
Author
|
Rebased |
GeorgeTsagk
force-pushed
the
attr-failures
branch
from
a5b9c2a to
832693a
Compare
Collaborator
Author
|
Added an itest to verify reported hold times from a multi hop error |
GeorgeTsagk
force-pushed
the
attr-failures
branch
from
832693a to
4ff0dd6
Compare
Collaborator
|
@GeorgeTsagk, remember to re-request review from reviewers when ready |
GeorgeTsagk
force-pushed
the
attr-failures
branch
from
4ff0dd6 to
51e069f
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (16 files)
🟠 High (3 files)
🟡 Medium (2 files)
AnalysisThis PR introduces attributable payment failures - a mechanism for identifying which hop along a payment route caused a failure. The changes span multiple critical subsystems simultaneously:
Severity bump triggers (all three fired; CRITICAL is already the maximum):
Changes to the HTLC error encryption path and wire protocol are security-sensitive - incorrect attribution data could leak routing information or cause hard-to-diagnose payment failures. To override, add a |
GeorgeTsagk
force-pushed
the
attr-failures
branch
2 times, most recently
from
be35c47 to
517ee50
Compare
Collaborator
Author
|
Rebased on |
Co-authored-by: Joost Jager <joost.jager@gmail.com>
Bump lightning-onion dependency to include attributable error support.
Preparation for the instantiation of an attributable error encrypter. This gets rid of the sphinx encrypter instantiation in OnionProcessor. This would otherwise be problematic when an attributable encrypter would need to be created there without having access to the error structure. Co-authored-by: Joost Jager <joost.jager@gmail.com>
We add some simple helpers for the purpose of parsing the attribution data from and to the wire format.
When propagating a failure backwards along a route we want to persist the received ExtraData of the downstream link so that any handling can occur on our node before returning the updated ExtraData to our upstream link.
We now call into the new encryption/decryption methods and provide the received attribution data as an argument. The result is then also returned to our upstream peer.
Add test coverage for the new attributable failures feature: - lnwire/attr_data_test.go: round-trip, TLV type, empty/nil cases - hop/error_encryptor_test.go: encode/decode, backward compat, hold time, reextract, introduction/relaying encrypter round-trips - htlcswitch/attributable_failure_test.go: end-to-end encrypt/decrypt with attribution data, backward compat without attr data, hold times
Add a hold_times field to the Failure proto message so that callers of SendPaymentV2 can see per-hop hold times reported via attributable errors. The hold times are persisted in the KV store (backward compatible via trailing optional read) and populated in both the database and streaming RPC paths. Changes: - lnrpc/lightning.proto: add repeated uint32 hold_times = 10 - payments/db: add HoldTimes to HTLCFailInfo + KV serialization - routing: copy HoldTimes from ForwardingError to HTLCFailInfo - lnrpc/routerrpc: populate HoldTimes in both marshallHtlcFailure and marshallError
Add an integration test that sets up Alice -> Bob -> Carol, triggers a payment failure at Carol (unknown payment hash), and verifies that Alice receives hold times in the RPC failure response. The test checks: - hold_times has one entry per route hop (1:1 correspondence) - failure source index matches Carol (index 2) - each hold time is within a reasonable bound
GeorgeTsagk
force-pushed
the
attr-failures
branch
from
517ee50 to
9030e8b
Compare
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
4 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.
Description
Adds support for Attributable failures (feature 36/37). This changes the error encryption and the data encoding for the
update_fail_htlcmessage. We now include the attribution structure in the extra data of the peer message, which helps with assigning blame across the route and also extracting the reported HTLC hold times for each node across the route.For now we default to using non-strict attribution decryption, which means that we're basically preserving the old behavior in order to give time to nodes to upgrade. Otherwise this could lead to premature blame of our direct peers, possibly blacklisting all of our channels early in the payment lifecycle.
This is based on the lightning-onion PR which is also updated to reflect the latest state of attributable failures Bolt PR.
Testing
Current unit/integration tests seem to be passing, which validates that we are preserving the old behavior.
Todo:
Even though the todo list above is important to consider this PR ready to merge, this PR at current state is still good for a first round of review.