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

Clarification on the occurences of TLV records with the same type #777

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

dr-orlovsky
Copy link
Contributor

Closes #776 as discussed within the issue

01-messaging.md Outdated
Comment on lines 197 to 211
The main reason for unique occurrences of some TLV record type relates back the
fact that if a decoder doesn't recognize a field, it can't be certain whether
there should only be one, or if multiple are permitted. Asserting this would
require some knowledge of what the field encodes or a scheme from which it can
be inferred knowing only the type, e.g. (a silly example) only prime types can
have multiple records. The first defeats the purpose of the proposal, and the
latter is cumbersome in practice and would further fragment the type space
beyond the existing even/odd rule.

A minor reasons for strictly-unique types is that permitting multiple of the
same type is less efficient on the wire due to the overhead of redundant
type-length pairs. It is also less efficient when allocating memory for the
decoded objects, e.g. one can't allocate the exact size of a vector upfront and
will incur more copying overhead when appending each item individually.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove these paragraphs entirely, they clutter the spec (IMHO).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I personally find this explanation (when was given it) quire useful; it helps understanding the reasons of particular design. The spec is not that large yet, so having two more paragraphs adding clarity is good. But I am fine to remove them LN community thinks they are unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @cfromknecht chime in, but I think the general feeling when we discussed this during a spec meeting was to remove or condense these paragraphs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done + rebased

@dr-orlovsky
Copy link
Contributor Author

The paragraph, according to @t-bast critics, is removed. Also, have done a rebase onto master

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6299890

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🛸

@@ -135,7 +135,8 @@ according to the message-specific format determined by `type`.
### Requirements

The sending node:
- MUST order `tlv_record`s in a `tlv_stream` by monotonically-increasing `type`.
- MUST order `tlv_record`s in a `tlv_stream` by strictly-increasing `type`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strictly-increasing is not the same as "unique", which this sentence implies with "i.e."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly increasing does imply unique, doesn't it? (the reverse isn't true)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below we have:

The strict monotonicity constraint ensures that all types are unique and can at most once. Fields that map to complex objects, e.g. vectors, maps, or appear at most once

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, but "i.e." implies that they are equivalent in that, as merriam-webster says "it introduces a rewording or a clarification". An implication is not a rewording.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your suggestion to change i.e. MUST not produce to hence MUST not produce?
If so, @dr-orlovsky can you apply this small change and we can merge?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that is a great idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Copy link
Collaborator

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack 6299890

@ariard
Copy link

ariard commented Jul 6, 2020

ACK

@t-bast t-bast merged commit bdd4271 into lightning:master Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BOLT 2: Clarifying requirements for multiple TLV record occurrences of the same type
6 participants