-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Basic structures for onion messages into LND #9868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 0-21-0-staging
Are you sure you want to change the base?
Basic structures for onion messages into LND #9868
Conversation
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start of the onion message saga 🎉, I think the msgmux
is the way to go here. I am not sure if we should use one server for all peers but if we use it we should buffer the update channel of the onionserver
.
Let's now decrypt the onion message in the next PR ?
5762808
to
9f23971
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing request for review in the mean time :) feel free to re-request when ready!
e5726a7
to
fefe590
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @gijswijs, for putting this together! I've added some comments. Overall, I like the design and how it's seamlessly integrated with the LND system/subsystems. I can form a clear mental picture of the changes. Nice work!
9549248
to
d872936
Compare
msgmux
msgmux
msgmux
d872936
to
99a62aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good! nice and clean. Couple of comments
lnwire/onion_message.go
Outdated
type OnionMessage struct { | ||
// BlindingPoint is the route blinding ephemeral pubkey to be used for | ||
// the onion message. | ||
BlindingPoint *btcec.PublicKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we rename it to PathKey
to be consistent with the latest version of the spec? It will also help clarity once the actual blob contains blinded path info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we should also explain more re what it is used for. an intermediate node expects an encrypted_recipient_data which it can decrypt into an encrypted_data_tlv using the path_key which it is handed along with the onion message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: +1 for renaming to path_key, maybe also do this for our whole codebase where we currently use blindingPoint quite often ?
server.go
Outdated
// SendOnionMessage sends a custom message to the peer with the specified | ||
// pubkey. | ||
// TODO(gijs): change this message to include path finding. | ||
func (s *server) SendOnionMessage(peerPub [33]byte, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if we should not have a dedicated OnionMessageServer? instead of adding this logic directly to the main server
struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean having a separate sub-rpc-server for onion-messages ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I mimicked the implementation of the customMessageServer
and that's also why this logic is in the main server
struct.
In phase 2 when we also will implement path finding for onion messages it definitely makes sense to rethink this, but for now I would keep it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are close here, please also add release notes similar 🙏
lnwire/onion_message.go
Outdated
type OnionMessage struct { | ||
// BlindingPoint is the route blinding ephemeral pubkey to be used for | ||
// the onion message. | ||
BlindingPoint *btcec.PublicKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: +1 for renaming to path_key, maybe also do this for our whole codebase where we currently use blindingPoint quite often ?
) | ||
|
||
// Subsystem defines the logging code for this subsystem. | ||
const Subsystem = "OMSG" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the G stand for here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onion -> o
message -> msg
onion + message -> omsg
require.NoError(ht.T, err) | ||
randomPub := randomPriv.PubKey() | ||
msgBlindingPoint := randomPub.SerializeCompressed() | ||
msgOnion := []byte{1, 2, 3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this fail because of the following (I know it is not a spec. MUST but why don't we follow it?):
SHOULD set onion_message_packet len to 1366 or 32834.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we will follow it.
There's an ongoing debate on how to handle onion_message_packet
sizes in the lightning-onion
package, but whatever solution we end up with, in one of the PRs in this saga the size will be clamped to those two values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Micro-nit: add a comment that this is only for testing purposes and the onionMsg is supposed to be either 1300 or 32... big
server.go
Outdated
// SendOnionMessage sends a custom message to the peer with the specified | ||
// pubkey. | ||
// TODO(gijs): change this message to include path finding. | ||
func (s *server) SendOnionMessage(peerPub [33]byte, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean having a separate sub-rpc-server for onion-messages ?
@gijswijs, remember to re-request review from reviewers when ready |
// manner as onions used to route HTLCs, with the exception that it uses | ||
// blinded routes by default. | ||
OnionBlob []byte | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for an ExtraData
field here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we need it on this level tho, why would I use a blinded route and add more information basically unblinded, I think the bulk of extra data will be in onionmsg_tlv
where we defintily need the ExtraData, but I mean there is no cost of provisioning it here as well.
/* lncli: `subscribeonion` | ||
SubscribeOnionMessages subscribes to a stream of incoming onion messages. | ||
*/ | ||
rpc SubscribeOnionMessages (SubscribeOnionMessagesRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be just for forwarded onion messages, or the ones received?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the ones received.
|
||
// OnionMessageServer is an instance of a message server that dispatches | ||
// onion messages to subscribers. | ||
OnionMessageServer *subscribe.Server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to verify my understanding: this is just hooked up into direct point-to-point messaging for now, to make a simple demo via the RPC server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, and to be able to see what messages are received in itests.
return peer.SendMessageLazy(true, msg) | ||
} | ||
|
||
// SendOnionMessage sends a custom message to the peer with the specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, you'll do this in a follow up PR, but I think the actor
package can fit rather nicely here.
We have two actor types: the processor, and the endpoint.
The processor handles the crypto logic to figure out who to send the message to next.
A message endpoint becomes an actor, identifiable by the public key of the node.
When we read an onion message off the wire, we send it to the processor actor, who the sends it to the relevant processor actor.
I have a PR that abstracts out the mailbox for actors, so it can be anything that implements the queue abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that PR. And I thought of using it. But I also wanted to keep the number of moving parts as low as possible. This PR saga will pull in the (yet unmerged) BackPressureQueue
in and I thought it was wise not to pull in another unmerged PR.
9857299
to
c72c704
Compare
Ok, this PR is ready for the next round of reviews.
The following will be addressed in later PRs:
Open questions:
|
fa0822b
to
3a18daf
Compare
can you rebase on 21-staging |
3a18daf
to
4c133b9
Compare
LogBytesPreview returns a slog attribute that shows a hex preview of the given byte slice. The result is truncated for readability. The preview length is fixed to avoid leaking full binary payloads in logs while still allowing identification of the data.
This message type is a message that carries an onion-encrypted payload used for BOLT12 messages.
This commit creates the necessary endpoints for onion messages. Specifically, it adds the following: - `SendOnionMessage` endpoint to send onion messages. - `SubscribeOnionMessages` endpoint to subscribe to incoming onion messages. It uses the `msgmux` package to handle the onion messages.
The only way to unblock SendCustomMessage is if the peer activates, disconnects or the server shuts down. This means that if the context is cancelled, we will still wait until one of those other events happen. With this commit we thread the context through to SendCustomMessage, so that if the context is cancelled, we can return early. This improves the cancellation semantics.
4c133b9
to
a988f91
Compare
✔️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// manner as onions used to route HTLCs, with the exception that it uses | ||
// blinded routes by default. | ||
OnionBlob []byte | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we need it on this level tho, why would I use a blinded route and add more information basically unblinded, I think the bulk of extra data will be in onionmsg_tlv
where we defintily need the ExtraData, but I mean there is no cost of provisioning it here as well.
require.NoError(ht.T, err) | ||
randomPub := randomPriv.PubKey() | ||
msgBlindingPoint := randomPub.SerializeCompressed() | ||
msgOnion := []byte{1, 2, 3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Micro-nit: add a comment that this is only for testing purposes and the onionMsg is supposed to be either 1300 or 32... big
} | ||
|
||
peer := msg.PeerPub.SerializeCompressed() | ||
log.DebugS(ctx, "OnionEndpoint received OnionMessage", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it makes sense to add the peer to the ctx so that it is printed below as well?
This first pull request introduces the basic structures for onion messages into LND.
It adds a new message type,
OnionMessage
, to thelnwire
package. This includes the message's definition, comprising a blinding point and an onion blob, along with the necessary serialization and deserialization logic for peer-to-peer communication.Additionally, it exposes functionality for handling these messages via gRPC by adding two new endpoints:
•
SendOnionMessage
: Allows a client to send an onion message to a specific peer.•
SubscribeOnionMessages
: Allows a client to subscribe to a stream of incoming onion messages. .Scope of this PR:
msgmux
endpoint inbrontide
Usage of
msgmux
msgmux provides a generic message routing system to decouple message handling from the main peer.Brontide
read loop. It's not widely used in LND as of yet, so an explanation might be in place.
peer.Brontide
instance is configured with amsgmux.Router
.onion_message.OnionEndpoint
, implement themsgmux.Endpoint
interface and are registered with the router.readHandler
, when a new message is received from a peer, it is first passed to themsgRouter.RouteMsg()
method.CanHandle()
the message, the router forwards the message to that endpoint for processing.RouteMsg
returnsnil
. Thebrontide.readHandler
then skips its large, legacy switch statement and proceeds to read the next message.RouteMsg
returnsErrUnableToRouteMsg
, and brontide falls back to its switch statement to process the message in the traditional way.This allows new message handling logic to be added as self-contained endpoints without modifying the peer's core read loop.
Out-of-scope (for this PR)