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

Update messaging and serialization #171

Conversation

Tibo-lg
Copy link
Member

@Tibo-lg Tibo-lg commented Jul 20, 2021

No description provided.

Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

I am confused how we differentiate a single_contract_info from a enumerated_contract_descriptor if they both have the same type?

@Tibo-lg
Copy link
Member Author

Tibo-lg commented Aug 6, 2021

I am confused how we differentiate a single_contract_info from a enumerated_contract_descriptor if they both have the same type?

Depending on the context, you know which one you want to read so there is no confusion. It's a bit the same as how you distinguish between a u16 or u64, you don't have a type prefix, you just know which one you expect.

@benthecarman
Copy link
Contributor

Depending on the context, you know which one you want to read so there is no confusion. It's a bit the same as how you distinguish between a u16 or u64, you don't have a type prefix, you just know which one you expect.

Oh I see, offer, accept, and sign, all still have their types defined

@benthecarman
Copy link
Contributor

If we are going to break backwards compatability with current messages, it may be worth to change it so the messages will fit for the current lightning spec. ie break up accept and sign tlvs so they can fit in a single message

@Tibo-lg
Copy link
Member Author

Tibo-lg commented Aug 10, 2021

Oh I see, offer, accept, and sign, all still have their types defined

Yes exactly, all messages that are sent directly on wire need to have a specific type number defined (from the custom LN space).

If we are going to break backwards compatability with current messages, it may be worth to change it so the messages will fit for the current lightning spec. ie break up accept and sign tlvs so they can fit in a single message

I think we discussed about that at some point, but the preferred approach was to have a protocol to split messages into packets. So we would keep the current serialization format but before being sent on wire each message (that is too large) would be split before being sent on wire, and reconstructed on the receiver side.

@benthecarman
Copy link
Contributor

I think we discussed about that at some point, but the preferred approach was to have a protocol to split messages into packets. So we would keep the current serialization format but before being sent on wire each message (that is too large) would be split before being sent on wire, and reconstructed on the receiver side.

Do we have a timeline on that?

@Tibo-lg
Copy link
Member Author

Tibo-lg commented Aug 10, 2021

Do we have a timeline on that?

Not really. Tbh honest I had kind of forgotten about it, but I realize I might actually need it rather soon. I don't think I can spend time on that before next meeting so maybe we can bring that up then?

@Tibo-lg Tibo-lg mentioned this pull request Aug 10, 2021
@Tibo-lg Tibo-lg added the breaking The proposed change breaks backward compatibility label Sep 16, 2021
- [Wire Messages](#wire-messages)
- [Type-Length-Value](#type-length-value)
- [Sub-types](#sub-types)
- [Plain sub-type](#plain-sub-type)
Copy link
Contributor

Choose a reason for hiding this comment

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

So is there precedent for using this scheme outside of the dlc protocol? Is this used in other cryptocurrency protocols? If so could you link to other projects documentation on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This scheme in particular I don't know, but it's just a serialization format. If we want to reuse something that exists already it's fine but then we should probably not use the LN stack at all for serialization (e.g. protobuf). The goal here was just to extend the serialization format to facilitate serialization of things that maybe are nota concerns in LN ( or at least that they have not formally defined)

TLV serialization should be used to serialize sibling types.
Each sibling should be assigned a type identifier.
The type identifiers are specific to a set of type variants, and can thus be reused across different sub-types.
Type identifiers are defined within the type definitions, starting from `0` and increasing by `1`.
Copy link
Contributor

Choose a reason for hiding this comment

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

So my understanding with this scheme, the hypothetical improvements are

  1. A fixed set of siblings allocated for the parent's TLV type, where as now we can have an open ended set of "siblings" by changing the nested fields the TLV type
  2. We are saving 3 bytes per nested field in a message (bigsize is 4 bytes, this is 1 byte). So likely we would save on the order of ~20 bytes per message?

Is this accurate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think your first point is correct. At the moment the format is not more or less flexible than it would be with this proposal, because we don't expect for anything else than the "TLV" type that is written in the spec.


1. type: 42786 (`oracle_info_v1`)
2. data:
1. implements: `oracle_info`
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: the 1,2,3..

Copy link
Member Author

Choose a reason for hiding this comment

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

This should render as 1 2 3, it's just easier to put only ones so you don't have to modify everything if you want to add something at the begging

Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

I prefer this version over #163

@Christewart
Copy link
Contributor

Are the version messages in #163 omitted for a reason or just forgot? Or perhaps I'm just missing it

@Tibo-lg Tibo-lg force-pushed the serialization-update-proposal-2 branch from c65bec2 to fce8bdd Compare October 6, 2021 01:43
@Tibo-lg
Copy link
Member Author

Tibo-lg commented Oct 6, 2021

Are the version messages in #163 omitted for a reason or just forgot? Or perhaps I'm just missing it

Sorry I'm not sure I see what you mean? The protocol version field is there as well: https://github.com/discreetlogcontracts/dlcspecs/pull/163/files#diff-e0f5b925f91a1c09c6daf26f9a7d28816cb9ff9f08863faca719b7ee0a1cc065R69

@Tibo-lg Tibo-lg force-pushed the serialization-update-proposal-2 branch from fce8bdd to 070fd79 Compare October 6, 2021 03:42
@Tibo-lg Tibo-lg force-pushed the serialization-update-proposal-2 branch from 070fd79 to bd3dbb1 Compare October 6, 2021 05:28
@Christewart Christewart closed this Dec 8, 2021
@Christewart
Copy link
Contributor

superseded by #163

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking The proposed change breaks backward compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants