search

lightningnetwork / lightning-onion

Onion Routed Micropayments for the Lightning Network
MIT License
396 stars 125 forks source link

crypto: relax failure message length check #59

Closed joostjager closed 1 year ago

joostjager commented 1 year ago

The spec does not dictate but only recommends a length of 256 bytes. Future tlv extensions may push the failure message length over this limit. With this change, receivers can ignore the lengthier extensions without handling it as an unreadable failure.

halseth commented 1 year ago

With his change we'll only check that the length is above some minimum.

Is that independent to the spec change proposed here: https://github.com/lightning/bolts/pull/1021 ? As that's only about what the sender should do. (so we can't enforce that the read error is exactly 1024 bytes)

joostjager commented 1 year ago

It is independent. We should already have allowed variable length failure messages.

joostjager commented 1 year ago

Wondering now, should the minimum check be removed too? If we'd receive a shorter failure message today, wouldn't we want to locate the error source and parse the message?

On the other hand, failure messages shorter than 256 bytes may complicate the implementation in lnd: https://github.com/lightningnetwork/lnd/pull/6913#discussion_r995654979

halseth commented 1 year ago

Wondering now, should the minimum check be removed too? If we'd receive a shorter failure message today, wouldn't we want to locate the error source and parse the message?

I would say we should keep it for now. It would be valuable to check what other implementations are doing; if they all send (at least) 256 bytes, we could make this required in the spec.

joostjager commented 1 year ago

Updated spec: https://github.com/lightning/bolts/pull/1021#discussion_r1005526225