Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion crates/codec/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@ bitvec.workspace = true
derive_more = { version = "2.0", default-features = false }
eyre = { workspace = true, optional = true }
thiserror = { version = "2.0", default-features = false }
zstd = "=0.13.3"
zstd = { version = "=0.13.3", optional = true }
ruzstd = "0.8"

[dev-dependencies]
eyre.workspace = true
serde_json = "1.0"

[features]
default = ["zstd"]
test-utils = ["dep:eyre", "scroll-l1/test-utils"]
zstd = ["dep:zstd"]
23 changes: 22 additions & 1 deletion crates/codec/src/decoding/v2/zstd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,18 @@

use std::io::Read;

use zstd::Decoder;
#[cfg(feature = "zstd")]
use ruzstd as _;
Copy link
Contributor

Choose a reason for hiding this comment

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

ruzstd is only used for decoding? I guess it should be fully compatible with the C impl, right?

Copy link
Member

@georgehao georgehao Dec 9, 2025

Choose a reason for hiding this comment

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

also curious about this?

dogeos using codec crate in guest, zstd won't compile on such target.

don't get this, why don't put decompress_blob_data at dogeos side

Copy link
Member Author

Choose a reason for hiding this comment

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

they also uses other things provided by scroll-codec, so two options:

  1. provide option to use rust zstd
  2. gate v2::zstd, and make zstd as optional dep.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. gate v2::zstd, and make zstd as optional dep.

Does that mean that they don't actually need decompress_blob_data? Otherwise, without zstd, how would it work?


/// The ZSTD magic number for zstd compressed data header.
const ZSTD_MAGIC_NUMBER: [u8; 4] = [0x28, 0xb5, 0x2f, 0xfd];

/// Uncompress the provided data.
#[cfg(feature = "zstd")]
pub fn decompress_blob_data(data: &[u8]) -> Vec<u8> {
use zstd::Decoder;
let mut header_data = ZSTD_MAGIC_NUMBER.to_vec();

header_data.extend_from_slice(data);

// init decoder and owned output data.
Expand All @@ -31,3 +35,20 @@ pub fn decompress_blob_data(data: &[u8]) -> Vec<u8> {

output
}

/// Uncompress the provided data.
#[cfg(not(feature = "zstd"))]
pub fn decompress_blob_data(data: &[u8]) -> Vec<u8> {
use ruzstd::decoding::StreamingDecoder;

let mut header_data = ZSTD_MAGIC_NUMBER.to_vec();
header_data.extend_from_slice(data);

// init decoder and owned output data.
let mut decoder = StreamingDecoder::new(header_data.as_slice()).unwrap();
// heuristic: use data length as the allocated output capacity.
let mut output = Vec::with_capacity(header_data.len());
decoder.read_to_end(&mut output).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why it's safe to use unwrap here. Are you sure this cannot fail? If not, we should return Result instead

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, but previous zstd call also unwrap and ignore errors, is there a such assumption that data is always valid?

let mut decoder = Decoder::new(header_data.as_slice()).unwrap();

while let Ok(size) = decoder.read(&mut dst) {

Copy link
Member Author

Choose a reason for hiding this comment

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

returning Result will change the API, it's a breaking change, ok for me, but ok for others?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this function is only used inside rollup-node, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to update the API.


output
}
Loading