Turn rs-zstd into official version #51
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update the dependency declaration in the Changes
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
libzstd/encoder/Cargo.toml
Outdated
| zstd = { version = "0.13", features = ["experimental"]} |
There was a problem hiding this comment.
I'm concerned that this will affect the compression of codec version < 7, thus affecting the rollup verifier's batch hash calculation. @jonastheis, would the current L1 DA syncer replace constructing local blocks and compress them to a batch? e.g., replace by getting blob raw data and decompress it, then verify against local blocks.
There was a problem hiding this comment.
Yes. I think the later is better designation and we should finally adopt to that. Making compression more than once and expect the result is identify seems a bit danger because the result would be affected by many factors: any update of minor version, configuration etc.
There was a problem hiding this comment.
Another comment would be, could this be replaced by Go's zstd lib?
|
Yes. It is simply a wrapper of the 'official' (i.e. Facebook's) zstd
2025年4月17日(木) 22:14 colin ***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In libzstd/encoder/Cargo.toml
<#51 (comment)>:
> \ No newline at end of file
+zstd = { version = "0.13", features = ["experimental"]}
Another comment would be, could this be replaced by Go's zstd lib?
—
Reply to this email directly, view it on GitHub
<#51 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZ5WHCPCI5ZNZBMTWSR4KL2Z6SLZAVCNFSM6AAAAAB2OS665SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDONZVGY4DSMJWGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
colinlyguo
changed the title
colinlyguo
force-pushed
the
test/prim_zstd
branch
from
3a782c0 to
c89e7e5
Compare
libzstd/encoder-standard/src/lib.rs
Outdated
| .expect("infallible"); | ||
| // with a hack in zstd we can set window log <= 17 with single segment kept | ||
| encoder | ||
| .set_parameter(CParameter::WindowLog(17)) |
There was a problem hiding this comment.
For new encoder, increase the windowLog from 17 -> 22 (128K -> 4M), it would help to improve the compressing ratio and enable us a signle frame scheme for any source data less than 4MB (should be enough in quite a long peroid)
There was a problem hiding this comment.
Changed in this commit: b64a7d3 and regenerated the .a files.
|
Things should be ok now |
colinlyguo
force-pushed
the
test/prim_zstd
branch
from
bd66ad5 to
9592886
Compare
With update in zstd decompression circuit, we can fully support zstd compressed data generated from official implement, with current configurations (raw literal, no magic bytes, no custom dictionary, etc).
Since no hacking in zstd source is needed anymore once the new decompression circuit has been merged into new release of prover (maybe in the f-release?), we can turn the depending of zstd into official one safely.
NOTICE: DO NOT apply this PR before the new decompression circuit has been applied.
Summary by CodeRabbit