Multi-frame onion format proposal #2363

cdecker · 2019-02-18T18:29:36Z

Quoting the description from lightningnetwork/lightning-onion#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.

Closes #2689

cdecker · 2019-02-19T20:54:38Z

Unit tests don't need this exposed though?

Depends on whether devtools/onion is considered a unit test (and should therefore #include <sphinx.c>) or it is a user-facing tool (that should use the exposed API in sphinx.h). I'm on the fence for this one, since we need the ability to set the session_key for reproduceability.

EDIT: If we include sphinx.c we don't even need the with_key constructor since we can just override the value in the struct as well.

rustyrussell · 2019-04-08T03:24:22Z

I want to apply this rework for an independent PR, so I'm appending the code to make the multi-hop parsing EXPERIMENTAL only so we can merge...

cdecker · 2019-04-08T19:54:44Z

common/sphinx.c

@@ -89,10 +89,14 @@ struct sphinx_path *sphinx_path_new_with_key(const tal_t *ctx,

 static size_t sphinx_hop_count_frames(const struct sphinx_hop *hop)
 {
+#if EXPERIMENTAL_FEATURES


This is likely redundant with the check on line 426, since this just clamps the number of frames to 1, never triggering the failure, and probably corrupting the frame if we end up using more anyway. I suggest we just keep the check below, and restore the normal behavior.

cdecker · 2019-05-10T08:39:30Z

During my rebase I accidentally removed @rustyrussell's experimental guards. Rebased and cherry-picked it back onto the PR :-)

For the multi-frame support we need to introduce the FRAME_SIZE parameter and I took the opportunity to fix up some of the naming. Signed-off-by: Christian Decker <decker.christian@gmail.com>

`struct sphinx_path` serves as a container for all the routing related information, with a couple of constructors that can be used for normal operation or testing (with pre-determined `session_key`). Signed-off-by: Christian Decker <decker.christian@gmail.com>

This is just taking the existing serialization code and repackaging it in a more useful form. Signed-off-by: Christian Decker <decker.christian@gmail.com>

This was a mismatch between the go tool and this test tool so far. Just aligning the tools to allows for easier testing. Signed-off-by: Christian Decker <decker.christian@gmail.com>

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

Just some reorganizations and clarifications before we add the multi-frame support. Signed-off-by: Christian Decker <decker.christian@gmail.com>

Shouldn't be used directly, but really useful for testing, since we can just cram a huge payload in without having to be valid. And we don't have a TLV spec yet. Signed-off-by: Christian Decker <decker.christian@gmail.com>

This is the actual change when constructing the multi-frame onion. It changes the rightshift, and how much filler we need to generate. It also has the realm stuffing with the number of additional frames. Signed-off-by: Christian Decker <decker.christian@gmail.com>

This is all it takes on the read side to use multiple frames. We are overshooting the padding a bit since we can at most use 16 additional frames, but ChaCha20 is cheap. Signed-off-by: Christian Decker <decker.christian@gmail.com>

This still only supports v0 payloads, but it is flexible enough to accomodate future payload formats as well, with the type-tag and the union holding the actual parsed data. It also better encapsulates the decoding of the former realm byte into payload type and frame count. Signed-off-by: Christian Decker <decker.christian@gmail.com>

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

The `runtest` command takes a JSON onion spec, creates the onion and decodes it with the provided private keys. It is fully configurable and can be used for the test-vectors in the spec. Signed-off-by: Christian Decker <decker.christian@gmail.com>

…FEATURES. This lets us merge it, since it has a nice refactor I would like to use for invoice hooks (raw payload access). Also checks that we don't give a payload too large to encode! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2019-05-23T10:25:44Z

Updated the PR to reflect the change in realm byte split. It now uses a 5bit + 3bit split like the recent change to lightning/bolts#593. Once that is merged we can go ahead as well.

rustyrussell

This is a really nice cleanup! Only one weird useless loop which needs deleting.

I've suggested a few tweaks, but really they can be done as add-on PRs if you prefer, so consider it acked once that loop in pay.c is removed!

rustyrussell · 2019-05-27T02:00:19Z

common/sphinx.c

@@ -166,7 +165,7 @@ static bool generate_header_padding(
 	)
 {
 	int i;
-	u8 cipher_stream[(NUM_MAX_HOPS + 1) * hopsize];
+	u8 cipher_stream[(NUM_MAX_FRAMES + 1) * FRAME_SIZE];


Either this change is wrong, or hopsize should not be a parameter to this function?

rustyrussell · 2019-05-27T02:02:01Z

common/sphinx.h

+#define FRAME_SIZE 65
+#define NUM_MAX_FRAMES 20
+#define ROUTING_INFO_SIZE (FRAME_SIZE * NUM_MAX_FRAMES)
+#define TOTAL_PACKET_SIZE (VERSION_SIZE + PUBKEY_SIZE + HMAC_SIZE + ROUTING_INFO_SIZE)


A properly marked /* BOLT #4 quote here would be illiminating and useful :)

rustyrussell · 2019-05-27T02:05:31Z

common/sphinx.c

+	return sp;
+}
+
+struct sphinx_path *sphinx_path_new_with_key(const tal_t *ctx,


Maybe wrap this declaratio (and defn) in #if DEVELOPER to emphasize it? Means devtools/onion is now DEVELOPER-only, but that's probably a Good Thing?

rustyrussell · 2019-05-27T02:08:35Z

common/sphinx.c

+		dest[0] = hop->realm;
+		memcpy(dest + 1, hop->payload, tal_bytelen(hop->payload));
+		memcpy(dest + FRAME_SIZE - HMAC_SIZE, hop->hmac, HMAC_SIZE);
+}


Weird indent?

Weird indent fixed :)

rustyrussell · 2019-05-27T02:13:14Z

lightningd/pay.c

-							   struct node_id,
-							   &ids[i]));
-		}
-	}


Removing this sanity check makes me uncomfortable. But why don't we simply make ids a struct pubkey array directly, and do the pubkey_from_node_id(&ids[i], &route[i].nodeid)) here in the loop and then fail immediately like this did?

rustyrussell · 2019-05-27T02:16:45Z

common/sphinx.h

+		       const struct short_channel_id *scid, struct amount_msat forward,
+		       u32 outgoing_cltv);
+/**
+ * Add a raw payload hop to the path.


Maybe note it's expose for testing...

rustyrussell · 2019-05-27T02:19:38Z

common/sphinx.c

-		memcpy(dest + FRAME_SIZE - HMAC_SIZE, hop->hmac, HMAC_SIZE);
+	u8 num_frames = sphinx_hop_count_frames(hop);
+	memset(dest, 0, num_frames * FRAME_SIZE);
+	dest[0] = hop->realm | (num_frames-1) << 4;


| binds less than << but I still want more brackets here please :) Also, this is now 3 is it not?

Maybe a heap of assertions here, like assert(hop->realm < (1 << 4) and assert(tal_bytelen(hop->payload) < sphinx_hop_count_frames(hop) * FRAME_SIZE) - HMAC_SIZE - 1)?

rustyrussell · 2019-05-27T02:23:44Z

common/sphinx.h

+
+enum sphinx_payload_type {
+	SPHINX_V0_PAYLOAD = 0,
+	SPHINX_RAW_PAYLOAD = 255,


Whenever we overload values for internal usage, I always worry about leakage (someone sends us a realm 255 onion in future).

Let's make route_step.realm and int, and use negative values for internal ones maybe? Or 256?

rustyrussell · 2019-05-27T02:25:59Z

common/test/onion-test-v0.json

@@ -0,0 +1,42 @@
+{


Would prefer to see this tested in a unit test rather than a devtool, though.

rustyrussell · 2019-05-27T02:26:31Z

common/sphinx.c

@@ -89,10 +89,14 @@ struct sphinx_path *sphinx_path_new_with_key(const tal_t *ctx,

 static size_t sphinx_hop_count_frames(const struct sphinx_hop *hop)
 {
+#if EXPERIMENTAL_FEATURES


cdecker · 2019-05-29T19:49:23Z

Re-added the Work in Progress tag since we need an upstream decision about which proposal should be merged.

rustyrussell · 2019-06-11T00:10:25Z

Looks like this should be closed in favor of the other PR; removed milestone tag anyway.

cdecker added Work in Progress routing labels Feb 18, 2019

cdecker self-assigned this Feb 18, 2019

cdecker force-pushed the multi-frame branch 2 times, most recently from aac29da to ad4aa42 Compare February 18, 2019 22:57

cdecker mentioned this pull request Mar 4, 2019

Frame-based onion payload format and construction for larger per-hop payloads lightning/bolts#586

Closed

cdecker force-pushed the multi-frame branch from 7d5b7b3 to 627cb49 Compare March 20, 2019 22:25

cdecker mentioned this pull request Mar 29, 2019

bolt04: Multi-frame sphinx onion routing proposal lightning/bolts#593

Closed

cdecker commented Apr 8, 2019

View reviewed changes

cdecker force-pushed the multi-frame branch 2 times, most recently from 77f361c to 7fc3e2c Compare May 10, 2019 08:38

cdecker and others added 14 commits May 21, 2019 21:44

sphinx: Consolidate some of the hard-coded parameter naming

3f7990d

For the multi-frame support we need to introduce the FRAME_SIZE parameter and I took the opportunity to fix up some of the naming. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sphinx: Add function to add a new v0 hop to a sphinx_path

4240cad

This is just taking the existing serialization code and repackaging it in a more useful form. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sphinx: Fix the onion cli tool to take public keys

ad636d1

This was a mismatch between the go tool and this test tool so far. Just aligning the tools to allows for easier testing. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sphinx: Clean up after migrating to the sphinx_path struct

2546a30

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

sphinx: Simplify the filler generation code

abcf9a0

Just some reorganizations and clarifications before we add the multi-frame support. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sphinx: Expose sphinx_add_raw_hop for testing

62f63d2

Shouldn't be used directly, but really useful for testing, since we can just cram a huge payload in without having to be valid. And we don't have a TLV spec yet. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sphinx: Variable left-shift when unwrapping onion

acccab5

This is all it takes on the read side to use multiple frames. We are overshooting the padding a bit since we can at most use 16 additional frames, but ChaCha20 is cheap. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sphinx: Remove standalone v0 payload in favor of the unionized one

38d29de

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

fixup! sphinx: Add differentiation between payload versions

6c83600

cdecker force-pushed the multi-frame branch from 7fc3e2c to 6c83600 Compare May 21, 2019 19:47

cdecker removed the Work in Progress label May 23, 2019

cdecker added this to the 0.7.1 milestone May 23, 2019

rustyrussell approved these changes May 27, 2019

View reviewed changes

cdecker mentioned this pull request May 29, 2019

Variable hop payload size for sphinx #2689

Merged

cdecker added the Work in Progress label May 29, 2019

rustyrussell removed this from the 0.7.1 milestone Jun 11, 2019

cdecker closed this Jun 11, 2019

Multi-frame onion format proposal #2363

Multi-frame onion format proposal #2363

Uh oh!

Conversation

cdecker commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdecker commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustyrussell commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdecker commented May 10, 2019

Uh oh!

cdecker commented May 23, 2019

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdecker commented May 29, 2019

Uh oh!

rustyrussell commented Jun 11, 2019

Uh oh!

Uh oh!

cdecker commented Feb 18, 2019 •

edited

Loading

cdecker commented Feb 19, 2019 •

edited

Loading

rustyrussell commented Apr 8, 2019 •

edited

Loading