Keep ChainHead epoch and slot consistent - #200
Open
cyc60 wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ChainHeadhas independentepochandslotfields and nothing keeps them consistent. Two of the four head-fetching functions can return a head whereepoch != slot // slots_per_epoch:get_chain_epoch_head(epoch=E)starts atE * slots_per_epochand walks backwards, returningChainHead(epoch=E, slot=slot_id - i). When epoch E's first slot was not proposed, the result claims epoch E while the slot actually lies in epoch E-1.get_chain_justified_headhas the same shape: it pinsepochto thecurrent_justifiedcheckpoint and walks backwards from that epoch's boundary, so the returned slot may belong to the previous epoch.get_chain_finalized_headandget_chain_latest_headboth deriveepoch = slot // slots_per_epochand were already correct.This is not theoretical. It caused a permanent consensus bug in v3-oracle:
PendingDepositScannerkeyed its work onchain_head.epochwhile validator balances are read atchain_head.slot, and the snapshot-restore path reconstructs its checkpoint fromupdate_timestamp, which only yields a slot. A restored oracle rebuilt the checkpoint one epoch below the one the voting oracles had stored, rescanned that epoch, and double-counted the deposits processed at the boundary — leavingtotal_depositspermanently too high and that oracle signing a different Merkle root than every peer. It only bites where the stepped-back path is live, i.e.WORKER_EPOCH_DIVISOR > 1(Gnosis). stakewise/v3-oracle#636 guards the scanner independently; this PR removes the underlying trap.get_chain_epoch_headNow walks forward within the requested epoch (
slot_id + i), so the returned slot always lies inside that epoch and the invariant holds by construction. In the common case — the epoch's first slot was proposed — this is identical to the previous behaviour, since it matches ati = 0. Only the missed-first-slot case changes, and it changes from "last block of the previous epoch" to "first proposed block of the requested epoch", which is what the function name promises. The 404 skip, the pre-Shapellaeth1_datafallback, and the terminalRuntimeErrorare all preserved.get_chain_justified_headKeeps the backwards walk — walking forward here would step past the justified boundary into non-justified slots — and instead derives
epochfrom the slot that was actually found.This hunk is a deliberate semantic change and is separable. When the justified epoch's first slot was missed,
.epochnow reports the epoch the slot really belongs to rather than the justified checkpoint epoch. Any consumer reading.epochas "the justified checkpoint epoch" while.slotsat in an earlier epoch had a latent instance of the same bug, but I could not audit consumers outside v3-oracle (which does not use this function). Happy to drop this hunk and its test if you would rather handle it separately.Also
ChainHeadgains a docstring stating the invariant. No field or API changes; consumers construct it directly.sw_utils/tests/test_consensus.py, which had no coverage before: both walk directions, the all-slots-missedRuntimeError, the pre-Shapella fallback and itsBlockNotFoundcontinuation, and a parametrized invariant check (32- and 16-slot epochs) across all four head-fetching functions. Six of these fail against the pre-fix implementation.