fix(op-node): carry attributes in deposits-only request event

[ajsutton]

Summary

Makes DepositsOnlyPayloadAttributesRequestEvent self-contained by carrying the original AttributesWithParent, eliminating the race condition where a pipeline reset clears lastAttribs between BuildStartEvent and PayloadProcessEvent.

This is an alternative to #19417 which changes CriticalErrorEvent to ResetEvent. That fix papers over the race by retrying; this fix eliminates the root cause by removing the dependence on mutable pipeline state.

What changed

• DepositsOnlyPayloadAttributesRequestEvent now carries an Attributes field
• EngineController stores attributes from BuildStartEvent in lastBuildAttribs (survives pipeline resets)
• All emit sites pass attributes through: build_invalid.go uses ev.Attributes directly, payload_process.go uses e.lastBuildAttribs
• New ApplyDepositsOnly method on AttributesQueue/DerivationPipeline accepts provided attributes, flushes channels, and returns deposits-only version — without reading lastAttribs
• Missing or mismatched attributes is a CriticalErrorEvent — no fallback

Emit site verification

There is exactly one place the event is constructed: emitDepositsOnlyPayloadAttributesRequest. All three call sites go through it:

1. build_invalid.go:47 — passes ev.Attributes directly from BuildInvalidEvent. Always correct — same build cycle.
2. payload_process.go:46 — SuperAuthority deny path. Passes e.lastBuildAttribs.
3. payload_process.go:65 — Holocene ExecutionInvalid. Passes e.lastBuildAttribs.

Staleness analysis for lastBuildAttribs

lastBuildAttribs is set in onBuildStart and read in onPayloadProcess. Could a second BuildStartEvent overwrite it before the first build's PayloadProcessEvent fires?

The AttributesHandler prevents concurrent builds. Its sentAttributes flag (set true before emitting BuildStartEvent, checked at line 167) prevents emitting a second BuildStartEvent while one is in flight. The flag is only cleared on reset, temporary error, or when attributes are consumed.

After a reset, stale builds fail at the EL. If a ResetEvent interleaves and eventually produces a new BuildStartEvent (overwriting lastBuildAttribs), the stale build's BuildSealEvent calls engine.GetPayload — but forceReset has already changed the EL's forkchoice via tryUpdateEngine, invalidating the old payload ID. The EL returns UnknownPayload, producing PayloadSealExpiredErrorEvent instead of PayloadProcessEvent. The deny check never fires for the stale build.

Defense-in-depth validation catches any remaining edge case. Both emitDepositsOnlyPayloadAttributesRequest and the handler in PipelineDeriver validate that the carried attributes match the event's Parent and DerivedFrom. If lastBuildAttribs is stale (wrong parent or derivation origin), the mismatch check fires a CriticalErrorEvent rather than silently using wrong attributes. This prevents incorrect derivation.

Why the race exists

Events are queued, not inline. Between BuildStartEvent (which queues ForkchoiceUpdateEvent + BuildStartedEvent) and PayloadProcessEvent (where the deny check fires), a ResetEvent can interleave and clear lastAttribs on the AttributesQueue. The deny check then emits DepositsOnlyPayloadAttributesRequestEvent, whose handler previously read lastAttribs — which was nil.

The EngineController's lastBuildAttribs field survives pipeline resets (only the pipeline's internal AttributesQueue.lastAttribs is cleared), bridging the gap.

Test plan

• New unit tests: TestApplyDepositsOnly_WithNilLastAttribs and TestApplyDepositsOnly_PreservesParentAndDerivedFrom
• Updated TestSuperAuthority/DeniedPayload_EmitsDepositsOnlyRequest to set lastBuildAttribs
• All derive, engine, and attributes tests pass
• Full op-node build succeeds
• Should be validated with the supernode interop reorg acceptance tests from test(interop): add txpool eviction test for invalid exec msg on reorg #19346

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

[ajsutton]

Claude: Reviewed for correctness issues that could cause derivation to reach incorrect results. No issues found.

Analysis of the five areas of concern:

1. lastBuildAttribs staleness: Safe. EngineController.OnEvent holds e.mu for the duration of each event handler. The event chain BuildStartEvent → BuildStartedEvent → BuildSealEvent → BuildSealedEvent → PayloadProcessEvent is synchronous, and onBuildStart always sets lastBuildAttribs before any downstream handler reads it. No concurrent BuildStartEvent can interleave due to the mutex.

2. ApplyDepositsOnly vs old DepositsOnlyAttributes: The skipped sanity checks are either inapplicable or relocated. The batch != nil check is irrelevant when attributes are carried in the event (the attributes are already produced, so batch state doesn't matter). The parent/derivedFrom consistency checks are now performed by both emitDepositsOnlyPayloadAttributesRequest (engine side) and PipelineDeriver.OnEvent (pipeline side) before calling ApplyDepositsOnly. FlushChannel() is still called. Correct.

3. lastAttribs update in ApplyDepositsOnly: Setting aq.lastAttribs = attrs keeps the pipeline's AttributesQueue state consistent for any subsequent pipeline stepping or a future DepositsOnlyAttributes call. This matches the behavior of the old code path and is necessary.

4. Validation of ev.Ref.ParentID() vs lastBuildAttribs.Parent.ID(): ev.Ref is derived from the execution payload via PayloadToBlockRef, which was built from the same attributes stored in lastBuildAttribs. The parent hash and number will match. The validation guards against the (now-impossible) case where they diverge.

5. lastBuildAttribs never cleared: While it persists across builds, every PayloadProcessEvent is necessarily preceded by a BuildStartEvent that refreshes it. The parent/derivedFrom validation provides a safety net against any hypothetical staleness. This is sufficient — adding an explicit clear (e.g., after PayloadSuccessEvent) would be a defense-in-depth improvement but is not necessary for correctness.