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

Stop compressing with zlib #2244

Merged
merged 2 commits into from
May 11, 2022
Merged

Stop compressing with zlib #2244

merged 2 commits into from
May 11, 2022

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Apr 26, 2022

While zlib provides good compression results for gossip, it's a dependency that had a few important CVEs in the past. Some implementations are reluctant to import it, so we decided to remove it from the specification in lightning/bolts#981

We stop compressing with zlib and only send uncompressed results, while still supporting receiving compressed data. We will remove support for decompression once our monitoring indicates that we stopped receiving compressed data.

@t-bast t-bast requested review from pm47 and sstone April 26, 2022 07:16
@codecov-commenter
Copy link

Codecov Report

Merging #2244 (91ec0b7) into master (f099409) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2244      +/-   ##
==========================================
+ Coverage   84.30%   84.31%   +0.01%     
==========================================
  Files         194      194              
  Lines       14383    14381       -2     
  Branches      589      587       -2     
==========================================
+ Hits        12125    12126       +1     
+ Misses       2258     2255       -3     
Impacted Files Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.12% <100.00%> (+0.32%) ⬆️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 85.98% <0.00%> (-0.38%) ⬇️
...clair/channel/publish/ReplaceableTxPublisher.scala 74.17% <0.00%> (+1.32%) ⬆️
...src/main/scala/fr/acinq/eclair/db/pg/PgUtils.scala 90.90% <0.00%> (+1.51%) ⬆️

@sstone
Copy link
Member

sstone commented May 10, 2022

LGTM, but how do we make sure that query messages still fit in 65 Kb ?

While zlib provides good compression results for gossip, it's a dependency
that had a few important CVEs in the past. Some implementations are
reluctant to import it, so we decided to remove it from the specification
in lightning/bolts#981

We stop compressing with zlib and only send uncompressed results, while
still supporting receiving compressed data. We will remove support for
decompression once our monitoring indicates that we stopped receiving
compressed data.
The previous calculation was wrong and could lead to messages bigger than 65kB.
@t-bast
Copy link
Member Author

t-bast commented May 10, 2022

LGTM, but how do we make sure that query messages still fit in 65 Kb ?

Thanks for spotting that, there was indeed an issue!
I don't know how we came up with the previous number of 3200, but it was clearly wrong and untested :/
This is fixed in a7653f9

@t-bast t-bast merged commit ee74f10 into master May 11, 2022
@t-bast t-bast deleted the remove-zlib branch May 11, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants