Variable hop payload size for sphinx #2689

cdecker · 2019-05-29T19:48:29Z

This is the competing proposal to #2363. It implements the variable size hop_payloads discussed duriing the last IRC spec meeting.

It is implemented as fixups based on that PR, so you should be able review just the fixup! commits and get a good picture of the differences. I also cleaned up all the common mentions of frames in the final commit, but left it in other places to facilitate reviews.

Closes #2363

jb55

just a few things that caught my eye

jb55 · 2019-06-12T09:36:22Z

common/sphinx.c

+	if (hop->realm == 0x00)
+		return 65;
+
+	if (size < 0xFD)


a comment would be helpful here

jb55 · 2019-06-12T09:36:35Z

common/sphinx.c

+
+	/* The hop must accomodate the hop_payload, as well as the varint
+	 * describing the length and HMAC. */
+	return vsize + size + HMAC_SIZE;;


extra semicolon

jb55 · 2019-06-12T09:41:29Z

common/sphinx.c

@@ -135,10 +137,14 @@ void sphinx_add_v0_hop(struct sphinx_path *path, const struct pubkey *pubkey,
 		       const struct short_channel_id *scid,
 		       struct amount_msat forward, u32 outgoing_cltv)
 {
+	u8 padding[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00,


perhaps this could be const u8 padding[]

jb55 · 2019-06-12T09:47:16Z

common/sphinx.c

 	u8 *buf = tal_arr(path, u8, 0);
 	towire_short_channel_id(&buf, scid);
 	towire_u64(&buf, forward.millisatoshis); /* Raw: low-level serializer */
 	towire_u32(&buf, outgoing_cltv);
+	towire(&buf, padding, ARRAY_SIZE(padding));


is sizeof(padding) instead of ARRAY_SIZE the preferred way to do this with byte-sized elements? not sure if the codebase has a particular style preference here.

ARRAY_SIZE is basically always preferable when you have an array. Unlike sizeof(), it will break compile if the argument is a pointer instead of an array, due to refactoring.

jb55 · 2019-06-12T09:49:29Z

common/sphinx.c

+		pos += varint_put(dest+pos, raw_size);
+
+	memcpy(dest + pos, hop->payload, raw_size);
+	pos += raw_size;


this is incremented but not used afterwards, is that intended?

jb55 · 2019-06-12T09:58:07Z

common/sphinx.c

+
+	memcpy(dest + pos, hop->payload, raw_size);
+	pos += raw_size;
+	memcpy(dest + hop_size - HMAC_SIZE, hop->hmac, HMAC_SIZE);


it's hard to tell what's going on here, but perhaps there could be an assert(pos < dest + hop_size - HMAC_SIZE) if I'm reading this correctly.

Yes, I'm pretty sure this should be "memcpy(dest + pos, hop->hmac, HMAC_SIZE); pos += HMAC_SIZE; assert(pos == hop_size);"

ALso, I don't think we should be memset() dest to 0 at the top: I'd prefer to memset just the expected padding in the legacy path. That way valgrind will catch any bugs if we miss some bytes somehow.

Made more explicit:

memcpy(dest + pos, hop->payload, raw_size); pos += raw_size; padding_size = hop_size - pos - HMAC_SIZE; memset(dest + pos, 0, padding_size); pos += padding_size; memcpy(dest + pos, hop->hmac, HMAC_SIZE);

😉

I still like to see an assert(pos == hop_size); at the end?

rustyrussell

Some good feedback from @jb55, and one nasty change from me re: endianness.

I don't think devtools/onion should do JSON, I think common/test/run-sphinx (or a new file) should.

rustyrussell · 2019-07-18T01:56:45Z

common/sphinx.c

+
+	memcpy(dest + pos, hop->payload, raw_size);
+	pos += raw_size;
+	memcpy(dest + hop_size - HMAC_SIZE, hop->hmac, HMAC_SIZE);


Yes, I'm pretty sure this should be "memcpy(dest + pos, hop->hmac, HMAC_SIZE); pos += HMAC_SIZE; assert(pos == hop_size);"

ALso, I don't think we should be memset() dest to 0 at the top: I'd prefer to memset just the expected padding in the legacy path. That way valgrind will catch any bugs if we miss some bytes somehow.

rustyrussell · 2019-07-18T02:00:28Z

common/sphinx.c

+		/* In addition to the raw payload we need to also shift the
+		 * length encoding itself and the HMAC away. */
+		vsize = varint_get(paddedheader, 3, &shift_size);
+		shift_size += vsize + HMAC_SIZE;


This is wrong on several levels.

I worry what happens if we don't have 3 bytes here, can we be tricked into reading off the end?

This uses bitcoin's varint and we are now using BigSize which is big-endian.

shift_size needs to be sanity checked carefully!

I worry what happens if we don't have 3 bytes here, can we be tricked into reading off the end?

No, we have paddedheader[2*ROUTING_INFO_SIZE];, i.e., 2600 bytes here.

This uses bitcoin's varint and we are now using BigSize which is big-endian.

Yep, fixing up

shift_size needs to be sanity checked carefully!

I went and just abort when shift_size >= ROUTING_INFO_SIZE since that'd shift out all non-filler infos and leave us with garbage at the end anyway :-)

rustyrussell · 2019-07-18T02:01:21Z

lightningd/peer_htlcs.c

+		handle_localpay(hin, hin->cltv_expiry, &hin->payment_hash,
+				hop_data->amt_forward,
+				hop_data->outgoing_cltv);
+/*		if (rs->nextcase == ONION_FORWARD) {
 			struct gossip_resolve *gr =


Erk, please delete don't comment out.

My bad, must've slipped in

rustyrussell · 2019-07-18T02:04:39Z

devtools/onion.c

@@ -1,10 +1,16 @@
+#include <ccan/mem/mem.h>


Erk, I just repurposed onion.c as a generic onion builder, which will clash horribly with this.

My approach with the TLV tests (which are also JSON) was to quote the spec in the test case (so if it's wrong or changes, check-source points it out) then pulls the JSON out of itself. We should generalize that, IMHO.

Yep, ran into quite a bit of trouble rebasing, but it's working fine now :-)

cdecker · 2019-07-18T16:13:00Z

Rebased and fixed up to reflect feedback 😉

jb55 · 2019-07-18T18:26:22Z

fixup ACK on my comments!

rustyrussell

Minor changes only, I'm eager to apply this!

rustyrussell · 2019-07-19T04:15:58Z

common/sphinx.c

+
+	memcpy(dest + pos, hop->payload, raw_size);
+	pos += raw_size;
+	memcpy(dest + hop_size - HMAC_SIZE, hop->hmac, HMAC_SIZE);


I still like to see an assert(pos == hop_size); at the end?

rustyrussell · 2019-07-19T04:17:26Z

bitcoin/varint.h

+size_t bigsize_put(u8 buf[VARINT_MAX_LEN], varint_t v);
+
+/* Big-endian variant of varint_get, used in lightning */
+size_t bigsize_get(const u8 *p, size_t max, varint_t *val);


These routines don't belong under bitcoin/, and since the last merge they're now redundant: we now have fromwire_bigsize() and towire_bigsize().

Agreed that they don't belong here, but they are useful since fromwire_* / towire_* require tal_arr which they can extend, whereas the sphinx operations require in-place operations. I tried using the fromwire and towire variants, but it turned out to involve way more work than just implementing it again.

I can move the functions in a later cleanup PR.

rustyrussell · 2019-07-19T04:18:22Z

common/sphinx.c

+	if (hop->realm != 0x00)
+		return false;
+#endif
+
 	memset(dest, 0, hop_size);


Still need to delete this...

It get's remove in a later commit :-)

rustyrussell · 2019-07-19T04:20:57Z

devtools/onion.c

-		if (secp256k1_ec_pubkey_create(secp256k1_ctx, &path[i].pubkey,
-					       privkeys[i]) != 1)
-			errx(1, "Could not decode pubkey");
+		assert(klen == 2 * PUBKEY_LEN || klen == 2 * PRIVKEY_LEN);


Remove this assert, since you give a proper message below?

rustyrussell · 2019-07-19T04:22:56Z

devtools/onion.c

-			errx(1, "Could not decode pubkey");
+		assert(klen == 2 * PUBKEY_LEN || klen == 2 * PRIVKEY_LEN);
+		if (klen == 2 * PRIVKEY_LEN) {
+			if (!hex_decode(argv[1 + i], klen, rawprivkey, PRIVKEY_LEN))


I would combine these two, for simplicity. hex_decode() will fail if the wrong length, and it avoids magic 2 *. The other option would be if (hex_data_size(klen) == PRIVKEY_LEN) but it still seems redundant?

Switched to use hex_data_size and pubkey_from_hexstr to do the conversion in one go. Would a counterpart for privkeys be desirable as well (privkey_from_hexstr)?

Hmm, hex_decode does this test internally, so it's redundant. But not harmful, and I really want this merged :)

rustyrussell · 2019-07-19T04:23:12Z

devtools/onion.c

+			fprintf(stderr,
+				"Provided key is neither a pubkey nor a "
+				"privkey: %s\n",
+				argv[1 + i]);


rustyrussell · 2019-07-19T04:24:45Z

devtools/onion.c

@@ -12,32 +13,55 @@
 #include <unistd.h>

 #define ASSOC_DATA_SIZE 32
+#define PUBKEY_LEN 33
+#define PRIVKEY_LEN 32


PUBKEY_CMPR_LEN from bitcoin/pubkey.h? Hmm we don't have a convenient PRIVKEY_LEN though...

If you're bored you could add one to bitcoin/privkey.h :)

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>

cdecker · 2019-07-26T14:48:07Z

Clean rebase to get code back on top of master, now doing a cleanup sweep to address @rustyrussell's feedback.

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 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>

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>

The realm has lost significance, so let's unify this into the type. Signed-off-by: Christian Decker <decker.christian@gmail.com>

See lightning/bolts#619 and lightning/bolts#619 for discussion. Signed-off-by: Christian Decker <decker.christian@gmail.com>

@rustyrussell

Simplifying some operations, erroring in some cases and moving to global defines for constants. Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <decker.christian@gmail.com>

cdecker · 2019-07-26T16:46:43Z

@rustyrussell I added the last commit to address your feedback. I think the last open question is the use of bigsize_put and bigsize_get which need to move into common/ and whether we want to keep them at all, or whether we prefer going the tal_arr + towire_bigsize + memcpy + tal_free route. Alternatively we could have towire_bigsize and fromwire_bigsize use them internally, but I'm not sure how much logic is then left to the wrapper.

rustyrussell

Ack ddf7da1

rustyrussell · 2019-07-30T01:02:48Z

devtools/onion.c

-			errx(1, "Could not decode pubkey");
+		assert(klen == 2 * PUBKEY_LEN || klen == 2 * PRIVKEY_LEN);
+		if (klen == 2 * PRIVKEY_LEN) {
+			if (!hex_decode(argv[1 + i], klen, rawprivkey, PRIVKEY_LEN))


Hmm, hex_decode does this test internally, so it's redundant. But not harmful, and I really want this merged :)

cdecker added the Work in Progress label May 29, 2019

cdecker requested a review from rustyrussell May 29, 2019 19:48

This was referenced May 29, 2019

Multi-frame onion format proposal #2363

Closed

plugin: Implement the htlc_accepted hook #2267

Merged

jb55 reviewed Jun 12, 2019

View reviewed changes

cdecker force-pushed the var-hop-payload branch from 4b731ea to 02ecf8a Compare June 29, 2019 14:44

cdecker force-pushed the var-hop-payload branch from 02ecf8a to 36e0ea0 Compare July 15, 2019 11:49

rustyrussell requested changes Jul 18, 2019

View reviewed changes

cdecker force-pushed the var-hop-payload branch from 998e117 to 8964f65 Compare July 18, 2019 14:46

rustyrussell reviewed Jul 19, 2019

View reviewed changes

cdecker added 2 commits July 26, 2019 16:44

sphinx: Consolidate some of the hard-coded parameter naming

a640b9a

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>

cdecker force-pushed the var-hop-payload branch from 8964f65 to b2d0490 Compare July 26, 2019 14:47

cdecker added 11 commits July 26, 2019 18:33

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

dc322ec

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

5a36cb6

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

e592266

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

sphinx: Simplify the filler generation code

ce953cf

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

5799bb7

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

7a304c7

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

487c44d

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

sphinx: Cleanup sphinx onion construction, remove realm

3ae9471

The realm has lost significance, so let's unify this into the type. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sphinx: Switch to big-endian number encoding

eed19e6

See lightning/bolts#619 and lightning/bolts#619 for discussion. Signed-off-by: Christian Decker <decker.christian@gmail.com>

devtools: Minor cleanup of the onion command line tool

ddf7da1

Simplifying some operations, erroring in some cases and moving to global defines for constants. Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <decker.christian@gmail.com>

cdecker force-pushed the var-hop-payload branch from b2d0490 to ddf7da1 Compare July 26, 2019 16:40

rustyrussell approved these changes Jul 30, 2019

View reviewed changes

rustyrussell merged commit 581694f into ElementsProject:master Jul 30, 2019

Variable hop payload size for sphinx #2689

Variable hop payload size for sphinx #2689

Conversation

cdecker commented May 29, 2019

jb55 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdecker Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdecker commented Jul 18, 2019

jb55 commented Jul 18, 2019

rustyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdecker commented Jul 26, 2019

cdecker commented Jul 26, 2019

rustyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdecker Jul 18, 2019 •

edited

Loading

rustyrussell Jul 19, 2019 •

edited

Loading