fix: don't fail rollback inside the volatile ledger#801
Conversation
WalkthroughThis PR refactors the ledger state rollback mechanism by moving error context from the volatile layer upward to the state layer. VolatileDB::rollback_to now returns raw point errors, while State::rollback_to enriches them with tip awareness. A new immutable_tip() helper and updated tip() logic support dual-source tip resolution. Comprehensive state integration tests validate the updated behavior across valid and invalid rollback targets. ChangesLedger Rollback Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
etorreborre
force-pushed
the
etorreborre/fix/ledger-rollback
branch
from
810189f to
e660c41
Compare
|
|
||
| # Internal dependencies ───────────────────────────────────────────────────────┐ | ||
| amaru-kernel = { workspace = true, features = ["test-utils"] } | ||
| amaru-stores.workspace = true |
There was a problem hiding this comment.
It's unfortunate that we introduce this dependency just for a test, especially given that MemoryStore is pretty broken and will be removed soon AFAIK.
Maybe there is a way to test this w/o it?
There was a problem hiding this comment.
I don't see anything immediate. It also seems to me that having a reliable in-memory ledger store is a useful thing to have. One way to validate it is to do what we do for the ChainStore and run the same tests across the in-memory version and the RocksDB version.
There was a problem hiding this comment.
It definitely is useful; we discussed it with Josh last week. For now though the plan is to clean up the old in memory store implementation that is obsolete.
There was a problem hiding this comment.
Just for context: we have #795 to track removing the MemoryStore (which I misremembered as InMemoryStore)
There was a problem hiding this comment.
@etorreborre no. The rollback logic here used to be quite straightforward and after a few successive updates from different people, I find it very confusing. I'll try to make a quick pass on it.
etorreborre
force-pushed
the
etorreborre/fix/ledger-rollback
branch
from
e660c41 to
5392be5
Compare
Signed-off-by: Eric Torreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/fix/ledger-rollback
branch
from
5392be5 to
c737792
Compare
Codecov Report❌ Patch coverage is
... and 55 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
KtorZ
force-pushed
the
etorreborre/fix/ledger-rollback
branch
from
4d488ba to
bb71af7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/amaru-ledger/tests/state_rollback.rs (2)
26-97: ⚡ Quick winNice coverage — one extra scenario worth bolting on.
The five tests cover before/after the volatile front, in-range with bad hash/slot, and a happy-path common ancestor — chef's kiss for the volatile-only space. What's missing is the "rollback exactly to the immutable tip while the volatile is non-empty" case, which is the precise scenario this PR's title is fixing ("don't fail rollback inside the volatile ledger"). Right now the suite verifies that the volatile bounds work, but not that crossing the volatile/immutable boundary behaves sanely (see related comment on
state.rs::rollback_to). Worth a follow-up test once the boundary semantics are nailed down — like Geralt always says, "evil is evil", and untested edge cases are evil 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-ledger/tests/state_rollback.rs` around lines 26 - 97, Add a test that verifies rolling back exactly to the immutable tip succeeds when the volatile window is non-empty: using make_state(), advance to an earlier point that becomes the immutable tip (e.g. forward_to(&mut state, point(100,1), 1)), then advance further into volatile (e.g. forward_to(&mut state, point(200,2), 2)), call state.rollback_to(&immutable_tip) and assert it returns Ok(()) and that *state.tip() == immutable_tip; locate this behavior around the existing tests using point, forward_to, state.rollback_to, and state.tip to ensure the boundary-crossing case is covered.
48-95: ⚡ Quick win
dbg!()left in committed tests — pull the handbrake before merge.The
dbg!(state.rollback_to(&to))calls (lines 49, 64, 79, 93) are dead handy when iterating locally, but once merged they'll print to stderr on everycargo testrun, polluting CI logs and making clean test output a bit less of a "she'll be right".assert!already gives a decent failure message; if you want richer diagnostics on failure, lean onassert_matches!(or a custom message onassert!) instead.♻️ Proposed cleanup (one example, apply to all four sites)
assert!(matches!( - dbg!(state.rollback_to(&to)), + state.rollback_to(&to), Err(BackwardError::UnknownRollbackPoint { rollback_point, .. }) if rollback_point == to, ));🤖 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-ledger/tests/state_rollback.rs` around lines 48 - 95, Remove the debug-print wrappers left in tests: don't call dbg!(state.rollback_to(&to)); instead invoke state.rollback_to(&to) directly inside the assert/matches. Update the four tests (eg. rollback_within_volatile_but_unknown_hash_is_rejected, rollback_within_volatile_but_unknown_slot_is_rejected, the earlier similar test, and rollback_after_volatile_front_is_rejected) so assertions match on the returned Result/BackwardError variants (UnknownRollbackPoint or RollbackPointInFuture) without printing to stderr; if you want richer failure context, replace assert!(matches!(...)) with assert_matches! or add a custom assert message, but remove all dbg! wrappers around state.rollback_to(&to).crates/amaru-ledger/src/state/volatile_db.rs (1)
443-455: ⚡ Quick winTest name says "succeeds", body says "stop right there, criminal scum".
The function name
test_rollback_to_slot_between_elements_succeedspromises a happy path, but the assertionresult.unwrap_err()says otherwise — it's actually verifying that rolling back to slot 25 (which sits between known elements 20 and 30) gets bounced asErr(&rollback_point). Bit of a head-scratcher when a future you (or someone else) goes scrolling through test output. A quick rename keeps the intent crystal.♻️ Proposed rename
- fn test_rollback_to_slot_between_elements_succeeds() { + fn test_rollback_to_slot_between_elements_fails() {🤖 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-ledger/src/state/volatile_db.rs` around lines 443 - 455, Rename the misleading test function test_rollback_to_slot_between_elements_succeeds to reflect that an error is expected (e.g., test_rollback_to_slot_between_elements_fails or test_rollback_to_slot_between_elements_returns_err), updating the function name accordingly in the test; keep the test body and assertions (result.unwrap_err() and db.len() checks) unchanged so the intent and behavior remain the same.crates/amaru-ledger/src/state.rs (1)
243-248: 💤 Low valueDoc comment got bumped behind the bouncer — let it through first.
The doc comment on line 245 sits after the
#[expect(...)]attributes and before thepub fn. Rustfmt and most Rust style guides put doc comments at the very top so tooling and humans alike see the docs as the "front door" of the item. Tiny fix, big readability win.♻️ Proposed reorder
- #[expect(clippy::panic)] - #[expect(clippy::unwrap_used)] - /// Tip of the immutable db (i.e. farthest point we can ever rollback to). - pub fn immutable_tip(&self) -> Point { + /// Tip of the immutable db (i.e. farthest point we can ever rollback to). + #[expect(clippy::panic)] + #[expect(clippy::unwrap_used)] + pub fn immutable_tip(&self) -> Point { self.stable.lock().unwrap().tip().unwrap_or_else(|e| panic!("no tip found in stable db: {e:?}")) }🤖 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-ledger/src/state.rs` around lines 243 - 248, Move the doc comment so it precedes the attribute annotations for the immutable_tip method: place the "/// Tip of the immutable db..." doc comment above the two #[expect(clippy::...)] attributes that decorate pub fn immutable_tip(&self) -> Point, keeping the attributes and the function body (self.stable.lock().unwrap().tip().unwrap_or_else(|e| panic!("no tip found in stable db: {e:?}"))) unchanged; this restores the doc comment to the top of the item for proper formatting and tooling visibility.
🤖 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-ledger/src/state.rs`:
- Around line 666-704: In rollback_to (in state.rs) handle the startup case
where to == immutable_tip() but volatile_tip() is Some by draining the volatile
DB and returning Ok(()) instead of calling self.volatile.rollback_to(to);
specifically, after computing immutable_tip and tip, detect if &immutable_tip ==
to and volatile_tip().is_some(), then call the volatile's method that
clears/drains all in-flight entries (use the VolatileDB API method that empties
it) and return Ok(()); this prevents VolatileDB::rollback_to from returning
UnknownRollbackPoint when the correct semantics are to drop the volatile state
and accept the immutable tip.
In `@crates/amaru-ledger/tests/state_rollback.rs`:
- Line 125: Update the inline comment that currently reads "Forward the ldeger
to a given point" to correct the typo to "Forward the ledger to a given point";
locate and edit the comment string in the test module (state_rollback.rs) where
that exact phrase appears so the symbol/context remains unchanged.
---
Nitpick comments:
In `@crates/amaru-ledger/src/state.rs`:
- Around line 243-248: Move the doc comment so it precedes the attribute
annotations for the immutable_tip method: place the "/// Tip of the immutable
db..." doc comment above the two #[expect(clippy::...)] attributes that decorate
pub fn immutable_tip(&self) -> Point, keeping the attributes and the function
body (self.stable.lock().unwrap().tip().unwrap_or_else(|e| panic!("no tip found
in stable db: {e:?}"))) unchanged; this restores the doc comment to the top of
the item for proper formatting and tooling visibility.
In `@crates/amaru-ledger/src/state/volatile_db.rs`:
- Around line 443-455: Rename the misleading test function
test_rollback_to_slot_between_elements_succeeds to reflect that an error is
expected (e.g., test_rollback_to_slot_between_elements_fails or
test_rollback_to_slot_between_elements_returns_err), updating the function name
accordingly in the test; keep the test body and assertions (result.unwrap_err()
and db.len() checks) unchanged so the intent and behavior remain the same.
In `@crates/amaru-ledger/tests/state_rollback.rs`:
- Around line 26-97: Add a test that verifies rolling back exactly to the
immutable tip succeeds when the volatile window is non-empty: using
make_state(), advance to an earlier point that becomes the immutable tip (e.g.
forward_to(&mut state, point(100,1), 1)), then advance further into volatile
(e.g. forward_to(&mut state, point(200,2), 2)), call
state.rollback_to(&immutable_tip) and assert it returns Ok(()) and that
*state.tip() == immutable_tip; locate this behavior around the existing tests
using point, forward_to, state.rollback_to, and state.tip to ensure the
boundary-crossing case is covered.
- Around line 48-95: Remove the debug-print wrappers left in tests: don't call
dbg!(state.rollback_to(&to)); instead invoke state.rollback_to(&to) directly
inside the assert/matches. Update the four tests (eg.
rollback_within_volatile_but_unknown_hash_is_rejected,
rollback_within_volatile_but_unknown_slot_is_rejected, the earlier similar test,
and rollback_after_volatile_front_is_rejected) so assertions match on the
returned Result/BackwardError variants (UnknownRollbackPoint or
RollbackPointInFuture) without printing to stderr; if you want richer failure
context, replace assert!(matches!(...)) with assert_matches! or add a custom
assert message, but remove all dbg! wrappers around state.rollback_to(&to).
🪄 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: cd242375-7bcb-4d70-94fd-df0e5701a7ba
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/amaru-ledger/Cargo.tomlcrates/amaru-ledger/src/state.rscrates/amaru-ledger/src/state/volatile_db.rscrates/amaru-ledger/tests/state_rollback.rs
| pub fn rollback_to(&mut self, to: &Point) -> Result<(), BackwardError> { | ||
| let _span = | ||
| trace_span!(amaru_observability::amaru::ledger::state::ROLL_BACKWARD, rollback_point = to.to_string()); | ||
| let _guard = _span.enter(); | ||
|
|
||
| // NOTE: This happens typically on start-up; The consensus layer will typically ask us to | ||
| // rollback to the last known point, which ought to be the tip of the database. | ||
| if self.volatile.is_empty() && self.tip().as_ref() == to { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if self.tip().as_ref() > to { | ||
| return Err(BackwardError::RollbackPointBeforeTip { rollback_point: *to, tip: self.tip().into_owned() }); | ||
| } | ||
|
|
||
| if self.volatile.is_empty() && self.tip().as_ref() < to { | ||
| return Err(BackwardError::RollbackPointInFuture(*to)); | ||
| } | ||
| trace_span!(amaru_observability::amaru::ledger::state::ROLL_BACKWARD, rollback_point = to.to_string()).in_scope( | ||
| || { | ||
| // NOTE: Rolling back to the tip of the immutable | ||
| // | ||
| // On start-up where the consensus layer will typically ask the ledger to rollback | ||
| // to the last known point, which ought to be the tip of the (immutable) database. | ||
| // | ||
| // Said differently, if the volatile db is empty, the rollback point MUST be the | ||
| // tip of the immutable. | ||
| let tip = match self.volatile_tip() { | ||
| Some(volatile_tip) => volatile_tip.point(), | ||
| None => { | ||
| let immutable_tip = self.immutable_tip(); | ||
|
|
||
| if &immutable_tip != to { | ||
| return Err(BackwardError::UnknownRollbackPoint { | ||
| rollback_point: *to, | ||
| tip: immutable_tip, | ||
| }); | ||
| } | ||
|
|
||
| immutable_tip | ||
| } | ||
| }; | ||
|
|
||
| if let Some(last) = self.volatile.view_back() | ||
| && last.anchor.0.point() < *to | ||
| { | ||
| return Err(BackwardError::RollbackPointInFuture(*to)); | ||
| } | ||
| // Would still be caught by the next check, but this is a special case for which we | ||
| // can provide a better error. | ||
| if to > &tip { | ||
| return Err(BackwardError::RollbackPointInFuture { rollback_point: *to, tip }); | ||
| } | ||
|
|
||
| self.volatile.rollback_to(to, |point| BackwardError::UnknownRollbackPoint(*point)) | ||
| self.volatile.rollback_to(to).map_err(|rollback_point| BackwardError::UnknownRollbackPoint { | ||
| rollback_point: *rollback_point, | ||
| tip, | ||
| }) | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for all callers of rollback_to on the ledger State to understand expected rollback targets
rg -nP --type=rust -C3 '\.rollback_to\s*\('
# Check whether immutable-tip rollback is exercised anywhere
rg -nP --type=rust -C3 'immutable_tip|RollbackPointBeforeTip'Repository: pragma-org/amaru
Length of output: 9591
🏁 Script executed:
# First, let's look at the test setup in state_rollback.rs to understand how volatile/immutable are structured
head -100 crates/amaru-ledger/tests/state_rollback.rs
# Then check the VolatileDB::rollback_to implementation to confirm the short-circuit
rg -A 30 'impl.*VolatileDB' crates/amaru-ledger/src/state/volatile_db.rs | head -200
# Look for the rollback_to method specifically in VolatileDB
rg -A 20 'fn rollback_to' crates/amaru-ledger/src/state/volatile_db.rsRepository: pragma-org/amaru
Length of output: 5184
🏁 Script executed:
# Get complete rollback_to implementation - need more lines
rg -A 50 'fn rollback_to' crates/amaru-ledger/src/state/volatile_db.rs | head -80
# Check the make_state helper
rg -A 20 'fn make_state' crates/amaru-ledger/tests/state_rollback.rs
# Check how forward_to works
rg -A 15 'fn forward_to' crates/amaru-ledger/tests/state_rollback.rsRepository: pragma-org/amaru
Length of output: 3169
🏁 Script executed:
# Check if there are any tests explicitly testing immutable_tip rollback
rg -A 10 'immutable_tip.*rollback|rollback.*immutable_tip' crates/amaru-ledger/tests/
# Look for any test setup that creates non-empty volatile + immutable
rg -B 5 -A 10 'rollback_to.*immutable' crates/amaru-ledger/
# Search more broadly for how the consensus layer or other callers invoke rollback_to
rg -B 5 -A 5 'rollback_to' crates/amaru-ledger/src/block_validator.rsRepository: pragma-org/amaru
Length of output: 458
🏁 Script executed:
# One more check: search for any test setup or scenario that matches the edge case
# Look for tests that set up non-empty volatile and try to rollback to initial point
rg -B 10 -A 10 'volatile.*immutable|immutable.*volatile' crates/amaru-ledger/tests/state_rollback.rs
# Check if there's any documentation about when rollback to immutable happens
rg -C 5 'consensus.*rollback|resync' crates/amaru-ledger/Repository: pragma-org/amaru
Length of output: 1132
Drain the volatile when rolling back to immutable tip, even with blocks in flight.
The consensus layer does ask to roll back to the immutable tip on startup (as the code comment right above confirms), but currently it fails if the volatile DB isn't empty. The VolatileDB::rollback_to will short-circuit with UnknownRollbackPoint when target_slot < first.anchor.0.slot(), even though draining the volatile entirely is semantically valid. The volatile-empty branch already handles this correctly, so it's inconsistent (and a bit like Aragorn at the Black Gate—the call is legit even if one gate doesn't recognise it).
Add a fallback: when to == immutable_tip() and volatile_tip() is Some, drain the volatile and return Ok(()) instead of delegating to self.volatile.rollback_to(to).
🤖 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-ledger/src/state.rs` around lines 666 - 704, In rollback_to (in
state.rs) handle the startup case where to == immutable_tip() but volatile_tip()
is Some by draining the volatile DB and returning Ok(()) instead of calling
self.volatile.rollback_to(to); specifically, after computing immutable_tip and
tip, detect if &immutable_tip == to and volatile_tip().is_some(), then call the
volatile's method that clears/drains all in-flight entries (use the VolatileDB
API method that empties it) and return Ok(()); this prevents
VolatileDB::rollback_to from returning UnknownRollbackPoint when the correct
semantics are to drop the volatile state and accept the immutable tip.
| ) | ||
| } | ||
|
|
||
| /// Forward the ldeger to a given point |
There was a problem hiding this comment.
Typo: "ldeger" → "ledger".
The Konami code of typos. One swift edit and we're back on the rails.
✏️ Proposed fix
-/// Forward the ldeger to a given point
+/// Forward the ledger to a given point📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Forward the ldeger to a given point | |
| /// Forward the ledger to a given point |
🤖 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-ledger/tests/state_rollback.rs` at line 125, Update the inline
comment that currently reads "Forward the ldeger to a given point" to correct
the typo to "Forward the ledger to a given point"; locate and edit the comment
string in the test module (state_rollback.rs) where that exact phrase appears so
the symbol/context remains unchanged.
4 participants
We can't currently rollback the ledger to a previous volatile state.
Summary by CodeRabbit
Improvements
Tests