cl: GLOAS audit fixes — clone aliasing, PTC consistency, memory leaks, nil panics#21248
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses several audit findings in Erigon's GLOAS (EIP-7732) consensus-layer code path: it tightens nil-handling around malformed SSZ inputs, deep-copies ExecutionPayloadEnvelope clones via SSZ roundtrip, makes notifyPtcMessages use a single consistent state for PTC derivation, bounds the pendingELPayloads queue and the blockTimeliness sync.Map, and replaces fragile strings.Contains topic-score matching with exact equality. New unit tests cover the cap/drain behavior, clone with nil message, exact topic matching, PTC window accessor, and nil envelope rejection.
Changes:
- PTC vote consistency:
notifyPtcMessagesnow usesblockState.GetPTCFromWindowand iterates aggregation bits directly;GetPTCFromWindowis exported and gains a nil-window guard. - Memory/lifetime fixes: cap
pendingELPayloadsat 1024 with drain that releases or reuses the backing array; clean upblockTimelinesson finalization; deep-cloneExecutionPayloadEnvelopevia SSZ roundtrip; useGloasVersioninNewExecutionPayloadEnvelope; add nil guards inProcessExecutionPayloadBid,verifyExecutionPayloadBidSignature, andProcessExecutionPayloadEnvelope. - Gossip topic scoring switched from substring to exact match.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cl/transition/impl/eth2/operations.go | Adds nil guards for signed bid/envelope, payload, withdrawals, expected withdrawals, builder index range. |
| cl/transition/impl/eth2/operations_gloas_test.go | New test verifying nil envelope rejection. |
| cl/phase1/network/gossip/score.go | Replaces strings.Contains with == for topic score lookups; drops strings import. |
| cl/phase1/network/gossip/score_test.go | New test for exact topic matching. |
| cl/phase1/forkchoice/utils.go | Cleans up blockTimeliness entries for finalized/pruned blocks in onNewFinalized. |
| cl/phase1/forkchoice/pending_el_payload_test.go | New tests for cap-and-drop and large-backing-array release. |
| cl/phase1/forkchoice/payload_vote.go | Removes redundant state cache field; uses GetPTCFromWindow; iterates aggregation bits directly. |
| cl/phase1/forkchoice/payload_vote_test.go | New tests for GetPTCFromWindow happy/error paths. |
| cl/phase1/forkchoice/forkchoice.go | Adds maxPendingELPayloads/pendingELPayloadsShrinkCap; enforces cap; drain releases or reuses backing array. |
| cl/phase1/core/state/cache_accessors.go | Exports GetPTCFromWindow; adds nil-window guard. |
| cl/cltypes/epbs_payload.go | Deep-clone envelope via SSZ roundtrip; nil-safe SignedExecutionPayloadEnvelope.Clone; fixes default Payload version to Gloas. |
| cl/cltypes/epbs_payload_test.go | New test verifying clone with nil Message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(f.pendingELPayloads) >= maxPendingELPayloads { | ||
| log.Warn("addPendingELPayload: dropping oldest pending EL payload", "queueLen", len(f.pendingELPayloads)) | ||
| copy(f.pendingELPayloads, f.pendingELPayloads[1:]) | ||
| f.pendingELPayloads[len(f.pendingELPayloads)-1] = PendingELPayload{} | ||
| f.pendingELPayloads = f.pendingELPayloads[:len(f.pendingELPayloads)-1] | ||
| } |
There was a problem hiding this comment.
Acknowledged — the O(n) shift is real but this code only runs when the queue hits 1024 (abnormal condition, already logged as Warn). Under normal operation the cap is never reached. A ring buffer would be O(1) amortized but adds complexity for a safety valve that is not on the hot path. Leaving as-is for now.
…sip scoring - forkchoice/utils: prune blockTimeliness sync.Map in onNewFinalized (memory leak) - forkchoice: cap pendingELPayloads at 1024 with memory-conscious drain strategy - forkchoice/payload_vote: use copy=true in notifyPtcMessages to avoid shared mutable state - cltypes: deep-copy ExecutionPayloadEnvelope.Clone() via SSZ round-trip - cltypes: fix NewExecutionPayloadEnvelope to use GloasVersion instead of BellatrixVersion - operations: add nil checks for signedBid/signedBid.Message in ProcessExecutionPayloadBid - operations: add BuilderIndex bounds check in verifyExecutionPayloadBidSignature - gossip/score: replace strings.Contains with exact topic match for GLOAS topics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… Clone error logging - payload_vote: pass PTC bit-position directly to applyPayloadAttestationVote instead of passing validatorIndex and re-scanning the PTC slice. Eliminates O(PtcSize²) per attestation and fixes incorrect PTC slot assignment when a validator appears more than once in the committee. - epbs_payload: log.Error on SSZ encode/decode failure in ExecutionPayloadEnvelope.Clone() so the error is visible (clonable.Clonable interface has no error return). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3a93161 to
97a505b
Compare
yperbasis
left a comment
There was a problem hiding this comment.
LGTM, nice fixes. A few notes (none blocking):
-
Test gap — no regression test for the actual deep-copy property of
ExecutionPayloadEnvelope.Clone().TestSignedExecutionPayloadEnvelopeCloneNilMessageonly exercises the nil-Message path. Worth adding a test that clones a populated envelope, mutates a field on the clone (e.g.Payload.BlockNumberorBuilderIndex), and verifies the original is unchanged — this is the core bug the PR is fixing. -
Latent nil-panic in
Clone()ife.beaconCfg == nil.NewExecutionPayloadEnvelope(nil)→NewEth1Block(GloasVersion, nil)→ dereferencesnil.MaxBytesPerTransaction. Production envelopes carry abeaconCfgso it's not reachable today, but ane.beaconCfg == nilearly-return would harden the fallback. The previous shallow code didn't allocate, so this case used to work. -
Duplicate-validator PTC issue still exists in
OnPayloadAttestationMessage. Copilot flagged this for the in-block path (fixed here) but the gossip path atcl/phase1/forkchoice/on_payload_attestation_message.go:70-79still does a linear scan that picks the first PTC index when a validator appears twice. Pre-existing, not a regression — but worth a follow-up tracking issue ifget_ptccan actually produce duplicates. -
Style nits. In
cl/cltypes/epbs_payload.gothe newlog \"github.com/erigontech/erigon/common/log/v3\"alias differs from the surrounding files (which import the package unaliased). Incl/phase1/forkchoice/forkchoice.gothe same import is in its own group between stdlib and the erigontech block, breaking the grouping. New test files also lack copyright headers (mixed convention incl/, but most have them). -
addPendingELPayloadlog spam. When the queue stays at cap under sustained back-pressure,log.Warnfires per insertion (up to 1024×). Acceptable since the cap shouldn't be hit under normal operation, but a one-shot or rate-limited warning would be friendlier.
Summary
notifyPtcMessages: old code used two different states (svsblockState) for PTC computation, causing validators to be mapped to wrong PTC positions. Now usesblockState.GetPTCFromWindowconsistently and iterates aggregation bits directly, eliminating the intermediateGetIndexedPayloadAttestationallocation and sort.ExecutionPayloadEnvelope.Clone()shallow copy: old Clone sharedPayloadandExecutionRequestspointers (aliasing). Now deep-copies via SSZ roundtrip. Also fixesNewExecutionPayloadEnvelopeto useGloasVersioninstead ofBellatrixVersion.ProcessExecutionPayloadBid,verifyExecutionPayloadBidSignature, andProcessExecutionPayloadEnvelopefor malformed SSZ inputs.pendingELPayloadsat 1024 to prevent unbounded growth;DrainPendingELPayloadsnow reuses backing array when small, releases to GC when large.blockTimelinesssync.Map inonNewFinalized(was growing without bound).strings.Containsto exact==match, removing fragile ordering dependency betweenexecution_payloadandexecution_payload_bid.No pre-GLOAS behavior changes — all fixes are in GLOAS-specific code paths or produce identical results for pre-GLOAS topics.
Test plan
TestSignedExecutionPayloadEnvelopeCloneNilMessage— nil Message CloneTestGetPTCFromWindow/TestGetPTCFromWindowRejectsSlotOutsideWindow— PTC window accessorTestPendingELPayloadsDropOldestAtCap/TestDrainPendingELPayloadsReleasesLargeBackingArray— cap limit and drainTestTopicScoreParamsExactTopicMatching— exact topic match, rejects suffixed namesTestProcessExecutionPayloadEnvelopeRejectsNilEnvelope— nil envelope rejectionmake lintpassesmake test-short)🤖 Generated with Claude Code