fix(electra): recompute deposit counters when the state object was replaced - #289
fix(electra): recompute deposit counters when the state object was replaced#289ander-deran-arteaga wants to merge 3 commits into
Conversation
…placed
f_deposits_num is written as exactly 0 for some epochs while t_deposits holds
the corresponding rows. Never slightly wrong, always exactly zero.
The counters are accumulated on one state and read from another, an epoch
later:
processPendingDeposits() state := p.baseMetrics.NextState
state.DepositsNum += 1
ExportToEpoch() DepositsNum: int(s.CurrentState.DepositsNum)
The value written into the epoch row for E-1 was accumulated while that object
was NextState, during the processing of E-1. It survives only if the same object
is still in the cache when E is processed. Three things replace or reset it in
between:
- AdvanceFinalized deletes and re-downloads a state whose root changed
- ensureDependencyStates re-downloads states evicted by CleanUpTo
- RefreshBlocks resets the counters outright, and post-Electra AddBlocks does
not refill them, since deposits arrive through the pending queue rather than
block bodies
A replaced object carries zeros and nothing recomputes them, so the epoch row
reports no deposits while the detail rows exist. Measured on mainnet: 252 epochs
over 450421-470670, 1,653 deposits and roughly 113,000 ETH absent from the
summaries, against 10,029 epochs written correctly. 97.5% right, and wrong in
exactly the way a lost accumulator is wrong.
The same reading explains why the consolidation counters fail on a different set
of epochs, overlapping the deposit failures in only 18 of 37 cases: RefreshBlocks
resets the deposit counters and not the consolidation ones, so the two have
different wipe conditions.
Fix: record on the state whether its deposit counters were ever accumulated, and
recompute them from the state itself when they were not. What
processPendingDepositsFor derives depends only on the state handed to it, so a
state that came back from the beacon node reaches the same numbers the original
run reached. It must run at most once per object, since the counters accumulate
with += rather than being assigned, and the new flag is what enforces that.
The recomputation runs before the existing map clears, so its contribution to
DepositedAmounts is discarded exactly as the surrounding code expects and the
reward path repopulates that map afterwards; only the two counters survive.
Deposit rows are persisted from NextState.DepositsProcessed, so recomputing for
CurrentState cannot duplicate them. The common case costs a boolean check.
Seven tests. One found a hole in the first version of this fix: RefreshBlocks
zeroed the counters without clearing the flag, so a refreshed state still
claimed it had been accumulated and the recomputation never fired. Others cover
the claim the fix rests on, that a re-downloaded state recomputes to the numbers
the original reached, keep "no deposits this epoch" distinguishable from "never
counted", and pin the doubling that makes the flag necessary.
Pre-existing failures in pkg/analyzer are unchanged, 13 before and 13 after;
they need a live beacon endpoint.
Closes #287
leobago
left a comment
leobago
left a comment
There was a problem hiding this comment.
Thanks @ander-deran-arteaga here's my review of this PR.
The diagnosis is correct and well-evidenced, and the shape of the fix is right. processPendingDepositsFor is a clean extraction: what it derives depends only on the state passed in, so recomputing from a re-downloaded state genuinely reproduces the original numbers. Resetting the flag inside RefreshBlocks (caught by their own test) is the right call. Common-path cost is one boolean check.
But the fix is incomplete in two ways and leaves one adjacent bug untouched.
- The NextState path still isn't guarded, so reprocessing can double the counters
processPendingDepositsFor sets PendingDepositsProcessed = true at the end, but only the new CurrentState callsite checks it. The original path, processPendingDeposits() -> processPendingDepositsFor(NextState), still runs unconditionally on every ProcessStateTransitionMetrics call.
Failure case: epoch X is reprocessed while state X is retained in cache (not re-downloaded, not refreshed). This happens on a dependency-triggered reprocess in AdvanceFinalized (epochsWithChangedBlocks[X-1]), on a PR #288 carried reprocess, and on a RefreshStateBlocks transient failure where the flag never got reset. processPendingDeposits() runs state.DepositsNum += ... a second time and the counters double. The epoch row for X, written when X+1 is later processed off that same state object, then reports double.
Part of this predates the PR, but the PR introduces a new variant: the recompute-as-CurrentState now sets the flag and counters during processing of X+1, and a subsequent reprocess-as-epoch-X doubles them. Since processPendingDepositsFor is right there being touched, the clean fix is to make it idempotent: early-return when state.PendingDepositsProcessed is already set, and drop the caller-side condition. TestRunningTwiceOnOneStateDoublesTheCounters currently pins the fragile behavior as intended and should be inverted to assert idempotence.
Addresses the review on #289. The previous version guarded one call site: PreProcessBundle checked PendingDepositsProcessed before recomputing from CurrentState. The other path, processPendingDeposits -> processPendingDepositsFor(NextState), still ran unconditionally on every ProcessStateTransitionMetrics call, so an epoch reprocessed against a state still held in cache accumulated its counters a second time and doubled them. A dependency-triggered pass in AdvanceFinalized, a carried reprocess, or a RefreshStateBlocks failure that left the flag unset all reach that path. The guard now lives in processPendingDepositsFor, where every path meets, and the caller-side condition is gone. Guarding at a call site only ever protects that call site. TestRunningTwiceOnOneStateDoublesTheCounters asserted the doubling as intended behaviour. It pinned the bug. Replaced with tests that assert idempotence for the scalar counters, for the per-validator amounts, and for the NextState path specifically. Found while writing those: RefreshBlocks zeroes DepositsNum and TotalDepositsAmount but left DepositedAmounts populated, so a refreshed state recomputed to correct scalars and doubled per-validator amounts. Its own doc says it "resets the accumulators first so that calling it on a state whose blocks were already set does not double-count values", and DepositedAmounts is an accumulator it missed. Reset it there, with a test that fails without the line. Five mutations, all caught.
There was a problem hiding this comment.
Pull request overview
This PR fixes an Electra-era metrics correctness bug where epoch-level deposit (and related) counters could be written as exactly zero when the state object that previously accumulated them was replaced (e.g., re-download after reorg/eviction) or reset (via RefreshBlocks). The fix makes the deposit counters recomputable from the state itself, ensuring epoch summaries stay consistent with the persisted detail rows.
Changes:
- Add a per-state flag (
PendingDepositsProcessed) to track whether pending-deposit counters have been accumulated for that specific state object, and reset it whenRefreshBlocksclears counters. - Refactor pending-deposit processing into an idempotent
processPendingDepositsFor(state)and invoke it for bothNextState(normal accumulation) andCurrentState(recompute on replaced objects before exporting epoch metrics). - Add focused unit tests covering recomputation, idempotence, and refresh/reset interactions (including the
RefreshBlocks-flag reset regression).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pkg/spec/state.go |
Adds PendingDepositsProcessed flag and ensures RefreshBlocks resets the flag and DepositedAmounts alongside deposit counters. |
pkg/spec/metrics/state_electra.go |
Introduces processPendingDepositsFor (idempotent), uses it for NextState and recomputation on CurrentState prior to epoch export. |
pkg/spec/metrics/deposit_recompute_test.go |
Adds unit tests validating recomputation on replaced states, idempotence, and refresh behavior. |
pkg/spec/deposit_counters_test.go |
Adds spec-level tests ensuring fresh states are unmarked and RefreshBlocks clears the accumulation flag. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks @leobago. The guard now lives inside I also replaced
While adding those tests, I found another issue in
I checked the blast radius before making the change: |
|
One thing I could not action, @leobago: the review says the fix is "incomplete in two ways and leaves one adjacent bug untouched", but the text ends after point 1. There are no inline comments on the PR and no follow-up comment carrying points 2 or the adjacent bug, so I have only the Could you paste the rest when you get a chance? The |
|
Thank you @ander-deran-arteaga. The deposit fix is complete, correct and well tested, so it can merge. The adjacent bug of the exact same shape, is in the consolidation counters, and is not touched by this PR and needs the same pattern here or as a fast follow. The deposit fixPreviously, the diagnosis was right but the fix was incomplete, because Fixed exactly as recommended. Also nice that you caught the related bug in The adjacent bug, consolidation countersSorry I left that part out in the previous review. The epoch row reads The fix is the pattern this PR already established. Add a |
ConsolidationsProcessed and ConsolidationsProcessedAmount have the same shape as the deposit counters fixed in the previous commit: appended to and accumulated with += while the state is NextState, read from CurrentState an epoch later by standard.go. They had both of the same failure modes. They double when an epoch is reprocessed against a retained NextState, because processPendingConsolidations ran unconditionally, and they read zero when the CurrentState object was re-downloaded in between, while t_consolidations still holds the rows. RefreshBlocks reset neither. Adds PendingConsolidationsProcessed, an early return in processPendingConsolidationsFor, a CurrentState recompute in PreProcessBundle next to the deposit one, and the resets in RefreshBlocks. The recompute touches CurrentState only, which feeds the epoch row; t_consolidations is persisted from NextState, so no row is written twice. Eight tests, each verified to fail without the part of the fix it covers, including one at the PreProcessBundle level that pins the call site rather than the function.
f_deposits_numis written as exactly 0 for some epochs whilet_depositsholds the corresponding rows. Never slightly wrong, always exactly zero.The counters are accumulated on one state and read from another, an epoch later:
The value written into the epoch row for E-1 was accumulated while that object was
NextState, during the processing of E-1. It survives only if the same object is still cached when E is processed. Three things replace or reset it in between:AdvanceFinalizeddeletes and re-downloads a state whose root changedensureDependencyStatesre-downloads states evicted byCleanUpToRefreshBlocksresets the counters outright, and post-ElectraAddBlocksdoes not refill them, since deposits arrive through the pending queue rather than block bodiesA replaced object carries zeros and nothing recomputes them, so the epoch row reports no deposits while the detail rows exist.
Measured on mainnet: 252 epochs over 450421-470670, 1,653 deposits and roughly 113,000 ETH missing from the summaries, against 10,029 epochs written correctly. 97.5% right, and wrong in exactly the way a lost accumulator is wrong.
The same reading explains why the consolidation counters fail on a different set of epochs, overlapping the deposit failures in only 18 of 37 cases:
RefreshBlocksresets the deposit counters and not the consolidation ones, so the two have different wipe conditions.Fix: record on the state whether its deposit counters were ever accumulated, and recompute from the state itself when they were not. What
processPendingDepositsForderives depends only on the state handed to it, so a re-downloaded state reaches the numbers the original run reached. It must run at most once per object, since the counters accumulate with+=rather than being assigned, and the new flag enforces that.The recomputation runs before the existing map clears, so its contribution to
DepositedAmountsis discarded exactly as the surrounding code expects and the reward path repopulates it afterwards; only the two counters survive. Deposit rows are persisted fromNextState.DepositsProcessed, so recomputing forCurrentStatecannot duplicate them. The common case costs a boolean check.Seven tests. One found a hole in the first version of this fix:
RefreshBlockszeroed the counters without clearing the flag, so a refreshed state still claimed it had been accumulated and the recomputation never fired.Closes #287