erigontech / erigontech/erigon

cl/phase1/forkchoice: OnBlock holds the fork-choice lock across the blocking EL NewPayload call (pre-Gloas), unlike the already-fixed Gloas envelope path

Open
#23,529 0 comments 0 reactions 1 assignee Claimed by @Sahil-4555 View on GitHub
Caplin
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

### Summary

`ForkChoiceStore.OnBlock` in `cl/phase1/forkchoice/on_block.go` acquires the fork-choice store's exclusive lock, `f.mu`, at function entry and, on the normal pre-Gloas path, holds it across a call chain into `engine.NewPayload`.

`engine.NewPayload` is an Engine API call whose latency depends on the execution layer's current load, including block execution and state-root computation. While this call is in progress, every other consumer requiring `f.mu` is blocked, including `OnTick`, `OnAttestation`, `GetHead`, and other fork-choice reads used by RPC or validator duties.

This is a **lock-scope and liveness problem**, not a conventional data race.

A similar problem has already been addressed in the Gloas execution-payload-envelope path, where `newPayloadWhileYieldingForkChoiceLock` explicitly releases `f.mu` before the blocking EL call and re-acquires it before committing the result. The corresponding pre-Gloas `OnBlock` path does not currently do this.

The gap remains present on `main` at 6bea6b1c68.

### Root Cause

`ForkChoiceStore.OnBlock` acquires the exclusive fork-choice lock near function entry:

```go
f.mu.Lock()
```

On the normal pre-Gloas path, the lock remains held while `OnBlock` calls:

```go
f.NewPayloadWithAdmission(...)
```

The relevant call chain is:

```text
OnBlock

│ f.mu.Lock()

OnBlock processing


NewPayloadWithAdmission(...)


engine.NewPayload(...)
```

`NewPayloadWithAdmission` does not itself yield `f.mu`; it reaches:

```go
f.engine.NewPayload(...)
```

As a result, the fork-choice mutex remains locked for the full duration of the EL call.

### Existing Correct Pattern in the Gloas Path

`newPayloadWhileYieldingForkChoiceLock` in `cl/phase1/forkchoice/on_execution_payload.go` explicitly yields the lock:

```go
f.mu.Unlock()
defer f.mu.Lock()

return f.withPayloadValidationAdmission(ctx, func() (...) {
if f.forkGraph.HasEnvelope(beaconBlockRoot) {
return execution_client.PayloadStatusValidated, nil
}

return f.engine.NewPayload(
ctx,
payload,
parentBlockRoot,
versionedHashes,
executionRequestsList,
)
})
```

This pattern ensures:

1. The potentially slow EL call does not hold the fork-choice mutex.
2. State can be revalidated after the lock is re-acquired before the result is committed.

The pre-Gloas path currently has no equivalent lock-yield behavior around:

```text
OnBlock
→ NewPayloadWithAdmission
→ engine.NewPayload
```

### Previously Identified but Deferred Gap

The lock-yield pattern was introduced in #22683:

> `cl: fix Gloas checkpoint sync with external execution clients`

That work explicitly did not perform a general pre-Gloas `OnBlock` lock-yield refactor.

The stacked follow-up draft #23249, `cl: preserve expanded Gloas lifecycle hardening`, touches `on_block.go` and includes fork-choice lock lifecycle changes, but its relevant changes are limited to stale-state refresh around the Gloas-specific pending-envelope path.

It does not modify the pre-Gloas `NewPayloadWithAdmission` call site or introduce equivalent unlock/relock behavior around the pre-Gloas `engine.NewPayload` call.

Therefore, the gap appears to remain open on:

```text
main @ 6bea6b1c68
```

### Why This Matters

There is already production precedent for this exact class of failure in the same fork-choice component.

Issue #22351 documented a mainnet incident where another operation performed while holding `f.mu` caused significant fork-choice contention. In that case, `latestMessagesStore.set` performed an O(n) map scan while `OnAttestation` held the lock.

`GetHead` was observed waiting approximately:

```text
1.2–1.5 seconds per slot
```

behind `f.mu`, contributing to validator-facing timing problems including wrong-head and late or missed attestations.

The contention pattern here is the same:

```text
Slow operation


held while owning f.mu

├── OnTick blocked
├── OnAttestation blocked
├── GetHead blocked
└── other fork-choice consumers blocked
```

The slow operation differs:

```text
#22351:
O(n) latest-messages map processing

This issue:
Blocking engine.NewPayload call
```

The hazard class is nevertheless the same: **a potentially long-running operation is performed while holding the global fork-choice mutex**.

### Impact

When `OnBlock` processes a gossip, API, or forward-sync block and the execution layer is slow or temporarily overloaded, `f.mu` remains locked for the duration of `engine.NewPayload`.

#### `OnTick` can stall

The slot-processing path can block attempting to acquire `f.mu`, potentially delaying proposer-boost-root reset, unrealized-checkpoint promotion, and other slot-boundary processing.

#### `OnAttestation` can stall

Incoming attestation processing can queue behind the mutex while the EL call is in progress.

#### `GetHead` and related reads can stall

RPC handlers, validator-duty logic, head queries, and other fork-choice reads can block for the same duration.

### Failure Scenario

```text
Goroutine A: OnBlock(block)

├── acquires f.mu

├── processes block

└── calls engine.NewPayload(...)

│ EL is slow

│ f.mu remains locked

├───────────────────────────────┐

Goroutine B: OnTick() │
│ │
└── waits for f.mu ◄────────────────────┤

Goroutine C: OnAttestation()

└── waits for f.mu ◄────────────────────┤

Goroutine D: GetHead()

└── waits for f.mu ◄────────────────────┘
```

Under sustained EL load, sync catch-up, expensive block execution, or storage stalls, this can significantly extend the critical section.

### Why Existing Tests and `go test -race` Do Not Catch This

This is not a data race. The accesses may all be correctly synchronized from the race detector's perspective.

The problem is:

```text
Correct synchronization
+
Incorrect lock scope
+
Potentially blocking external operation
=
Liveness and latency failure
```

Therefore, `go test -race` does not detect this class of issue by itself.

Existing tests generally use fast or mock execution-layer implementations where `NewPayload(...)` returns immediately. The contention window is therefore not meaningfully exercised.

Existing tests may verify that `OnBlock`, `OnTick`, `OnAttestation`, and `GetHead` eventually succeed without testing whether they can make progress **while `OnBlock` is blocked inside the EL call**.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.