Bolt 1: Specify that extensions to existing messages must use TLV by t-bast · Pull Request #754 · lightning/bolts

t-bast · 2020-03-02T08:31:29Z

The spec already prepared a hook to add additional information to existing
messages (additional bytes at the end of a message must be ignored).

Since we're using TLV in many places, it makes sense to use that optional
additional space at the end of each message to allow an optional tlv stream.

This requires making a few previously optional fields mandatory:

channel_reestablish commitment points: it makes sense to always include those
regardless of whether option_dataloss_protect or option_static_remotekey are set.
option_upfront_shutdown_script: if you're not using one, just set the length to 0.
That field is moved to a TLV record because luckily, the resulting bytes are the same.
This provides more flexibility to later remove the requirement of making this field mandatory.

No need to change the channel_update's htlc_maximum_msat because
the message_flags encode its presence/absence.
It can still be either included or omitted without causing issues to the extension stream.

NB: this is a continuation of #714, which was merged too soon then reverted.

The spec already prepared a hook to add additional information to existing messages (additional bytes at the end of a message must be ignored). Since we're using TLV in many places, it makes sense to use that optional additional space at the end of each message to allow an optional tlv stream. This requires making a few previously optional fields mandatory: - channel_reestablish commitment points: it makes sense to always include those regardless of whether `option_dataloss_protect` or `option_static_remotekey` are set. - option_upfront_shutdown_script: if you're not using one, just set the length to 0. That field is moved to a TLV record because luckily, the resulting bytes are the same. This provides more flexibility to later remove the requirement of making this field mandatory. No need to change the `channel_update`'s `htlc_maximum_msat` because the `message_flags` encode its presence/absence. It can still be either included or omitted without causing issues to the extension stream.

t-bast · 2020-03-02T09:36:42Z

Here are more details on why moving upfront_shutdown_script to a TLV field is fully backwards-compatible to help reviewers.

Previously we used 2 bytes for the length. Fortunately a TLV for which the data part has less than 252 bytes also uses 2 bytes for tag+length, which matches the previous encoding. Since valid scripts are always less than 252 bytes, that means they result in the exact same encoding. Since TLV streams are ordered, having upfront_shutdown_script use the tag 0x00 ensures this will always be the first field encoded, so legacy nodes will always be able to correctly decode it.

1. Empty script

Old encoding: length
               0000

TLV encoding: tag   length
               00     00

2. Non-empty script

Old encoding: length               script
               0014 0123456789012345678901234567890123456789

TLV encoding: tag  length                   script
               00    14      0123456789012345678901234567890123456789

It's also more future-proof: we won't expect all nodes to use upfront_shutdown_script. Moving that field into a TLV allows us to remove the requirement to always include it in the future when more nodes have upgraded. Otherwise we're stuck with that field in the message forever.

Roasbeef

Very clever work around ;)

I've implemented this for lnd at: lightningnetwork/lnd#3966. It was pretty straight forward as expected, no major surprises. Once that lands we'll also be ready for the TLV revolution 😈

Roasbeef · 2020-03-12T00:28:51Z

+  - upon receiving a message of _odd_, unknown type:
+    - MUST ignore the received message.
+  - upon receiving a message of _even_, unknown type:
+    - MUST fail the channels.


I think must fail the channels is too strong here (applies to this entire section as well). It's an issue generally elsewhere in the spec, but you just inconvenience yourself by closing channels if you get a funny looking message. An alternative would be "close the transport connection", or "ignore the message".

It's true, we could simply close the connection on such cases, simply halting progress to notify the remote peer that something is wrong. I think the spec is a bit too hardcore on closing channels on some occasions, maybe it's a good idea to start making it more lenient.

Done in 70f1ed1

Roasbeef · 2020-03-12T00:30:32Z

+message. The base `init` message (without extensions) for these examples is
+`0x001000000000` (all features turned off).
+
+The following `init` messages are valid:


That init message (along with the error) message as is, can't really be extended :/

It includes a variable sized byte slice that can occupy the entire message. If we really need to extend it in the future, we can create another init message.

What do you mean? Since that variable size byte slice starts with a u16 that explicitly sets its length, I believe it can be extended. For the specific case of init, it is even already extended with a TLV stream since we added the networks TLV. Can you clarify if I misunderstood?

Ah do you mean that the existing message may fill the 65535 bytes of the message? In that case definitely, it will hit the message size limit and fail. Do you think we should mention that the extension can only be done within the bounds of the message length limit?

Yeah I mean the feature vectors can fill the entire message. Adding a mention/note of this sounds good to me. In the typical case this shouldn't happen, but implementations should be ready for it.

Great, I'll add that.

Added in a58cece.

Did you really implement that many hidden features in lnd that you're afraid of filling up the init? 😆

We don't need to be that strict and unforgiving. Let's give implementation the choice to simply close the connection.

Add a note that when a message payload already fills all the available bytes, it's not possible to add an extension.

Roasbeef

LGTM 🎏

rustyrussell · 2020-03-30T19:44:21Z

Ack!

t-bast · 2020-03-31T06:57:39Z

Merging as agreed during yesterday's meeting: http://www.erisian.com.au/meetbot/lightning-dev/2020/lightning-dev.2020-03-30-19.06.log.html

t-bast requested review from Roasbeef, cfromknecht and rustyrussell March 2, 2020 08:32

cdecker mentioned this pull request Mar 2, 2020

Lightning Specification Meeting 2020/03/02 #744

Closed

15 tasks

t-bast requested a review from cdecker March 2, 2020 09:36

Roasbeef requested changes Mar 12, 2020

View reviewed changes

t-bast added 2 commits March 12, 2020 10:45

MAY close channels instead of MUST

70f1ed1

We don't need to be that strict and unforgiving. Let's give implementation the choice to simply close the connection.

Explicit example of 0-length shutdown_script

67b342d

t-bast mentioned this pull request Mar 16, 2020

Lightning Specification Meeting 2020/03/16 #756

Closed

18 tasks

Extension not always possible for big packets

a58cece

Add a note that when a message payload already fills all the available bytes, it's not possible to add an extension.

t-bast mentioned this pull request Mar 27, 2020

Lightning Specification Meeting 2020/03/30 #757

Closed

18 tasks

Roasbeef approved these changes Mar 30, 2020

View reviewed changes

t-bast merged commit f068dd0 into master Mar 31, 2020

t-bast deleted the b01-message-extension branch March 31, 2020 06:58

sidhujag mentioned this pull request Apr 11, 2020

alice private key,HD chain issue? omnilaboratory/OmniBOLT-spec#1

Open

Conversation

t-bast commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-bast commented Mar 2, 2020

Uh oh!

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

Uh oh!

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented Mar 30, 2020

Uh oh!

t-bast commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

t-bast commented Mar 2, 2020 •

edited

Loading