Skip to content

fix(storage): reject a data directory from another network - #556

Open
MegaRedHand wants to merge 1 commit into
mainfrom
fix/verify-db-genesis-matches
Open

fix(storage): reject a data directory from another network#556
MegaRedHand wants to merge 1 commit into
mainfrom
fix/verify-db-genesis-matches

Conversation

@MegaRedHand

Copy link
Copy Markdown
Collaborator

Stacked on #554 (fix/resume-db-without-checkpoint-url). Base is that branch, so this diff is just the genesis check. Merge #554 first.

🗒️ Description / Motivation

Store::from_db_state decided whether a data directory was ours by comparing genesis_time alone, read from the persisted ChainConfig, and on mismatch logged a warning and returned None — "treat as empty":

if persisted_config.genesis_time != expected_genesis_time {
    warn!(..., "Persisted DB has a different genesis_time; treating as empty");
    return Ok(None);
}

Two problems with that.

1. "Treat as empty" is not empty. The caller then wrote a fresh anchor over the foreign chain's rows without clearing them. get_signed_blocks_by_slot_range resolves each slot through BlockRoots with no anchor check:

for slot in start_slot..=end_slot {
    let Some(root_bytes) = view.get(Table::BlockRoots, &encode_block_root_key(slot))? else { continue };
    // ... no check that this root belongs to the current chain
}

so for slots the new chain had not reached yet, BlocksByRange served the other network's blocks to peers, who reject them and score-penalize us. (Fork choice is unaffected: compute_lmd_ghost_head is seeded from latest_justified, so foreign roots are unreachable and never accumulate weight.)

2. ChainConfig is { genesis_time: u64 }. A network regenerated with the same genesis time but a different validator set was not detected at all — the DB was resumed as if it were ours. #554 makes the DB readable without --checkpoint-sync-url, which puts that case on the default restart path.

What Changed

crates/common/types/src/genesis.rs — new verify_state_genesis(state, genesis_time, expected_validators) plus a GenesisConfig::verify_state convenience wrapper, and a GenesisMismatch error enum. Compares genesis time and the full validator registry: count, sequential indices, and both pubkeys per validator. The validator set is fixed at genesis (nothing in the state transition mutates it), so any state of our chain must carry exactly that registry, whatever slot it sits at.

crates/storage/src/store.rsfrom_db_state takes &GenesisConfig and verifies the finalized state (never pruned, and the state the anchor is rebuilt from) rather than the persisted config. A mismatch is now Error::GenesisMismatch. A missing finalized state is Error::UnexpectedMissingState instead of silently "empty".

bin/ethlambda/src/checkpoint_sync.rsverify_checkpoint_state had its own copy of the same four checks; it now delegates to the shared function, and GenesisTimeMismatch / ValidatorCountMismatch / NonSequentialValidatorIndex / ValidatorPubkeyMismatch collapse into one Genesis(#[from] GenesisMismatch). Its checkpoint-only sanity checks stay put — slot != 0 must not apply to the DB path, since a data directory legitimately sits at genesis while a downloaded anchor never does.

crates/storage/src/lib.rs — export Error. It was a private type appearing in public signatures, so callers could not name it to match on it.

bin/ethlambda/src/main.rsfetch_initial_state propagates the error (if let Some(store) = Store::from_db_state(..)?) instead of swallowing it with if let Ok(Some(..)).

Correctness / Behavior Guarantees

DB in data directory before after
Ours resumed resumed
Different GENESIS_TIME warn → new anchor written over foreign rows startup aborts, DB untouched
Same GENESIS_TIME, different validator set undetected, resumed as ours startup aborts
Genuinely empty initialize initialize (unchanged)

Aborting is deliberate rather than falling back: there is no safe way to reuse the directory, so the operator has to fix --data-dir or remove it. The error names what differs, e.g. persisted state does not match the configured genesis: validator 1 pubkey mismatch (attestation or proposal key).

Operational note for reviewers: any flow that reuses a data directory across a genesis regeneration now fails to boot loudly instead of silently restarting from a fresh anchor. lean-quickstart's --generateGenesis implies --cleanData, so local devnets are unaffected; ansible/Hive flows that regenerate genesis in place would need to clear the directory.

Tests Added / Run

crates/common/types — 5 tests on verify_state: accepts a state from the same genesis; rejects different genesis time, different validator count, swapped validator keys at the same count and genesis time, and non-sequential indices.

crates/storagefrom_db_state_errors_on_genesis_time_mismatch and from_db_state_errors_on_validator_set_mismatch.

bin/ethlambdafails_when_db_genesis_time_differs (also asserts the original DB still loads under its own genesis, i.e. it was not overwritten) and fails_when_db_validator_set_differs.

One test changed contract rather than being fixed: from_db_state_returns_none_on_genesis_time_mismatch encoded the "treat as empty" behavior this PR removes. It is replaced by the two ..._errors_on_... tests above. Flagging explicitly since that is a deliberate contract change, not a broken test.

make fmt && make lint && make test    # 30 test binaries, 0 failures

Related Issues / PRs

✅ Verification Checklist

  • Ran make fmt — clean
  • Ran make lint (clippy with -D warnings) — clean
  • Ran make test (cargo test --workspace --profile release-fast) — all passing

@MegaRedHand
MegaRedHand force-pushed the fix/verify-db-genesis-matches branch from b70d070 to 1da123b Compare August 3, 2026 19:10
Base automatically changed from fix/resume-db-without-checkpoint-url to main August 3, 2026 21:59
MegaRedHand added a commit that referenced this pull request Aug 3, 2026
## 🗒️ Description / Motivation

Restarting a node without `--checkpoint-sync-url` destroyed its chain.
`fetch_initial_state` gated the on-disk state lookup on that flag:

```rust
if checkpoint_urls.is_empty() {
    info!("No checkpoint sync URL provided, initializing from genesis state");
    let genesis_state = State::from_genesis(genesis.genesis_time, validators);
    return Ok(Store::from_anchor_state(backend, genesis_state));
};
// ... only past this point was Store::from_db_state tried
```

So a redeploy against a populated RocksDB wrote a slot-0 genesis anchor
over a perfectly current chain, and operators had to pass a checkpoint
URL purely as a *fallback trigger* even when the DB was fresh and the
URL was never fetched.

This makes on-disk state authoritative: `--checkpoint-sync-url` becomes
a fallback for when there is nothing resumable, not a precondition for
reading what is there.

## What Changed

**`bin/ethlambda/src/main.rs`** — `fetch_initial_state` tries
`Store::from_db_state` *before* the empty-URL early return:

```
gap = current_slot − store.head_slot()
  gap ≤ MAX_RESUMABLE_DB_STATE_AGE  → resume from DB                    info!
  gap > MAX, no checkpoint URLs     → resume from DB                    warn!  (new)
  gap > MAX, checkpoint URLs set    → fall through to checkpoint sync
no resumable DB, checkpoint URLs    → checkpoint sync
no resumable DB, no URLs            → genesis
```

Also removes the `info!(url_count, "Starting checkpoint sync")` that was
emitted *before* the DB was consulted. It fired on every successful
resume, so grepping a boot log for `"Starting checkpoint sync"`
false-positived on nodes that never synced. Each outcome now logs
exactly one line, at the point the decision is made:

| Boot log line | Outcome |
| --- | --- |
| `Resuming from existing DB head_slot=… current_slot=… gap=…` | Resumed
from disk, nothing downloaded |
| `DB is stale; resuming anyway head_slot=… current_slot=… gap=…` |
Resumed past the window, no URL to prefer |
| `DB is stale; checkpoint sync head_slot=… current_slot=… gap=…` | Past
the window, a URL took over |
| `Starting checkpoint sync checkpoint_urls=[…]` | Downloading a
checkpoint |
| `No checkpoint sync URL provided, initializing from genesis state` |
Started from genesis |

**`bin/ethlambda/src/cli.rs`** — `--checkpoint-sync-url` help text no
longer claims it "skips genesis initialization"; it is documented as a
fallback.

**`docs/checkpoint_sync.md`** — new *Restarts and Existing State*
section: precedence table, the resume window and why it is measured
against the head rather than the finalized checkpoint, the P2P-catch-up
caveat, and why an all-URLs-fail abort is intentional.

## Correctness / Behavior Guarantees

| DB state (matching `GENESIS_TIME`) | `--checkpoint-sync-url` | before
| after |
| --- | --- | --- | --- |
| absent | omitted | genesis | genesis |
| absent | set | checkpoint sync | checkpoint sync |
| fresh (head-lag ≤ 450) | omitted | **genesis, resets to slot 0** |
**resume** |
| fresh | set | resume | resume |
| stale (head-lag > 450) | omitted | **genesis, resets to slot 0** |
**resume + warning** |
| stale | set | checkpoint sync | checkpoint sync |

- `MAX_RESUMABLE_DB_STATE_AGE` keeps its value and its meaning in the
URL-present case; it now only decides *whether a checkpoint is
preferable to what we already have*, never whether the DB is readable.
- Staleness is still measured against the head (`current_slot -
head_slot`), so a node whose head is current resumes during a finality
stall.
- **Stale DB + no URL resumes rather than refusing to boot.** No
checkpoint URL was configured, so there is no anchor to switch to and
the node runs against the data directory it was given. The warning
exists because range sync may not close a gap this large: peers prune
block signatures past `SIGNATURE_PRUNING_RANGE` (~1 day), so beyond that
horizon they cannot serve the missing history and the node needs a
checkpoint URL. Refusing to start instead would break unattended
restarts after a routine 31-minute outage.
- **Stale DB + URLs set + every URL failing still aborts.** Deliberate,
and documented as such: configuring the flag asks for a specific anchor,
so an unreachable source is a misconfiguration to surface at boot rather
than paper over by starting a node that is hours behind. Omitting the
flag is how you ask for "resume whatever is on disk"; that path never
aborts.
- **No new flag.** Omitting `--checkpoint-sync-url` no longer means
"start from genesis" when a DB exists; to deliberately start over,
remove the data directory. That is already the documented idiom for a
clean checkpoint sync, and an `--ignore-existing-db` flag would only
reintroduce the write-genesis-over-live-data footgun behind a flag.
- **Unchanged / out of scope:** a `GENESIS_TIME` mismatch still degrades
silently (`from_db_state` logs `"Persisted DB has a different
genesis_time; treating as empty"`), so with no URL the node writes
genesis over a foreign-network DB. Pre-existing behavior, addressed
separately in #556;
`initializes_from_genesis_when_db_genesis_time_differs` pins it here as
a known hazard rather than a desired invariant.

## Tests Added / Run

Six unit tests in `bin/ethlambda/src/main.rs` driving
`fetch_initial_state` against `InMemoryBackend`:

| Test | Gap | Asserts |
| --- | --- | --- |
| `initializes_from_genesis_when_db_is_empty` | — | head slot 0 |
| `resumes_from_fresh_db_without_checkpoint_url` | `MAX / 2` | head slot
is the seeded slot, not 0 |
| `resumes_from_stale_db_without_checkpoint_url` | `MAX + 100` | resumes
despite `gap > MAX_RESUMABLE_DB_STATE_AGE` |
| `resumes_from_fresh_db_with_checkpoint_url` | `= MAX` | resume wins
over a URL; nothing is dialed |
| `falls_through_to_checkpoint_sync_when_db_is_stale` | `MAX + 1` | past
the window the URL takes over, and an unreachable one aborts |
| `initializes_from_genesis_when_db_genesis_time_differs` | — | head
slot 0 (DB treated as empty) |

The seeded anchor sits above slot 0 because a genesis re-init also
yields head slot 0; that is what makes "resumed" distinguishable from
"started over". Staleness is induced purely by choosing `genesis_time`
(`current_slot` derives from the wall clock against it), so no clock
injection.

The two no-URL resume tests cannot pin the threshold on their own: both
no-URL branches return the same store, so inverting the comparison
leaves them green. The pair that can are the two with a URL set, where
the outcomes differ. Verified by mutation:

| Mutation | Result |
| --- | --- |
| `gap <= MAX` → `gap > MAX` | both URL tests fail, the four others pass
|
| `gap <= MAX` → `gap < MAX` | the `= MAX` boundary test fails |

Those two use `#[tokio::test(start_paused = true)]` so the checkpoint
retry backoff (5 attempts × 5s) costs no wall clock; the connection
refusal against `http://127.0.0.1:1` is immediate. That needs tokio's
`test-util` feature as a **dev**-dependency, so it never reaches the
shipped binary.

Commands run:

```
cargo test -p ethlambda --profile release-fast --bin ethlambda   # 33 passed
make fmt && make lint && make test                              # all clean (550 passed, 7 pre-existing ignored)
```

Local multi-client devnet verification is in progress; I'll post the
boot logs showing a keep-DB restart with no `--checkpoint-sync-url` as a
comment.

## Related Issues / PRs

- Related to #505 (head-lag resume gate, which this builds on)
- #556 rejects a data directory from another network, covering the
`GENESIS_TIME`-mismatch hazard this PR only pins
- #560 carries the unrelated `CLAUDE.md` RPC-port note that was
originally in this branch
- #559 tracks a pre-existing bug this PR makes easier to hit: the duty
sync gate reports Synced while a node backfills from a stale resume, so
it attests and proposes on an old head. Not addressed here
- Logging a `Store::from_db_state` read error instead of discarding it
(the `Err` arm of `if let Ok(Some(_))`, unreachable today) is left to a
follow-up PR

## ✅ Verification Checklist

- [x] Ran `make fmt` — clean
- [x] Ran `make lint` (clippy with `-D warnings`) — clean
- [x] Ran `make test` (`cargo test --workspace --profile release-fast`)
— all passing
`Store::from_db_state` compared only `genesis_time`, taken from the
persisted `ChainConfig`, and on mismatch logged a warning and returned
`None` — "treat as empty". The caller then wrote a fresh anchor over the
foreign chain's rows without clearing them. `get_signed_blocks_by_slot_range`
resolves slots through `BlockRoots` with no anchor check, so for slots the
new chain had not reached yet `BlocksByRange` served the other network's
blocks to peers, who reject them and penalize our score.

Worse, `ChainConfig` carries only `genesis_time`, so a network regenerated
with the same genesis time but a different validator set was not detected
at all: the DB was resumed as if it were ours. Making the DB readable
without `--checkpoint-sync-url` (previous commit) puts that case on the
default restart path.

Compare the whole genesis instead — genesis time plus the full validator
registry (count, sequential indices, both pubkeys) — against the finalized
state rather than the persisted config, and make a mismatch fatal. The
validator set is fixed at genesis, so any state of our chain must carry
exactly that registry. Refusing to boot is deliberate: there is no safe
way to reuse the directory, and the operator needs to fix --data-dir or
remove it.

The comparison lives in `ethlambda-types` as `verify_state_genesis`, shared
with `checkpoint_sync::verify_checkpoint_state`, which had its own copy of
the same four checks; its `GenesisTimeMismatch`/`ValidatorCountMismatch`/
`NonSequentialValidatorIndex`/`ValidatorPubkeyMismatch` variants collapse
into one `Genesis(#[from] GenesisMismatch)`. The checkpoint-only sanity
checks (slot != 0, finalized <= slot, header pairing) stay there, since a
DB legitimately sits at genesis while a downloaded anchor never does.

`from_db_state_returns_none_on_genesis_time_mismatch` asserted the old
"treat as empty" contract and is replaced by two tests asserting the new
fatal one, one of them covering the same-genesis-time case the old check
could not see.
@MegaRedHand
MegaRedHand force-pushed the fix/verify-db-genesis-matches branch from 1da123b to 00908e9 Compare August 3, 2026 22:14
@MegaRedHand
MegaRedHand marked this pull request as ready for review August 3, 2026 22:15
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

This PR introduces a critical security fix for network identity verification while refactoring genesis validation into a shared, testable module. The changes prevent accidental cross-network data corruption and ensure validator registry integrity.

Security & Correctness

Critical Fix: Foreign Network Detection

  • File: crates/storage/src/store.rs (lines 647-666)
  • Issue Resolved: Previously, from_db_state only checked genesis_time and silently treated mismatches as empty databases (with just a warning). This allowed a node to potentially re-initialize on top of a foreign chain's data, leaving orphaned blocks that could be served to peers via BlocksByRange.
  • Fix: Now validates the full validator registry (count, sequential indices, and both XMSS pubkeys) against the configured genesis. Returns Error::GenesisMismatch as a hard error, aborting startup.

Checkpoint Sync Validation

  • File: bin/ethlambda/src/checkpoint_sync.rs (lines 202-205)
  • Improvement: Replaces inline validation with shared verify_state_genesis function, ensuring checkpoint sync and disk-resume paths use identical network-identity checks. The Genesis error variant (line 49) properly encapsulates all mismatch scenarios.

Code Quality

Error Handling

  • File: crates/common/types/src/genesis.rs (lines 7-21)
  • The new GenesisMismatch enum provides structured error contexts for debugging. The #[from] implementations in CheckpointSyncError and Error enable ergonomic propagation.

Performance

  • File: crates/storage/src/store.rs (lines 657-666)
  • Loading the full finalized state at startup to verify validators is O(n) in validator count. For networks with large validator sets (e.g., 1M+), this adds startup overhead but is acceptable given the security requirements. Consider documenting this cost.

Testing

  • File: crates/storage/src/store.rs (lines 2993-3060)
  • Excellent test coverage for the three cases: empty DB, matching genesis, and both types of mismatch (time vs. validators).
  • File: bin/ethlambda/src/main.rs (lines 981-1022)
  • Integration tests verify that foreign databases are neither loaded nor overwritten.

Minor Observations

  1. Error Message Clarity

    • File: crates/common/types/src/genesis.rs (line 13)
    • The format string "(expected {position}, got {got})" is correct since position equals the expected index, but consider renaming the field to expected in NonSequentialIndex for clarity, or adjust the template to avoid confusion.
  2. State Retrieval Safety

    • File: crates/storage/src/store.rs (line 661)
    • Using latest_finalized to load the verification state assumes the finalized state is never pruned. Ensure this invariant holds in your pruning strategy, or document that the latest finalized state must be retained.
  3. Iterator Safety

    • File: crates/common/types/src/genesis.rs (line 70)
    • The zip usage is safe due to the preceding length check, but consider adding a debug assertion or using zip_eq (from itertools) in future to panic on length mismatch in debug builds.

Consensus Considerations

The validator registry verification correctly enforces Ethereum's network identity definition:

  • Genesis time and validator set are the canonical network identifiers
  • Sequential index validation (0..n) ensures state integrity
  • Dual pubkey comparison (attestation + proposal) aligns with 3SF-mini's XMSS requirements

Acknowledgment: The refactoring effectively deduplicates validation logic between sync and storage layers, reducing maintenance burden and preventing divergence in security-critical checks.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  1. High: the new startup guard only proves that the finalized state matches the configured genesis; it does not prove that the persisted canonical indices are clean. Store::from_db_state accepts the DB after genesis.verify_state(&state) on the finalized state (crates/storage/src/store.rs), but BlocksByRange is still served directly from Table::BlockRoots (crates/storage/src/store.rs, crates/net/p2p/src/req_resp/handlers.rs). Since init_store writes new anchor metadata/index rows without clearing old ones (crates/storage/src/store.rs), a data dir that was already contaminated by the previous “re-anchor over foreign DB” behavior can still pass this new check and continue serving stale foreign canonical blocks. If the intent is to close that security hole, startup needs either a stronger integrity check over the canonical chain/index tables or a one-time cleanup/migration path for previously mixed directories.

No other correctness issues stood out in the diff; the shared genesis-identity check itself is a solid improvement.

I couldn’t run the Rust tests here because this environment has a read-only Cargo/rustup home and no network access, so this is a static review only.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR prevents a node from reusing a data directory belonging to another network.

  • Adds shared genesis identity verification covering genesis time, validator count, sequential indices, and validator public keys.
  • Applies that verification to persisted finalized state and checkpoint-sync state.
  • Propagates storage identity and missing-state errors to abort startup instead of overwriting foreign data.
  • Exports the storage error type and documents the new startup behavior.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code defect identified.

The shared identity validation matches the state model’s immutable validator registry, persisted-state failures now propagate before any replacement anchor is written, and checkpoint-specific sanity checks remain intact.

Important Files Changed

Filename Overview
crates/common/types/src/genesis.rs Adds a shared, well-tested genesis identity check over genesis time and the immutable validator registry.
crates/storage/src/store.rs Validates the persisted finalized state before resuming and returns explicit errors rather than treating foreign data as empty.
bin/ethlambda/src/checkpoint_sync.rs Reuses the shared genesis verifier while retaining checkpoint-specific structural checks.
bin/ethlambda/src/main.rs Propagates persisted-store errors so foreign data aborts startup without being overwritten.
crates/storage/src/error.rs Adds explicit errors for missing finalized state and genesis mismatch.
crates/storage/src/lib.rs Publicly exports the storage error type used by public fallible APIs.
docs/checkpoint_sync.md Documents foreign-state rejection and the shared checkpoint/persisted-state identity checks.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Open data directory] --> B{Chain metadata present?}
  B -- No --> C[Initialize from genesis or checkpoint]
  B -- Yes --> D[Load finalized state]
  D --> E{Matches configured genesis?}
  E -- Yes --> F[Resume persisted store]
  E -- No --> G[Abort startup without modifying DB]
  D --> H{State missing or unreadable?}
  H -- Yes --> G
Loading

Reviews (1): Last reviewed commit: "fix(storage): reject a data directory fr..." | Re-trigger Greptile

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.

1 participant