-
Notifications
You must be signed in to change notification settings - Fork 492
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
BOLT 7: Onion message support (features 38/39) #759
Conversation
This tracks lightning/bolts#759 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This tracks lightning/bolts#759 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
305ef45
to
6b6c5fc
Compare
This tracks lightning/bolts#759 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This tracks lightning/bolts#759 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Got really confused trying to understand how this proposal and #755 are related: is one a later version of the other? |
Oops, they were (almost!) identical. I closed the prior one since this one has comments. I'm rebasing on master now, having applied the typo fixes (thanks!). My only q is whether we should drop the The downside is that you can't reply with an error if the payload itself is unparsable... |
6b6c5fc
to
5551ab8
Compare
5edf3ce
to
c573e04
Compare
Trivial squash and rebase on master. |
c573e04
to
b47c545
Compare
Hmm, should we give this a real feature number? |
Since we're now tentatively self-assigning incremental feature numbers, I'm switching this to 38/39. |
b47c545
to
1ede04a
Compare
OK, this is the simplified version, FTW. I'm implementing it now: for the moment c-lightning will accept and send the old version too, to ease transition. Thanks @TheBlueMatt ! |
784946a
to
37d17c4
Compare
These use onion encoding for simple one-way messaging: there are no error returns. However, every onion uses route blinding *even if it doesn't need to*. You can prove what path was used to reach you by including `path_id` in the encrypted_data_tlv. Note that this doesn't actually define the payload we're transporting: that's explictly defined to be payloads in the 64-255 range. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
OK, I squashed into a single commit and fixed some whitespace (and changed a "MUST send an error" to "MUST ignore" for consistency). This now applies directly onto master now blinded payments are merged! |
1714bd7
to
8b4aa86
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.
Looks mostly good to me, only minor comments.
@thomash-acinq can you do a thorough review and double-check the test vectors against eclair?
Typo fixes from @t-bast, @thomash-acinq and @remyers.
@thomash-acinq points out: 1. We absolutely can put other fields in `encrypted_data_tlv`, esp. padding, and test vectors do this. 2. Presumably it was supposed to refer to onionmsg_tlv, so fix that. 3. And of course we need to allow payload fields!
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.
ACK 217a99e
@valentinewallace can you have a look at the latest state of the PR (and ideally the test vectors)? If everything looks good, we can (finally) merge this! |
I figure that this topic has been raised somewhere, but isn't the Onion Messaging system a potential DOS vector? If that discussion was resolved, is the documentation of that discussion in a different place? I am trying to catch up on the state of this and BOLT12 and IIRC this was an open point of debate for a while. |
Should onion messages get added here? |
as the `hop_payloads` format, except there is no "legacy" length: a 0 | ||
`length` would mean an empty `onionmsg_payload`. | ||
|
||
Onion messages are unreliable: in particular, they are designed to |
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.
Since OMs are unreliable and a sender might have to retry multiple times (maybe even try numerous paths simultaneously to gain more reliability), does it make sense to add an idempotency key to onion messages?
- MUST set the `onion_message_packet` `version` to 0. | ||
- MUST construct the `onion_message_packet` `onionmsg_payloads` as detailed above using Sphinx. | ||
- MUST NOT use any `associated_data` in the Sphinx construcion. | ||
- 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.
Does this mean that an OM can travel far more than 20 hops?
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.
Potentially, yes. With varonions the limitation to fixed 20 hops went away anyway, so even the smaller 1366 byte onion could potentially contain more hops. It all deppends on the payload you want to deliver at each hop.
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.
Thank you for the answer. So if I want to calculate the maximum number of hops I can go through, does this look right?
32768 / 70 =~ 468 hops
Agreed to merge pending @valentinewallace acking test vectors. |
Merged by agreement at 31-07-2023 meeting. |
adding link to spec meeting where this was merged #1098 |
Responding to myself here to document the resolution of this. The current assessment is that the bandwidth of onion messages required for "proper usage" with BOLT12 is fairly low, and implementations are advised to implement some basic rate limiting to mitigate this. Rate-limiting strategy is left as an exercise to the implementer. |
Yeah, when we have several implementation that support it we can also consider to add a line in the spec for people that will go to implement it in th future |
These use onion encoding for simple one-way messaging: there are no error returns.
Any reply is done with an enclosed blinded path, a-la @t-bast (in fact, this is based on #765)
Note that this defines the message system, not the contents of messages
(e.g. invoice requests from offers).