-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
lnrpc+peer: custom peer messages #5346
Conversation
Concept ACK |
2a118cb
to
0eb5424
Compare
Unit test added |
3cbd4c8
to
6789090
Compare
I think this PR is complete and has a concept-ack. Is it worth the 0.14 tag? |
6789090
to
1d8fee0
Compare
I'm here to ACK and let everyone know that we need this, to enable Offers, a set of features we would like to have access to for wallets ASAP. |
@BitcoinErrorLog a PR like this in isolation doesn't enable stuff like Offers. Offers uses a distinct invoice format from BOLT 11 (so you'd need RPC calls to create/parse/pay them), and it also requires a new type of onion handling (handle the onion messages that don't have any HTLC attached) within the core routing network. |
What about just sending a reply pubkey (or hop hint) to use? Message extension RPCs like this are cool, but if you want to do end to end interactions (outside of connecting to a peer you knows offers these features to send stuff back and forward), you'd need to assume every node in the critical path understands the messages. AFAICT, all the sample use cases you've enumerated assume that you're making direct connections as other wise you'd need to use the existing onion routing protocol which has the size constraints mentioned. |
I call this 'not easy', because you aren't sure if there is a return path and the other complexities/edge cases that exist.
Yes, my intention with these extensions is to just do direct communication. I guess you could use it to set up some kind of forwarding service as well, but haven't given that any thought. |
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.
Pretty straight forward diff, ends up adding a lot of utility fo general experimatntion in the protocol which is cool. The only thing I think we need to examine more closely, is how we handle the cases where there exits no subscription, and also how to govern ourt actions depending on if we receive an even or odd type for the unknown message.
@@ -512,6 +512,10 @@ func MainRPCServerPermissions() map[string][]bakery.Op { | |||
Entity: "offchain", | |||
Action: "write", | |||
}}, | |||
"/lnrpc.Lightning/SendCustomMessage": {{ |
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.
Open questions if which sub-server this should live in 🤔
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.
Because there isn't an obvious sub-server for it to live in, I opted for the main one.
@@ -51,3 +52,32 @@ func sendCustom(ctx *cli.Context) error { | |||
|
|||
return err | |||
} | |||
|
|||
var subscribeCustomCommand = cli.Command{ |
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 how useful this CLI command will be in practice, but don't see much harm in including it /shruggie
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, its use is doubtful. But because this is generic functionality, there may be an unexpected use for it. In any case, it is useful in development.
peer/brontide.go
Outdated
@@ -1422,6 +1431,13 @@ out: | |||
|
|||
discStream.AddMsg(msg) | |||
|
|||
case *lnwire.Custom: | |||
err := p.cfg.HandleCustomMessage(p.PubKey(), msg) |
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.
If there's no custom message handler, then we should treat this as an error. In addition, if the message's type is even, even if there's a handler, we should likely return an error state.
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.
Added error case, even though a handler is always registered currently.
Not sure about even types. lnd
doesn't know whether the external handler understands an even message? This would probably require an ack from the external handler, similar to what the htlc interceptor does. But then also with support for multiple handlers. I think that would make this PR a whole lot more complex, because the messages stay up in the air until the external process has given a response. And if I am seeing this right, the only difference is in what is logged locally? Because there isn't an actual response message on the wire.
1d8fee0
to
fd5f444
Compare
fd5f444
to
873a520
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.
Great addition, I could think of a few usecases for this myself. The addition of an RPC interface adds loads of possibilities for the casual developer, and I think should be added for an easier development experience.
873a520
to
15f2032
Compare
Visit https://dashboard.github.orijtech.com?back=0&pr=5346&remote=true&repo=joostjager%2Flnd to see benchmark details. |
@joostjager to get a baseline for gobencher, please first trigger a commit from your main/master branch then push again to a branch. |
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 (pending @Roasbeef)! Super excited for this one
15f2032
to
7db7f4a
Compare
Rebased |
7db7f4a
to
182e837
Compare
Re-rebased. Waiting for final review. |
182e837
to
cf197b0
Compare
One last rebase? |
cf197b0
to
f88d1c8
Compare
One last rebase |
Seems that there are test and linter issues. |
f88d1c8
to
ade50d0
Compare
Linter was just collateral damage of a broken test. All fixed now again. |
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 🌳
Yeah we're aware of the flake, def not introduced by this PR. |
Lightning nodes have a connection to each of their peers for exchanging messages. In regular operation, these messages coordinate processes such as channel opening and payment forwarding.
The lightning spec however also defines a custom range (>= 32768) for experimental and application-specific peer messages.
Custom peer messages open up a whole range of interesting new possibilities. They allow the lightning protocol with its transport mechanisms (including tor) and public key authentication to be leveraged for application-level communication. Note that peers exchange these messages directly. There is no routing/path finding involved.
Some examples:
Application-level communication was already possible using custom tlv records, but considerable restrictions apply. The most obvious being an absolute maximum message size of ~1300 bytes (depending on the route). Also there is no easy way for the receiving node to send a message back. This would involve finding a separate (possibly non-existent) return route. The upside of using custom tlv records is that the data is onion routed just as the carrying payment is, protecting the sender's identity.
For peers that already know each other (for example because they maintain a channel) or don't mind knowing each other, custom tlv records don't make too much sense but do make communication cumbersome and unreliable.
This PR adds to
lnd
the capability to send and receive custom peer messages via newSendCustomMessage
andSubscribeCustomMessages
rpc methods. For easy testing,lncli
commands are provided.To send a message:
lncli sendcustom --peer 03e3927c8e408c92be06a3dd0031600ff599abd00588e0e4bb9db217c6f16f0138 --type 42000 --data ff00ff11
To listen for incoming messages:
lncli subscribecustom
Looking for concept ACK.
Todo: