[MegaRedHand]

Motivation

POST /eth/v1/validator/liveness/{epoch} bounds the epoch by the store's tick-driven clock (get_current_store_epoch). At the first moments of an epoch, before the boundary slot's tick has run, that clock still reads the previous epoch, so a validator client asking for the next epoch right at the boundary (doppelganger protection polls at every epoch start) gets a 400. beacon-APIs bounds it by the current (wall-clock) epoch. Same bug class as the duties fixes in #663, found while auditing for it.

Change

• The window (previous, current, next epoch) is computed from node::wall_slot, the clock attestation_data and the duties endpoints already use.
• New test: store tick and head in epoch W-1, wall clock in W: W-2 is a 400, W-1, W and W+1 are served, W+2 is a 400. It fails before the fix.
• The existing liveness tests pinned epoch 5 for both the state and the store clock, which a wall-clock bound cannot honor, so they now derive the epoch from the wall clock; their assertions are unchanged.

cargo test -p ethlambda-rpc --lib --profile release-fast, clippy and fmt clean.

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the changes to the validator liveness endpoint and its tests.

Summary

This PR changes the liveness endpoint's epoch validation from using the store's tick-driven epoch (get_current_store_epoch) to the wall clock epoch (compute_epoch_at_slot(wall_slot(store))). The rationale is correct: the store's tick lags behind the wall clock at epoch boundaries, which would incorrectly reject valid requests for the next epoch.

Issues Found

1. Potential panic in wall_epoch() test helper — unbounded recursion risk

File: crates/net/rpc/src/beacon/validator.rs, lines 1154-1158

fn wall_epoch() -> u64 {
    compute_epoch_at_slot(crate::beacon::node::wall_slot(
        &beacon_store_at(fulu_state()).0,
    ))
}

Problem: beacon_store_at(fulu_state()) creates a fresh store from genesis state every call. If wall_slot depends on system time (which "wall clock" implies), this is non-deterministic across test runs. The credited_state() function then calls wall_epoch() and passes that epoch to credited_state_at(epoch), but beacon_store_at(fulu_state()) in wall_epoch() uses a different store than store_for(credited_state()) in the actual tests.

This creates a race condition: if the system time advances between wall_epoch() and store_for() calls, the state epoch won't match the wall epoch used for validation.

Suggestion: Cache the store or use a deterministic clock for tests. Consider:

fn wall_epoch(store: &Store) -> u64 {
    compute_epoch_at_slot(crate::beacon::node::wall_slot(store))
}

Then in credited_state():

fn credited_state() -> (BeaconState, u64) {
    let state = fulu_state();
    let store = beacon_store_at(state.clone()).0; // or shared setup
    let epoch = wall_epoch(&store);
    (credited_state_at(epoch), epoch)
}

2. Test helper credited_state_at has hidden dependency on global state

File: crates/net/rpc/src/beacon/validator.rs, lines 1162-1173

fn credited_state_at(epoch: u64) -> BeaconState {
    let mut state = fulu_state();
    let BeaconState::Fulu(fulu) = &mut state else {
        unreachable!("built as fulu")
    };
    fulu.slot = compute_start_slot_at_epoch(epoch);
    // ...
}

Problem: fulu_state() likely returns a state with some genesis configuration. Setting fulu.slot to compute_start_slot_at_epoch(epoch) where epoch is derived from wall clock time could create a state with a slot far in the future relative to what the state's internal configuration (genesis time, etc.) expects. This may cause compute_epoch_at_slot or other epoch-related calculations to behave unexpectedly if there are invariants about slot progression.

Verification needed: Does fulu_state() set genesis_time appropriately? If genesis_time is fixed (e.g., test default) and wall clock is months later, the state's slot could be millions of slots ahead, potentially causing overflow in epoch calculations.

3. Missing bounds check in wall_epoch() - 1 and wall_epoch() + 1

File: crates/net/rpc/src/beacon/validator.rs, lines 1230, 1251

let (_, json) = post_liveness(store, wall_epoch() - 1, &["2", "3"], Default::default()).await;
// ...
for epoch in [wall_epoch() - 2, wall_epoch() + 2] {

Problem: If wall_epoch() returns 0 or 1, wall_epoch() - 2 underflows (u64). While unlikely in practice with mainnet genesis far from zero, this is a test code smell.

Suggestion: Use saturating_sub or assert preconditions:

let epoch = wall_epoch();
assert!(epoch >= 2, "test assumes epoch >= 2");
for e in [epoch - 2, epoch + 2] { ... }

4. New test the_window_follows_the_wall_clock_not_the_store_tick has subtle bug

File: crates/net/rpc/src/beacon/validator.rs, lines 1271-1287

async fn the_window_follows_the_wall_clock_not_the_store_tick() {
    let wall = wall_epoch();
    let store = store_for(credited_state_at(wall - 1));
    for (epoch, expected) in [
        (wall - 2, StatusCode::BAD_REQUEST),
        (wall - 1, StatusCode::OK),
        // ...
    ] {

Problem: The test creates a state at wall - 1, but credited_state_at(wall - 1) sets fulu.slot = compute_start_slot_at_epoch(wall - 1). However, wall_epoch() internally creates a separate store with fulu_state() to compute the wall epoch. There's no guarantee these two stores/states are consistent.

More critically: the liveness check at wall - 1 looks at current_epoch_participation[2] for validator 2. But if the store's internal tick is at wall - 1 (as intended by this test), and we're querying wall - 1, the "current" epoch from the store's perspective matches the query. The test doesn't actually verify the bug fix — it doesn't demonstrate a case where the old get_current_store_epoch would have failed.

Suggestion: To properly test the fix, you need to simulate the boundary condition explicitly:

• Set up store with tick at epoch N
• Set wall clock to epoch N+1
• Verify that query for N+1 succeeds (would fail with old code)

This requires controlling the wall clock or mocking wall_slot, which the current test structure doesn't do.

5. Comment accuracy in production code

File: crates/net/rpc/src/beacon/validator.rs, lines 214-216

// The wall clock, not the store's tick-driven one: the latter still reads
// the previous epoch until the slot tick runs, so a client asking for the
// next epoch right after a boundary would be refused.

Minor issue: "the previous epoch" is slightly imprecise — the store's tick reads the epoch of its current slot, which could be the same epoch or previous epoch depending on boundary timing. More accurate: "the store's tick may still reflect the previous epoch for a short period after the boundary."

Positive Aspects

1. Correct security fix: Using wall clock for API validation prevents false rejections at epoch boundaries, which could cause validator clients to miss duties.

2. Good test coverage: The new test verifies the window behavior across multiple epochs.

3. Proper error handling maintained: The epoch + 1 < current || epoch > current + 1 check correctly implements the spec's allowed window.

Recommendations

Priority	Item	Action
High	Test determinism	Refactor wall_epoch() to use shared store; avoid creating multiple stores
Medium	Underflow safety	Add saturating_sub or preconditions in tests
Medium	Test validity	Add explicit test simulating tick/wall clock divergence
Low	Comment precision	Clarify "previous epoch" wording

The core logic change is correct and addresses a real consensus-layer issue. The test infrastructure needs hardening to be reliable and to actually validate the boundary condition that motivated the fix.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. The change is narrowly scoped to RPC liveness windowing and does not touch fork choice, attestation validation, justification/finalization, STF, XMSS, or SSZ logic.

• crates/net/rpc/src/beacon/validator.rs:217 fixes a real boundary bug: using wall_slot(store) instead of the tick-driven store time makes /eth/v1/validator/liveness/{epoch} accept the expected current/next epoch immediately after an epoch boundary, even before the slot tick updates the store. That matches the endpoint’s intended UX and avoids false 400s.
• crates/net/rpc/src/beacon/validator.rs:224 still derives participation from the head state’s epoch, so the behavioral change is limited to request-window validation, not to liveness crediting itself. I don’t see a consensus-safety risk here.
• crates/net/rpc/src/beacon/validator.rs:1155 and crates/net/rpc/src/beacon/validator.rs:1272 add a good regression test for the wall-clock/store-tick skew case. That’s the key scenario to cover.

No obvious security or memory-safety concerns:

• The new path only reads wall-clock time and compares epoch numbers.
• No new allocation hotspots beyond existing request handling.
• Error handling remains consistent with the rest of the endpoint.

Minor note:

• crates/net/rpc/src/beacon/validator.rs:218 still uses epoch + 1 < current. If Epoch is an unsigned integer, epoch == u64::MAX would overflow in debug builds. In practice parse_epoch may already bound this, but if not, epoch.saturating_add(1) < current would be a slightly more defensive pattern. Low severity, likely unreachable through normal API use.

If you want, I can also sanity-check the full PR against the base branch with git diff/blame context, not just /tmp/pr_diff.txt.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 670: bound liveness by the wall clock

The change is small and correct. I only read the diff and the wall_slot helper, and I didn't build or run the tests.

The old check used the store's tick-driven epoch, which lags the real epoch until the slot tick runs. That would refuse a validator client asking for the next epoch just after a boundary. wall_slot (node.rs:29) already guards against clock errors and a zero slot duration, and /lean/v0/node/syncing uses the same computation. Reusing it keeps one definition of "now".

Notes

1. Flaky tests near an epoch boundary (validator.rs, liveness tests).

   • wall_epoch() reads the real clock on every call, and each test calls it several times. That includes credited_state(), post_liveness(...) and the epoch arithmetic.
   • If an epoch boundary falls between two calls, the state is built for one epoch while the request targets another, and the test fails.
   • With 32-slot epochs the window is tiny, but it is non-deterministic. the_window_follows_the_wall_clock_not_the_store_tick does it right: it captures let wall = wall_epoch(); once.
   • I'd do the same in the other tests, capturing it once at the top of each. Alternatively, retry the whole test if the epoch changed.

2. Possible epoch + 1 overflow (validator.rs:218).

   • parse_epoch yields an arbitrary u64. For epoch == u64::MAX, epoch + 1 panics in debug builds and wraps to 0 in release.
   • In release it wraps to 0, which is < current, so the request is rejected correctly anyway.
   • Using epoch.saturating_add(1) < current or epoch.abs_diff(current) > 1 would be clearer and panic-free. This was probably already there before the PR, so it's optional.

3. Participation lookup is unchanged (validator.rs:224-240).

   • When the wall epoch is ahead of the head state's epoch, for example the head is one epoch behind, the lookup returns None for the requested epoch.
   • That gives is_live = false unless the observed-liveness set has an entry. Before this change the same case was refused with a 400.
   • Answering "not live" is reasonable, and the new test covers it, since credited_state_at(wall - 1) is queried at wall, wall + 1 and so on.

Otherwise this looks good to merge.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt