Pull request overview

Fixes Caplin forkchoice stalls after the GLOAS merge by unblocking beacon-state reads/decodes on mainnet-sized states and reducing lock contention in head computation.

Changes:

• Removes the fixed SSZ size cap and avoids redundant beacon-state cache initialization during disk reads.
• Computes checkpoint state before acquiring the forkchoice mutex to prevent long OnTick stalls.
• Adds a fallback to use the anchor state when a checkpoint state can’t be obtained from the fork graph.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

		return nil, fmt.Errorf("failed to read length: %d, want 8, root: %x", n, blockRoot)
	}

	length := binary.BigEndian.Uint64(lengthBytes)
	if length > uint64(cap(f.sszBuffer)) {
		f.sszBuffer = make([]byte, length)
	} else {
		f.sszBuffer = f.sszBuffer[:length]
	}

		return nil, fmt.Errorf("failed to read length: %d, want 8, root: %x", n, blockRoot)
	}

	envelopeLength := binary.BigEndian.Uint64(lengthBytes)
	if envelopeLength > uint64(cap(f.sszBuffer)) {
		f.sszBuffer = make([]byte, envelopeLength)
	} else {
		f.sszBuffer = f.sszBuffer[:envelopeLength]
	}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Nice diagnosis and the four issues are all real, but I'd like to see a couple of correctness/safety concerns addressed before merging.

Major

1. Anchor fallback cached under the wrong key

In utils.go, after the fallback to the anchor state:

f.checkpointStates.Store(checkpoint, checkpointState)

checkpointState is built from the anchor — actives, activeBalance, and state.Epoch(baseState.BeaconState) reflect the anchor's epoch, not checkpoint.Epoch. This gets cached under the missing checkpoint's key and serves all subsequent calls for that checkpoint, so:

• Attestation weights in computeVotes use anchor-epoch active sets / balances rather than the checkpoint-epoch ones.
• If the real checkpoint state becomes available later (e.g. via backfill), the cache keeps returning the stale fallback.

Options: build actives/balances from a state aligned to checkpoint.Epoch, or skip caching the fallback so the next call re-resolves. At minimum, document the inaccuracy and consider not caching when baseState came from the fallback path.

2. Latent data race on forkGraphDisk.currentState / currentStateBlockRoot

The pre-existing implicit protection was that OnBlock → AddChainSegment writes f.currentState/f.currentStateBlockRoot under f.mu, and all callers of forkGraph.GetState were also under f.mu. Moving getCheckpointState out from under f.mu means the read at fork_graph_disk.go:480 (if f.currentState != nil && f.currentStateBlockRoot == blockRoot) can now race with the AddChainSegment write. Worst case is a torn 32-byte hash read causing a spurious cache miss (still correct, just slower), but the race detector will likely flag this. Consider giving forkGraphDisk its own RWMutex over those two fields, or atomic.Pointer[CachingBeaconState] + a parallel atomic hash.

Minor

3. Unbounded allocation from disk length field

Removing maxSSZBufferSize lets a corrupt or truncated *.snappy_ssz file drive make([]byte, length) with an arbitrary 64-bit value before any SSZ validation. It's our own datadir (not an attack surface), but a partially-written file after an unclean shutdown could now OOM/panic instead of returning a clean error. Consider keeping a much more generous cap (e.g. bump to 1 GiB, or bound by file size × max snappy expansion).

4. Stale error message after the fallback

if baseState == nil {
    return nil, errors.New("getCheckpointState: baseState not found in graph")
}

After the anchor fallback also fails, this still says "not found in graph". Suggest "getCheckpointState: baseState and anchor fallback both missing".

5. Performance: anchor re-read per missing checkpoint

Each new justified checkpoint that misses the graph triggers a fresh anchor-state read from disk (~327 MB on mainnet). A small LRU (or just remembering the anchor *checkpointState) would amortize this during the post-sync window.

Test coverage

The bug was a silent multi-month stall on mainnet that escaped CI. Worth adding:

• A unit test for the getCheckpointState fallback path (fork graph returns nil for the checkpoint root, verify anchor is consulted).
• A unit test for readBeaconStateFromDisk round-tripping a >128 MB blob to lock in that mainnet-sized states are accepted.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Follow-up commit 1c66977 cleanly addresses my prior review. One small nit from Copilot #2 worth tightening — the field comment on currentState / currentStateBlockRoot (fork_graph_disk.go:97-99) says they're "protected by currentStateMu when accessed concurrently", but two read sites still skip the mutex: isBlockRootTheCurrentState (lines 215-219) and the missing-segment diagnostic in AddChainSegment (lines 254-256). Currently safe because AddChainSegment only runs under the outer f.mu (held by OnBlock), so f.mu serializes those reads against the writer — but the comment promises a uniform invariant the code doesn't actually maintain, and a future read path added outside f.mu without currentStateMu would silently race.

Two options, your pick:

1. Cheapest fix — wrap the two unguarded reads in currentStateMu.RLock() / RUnlock(). Overhead is negligible and the invariant becomes uniform.
2. Narrow the doc — keep the dual-protection scheme but rewrite the field comment to spell it out, e.g. "Reads under f.mu rely on that lock (only AddChainSegment writes); reads outside f.mu must take currentStateMu".

Either is fine — approving so this isn't blocked.