-
Notifications
You must be signed in to change notification settings - Fork 492
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
Conversation
There was a problem hiding this 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.
I've thought about this more, and I'm not a fan of any kind of Another angle that could be explored for this would be a new
Each bit indicates support for a specific compression algorithm. Nodes then need to remember what compression options are supported by their peers and act accordingly when doing gossip queries. If our peer set What do you think @TheBlueMatt @ariard @bmancini55? Should I amend this PR to go in that direction? |
Thanks @t-bast! I agree, 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 |
35c479d
to
673c912
Compare
Done in 673c912 |
init
8e2257d
to
bc87bda
Compare
There was a problem hiding this 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.
8548f2f
to
c4322b7
Compare
Rebased and clarified the case where no compression algorithm is supported in c4322b7 |
c4322b7
to
5071b93
Compare
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) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Electrum version 4.1.0 uses bit 25 for Trampoline: https://github.com/spesmilo/electrum/blob/92226b077ad9d248e0f3325f3de89b34b1778ea5/RELEASE-NOTES#L76
There was a problem hiding this comment.
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.
This adds the new feature bit from lightning/bolts#825
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. |
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.
Here is a PR to support this feature on eclair: ACINQ/eclair#1956 |
This adds the new feature bit from lightning/bolts#825
init
init
(features 32/33)
Needs rebase. |
This adds the new feature bit from lightning/bolts#825
This adds the new feature bit from lightning/bolts#825
There was a problem hiding this 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
e0a61bd
to
3cf46fd
Compare
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.
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.
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.
This adds the new feature bit from lightning/bolts#825
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.
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
3cf46fd
to
192bfc3
Compare
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. |
I'll reach out to a few mobile wallets to see if they are fine with deprecating zlib. |
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.
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.
Replaced by #981 (a.k.a death to zlib) |
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