Onion messaging support #68
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gijswijs, 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 significantly extends the lightning-onion package by integrating full support for onion messaging, a distinct type of onion routing. The changes involve fundamental modifications to how onion packets are structured, created, and processed, enabling greater flexibility in payload sizes and strict adherence to TLV encoding for messages. This enhancement is crucial for future-proofing the library and supporting advanced messaging capabilities within the Lightning Network.
Highlights
- Onion Messaging Support: This pull request introduces comprehensive support for 'onion messaging' within the
lightning-onionpackage, distinguishing it from traditional payment onions. This includes new packet structures, creation, and processing logic tailored for onion messages. - Flexible Routing Info Sizes: The fixed-size
RoutingInfofield inOnionPackethas been replaced with a dynamic slice ([]byte), enabling support for variable payload sizes. New constantsStandardRoutingInfoSize(1300 bytes) andJumboRoutingInfoSize(32768 bytes) are introduced to accommodate different onion message lengths, including those exceeding the previous 1300-byte limit. - Payload Type Enforcement: Onion messages are now strictly enforced to use TLV (Type-Length-Value) payloads, aligning with BOLT 7 specifications. The packet creation and processing logic has been updated to validate this, rejecting legacy payloads for onion messages.
- Updated Packet Creation and Processing: The
NewOnionPacketandProcessOnionPacketfunctions now accept functional options (OnionPacketOption,ProcessOnionOpt) to specify if the packet is an onion message. This allows for specialized handling, such as dynamic routing info sizing and payload type validation, based on the packet's purpose. - New Test Vectors and Comprehensive Testing: New test vectors from the BOLT 4 spec for onion messaging have been integrated. Dedicated tests (
TestBuildOnionMessageBlindedRoute,TestOnionMessageRouteBlinding,TestTLVPayloadMessagePacket) have been added to validate blinded route construction, packet processing, and TLV payload handling for onion messages. Tests for zero-length and larger than 1300-byte payloads are also included. - CLI Tool Enhancement: A new
--onion-messageflag has been added to thecmd/main.goCLI tool, allowing users to generate onion message packets directly. - Dependency Updates: Several Go module dependencies, including
btcd,btcec/v2,stretchr/testify, andgolang.org/x/crypto, have been updated to their latest versions. New indirect dependencies likelnd/tlvhave also been added.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for onion messaging, including handling of blinded routes and variable-sized payloads. The changes include new logic for packet creation and processing, and are supported by test vectors and new unit tests. I have a few suggestions to improve code clarity and robustness.
gijswijs
force-pushed
the
onion-messaging
branch
2 times, most recently
from
6aa8a12 to
dd68d45
Compare
|
cc: @yyforyongyu @ellemouton for review |
There was a problem hiding this comment.
Second round done. My main comments are,
- the
legacy vs empty tlv streamcheck, and wonder whether it's needed, and if so, can we do it in a higher layer instead. Also wondering if we could drop the support for legacy payload. - code format needs to be fixed.
- need to make it compile, ran linter and got
> golangci-lint run
cmd/main.go:1: : # github.com/gijswijs/lightning-onion/cmd
cmd/main.go:212:25: undefined: sphinx.OnionPacketOption
cmd/main.go:214:40: undefined: sphinx.WithOnionMessage
cmd/main.go:217:14: cannot use ... in call to non-variadic sphinx.NewOnionPacket (typecheck)
package mainI also noticed that we don't have any CI here, maybe a minimal CI that includes jobs like unit test, lint, and check commits would be a good starting point, and it should be easy to add as we can just copy files from lnd. Another PR tho.
sphinx.go
Outdated
| totalPayloadSize := paymentPath.TotalPayloadSize() | ||
|
|
||
| var routingInfoLen int | ||
| maxRoutingInfoErr := ErrStandardRoutingInfoSizeExceeded |
There was a problem hiding this comment.
var maxRoutingInfoErr error
There was a problem hiding this comment.
You don't like short variable declarations? I'm not sure what you want here.
|
|
||
| // If this is an onion message, we only expect TLV | ||
| // payloads. | ||
| if cfg.isOnionMessage && isLegacy { |
There was a problem hiding this comment.
Think this legacy vs empty tlv kinda increases the complexity a lot - I wonder if we need to check it here, like a higher layer should be able to catch and error out there?
There was a problem hiding this comment.
@ellemouton made a similar remark here: #68 (comment)
NewOnionPacket is exposed, so the higher layer you reference is outside of this library. Do we want to offload those checks to the ones importing this library?
I think those checks make the library more easy to use, because they give useful error messages.
My $0.02.
| // header. The MaxPayloadSize bytes at the end are used to | ||
| // encrypt/decrypt the fillers when processing the packet of generating | ||
| // the HMACs when creating the packet. | ||
| numStreamBytes = routingInfoSize * 2 |
There was a problem hiding this comment.
I really like the docs here, and wonder if we could somehow retain it, like make numStreamBytes a method that takes routingInfoLen and returns it, otherwise I see routingInfoLen * 2 and will be wondering why the *2
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for onion messaging, including blinded routes and variable-sized onion packets, which is a significant feature addition. The implementation is well-supported by new tests, including vectors from the BOLT specification.
My review focuses on enhancing the code quality. I've suggested improvements to test robustness by replacing ignored errors with explicit checks, and a simplification of some conditional logic to improve readability and maintainability. These suggestions align with general Go best practices for writing clear and robust code.
sphinx.go
Outdated
| routingInfoLen = StandardRoutingInfoSize | ||
| default: | ||
| routingInfoLen = JumboRoutingInfoSize |
There was a problem hiding this comment.
the spec says that the length of the packet for onion messages should be either 1366 or 32834 but not that it must. ie, i think you need to support it being other lengths too.
There was a problem hiding this comment.
Yeah, it says should but why support random lengths? This is what the rationale is in the spec.
lenallows larger messages to be sent than the standard 1300 bytes allowed for an HTLC onion, but this should be used sparingly as it reduces the anonymity set, hence the recommendation that it either looks like an HTLC onion, or if larger, be a fixed size.
Let's nudge people in using this correctly and automatically switch to the fixed large size if they cross the standard routing info size threshold.
sphinx.go
Outdated
| // If this is an onion message, a blinding point must be provided. | ||
| if cfg.isOnionMessage && cfg.blindingPoint == nil { | ||
| return nil, fmt.Errorf("blinding point must be provided for " + | ||
| "onion messages") | ||
| } | ||
|
|
There was a problem hiding this comment.
and that assocData is empty. But imo, this is a business logic check that should happen at a higher level
There was a problem hiding this comment.
Hmm, not sure if I agree. I would say this is distinct from business logic and qualifies as validation logic. It's not really a business policy that you need to supply a blinding point, it's a fundamental building block of onion messages as such. Also, this is a package that people use, and it's helpful to supply them with meaningful error messages.
But we can probably debate this for hours. I'm ok with it either way. Just say the word.
| @@ -41,12 +41,25 @@ const ( | |||
| LegacyHopDataSize = (RealmByteSize + AddressSize + AmtForwardSize + | |||
| OutgoingCLTVSize + NumPaddingBytes + HMACSize) | |||
|
|
|||
There was a problem hiding this comment.
when you say breaking change - i think you just mean that compilation breaks - which imo i not a problem & we dont need to account for it. at the time that we update LND to point to the new onion pkg version, we just fix the compilation
There was a problem hiding this comment.
Yeah, correct, compilation breaks because people currently use MaxPayloadSize which is exposed. I tried to make this PR such that people could update the lightning-onion package without changing their code. I'm not sure how widely used lightning-onion is at this moment. If we only use it in lnd than obviously this isn't a concern.
payload.go
Outdated
| func (hp *HopPayload) Decode(r io.Reader, isMessage ...bool) error { | ||
| // To preserve backwards compatibility, we'll default to isMessage being | ||
| // false if it is not provided. | ||
| isMsg := len(isMessage) > 0 && isMessage[0] |
There was a problem hiding this comment.
same comment as previous commit. i dont think we need to do this. Also adding a variadic param here doesnt make sense.
There was a problem hiding this comment.
afaik, using a variadic argument is idiomatic Go for maintaining backward compatibility. It doesn't break existing callers.
But I guess we're ok with breaking existing callers, so I'll go ahead and remove this.
This commit removes an unused var and changes bytes.Compare to the idiomatic bytes.Equal.
This commit adds the spec test vector for blinded onion messages. It also adds a test that tests BuildBlindedRoute, decryptBlindedHopData and NextEphemeral against this vector.
We add TestOnionMessageRouteBlinding which verifies that the onion message packet from the test vector can be processed correctly by the nodes in a blinded route.
gijswijs
force-pushed
the
onion-messaging
branch
from
0220389 to
e4bf9cc
Compare
|
@gijswijs, remember to re-request review from reviewers when ready |
TestTLVPayloadMessagePacket creates a onion message with payload and the blinded route from the test vector. It then checks if the onion packet we create is equal to the one provided in the test vector.
Since the onion message payload can be zero-length, we need to decode it correctly. This commit adds a boolean flag to the HopPayload Decode that tells whether the payload is an onion message payload or not. If it is, the payload is decoded as a tlv payload also if the first byte is 0x00. sphinx_test: Add zero-length payload om test
Onion messages allow for payloads that exceed 1300 bytes, in which case the payload should become 32768 bytes. This commit introduces support for those jumbo packets. sphinx_test: test jumbo size onion message packets This commit adds a helper function to create onion messages of a specified length. This helper is then used to test the handling of packets larger than 1300 bytes specifically for onion messages.
When we are parsing onion messages, we must ensure that a blinding point is provided.
The field MaxPayloadSize is added to the sphinx package to allow for backwards compatibility with the old sphinx package. Removing it would have been a breaking change.
gijswijs
force-pushed
the
onion-messaging
branch
from
e4bf9cc to
6d838d2
Compare
This PR adds support for onion messaging to the
lightning-onionpackage. It does the following things:TestBuildOnionMessageBlindedRoutethat tests BuildBlindedRoute, decryptBlindedHopData and NextEphemeral against this vector.TestOnionMessageRouteBlindingwhich verifies that the onion message packet from the test vector can be processed correctly by the nodes in a blinded route.TestTLVPayloadMessagePacketthat creates a onion message with payload and the blinded route from the test vector.It also adds support for zero-length payloads and longer than 1300 bytes payloads that are specifically supported by the spec, but not tested by the test vectors supplied. Both features are accompanied by their own unit tests.