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

Advertize compression algorithms support in init (features 32/33) #825

Closed
wants to merge 1 commit into from

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Dec 14, 2020

Add a list of supported compression algorithms to the init message.
These compression algorithms may be used in several places in the future, but currently only in extended gossip queries.

Without this explicit field, it's impossible to opt-out of zlib compression.
If your node implementation does not support zlib, it will only be discovered when your peer sends you a query_short_channel_ids that asks for a compressed response. Your only choices are to close the connection or send back uncompressed data, which may or may not be accepted by your peer.

Fixes #811

Copy link
Contributor

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

09-features.md Outdated Show resolved Hide resolved
@t-bast
Copy link
Collaborator Author

t-bast commented Dec 23, 2020

I've thought about this more, and I'm not a fan of any kind of option_raw_format: feature bits should not be used to advertise a lack of functionality. But I'm not a fan of an explicit zlib_compress feature bit either.

Another angle that could be explored for this would be a new tlv field in the init message:

1. type: 3 (`compression_methods`)
2. data:
        * [`tu64`:`supported_algorithms`]

Each bit indicates support for a specific compression algorithm.
The only supported values for now would be 0x00 (only uncompressed data) and 0x01 (support for zlib compression).
I don't expect us to ever support a lot of compression options, but since we have truncated uint64 available in all implementations, this gives us 64 potential algorithms that can be used (more than enough).

Nodes then need to remember what compression options are supported by their peers and act accordingly when doing gossip queries. If our peer set 0x00, we should only use the raw format. If he set 0x01 we can use either raw or zlib format.

What do you think @TheBlueMatt @ariard @bmancini55? Should I amend this PR to go in that direction?

@bmancini55
Copy link

Thanks @t-bast!

I agree, option_raw_format seems clunky. I do think zlib_compress could work if we follow similar rules that you just outlined, basically defaulting to using raw in the absence of feature agreement.

That said, I like your new TLV proposal. It makes sense that supported compression would be indicated in the same way the encoding flags are used.

One suggestion would be to put the TLV and associated logic under an option_compression feature. This way it would be clear which nodes are legacy and which support these cascading compression rules. Peer selection could then make determinations beforehand to connect to nodes that support these rules.

@t-bast
Copy link
Collaborator Author

t-bast commented Dec 28, 2020

One suggestion would be to put the TLV and associated logic under an option_compression feature. This way it would be clear which nodes are legacy and which support these cascading compression rules. Peer selection could then make determinations beforehand to connect to nodes that support these rules.

Done in 673c912

@t-bast t-bast changed the title Advertize zlib support via feature bits Advertize compression algorithms support in init Dec 28, 2020
01-messaging.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@t-bast
Copy link
Collaborator Author

t-bast commented Jan 5, 2021

Rebased and clarified the case where no compression algorithm is supported in c4322b7

