fix: building in zkvm guest #457
Conversation
CodSpeed Performance ReportMerging #457 will not alter performanceComparing Summary
|
|
|
||
| use zstd::Decoder; | ||
| #[cfg(feature = "zstd")] | ||
| use ruzstd as _; |
There was a problem hiding this comment.
ruzstd is only used for decoding? I guess it should be fully compatible with the C impl, right?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
they also uses other things provided by scroll-codec, so two options:
- provide option to use rust zstd
- gate v2::zstd, and make zstd as optional dep.
There was a problem hiding this comment.
- 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?
|
|
||
| use zstd::Decoder; | ||
| #[cfg(feature = "zstd")] | ||
| use ruzstd as _; |
There was a problem hiding this comment.
What does this line do? Since we only use ruzstd when not(feature = "zstd")
There was a problem hiding this comment.
suppress unsued crate if such lint enabled.
There was a problem hiding this comment.
Instead of suppressing the unused crate, can you make both zstd and ruzstd optional? Then add a line at the top of this module to ensure that either zstd or ruzstd is enabled (not both). I think this should be something like:
#[cfg(not(any(feature = "zstd", feature = "ruzstd")))]
compile_error!("You must enable exactly one of the `zstd` or `ruzstd` features");
#[cfg(all(feature = "zstd", feature = "ruzstd"))]
compile_error!("Features `zstd` and `ruzstd` are mutually exclusive");| 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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ok, but previous zstd call also unwrap and ignore errors, is there a such assumption that data is always valid?
There was a problem hiding this comment.
returning Result will change the API, it's a breaking change, ok for me, but ok for others?
There was a problem hiding this comment.
I believe this function is only used inside rollup-node, no?
There was a problem hiding this comment.
I think it's fine to update the API.
There was a problem hiding this comment.
I agree that is fine to update the API. The reason we used unwrap is that it is undefined behaviour in the protocol spec to have invalid data in batches. However, I agree that an update to the API is preferable.
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Added a comment inline about making both crates optional. We should also sync the rollup node from genesis to check if this feature is valid. I will run a sync of sepolia rollup node using this branch on a vm and report back once I have results.
|
|
||
| use zstd::Decoder; | ||
| #[cfg(feature = "zstd")] | ||
| use ruzstd as _; |
There was a problem hiding this comment.
Instead of suppressing the unused crate, can you make both zstd and ruzstd optional? Then add a line at the top of this module to ensure that either zstd or ruzstd is enabled (not both). I think this should be something like:
#[cfg(not(any(feature = "zstd", feature = "ruzstd")))]
compile_error!("You must enable exactly one of the `zstd` or `ruzstd` features");
#[cfg(all(feature = "zstd", feature = "ruzstd"))]
compile_error!("Features `zstd` and `ruzstd` are mutually exclusive");
dogeos using codec crate in guest, zstd won't compile on such target.