Limit attributable return fields to 32 KiB

[joostjager]

Cap failure return packets and fulfillment payloads at 32 KiB to reserve room for attribution data and future extensions.

Truncate oversized legacy failure packets when forwarding, while treating oversized fulfillment payloads as protocol violations.

[TheBlueMatt]

IMO we should either warn + truncate (cause we want this to not cause channel closure) or require an error + FC (because we want it to be a protocol violation and intend to enforce it). Being wishy-washy is always bad.

[t-bast]

If honest nodes never create a fulfillment_payload larger than 32768 bytes, I think we should immediately force-close when receiving a larger one (our direct peer is obviously malicious).

[t-bast]

I see, the reason for potentially warning + truncate instead of force-close is backwards-compat for the update_fail_htlc case? But I think it's safe to assume that no honest implementation today would ever generate an update_fail_htlc message larger than 32kB?

[t-bast]

On the other hand, implementation-wise, just truncating it simpler 🙃

[joostjager]

If we truncate, I think we should truncate to a fixed length and not make it variable depending on what other fields the directly downstream node populates.

For fulfill, my preference would be error+FC. In my experience, strictness pays off over time.

[t-bast]

Sounds good to me!

[t-bast]

I have one comment that came up, that is rather related to #1344 and requires a breaking change. We're currently using the whole fulfillment payload when creating our attribution HMACs. When using a large fulfillment payload (~32kB), it can be costly to compute those 20 HMACs. We could get rid of this cost by committing to a tagged_hash of the fulfillment payload in the attribution HMACs instead of directly committing to the whole fulfillment_payload. What do you think @joostjager @TheBlueMatt? Can we still introduce a breaking change to fix that or do we accept that this can be slightly abused?

[joostjager]

Good point. I don't know how important it is, but there doesn't seem to be much downside. If we want to be consistent, we should probably also do it for the failure case, which is a little bit more breaking. But ofc attribution data currently only comes into play when the normal failure attribution doesn't work, so I think it's ok to go all the way?

An alternative might be to clone the intermediate hmac state and reuse it for the 20 hmacs? That might actually be cleaner on the protocol level. For the failure case, the ordering is such that it works I believe. Then we don't need to break anything.

[joostjager]

Try out in rust-lightning: https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4909, it seems trivial. I think we don't need the intermediate hmac @t-bast ?

[t-bast]

Try out in rust-lightning: https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4909, it seems trivial. I think we don't need the intermediate hmac @t-bast ?

I don't think this is a good idea: using internal state of a cryptographic protocol is kind of a hack that breaks encapsulation...it depends on the library you're using whether this will be exposed or not, which means that some implementations may not be able to do that easily/safely.

I think we should prioritize having a clean protocol spec: either we decide to use a tagged-hash and break backwards-compat, or we accept that there's a small CPU "DoS" vector and just run with it anyway.

On the eclair side, we don't mind breaking backwards-compat: we currently fill attribution data but don't use it yet.

[joostjager]

I don't think this is a good idea: using internal state of a cryptographic protocol is kind of a hack that breaks encapsulation...it depends on the library you're using whether this will be exposed or not, which means that some implementations may not be able to do that easily/safely.

But does it break encapsulation? Cloning the context via the public API doesn't access cryptographic internals?

[TheBlueMatt]

I'd say cloning an intermediate hash state is a pretty normal thing to do. Adding an intermediate hash isn't crazy either, but it seems more annoying to break it again for that.

[t-bast]

I think I misunderstood the whole point about cloning the hmac state, I thought we somehow needed a spec change in that case as well. But it's just a way to keep the spec like it is today and simply remove the performance issue related to performing 20 HMACs on a large message, so it's just an implementation detail. Sounds good to me then, no need to bother with this!

[t-bast]

ACK 5dd1c2c 👍

[joostjager]

Fix up squashed

[t-bast]

ACK 82e944f