-
Notifications
You must be signed in to change notification settings - Fork 675
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
Add zstd compression #1278
Add zstd compression #1278
Conversation
…o-internal into add-zstd-compression
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 - up to you if you want to add the log or not.
if int64(len(decompressed)) > z.maxSize { | ||
return nil, fmt.Errorf("%w: (%d) > (%d)", ErrDecompressedMsgTooLarge, len(decompressed), z.maxSize) |
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.
Why do we return an error here? At this point we've already decompressed the payload so it seems like a waste to drop this message - i think it makes sense to limit the size of the thing we're compressing or the size of the thing we're decompressing but it feels strange to limit the size of the result of the decompression
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.
if we don't do this error handling then i think we can also not do the weird extra byte allocation in the earlier line
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.
We did not decompress the message in this case. We stopped decompressing the message because it may have been a zip bomb.
Out of interest, do you have a sample of the data you're compressing/decompressing (anything available, even a bulk network dump)? After a peek at the code it does seems to be mainly network messaging data, if I am not mistaken? |
Can't speak to the composition of messages during the test Patrick mentioned, but yes, this compression is only used for P2P messages. |
Still needs resolution to the following:
Note this changes metric names from
[message type]_compress_time
/[message type]_decompress_time
tozstd_[message type]_compress_time
andgzip_[message type]_compress_time
. Grafana dashboards will need to be updated accordingly.Note this deprecates the
--network-compression-enabled
flag in favor of new--network-compression-type
.Why this should be merged
zstd compression appears significantly faster than gzip, and marginally better at compressing messages.
How this works
Adds new zstd
Compressor
. Default behavior is still gzip. zstd is forbidden (via config) until v1.10.0. We need to update it so that it's forbidden until network upgrade time passes, but there's no var in the code for that yet.How this was tested
Existing/new UT