-
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
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.
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 |
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
onionmessage/onion_endpoint.go
Outdated
} | ||
|
||
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?
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 I follow. We get the peer from which we receive the message from the 'msg' itself. Do you mean the peer that is parsing the message, so the node itself?
The log message now shows this:
2025-10-09 11:00:14.625 [DBG] OMSG onion_endpoint.go:71: OnionEndpoint received OnionMessage peer=02765192bdda4b72ac78a87661656102ce891ffd9961570850164d258e7ed2c88f path_key=02e97c12b29f onion_blob=010203 "blob length"=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.
I was thinking of something like this:
ctx = btclog.WithCtx(ctx,
slog.String("peer", hex.EncodeToString(peer)),
lnutils.LogPubKey("path_key", onionMsg.PathKey),
)
log.DebugS(ctx, "OnionEndpoint received OnionMessage",
btclog.HexN("onion_blob", onionMsg.OnionBlob, 10),
slog.Int("blob_length", len(onionMsg.OnionBlob)))
then below in
log.ErrorS(ctx, "Failed to send onion message update", err)
you will have all the information when using this ctx again.
a988f91
to
bbe7671
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 🙏
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!, very easy to follow. Left some nits
bbe7671
to
72ecd16
Compare
496c5e3
to
8eb29f1
Compare
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.
72ecd16
to
0cbf2ad
Compare
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)