Skip to content

WIP: Multi-frame format#33

Closed
cdecker wants to merge 9 commits intolightningnetwork:masterfrom
cdecker:multi-frame
Closed

WIP: Multi-frame format#33
cdecker wants to merge 9 commits intolightningnetwork:masterfrom
cdecker:multi-frame

Conversation

@cdecker
Copy link
Contributor

@cdecker cdecker commented Feb 18, 2019

Quoting the description from #31:

  • Use the 4 MSB in the first realm byte as a counter of how many 65 byte payload-chunks are destined for the processing node. This clearly distinguishes them from the current payload and directly gives an indication on how much it has to read/allocate.
  • Separate payload parsing from the payload extraction, i.e., the onion processing code just hands back a slice of bytes and a realm that are then passed to external parsers that don't interact with the onion processing at all. Mirroring this, when creating a new onion we just get passed the 4 LSB of the realm, and an opaque slices of bytes that may or may not be TLV encoded, we don't care.
  • Given the payload length we can infer how many hops we need to read/write. The format looks like this:

For a payload of length <= 32:

|-------+-------------------+------|
| realm | payload + padding | HMAC |
|-------+-------------------+------|

For payloads > 32 and <= 162 for example:

|-------+--------------+-----------------+-------------------------+------|
| realm | payload[:64] | payload[64:129] | payload[129:] + padding | HMAC |
|-------+--------------+-----------------+-------------------------+------|

In other words we use the realm byte from the first hop to determine the number of hops to read, the first hop still has room for 64 bytes of payload (per-hop-payload + HMAC size - realm byte). Any intermediate hop has the full 65 bytes available. The last hop has 33 bytes of payload available, since we will use its HMAC to pass it on to the next node in the path. Notice that we do not decrypt the payloads after the first since processing the first hop already decrypted all the following hops, including the ones we'll be processing. In addition we get a better use of the limited space that we have available and the entire payload is contiguous in memory and can be passed out to the parser as is, without having to stitch it together.

The implementation is also rather trivial, all we need to do is to pass the payload as a byte slice out during processing, and to get the next onion instead of shifting by 65 bytes and padding with 0s, we shift by 65*nhops and pad that with 0s. So the only thing we really need to do is to have the rightShift, headerWithPadding, and copy not take 65, but 65*nhops as arguments.

Roasbeef and others added 9 commits February 4, 2019 14:20
We'll be encoding to and decoding from a variable length byte-slice
once we complete the multi-hop proposal.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
…frames

Since we want to use multiple former per-hop-payloads as a single
payload, we want to differentiate the notion of a frame from a
payload, since the latter can be now be made up of multiple frames.

Signed-off-by: Christian Decker <@cdecker>
This struct now contains all the information necessary.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
HopPayload represents the raw, unparsed payload, from which the
HopData can be parsed. The sphinx code internally only handles
HopPayload, and the `HopData()` method can be used to parse and return
the HopData. This completes the transition to HopPayload.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is the central commit in this whole series, it modifies the
right- and left- shifts to account for multi-frame payloads and it
modifies the filler generation code to generate multi-frame fillers if
the payload requires it.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is a simple test that create a 5 hop onion, and the third hop
gets a payload that spans 2 frames.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
This should make testing a lot easier, since we can just have a bunch of these
sitting in the repo and test them.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
@cdecker
Copy link
Contributor Author

cdecker commented Feb 18, 2019

Please consider this PR still work in progress, since I'm still working on the command-line tool so we can create test-vectors for the spec.

@cdecker cdecker changed the title Multi-frame format WIP: Multi-frame format Feb 18, 2019
@rustyrussell
Copy link

Would you believe that the only thing I don't like is the you used the 4 MSB not 4 LSB of byte-formerly-known-as-realm? :)

@cdecker
Copy link
Contributor Author

cdecker commented Feb 19, 2019

Would you believe that the only thing I don't like is the you used the 4 MSB not 4 LSB of byte-formerly-known-as-realm? :)

How do you mean? Currently the format is rather nice when encoding the former-realm as hex: 41 means that we use 4 additional frames to encode a payload of type 1 :-)

}

func (hp *HopPayload) HopData() (*HopData, error) {
if hp.Realm[0] != 0x00 {
Copy link
Member

@Roasbeef Roasbeef May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this in place, how does a node extract the original hop payload if it's provided with additional frames?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up modifying to make this check against the "true" realm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that check should have only checked the lower 4 bits, which according to the proposal are the realm.

if _, err := w.Write(hd.ExtraBytes[:]); err != nil {
return err
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the ExtraBytes also be omitted? Otherwise it's 13 wasted bytes that can't be used by the greater payload.

@Roasbeef
Copy link
Member

Closing in favor of #36.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants