chore: make bootstrap leaner and more complete#1012
Open
jeluard wants to merge 28 commits into
Open
Conversation
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR reworks bootstrap snapshot creation and loading around zstd archives, packaged blocks, optional snapshot metadata, and a new publish subcommand. It also rewires Mithril and db-analyser progress handling, updates docs and Makefile targets, and adds SHA-256 and zstd dependencies. ChangesBootstrap snapshot pipeline rework
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CreateCmd
participant DbAnalyser
participant Archive
participant Manifest
participant PublishCmd
participant AWS
CreateCmd->>DbAnalyser: run_db_analyser(with_progress)
DbAnalyser->>CreateCmd: Progress / SwitchToReplay
CreateCmd->>Archive: write_snapshot_archive(.tar.zst)
CreateCmd->>Manifest: write_manifest(sha256, url)
PublishCmd->>Manifest: load snapshots.json
PublishCmd->>AWS: aws s3 cp archive
PublishCmd->>Manifest: persist updated url
sequenceDiagram
participant Bootstrap
participant SnapshotsJSON
participant Download
participant RocksDBStore
Bootstrap->>SnapshotsJSON: load snapshots.json
Bootstrap->>Download: fetch archive (optional url)
Download->>Download: verify sha256, extract zstd
Bootstrap->>RocksDBStore: import_packaged_blocks(blocks)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Changelog check failedExpected release heading: Errors:
Tip Nothing relevant to add to the changelog? Add |
There was a problem hiding this comment.
Pull request overview
This PR refactors Amaru’s bootstrap snapshot workflow to be CLI-driven (create/publish under amaru snapshot …), changes packaged bootstrap payloads from headers to blocks, and introduces stronger artifact handling (zstd tar archives + optional SHA-256 verification) while standardizing progress reporting via amaru-progress-bar.
Changes:
- Replace the bash-based publishing flow with a Rust CLI subcommand:
amaru snapshot publish. - Switch bootstrap “packaged headers” to “packaged blocks” and import both blocks + headers into RocksDB during bootstrap.
- Move snapshot archive compression from gzip to zstd, and add optional SHA-256 verification for downloaded archives.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/publish-bootstrap-snapshots | Removes the legacy bash publishing script. |
| Makefile | Updates snapshot targets to use the new snapshot create/publish CLI. |
| docs/BOOTSTRAP.md | Updates bootstrap docs to reflect the new snapshot workflow (partially; some references still drift). |
| crates/amaru/tests/cli.rs | Updates CLI alias tests (currently broken: references a non-existent alias). |
| crates/amaru/src/bootstrap.rs | Adds zstd extraction + optional checksum verification; switches bootstrap import from packaged headers to packaged blocks. |
| crates/amaru/src/bin/ledger/cmd/mithril.rs | Wires Mithril download to the shared progress-bar abstraction. |
| crates/amaru/src/bin/amaru/main.rs | Routes the new snapshot publish subcommand. |
| crates/amaru/src/bin/amaru/cmd/snapshot/publish/mod.rs | Implements publishing snapshots to S3 and updating the embedded manifest (has several correctness issues). |
| crates/amaru/src/bin/amaru/cmd/snapshot/mod.rs | Registers the new Publish snapshot subcommand. |
| crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs | Refactors snapshot creation; generates packaged bootstrap blocks and writes/updates a manifest. |
| crates/amaru/src/bin/amaru/cmd/snapshot/create/koios.rs | Adjusts snapshot target struct fields after refactor. |
| crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs | Adds progress reporting around db-analyser execution via progress-bar abstraction. |
| crates/amaru/src/bin/amaru/cmd/snapshot/create/archive.rs | Switches archive output to zstd-compressed tar (.tar.zst). |
| crates/amaru/config/bootstrap/preprod/snapshots.json | Reorders fields; still points at .tar.gz artifacts. |
| crates/amaru/Cargo.toml | Adds sha2 and zstd deps for checksum + zstd extraction. |
| crates/amaru-mithril/src/lib.rs | Removes indicatif usage and exposes immutable block iteration + progress abstraction. |
| crates/amaru-mithril/Cargo.toml | Drops indicatif, adds amaru-progress-bar. |
| crates/amaru-ledger/src/bootstrap.rs | Updates bootstrap error guidance (currently points to a non-existent CLI command). |
| crates/amaru-kernel/src/lib.rs | Re-exports extract_block_header_cbor. |
| crates/amaru-kernel/src/cardano/raw_block.rs | Implements extract_block_header_cbor. |
| Cargo.toml | Adds workspace deps for sha2 and zstd. |
| Cargo.lock | Locks new deps. |
Comments suppressed due to low confidence (2)
crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs:266
- This comment and error message still talk about packaging headers /
bootstrap.headers.json, but the code now packages full blocks intobootstrap.blocks.json. Keeping the old wording will mislead users when this validation triggers.
// Fail fast: every target except the oldest must carry a parent_point, since
// bootstrap packages headers for the 2nd and 3rd snapshots from it. Without
// this, create-bootstrap-snapshots succeeds but bootstrap later fails for want of the
// packaged bootstrap.headers.json.
if let Some(target) = targets.iter().skip(1).find(|target| target.parent_point.is_none()) {
return Err(format!(
"target epoch {} (slot {}) is missing parent_point; required to package bootstrap headers",
target.epoch, target.slot
crates/amaru-ledger/src/bootstrap.rs:64
- This user-facing message suggests
amaru create-bootstrap-snapshots, but that's not a CLI command (the CLI isamaru snapshot create, withcreate-snapshotsas a hidden legacy alias).
minimum_version: amaru_kernel::ProtocolVersion,
},
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/BOOTSTRAP.md (2)
63-63: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDrop the stale
jqrequirement.docs/BOOTSTRAP.md:63should list onlyawsonPATH, and “publish script” should be updated to match the Rustsnapshot publishcommand.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/BOOTSTRAP.md` at line 63, The bootstrap documentation still lists a stale jq dependency for the publish flow. Update the wording in BOOTSTRAP.md so the PATH requirement mentions only aws, and adjust the “publish script” phrasing to refer to the Rust snapshot publish command instead; keep the change aligned with the publish-related documentation text in this section.
30-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSwap this
.tar.gzreference to.tar.zst.docs/BOOTSTRAP.md:30still calls out gzip, but the snapshot archiver already writes zstd tarballs. Tiny fix, big “don’t trip over the old path” energy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/BOOTSTRAP.md` at line 30, The Bootstrap docs still reference the old gzip archive format, while the snapshot archiver now produces zstd tarballs. Update the Archive description in BOOTSTRAP.md to use the current .tar.zst extension and keep the wording aligned with the existing snapshot output naming used by the archiver.crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs (1)
259-268: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "headers" wording — it's blocks now, not headers.
This whole pipeline packages
bootstrap.blocks.json(seePACKAGED_BLOCKS_FILE_NAMEandwrite_packaged_blocks), but the comment still name-dropsbootstrap.headers.jsonand the error yells about packaging "bootstrap headers". Bit of a Blade Runner "more human than human" mismatch — the words no longer match the flesh. Worth a quick tidy so future-you isn't chasing a file that doesn't exist.🩹 Suggested wording fix
- // Fail fast: every target except the oldest must carry a parent_point, since - // bootstrap packages headers for the 2nd and 3rd snapshots from it. Without - // this, create-bootstrap-snapshots succeeds but bootstrap later fails for want of the - // packaged bootstrap.headers.json. + // Fail fast: every target except the oldest must carry a parent_point, since + // bootstrap packages blocks for the 2nd and 3rd snapshots from it. Without + // this, create-bootstrap-snapshots succeeds but bootstrap later fails for want of the + // packaged bootstrap.blocks.json. if let Some(target) = targets.iter().skip(1).find(|target| target.parent_point.is_none()) { return Err(format!( - "target epoch {} (slot {}) is missing parent_point; required to package bootstrap headers", + "target epoch {} (slot {}) is missing parent_point; required to package bootstrap blocks", target.epoch, target.slot )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs` around lines 259 - 268, The stale wording in the parent_point validation message and nearby comment still refers to bootstrap headers, but the snapshot pipeline now packages blocks. Update the explanatory comment and the Err text in create::snapshot::create so they mention bootstrap blocks and match the behavior of write_packaged_blocks and PACKAGED_BLOCKS_FILE_NAME, keeping the failure context accurate for target.epoch and target.slot.
🧹 Nitpick comments (5)
crates/amaru-mithril/src/lib.rs (1)
17-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the extra
extract_block_header_cboralias. The re-export already puts it in scope here, so_extract_block_header_cboris redundant at the call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-mithril/src/lib.rs` around lines 17 - 36, The extra import alias for extract_block_header_cbor is redundant because the public re-export already makes it available in this module. Update the imports in lib.rs to remove the _extract_block_header_cbor alias from the amaru_kernel use list, and adjust any call sites to use extract_block_header_cbor directly so the module only relies on the re-export.crates/amaru-mithril/Cargo.toml (1)
22-29: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDrop the
terminalfeature fromamaru-mithril
crates/amaru-mithrilonly uses theProgressBartrait; the concreteTerminalProgressBarlives in the downstream binary crate. Enablingterminalhere just pulls inindicatiffor every consumer of the library. Enable it where the terminal backend is actually constructed instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru-mithril/Cargo.toml` around lines 22 - 29, Remove the terminal backend feature from amaru-mithril’s dependency setup so the library only depends on the ProgressBar trait and does not pull in indicatif unnecessarily. Update the amaru-progress-bar dependency in the Cargo.toml for amaru-mithril to stop enabling the terminal feature, and leave terminal backend activation to the downstream crate that constructs TerminalProgressBar.crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs (1)
526-529: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sha256_fileslurps the whole archive into RAM.
fs::readloads the entire.tar.zstinto memory before hashing. For the wee preprod snapshots that's grand, but mainnet bootstrap archives can be multiple GB — that's a lot of ballast to carry just to compute a digest, and the same pattern shows up in the bootstrap-side verification. Streaming the file through the hasher in chunks keeps memory flat regardless of size. Not a blocker, just future-proofing before it bites on the big networks.♻️ Streaming variant
-fn sha256_file(path: &Path) -> Result<String, io::Error> { - let bytes = fs::read(path)?; - Ok(hex::encode(Sha256::digest(&bytes))) -} +fn sha256_file(path: &Path) -> Result<String, io::Error> { + let mut file = fs::File::open(path)?; + let mut hasher = Sha256::new(); + io::copy(&mut file, &mut hasher)?; + Ok(hex::encode(hasher.finalize())) +}Can you confirm the expected archive sizes on mainnet so we know how urgent this is?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs` around lines 526 - 529, The sha256_file helper currently reads the entire archive into memory before hashing, which can spike RAM on large mainnet snapshots. Update sha256_file to stream the file through Sha256 in chunks instead of using fs::read, and apply the same pattern anywhere the bootstrap-side verification hashes archives. Use the existing sha256_file function and related snapshot verification code as the touchpoints.crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs (1)
182-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame template on both branches — collapse the
if/else.The restore vs replay arms hand back byte-identical template strings; only
totaldiffers. This is a bit of copy-pasta (a la every Ubisoft tower). Pull the template out and just pick the total.♻️ Suggested dedup
- if s.bar.is_none() { - let (total, template) = if s.restore_total > 0 { - ( - s.restore_total, - "{spinner:.green} {elapsed_precise} [{bar:40.cyan/blue}] {pos}/{len} slots (eta {eta})", - ) - } else { - ( - s.replay_total, - "{spinner:.green} {elapsed_precise} [{bar:40.cyan/blue}] {pos}/{len} slots (eta {eta})", - ) - }; - let factory = s.factory.clone(); - s.bar = Some(factory(total, template)); - } + if s.bar.is_none() { + let total = if s.restore_total > 0 { s.restore_total } else { s.replay_total }; + let factory = s.factory.clone(); + s.bar = Some(factory(total, PROGRESS_BAR_TEMPLATE)); + }(Define
const PROGRESS_BAR_TEMPLATE: &str = "...";once and reuse it in theSwitchToReplayarm too.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs` around lines 182 - 199, The `DbAnalyserLogAction::Progress` branch in `db_analyser.rs` duplicates the same progress bar template in both the restore and replay cases. Refactor the `if s.restore_total > 0` selection so it only chooses the `total` value, and reuse a single shared template constant or local variable for the bar format. Keep the behavior in `DbAnalyserLogAction::Progress` and the related `SwitchToReplay` path consistent by centralizing the template string.crates/amaru/src/bootstrap.rs (1)
411-422: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStream the checksum instead of slurping the whole archive.
std::fs::read(&archive_path)?loads the full snapshot archive into memory and blocks the async runtime. Hash it in chunks, or move the sync I/O intospawn_blockingif you want to keep this shape. The hash compare can stay exact; the snapshot writer already emits lowercase hex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/bootstrap.rs` around lines 411 - 422, Use a streamed checksum for the archive validation in bootstrap.rs instead of reading the entire file into memory. Update the checksum comparison logic inside the snapshot hash check to hash the file in chunks, or move the blocking filesystem read and digest work into spawn_blocking, while keeping the existing BootstrapError::ChecksumMismatch behavior and exact lowercase hex comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/amaru-mithril/src/lib.rs`:
- Around line 447-469: The ImmutableBlocksIter block parsing path in lib.rs
currently swallows malformed entries by continuing when
_extract_block_header_cbor fails, which causes bad immutable blocks to disappear
silently. Update the loop that builds ImmutableBlock values so a failed
_extract_block_header_cbor returns an error instead of skipping the entry, and
make sure the error propagates through ImmutableBlocksIter to
packaged_blocks_for_target so invalid snapshots are reported rather than
partially assembled.
In `@crates/amaru/src/bin/amaru/cmd/snapshot/publish/mod.rs`:
- Around line 98-100: In snapshot publish flow, the archive naming and epoch
selection are both incorrect: `archive_name` in `publish_snapshots` must match
the `.tar.zst` format produced by `create`, and the `object_url` should use that
same suffix so `archive_path.is_file()` finds the real archive. Also update the
default `start_epoch` logic in the same snapshot publish path to choose the
earliest unpublished entry rather than `max()`, so the `start..start+2` window
begins at the manifest instead of skipping ahead to an `epoch not found` error.
- Around line 64-74: The default start_epoch selection in the publish flow is
picking the newest unpublished epoch instead of the oldest one. Update the logic
in the snapshot publish command so the fallback derived from entries uses the
minimum unpublished epoch, not the maximum, while keeping the existing
target_epochs window in order. Use the start_epoch and target_epochs flow in the
snapshot publish module to locate the change.
In `@crates/amaru/tests/cli.rs`:
- Around line 190-193: Rename the test `legacy_create_snapshots_alias_works` so
its name matches what it actually verifies, since it now calls `amaru_help` with
`create-bootstrap-snapshots` rather than checking the old `create-snapshots`
alias. Update the test name to reflect the canonical command behavior and keep
the assertion about `--network` tied to that renamed target, so readers are not
misled about alias/backward-compatibility coverage.
In `@Makefile`:
- Around line 82-84: The publish-bootstrap-snapshots flow is not honoring
AMARU_EPOCH, so the snapshot publish command still falls back to the latest
manifest epoch. Update the publish-bootstrap-snapshots target to pass the
environment value through to the cargo invocation, and/or bind Args.epoch in
amaru::cmd::snapshot::publish::mod to amaru::env_vars::EPOCH so the existing
--epoch handling picks it up. Use the publish-bootstrap-snapshots Makefile
target and the snapshot publish Args struct as the main places to wire this
through.
---
Outside diff comments:
In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs`:
- Around line 259-268: The stale wording in the parent_point validation message
and nearby comment still refers to bootstrap headers, but the snapshot pipeline
now packages blocks. Update the explanatory comment and the Err text in
create::snapshot::create so they mention bootstrap blocks and match the behavior
of write_packaged_blocks and PACKAGED_BLOCKS_FILE_NAME, keeping the failure
context accurate for target.epoch and target.slot.
In `@docs/BOOTSTRAP.md`:
- Line 63: The bootstrap documentation still lists a stale jq dependency for the
publish flow. Update the wording in BOOTSTRAP.md so the PATH requirement
mentions only aws, and adjust the “publish script” phrasing to refer to the Rust
snapshot publish command instead; keep the change aligned with the
publish-related documentation text in this section.
- Line 30: The Bootstrap docs still reference the old gzip archive format, while
the snapshot archiver now produces zstd tarballs. Update the Archive description
in BOOTSTRAP.md to use the current .tar.zst extension and keep the wording
aligned with the existing snapshot output naming used by the archiver.
---
Nitpick comments:
In `@crates/amaru-mithril/Cargo.toml`:
- Around line 22-29: Remove the terminal backend feature from amaru-mithril’s
dependency setup so the library only depends on the ProgressBar trait and does
not pull in indicatif unnecessarily. Update the amaru-progress-bar dependency in
the Cargo.toml for amaru-mithril to stop enabling the terminal feature, and
leave terminal backend activation to the downstream crate that constructs
TerminalProgressBar.
In `@crates/amaru-mithril/src/lib.rs`:
- Around line 17-36: The extra import alias for extract_block_header_cbor is
redundant because the public re-export already makes it available in this
module. Update the imports in lib.rs to remove the _extract_block_header_cbor
alias from the amaru_kernel use list, and adjust any call sites to use
extract_block_header_cbor directly so the module only relies on the re-export.
In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs`:
- Around line 182-199: The `DbAnalyserLogAction::Progress` branch in
`db_analyser.rs` duplicates the same progress bar template in both the restore
and replay cases. Refactor the `if s.restore_total > 0` selection so it only
chooses the `total` value, and reuse a single shared template constant or local
variable for the bar format. Keep the behavior in
`DbAnalyserLogAction::Progress` and the related `SwitchToReplay` path consistent
by centralizing the template string.
In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs`:
- Around line 526-529: The sha256_file helper currently reads the entire archive
into memory before hashing, which can spike RAM on large mainnet snapshots.
Update sha256_file to stream the file through Sha256 in chunks instead of using
fs::read, and apply the same pattern anywhere the bootstrap-side verification
hashes archives. Use the existing sha256_file function and related snapshot
verification code as the touchpoints.
In `@crates/amaru/src/bootstrap.rs`:
- Around line 411-422: Use a streamed checksum for the archive validation in
bootstrap.rs instead of reading the entire file into memory. Update the checksum
comparison logic inside the snapshot hash check to hash the file in chunks, or
move the blocking filesystem read and digest work into spawn_blocking, while
keeping the existing BootstrapError::ChecksumMismatch behavior and exact
lowercase hex comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 749c606d-9670-46a6-b255-5f3e0b1fc628
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlMakefilecrates/amaru-kernel/src/cardano/raw_block.rscrates/amaru-kernel/src/lib.rscrates/amaru-ledger/src/bootstrap.rscrates/amaru-mithril/Cargo.tomlcrates/amaru-mithril/src/lib.rscrates/amaru/Cargo.tomlcrates/amaru/config/bootstrap/preprod/snapshots.jsoncrates/amaru/src/bin/amaru/cmd/snapshot/create/archive.rscrates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rscrates/amaru/src/bin/amaru/cmd/snapshot/create/koios.rscrates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rscrates/amaru/src/bin/amaru/cmd/snapshot/mod.rscrates/amaru/src/bin/amaru/cmd/snapshot/publish/mod.rscrates/amaru/src/bin/amaru/main.rscrates/amaru/src/bin/ledger/cmd/mithril.rscrates/amaru/src/bootstrap.rscrates/amaru/tests/cli.rsdocs/BOOTSTRAP.mdscripts/publish-bootstrap-snapshots
💤 Files with no reviewable changes (2)
- scripts/publish-bootstrap-snapshots
- crates/amaru/src/bin/amaru/cmd/snapshot/create/koios.rs
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs (1)
287-300: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCarry the replay delta when switching bars.
Line 291 returns
SwitchToReplaybefore calculating the delta for the line that crossesstart_slot, so that progress update is dropped; the test on Lines 433-435 also expects aProgressaction that this branch can’t return. Tiny warp-pipe bug: the bar changes worlds, but Mario’s first jump doesn’t count.🐛 Proposed fix
pub(super) enum DbAnalyserLogAction { PassThrough, Suppress, Progress { done: Option<u64> }, - SwitchToReplay, + SwitchToReplay { done: u64 }, } @@ - DbAnalyserLogAction::SwitchToReplay => { + DbAnalyserLogAction::SwitchToReplay { done } => { if let Some(s) = shared.as_ref() { if let Ok(mut s) = s.lock() { if let Some(pb) = s.bar.take() { pb.clear(); } @@ s.bar = Some(factory( total, "{spinner:.green} {elapsed_precise} [{bar:40.green}] {pos}/{len} slots (eta {eta})", )); + if let Some(pb) = s.bar.as_ref() { + pb.tick(done as usize); + } } } continue; } @@ fn progress_action(&mut self, _elapsed_secs: f64, current_slot: Slot) -> DbAnalyserLogAction { + let current = current_slot.as_u64(); + let target = self.target_slot.as_u64(); + let capped = current.min(target); + if self.in_restore_phase && !self.is_restoring_resume_snapshot(current_slot) { self.in_restore_phase = false; self.last_done = self.start_slot.as_u64(); - return DbAnalyserLogAction::SwitchToReplay; + let delta = capped.saturating_sub(self.last_done); + self.last_done = capped; + return DbAnalyserLogAction::SwitchToReplay { done: delta }; } - let current = current_slot.as_u64(); - let target = self.target_slot.as_u64(); - let capped = current.min(target); let delta = capped.saturating_sub(self.last_done); self.last_done = capped; DbAnalyserLogAction::Progress { done: Some(delta) } @@ assert_eq!( relay.handle_line("[32.0s] BlockNo 42 SlotNo 134100000"), - DbAnalyserLogAction::Progress { done: Some(7242) } + DbAnalyserLogAction::SwitchToReplay { done: 7242 } );Also applies to: 430-435
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs` around lines 287 - 300, The `progress_action` branch in `DbAnalyserLogAction` drops the first progress delta when crossing from restore to replay because it returns `SwitchToReplay` before computing `done`. Update `progress_action` so the transition at `start_slot` preserves and emits that delta (and only switches modes after accounting for it), using `last_done`, `start_slot`, and the `SwitchToReplay` path as the key points to adjust. This should also satisfy the expected `Progress` action in the related test case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs`:
- Around line 287-300: The `progress_action` branch in `DbAnalyserLogAction`
drops the first progress delta when crossing from restore to replay because it
returns `SwitchToReplay` before computing `done`. Update `progress_action` so
the transition at `start_slot` preserves and emits that delta (and only switches
modes after accounting for it), using `last_done`, `start_slot`, and the
`SwitchToReplay` path as the key points to adjust. This should also satisfy the
expected `Progress` action in the related test case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb1b4b46-d6e3-4567-b581-b6851b48eba6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/amaru-mithril/src/lib.rscrates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rscrates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rscrates/amaru/src/bin/amaru/cmd/snapshot/publish/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/amaru/src/bin/amaru/cmd/snapshot/publish/mod.rs
- crates/amaru-mithril/src/lib.rs
- crates/amaru/src/bin/amaru/cmd/snapshot/create/mod.rs
jeluard
changed the title
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs (2)
166-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPull that progress template out into a
constso the two bars can't drift apart.The exact same format string is hand-typed at Line 176 and Line 190. Right now the restore and replay bars are twins, but if someone tweaks one and forgets the other you get a mismatched pair — a bit like recasting Aunt May halfway through a Spider-Man trilogy. A shared
constkeeps 'em in lockstep.♻️ Suggested tidy-up
const DB_ANALYSER_PROGRESS_TEMPLATE: &str = "{spinner:.green} {elapsed_precise} [{bar:40.green}] {pos}/{len} slots (eta {eta})";- s.bar = Some(factory( - total, - "{spinner:.green} {elapsed_precise} [{bar:40.green}] {pos}/{len} slots (eta {eta})", - )); + s.bar = Some(factory(total, DB_ANALYSER_PROGRESS_TEMPLATE));- s.bar = Some(factory( - total, - "{spinner:.green} {elapsed_precise} [{bar:40.green}] {pos}/{len} slots (eta {eta})", - )); + s.bar = Some(factory(total, DB_ANALYSER_PROGRESS_TEMPLATE));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs` around lines 166 - 198, The progress bar format string is duplicated in the `DbAnalyserLogAction` handling, which risks the restore and replay bars drifting apart. Extract the shared template into a single `const` in `db_analyser.rs` and use it from both the `Replay`/restore branch and the `Progress` branch inside the log handling code so `factory(...)` always receives the same string.
251-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe old
Report(...)tests got binned — worth resurrecting coverage for the new action logic.The restore→replay transition, the
saturating_subdelta maths, and thecapped = current.min(target)clamp inprogress_actionare exactly the sort of edge-casey logic that loves to sneak a regression past you (one dropped slot and the bar's off like a plot hole in Prometheus). With the previous test module gone, this path is now flying blind.As per coding guidelines ("Write unit tests, property tests using proptest ... prefer simulation tests for complex logic"), a few cases would go a long way: the initial restore phase, the switch, out-of-order/backwards slots (delta stays
0), and slots pasttarget_slot.Want me to draft a
#[cfg(test)]module (plus a proptest for the delta/clamp invariants) to fill the gap?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs` around lines 251 - 299, Add tests for the new DbAnalyserLogAction flow in db_analyser::handle_line and progress_action, since the old Report coverage was removed. Cover the restore-to-replay transition in progress_action when in_restore_phase is true, ensure capped = current.min(target) is exercised for slots past target_slot, and verify saturating_sub keeps delta at 0 for backward/out-of-order slots. Include a small #[cfg(test)] module around DbAnalyserLogAction, should_report_progress, and progress_action, and add a proptest or simulation-style test to lock in these invariants.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rs`:
- Around line 166-198: The progress bar format string is duplicated in the
`DbAnalyserLogAction` handling, which risks the restore and replay bars drifting
apart. Extract the shared template into a single `const` in `db_analyser.rs` and
use it from both the `Replay`/restore branch and the `Progress` branch inside
the log handling code so `factory(...)` always receives the same string.
- Around line 251-299: Add tests for the new DbAnalyserLogAction flow in
db_analyser::handle_line and progress_action, since the old Report coverage was
removed. Cover the restore-to-replay transition in progress_action when
in_restore_phase is true, ensure capped = current.min(target) is exercised for
slots past target_slot, and verify saturating_sub keeps delta at 0 for
backward/out-of-order slots. Include a small #[cfg(test)] module around
DbAnalyserLogAction, should_report_progress, and progress_action, and add a
proptest or simulation-style test to lock in these invariants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a714b1fb-39f6-4f21-a4f8-5201f070367e
📒 Files selected for processing (4)
crates/amaru-ledger/src/bootstrap.rscrates/amaru/src/bin/amaru/cmd/snapshot/create/db_analyser.rscrates/amaru/src/bin/amaru/cmd/snapshot/publish/mod.rscrates/amaru/src/bootstrap.rs
✅ Files skipped from review due to trivial changes (1)
- crates/amaru-ledger/src/bootstrap.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/amaru/src/bin/amaru/cmd/snapshot/publish/mod.rs
- crates/amaru/src/bootstrap.rs
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 38-43: The changelog entry is out of release order because the
unreleased heading currently starts at v10.10.20260716 instead of the expected
latest unreleased version. Update the heading and keep the new amaru note under
the existing unreleased section for v10.10.20260709, or rename the heading only
if this newer release cut is intended, so the top-level release order remains
newest-first.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39c4edd1-721f-47b0-a6eb-8edc34538f51
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.mdMakefilecrates/amaru-kernel/src/lib.rscrates/amaru-ledger/src/bootstrap.rs
💤 Files with no reviewable changes (1)
- Makefile
✅ Files skipped from review due to trivial changes (2)
- crates/amaru-kernel/src/lib.rs
- crates/amaru-ledger/src/bootstrap.rs
KtorZ
requested changes
Jul 13, 2026
jeluard
marked this pull request as draft
July 13, 2026 12:06
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
jeluard
force-pushed
the
jeluard/all-blocks
branch
from
July 14, 2026 14:28
315c5d2 to
7187e26
Compare
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
jeluard
force-pushed
the
jeluard/all-blocks
branch
from
July 14, 2026 14:35
326cf88 to
5ca31dc
Compare
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
jeluard
force-pushed
the
jeluard/all-blocks
branch
from
July 14, 2026 15:07
c9d7d02 to
9286482
Compare
jeluard
marked this pull request as ready for review
July 14, 2026 15:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overall simplification of the bootstrap process. Noticeably, do not use an embedded set of
snapshot.jsonbut rely on R2 hosted index file (anonymous connections can't list R2 buckets).Summary by CodeRabbit
.tar.zst.