Clarify fulfillment payload padding requirement

[joostjager]

Require a padding record only when the serialized TLV stream does not already meet the 256-byte bucket requirement.

[t-bast]

I thought you were also talking about the padding done for failure messages, since your comment didn't include an exact link to the spec. We have weird repeated requirements where the first one says MUST and the second one says SHOULD here: https://github.com/lightning/bolts/blob/master/04-onion-routing.md#requirements-4

That's why I suggested to open a follow-up PR, I didn't understand that your comment was explicitly scoped to fulfillment payload. Probably worth taking a look at every place where we pad in a similar way?

[joostjager]

I think the MUST+SHOULD is a result of lnd not able to parse 256+ byte failure reasons in the past. Maybe we can just drop that now?

But yes, my comment was scoped just to the fulfill payload. It's not that the spec is wrong as is now, but it also isn't completely clear the padding tlv can be omitted.

[t-bast]

But yes, my comment was scoped just to the fulfill payload.

Sorry then, I was too trigger-happy when I merged that PR, I didn't understand what you meant!

I think the MUST+SHOULD is a result of lnd not able to parse 256+ byte failure reasons in the past. Maybe we can just drop that now?

Yes I think that since we're doing a dedicated PR and it has been quite some time, it's a good opportunity to clean that up now.

[joostjager]

Updated

[joostjager]

One unfortunate thing about padding to 256 bytes using only TLV type 1 (padding) is that it is impossible for otherwise-empty fulfillment payloads. The BigSize length transition skips from a 254-byte record to a 257-byte record, so the next valid bucket is 512 bytes, or 528 bytes including the Poly1305 tag 😞

[joostjager]

Not sure if I am still all that happy with the in-tlv padding

[t-bast]

One unfortunate thing about padding to 256 bytes using only TLV type 1 (padding) is that it is impossible for otherwise-empty fulfillment payloads.

Why do you think a present but empty padding TLV makes sense (if I understand your scenario correctly - please be more precise if I'm misunderstanding)? Can you detail that scenario? It looks like an edge case that we simply will never reach, doesn't it?

[t-bast]

Do we want to take this opportunity to use multiples of 256 bytes here as well for consistency?

[joostjager]

It can't be a MUST. I could re-add a new SHOULD? 😅

In this touch-up PR I wouldn't want to change anything really, just clarify.

[t-bast]

It is on the sender requirements, so it can definitely be a MUST as long as we don't change the receiver requirements yet? But we can indeed skip it, maybe worth checking with lnd and cln whether they think it makes sense to do.

[joostjager]

Why do you think a present but empty padding TLV makes sense (if I understand your scenario correctly - please be more precise if I'm misunderstanding)? Can you detail that scenario? It looks like an edge case that we simply will never reach, doesn't it?

I was thinking of an otherwise-empty payload as cover traffic, since omitting fulfillment_payload may reveal to upstream nodes whether the recipient returned application data.

[t-bast]

I was thinking of an otherwise-empty payload as cover traffic, since omitting fulfillment_payload may reveal to upstream nodes whether the recipient returned application data.

Can't you just include an all-0 fulfillment payload then instead? Or any other unrelated "cover traffic" TLV if necessary?

[joostjager]

Can't you just include an all-0 fulfillment payload then instead? Or any other unrelated "cover traffic" TLV if necessary?

A literal all-zero fulfillment_payload would not carry a valid Poly1305 tag and would also be recognizable as cover traffic by the upstream node?

The 256-byte issue is specific to using a one-byte-encoded padding type such as type 1. The record size is 1 + BigSize(length) + length: it jumps from 254 bytes with a 252-byte value to 257 bytes with a 253-byte value because the length encoding expands from one to three bytes. Had we chosen a padding type of at least 253, its three-byte type encoding plus a one-byte length and 252-byte value could produce exactly 256 bytes.

We could define a separate cover TLV, but having both padding and cover fields in the specification seems redundant?

[Roasbeef]

Can we add some test vectors here?

[joostjager]

Added test vectors. They show the limitation of type-1 TLV padding pretty clearly: some 256-byte boundaries can't be reached. Not sure whether we still want to change anything about that or just roll with what we have.