Onion format variation a-la @roasbeef (vs #593) #604
Conversation
The clarifications were tacked on after the fact, but they should really be part of the conventions. I also updated the links to use the reference style, which results in better text flow and makes it easier to read the source. Signed-off-by: Christian Decker <decker.christian@gmail.com>
Introduces `frame`s and all related concepts such as `FRAME_SIZE`. It also fixes a conceptual issue where `hops_data` would be used both for the section of the overall packet as well as the wire format for individual payloads. This separation now lends itself better to the incremental wrapping of the onion. Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
They were cluttering the spec document, and not easily integrated into the tests for implementations. This just cleans up the spec and allows automated testing going forward. Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
…e / TLV. The TLV format leaves us with more room for the final node, which is nice as that's where most proposals want to put more data in the onion. It's break-even in for the non-final nodes, since amount_to_forward tends to be < 6 bytes, and outgoing_cltv_value < 2. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
|
I'm fine with this small tweak. If I'm not missing anything it achieves the same tradeoffs as @cdecker's proposal, in fact the only change is that the least significant bits of the We have room for new messages in two different places:
I think that this is clearly enough for the foreseeable future (and it achieves both backwards-compatibility and simplicity, which is very important to get it deployed smoothly). In the longer term, we know we want to change a few things in a version 1 of the onion message (per-hop versioning for example). Version 1 can be an opportunity to provide even more room for new messages if developers can't achieve what they want with version 0. All those reasons make this a good pragmatic proposal which could unblock trampoline/rendezvous routing and I think we should move forward. If we all agree with that, we only need test vectors for the new realm values and the feature bit and we should be good to go. |
There was a problem hiding this comment.
This addresses my objection to bit-packing the packet type + number of frames into the realm byte, but it doesn't (to my current reading) address the small number of possible types for TLV in the onion.
I think a much smaller modification to the prior proposal is possible compared to the iteration in this PR. As is, it completely re-defines the current hop data payloads which results in additional code impact for all nodes involved in a route. I think completely re-defining the hop payload is an interesting idea that demonstrates the flexibility of TLV in the onion packets in general, but one that seems tangential to allowing nodes to add structure to the currently unused extra blob data.
| The number of additional frames allocated to the current hop is determined by the 4 most significant bits of `num_frames_and_realm`, while the 4 least significant bits determine the payload format. | ||
| Therefore the number of frames allocated to the current hop is given by `num_frames = (num_frames_and_realm >> 4) + 1`. | ||
| For simplification we will use `hop_payload_len` to refer to `num_frames * FRAME_SIZE`. | ||
| The `hop_payload` consists of at least one `frame` followed by up to 15 additional `frame`s. For simplification we will use `hop_payload_len` to refer to `num_frames * FRAME_SIZE`. |
There was a problem hiding this comment.
If the entire byte is available, why restrict it to 15 instead of ~20 which lets nodes utilize all the available space?
|
Thinking about this more, I really like the signaling unification and also
clean slate for the payload for each hop. In comparison, the other proposal
made the payloads into a hybrid of the old and new, while with this,
payloads are just opaque blobs to be passed up (which lets us do things
like shave the short id for the final hop) . Also to my reading, the prior
proposal left the current 13 padding bytes unutilized, while with this
there's no longer any distinction.
…On Sun, May 5, 2019, 11:34 PM Rusty Russell ***@***.***> wrote:
This splits the difference between #593
<#593> from @cdecker
<https://github.com/cdecker> and comments from @Roasbeef
<https://github.com/Roasbeef>
I based it on #593
<#593> so the
differences are clear. It leaves realm 0 alone (naming it legacy), just
defines realms 1-15.
These are TLVs, which in is a win for the final node since
short_channel_id is unnecessary there.
(See #603 <#603>
which explicitly describes that encoding smaller numbers with fewer than 8
bytes in a TLV is allowed).
------------------------------
You can view, comment on, or merge this pull request online at:
#604
Commit Summary
- bolt04: Formatting cleanup and fold clarifications into conventions
- bolt04: Introduce the notion of frames and make payloads variable
- bolt04: Update the technical description of the multi-frame proposal
- bolt04: Move the test vectors into JSON documents
- bolt04: Update spell check dictionary
- BOLT 4: leave legacy realm byte alone, use values 1-15 for
multi-frame / TLV.
File Changes
- *M* .aspell.en.pws
<https://github.com/lightningnetwork/lightning-rfc/pull/604/files#diff-0>
(17)
- *M* 04-onion-routing.md
<https://github.com/lightningnetwork/lightning-rfc/pull/604/files#diff-1>
(547)
- *A* bolt04/onion-error-test.json
<https://github.com/lightningnetwork/lightning-rfc/pull/604/files#diff-2>
(43)
- *A* bolt04/onion-test-v0.json
<https://github.com/lightningnetwork/lightning-rfc/pull/604/files#diff-3>
(62)
Patch Links:
- https://github.com/lightningnetwork/lightning-rfc/pull/604.patch
- https://github.com/lightningnetwork/lightning-rfc/pull/604.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#604>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AAHTWLVMIEVKX7ICP6JEWLLPT7GQ5ANCNFSM4HK5OYQQ>
.
|
|
It has one small downside of having to keep a mapping of |
|
Closed in favor of revisions to #593 |
| values also allows use of more than one frame. | ||
|
|
||
| 1. tlv: `tlv_hop_data` | ||
| 2. types: |
There was a problem hiding this comment.
Rather than unroll these fields and take up 4 types, we can instead have them all packed under a single type. All the fields are fixed length, so there's no additional signalling overhead and we also save a few bytes as well. When allocating space in the TLV namespace we should prefer to pack several types into one, if all the types are related to a discrete use case. This model promotes more efficient utilization of the payload space.
There was a problem hiding this comment.
Well like we discussed in the IRC meeting some of the types would be omitted, for example in the last hop_payload. So while I agree that we may avoid defining types that must always be used in combination, we should also keep in mind that swapping out or omitting individual types is also an important feature here.
|
Closed in favor of PR #619 as discussed during the IRC meeting on 2019/06/10. |
This splits the difference between #593 from @cdecker and comments from @Roasbeef
I based it on #593 so the differences are clear. It leaves realm 0 alone (naming it legacy), just defines realms 1-15.
These are TLVs, which in is a win for the final node since
short_channel_idis unnecessary there.(See #603 which explicitly describes that encoding smaller numbers with fewer than 8 bytes in a TLV is allowed).