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

Encodings in gossip_queries #811

Closed
bmancini55 opened this issue Oct 26, 2020 · 3 comments
Closed

Encodings in gossip_queries #811

bmancini55 opened this issue Oct 26, 2020 · 3 comments

Comments

@bmancini55
Copy link

Currently the spec requires an implementation to support both raw and zlib deflate encodings for short_channel_ids in reply_channel_range and query_short_channel_ids messages.

There is discrepancy of the default encoding used when replying to a query with reply_channel_range message. It would be nice to have clarification surrounding when zlib deflate or raw encodings should be sent in reply to a query.

A bigger request is that rust-lightning would like to not add a zlib dependency to the project. There is currently no way to signal support or lack thereof for zlib. It would be nice if a node could signal this support in Init. There is likely overlap with #804 depending on how reply sequencing is structured, so I wanted to raise this issue for discussions while that is being discussed.

@ariard
Copy link
Contributor

ariard commented Nov 4, 2020

@bmancini55 Or can you add a option_scid_encodings in Init and a new u8 supported_encodings in gossip_timestamp_filter, which is also re-purposed as a encoding negotiation message ? Though I've not thought that much about soundness of this, do people expect to extend with other encodings in the future?

@bmancini55
Copy link
Author

@ariard, that's an interesting idea!

From my understanding of the historical conversation there isn't much interest in supporting additional encodings. That said, I believe compression addresses two concerns:

  1. Trades bandwidth for processing
  2. Increases the limit of scids we can share for a single block

The latter exists because BOLT7 requires sequential reply_channel_range messages to start on the next block. There is a limit of ~8k channels per message due to the number of scids you can pack into the 65535-byte Lightning message payload. If a block has more than ~8k scids there is no way to share that in a reply_channel_range, unless you compress it.

My hope is that if we allow resumption on the same block in #804, concern 2 goes away and encoding becomes more a preferential thing than an escape hatch.

t-bast added a commit that referenced this issue Dec 28, 2020
Add a tlv field in `init` to list supported compression algorithms.

This compression will be used in several places, currently in extended
gossip queries.

Fixes #811
t-bast added a commit that referenced this issue Dec 28, 2020
Add a tlv field in `init` to list supported compression algorithms.

This compression will be used in several places, currently in extended
gossip queries.

Fixes #811
t-bast added a commit that referenced this issue Jan 5, 2021
Add a tlv field in `init` to list supported compression algorithms.

This compression will be used in several places, currently in extended
gossip queries.

Fixes #811
t-bast added a commit that referenced this issue Feb 2, 2021
Add a tlv field in `init` to list supported compression algorithms.

This compression will be used in several places, currently in extended
gossip queries.

Fixes #811
t-bast added a commit that referenced this issue Nov 23, 2021
Add a tlv field in `init` to list supported compression algorithms.

This compression will be used in several places, currently in extended
gossip queries.

Fixes #811
t-bast added a commit that referenced this issue Dec 6, 2021
Add a tlv field in `init` to list supported compression algorithms.

This compression will be used in several places, currently in extended
gossip queries.

Fixes #811
@t-bast
Copy link
Collaborator

t-bast commented Jun 8, 2022

We removed zlib support in #981, thus fixing this issue.

@t-bast t-bast closed this as completed Jun 8, 2022
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 a pull request may close this issue.

4 participants
@bmancini55 @ariard @t-bast and others