cl: catch up Gloas alpha11 execution requests - #22091
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Erigon’s CL (Caplin) Gloas support to match the latest consensus-specs execution-request shape (now including builder deposits/exits), tightens execution-payload-envelope request limits via MAX_REQUEST_PAYLOADS, and adds/updates a broad set of regression + spectest coverage (including bumping fixtures to v1.7.0-alpha.11).
Changes:
- Extend
ExecutionRequeststo include Gloasbuilder_depositsandbuilder_exitsacross SSZ/JSON/hash/clone, plus decoding/encoding from/to the flat EIP-7685 list representation. - Add builder deposit/exit request processing through state transition and supporting EPBS/bid validation changes.
- Enforce and propagate
MAX_REQUEST_PAYLOADScaps for execution payload envelope by-range/by-root (server, client, and chunked retry logic), plus Beacon REST hardening (size bounds, content-type parsing, etc.).
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test-fixtures.json | Bump consensus-spec fixtures to v1.7.0-alpha.11 mainnet tarball. |
| execution/types/eip7685_requests.go | Add builder request type constants and flat request lengths/types. |
| execution/engineapi/sszrest_wire.go | Decode execution requests via cltypes.DecodeExecutionRequestsList and ignore requests pre-Electra in SSZREST getPayload. |
| execution/engineapi/sszrest_test.go | Add tests for builder request decoding and pre-Electra getPayload behavior. |
| cl/transition/machine/machine.go | Extend block operation processor interface with builder deposit/exit handlers. |
| cl/transition/machine/block.go | Explicitly reject builder-index voluntary exits. |
| cl/transition/impl/eth2/operations.go | Wire builder requests into parent payload processing; update bid validation and pending-payment proposer binding; add nil hardening. |
| cl/transition/impl/eth2/operations_gloas_test.go | Add regression tests for proposer slashing clearing logic and builder request handling. |
| cl/spectest/consensus_tests/operations.go | Add spectest handlers for builder deposit/exit requests; adjust bid handler input. |
| cl/spectest/consensus_tests/appendix.go | Register new operations + SSZ static tests; version-aware ExecutionRequests instantiation. |
| cl/sentinel/handlers/execution_payload_envelopes.go | Apply MAX_REQUEST_PAYLOADS enforcement to by-range/by-root handlers. |
| cl/sentinel/handlers/execution_payload_envelopes_test.go | Update tests for version-aware requests and new payload caps; add over-limit test. |
| cl/rpc/rpc.go | Add MaxRequestPayloads() helper and enforce limits for envelope requests. |
| cl/rpc/rpc_test.go | Add tests for envelope request limit enforcement and fallback behavior. |
| cl/phase1/stages/gloas_payload_test.go | Use version-aware ExecutionRequests for Gloas fixtures. |
| cl/phase1/network/services/execution_payload_bid_service.go | Switch proposer-preference matching to forkchoice shuffling-dependent-root semantics; strengthen bid validation; refine pending-bid keying. |
| cl/phase1/network/services/execution_payload_bid_service_test.go | Expand bid service tests for version gating, dependent root, blob limits, randao checks, and pending-queue key uniqueness. |
| cl/phase1/network/envelopes.go | Chunk by-root/by-range envelope requests using MAX_REQUEST_PAYLOADS; retain partial responses across failures. |
| cl/phase1/network/envelopes_test.go | Add tests for filtering unsolicited envelopes / requested-root retention. |
| cl/phase1/forkchoice/mock_services/forkchoice_mock.go | Respect alwaysCopy by returning a copied state when requested. |
| cl/phase1/core/state/upgrade.go | Use version-aware empty ExecutionRequests root during Gloas upgrade. |
| cl/phase1/core/state/epbs.go | Add builder-deposit signature verification; builder registry hardening/limits; overflow guards; implement builder deposit request application. |
| cl/phase1/core/state/epbs_test.go | Add tests for builder request signature domain behavior, registry limit enforcement, and overflow guards. |
| cl/cltypes/solid/builder_requests.go | New SSZ/HTR types for BuilderDepositRequest and BuilderExitRequest. |
| cl/cltypes/execution_requests.go | Expand ExecutionRequests to 5 lists with version-aware SSZ/JSON/HTR/clone + flat-list decoder. |
| cl/cltypes/epbs_payload.go | Ensure bid/container types support updated request root handling and list sizing. |
| cl/cltypes/epbs_payload_test.go | Add tests for proposer index inclusion in BuilderPendingPayment SSZ and clone behavior. |
| cl/cltypes/epbs_builder.go | Add proposer index field to pending payments; deep-copy withdrawal/payment clones; update SSZ/HTR. |
| cl/cltypes/beacon_block.go | Construct version-aware ExecutionRequests and include builder lists at Gloas boundaries; update flat-list encoding helper. |
| cl/cltypes/beacon_block_test.go | Add tests for builder request list encoding/decoding and JSON version gating/null handling. |
| cl/cltypes/beacon_block_blinded.go | Use version-aware ExecutionRequests for blinded body construction. |
| cl/clparams/config.go | Add MAX_REQUEST_PAYLOADS, builder request limits/types, DomainBuilderDeposit, and PayloadBuilderVersion to config. |
| cl/beacon/handler/epbs.go | Harden EPBS endpoints (size limits, content-type parsing, PTC duties caps); aggregate payload attestations; adjust bid response shape. |
| cl/beacon/handler/epbs_test.go | Add tests for new size/cap behaviors, SSZ bid submission, and payload attestation aggregation behavior. |
| cl/beacon/handler/block_production.go | Decode execution requests via unified decoder; compute Gloas requests root from envelope container; reject blinded blocks at Gloas. |
| cl/beacon/handler/block_production_test.go | Add test ensuring blinded blocks are rejected at Gloas. |
| cl/beacon/builder/client.go | Make ExecutionRequests instantiation version-aware for builder API calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yperbasis
left a comment
There was a problem hiding this comment.
Requesting changes. The per-bid full-state copy (inline on execution_payload_bid_service.go) is the blocker; a payload-attestation aggregation under-count and an error-handling nit follow inline.
Minor (pre-existing line, not inline): block_production_test.go:481 uses a version-0 &cltypes.ExecutionRequests{} in a Gloas FULL-payload fixture — prefer NewExecutionRequestsWithVersion(..., clparams.GloasVersion) to mirror a real envelope. Harmless today.
…indings - forkchoice: drop the forced full-state copy per envelope; verification only reads the state, so use the shared reference (reverts the alwaysCopy flip and replaces the stale comment) - bid service: fetch the parent state only on a validation-state cache miss, deduplicating concurrent fetches for the same (parent, slot) - engineapi: resolve the beacon config from the running chain at the SSZ-REST getPayload boundary instead of hardcoding mainnet - handler: derive the max signed bid SSZ size from EncodingSizeSSZ instead of magic numbers - clparams: single MaxRequestPayloadsLimit method replaces the fallback logic duplicated between cl/rpc and sentinel handlers - state: merge the builder/validator deposit signature verification into one domain-parameterized helper
|
Addressed another round of review findings in
One observation from the follow-up adversarial review, for the record: the alwaysCopy revert makes envelope processing a (benign, correct-valued) writer of the shared Validation:
An adversarial subagent review of the commit itself reported no correctness regressions (it specifically traced the |
yperbasis
left a comment
There was a problem hiding this comment.
High
-
cl/beacon/handler/epbs.go:866— queued bids get HTTP 400 and are never gossip-published.ProcessMessagenow returnsErrIgnore-wrapped errors when it queues a bid (proposer preferences / parent state not yet available), but the handler maps any error to 400 and returns before thePublishat line 879. Other pool handlers treaterrors.Is(err, services.ErrIgnore)as success (e.g.pool.go:312), andprocessPendingBidsnever publishes either — so a valid bid POSTed just before preferences arrive is rejected and never propagates from this node. Add theErrIgnorecarve-out, and consider publishing queued bids once they validate. -
cl/phase1/network/services/execution_payload_bid_service.go:369— unsynchronized copy of a possibly-shared live state, now cached. For head-parent bids (the common case)GetStateAtBlockRoot(root, false)returns the sharedf.currentStatepointer and releases the RLock on return; the subsequentSlot()/Copy()race withOnBlock's in-placeTransitionState, and the possibly-torn copy is persisted invalidationStateCacheand reused for randao/builder/signature checks. (Pattern pre-exists, e.g.proposer_preferences_service.go:114, but caching makes it worse.) Also, for non-head parents the call already returns a freshly replayed caller-owned state, so the unconditionalCopy()materializes the full state twice per miss —alwaysCopy=trueand dropping the explicitCopyremoves that; the head-root copy additionally needs to happen under the forkchoice lock. -
cl/phase1/network/services/execution_payload_bid_service.go:118—validationStateCachepins up to 4 fullCachingBeaconStates with no TTL or invalidation. Hundreds of MB each at mainnet scale, and entries are dead ~2 slots after creation (bids are only valid for the current/next slot) yet survive until LRU displacement. No other gossip service pins full states. Uselru.NewWithTTL/ slot-tick pruning, or cache something slimmer.
Medium
-
cl/phase1/network/services/execution_payload_bid_service.go:318— highest-bid check runs after state fetch and BLS verify. It is a field-only LRU lookup and an IGNORE condition with no spec-mandated order. With up to ~256 builders bidding per slot, every losing bid pays a full BLS verification before being discarded. Hoist the value check ahead of the expensive work (keeping the pre-Addre-check). -
cl/phase1/core/state/epbs.go:378— builder-registry logic duplicated across state, transition and gossip. Four copies of the builder-by-pubkey scan (epbs.go:233/335/381,operations.go:1794);ApplyBuilderDepositRequestduplicates ~80–85% ofApplyDepositForBuilder; and the usable-builder predicate is spelled out twice (validateBuilderAvailabilitybid service :410 vsProcessExecutionPayloadBidoperations.go:532–543) — where the nil/bounds guard atoperations.go:536is dead code (IsActiveBuilderjust performed those exact checks) and the same builder entry is fetched four times. Centralize state-layer helpers (BuilderIndexByPubkey,GetPayloadBuilder/ValidateBuilderForBid), preserving ErrIgnore-only-for-cover-bid on the gossip side and the self-build bypass. -
cl/cltypes/execution_requests.go:83— the Electra-vs-Gloas schema decision is hand-maintained in ~7 places. TheeffectiveVersion() < GloasVersionbranch is repeated inEncodingSizeSSZ/EncodeSSZ/DecodeSSZ/HashSSZplus both JSON methods, and the ordered type-byte mapping lives in two files (GetExecutionRequestsListif-chain,beacon_block.go:763, vs theDecodeExecutionRequestsListswitch that enforces strictly-ascending order). Missing one site at the next fork means silent hash/encoding divergence. A version-dispatchedschema()(exactBeaconBody.getSchemaprecedent) plus one canonical{typeByte, minVersion, list}table driving both encode and decode;Clone()'s five identical copy loops can share a small helper. -
cl/clparams/config.go:703— EIP-8282 type bytes added as yaml-configurable, but the spec defines them as Constants. A bogus yaml key silently diverges CL from the EL's hard constants (execution/types/eip7685_requests.go) at the SSZ-REST decode boundary. Since this extends the pre-existing 0x00–0x02 pattern, the proportionate fix is a startup cross-check asserting the cfg values match theexecution/typesconstants (all five types).
Low
cl/beacon/handler/epbs.go:271— onViewHeadState/aggregation failure the pool GET returns an empty 200 with a Debug-only log; sibling endpoints surface 503 (e.g. :917).- Nits: verbatim-duplicated queue-and-ignore blocks in
ProcessMessage(bid service :184/:201) andGetHeaderrunning twice per accepted bid;executionRequestsFromList(sszrest_wire.go:261) is a one-line pass-through — inline it;maxExecutionPayloadEnvelopeRequestSize = MaxRlpBlockSize*4borrows an EL RLP constant with an undocumented ×4 while sibling caps derive from SSZ sizes.
|
Addressed the latest review feedback in a200ffe:\n\n- Split queued bid handling from hard ErrIgnore with a new ErrBidQueued sentinel. REST now returns 200 and republishes only for actually queued dependency/preference cases; hard ignore cases return 400 and are not gossiped.\n- Moved the highest-bid check ahead of proposer-preference/state dependency work, while keeping the final re-check before storing. This rejects lower bids before pending queue/state fetch work.\n- Added TTL to the bid validation-state cache so full copied states are bounded by both size and slot-time expiry.\n- Added CL/EL execution request type constant validation for scheduled Electra and Gloas request types, including the Gloas-only edge where base request types are still present in the Gloas schema.\n\nValidation:\n- make lint\n- go test ./cl/beacon/handler -run 'TestPostExecutionPayloadBid'\n- go test ./cl/phase1/network/services -run 'TestExecutionPayloadBidService(HighestBid|RejectsLowerBidBeforeStateFetch|WaitsForProposerPreferences|WaitsForParentState)'\n- go test ./cl/clparams -run 'TestCustomConfig'\n- git diff --check\n\nI also reran the adversarial subagent review loop after the fixes; both the safety/perf and spec/regression reviewers reported convergence with no new actionable findings. |
| for i, request := range requests { | ||
| if len(request) <= 1 { | ||
| return nil, fmt.Errorf("execution request %d has no request data", i) | ||
| } |
yperbasis
left a comment
There was a problem hiding this comment.
Reviewed head 7d5541ba92 against consensus-specs v1.7.0-alpha.11 (beacon-chain/fork/p2p/validator), plus local build, affected test packages, and lint — no spec deviations found. Approving; findings below are non-blocking.
Findings
PostEthV1BeaconExecutionPayloadBidstill publishes the bid to gossip whenProcessMessagereturnsErrBidQueued, i.e. before signature/builder/randao validation; peers will REJECT an invalid bid and down-score us. Not a regression (the oldnilreturn published too), but since this PR introduces the queued distinction, consider deferring the publish until the pending bid validates.- Low: the final
validateHighestBid+seenCache.Add+HighestBids.AddinvalidateAndStoreBidis check-then-store with no lock spanning it — two concurrently validated bids that both beat the old max can land lower-last, and two concurrent first bids from one builder can both do full BLS. A small mutex around the store would close it. - Note:
EngineServer.beaconChainConfig()falls back chain-name → mainnet config, so an external CL on a custom-genesis devnet gets mainnet list caps in SSZ getPayload decoding. Only observable on minimal-preset networks; fine to leave. - Note: until #22093 lands, any builder-deposit-contract traffic on devnet-6 splits erigon's EL from other clients (
requests_hashover types 0x03/0x04) — the green kurtosis job just means assertoor doesn't exercise it. Relatedly,KnownRequestTypesnow includes 0x03/0x04 unconditionally (currently unreferenced); when #22093 wires it into newPayload validation, the pre-Gloas fork-gating has to happen there.
Nits
GetEthV1BeaconPoolPayloadAttestationsreturns 200-with-empty-list whenViewHeadState/aggregation fails, masking "not synced" as "no attestations"; the four identical returns could also collapse to one exit point.requestEnvelopesByRangederivescountfrom the full block span before chunking, so widely spaced blocks sweep empty slot ranges — bounded, and it's a fallback path.maxSignedExecutionPayloadBidSSZSize()is recomputed per request; a package-level var would do.
Re the Copilot comment on DecodeExecutionRequestsList rejecting 1-byte entries (#22091 (comment)): false positive. Both the engine API ("has a length of 1-byte or shorter … MUST return -32602"; "Elements MUST be longer than 1-byte") and consensus-specs get_execution_requests (assert len(request_data) != 0, kept in the gloas alpha11 version) mandate the rejection, and TestDecodeExecutionRequestsListRejectsInvalidShape already pins it.
|
Addressed the latest adversarial-review findings in |
…e_36 Picks up blk_rc_36 now merged to main (#22246) plus Gloas CL (#22091). The three db/snapshotsync conflicts were purely the #22343 rename (this branch renamed snapshotsync.RoSnapshots -> BaseRoSnapshots; main's finalized blk_rc_36 kept the old name): took main's canonical reclamation code (which already includes the Close TOCTOU fix) and re-applied the rename in snapshots.go, merger.go and snapshots_race_test.go.
Summary
Catches Caplin/Gloas up with the
consensus-specsv1.7.0-alpha.11execution-request and builder-request shape, while keeping the broader EL syscall/contract/devnet work in #22093.withdrawable_epochfor exited builders, matching the current repository spectest fixtures.get_shuffling_dependent_rootsemantics for execution payload bid proposer-preference matching, avoids redundant full-state reads before bid deduplication, and keeps pending bids queued while parent state is temporarily unavailable.PTC_SIZE, PTC duties POST no longer has a synthetic item cap beyond the existing body bound, and queued execution payload bids return gossipIGNOREwhile waiting for dependencies/preferences.ExecutionRequestsSSZ/JSON/hash shape while using the Gloas request shape at Gloas-specific boundaries, including JSONnulllist handling and zero-valueExecutionRequestsfallback behavior.ExecutionRequests.Clone()contents, including builder request lists, to avoid mutable-list aliasing.Refs #22008.
Spec note
The repository fixture source is currently pinned to
consensus-specsv1.7.0-alpha.11intest-fixtures.json. A later consensus-specs master change adds the swept-only top-up predicate for exited builders (withdrawable_epoch != FAR_FUTURE_EPOCH && balance == 0); this PR intentionally keeps the alpha11 fixture behavior somake -C cl/spectest gloasstays green.Validation
GOCACHE=/private/tmp/erigon-gloas-go-cache go test ./cl/beacon/handler -run 'TestPostPayloadAttestations|TestPostPtcDuties|TestAggregatePayloadAttestation' -count=1GOCACHE=/private/tmp/erigon-gloas-go-cache go test ./cl/phase1/network/services -run 'TestExecutionPayloadBidService' -count=1GOCACHE=/private/tmp/erigon-gloas-go-cache CGO_CFLAGS=-D__BLST_PORTABLE__ go test ./cl/transition/impl/eth2 -run 'Test.*(Attestation|Builder|Gloas|Payment)' -count=1GOCACHE=/private/tmp/erigon-gloas-go-cache GOLANGCI_LINT_CACHE=/private/tmp/golangci-gloas-cache-22091-reviewfix make lintgo test ./cl/beacon/handler ./cl/phase1/network/services -count=1was also attempted inside the sandbox;cl/phase1/network/servicespassed, whilecl/beacon/handlerhit the sandbox'shttptestport bind restriction inTestGetBlobsFromFrozenSnapshots, unrelated to this PR.Review
adversarial-code-reviewwas run with the Erigon CL / Gloas specialization and subagents until convergence.Findings addressed during the latest convergence loop:
PTC_SIZE, notMAX_PAYLOAD_ATTESTATIONS.ErrIgnore, and pending bids are retained while parent state is temporarily unavailable.Final subagent follow-up on
c9fbcf4c8areported no remaining Critical/High/Medium actionable findings for spec/regression/pre-fork compatibility or data-race/performance/error-boundary/corner-case safety.