feat(snapshotter): deduplicate RocksDB proofs SSTs - #4915
Conversation
🟡 Heimdall Review Status
|
|
✅ All benchmarks green — 14 within ±2% (deterministic instruction counts). View run Benchmark details (14)
|
1620b30 to
bdeca99
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| for entry in entries { | ||
| if Self::verify_outputs(target_dir, &entry.output_files)? { | ||
| info!(target: "reth::cli", file = %entry.file_name, "Reusing verified proofs snapshot artifact"); | ||
| continue; | ||
| } | ||
|
|
||
| Self::cleanup_outputs(target_dir, &entry.output_files); | ||
| let archive_path = Self::download_archive(&entry, &cache_dir).await?; | ||
| Self::extract_tar_zst(&archive_path, target_dir)?; | ||
| tokio::fs::remove_file(&archive_path).await.ok(); | ||
|
|
||
| Self::extract_and_cleanup(&archive_path, target_dir, &cache_dir).await | ||
| if !Self::verify_outputs(target_dir, &entry.output_files)? { | ||
| Self::cleanup_outputs(target_dir, &entry.output_files); | ||
| eyre::bail!( | ||
| "proofs archive extracted but output verification failed: {}", | ||
| entry.file_name | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Issue: Blocking I/O on the async runtime
Both verify_outputs (line 165, 175) and extract_tar_zst (line 172) perform synchronous file I/O — verify_outputs reads and BLAKE3-hashes every output file, and extract_tar_zst decompresses an entire tar.zst archive. Calling these directly from the async run_from_manifest method blocks the tokio runtime thread.
The previous implementation wrapped extract_tar_zst in tokio::task::spawn_blocking, which was removed in this refactor. For SST files that may be hundreds of megabytes, the blocking time is significant.
Consider wrapping these calls in spawn_blocking:
let target = target_dir.to_path_buf();
let outputs = entry.output_files.clone();
let verified = tokio::task::spawn_blocking(move || Self::verify_outputs(&target, &outputs)).await??;and similarly for extract_tar_zst.
| .get("size") | ||
| .and_then(|s| s.as_u64()) | ||
| .ok_or_else(|| eyre::eyre!("proofs component missing 'size' field in manifest"))?; | ||
| let archive_base_url = manifest.base_url.as_deref().unwrap_or_else(|| { |
There was a problem hiding this comment.
Nit: fallback base URL from manifest_url can produce an invalid URL when the manifest path has no /
rsplit_once('/') returns None when the manifest URL has no / path separator, causing the fallback to be the entire manifest_url string (e.g. "http://example.com"). Then on line 245 the trailing-slash fixup and Url::join would work, but the intent of the fallback — stripping the manifest.json leaf — would silently produce the wrong base. This is an existing pre-PR edge case and practically unreachable, just noting it.
| } | ||
|
|
||
| #[derive(Debug, Deserialize)] | ||
| struct ProofsDownloadManifest { |
There was a problem hiding this comment.
Nit: Duplicate ProofsStaticManifest type
This file defines its own ProofsStaticManifest (Deserialize-only) at line 115, while base_snapshotter::snapshot already exports a public ProofsStaticManifest with both Serialize and Deserialize. The download crate already depends on base-snapshotter indirectly through reth_cli_commands.
If there's no dependency constraint preventing it, reusing the snapshotter's type would keep the manifest schema in one place. If these crates intentionally don't depend on each other, at minimum consider adding a comment noting the parallel definition.
Review SummaryThe PR cleanly splits the monolithic proofs archive into immutable SST archives (content-addressed, shared across runs) and per-run mutable metadata. The manifest extension approach ( FindingsBlocking I/O on the async runtime ( Duplicate type definition ( Design Observations (non-blocking)
|
bdeca99 to
14e64ae
Compare
|
|
||
| Self::cleanup_outputs(target_dir, &entry.output_files); | ||
| let archive_path = Self::download_archive(&entry, &cache_dir).await?; | ||
| Self::extract_tar_zst(&archive_path, target_dir)?; |
There was a problem hiding this comment.
Issue: extract_tar_zst no longer validates tar entry paths against output_files
The previous implementation used a fixed, single-component filename validated before extraction. Now, extraction uses archive.unpack(target_dir) which writes whatever paths are in the tar, while output_files is only checked after extraction in verify_outputs.
A malicious or corrupted manifest could declare output_files as ["proofs/000001.sst"] but package a tar containing proofs/000001.sst plus additional unexpected files (e.g., proofs/LOCK or other files outside the declared set). These extra files would be written to disk but never verified or cleaned up by cleanup_outputs (which only removes declared output_files).
Consider either:
- Validating that extracted entries match
output_filesexactly (rejecting unexpected entries), or - Extracting into a temporary directory first, verifying, then moving only the declared files.
Review SummaryThis PR splits the monolithic RocksDB proofs snapshot into immutable SST table archives and per-run metadata archives, with BLAKE3-based deduplication on the download side and content-addressed SST deduplication on the upload side. The architecture is sound — immutable SSTs are shared across runs, and only mutable RocksDB metadata is re-downloaded. Findings1. Blocking I/O on the async runtime (existing finding, confirmed) 2. Tar extraction writes undeclared files without cleanup (new finding, inline comment posted) 3. Duplicate Positive observations
|
14e64ae to
fd796cd
Compare
Review Summary —
|
fd796cd to
8c3076e
Compare
Review SummaryThis PR cleanly splits RocksDB proof snapshots into immutable SST archives (shared/deduplicated across runs) and per-run mutable metadata. The manifest extension ( Existing Inline Findings (from prior review pass)The four inline comments already posted cover the key issues:
Additional Observations
Not Block-Production SensitiveThis PR only touches snapshot infrastructure ( |
bfa11b5 to
f65f76d
Compare
f65f76d to
c6323f7
Compare
| /// Parses the optional proofs extension from a manifest JSON document. | ||
| pub fn from_manifest_bytes(bytes: &[u8]) -> Result<Option<Self>> { | ||
| Self::from_named_manifest_bytes(bytes, "proofs_static") | ||
| } |
There was a problem hiding this comment.
Issue: from_manifest_bytes hardcodes "proofs_static" on a generic type
RocksDbStaticManifest::from_manifest_bytes always looks up the "proofs_static" key, but this struct also backs RocksdbStaticManifest (the main RocksDB index type). Calling RocksdbStaticManifest::from_manifest_bytes(bytes) would parse the proofs extension instead of "rocksdb_static" — silently returning the wrong data.
Since both ProofsStaticManifest and RocksdbStaticManifest are type aliases to the same struct, the compiler won't catch this at any call site.
Consider either:
- Removing the convenience method and requiring callers to use
from_named_manifest_byteswith the explicit key, or - Making
ProofsStaticManifestandRocksdbStaticManifestdistinct newtypes (or at least removing the convenience alias on the base struct)
|
<!-- CLAUDE_REVIEW_SUMMARY --> Review SummaryThis PR splits proof snapshots into immutable SST archives and per-run metadata, enabling deduplication of unchanged RocksDB SST tables across snapshot runs. The overall approach is sound — content-addressed archive names, BLAKE3 verification, and reuse of existing remote static files are well-designed. New Findings
Prior Findings (from earlier review runs, still applicable)
|
2 participants
Summary
proofs_staticmanifest extension so existing SSTs are skipped during later snapshot runsbase snapshot download --proofsto restore, verify, and reuse the incremental proof artifactsTesting
cargo test -p base-snapshotter --lib --no-fail-fastcargo test -p base-execution-cli commands::download::tests --no-fail-fastcargo test -p base-execution-cli commands::download::tests::generated_fake_rocksdb_proofs_snapshot_restores_end_to_end -- --nocapturecargo clippy -p base-snapshotter --all-targets -- -D warningscargo clippy -p base-execution-cli --all-targets -- -D warnings