htlcswitch: re-add single decode hop iterator#10665
Conversation
Summary of ChangesHello, 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 restores a previously removed method for decoding individual onion packets, which is essential for external payment services that process HTLCs one at a time. The re-introduced method has been updated to support modern features like route blinding, ensuring compatibility and enhanced privacy. This change streamlines the integration of external services with the HTLC interception API by providing a more natural and efficient decoding interface. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. 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 a new DecodeHopIterator function within the htlcswitch/hop package to decode and process Sphinx onion packets, including support for blinded paths. Comprehensive unit tests for this new functionality have been added, covering various valid and error scenarios. Additionally, the logging setup for the htlcswitch/hop package has been updated to initialize a logger by default, and the mockIteratorDecoder has been adjusted to reflect the new DecodeHopIterator signature. The review feedback suggests improvements related to adhering to line length guidelines for function signatures and calls, simplifying redundant error handling logic, making the blindingPoint.WhenSome call more concise, and updating comments for clarity and detailed documentation of the new exported function.
| noBlinding, | ||
| ) |
There was a problem hiding this comment.
The processor.DecodeHopIterator function call exceeds the recommended line length. Please wrap the arguments according to the style guide (Repository Style Guide, lines 170-186).
reader, tc.rHash, incomingCltv,
incomingAmt, noBlinding,| // log is a logger that is initialized with no output filters. This | ||
| // means the package will not perform any logging by default until the caller | ||
| // requests it. | ||
| var log btclog.Logger |
There was a problem hiding this comment.
The comment for the log variable states that it 'will not perform any logging by default until the caller requests it.' However, the newly added init() function now initializes the logger by default. Please update the comment to reflect this change for clarity.
| // log is a logger that is initialized with no output filters. This | |
| // means the package will not perform any logging by default until the caller | |
| // requests it. | |
| var log btclog.Logger | |
| // log is a logger that is initialized with no output filters. This | |
| // means the package will perform logging by default, but the caller can | |
| // customize it using UseLogger. | |
| var log btclog.Logger |
There was a problem hiding this comment.
The approach is copied from other lnd subpackage log.go files (like htlcswitch/log.go and funding/log.go) so I think it's probably okay to keep.
|
@calvinrzachman, remember to re-request review from reviewers when ready |
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
LGTM - had some minor nits
| var ( | ||
| fwdAmt uint64 = 500_000 | ||
| outgoingCltv uint32 = 144 | ||
| ) | ||
| var payloadBuf bytes.Buffer |
There was a problem hiding this comment.
put this in one var block
There was a problem hiding this comment.
merged both var blocks into one ✅
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| reader := bytes.NewReader(tc.onionBytes) |
There was a problem hiding this comment.
Nit: the subtests don't call t.Parallel(). The outer test does, and the sphinx router is read-only across all subtests, so parallel execution is safe. lnd style generally includes t.Parallel() as the first line inside each t.Run closure.
There was a problem hiding this comment.
thanks for pointing this out. updated so the subtests now run in parallel.
This commit re-adds the singular DecodeHopIterator method on OnionProcessor, which was removed in 42069ef as unused within lnd itself. The method is needed by the external payment service, which uses the HTLC interception API to receive onions one at a time and decrypts them locally using its own sphinx router. The signature accepts the same per-packet fields as DecodeHopIterators (incoming amount, CLTV, and blinding point) so that route blinding works out of the box.
Without this, running hop tests in isolation panics because the package's log variable is nil until the parent htlcswitch package calls UseLogger during init.
calvinrzachman
force-pushed
the
decode-hop-iterator-blinding
branch
from
203e52f to
24f2b24
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
⚪ Excluded from counting (1 file)
AnalysisAll non-test files reside under No severity bumps triggered: 3 non-test files, 84 non-test lines changed (thresholds: >20 files or >500 lines). All files are within a single critical package. To override, add a |
ziggie1984
merged commit 519807c
into
lightningnetwork:elle-base-branch-payment-service
5 participants
Change Description
This commit revives the singular
DecodeHopIteratormethod onOnionProcessor, which was removed in 42069ef after having been replaced by the batch processedDecodeHopIteratorswithin lnd itself. The singular version of the method is more natural for use by the external invoice registry ("payment service"), which uses the HTLC interception API to receive onions one at a time and decrypts them locally using its own sphinx router. It doesn't have need for the batch version's transaction-based replay log, the id/re-forward parameters, etc. that the channel link uses when processing multiple HTLCs per commitment update.The old function signature is updated to include updates made since its removal. It accepts the same per-packet fields as
DecodeHopIterators(incoming amount, CLTV, and blinding point) so that route blinding is supported.