Skip to content

fix(replay): repair genesis history for v2 (StorageV2) archives#24

Open
procdump wants to merge 6 commits into
raylsnetwork:mainfrom
procdump:fix/storages-history2
Open

fix(replay): repair genesis history for v2 (StorageV2) archives#24
procdump wants to merge 6 commits into
raylsnetwork:mainfrom
procdump:fix/storages-history2

Conversation

@procdump

@procdump procdump commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Background: what is StorageV2?

reth has two on-disk storage layouts, chosen per node at datadir init and recorded in DB metadata (--storage.v2 / StorageSettings), not by any chain hardfork:

  • v1 (legacy): everything in MDBX, keyed by the plain address/slot.
  • v2 (StorageV2): hashed state tables are canonical, the history indices (AccountsHistory, StoragesHistory) live in RocksDB, receipts/changesets in static files. Storage is keyed by keccak256(slot).

StorageV2 doesn't change consensus, block validity, or the state root — only the on-disk layout. Rayls production snapshots (and archives built from them) are v2.

The problem

On v2 archives, historical reads of genesis-seeded storage return 0x0 — e.g. eth_getStorageAt at an early block for the static validator set, or eth_getCode for immutable system contracts.

Root cause is in reth's v2 genesis init (init_genesis_with_settings), which writes genesis state and genesis history through separate paths and keys them inconsistently:

  • Storage historyinsert_storage_history keys StoragesHistory by the plain slot, but the v2 read looks it up by keccak256(slot) (StorageSlotKey::to_hashed). The read never finds the genesis entry → NotYetWritten0x0. Present in every v2 genesis. (Genesis state is fine: insert_genesis_hashes writes HashedStorages under keccak256(slot) — which is why current-block reads work but historical ones don't.)
  • Account history — write and read both use the plain address, so there's no mismatch; genesis account history is correct. It only breaks via IndexAccountHistoryStage's first-sync clear, which rayls-replay never runs.

Genesis has no changeset (it's the initial state, not a change from a prior block), so the normal changeset-driven hashed indexing can't reconstruct genesis history — it must be repaired explicitly. This is fundamentally an upstream reth (v1.11.0) defect; we pin reth by tag and can't patch it, so the repair lives in the archive tooling.

What's changed

fix_genesis_history (storage re-key — the necessary fix): for each genesis (address, hashed slot), read-merges the genesis block into the earliest existing shard (via storage_history_shards), never overwriting the open shard. This preserves the post-genesis history of genesis slots that are also mutated later (balances, registry state) and places block 0 where an early-block read looks. When prepending would overflow a full shard (a hot genesis slot with ≥ NUM_OF_INDICES_IN_SHARD = 2000 post-genesis changes), the slot's shards are re-chunked into ≤2000 pieces (stale keys deleted, cascades handled). Per-slot prefix reads, no full-table scan; idempotent.

fix_genesis_account_history (account seed — defensive): re-seeds a block-0 entry for ≤1-shard genesis accounts via read-merge; multi-shard accounts (post-genesis changesets) are left untouched. A no-op on replay-built archives; included as insurance for cleared-history archives.

Wiring in rayls-replay: both run automatically at the end of a normal replay, so freshly built archives boot correct with no extra step. --fix_genesis_history remains for repairing archives built before this (e.g. current mainnet). Both no-op on v1 archives and on already-correct entries. open_db is deferred past the maintenance/unwind early-return paths so those modes don't open the consensus DB.

Test-network tooling: DISABLE_PRUNING env var runs the local observer as a full archive (drops reth's --full, which otherwise prunes account/storage history beyond ~10k blocks), so its datadir can seed rayls-replay. Validators unaffected.

Testing

Full-DB unit tests (cargo test -p rayls-execution-evm --features archive-replay) build a v2 storage stack, run canonical genesis init, and assert re-keying end to end:

  • bug precondition (hashed key lacks the genesis entry, plain key carries it)
  • immutable slot → [0]
  • single-shard mutable slot → [0, 5] (post-genesis change not clobbered)
  • full first shard → re-chunk: 4001 blocks split into three ≤2000 shards, block 0 in the earliest, open shard keyed u64::MAX, all blocks preserved
  • idempotent second run
  • account path: no-op when correct, re-seed after a simulated clear, skip multi-shard

Verified end to end on a local 4-node network: rebuilt an archive via rayls-replay and confirmed eth_getStorageAt(RLS_PROXY, ERC1967_IMPL_SLOT, --block 1) returns the implementation address, with a negative control (the un-fixed source datadir returns 0x0).

Notes for reviewers

  • This PR is another approach to Replay archival genesis - storage re-key + account history seed #18. Eventually one of the would land.
  • The fix is archive-replay-gated, so it affects rayls-replay only; the live rayls-network node is unchanged. Live-synced archive observers are therefore not covered — repair their datadir with rayls-replay --fix_genesis_history if needed.
  • Open follow-up: confirm the archive-replay-gated tests execute under CI's cargo test --workspace (feature unification via bin/rayls-replay should enable them).

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 4m 5s —— View job


Code Review — fix(replay): repair genesis history for v2 archives

  • Gather context (PR description, changed files)
  • Analyze code quality, correctness, and edge cases
  • Post review findings

Good fix. The root cause diagnosis is correct, the implementation is safe (atomic batch commit, idempotent, per-slot prefix scans), and the test coverage is solid. A few things to flag:


Medium — Stale plain-slot entries are never deleted

fix_genesis_history_with adds history entries under the hashed key but leaves the original plain-slot entries written by insert_storage_history untouched. After the fix, StoragesHistory holds both:

  • StoragesHistory[plain_slot]{0} (written by genesis init, stale, never read by v2)
  • StoragesHistory[hashed_slot]{0} (newly added by this fix, correct)

For a genesis with many storage-heavy contracts (e.g., the ConsensusRegistry), this permanently doubles the on-disk history for genesis slots. The function docstring calls this a "re-key" but it's actually an "add under new key". If you're intentionally keeping the old entries (defensive: no risk of data loss from a delete that could go wrong), that's a fine tradeoff — but it should be documented explicitly and the function/operation name "re-key" is slightly misleading.

Fix this →


Low — fix_genesis_account_history_with has no overflow guard

fix_genesis_history_with carefully checks before prepending that the earliest shard won't exceed NUM_OF_INDICES_IN_SHARD and re-chunks if it would. The account version at init.rs:571-582 has no such guard — it reads the ::last shard and unconditionally prepends block 0:

let blocks: Vec<u64> = core::iter::once(block).chain(existing_list.iter()).collect();
BlockNumberList::new(blocks)...

In practice this is safe because addresses with > 1 shard are skipped and cleared immutable accounts have 0 entries, so the single shard at most has NUM_OF_INDICES_IN_SHARD entries. But if IndexAccountHistoryStage is ever tweaked to produce a full 2000-entry first shard for an account that later becomes immutable, this silently produces an oversized shard. Adding a debug_assert! or a log warning when existing_list.len() >= NUM_OF_INDICES_IN_SHARD would make the invariant explicit.


Low — CLI flag name uses underscores

init.rs:97: #[arg(long)] on fix_genesis_history produces --fix_genesis_history. Clap auto-derives --fix-genesis-history for snake_case fields only if rename_all is set; otherwise it uses the literal field name with the underscore. All other long flags in Cli use hyphens (e.g., --from-block, --unwind-to). This inconsistency will be visible in --help. The fix is a #[arg(long = "fix-genesis-history")] annotation.

Fix this →


Nit — Loose assertion in the re-chunk test

tests.rs:991:

assert!(fixed >= 3, "expected at least our three slots re-keyed, got {fixed}");

test_genesis() may itself contain storage-bearing accounts, so fixed may be > 3 and the test passes vacuously for those extra slots. A stronger bound (e.g., collecting which slots were seeded via storage_history_shards before the call) or at least assert!(fixed == 3 + N_test_genesis_slots) would make regressions detectable. Not a blocking issue.


Positive observations

  • Atomic batch writes: all changes per call are accumulated in a single batch and committed once; any failure leaves the DB untouched.
  • Idempotency is correctly guarded: shards.iter().any(|(_, list)| list.contains(block)) catches both a prior run's hashed entries and (for the re-chunk path) any partial state.
  • Re-chunking edge case handled: the overflow path correctly collects all block numbers, deletes stale keys, and emits new ≤2000-entry shards; the resulting all vector is globally sorted (block 0 < any post-genesis block), so BlockNumberList::new is satisfied.
  • Deferred open_db: moving the consensus DB open past the maintenance/unwind early-returns is clean and avoids touching a dummy path in those modes.
  • Feature-gating: everything is correctly guarded behind #[cfg(feature = "archive-replay")], so the live rayls-network binary is unaffected.
  • Test coverage: the two new full-DB tests cover the three structural cases (immutable/single-shard/overflow) plus idempotency and the IndexAccountHistoryStage-clear simulation. The end-to-end manual verification on a 4-node network is a good sanity check.

@procdump procdump marked this pull request as ready for review July 9, 2026 09:50
@procdump procdump requested review from bronxyz and kstoykov as code owners July 9, 2026 09:50
procdump added 5 commits July 9, 2026 12:51
Genesis storage/account history in reth's v2 (hashed-state) genesis init
is written wrong: insert_storage_history keys StoragesHistory by the plain
slot while the v2 read looks up keccak256(slot) (genesis storage reads 0x0
at historical blocks), and IndexAccountHistoryStage clears AccountsHistory
and rebuilds from changesets, dropping immutable genesis accounts (empty
code on historical eth_call). Genesis has no changeset, so the normal
hashed indexing can't reconstruct either — it must be repaired explicitly.

Add two RethEnv methods (archive-replay only):
- fix_genesis_history: re-key genesis storage history to hashed slots.
  For each genesis (addr, hashed_slot) it read-merges the genesis block
  into the EARLIEST existing shard (via storage_history_shards), rather
  than overwriting the open shard. This preserves the post-genesis history
  of genesis slots that are also mutated later (e.g. balance/registry
  slots) and places block 0 in the shard the historical read consults for
  early blocks. Idempotent; per-slot prefix reads, no full-table scan.
- fix_genesis_account_history: re-seed a block-0 AccountsHistory entry for
  genesis accounts with <=1 shard via read-merge; multi-shard accounts
  (post-genesis changesets) are left untouched.

Wire into rayls-replay: run both automatically at the end of a normal
replay (freshly built archives boot correct with no extra step), and keep
--fix_genesis_history to repair archives built before this (e.g. mainnet).
Both no-op on v1 archives and on already-correct entries. Defer open_db
past the maintenance/unwind early returns so those modes don't open the
consensus DB.
Extract the fix logic into fix_genesis_history_with / fix_genesis_account
_history_with (take &ProviderFactory + use_hashed_state, return the count
fixed); the &self methods delegate. Lets a unit test drive the real logic
against a manually-built v2 storage stack without constructing a full
RethEnv.

Add test_fix_genesis_history_rekeys_and_preserves_post_genesis: builds a
v2 provider factory, runs canonical genesis init (which writes genesis
storage history under the buggy plain slot), simulates the post-genesis
history a replay appends under the hashed slot, then re-keys and asserts:
- precondition: hashed key lacks the genesis entry, plain key carries it
- immutable slot seeded with [0]
- single-shard mutable slot -> [0, 5] (post-genesis change preserved)
- multi-shard slot -> genesis prepended into the earliest shard, open
  shard untouched
- idempotent second run re-keys nothing

Runs under cargo test -p rayls-execution-evm --features archive-replay.
Establishes the key difference between the two halves of the bug: the v2
historical read looks up AccountsHistory by the PLAIN address (no hashing),
and insert_account_history writes the plain address too — so, unlike
StoragesHistory, there is no key mismatch. Genesis init seeds account
history correctly, so fix_genesis_account_history is a no-op on a
replay-built archive (which only appends, never runs the first-sync clear).

test_fix_genesis_account_history_seeds_after_clear_and_skips_multishard:
- genesis init already seeds AccountsHistory[addr]=[0] -> fix no-ops
- after simulating IndexAccountHistoryStage's first-sync clear, the fix
  re-seeds block 0 for cleared single-shard accounts
- multi-shard (post-genesis changeset) accounts are left untouched
- idempotent

So the account fix is defensive (covers the cleared-history case a normal
node produces); the necessary, always-applicable fix is the storage
re-key.
Observers hardcoded --full, which prunes account/storage history to ~10k
blocks. Gate it behind DISABLE_PRUNING in both launch paths (local-testnet.sh
and start-local-observer.sh): when set, observers run without --full so
history is retained and their datadir can seed rayls-replay. Validators are
unaffected. Documented in .env.example (left commented so an inline
DISABLE_PRUNING=1 is not overwritten when .env is sourced).
Prepending block 0 into the earliest shard overflows it when that shard is
already full (a hot genesis-seeded slot — e.g. an ERC-20 balance for a
genesis-funded holder — with >= NUM_OF_INDICES_IN_SHARD post-genesis
changes), producing a 2001-entry shard that violates reth's per-shard
invariant.

Keep the fast in-place prepend for the common case (single open shard with
room), but when it would overflow, gather the slot's full block list,
prepend genesis, delete the old shards and re-split into <=2000 chunks
(non-final keyed by highest block, final keyed u64::MAX). Batch deletes
precede puts so stale shard keys don't linger. Handles arbitrary cascades
(N input shards -> N+1 output).

Extend the storage test's slot_c to a full sealed shard [1..=2000] + full
open shard [2001..=4000]: asserts the 4001 blocks re-chunk into three
<=2000 shards, block 0 in the earliest, open shard keyed u64::MAX, every
block preserved, and idempotent on re-run.
@procdump procdump force-pushed the fix/storages-history2 branch from b4be69c to 5c84c6d Compare July 9, 2026 09:51
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 4m 21s —— View job


Code Review — fix(replay): repair genesis history for v2 archives

  • Gather context (PR description, changed files)
  • Read changed files in detail
  • Analyze code quality, correctness, and edge cases
  • Post review findings

Good fix overall. The root-cause diagnosis is precise, the implementation is atomic and idempotent, and the test coverage hits the right structural cases. Specific findings below.


Medium — Stale plain-slot entries are never deleted

fix_genesis_history_with adds hashed-key entries but leaves the original plain-slot entries written by insert_storage_history in place. After the fix StoragesHistory holds both:

  • StoragesHistory[(addr, plain_slot, u64::MAX)]{0} — written at genesis init, never read by v2
  • StoragesHistory[(addr, keccak256(slot), u64::MAX)]{0} — written by this fix, correct

For genesis-heavy contracts (e.g. ConsensusRegistry with the full validator set) this permanently doubles on-disk history for every genesis slot. The function docstring calls it a "re-key" but it's effectively "add under new key". If the conservative approach (no deletes) is intentional — valid reasoning to avoid a delete going wrong — it should be documented explicitly in the function doc and the name updated.

Fix this →


Low — CLI flag name uses underscore

init.rs:97 (actually main.rs:97): #[arg(long)] on the fix_genesis_history field emits --fix_genesis_history. Every other flag in Cli uses hyphens (--from-block, --unwind-to, --verify-every-block, --storage-v2). The mismatch is visible in --help output and will break any existing shell scripts or runbook commands. The inline comment at main.rs:286 also references the old name with an underscore.

Fix: add #[arg(long = "fix-genesis-history")] on the field, and update the comment at line 286.

Fix this →


Low — fix_genesis_account_history_with has no overflow guard

init.rs:574–581: for single-shard accounts the code reads the ::last shard and unconditionally prepends block 0:

let blocks: Vec<u64> = core::iter::once(block).chain(existing_list.iter()).collect();
BlockNumberList::new(blocks)...

The multi-shard skip (shard_counts.get(*addr) > 1) ensures the account has at most one shard, but that shard could have exactly NUM_OF_INDICES_IN_SHARD (2000) entries if reth's indexer ever produces a full-but-uncapped open shard. Prepending then creates an oversized list that violates reth's shard invariant.

The storage fix handles this with a re-chunk path. A minimum debug_assert!(existing_list.len() < NUM_OF_INDICES_IN_SHARD) or an explicit overflow check (return error / log + skip) would make the invariant explicit and surface future violations early.


Low — Multi-shard accounts after an IndexAccountHistoryStage clear still miss block 0

fix_genesis_account_history_with intentionally skips accounts with > 1 shard on the assumption they were correctly indexed by IndexAccountHistoryStage. But IndexAccountHistoryStage rebuilds history from AccountChangeSets, which have no entry for block 0 (genesis has no changeset). A multi-shard account in a cleared-history archive therefore never gets a block-0 entry. An eth_call or eth_getCode at block 0 on those accounts would still return empty.

For replay-built archives (where IndexAccountHistoryStage never runs) this is a non-issue — worth noting explicitly in the function doc. For manually repaired archives that were indexed by the stage, the limitation remains.


Nit — Loose assertion in the re-chunk test

tests.rs:991:

assert!(fixed >= 3, "expected at least our three slots re-keyed, got {fixed}");

test_genesis() may contain additional storage-bearing accounts, so fixed can exceed 3 and the assertion passes vacuously. An exact bound (e.g., counting the test-specific slots before the call via storage_history_shards or tracking the exact genesis slot count) would make regressions detectable.


Positive observations

  • Correct root cause isolation: the plain/hashed mismatch in genesis StoragesHistory init is precisely identified; the account history non-issue (same key on read and write) is correctly distinguished.
  • Atomic batch commit: all batch.put / batch.delete operations for a call accumulate in a single batch committed once; any failure leaves the DB untouched.
  • Idempotency: the shards.iter().any(|(_, list)| list.contains(block)) guard correctly covers both a prior run's hashed entries and any post-idempotency partial state.
  • Re-chunk edge case: the overflow path correctly collects all block numbers, deletes stale keys, and emits new ≤2000-entry shards with the last keyed u64::MAX; all is globally sorted so BlockNumberList::new is satisfied.
  • Deferred consensus_store open: moving open_db past the fix_genesis_history and unwind_to early-returns is clean; those maintenance modes no longer require a valid consensus-DB path.
  • Feature-gating: everything behind #[cfg(feature = "archive-replay")] — live rayls-network binary unaffected.
  • Fast path / rechunk selection: the shards.len() == 1 && len < NUM_OF_INDICES_IN_SHARD check correctly routes single-shard-with-room through the cheap in-place prepend and everything else through the safe rechunk.
  • Test coverage: the three structural cases (immutable / single-shard mutable / full-first-shard overflow) plus idempotency and the IndexAccountHistoryStage-clear simulation cover the key invariants.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 3m 36s —— View job


Code Review — fix(replay): repair genesis history for v2 archives

  • Gather context (PR description, changed files)
  • Read CLAUDE.md for repo-specific guidelines (none found)
  • Analyze changed files in detail
  • Post review findings

The root-cause diagnosis is correct and the implementation is safe: atomic batch writes, idempotent guards, per-slot prefix scans. The previous automated review already flagged three issues; I confirm those and add one new one below.


Medium — Stale plain-slot entries are never removed

fix_genesis_history_with writes entries under the hashed key but never removes the original plain-slot entries written by insert_storage_history. After the fix StoragesHistory holds both:

StoragesHistory[plain_slot, addr] → {0}   (stale — never read by v2, from genesis init)
StoragesHistory[hashed_slot, addr] → {0}  (new — correct)

For a genesis with many storage-heavy contracts (e.g. the full ConsensusRegistry + RLS proxy) this permanently doubles the on-disk history for every genesis slot. The function is also named "re-key" but behaves as "add-under-new-key", which will confuse future readers.

If keeping the old entries is a deliberate safety trade-off (no risk of losing data from a bad delete), document that explicitly and rename the operation. Otherwise, delete the plain-slot shard(s) before the batch.commit(). The plain key is StorageShardedKey::last(addr, *slot) where slot is the raw (un-hashed) B256.

Fix this →


Low — Re-chunk path relies on implicit ascending shard ordering

init.rs:700-703: the all vector is built by prepending block 0 to the blocks from each shard, in the order shards is returned:

let mut all: Vec<u64> = core::iter::once(block).collect();
for (_, list) in &shards {
    all.extend(list.iter());
}

This is globally sorted only if storage_history_shards returns shards in ascending order of highest_block_number. The comment says "ascending by highest block" but there is no assertion enforcing it. If the RocksDB scan order ever changes (e.g. a new iterator variant), BlockNumberList::new would silently produce an invalid list or return an error on a real archive.

A debug_assert! after building all would make the invariant explicit and catch a regression early:

debug_assert!(all.windows(2).all(|w| w[0] < w[1]), "shard blocks must be strictly ascending");

Fix this →


Low — fix_genesis_account_history_with has no overflow guard (confirming prior review)

init.rs:573-581: block 0 is unconditionally prepended to the existing list with no capacity check:

let blocks: Vec<u64> = core::iter::once(block).chain(existing_list.iter()).collect();
BlockNumberList::new(blocks)...

The invariant that multi-shard accounts are skipped means a single shard has at most NUM_OF_INDICES_IN_SHARD entries in practice, but nothing enforces this structurally. At minimum a debug_assert! here matches the pattern from the storage path and documents the invariant.


Low — CLI flag name has underscore (confirming prior review)

main.rs:97: #[arg(long)] on field fix_genesis_history generates --fix_genesis_history. Every other flag in Cli uses hyphens (--from-block, --unwind-to, --verify-every-block, etc.). Users will see the underscore variant in --help.

#[arg(long = "fix-genesis-history")]
fix_genesis_history: bool,

Fix this →


Nit — Loose assertion in the re-chunk test (confirming prior review)

tests.rs:991:

assert!(fixed >= 3, "expected at least our three slots re-keyed, got {fixed}");

test_genesis() likely seeds its own storage-bearing accounts, so fixed can exceed 3 and the assertion passes vacuously for any slot count ≥ 3. A tighter bound like assert_eq!(fixed, 3 + N) where N is the count of genesis accounts from test_genesis() with storage would catch a future regression where one of the three test slots was silently skipped.


Positive observations

  • Atomic writes: the batch is committed once at the end; any error path drops the batch without touching the DB.
  • Idempotency: the hashed-key shard check (shards.iter().any(|(_, list)| list.contains(block))) correctly detects both the first-run missing-entry case and a prior run's hashed entries.
  • Re-chunk produces a globally sorted all vector: because block == 0 < any post-genesis block and shards within a slot are already sorted, the final vector is globally sorted even without an explicit sort, making BlockNumberList::new safe.
  • Graceful-stop path skips the fix: the shutdown_rx guard at main.rs:272 ensures the fix only runs on full completion, leaving interrupted archives resumable.
  • Deferred open_db: moving the consensus DB open past the maintenance/unwind early-returns is clean and prevents touching the consensus DB in modes that don't need it.
  • Feature-gating: all of this is behind #[cfg(feature = "archive-replay")]; the live rayls-network binary is unaffected.

- Rename CLI flag --fix_genesis_history -> --fix-genesis-history to match the
  hyphenated style of every other flag; update the inline comment.
- fix_genesis_history: debug_assert the re-chunk block list is strictly
  ascending, documenting the invariant storage_history_shards ordering relies on.
- Document that the retained plain-slot genesis entries are intentional (never
  read by v2; deleting them is avoidable risk for no gain).
- Document fix_genesis_account_history's limitation: a multi-shard genesis
  account in a cleared-history archive is skipped and genesis has no changeset
  to rebuild from, so it won't regain block 0 (non-issue for replay archives).
- Strengthen the storage test: replace the loose 'fixed >= 3' with an outcome
  check that every genesis alloc storage slot carries block 0 under its hashed
  key after the fix.

Not addressed (per request): deleting the stale plain-slot entries; and the
account overflow guard, since the multi-shard skip already makes an oversized
single shard unreachable in practice.
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 1m 55s —— View job


Code Review — fix(replay): repair genesis history for v2 archives (post-update)

  • Gather context (PR description, prior review comments)
  • Read changed files in current state
  • Check which prior issues were addressed
  • Post review findings

Good follow-up. Most of the prior findings are resolved; one low-severity item remains open.


Resolved since the previous reviews

CLI flag now uses hyphens (main.rs:96):

#[arg(long = "fix-genesis-history")]
fix_genesis_history: bool,

The comment at main.rs:283 also correctly reads --fix-genesis-history. ✅

debug_assert! added for ascending shard order (init.rs:721–724):

debug_assert!(
    all.windows(2).all(|w| w[0] < w[1]),
    "re-chunk block list must be strictly ascending"
);

Correctly placed after all is fully built, before the delete/re-chunk loop. ✅

Stale plain-slot entries documented as intentional (init.rs:622–625): the docstring now explicitly states that the original plain-slot entry is left in place and explains why. The "re-keyed" language in the info! log at line 760 is slightly at odds with the doc ("adds under new key"), but the doc is what matters for future readers and it's clear. ✅

Test assertion strengthened (tests.rs:998–1007): the loose assert!(fixed >= 3) is gone; the test now iterates the full genesis alloc and asserts every slot carries block 0 under its hashed key. The per-slot structural checks (slot_a immutable, slot_b mutable, slot_c overflow) are now also fully deterministic. ✅


Remaining — Low: fix_genesis_account_history_with has no overflow guard

init.rs:583–587: block 0 is unconditionally prepended with no capacity check:

let blocks: Vec<u64> =
    core::iter::once(block).chain(existing_list.iter()).collect();
BlockNumberList::new(blocks)
    .map_err(|e| eyre::eyre!("failed to build block number list: {e}"))?

The <= 1 shard filter (init.rs:566) means the single shard can have at most NUM_OF_INDICES_IN_SHARD entries in practice, but that invariant isn't enforced structurally. A debug_assert! would make it explicit and mirror the pattern in the storage path:

// just before building `blocks`:
debug_assert!(
    existing_list.len() < NUM_OF_INDICES_IN_SHARD as u64,
    "single-shard account must have room for the genesis block"
);

Not a blocking issue given the current <= 1 guard, but adds no risk and surfaces any future regression early.

Fix this →


Positive observations

  • The four prior review findings (CLI flag, ascending-order assert, plain-slot doc, test tightness) are all cleanly addressed.
  • The debug_assert! for ascending order in the re-chunk path was the right call — it's cheap in release and catches the invariant without slowing down hot paths.
  • Docstring at init.rs:603–625 is now comprehensive and accurately describes the conservative no-delete tradeoff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants