Skip to content
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

Onion format variation a-la @roasbeef (vs #593) #604

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@rustyrussell
Copy link
Collaborator

commented May 6, 2019

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_id is unnecessary there.

(See #603 which explicitly describes that encoding smaller numbers with fewer than 8 bytes in a TLV is allowed).

cdecker added some commits Mar 28, 2019

bolt04: Formatting cleanup and fold clarifications into conventions
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>
bolt04: Introduce the notion of frames and make payloads variable
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>
bolt04: Update the technical description of the multi-frame proposal
Signed-off-by: Christian Decker <decker.christian@gmail.com>
bolt04: Move the test vectors into JSON documents
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>
bolt04: Update spell check dictionary
Signed-off-by: Christian Decker <decker.christian@gmail.com>

@rustyrussell rustyrussell requested review from cdecker and Roasbeef May 6, 2019

BOLT 4: leave legacy realm byte alone, use values 1-15 for multi-fram…
…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>
@t-bast

This comment has been minimized.

Copy link

commented May 6, 2019

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 realm are used to encode the number of frames instead of the most significant bits (and we commit to using TLV for all those 16 realm values which is what we decided anyway).

We have room for new messages in two different places:

  • in realms 0x01 to 0x0f by leveraging the TLV types that aren't yet used
  • in all the other realm values with complete liberty over the format of the hop payload

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.

@Roasbeef
Copy link
Member

left a 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`.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef May 10, 2019

Member

If the entire byte is available, why restrict it to 15 instead of ~20 which lets nodes utilize all the available space?

@Roasbeef

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@cdecker

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

It has one small downside of having to keep a mapping of realm -> num_frames in the code processing the onion, whereas #593 was making that simply the first few bits, but like mentioned it's a minor thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.