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
- 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
Assessment
This issue has not been assessed yet.