01-messaging.md Outdated Show resolved Hide resolved
09-features.md Outdated
| 18/19 | `option_support_large_channel` | Can create large channels | IN | | [BOLT #2](02-peer-protocol.md#the-open_channel-message) |
| 20/21 | `option_anchor_outputs` | Anchor outputs | IN | `option_static_remotekey` | [BOLT #3](03-transactions.md) |
| 22/23 | `option_anchors_zero_fee_htlc_tx` | Anchor commitment type with zero fee HTLC transactions | IN | | [BOLT #3][bolt03-htlc-tx], [lightning-dev][ml-sighash-single-harmful]|
| 24/25 | `option_compression` | Compression algorithms advertised in `init` | IN | | [BOLT #1](01-messaging.md#the-init-message) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds...risky. There's no guarantee this will be the final spec feature bit.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Jul 23, 2021
@TheBlueMatt
Copy link
Collaborator

I have an implementation of this at lightningdevkit/rust-lightning#1015, happy to test with any other implementations :).

@t-bast
Copy link
Collaborator Author

t-bast commented Aug 13, 2021

I have an implementation of this at lightningdevkit/rust-lightning#1015, happy to test with any other implementations :).

I have a very old branch with early eclair support for it, but it needs many changes now, I'll get back to it when I have some time available.

t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 15, 2021
Implement lightning/bolts#825

This lets us specify which compression algorithms we support and use one
that our peer supports as well when syncing the network graph.
@t-bast
Copy link
Collaborator Author

t-bast commented Sep 15, 2021

I have an implementation of this at lightningdevkit/rust-lightning#1015, happy to test with any other implementations :).

Here is a PR to support this feature on eclair: ACINQ/eclair#1956
If you can test it against rust-lightning it would be great!

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Nov 10, 2021
@t-bast t-bast changed the title Advertize compression algorithms support in init Advertize compression algorithms support in init (features 32/33) Nov 12, 2021
@TheBlueMatt
Copy link
Collaborator

Needs rebase.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Nov 13, 2021
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Nov 13, 2021
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM, I have only some small comments

t-bast added a commit to ACINQ/eclair that referenced this pull request Nov 30, 2021
Implement lightning/bolts#825

This lets us specify which compression algorithms we support and use one
that our peer supports as well when syncing the network graph.
t-bast added a commit to ACINQ/eclair that referenced this pull request Nov 30, 2021
Implement lightning/bolts#825

This lets us specify which compression algorithms we support and use one
that our peer supports as well when syncing the network graph.
t-bast added a commit to ACINQ/eclair that referenced this pull request Nov 30, 2021
Implement lightning/bolts#825

This lets us specify which compression algorithms we support and use one
that our peer supports as well when syncing the network graph.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Nov 30, 2021
t-bast added a commit to ACINQ/eclair that referenced this pull request Dec 1, 2021
Implement lightning/bolts#825

This lets us specify which compression algorithms we support and use one
that our peer supports as well when syncing the network graph.
@TheBlueMatt
Copy link
Collaborator

Needs rebase. @t-bast and I successfully tested this between LDK and eclair.

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
@TheBlueMatt
Copy link
Collaborator

Meeting discussion pivoted to just dropping zlib - it only adds so much and the complexity overhead of this PR maybe isn't worth it vs just deprecating zlib to begin with.

@t-bast
Copy link
Collaborator Author

t-bast commented Dec 7, 2021

I'll reach out to a few mobile wallets to see if they are fine with deprecating zlib.

TheBlueMatt added a commit to TheBlueMatt/lightning-rfc that referenced this pull request Apr 20, 2022
Gossip query compression is not very useful - it was added for
mobile clients to, in theory, sync the gossip data directly from
P2P peers, but to my knowledge no mobile clients actually use it
for that, or at least use it where the gossip *query* data is a
substantial portion of their overall bandwidth usage.

Further, because of the semantics of `gossip_timestamp_filter`, its
impractical to ensure you receive a reliable, full view of the
gossip data without re-downloading large portions of the gossip
data on startup.

Ultimately, gossip queries are a pretty non-optimal method of
synchronizing the gossip data. If someone wants highly optimized
gossip data synchronization a new method based on set
reconciliation needs to be propose.

Finally, the current gossip query encoding semantics do not allow
for negotiation and instead require all lightning implementations
take a zlib dependency in some form or another. Given the recent
zlib decoding memory corruption vulnerability, this seems like an
opportune time to simply remove the zlib support, requiring that
nodes stop sending compressed gossip query data (though they can
support reading such gossip query data as long as they wish).

This is an alternative to the suggested gossip query encoding
support in lightning#825.
TheBlueMatt added a commit to TheBlueMatt/lightning-rfc that referenced this pull request Apr 21, 2022
Gossip query compression is not very useful - it was added for
mobile clients to, in theory, sync the gossip data directly from
P2P peers, but to my knowledge no mobile clients actually use it
for that, or at least use it where the gossip *query* data is a
substantial portion of their overall bandwidth usage.

Further, because of the semantics of `gossip_timestamp_filter`, its
impractical to ensure you receive a reliable, full view of the
gossip data without re-downloading large portions of the gossip
data on startup.

Ultimately, gossip queries are a pretty non-optimal method of
synchronizing the gossip data. If someone wants highly optimized
gossip data synchronization a new method based on set
reconciliation needs to be propose.

Finally, the current gossip query encoding semantics do not allow
for negotiation and instead require all lightning implementations
take a zlib dependency in some form or another. Given the recent
zlib decoding memory corruption vulnerability, this seems like an
opportune time to simply remove the zlib support, requiring that
nodes stop sending compressed gossip query data (though they can
support reading such gossip query data as long as they wish).

This is an alternative to the suggested gossip query encoding
support in lightning#825.
@t-bast
Copy link
Collaborator Author

t-bast commented Apr 26, 2022

Replaced by #981 (a.k.a death to zlib)

@t-bast t-bast closed this Apr 26, 2022
@t-bast t-bast deleted the zlib-feature-flag branch April 26, 2022 07:08
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.

Encodings in gossip_queries
7 participants