Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bolt 1: Specify that extensions to existing messages must use TLV #714

Open
wants to merge 3 commits into
base: master
from

Conversation

@t-bast
Copy link
Collaborator

t-bast commented Dec 9, 2019

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 would make sense to use that optional
additional space at the end of each message to allow an optional tlv stream.

We keep the "additional bytes must be ignored" rule for what happens after that
tlv stream, giving us extra flexibility to add another type of optional content
to existing messages later if needed.

This was already proposed in #630 but rejected. I found additional reasons to
specify this now rather than on a per-message basis. We don't want to have two
competing and incompatible ways of extending existing messages.

Imagine node A and node B are peers, both running experiments by leveraging the
"ignore additional data" rule. However node A uses a TLV stream (as proposed in
this PR) while node B uses a different encoding. Unluckily node B also started his
encoding with something that can be interpreted as a varint, so node A interprets
this as a TLV stream, but it decodes as a two-bytes even number (which has a 50%
chance of happening). She (validly) thinks it's an even TLV she doesn't understand,
so she fails the connection and closes the channels.

This is a major inconvenience when experimenting with message extensions. I believe
what this PR proposes is reasonable and prevents these competing encodings to mess
up experimentations.

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 would make sense to use that optional
additional space at the end of each message to allow an optional tlv stream.

We keep the "additional bytes must be ignored" rule for what happens after that
tlv stream, giving us extra flexibility to add another type of optional content
to existing messages later if needed.
@t-bast t-bast requested review from cfromknecht, niftynei and pm47 Dec 9, 2019
@t-bast

This comment has been minimized.

Copy link
Collaborator Author

t-bast commented Dec 9, 2019

Open question: should the TLV stream be prefixed with its length or not?

It seems like the gossip_queries tlv streams today don't prefix with the length (length is inferred from the overall message length).

The inconvenient when we don't prefix with the length is that we completely remove the possibility of having another bag of uninterpreted bytes at the end of the message (if we decide some day that we want to extend messages with something else than TLV). Maybe we don't care, maybe we do :)

I'm leaning towards removing the tlv stream length prefix: in that case this PR makes even more sense to do now, because we don't want to have some messages be extended with a TLV stream and others extended differently, that would be a big mess. WDYT @rustyrussell @cfromknecht @sstone ?

UPDATED: removed the length prefix in 6117c30.

@cdecker

This comment has been minimized.

Copy link
Collaborator

cdecker commented Dec 9, 2019

If we limit message extensions to be TLV-based we should definitely drop the length prefix.

I'd like to point out that there is a small caveat for gossip broadcast messages (node_announcement, channel_announcement and channel_update) which requires that they include the fields in the signature, which is the main reason we went for a canonical encoding for TLVs (no duplicate types and monotonically increasing type order). Just as a caveat to the ignore word that is being used in the PR.

Other than that small wording issue, I'm definitely in favor of consolidating towards TLVs.

t-bast added 2 commits Dec 10, 2019
Clarify that extension bytes are included in signatures.
This applies to:

- option_upfront_shutdown_script: if you're not interested, just set the length to 0
- channel_reestablish commitment points: it makes sense to always include those regardless of whether `option_dataloss_protect` or `option_static_remotekey` are set

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

This comment has been minimized.

Copy link
Collaborator Author

t-bast commented Dec 10, 2019

I added two commits:

  • 6117c30 simply removes the length-prefix as agreed during the spec meeting
  • 274a965 makes mandatory some message optional fields that would otherwise make parsing the extension non-deterministic: the commit message contains additional details. @Roasbeef @cfromknecht let me know if making these fields mandatory seems ok to you (thanks for raising the issue during the meeting).
@t-bast

This comment has been minimized.

Copy link
Collaborator Author

t-bast commented Jan 23, 2020

Do you think we should re-work upfront_shutdown_script to move it to a TLV record instead of the current optional encoding? That would simplify the migration to TLV for all messages.

It could be done in an interesting backwards-compatible way:

  • assign type 0x00 to upfront_shutdown_script
  • then for all scripts that have a length < 253 the TLV encoding will perfectly match the current encoding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.