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 4: Define custom tlv type range #660

Merged
merged 1 commit into from Oct 28, 2019

Conversation

@joostjager
Copy link
Contributor

joostjager commented Aug 19, 2019

This PR defines a type range for custom tlv records. The goal is to prevent unofficial fields from allocating identifiers in the official range. This could cause problems when new fields are added to this specification and turn out to collide with custom records that are used in the wild.

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Aug 19, 2019

I think what you suggest is similar to #623 but less general, WDYT?
I think that if we want to explicitly allow something like that in the spec, we'd want it more generally than only for Bolt 4 so #623 makes more sense.

@joostjager

This comment has been minimized.

Copy link
Contributor Author

joostjager commented Aug 19, 2019

I don't think it is the same. Afaik #623 is about extensions of the node software itself, while custom tlv fields can be used with unmodified nodes to communicate application specific values from sender to receiver. For example an account number to deposit the funds onto. This isn't something that would be signaled on the p2p message level.

The point is that we already allow custom fields in the spec. As long as they are odd-numbered, they are accepted by receivers. The goal of this PR is to prevent informal pollution of the official range before it is too late.

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Aug 19, 2019

custom tlv fields can be used with unmodified nodes to communicate application specific values from sender to receiver

Can you detail how you achieve that? This isn't clear at all to me, if the node doesn't explicitly add support I don't see how this would happen with today's spec and implementations. Or you mean unmodified on the receiving end (but modified on the sending end)? If you modify the sending end then I think we should make a somewhat more general change to encompass any kind of experimentation optional change (not only onion).

I agree we need to avoid polluting tlv namespaces, but from my point of view right now anything that uses a tlv not registered in the spec is doomed to be overridden at some point and "split away" from the network. I'm not sure what we should do to make this better.

@joostjager

This comment has been minimized.

Copy link
Contributor Author

joostjager commented Aug 19, 2019

The way to achieve this is to accept a map of <int, []byte> on the send payment call. Those bytes will be encoded literally in the final hop payload with the given (custom) keys. At the receiver, the same map will be extracted and signaled to an application in the 'invoice settled' event.

The node itself doesn't need to know what those values are. It is just passing them on along with the payment.

Alternatively, a single official type user_data could be defined that contains an opaque byte array with whatever custom data the application requires.

I wouldn't want to generalize this to features to allow experimentation on every level.

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Aug 19, 2019

Alternatively, a single official type user_data could be defined that contains an opaque byte array with whatever custom data the application requires.

I think that would make more sense (if I understand your use-case correctly) than encouraging people to use new tlv types that they would choose and which wouldn't be spec-ed.

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Aug 19, 2019

Summarizing feedback from the IRC meeting: this should probably move to Bolt 1 to apply to tlv types in all namespaces.

@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Aug 20, 2019

Thinking about this further, opening up the TLV's of every message to any inserted format basically allows the passing of arbitrary data over the network. Restricting the 'anything goes' to just the onion payload means that these extensions are limited to 'paid' messages, which seems like a reasonable tradeoff. The space is also a bit more limited by nature.

Allowing 3rd party devs to add optional information to opening messages and the like seems like it'd offer a wider set of applications to use lightning as rails for their projects. I can see advantages and disadvantages to this level of extensibility.

Strongly in favor of adopting this for the onion TLV's, however. Which seems more short-term practicable anyway given that these already have TLVs.

@joostjager joostjager force-pushed the joostjager:custom-tlv-fields branch 3 times, most recently from 18f1b11 to 2c582e7 Aug 20, 2019
@joostjager

This comment has been minimized.

Copy link
Contributor Author

joostjager commented Aug 20, 2019

Moved definition of custom type range to BOLT 1.

Also changed range from >=2^32 to >=2^16. It seems that 65536 official types is enough (...) and prevents annoyances in dealing with long integers for custom types.

@t-bast

This comment has been minimized.

Copy link
Collaborator

t-bast commented Aug 21, 2019

Allowing 3rd party devs to add optional information to opening messages and the like seems like it'd offer a wider set of applications to use lightning as rails for their projects. I can see advantages and disadvantages to this level of extensibility.

TBH we are already using this outside of the onion payload (to experiment with gossip, routing and static backup improvements). So if we're doing it, it's likely that LND and CL will be doing it as well, and other devs too. So I'd like to define this range for all tlvs to make our experimentations spec-compliant ;)

01-messaging.md Outdated Show resolved Hide resolved
01-messaging.md Outdated Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:custom-tlv-fields branch from 2c582e7 to e05e34a Sep 2, 2019
@cdecker

This comment has been minimized.

Copy link
Collaborator

cdecker commented Oct 18, 2019

ACK e05e34a

Copy link
Member

Roasbeef left a comment

LGTM 🌠

@t-bast
t-bast approved these changes Oct 21, 2019
Copy link
Collaborator

t-bast left a comment

ACK e05e34a.
Sounds like we have an agreement from everyone, let's merge this during the next spec meeting.

@Roasbeef Roasbeef merged commit 3d60581 into lightningnetwork:master Oct 28, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.