Gloas support for Glamsterdam devnet-7 (types + transport) - #302
Draft
AntiD2ta wants to merge 37 commits into
Draft
Gloas support for Glamsterdam devnet-7 (types + transport)#302AntiD2ta wants to merge 37 commits into
AntiD2ta wants to merge 37 commits into
Conversation
Additive Gloas (Glamsterdam devnet-7) type surface ported from the ethpandaops
fork with imports rewritten to attestantio, additive-only. Target
spec consensus v1.7.0-alpha.12.
What landed:
- spec/version restructure: DataVersion/BuilderVersion moved to a leaf
spec/version package with an alias shim in package spec. Added
DataVersionGloas. Heze trimmed (out of scope).
- spec/gloas package (92 source files) + SSZ regenerated with dynssz-gen
(go generate ./spec/gloas), byte-identical to the fork output.
- Gloas arms added to 10 existing Versioned* wrappers (additive only).
- 4 new ePBS versioned wrappers: VersionedExecutionPayloadEnvelope,
VersionedSignedExecutionPayloadEnvelope, VersionedSignedExecutionPayloadBid,
VersionedExecutionRequests.
- api/v1/gloas: SignedExecutionPayloadEnvelopeContents (+ codecs, SSZ regen).
Key decisions:
- Used const aliases (not the fork's var) in spec/version.go to preserve the
original const semantics; preserved spec.DataVersionFromString, which the fork
drops (kept additive).
- Kept the fork's ssz-index struct tags: they are load-bearing for dynssz-gen
(removing them changes the generated SSZ output — verified empirically), even
though attestantio's other forks use positional SSZ.
- Excluded the fork's breaking changes: no BaseFeePerGas ->
BaseFeePerGasLE rename (bellatrix/capella arms untouched); preserved
VersionedSignedBeaconBlock.ExecutionRequests() returning *electra.ExecutionRequests
(gloas arm returns an explicit error). Heze omitted entirely.
- Lint: added //nolint:gocyclo to the version-switch methods that exceeded the
complexity threshold once the gloas case was added (matches the repo's
existing convention). Added a scoped .golangci.yml exclusion for line-length
(lll + revive) on the gloas type packages: gloas ePBS types carry long dynssz
size-expression tags that gofmt aligns past 132 chars and that cannot be
wrapped, and the ssz-index tags cannot be dropped.
Files changed:
- new: spec/gloas/ (92 files), spec/version/, spec/version.go, api/v1/gloas/,
spec/versioned{executionpayloadenvelope,executionrequests,
signedexecutionpayloadbid,signedexecutionpayloadenvelope}.go
- modified: 10 spec/versioned*.go wrappers, .golangci.yml
- deleted: spec/dataversion.go, spec/builderversion.go (moved to spec/version/)
Verified: go build ./...; go test ./... (2168 tests); changed packages
golangci-lint clean; SSZ regen idempotent and byte-identical to fork.
Blockers / notes for next iteration:
- Acceptance criterion "gloas ssz_static consensus-spec tests (v1.7.0-alpha.12)"
is BLOCKED: ethereum/consensus-spec-tests has not published v1.7.0-alpha.12
(latest release is v1.6.0-beta.0, predating gloas). The ported
spec/gloas/consensusspec_test.go skips gracefully until CONSENSUS_SPEC_TESTS_DIR
points at gloas ssz_static vectors.
- Acceptance criterion "devnet-7 block and state decode (demo)" is BLOCKED:
needs a Gloas-capable endpoint / devnet-7 fixtures (a Kurtosis devnet).
- Pre-existing, unrelated: spec/capella/generate.go has an unused
//nolint:revive (nolintlint) — not touched by this change.
Fixes from /review-local (all reviewers APPROVED / SPEC COMPLIANT; only non-blocking findings remained): - spec/gloas/attestation_test.go: the ported test instantiated electra.Attestation instead of gloas.Attestation (inherited verbatim from the fork) — it passed but gave zero coverage of gloas.Attestation's own CommitteeIndex/AggregateValidatorIndex/MarshalSSZ. Swapped to gloas.Attestation; all three tests now genuinely exercise the gloas type (incl. the progressive-bitlist SSZ path). Flagged by both spec-reviewer and code-quality-reviewer. - spec/versionedsignedaggregateandproof.go: lowercased the stray "no Gloas ..." error string to "no gloas ..." for consistency with sibling arms. - spec/versionedsignedexecutionpayloadbid.go: corrected the copyright header (2021-2024 carried over from versionedsignedbeaconblock.go) to 2026 for this new file. - .golangci.yml: dropped the redundant `- lll` from the gloas line-length exclusion (lll is globally disabled; only revive's line-length-limit fires). Verified: go build ./...; go test ./... (2168); changed packages golangci-lint clean. Not fixed here (captured as follow-up / out of scope): - security-reviewer MEDIUM: negative make() panic on malformed transaction JSON in spec/gloas/executionpayload_json.go (verbatim copy of the pre-existing spec/deneb pattern; a library-wide latent DoS, not a gloas regression). Should be fixed uniformly across deneb/electra/fulu/gloas in a dedicated task.
AntiD2ta
changed the title
AntiD2ta
marked this pull request as draft
July 23, 2026 13:31
Turns on the attgo-linter current-year check (enable_current_year: true) in .golangci.yml and brings every copyright header the check flags up to date. - New spec/gloas files (ported from the fork) and spec/version.go: header year set to 2026 (new-file rule). - Pre-existing spec files touched by this branch: header extended to a "<original-year> - 2026" range, preserving the original authorship year (modified-file rule). - .gitignore: ignore the custom-gcl binary produced by "golangci-lint custom" (the .custom-gcl.yml build config stays tracked). The current-year check is git-aware and only runs through the custom-gcl binary (it is a plugin module); the standard golangci-lint binary does not include it. These are header-only edits — no code or generated SSZ output changed. Files changed: .golangci.yml, .gitignore, and 66 copyright headers under spec/ and spec/gloas/. Verified: golangci-lint custom && ./custom-gcl run -> 0 issues; go build ./... clean; full test suite (2168 tests) green.
…ity)
Additive port of the Gloas (Glamsterdam devnet-7) ePBS transport surface from
the ethpandaops fork, imports rewritten to attestantio, additive only.
What landed:
- 3 ePBS HTTP endpoints in http/: SignedExecutionPayloadEnvelope (GET),
SubmitExecutionPayloadBid (POST), SubmitExecutionPayloadEnvelope (POST,
stateless Contents form) + their api/ opts types.
- 3 provider interfaces in service.go: ExecutionPayloadProvider,
ExecutionPayloadEnvelopeSubmitter, ExecutionPayloadBidSubmitter.
- 7 new SSE event topics with typed handlers in http/events.go
(execution_payload, execution_payload_available, execution_payload_bid,
execution_payload_gossip, fast_confirmation, payload_attestation_message,
proposer_preferences); handler fields/types in api/eventsopts.go; 3 new
api/v1 event payload types (ExecutionPayloadEvent,
ExecutionPayloadAvailableEvent, FastConfirmationEvent) + the
unmarshalVersionedEventData helper (accepts both {version,data}-wrapped and
bare payloads).
- multi/mock/testclients parity for the 3 endpoints.
- New HTTP tests for the 3 ePBS endpoints (success + error paths), 3 api/v1
event round-trip tests, and TestEventHandler extended with the 7 new topics
(wrapped + bare).
- http/main_test.go: shared test service now built with
WithCustomSpecSupport(true) (the interim devnet runs the minimal preset).
Key decisions:
- Dropped the fork's spec/all "Agnostic*" endpoint/opts/interface variants
(spec/all not adopted) and all Heze arms (Heze out of scope); the
non-agnostic variants use the versioned wrappers ported earlier.
- Added a local dynSSZForRequest helper mirroring the repo's existing inline
dynssz pattern (GetGlobalDynSsz for mainnet, NewDynSsz(spec) for custom
presets) rather than the fork's Service-cached version, which depends on
fields this tree lacks.
- Added ePBS methods to testclients erroring/sleepy for provider parity even
though the fork ships none (networkutil excluded - no provider methods).
- Two wire details intentionally DIVERGE from the (stale) fork to match the
live beacon-APIs spec (master), verified against the upstream spec:
* submit-envelope header is Eth-Blob-Data-Included:true (fork used the
removed Eth-Execution-Payload-Blinded:false).
* FastConfirmationEvent carries current_slot (fork omitted it), parsed
tolerantly.
Verified:
- go build ./...; gosilent test ./... (2201); golangci-lint via custom-gcl
(attgo current-year plugin) 0 issues; go generate ./spec/gloas
byte-identical.
- Against a live Gloas devnet (Lighthouse, minimal preset): the 3 ePBS
endpoint tests pass (incl. a real envelope fetch at head and both submit
ReachesServer cases) and TestEventHandler passes for all 7 new topics.
Files changed:
- new: http/{signedexecutionpayloadenvelope,submitexecutionpayloadbid,
submitexecutionpayloadenvelope}.go (+ _test.go); api/{signed...,submit...}
opts.go (3); api/v1/{executionpayloadevent,executionpayloadavailableevent,
fastconfirmationevent}.go (+ _test.go); multi/*.go (3); mock/*.go (3).
- modified: service.go, api/eventsopts.go, http/events.go,
http/events_internal_test.go, http/main_test.go, mock/service.go,
testclients/{erroring,sleepy}.go.
Blockers / notes for next iteration:
- Existing versioned HTTP endpoints (beacon state, and by extension the
validators endpoint, blocks, proposals) lack a gloas arm in their version
switch, so they fail against a gloas endpoint with "unhandled state version
gloas" (http/beaconstate.go). Out of scope here (transport only); needs a
dedicated "gloas-enable existing endpoints" slice.
- spec/gloas/executionpayload_json.go MarshalJSON panics on a nil
BaseFeePerGas (unguarded .Dec()); the SSZ marshaler already guards it. JSON
path only; latent. Same robustness family as the deneb-inherited make() DoS.
AntiD2ta
changed the title
AntiD2ta
marked this pull request as ready for review
July 23, 2026 17:40
Two latent, JSON-path-only panics in the hand-written
spec/gloas/executionpayload_json.go (the SSZ path is unaffected):
- MarshalJSON dereferenced a nil BaseFeePerGas (*uint256.Int) through an
unguarded .Dec(); a zero-value or partially-populated ExecutionPayload
(e.g. the shape mock/signedexecutionpayloadenvelope.go builds) panicked.
Now guarded with `if baseFeePerGas == nil { baseFeePerGas = new(uint256.Int) }`,
mirroring the nil guard the generated SSZ marshaler already applies.
- UnmarshalJSON computed make([]byte, (len(elem)-4)/2) per transaction
element; a 1- or 2-byte raw JSON element slipped past the empty-element
guard and underflowed to make([]byte, -1) (makeslice panic — a remote
DoS from malformed beacon-node JSON). The guard is now `len(elem) < 4`,
a strict superset of the prior len==0 and "" checks (no previously-valid
encoding is newly rejected), keeping the explicit "0x" empty check.
Key decisions:
- Kept the marshal fix as an explicit *uint256.Int nil-guard (not a "0"
string default) to mirror the SSZ sibling one-for-one, per the fix intent.
- Widened the unmarshal guard's error from "missing" to "missing or
malformed": the len<4 branch now also catches present-but-truncated
tokens, not only absent ones.
- Scope is the gloas copy only. The identical unguarded pattern in the
spec/deneb (and other-fork) siblings is deliberately left untouched — a
library-wide sweep is tracked as separate follow-up work.
Test: new spec/gloas/executionpayload_test.go (package gloas_test,
table-driven, testify/require) marshals a zero-value payload (nil
BaseFeePerGas) and unmarshals 1- and 2-byte raw transaction elements; both
reproduced as RED panics before the guards, GREEN after.
Files changed:
- modified: spec/gloas/executionpayload_json.go
- new: spec/gloas/executionpayload_test.go
Verified: go build ./...; gosilent test ./... (2204); go generate
./spec/gloas byte-identical (no SSZ drift); custom-gcl (attgo current-year
plugin) 0 issues; -race clean on the new tests.
Notes for next iteration:
- A separate, pre-existing bug was found in the same UnmarshalJSON loop and
captured as follow-up work: the MaxTransactionsPerPayload /
MaxBytesPerTransaction bounds checks return errors.Wrap(err, ...) where
err is nil, so pkg/errors.Wrap(nil, ...) returns nil and the checks fail
open (oversized input silently accepted). Fail-open, not a panic/DoS;
inherited from spec/deneb; needs a library-wide fix.
…ggregate, attestation submit) Additive Gloas (Glamsterdam devnet-7) enablement for the mechanical half of the existing versioned HTTP endpoints: add a spec.DataVersionGloas arm to each fork-version switch, decoding into / encoding from the spec/gloas types already on the branch. Every arm is a structural mirror of its electra/fulu sibling; no existing behaviour changes. What landed: - http/beaconstate.go: gloas arms in beaconStateFromSSZ and beaconStateFromJSON (decode into gloas.BeaconState). Fixes "unhandled state version gloas" on the BeaconState endpoint and its cascade into the Validators endpoint (which decodes the beacon state). - http/aggregateattestation.go: gloas arm in decodeAggregateAttestation. A Gloas node reports Eth-Consensus-Version: gloas for the aggregate endpoint, which previously fell through to "unknown consensus version". Decodes into gloas.Attestation. - http/submitattestations.go: gloas arm in createUnversionedAttestations (attestations[i].Gloas.ToSingleAttestation), for callers submitting gloas-versioned attestations. - http/submitaggregateattestations.go: gloas arm in createUnversionedAggregates (appends the electra-typed .Gloas field). - 3 internal unit tests (package http): the gloas aggregate-decode arm, and the two submit-encode helpers (createUnversionedAttestations / createUnversionedAggregates had no prior unit test at all). Key decisions: - attestationpool.go is deliberately NOT given a gloas arm. verifyAttestationPool switches on datum.Version, but spec/versionedattestation_json.go's UnmarshalJSON identifies the attestation variant only by the presence of committee_bits and always labels it Electra, never Gloas. Gloas attestations are wire-identical to Electra, so they already decode and verify correctly as Electra; a gloas arm there is unreachable dead code and would need a duplicate verifyGloasAttestation helper (datum.Gloas is *gloas.Attestation, not *electra.Attestation). - Scope is the mechanical endpoints only. The ePBS block/proposal endpoints (signedbeaconblock, proposal, blindedproposal, submitproposal, submitblindedproposal, submitbeaconblock, submitblindedbeaconblock) are NOT mechanical: under ePBS the block carries a SignedExecutionPayloadBid rather than an inline payload, api/v1/gloas ships no proposal types yet, and "blinded proposal" needs a design decision. They are deferred as separate follow-up work. - Copyright headers on the 4 touched source files bumped to a 2020 - 2026 range; the modified test file to 2025 - 2026 (attgo current-year lint gate). New test files carry 2026-only headers. Verification: - go build ./...; gosilent test ./... (2204); custom-gcl run ./http/... 0 issues; go generate ./spec/gloas byte-identical (no SSZ drift). - Against a live Gloas devnet (Lighthouse, minimal preset, head block version gloas, service built with WithCustomSpecSupport(true)): TestBeaconState/Head (SSZ) and TestValidators (Head/Finalized/Justified/ExitedValidators) go green, the 3 new internal tests pass, and the pre-existing ePBS execution-payload envelope/bid endpoint tests and SSE event-handler tests stay green. Files changed: - modified: http/beaconstate.go, http/aggregateattestation.go, http/submitattestations.go, http/submitaggregateattestations.go, http/aggregateattestation_internal_test.go - new: http/submitattestations_internal_test.go, http/submitaggregateattestations_internal_test.go Blockers / notes for next iteration: - gloas.BeaconState JSON codec bug: execution_payload_availability (a Bitvector[SLOTS_PER_HISTORICAL_ROOT]) is marshalled as a decimal-string array and unmarshalled as base64, instead of a single 0x hex string the way justification_bits is handled in the same file. The JSON beaconstate arm added here is correct but delegates to this buggy hand-written codec, so TestBeaconState/Head_(json) stays red against a Gloas node. Fixing one field may expose others (e.g. PTCWindow uses the same shape) — needs a field-by-field JSON audit of gloas.BeaconState. The SSZ path (the endpoint's preferred encoding) is unaffected and green. Tracked as separate follow-up work. - TestValidators/ManyValidatorIndices fails against the devnet only because it expects 10000 validators (mainnet scale) and the devnet has 256; unrelated to gloas or this change. - The ePBS block/proposal endpoint arms remain to be done (separate follow-up).
…lers
The hand-written spec/gloas JSON unmarshalers hex-decoded a value and copied
it straight into a fixed-size array with no length check. hex.DecodeString
succeeds for any even-length input, so a short value silently zero-padded the
array's tail and a long one was silently truncated, while UnmarshalJSON still
returned nil. The caller received a corrupted root, hash, address or signature
with no error surfaced -- fail-open silent data corruption reachable from
malformed or malicious beacon-node JSON, and worse than a panic because
fork-choice and signature-domain logic keyed on the value then produces a wrong
answer rather than crashing.
20 unguarded sites across 13 files now reject a wrong-length value before the
copy, each with a descriptive error naming the field, matching the guard idiom
already used by the package's other unmarshalers (attestation.go,
builderdepositrequest_json.go, aggregateandproof.go) and by the sibling fork
packages:
if len(x) != phase0.RootLength {
return errors.New("incorrect length for <field>")
}
Sites, by file (length constant chosen from the field's real Go type):
- beaconblockbody_json.go: RANDAOReveal (SignatureLength), Graffiti
(GraffitiLength)
- beaconstate_json.go: LatestBlockHash (Hash32Length)
- executionpayloadbid_json.go: ParentBlockHash, BlockHash (Hash32Length);
ParentBlockRoot, PrevRandao, ExecutionRequestsRoot (RootLength); FeeRecipient
(bellatrix.FeeRecipientLength)
- executionpayloadenvelope_json.go: BeaconBlockRoot, ParentBeaconBlockRoot
(RootLength)
- payloadattestationdata_json.go: BeaconBlockRoot (RootLength)
- proposerpreferences_json.go: FeeRecipient (bellatrix.FeeRecipientLength)
- signature fields (SignatureLength): signedbeaconblock_json.go,
signedexecutionpayloadbid_json.go, signedexecutionpayloadenvelope_json.go,
signedproposerpreferences_json.go, payloadattestation_json.go,
payloadattestationmessage_json.go, indexedpayloadattestation_json.go
Key decisions:
- phase0.RootLength vs phase0.Hash32Length are both 32, but the constant
records the field's semantic type (Merkle root vs execution block hash) so a
future type change stays type-correct rather than accidentally correct.
- The YAML layer needed no change: every spec/gloas *_yaml.go UnmarshalYAML
routes through UnmarshalJSON, except builderdepositrequest_yaml.go which
calls the already-guarded shared unpack(). No hex-decode-into-array code
exists in the YAML layer, so the JSON fix covers YAML transitively. Verified
by reading all 18 YAML unmarshalers, not assumed.
- The duplication across the 20 guards is deliberate. A decodeFixedBytes helper
already exists in api/v1/executionpayloadevent.go and consolidating onto it
is tracked as separate follow-up work; introducing a cross-package
abstraction in the same change as a data-corruption fix would make the fix
harder to review and to backport. This change stays mechanical and uniform.
- Adding six guards pushed ExecutionPayloadBid.UnmarshalJSON to gocyclo
complexity 34 against the default threshold of 30. .golangci.yml carries no
gocyclo settings block, but the repo's actual answer to this is the inline
//nolint:gocyclo directive -- 33 of them, two already in this package
(beaconstate_json.go, executionpayload_json.go) and one on the closest
structural sibling, spec/deneb/executionpayloadheader_json.go's
UnmarshalJSON. This change follows that convention rather than restructuring
the function, which keeps the patch purely additive: 70 insertions, 0
deletions, every added line a guard, an import, or the directive. No existing
line moves, so the diff a reviewer must audit contains only new checks.
- spec/gloas/builder.go was left alone: its execution-address length check
exists and fails closed, but names the wrong field ("fee recipient") and has
an unreachable duplicate after the copy. Different bug class, captured as
separate follow-up work rather than folded in here.
Test: new spec/gloas/jsonlength_test.go (package gloas_test, table-driven,
testify/require). TestJSONFixedLengthGuards has one case per newly-guarded
site (23 cases), with both too-short and too-long variants for a representative
root, address and signature. Every case was confirmed RED before its guard
landed -- and revealingly so: without the guard the unmarshaler does not merely
return nil, it sails past the corrupt field entirely and errors later on an
unrelated one. TestJSONFixedLengthRoundTrip is the counterpart, asserting nine
types still marshal/unmarshal byte-identically so the guards cannot over-reject;
it was mutation-checked (an off-by-one length constant makes it fail), so it is
not vacuous.
Files changed:
- modified: spec/gloas/{beaconblockbody,beaconstate,executionpayloadbid,
executionpayloadenvelope,indexedpayloadattestation,payloadattestation,
payloadattestationdata,payloadattestationmessage,proposerpreferences,
signedbeaconblock,signedexecutionpayloadbid,signedexecutionpayloadenvelope,
signedproposerpreferences}_json.go
- new: spec/gloas/jsonlength_test.go
Verified: go build ./... clean; gosilent test ./... (2236) green; -race clean on
spec/gloas; custom-gcl (attgo current-year plugin) 0 issues repo-wide; go
generate ./spec/gloas byte-identical (no SSZ drift). A scripted sweep confirms
all 30 copy()-into-fixed-array sites in the hand-written spec/gloas sources now
have a preceding length check, 0 unguarded. An independent reviewer additionally
mutation-tested the guards -- deleting any one of them fails exactly and only
its own subtest -- and the round-trip test was mutation-checked the same way (an
off-by-one length constant makes it fail), so neither test is vacuous.
Blockers / notes for next iteration:
- The consensus-spec ssz_static vectors gate could NOT be run, and not for a
local reason: ethereum/consensus-spec-tests has no v1.7.0-alpha.12 release
published at all. The newest release is v1.6.0-beta.0 (2025-09-24), which
predates gloas and carries no tests/mainnet/gloas directory, so the target
vectors are unobtainable rather than merely absent from this machine. Any
plan step that gates on those vectors is blocked upstream until the spec
release ships them. This change touches only the JSON codec, and go generate
confirms no SSZ drift, so the gate has no bearing on it either way.
- The same unguarded copy()-into-fixed-array pattern very likely persists in
the other fork packages this code was modelled on; only spec/gloas was swept
here. A library-wide audit is worth scoping.
- A security review of this change confirmed two residual fail-open paths in
spec/gloas that are outside this bug class and were deliberately left alone,
both already captured as separate work:
* executionpayload_json.go's transaction bounds checks return
errors.Wrap(err, ...) where err is provably nil, so UnmarshalJSON returns
success. Reproduced: a ~3 MB body with 1,048,577 empty transaction entries
parses "successfully" with a zeroed tail (Transactions, Withdrawals,
BlobGasUsed, ExcessBlobGas, BlockAccessList, SlotNumber), contradicting the
values the document declared. Network-reachable via
http/signedexecutionpayloadenvelope.go.
* fixed-width bitvectors (PayloadAttestation.AggregationBits,
BeaconState.JustificationBits, Attestation.CommitteeBits) are hex-decoded
into slice-typed fields with no length check, so a wrong-length value is
stored as-is and UnmarshalJSON returns nil. NOT mechanically fixable: two
of the three carry dynssz-size tags (PTC_SIZE/8, MAX_COMMITTEES_PER_SLOT/8)
whose byte width is resolved at runtime from the node's config, so a
hardcoded guard would reject valid minimal-preset data -- the preset this
branch is validated against. Only JustificationBits is preset-independent.
- The premise that these guards should be consolidated onto the existing
decodeFixedBytes helper needs revisiting before anyone acts on it: that
helper is unexported in package api/v1 with two callers, both in its own
defining file, and emits "incorrect length %d for %s" against the house form
"incorrect length for <field>" (138 uses under spec/ vs 58). A gloas-only
adoption would add a third dialect rather than remove one; if wanted, it
should be a single repo-wide sweep preserving the house string.
…aler The hand-written spec/gloas/executionpayload_yaml.go MarshalYAML called e.BaseFeePerGas.Dec() unconditionally, where BaseFeePerGas is a nilable *uint256.Int. (*uint256.Int).Dec() calls IsZero(), which indexes z[0] immediately, so a nil receiver panics rather than returning a zero string. An earlier change fixed exactly this on the JSON path and the generated SSZ marshaler has always guarded it; only the hand-written YAML marshaler was missed, leaving the three encodings of one type disagreeing about whether a zero-value payload is marshalable. YAML is not a niche path here: String() marshals through it, so the panic is reachable from ExecutionPayload.String(), and transitively from ExecutionPayloadEnvelope.String() and spec.VersionedSignedExecutionPayloadEnvelope.String() -- the envelope YAML marshaler passes the payload to the encoder, which dispatches into the payload's own MarshalYAML. Merely logging a fetched or mocked envelope crashed the caller, and mock/signedexecutionpayloadenvelope.go builds precisely the crashing shape (a payload with only Transactions and Withdrawals set). The fix is the nil guard its JSON sibling already carries, applied verbatim. Key decisions: - The guard is a byte-for-byte mirror of the JSON sibling's: same comment text, same baseFeePerGas variable, same placement immediately after the extraData/blockAccessList prep. Shorter forms were considered and rejected. Mutating the receiver (e.BaseFeePerGas = new(uint256.Int)) saves two lines but has a marshaler write to its receiver, which races the moment one payload is logged and served concurrently. Substituting a literal "0" string avoids the allocation but breaks the symmetry this change exists to restore. cmp.Or is one line and stdlib, but appears nowhere in this repo and would itself become a new kind of drift between the two marshalers. - Substituting a zero value rather than erroring is deliberate, and matches what the JSON and generated SSZ marshalers of this type already do; making YAML alone fail closed would leave the diagnostic path stricter than the hashing and signing paths. Nothing is masked at a trust boundary either: both decoders guarantee a non-nil BaseFeePerGas -- codecs.RawJSON rejects a payload missing the required base_fee_per_gas key, and the SSZ decoder allocates unconditionally -- so a nil value can only originate from in-process construction, never from remote input. - The guard costs nothing measurable. A benchmark of the real MarshalYAML shows the nil and non-nil branches at an identical 486 allocs/op: the compiler proves new(uint256.Int) does not escape, since Dec() does not retain its receiver, so it is stack-allocated. - Scope stayed gloas-only. The identical unguarded pattern in spec/deneb is confirmed and captured as separate follow-up work rather than folded in (see below); widening a one-line fix into a cross-fork sweep would make both harder to review. - Criterion coverage for "the JSON and YAML pair must not drift" is met by two adjacent mirrored tests rather than one unified table, so the existing JSON test is left untouched and the diff stays additive. Sweep (the reason no other file changed): all 18 hand-written spec/gloas *_yaml.go MarshalYAML functions were audited for nilable-pointer dereferences, field types resolved via go doc rather than inferred. BaseFeePerGas is the only pointer field any of them dereferences. Every other deref is a value receiver on a fixed-size byte array (phase0.Root, phase0.Hash32, bellatrix.ExecutionAddress) or on a slice type (bitfield.Bitvector4), neither of which can be nil. Nilable pointers that are merely assigned into a YAML target struct are not a panic: goccy/go-yaml checks isInvalidValue before canEncodeByMarshaler, so a nil pointer is emitted as null and its MarshalYAML is never invoked. That last point was verified by reading the encoder, not assumed, and it is also what makes the transitive String() chain safe at every hop except the innermost one fixed here. Test: new TestExecutionPayloadMarshalYAML in spec/gloas/executionpayload_test.go (the direct sibling of the existing JSON test), plus spec/gloas/executionpayloadenvelope_test.go and spec/versionedsignedexecutionpayloadenvelope_test.go for the two transitive String() callers. All three were confirmed RED before the guard landed -- reverted to the pre-fix marshaler, each failed with a nil-pointer deref raised through the goccy encoder, then passed once restored -- so none is vacuous. The versioned wrapper's ZeroValue case pins a non-obvious fact worth recording: version.DataVersion's zero value is DataVersionUnknown = iota, not DataVersionPhase0, so a zero-value wrapper takes String()'s default arm and renders "unknown version" rather than the empty string that the switch's first case label suggests. Files changed: - modified: spec/gloas/executionpayload_yaml.go (nil guard + uint256 import; 9 insertions, 1 deletion, no existing line moved) - modified: spec/gloas/executionpayload_test.go (TestExecutionPayloadMarshalYAML) - new: spec/gloas/executionpayloadenvelope_test.go - new: spec/versionedsignedexecutionpayloadenvelope_test.go Verified: go build ./... clean; gosilent test ./... (2241) green; -race clean on the new tests; custom-gcl (attgo current-year plugin) 0 issues repo-wide; go generate ./spec/gloas byte-identical, no SSZ drift; go.mod and go.sum untouched (holiman/uint256 was already a direct dependency of this package). Five independent reviewers (spec, security, efficiency, code quality, simplicity) each approved, three of them re-running the mutation check themselves. Blockers / notes for next iteration: - The same unguarded .Dec() exists at four sites in spec/deneb and is now tracked as separate follow-up work: executionpayload and executionpayloadheader, JSON and YAML each. Their generated SSZ marshalers guard it (3 sites apiece) while all four hand-written marshalers do not -- the identical asymmetry fixed here, but in a shipped production fork rather than a devnet branch, so it is not gated on Glamsterdam and may warrant landing on master directly. A scripted sweep of every *_json.go and *_yaml.go under spec/ containing .Dec() found exactly these four unguarded sites and no others. - The broader pattern is worth a slice of its own: hand-written codecs systematically miss nil guards that their generated SSZ siblings have. This is the second instance found in as many sessions. - Two pre-existing inefficiencies in the touched function were measured and deliberately left alone: the bytes.ReplaceAll post-pass costs a full extra buffer copy on every call, and the per-transaction fmt.Sprintf is ~2 allocs/element. At 2000 transactions the whole call is ~10ms/17.5MB, so neither dominates, and the obvious hex.EncodeToString rewrite measured ~17% faster but ~3x the memory. Not worth changing while this path is diagnostic-only, and any real fix belongs in the JSON marshaler too, which shares the pattern. - The consensus-spec ssz_static vectors gate still cannot run: the newest ethereum/consensus-spec-tests release remains v1.6.0-beta.0 (2025-09-24), which predates gloas and ships no tests/mainnet/gloas directory, so the v1.7.0-alpha.12 vectors are unobtainable upstream rather than merely absent locally. This change touches only the YAML codec and go generate confirms no SSZ drift, so the gate has no bearing on it either way.
The Gloas ePBS transport port added typed handlers, api.EventsOpts fields and
handler-func types for seven new SSE topics, and dispatch arms for all seven in
handleEvent and checkEventSpecificHandler -- but never added the topic strings
to apiv1.SupportedEventTopics, the allow-list checkEventsOpts gates on.
Events() runs checkEventsOpts before it builds the subscription, so every one of
those topics was refused with "unsupported event topic <name>" and never reached
the wire. The whole surface -- 7 handlers, 7 opts fields, 7 handler-func types,
14 dispatch arms -- was unreachable dead code.
The seven registered here: execution_payload, execution_payload_available,
execution_payload_bid, execution_payload_gossip, fast_confirmation,
payload_attestation_message, proposer_preferences. Entries keep the map's
alphabetical order, which now matches both switches in http/events.go
one-for-one: allow-list, checkEventSpecificHandler and handleEvent are all 22
topics with empty set difference in both directions.
Why CI stayed green, which is the more useful part: the existing test calls
handleEvent directly, downstream of the gate, so it could not see the defect.
But it is weaker than that -- http/main_test.go's TestMain returns without
calling m.Run() unless HTTP_ADDRESS is set, and internal (package http) and
external (package http_test) test files share one TestMain, so `go test ./http/`
reports ok having executed zero tests. No test in http/ can fail a build in
ordinary CI, whatever it asserts. That is why the unconditional guard for this
regression class lives in api/v1, not next to the code it guards.
Key decisions:
- Coverage is split deliberately across two packages. api/v1's
TestSupportedEventTopicsGloas asserts allow-list membership and is the only
new test that runs without a configured endpoint. http's TestEventsGloasTopics
exercises the real public Events() and runs against a live Gloas devnet.
- The gate test asserts on the "no handler for <topic> event" error rather than
only on success. checkEventsOpts emits "unsupported event topic X" from the
allow-list check and "no handler for X event" from the handler check
immediately after, so the second message is positive proof the topic cleared
the allow-list -- and needs no network I/O or background subscription.
Asserting only "no error" would be weaker, since Events() returns nil for
everything once past the gate.
- The routing test uses a zero-value &Service{}: handleEvent only dispatches,
and every handleXEvent method has an unnamed receiver and takes its logger
from the context, so none reads service state. Verified by reading all seven.
That keeps a pure dispatch check free of any node dependency.
- Event.UnmarshalJSON's separate topic switch was deliberately left at its
original 15 topics and captured as follow-up work. Nothing regressed: before
this change a Gloas topic was refused at both the Events() gate and there, so
it is a second independent gap, not damage. The client's own event path never
reaches it -- handleEvent builds &apiv1.Event{...} directly. Fixing it well
also means confronting that the switch's typed result is discarded on the next
line, which is a behavioural change to a shipped API and needs its own call.
- A three-line comment on SupportedEventTopics records that it is the allow-list
Events() validates against and is maintained separately from that switch, so
the drift hazard is discoverable without hand-diffing two lists.
Test: new TestEventsGloasTopics (http/events_test.go, 10 subtests through the
public Events()), TestSupportedEventTopicsGloas (api/v1/event_test.go, 7
subtests), TestEventHandlerGloasRouting (http/events_internal_test.go, 7
subtests asserting the topic-specific handler wins over the generic one).
None is vacuous, and each was checked differently. The gate test was RED before
the fix: 15 of 16 subtests then failed with "unsupported event topic <name>"
while its UnknownTopic control passed. The api/v1 guard was mutation-checked --
deleting the seven entries fails exactly its seven subtests. The routing test
covers pre-existing behaviour so it passed immediately; it was mutation-checked
instead, by disabling one topic's specific-handler branch, which failed exactly
that topic's subtest while its generic fallback still passed.
Files changed:
- modified: api/v1/event.go (7 map entries, comment, copyright)
- modified: api/v1/event_test.go (TestSupportedEventTopicsGloas)
- modified: http/events_test.go (TestEventsGloasTopics)
- modified: http/events_internal_test.go (TestEventHandlerGloasRouting)
Verified: go build ./... clean; gosilent test ./... 2248 green, and -race clean;
custom-gcl (attgo) 0 issues repo-wide; go vet and gofmt clean on all four files
(note .golangci.yml sets run.tests: false, so lint never inspects the test
files -- go vet is what covers them). Against a live Gloas devnet (Lighthouse,
minimal preset, head block version gloas) all three new tests pass, also under
-race. A baseline run with these four files stashed produced a byte-identical
set of 41 pre-existing http failures, so there are no regressions: those are the
syncing devnet's "client is not synced" plus known devnet-scale and codec
issues. Passing count went 140/199 to 170/229. Five independent reviewers ran;
after one fix round all five approve.
Blockers / notes for next iteration:
- No http/ test can gate CI, per the TestMain analysis above. Until that is
addressed, any http-only regression guard is invisible to CI, and this class
of defect can ship green again. Tracked as follow-up work.
- Event.UnmarshalJSON still rejects all seven topics; confirmed by probe, so a
generic apiv1.Event JSON round-trip fails for Gloas topics. Follow-up.
- A severe pre-existing bug was found and reproduced in multi/events.go while
chasing a reviewer's aside: for active clients, activeHandler.opts is the
substituted options struct, so each wrapper method forwards into itself
(h.opts.Handler == h.genericHandler). It self-recurses to a fatal,
unrecoverable stack overflow on the first event from the primary active
client, and the caller's handlers are dropped entirely since only Common is
copied. Reproduced with a temporary internal probe, since activeHandler is
unexported; the probe was deleted and multi/ is untouched here. Unrelated to
Gloas -- multi/events.go predates it -- but it means multi.Events() cannot
work today. Tracked as follow-up, and worth prioritising.
- Two further pre-existing issues captured as follow-up: ExecutionPayloadEvent
defaults execution_optimistic to false for execution_payload_gossip, whose
spec schema has no such field, so the two topics decode to byte-identical
structs and a merely gossip-validated payload can read as fully verified; and
log forging via unvalidated RawJSON of attacker-controlled SSE payloads at 22
sites in http/events.go, where a hostile node can emit a second "level" key
and downgrade a logged error to info.
- The consensus-spec ssz_static vectors gate does not apply: nothing under
spec/ was touched, so there is no SSZ drift to check. Those v1.7.0-alpha.12
vectors remain unobtainable upstream in any case.
- One unidentified transient test failure appeared in a single CI-mode run and
did not reproduce in three further runs, one of which reported all 19 packages
passing individually. The compressed output did not name it. Not in a package
this change can affect (api/v1 passed 549/549 in that detail run), but worth
watching for a flake.
Both spec.VersionedAggregateAndProof.Gloas and
spec.VersionedSignedAggregateAndProof.Gloas were declared
*electra.{,Signed}AggregateAndProof. Under Gloas, Attestation is a
ProgressiveContainer whose aggregation_bits are a ProgressiveBitlist
(consensus-spec v1.7.0-alpha.12, specs/gloas/beacon-chain.md); electra's are a
regular Bitlist. A progressive bitlist merkleizes into a right-leaning chain of
subtrees whose depth follows the actual data length, while a regular bitlist
pads to a fixed depth derived from the type limit, so the two produce different
hash tree roots for identical bytes. Measured on one logical aggregate:
gloas 0xc565330d..., electra 0xbcf25de3....
That root is the BLS signing root. VersionedAggregateAndProof.HashTreeRoot() is
what a validator signs before wrapping the signature for submission, so with the
electra container reached from the Gloas arm the validator signed the wrong
preimage and the beacon node rejected the aggregate as invalid-signature, with
nothing in the payload to indicate why. The gloas containers ported earlier were
meanwhile referenced nowhere outside spec/gloas.
The fix is one retyped field per wrapper. Every DataVersionGloas accessor arm
already compiles unchanged against the gloas containers, since the field names
along Message.Aggregate.Data match electra's one for one.
Key decisions:
- Both wrappers are fixed, not just the signed one. The signed wrapper alone
would have made the submit path type-correct while HashTreeRoot() on the
unsigned wrapper still returned the electra root, leaving the
invalid-signature symptom exactly as it was. They are one defect: the
validator flow is build unsigned, hash, sign, wrap, submit. The unsigned
wrapper has no callers anywhere in this library, which is why the mistyping
went unnoticed; it is public API for downstream signers.
- Additive, and safe to retype: both Gloas arms are new on this branch and
unreleased, so no downstream consumer exists. The one in-repo consumer,
createUnversionedAggregates, appends the arm into a []any for JSON marshalling
and needed no change. A welcome side effect is that assigning a populated
electra value into a Gloas arm is now a compile error rather than a silently
wrong signature.
- The retyped fields carry a three-line comment answering the question a reader
will ask, namely why Gloas diverges when Fulu does not. Its absence is what
let a mechanical copy of the Fulu line become a signing bug.
- The JSON wire shape is unaffected: gloas and electra aggregate marshallers are
structurally identical, and the progressive/regular distinction is purely a
merkleization concept. Only the root changes.
- No arm was added to attestationpool.go or any decode path. Gloas aggregates
arrive wire-identical to electra and VersionedAttestation already types its
Gloas arm as *gloas.Attestation, so the fetch-to-submit path now type-checks
end to end with no conversion.
Test: spec/versionedaggregateandproof_test.go and
spec/versionedsignedaggregateandproof_test.go, four tests covering the Gloas arm
of both wrappers: the hash tree root, the fetch-aggregate-sign-submit path, and
every accessor including its missing-arm error.
The root test asserts both that the Gloas root differs from electra's for the
same logical aggregate and that it equals a pinned value. Both are needed, and
finding out why was the useful part of this change. Mutation testing showed the
difference assertion alone is insensitive: swapping the progressive bitlist
mixin for a regular one inside the generated codec, or returning a zero root,
still leaves the root unequal to electra's, so the assertion passed while the
merkleization was broken. An inequality pins a boundary, not a value. The pin
closes that, and its comment is explicit that it guards drift in this
repository's own codec and is not independent spec verification, which needs the
consensus-spec ssz_static vectors.
Every test was mutation-checked rather than assumed: corrupting the unsigned
wrapper's HashTreeRoot arm to delegate to the inner attestation fails the root
test; an off-by-one in the AggregatorIndex arm fails the fetch-to-submit test
and exactly the AggregatorIndex subtest, leaving the other five passing; a
zeroed SelectionProof arm fails only that subtest.
A first version of the root test also carried a "Fulu control" asserting the
Fulu and Electra roots match. It was deleted: both fields are the same Go type
and it was handed the same pointer, so it asserted only that the hasher is
deterministic, not what its comment claimed. A reflection-based test asserting
the ssz-type struct tag reachable through each arm was written first, as the
only way to get an assertion-level failure out of a type change, and deleted
once it had served that purpose, since the pinned root subsumes it.
Files changed:
- modified: spec/versionedaggregateandproof.go (import, comment, field)
- modified: spec/versionedsignedaggregateandproof.go (import, comment, field)
- modified: http/submitaggregateattestations_internal_test.go (gloas types; its
comment had asserted the now-disproven premise that the Gloas aggregate wire
format is unchanged from electra)
- new: spec/versionedaggregateandproof_test.go
- new: spec/versionedsignedaggregateandproof_test.go
Verified: go build ./... and go vet ./... clean; gosilent test ./... 2261 green,
and -race clean on spec/...; custom-gcl 0 issues repo-wide; gofmt clean on all
five files; go generate ./spec/gloas byte-identical, no SSZ drift; go.mod and
go.sum untouched. Note .golangci.yml sets run.tests: false, so lint never
inspects the test files and go vet is what covers them.
Five independent reviewers ran. Spec compliance independently recomputed the
pinned root two ways outside this repository, once from a hand-written
transcription of the merkleization rules in ssz/simple-serialize.md and once via
eth-remerkleable at the version consensus-specs itself pins, both matching byte
for byte. It also read the consensus-specs build tooling rather than only the
markdown, confirming that a container not redefined in a later fork is carried
forward as source text and recompiled in that fork's namespace, so Gloas's
AggregateAndProof resolves to Gloas's Attestation even though gloas/validator.md
carries no explicit "Modified AggregateAndProof" section the way
electra/validator.md does. Efficiency benchmarked the two merkleizations and
found progressive neither materially faster nor slower at any realistic size.
After one fix round, which cut the test files from 416 lines to 311, all five
approve.
Blockers / notes for next iteration:
- Two pre-existing defects found during review and captured as separate
follow-up work, both fork-independent rather than Gloas-specific. First, both
versioned aggregate wrappers fail open on partially-populated containers:
HashTreeRoot() returns a signable root when Aggregate or Data is nil, and the
two states produce the identical root, so the root is not injective over them.
Since this method feeds a BLS signer, that is the more interesting of the two.
The accessors panic on the same partial structs. Neither is remote-triggerable
because neither wrapper is a deserialization target. Second,
createUnversionedAggregates POSTs a body of [null] when Version names a nil
arm, since it never checks the selected arm is populated even though the
wrapper has an IsEmpty() method that would detect it.
- The consensus-spec ssz_static vectors gate does not apply here: nothing under
spec/gloas changed and go generate confirms no drift. Those v1.7.0-alpha.12
vectors also remain unobtainable upstream, the newest published release still
predating gloas, which is why the pinned root is a drift guard rather than
spec verification.
- The gloas aggregate containers this change makes reachable are not in the
ethpandaops fork at all, on any branch: they were authored during the earlier
types work under a port framing. Their shapes were checked against the spec
here and match, but the same caveat may apply to other types that work
introduced, and is worth a look.
- No live-network verification was attempted, and none is needed: the change is
a compile-time retype whose observable effect is a hash tree root, verified
against an independent implementation of the merkleization rules. Submitting a
real Gloas aggregate would additionally need a validator able to sign one.
Five http endpoints decode SSZ through one of two branches: a dynssz codec
built from the spec the node reports, or the statically generated
mainnet-preset codec. The devnet enablement passed WithCustomSpecSupport(true)
to all four service constructors in http/main_test.go, so every test took the
dynssz branch and the static one -- the default, and therefore the path every
mainnet client that does not opt in actually runs -- had no coverage left. A
regression in it would have shipped with a green suite.
The flag is correct and stays. What is added is a default-mode path beside it.
Why the coverage cannot come from a live node: the static codecs are generated
at the mainnet preset, and the only Glamsterdam endpoint available is
minimal-preset. Measured, a mainnet-preset gloas.BeaconState needs 3,134,845
bytes of fixed fields while the devnet serves 110,789, so the static decode
rejects it outright -- RANDAO_MIXES alone is 2MiB at mainnet against 2KiB at
minimal. The new unit tests therefore marshal their own bodies in-process with
the same generated codecs and need no beacon node.
Coverage is per fork arm, because the branch is hand-copied per arm in the
production switch: 8 arms for beaconStateFromSSZ, 7 for
signedBeaconBlockFromSSZ, 12 for beaconBlockProposalFromSSZ (blinded and full),
plus blobsFromSSZ and blobSidecarsFromSSZ with their empty-body early return,
which no other test reaches. 27 rows against 27 independently breakable lines.
Key decisions:
- The seam is one constructor, newTestService(ctx, customSpecSupport,
params...), which replaces four near-identical http.New blocks. A test
wanting the other mode calls it with false; no address, timeout or bearer
token handling is restated. An earlier draft wrapped it in a second shared
global plus a testServiceDefaultSpec/testServiceWithSpecMode accessor pair,
and that needed a sync.Map to stop the weight-1 coordinator semaphore being
acquired twice by one test and deadlocking. All of it was deleted on review:
with the single caller using the constructor directly nothing acquires
twice, so testService keeps its original acquire block verbatim. The guard
was also narrower than it looked, since t.Run yields a fresh *testing.T and
a parent/subtest pair would still have deadlocked. Removing the cause beat
guarding the symptom, and main_test.go now nets 16 fewer lines.
- Assertions read through the versioned wrapper's own Slot() accessor rather
than checking the fork field non-nil. This matters: the decoders assign
response.Data.<Fork> = &<fork>.BeaconState{} before they branch, so NotNil
can never fail, and a zero-value container re-marshals byte-identically to a
fresh allocation. The first draft of the beacon-state test asserted NotNil,
passed, and was vacuous. Slot() fails both on a skipped decode (slot 0) and
on a body landing in another fork's arm.
- The live test asserts the default mode's expected failure on a non-mainnet
preset instead of skipping there. A skip would never run in the only
environment available, and the assertion doubles as a fail-open guard:
deleting a static decode call produces no error at all, just a zero-valued
state that reads as real. It is scoped to the beacon state because that is
the one SSZ endpoint the devnet serves in both modes -- the block endpoints
have no gloas arm yet and the devnet head carries no blobs.
- beaconstate_test.go's ad-hoc jsonService is a fifth copy of the same
bearer-token branching and was deliberately left alone. A second,
structurally different copy lives in events_internal_test.go, which is
package http and so cannot reach a package http_test helper at all; a
correct de-duplication spans that boundary and belongs to the tracked
de-duplication work, not here.
Files changed:
- modified: http/main_test.go (newTestService replaces four http.New blocks;
35 insertions, 51 deletions)
- new: http/staticsszdecode_internal_test.go (static branch, all five
decoders, 31 subtests)
- new: http/staticsszdecode_test.go (both spec modes through the real
endpoint)
Verified: go build ./... , go vet ./http/ and gofmt clean; gosilent test ./...
2261 green; custom-gcl 0 issues repo-wide; go generate ./spec/gloas
byte-identical. Against the live Gloas devnet (Lighthouse, minimal preset, head
gloas) the 33 new tests pass, also under -race, and the full http suite goes 233
pass / 60 fail with the 60-name failure set byte-identical to the pre-change
baseline -- zero regressions, +39 passes, exactly the 33 new subtests plus their
6 parents. Note .golangci.yml sets run.tests: false, so lint never inspected
these three files; go vet and gofmt are what cover them.
Coverage is real, not assumed. Seven distinct mutations were applied to the
production decoders and each was caught by exactly its own subtest and no
other: removing the static decode in the beaconstate electra and gloas arms,
the signedbeaconblock deneb arm, the proposal capella-full and electra-blinded
arms, blobs and blobsidecars; inverting the branch condition so default mode
reaches the nil dynssz codec panics as it should. Two were re-run after the
review trims. Every production file was confirmed clean at HEAD afterwards.
Blockers / notes for next iteration:
- These tests still cannot fail CI. TestMain returns without calling m.Run()
unless HTTP_ADDRESS is set, and the internal and external test files share
that one TestMain, so go test ./http/ reports ok having executed zero tests
-- including the new tests, which make no network calls of their own. The
coverage restored here only becomes a CI gate once that is fixed; it is
tracked separately and criterion 5 explicitly held it out of scope, so
TestMain is untouched.
- SignedBeaconBlock against the devnet fails in both modes with "unhandled
block version gloas", which is the deferred ePBS block/proposal endpoint
work, not a defect here. BlobSidecars returns 400 "block is pre-Deneb and
has no blobs" for a gloas head block, which looks like a node-side quirk
rather than a library one; worth a second look if blob coverage is wanted
against this devnet.
- The 60 pre-existing devnet failures are the syncing node's "client is not
synced" plus the known JSON beacon-state codec bug and devnet-scale
mismatches. Count differs from earlier sessions only because sync distance
has grown; the failure set was captured before and after and is identical.
- Two hand-written service constructions remain outside the new seam, in
beaconstate_test.go and events_internal_test.go. The latter is in package
http and cannot use a package http_test helper, so any real fix needs an
exported or duplicated seam.
DataVersion.MarshalJSON indexed the fixed-size dataVersionStrings array directly with no bounds check, where String() guarded the identical access and returned "unknown". A version past the end of the table therefore panicked with index out of range during marshal, aborting the marshal of the whole enclosing response object. encoding/json re-panics anything that is not its own sentinel, so the caller cannot recover it. The normal decode path cannot produce such a value -- UnmarshalJSON is a closed map lookup that rejects unrecognised strings. It needs a DataVersion built directly: a caller casting an int, a fork constant added to the enum and not to the table, or an SSZ union decoder copying an unvalidated wire field into the enum. MarshalJSON now takes its string from String(), so dataVersionStrings is indexed at exactly one site and the two readers cannot diverge. That alone was not enough, and finding out why was the substance of this change. String()'s own guard read `int(d) >= len(dataVersionStrings)`, but DataVersion is uint64: for any value at or above 1<<63 the conversion to signed int wraps negative, the comparison is false, and the array is then indexed with the unconverted uint64. Delegating to String() would have closed 9 through 2^63-1 and left the top half of the domain panicking -- from String() as well as from MarshalJSON. Reproduced before fixing: DataVersion(1<<63) panicked with "index out of range [9223372036854775808] with length 9" from both. The guard now compares in uint64, matching the type being bounded and the form the sibling BuilderVersion.String() already used; dataversion.go was the anomaly in its own package. Key decisions: - Both defects are one fix, not two. The acceptance criterion is that marshalling an out-of-range version returns a value rather than panicking, and 1<<63 is out of range, so stopping at the delegation would have left the criterion unmet while looking met. - The test marshals &test.version, not test.version, and says why. MarshalJSON has a pointer receiver, so encoding/json only invokes it for an addressable value; json.Marshal on a bare DataVersion silently bypasses the marshaler and emits the underlying number. A test written the obvious way would have passed against the unfixed code and proven nothing. - Expected values are pinned literals rather than String() output. Once MarshalJSON is implemented in terms of String(), comparing the two holds however either drifts. The literals also lock the enum's iota block and the string table together: insert a fork mid-enum without extending the table and every later row shifts and fails, which is the divergence class this change exists for. - One table with the out-of-range values as ordinary rows, following api/v1/validatorstate_test.go for the same bug class. require.NotPanics is kept deliberately: an unwrapped panic aborts the whole test binary, so siblings never report, while wrapped it fails exactly one subtest. - Scope held to DataVersion. The structurally identical unguarded index in BuilderVersion.MarshalJSON, and the SSZ decoders that make these panics remotely reachable, are tracked separately rather than folded in. Files changed: - modified: spec/version/dataversion.go (MarshalJSON delegates to String(); String()'s guard compares in uint64; 2 lines changed, 4 comment lines added) - new: spec/version/dataversion_test.go (12 rows; the first test file in this package) Verified: go build ./... clean; gosilent test ./... 2273 green, up from 2261; -race clean; custom-gcl 0 issues repo-wide; gofmt and go vet clean on both files -- .golangci.yml sets run.tests: false, so lint never inspects the test file and vet is what covers it. The SSZ codegen and consensus-spec vector gates do not apply: nothing under spec/gloas was touched. Coverage is mutation-proven, not assumed. Three mutations, each failing exactly its own rows and no others: reverting MarshalJSON to the direct index fails the three out-of-range rows while all nine valid-version rows still pass; reverting the guard to int(d) fails only SignedRangeBoundary and MaxUint64; corrupting one string-table entry fails only that row. The first of those is the evidence that the wire format is unchanged -- the valid-version output is byte-identical with and without the fix. Blockers / notes for next iteration: - BuilderVersion.MarshalJSON carries the identical unguarded index and is worse: responseBuilderVersionStrings has length 1, so it panics for any non-zero value, no extreme input needed. Its String() already has the correct uint64 guard, so the fix is the same one-line delegation. It is reachable from untrusted relay registration traffic via the validator-registration SSZ decoder, and is worth prioritising. - Three generated SSZ union decoders copy an unvalidated 8-byte discriminant straight into the version enum and return nil. Every offset in those decoders is range-checked; the discriminant is not. DataVersion is now closed against anything they can produce, so this is no longer a panic vector through it, but it still is through BuilderVersion, and a decoded union naming no arm is a state no constructor can produce. These are generated files, so any fix belongs in the generator config or in hand-written validation beside them. - Pre-existing and deliberately untouched: fmt.Appendf(nil, "%q", ...) measured ~6x slower than a manual quote-wrap with one extra allocation, unchanged by this commit and called once per response object rather than per sub-item. - dataVersionStrings, the iota block and dataVersionMap are three hand- maintained parallel lists of the same fork names. The pinned rows now lock the first two together; the map is guarded by nothing.
The observational half of the payload timeliness committee surface already
existed on this branch -- the four PayloadAttestation* containers,
gloas.BeaconState.PTCWindow, and the payload_attestation_message SSE topic --
so a validator could watch PTC activity but had no way to take part in it.
The four endpoints that make it participatory were missing entirely: duties,
data production, pool retrieval, and submission.
None of this is a port. The ethpandaops fork has no PTC endpoints on any
branch, so every wire-format detail here was read from the beacon-APIs spec
directly and then checked against two running clients rather than
pattern-matched from an adjacent endpoint. That turned up three things the
endpoint table alone would not have given:
- producePayloadAttestationData defines a 204 meaning "no block has been seen
for this slot; do not attest". Nothing in this package inspected a status
code before, and s.get returns a 204 with no error and no content type, so
the existing shape would have surfaced it as "unhandled content type
unknown".
- submitPayloadAttestationMessages requires an Eth-Consensus-Version header.
Prysm refuses a headerless request before it looks at the body.
- Lighthouse does not implement the pool GET at all; prysm implements all
four. The pool GET is the only one of the four with no Lighthouse route.
Key decisions:
- A 204 returns the new sentinel ErrNoPayloadAttestationData rather than an
empty-but-valid response. The response form is the tempting one, since the
wrapper already has IsEmpty(), but it hands a caller who forgets to check a
zero-valued PayloadAttestationData that its accessors report without error
-- a signable vote for the zero root. This is the fail-open class the
hallucination log keeps recording, so it fails closed instead, in the shape
sql.ErrNoRows uses for the same reason: a normal outcome, modelled as a
sentinel precisely so it cannot be mistaken for data.
- Each endpoint splits its network fetch from its response decode. This is not
layering for its own sake: it is the only way the two most interesting paths
of producePayloadAttestationData get covered at all. A node produces data
only for its current slot, so a 204 cannot be provoked on demand, and
neither client serves this endpoint as SSZ. Both are exercised in-process
against a constructed httpResponse.
- PayloadAttestationData decodes SSZ with the static generated codec, not the
dynssz codec every other Gloas endpoint uses. The container is a root, a
slot and two bools with no dynssz-* tags, so it is preset-independent and
the static codec is correct at any preset -- which matters here because the
devnet runs minimal. Benchmarked: 13ns/1 alloc against 6,700ns/114 allocs
for the dynssz path under WithCustomSpecSupport, which rebuilds its type
cache per call.
- Three wrappers, not the two the versioned responses need. The third wraps
PayloadAttestationMessage so the required consensus-version header is
derived from the caller's data instead of hard-coded "gloas", matching both
existing submitters. The durable reason is not the header though: Messages
is a shipped exported field, and typing it []*gloas.PayloadAttestationMessage
would make it the only fork-typed submitter opts in the repo and a breaking
change at the next fork.
- The wrappers' accessors funnel through one private helper and collapse the
seven pre-Gloas versions into a single arm that interpolates the version.
The precedent spells out seven arms per accessor with hard-coded messages;
the text produced is byte-identical and the drift between accessors that the
older wrappers already show becomes impossible.
- providerparity_test.go makes the multi/mock/testclients parity a build
error. Nothing else in this repo enforces it -- implementations are reached
by runtime type assertion, so a missing method compiles cleanly and fails
only where a caller asserts. It holds declarations and no test function,
which was verified to still fail the build: renaming mock's PTCDuties breaks
`go test ./...` outright.
- PTCDuties builds its request body with json.Marshal over a []string rather
than the hand-rolled fmt.Fprintf loop its analogue uses. Same bytes, and it
avoids adding a fifth copy of a loop that already has tracked
de-duplication work against it. Measured, it is also the faster of the two.
- Mock override hooks follow each endpoint's named analogue rather than being
applied uniformly: PTCDuties and PayloadAttestationData get them, matching
AttesterDuties and AttestationData; the pool and the submitter do not,
matching theirs.
Two defects were fixed during review, both the same fail-open shape:
- The JSON arm seeded its decoder with an empty datum, and decodeJSONResponse
only writes the keys it finds, so a body of {"version":"gloas"} with no data
key at all returned success wrapping a zero datum. It now seeds a typed nil
pointer, which makes an absent or null data key distinguishable from a
present one. Probed afterwards: a present-but-partial data object is
rejected too, by the container's own required-field decoding.
- verifyPayloadAttestationPool early-returned when no slot filter was given,
which skipped the element loop entirely, so [null,null] came back
unvalidated. It now always reads each element and filters by slot only when
a slot was asked for. A null element cannot arrive as a nil slice element --
the wrap loop always allocates -- so this errors rather than panicking, which
was verified rather than assumed.
Files changed:
- new: api/v1/ptcduty.go (+test)
- new: api/{ptcduties,payloadattestationdata,payloadattestationpool,submitpayloadattestationmessages}opts.go
- new: spec/versionedpayloadattestation{data,,message}.go (+3 tests)
- new: http/{ptcduties,payloadattestationdata,payloadattestationpool,submitpayloadattestationmessages}.go
(+4 external tests, +4 internal tests)
- new: multi/ and mock/ wrappers for all four
- new: providerparity_test.go
- modified: service.go (4 provider interfaces), errors.go (1 sentinel),
mock/service.go (2 hook fields), testclients/{erroring,sleepy}.go (4 methods each)
38 files, 2971 insertions, 0 deletions. No existing file under http/ was
modified, which is what makes a regression in the existing suite structurally
impossible rather than merely unobserved.
Verified: go build ./... clean; gosilent test ./... 2306 green, up from 2273;
custom-gcl 0 issues repo-wide; go vet and gofmt clean on all 38 files; go
generate ./spec/gloas byte-identical. Against a live Glamsterdam devnet-7
prysm node (minimal preset) all 40 new subtests pass, also under -race.
Lighthouse was not usable as the target: it reports is_syncing true, so every
call fails the synced assertion. Note .golangci.yml sets run.tests: false, so
lint never inspects the test files -- go vet and gofmt are what cover them.
Coverage is mutation-proven. Eight mutations, each failing exactly its own
subtest and no others: neutering the 204 guard; renaming the consensus-version
header, which failed the submit test on precisely the assertion written to
detect it; removing the nil-arm guard that would put [null] in a request body;
transposing the two boolean accessors; wrapping every pool element with the
first element's attestation; removing the slots-per-epoch zero guard; and,
after the review trims, re-running the boolean transposition and the parity
assertion. The boolean one is the instructive case: the fixture originally set
both flags true, so the transposition was invisible to every accessor test and
only a dedicated test caught it. The fixture now sets them apart, which lets
the ordinary subtest catch it and the dedicated test be deleted.
Five reviewers ran. Spec independently confirmed the preset-independence claim
by reading the generated codec, and re-ran everything live. Efficiency
benchmarked the static-versus-dynamic SSZ decision. Security probed the
decoders with hostile bodies through a test overlay. After one fix round all
five approve.
Blockers / notes for next iteration:
- The success path of producePayloadAttestationData has no live coverage. A
node serves it only for its current slot, and this devnet's chain has
stalled: head slot is around 59450 against a wall-clock slot around 71500,
so lighthouse answers 400 "Invalid slot" for every slot and prysm returns
nothing at the current one. A healthy devnet would close this; it is the one
acceptance criterion not verified end to end.
- The same stall makes wall-clock-derived epochs unusable: asking prysm for
duties at the wall-clock epoch hangs the request indefinitely, which is why
the new tests derive epoch and slot from the node's head. That is also
correct on a healthy chain, where the two coincide. It is worth knowing that
the pre-existing TestAttesterDuties fails here for exactly this reason and
not for anything this change did.
- No http/ test can gate CI, since TestMain returns without calling m.Run()
unless HTTP_ADDRESS is set. Everything added here under http/ is therefore
invisible to ordinary CI; the api/v1, spec/ and root-package tests, which
include the parity assertions, are not. Tracked separately.
- The pool GET is prysm-only today. Anything downstream that needs it should
not assume a Lighthouse node can serve it.
- Two pre-existing defects found during review and captured as separate
follow-up work, both fork-independent rather than Gloas-specific. The
epoch-range check on duties now exists in three copies and only the new one
guards a node reporting SLOTS_PER_EPOCH of 0, where the other two underflow
the range end to MaxUint64 and accept everything, or nil-deref on a null
duty. And thirteen http endpoints pass a fresh &api.CommonOpts{} instead of
&opts.Common, silently discarding the caller's per-call timeout; the zero
value means "use the default", so the bug is invisible.
- Unchecked aggregation_bits length in the gloas payload-attestation JSON
decoder is real but pre-existing and already tracked; it cannot panic,
because go-bitfield guards the vector length.
Two copy-paste duplications introduced by the ePBS transport work, both
"use the abstraction that already exists", both zero behaviour change.
The request-body content-type negotiation over an `any` value -- JSON when
enforced, otherwise the dynssz codec -- existed twice: once as the private
method submitExecutionPayloadEnvelopeData, once inline inside
postExecutionPayloadBid. The two produced identical error values, so a change
to negotiation had to land in both places and could silently drift. They are
now one method, marshalRequestBody, called from both submitters.
Separately, decodeFixedBytes (trim 0x, hex-decode, length-check, copy) had two
callers, both inside its own defining file, while fastconfirmationevent.go and
executionpayloadavailableevent.go hand-rolled the identical sequence. Both now
call it: four callers, two of them outside the defining file.
The interesting part was proving "no behaviour change" rather than asserting
it, because the two halves had opposite safety nets. The api/v1 targets were
already fully covered -- the existing tests pin the exact error strings, down
to `incorrect length 31 for block` -- so those tests passing unmodified is the
proof, and git confirms no test file was touched. The http targets had none:
the JSON arm of both submitters had zero coverage, since WithEnforceJSON is
only ever exercised against responses. A broken extraction there would have
shipped green, so the negotiation got a test first.
Key decisions:
- decodeFixedBytes lost its wantLen parameter, deriving the required length
from len(dst). This was forced: taking the callers from two to four, all
passing 32, made unparam report `wantLen always receives 32`. It is a true
positive, so the parameter went rather than a nolint. Every caller passes a
slice of a full fixed-size array, so len(dst) is the same constant 32 it
replaces, and it closes a real hazard -- a wantLen larger than len(dst) would
previously pass the guard and then let copy silently truncate, admitting a
wrong root. Verified two ways: hard-coding 32 in place of len(dst) leaves the
suite green, and reverting to the four-argument form reproduces the unparam
finding while the original two call sites do not trigger it at all.
- marshalRequestBody lives in its own file rather than in either caller or in
http.go. Its two callers are in different files, so either would make
ownership arbitrary, and the package already keeps small single-purpose
helpers this way (json.go, contenttype.go, domain.go, stateid.go). The
response-side negotiation in http.go is the Accept header, a different
concern.
- submitproposal.go was deliberately left alone despite also branching on
enforceJSON. It dispatches per fork version over a typed
*api.VersionedSignedProposal rather than marshalling one already-selected
`any`, so it cannot use this helper; unifying them would be a mismatch, not
a missed opportunity.
- ptcduty.go keeps its hand-rolled decode. Its length error is the bare
`incorrect length for public key`, pinned verbatim by its own test, so
converting it would change the emitted string and break that test -- the one
thing this change must not do. Duty types are also outside the scope of the
event-type de-duplication.
- The new test builds a bare &Service{} instead of going through the
constructor. With customSpecSupport false, dynSSZForRequest returns the
process-global codec and makes no network call, so both arms are reachable
in-process; this matches the package's existing internal tests, which
construct httpResponse values directly.
- errors.go gets a copyright-year bump. That is a pre-existing lint failure,
not fallout from this change: it reproduces at clean HEAD with a cold lint
cache, and was previously masked because golangci-lint had the root package
cached. Editing api/v1 invalidates that cache, since the root package
imports it, which is the only reason it surfaced here. Without the bump the
lint gate is not green.
Files changed:
- new: http/marshalrequestbody.go, http/marshalrequestbody_internal_test.go
- modified: http/submitexecutionpayloadbid.go (inline negotiation deleted),
http/submitexecutionpayloadenvelope.go (submitExecutionPayloadEnvelopeData
deleted)
- modified: api/v1/executionpayloadevent.go (helper signature, 2 call sites),
api/v1/fastconfirmationevent.go, api/v1/executionpayloadavailableevent.go
- modified: errors.go (copyright header only)
Net -79/+9 production lines plus the helper and its test. No test file that
existed before this change was modified.
Verified: go build ./... clean; gosilent test ./... 2306 green, matching the
pre-change count exactly because http/main_test.go returns without calling
m.Run() unless HTTP_ADDRESS is set; -race clean on api/v1; gofmt and go vet
clean on all eight files; custom-gcl 0 issues repo-wide on a cold cache. Note
.golangci.yml sets run.tests: false, so lint never inspects the test files --
go vet and gofmt are what cover them.
Against a live Glamsterdam devnet-7 prysm node the full http suite was run
before and after: the failure set is byte-identical across all 147 entries,
and the only new passes are the five belonging to the new test. Those 147 are
pre-existing devnet conditions, dominated by 107 "client is not active" that
begin once the live-network tests start and cascade; every in-process test
passes in both runs.
Coverage is mutation-proven, not assumed. Ten mutations, each failing exactly
its own subtests: inverting the negotiation arm fails all four of the new
subtests; dropping the SSZ error wrap fails only SSZMarshalFailure; returning
a content type other than Unknown on JSON-marshal failure fails only
JSONMarshalFailure; making the length guard one-sided fails only the
over-length cases across all three event types; dropping the %d from the
helper's error fails exactly the length subtests; neutering its copy fails
exactly the round-trip subtests. The two that matter most are the pair that
must stay green: hard-coding 32 for len(dst), which proves the signature
change is behaviour-identical rather than merely untested.
Five reviewers ran and all approve. Security built a 24-case differential
harness running each hostile input through both the old wantLen logic and the
new len(dst) form and found zero divergence in accept/reject decision or
destination bytes. Efficiency measured escape analysis and benchmarks: dst
does not escape at any call site and there is no new allocation or boxing.
After one round of trims, all five approve.
Blockers / notes for next iteration:
- The devnet chain is dead and this is the more important note. Both consensus
clients have zero peers and are around 24,000 slots behind wall clock, with
the execution clients stalled and divergent. Lighthouse additionally cannot
run epoch processing forward -- it reports
EpochProcessingError(BeaconStateError(InvalidIndicesCount)) -- so every
validator duties endpoint returns 500, and it reports is_syncing true, which
the synced assertion refuses. Prysm reports is_syncing false on the same
chain because it derives that flag from sync-service activity and with no
peers there is nothing to do, which is the only reason it is usable as a test
target. Recreating the enclave is needed before any chain-dependent
verification means anything; Lighthouse may still fail epoch processing
afterwards, which would be a client bug worth reporting upstream.
- Asking prysm for attestation data at a wall-clock slot never returns on this
chain, while the same call at its head fails in a millisecond.
aggregateattestation_test.go derives its slot from wall clock, so it blocks
for the full client timeout and every live test after it fails with "client
is not active". That cascade is what produces most of the 147 failures, and
it means the shared-service harness turns one timed-out request into a
package-wide failure. Worth fixing independently of any fork work.
- No http/ test can gate CI, since TestMain returns without calling m.Run()
unless HTTP_ADDRESS is set. The new negotiation test is therefore invisible
to ordinary CI even though it needs no network; the api/v1 coverage, which
is what proves the decode half, is not. Tracked separately.
- proposerduty.go already emits the %d-parameterised length error that
decodeFixedBytes produces, and its own test pins that form, so converting it
would be a safe zero-behaviour-change adoption. Its two duty siblings use the
bare errors.New form and would not be. Any wider sweep should check each file
individually rather than assuming the four duty types share one constraint.
- About forty further copies of the hand-rolled decode remain across nineteen
other api/v1 files, payloadattributesevent.go alone holding twelve. They are
deliberately untouched: the house form emits a message without the %d and
many tests pin it, so a wider adoption is a repo-wide sweep that has to pick
one error string and update the assertions that pin the other, not a
per-package change.
- golangci-lint is non-deterministic here in a way worth knowing about. It
reported a nolintlint finding in spec/capella once and then reported zero
issues on the two following runs, cold and warm, in a file this change does
not touch. The same cache behaviour is what hid the errors.go finding. A
single green lint run is not strong evidence; clean the cache and repeat.
…posal wrapper
Groundwork for the gloas-onwards block production endpoint
(GET /eth/v4/validator/blocks/{slot}). The endpoint itself is not here; this is
the type surface it returns, landed separately because it is independently
verifiable without a node and because the endpoint's shape depends on it.
None of this is a port. The ethpandaops fork has no v4 support on any branch --
its proposal.go is still on /eth/v3/validator/blocks/%d on both gloas and
master, and no v4 file exists -- so every wire detail was read from the
beacon-APIs spec at commit 4b4d89a and not pattern-matched from the v3
endpoint.
Reading the spec rather than trusting a second-hand summary immediately paid
for itself. produceBlockV4 was documented internally as having no
execution_payload_value, on the reasoning that payload value now arrives via
the bid, so the existing ExecutionValue concept did not apply. That was true
when written and is now false: execution_payload_value was added as a required
response field with a matching Eth-Execution-Payload-Value header by
beacon-APIs PR #631, which is the current tip. Traced with
`git log -S execution_payload_value -- apis/validator/block.v4.yaml` against a
fresh clone rather than inferred. The wrapper therefore carries both value
components.
Key decisions:
- BlockContents mirrors SignedExecutionPayloadEnvelopeContents rather than
api/v1/fulu.BlockContents, which is the closer-looking analogue. Fulu's tags
are ssz-max/ssz-size only, so its generated codec is correct at the mainnet
preset alone; the envelope contents added on this branch carry dynssz-max as
well. The validation devnet runs the minimal preset, so the mainnet-only form
would have been undecodable exactly where it is first exercised.
- IncludePayload is *bool, not bool. It is the axis that replaced blinded
versus unblinded, and it is an operational cliff rather than a formatting
flag: true means the envelope travels with the block so any node can publish
it, false means the producing node caches it and must also be the publisher.
The spec marks the parameter required with no default. A Go bool would
zero-value to false, silently selecting the mode that constrains where the
block can be published, so absence is made representable and will be
rejected rather than resolved.
- The wrapper keeps execution_payload_included as a flag over two arms, which
is the shape the existing proposal wrapper already uses for blinded. Worth
noting the inversion: there the contents arm is the unblinded one, here the
contents arm is the payload-included one.
- Accessors funnel through two private helpers, block() and contents(), not
one. Both arms carry a block, but only the included arm has an envelope,
blobs and proofs. Asking for an envelope when the payload was excluded is a
caller error -- it has to be fetched from the producing node -- so those
accessors return an error saying so rather than an empty value, which would
yield a proposal that cannot be published. The pre-gloas and unknown-version
arms are collapsed into one case each, so the two helpers cannot drift.
- The accessor set is deliberately eight, not the eighteen the older proposal
wrapper carries. Slot and RandaoReveal are what the endpoint's own
consistency checks need; the envelope, blobs and proofs are what a caller
needs to publish; Value, IsEmpty and String are house form. Anything else is
reachable from Block() and was left out rather than mirrored speculatively.
- The JSON test compares re-marshalled bytes instead of using require.Equal on
the structs. Struct equality fails here for a reason that is not this
codec's: the nested gloas.ExecutionPayload codec normalises a nil
Transactions, ExtraData or BlockAccessList to an empty slice on the way back
in, so the assertion would pin another package's conventions. Byte equality
keeps the assertion sensitive to what matters, which the mutation testing
below confirms.
Files changed:
- new: api/v1/gloas/blockcontents.go, blockcontents_json.go,
blockcontents_yaml.go, blockcontents_ssz.go (generated), blockcontents_test.go
- new: api/versionedepbsproposal.go, api/versionedepbsproposal_test.go,
api/epbsproposalopts.go
- modified: api/v1/gloas/generate.yaml (one codegen entry)
Verified: go build ./... clean; gosilent test ./... 2334 green, up from 2306;
custom-gcl 0 issues repo-wide; go vet and gofmt clean on all eight new files;
go generate ./api/v1/gloas leaves the pre-existing generated sibling
byte-identical. Note .golangci.yml sets run.tests: false, so lint never
inspects the test files -- go vet and gofmt are what cover them. 21 tracked
files under api/ are gofmt-dirty at HEAD; none of them are touched here.
Coverage is mutation-proven rather than assumed. Swallowing the RawJSON
presence error fails exactly the four missing-field subtests and nothing else;
dropping the blobs unmarshal fails both the dedicated blob test and the
round-trip byte comparison, which is the evidence that comparing bytes did not
weaken the assertion. Both were reverted and the suite confirmed green after.
One test defect was caught by running the RED rather than by review: the
blobs-included subtest passed against a stub returning nil, because
require.Empty is satisfied by nil. It asserts a length now. A vacuous
assertion that looks like coverage is the failure mode this repository's
hallucination log keeps recording, and it is only visible if you read which
subtests fail during RED.
Blockers / notes for next iteration:
- The endpoint, its opts validation, the validator-side envelope GET, the
provider interfaces and the multi/mock/testclients parity are all still to
do, as are the two mechanical gloas arms on signedbeaconblock and
submitproposal. Nothing here is reachable from a client yet: these types have
no caller, so a mistake in them is currently invisible outside their own
tests.
- submitproposal's SSZ path marshals with the plain generated codec, no
dynssz. A gloas block reaches preset-sized fields -- SyncAggregate's
committee bits are 512 entries at mainnet and 32 at minimal -- so the gloas
arm may need the request-scoped dynssz codec that the newer submitters use.
This should be settled empirically against the minimal-preset devnet, not by
reasoning, and if it turns out to affect every fork equally then it is a
pre-existing limitation to capture separately rather than to fix inside the
gloas arm.
- The blinded surface is deliberately untouched and must stay that way: there
is no Gloas.SignedBlindedBeaconBlock schema, and two of the three blinded
endpoints have been removed from the spec entirely.
- No reviewer pass has run over this yet. It was committed as a verified
subset to hand over working code, not as a finished slice.
…e GET
Completes the Gloas block-production surface on top of the types landed
previously. Two new endpoints plus the two mechanical arms on existing ones, so a
validator can now produce, publish and retrieve a post-Gloas block end to end.
None of this is a port. The ethpandaops fork has no v4 support on any branch, so
every wire detail was read from the beacon-APIs spec at commit 4b4d89a -- still
the tip, re-checked rather than assumed, since drift on this endpoint had already
invalidated one earlier assumption -- and then exercised against a running node.
Verifying against a real node rather than the spec alone is what turned up the
things that mattered:
- prysm omits execution_payload_value entirely, both the body field and the
Eth-Execution-Payload-Value header, although the spec marks it required. Added
by beacon-APIs #631 two days before this work; prysm predates it. Both value
components are therefore treated as zero when absent rather than demanded.
- prysm defaults include_payload to true when the parameter is absent, despite
the spec marking it required with no default. Found by mutation: dropping the
parameter from the query fails only the payload-excluded tests. So a caller who
left it unset would silently get whichever mode its node preferred, which is
the argument for the pointer type arriving from the opposite direction than
expected.
- prysm serves the removed blinded shape from the validator envelope GET,
payload_root where the spec requires payload. Reproducible across a 40-sample
probe on two of three prysm nodes. Our decoder rejects it, correctly; that
captured body is now the fixture pinning the rejection.
Key decisions:
- The SSZ arms decode through s.dynSSZForRequest, not the generated codec, and
this was settled by measurement rather than argument. Against real
minimal-preset bytes the generated codec fails with "first offset 336 does not
match expected 396": a gloas body holds a preset-sized sync committee
bitvector, 64 bytes at mainnet against 4 at minimal, and SSZ addresses
variable-size fields by offsets stored after it, so every offset shifts by
exactly 60. The error names ProposerSlashings, which is merely the first
variable field validated -- a misleading name worth knowing about. An
in-process test reproduces the same failure by sizing its fixture from the
node's spec, so the requirement is pinned without needing the node's block
production.
- The envelope's SSZ arm also uses the dynamic codec, but for a weaker reason,
stated as such in the code. Measured, that container encodes byte-identically
at both presets and the generated codec reads minimal bytes fine, so nothing
here proves the dynamic one necessary. It is kept because it cannot be wrong at
a preset this code has not run against, and the endpoint is called once per
proposal. The comment says that rather than claiming a requirement.
- The 404 sentinel joins the api.Error rather than replacing it. This looked like
a free simplification and was not: multi.doCall decides whether a node is
broken by unwrapping to an api.Error and reading its status, so a bare sentinel
turned the answer every non-producing node gives into "the whole set is
failing". Demonstrated with four real clients against the devnet -- one call
left 0 of 4 active; with the join, 4 of 4. Joining also keeps the response body,
which is the only way to tell an empty cache from a node that does not serve
the route at all, both being 404s. Nimbus does not serve it, so that is not
hypothetical.
- What the node returns is checked against what was asked for, in one helper per
endpoint. Slot, payload-inclusion mode and RANDAO reveal for the proposal; the
beacon block root for the envelope. None of these can be provoked live -- a
node that honours the request never disagrees with it -- so each is tested in
process, and separately a pair of tests points the service at a deliberately
disagreeing local server to prove the guards are reachable at all. Deleting
either call site fails those and nothing else; without them an unwired guard
leaves the suite green.
- Value headers are bounded before parsing. big.Int.SetString is quadratic in its
input, the length is the node's to choose, and the parse happens after the
request's own deadline is released: a 9MiB header measured 75 seconds of CPU on
the one path with a slot to meet, against 56 microseconds once bounded.
Negative values are refused too, since Value() would otherwise subtract them
and make a proposal look cheaper than a rival's. The same flaw is left untouched
in proposal.go, being pre-existing and outside this change, and is captured as
follow-up work -- it is the endpoint in production use, so it matters more than
the instance fixed here.
- submitproposal's gloas body is a plain SignedBeaconBlock, not a contents
wrapper. Post-Gloas the blobs travel in the envelope, published separately.
AssertPresent also refuses a gloas proposal marked blinded outright rather than
ignoring the flag, which would publish a plain block while the caller believed
otherwise.
- VersionedSignedProposal's Slot, ProposerIndex, String and assertMessagePresent
gained gloas arms as well as AssertPresent. Adding the field without them left
a populated proposal reporting its own fork as unsupported; nothing in this repo
calls those accessors, so it would have surfaced only in a consumer.
- providerparity_test.go grows by two blocks rather than a file, per its own note.
Verified it still bites: renaming mock's EPBSProposal breaks the build.
Files changed:
- new: http/epbsproposal.go, http/executionpayloadenvelope.go (+5 test files)
- new: api/executionpayloadenvelopeopts.go
- new: mock/{epbsproposal,executionpayloadenvelope}.go (+1 test),
multi/{epbsproposal,executionpayloadenvelope}.go
- modified: http/signedbeaconblock.go and http/submitproposal.go (gloas arms),
api/versionedsignedproposal.go (Gloas field + 5 arms), api/epbsproposalopts.go
(corrected a doc claim), service.go (2 provider interfaces), errors.go
(1 sentinel), mock/service.go (2 hooks), testclients/{erroring,sleepy}.go,
providerparity_test.go, http/staticsszdecode_internal_test.go
28 files, 2778 insertions, 3 deletions.
Verified: go build ./... clean; gosilent test ./... 2338 green, up from 2334;
-race clean on api, mock, multi and the root package; custom-gcl 0 issues
repo-wide on a cold cache; go vet and gofmt clean on all 28 files. Note the lint
gate is ./custom-gcl, not golangci-lint, which exits 3 here because the config
references plugins it does not carry.
Against the live devnet the full http suite was run before and after, from a
detached worktree at the previous commit so the comparison is against real
baseline behaviour rather than a stash. The three TestSignedBeaconBlock failures
are fixed. Three TestBlobs/TestBlobsSidecars subtests newly fail, and they are
worth understanding rather than dismissing: those tests fetch the head block
first and require.NoError it, which aborted them at setup on a gloas chain, so
the parent looked red while its version gate and every subtest below were
unreachable. They now run for the first time here and fail on their own fixture
assumption, that the head block carries blobs -- it does not, and the endpoints
correctly return an empty list. Captured as follow-up work.
TestBeaconBlockRootTimeout flips on the unchanged baseline too.
Coverage is mutation-proven, 25 mutations in all, each failing exactly its own
tests. The instructive ones: swallowing the ParseBool error fails only the
malformed-header test, which is the case proposal.go's EqualFold form gets wrong;
substituting the generated codec fails the preset test and both live SSZ tests;
transposing the SSZ arms fails four; reading the JSON flag from the header fails
all eleven JSON tests, which is what proves the body is genuinely its source; and
sending the wrong consensus-version header fails only live, a class no in-process
test can reach. Two mutations survived a first pass and both were real gaps: the
payload-inclusion check had no test, and the mutation harness itself
under-reported by matching only indented failures, hiding top-level ones.
Five reviewers ran. Security demonstrated two high findings with working exploits
-- the header parse and the client-rotation drain -- and re-verified both fixes by
re-running its own demonstrations. Efficiency benchmarked the decode paths.
Spec caught a doc comment inherited from the v3 options struct claiming a default
this endpoint deliberately does not apply. Two reviewers independently reproduced
the rotation drain, which is what made it credible.
Blockers / notes for next iteration:
- The include_payload=false round trip cannot be completed end to end, and this
is the one acceptance criterion left unticked rather than worked around.
Retrieving the cached envelope needs a node that serves the spec's shape;
prysm serves the deleted blinded one, nimbus does not implement the endpoint or
v4 at all, and grandine implements v4 but was partitioned off the chain
throughout. Tolerating payload_root would mean re-introducing a container the
spec removed, against the decision that keeps the blinded surface frozen.
- submitproposal's SSZ path is mainnet-preset only, and silently so: the
generated codec zero-pads a short sync committee bitvector rather than
refusing it, so a minimal-preset block marshals cleanly and the node rejects
the bytes. Verified identical for electra and gloas, so it is fork-independent
and left alone here, with the arm mirroring its siblings and a comment pointing
at the tracked defect. The live submit test therefore goes through JSON.
- Retrieving the envelope through a multi-client returns nothing when the first
active client is not the producing node, because doCall has no "try the next
one without deactivating it" outcome. The alternative was the rotation drain,
so this is the better of the two available behaviours, not a good one.
- The devnet's health script reports UNHEALTHY on a working chain when one node
partitions, deriving head epoch and missed-slot rate from the wrong node. It
said destroy and relaunch; the four-node cluster was finalizing normally. Do
not act on that verdict without checking the reference node directly.
- No http/ test can gate CI, since TestMain returns without calling m.Run()
unless HTTP_ADDRESS is set. Almost everything added here is therefore invisible
to ordinary CI; the mock, api and root-package tests are not, which is why the
mock's default-response coverage lives in mock/ rather than being asserted
through an http/ test.
…tation bitfield.Bitvector512 is a mainnet-fixed type. BitAt guards on `len(b) != 64` and Len() returns the constant 512, so on the minimal preset -- PTC_SIZE=16, a two-byte AggregationBits, which is what the validation devnet runs -- a correctly decoded, correctly marshalable value reads false at every index and SetBitAt discards every write. The SSZ path is sound (dynssz ceils PTC_SIZE/8 to one byte), which is why this survived transport validation: only the Go accessors are wrong, and only at a preset the tests never used. Three additive accessors -- PTCSize, AttestedAt, SetAttestedAt -- plus field documentation. Nothing is retyped, so the decision that keeps these fields mainnet-typed is untouched; there is also nothing to retype to, go-bitfield having no runtime-sized vector and no Bitvector16. Key decisions: - Out-of-range is an error, not a false or a silent no-op. This is the crux rather than a nicety: the defect is not merely that the width check is wrong but that failing it is indistinguishable from an unset bit. A bool-only reader would have reproduced the same defect at a different width. Two mutations demonstrate the bound is load-bearing -- weakening `>=` to `>`, or making PTCSize return 512, both panic with "index out of range [2] with length 2" against a minimal-preset value. - Reads are implemented over the bytes rather than delegated to the length-tolerant BitIndices, and this was settled by measurement, not preference. Delegation was the obvious option -- BitIndices and Count are already width-correct -- but there is no tolerant single-bit reader, so `slices.Contains(bits.BitIndices(), i)` is O(n) with a 4KB make() per call against O(1) and no allocation. Benchmarked: 4.45ns against 799.3ns at mainnet, and 2.8us against 101us for a full 512-position sweep at realistic participation; the naive per-call form is 113x slower, being O(n^2) over the sweep. The success path is 0 allocs. The bit math had to exist for the setter regardless, so sharing it also makes the two directions agree by construction. - The setter cannot delegate at all, and that asymmetry is why this could not have been closed by documentation. bitfield's length-tolerant family -- BitIndices, Count, Bytes -- is all readers; SetBitAt is the only setter and is length-strict. A doc comment can redirect a read; there is nothing to redirect a write to, so constructing a minimal-preset aggregate was not expressible against this type at all. - The tests build their fixtures as hand-written bytes and cross-check against BitIndices/Count as an independent oracle. Deriving fixtures from SetAttestedAt would have asserted only that the code agrees with itself -- it would pass just as happily with MSB-first ordering, and the write would then be invisible to every other reader of these bits. The oracle is the point: it is what makes a wrong bit order fail. - PTCSize documents that the width is the producing node's claim, not a verified fact. Nothing on the JSON path checks aggregation_bits against PTC_SIZE, so the accessors are safe at any width but authoritative at none. Fixing that is tracked separately, with one correction recorded: the planned upper-bound-only check would reject the harmless over-length case (which already fails SSZ and visibly contradicts itself) while accepting the under-length one, which round-trips cleanly and lets a node shrink the committee it reports. A fixed-width vector needs equality. - Value bumped from three exported methods to four documented traps: Shift panics outright on a value shorter than eight bytes -- binary.BigEndian.Uint64 on a 2-byte slice -- so it fails on a legitimate minimal-preset value rather than merely misbehaving. Nothing here calls it; the field comment would otherwise have read as exhaustive guidance while omitting the one method that crashes. Files changed: - new: spec/gloas/payloadattestation_test.go - modified: spec/gloas/payloadattestation.go (3 accessors + shared bound check), spec/gloas/attestation.go (CommitteeBits field doc; notes electra's identical latent trap without sweeping it), spec/versionedpayloadattestation.go (doc pointer on the accessor that hands out the trapped value), spec/versionedpayloadattestation_test.go and http/payloadattestationpool_internal_test.go (call sites converted off BitAt) - modified: api/versionedsignedproposal.go, http/signedbeaconblock.go, http/submitproposal.go (copyright header line only) 9 files, 352 insertions, 12 deletions. Verified: go build ./... clean; gosilent test ./... 2355 green, up from 2338; -race clean on spec and api; go vet and gofmt clean on all nine files; custom-gcl 0 issues repo-wide on a cold cache; go generate ./spec/gloas leaves the generated siblings byte-identical, which is what confirms the struct-tag whitespace change in attestation.go is inert. Note the lint gate is ./custom-gcl, not golangci-lint, which exits 3 here. The three copyright-header bumps are pre-existing branch debt, not fallout: the same three findings reproduce at HEAD in a detached worktree, they are the files earlier gloas-port commits touched, and the rule diffs against origin/master. Without them the gate is not green. Coverage is mutation-proven, five mutations each failing exactly its own tests: MSB-first bit ordering fails both directions at every width; PTCSize returning the mainnet constant and the off-by-one bound both panic; the setter ignoring its flag fails only the clearing assertions; the setter writing byte 0 fails the byte and oracle assertions. The compressed test runner under-reports here, showing "1 failed" where --verbose showed three, so mutation verdicts were read verbatim. The harness also adjudicated a review disagreement: after deleting a test argued to be redundant, all five mutations were still caught, which is what turned "looks subsumed" into evidence. Also verified against a live devnet node, since http/ tests are skipped entirely unless HTTP_ADDRESS is set: the converted call site passes, and fails when AttestedAt is neutered, which is what proves it exercises the accessor rather than merely compiling against it. Blockers / notes for next iteration: - Retrieving participation through spec.VersionedPayloadAttestation still means reaching into .Gloas, since the wrapper has no AttestedAt/PTCSize of its own. That is the shape the decided scope asked for -- accessors on the container, gloas only -- but it means consumers dereference an arm the wrapper would otherwise nil-check for them. A versioned-level pair would remove both that hazard and the need to name the fork type. - electra.Attestation.CommitteeBits carries the same tag and the same latent trap, deliberately noted rather than fixed. It has no broken path today because its two helpers already read through BitIndices, but that is a property of those helpers, not of the type. - The lint gate is not fully deterministic. A reviewer saw a nolintlint finding in spec/fulu at HEAD that does not reproduce on this tree either repo-wide or scoped, cold cache or warm; the file and the config are byte-identical to HEAD, so only cache state differed. Third recorded instance. A single green run remains weak evidence.
multi.Service.Events built a substituted options struct per client, pointed its handler fields at activeHandler's own methods, and stored that same struct as the thing the wrappers forwarded through. So h.opts.Handler(event) called h.genericHandler -- itself, unboundedly. A Go stack overflow is fatal and unrecoverable, recover() cannot catch it, so it takes the process down. Three defects share that root cause. The struct also copied only Common, not Topics; and the inactive-client path handed the underlying client the caller's raw opts, bypassing active-address filtering entirely. What the bug actually did in production is not what it looks like, and this only came out by running it. Against a real node the recursion never fires: http rejects a subscription naming no topics, so every active client failed to subscribe, was demoted to the inactive list, and was re-subscribed there with the caller's raw opts -- bypassing the wrapper, hence no recursion, and hence no filtering either. Measured against two devnet nodes over 45s, before and after, from a detached worktree at the previous commit: 14 head events across ~7 slots before, every event delivered twice, once per node, unfiltered; 6 across 6 slots after. So the real impact was duplicate unfiltered delivery -- precisely the inconsistency the wrapper exists to prevent -- and the Topics defect was masking the recursion one. The overflow needs a client that accepts a topic-less subscription; the mock does, which is what the unit test uses, and its RED run dies with "fatal error: stack overflow". Key decisions: - Substitution is nil-aware rather than unconditional, and this is load-bearing rather than tidiness. http dispatches per topic through a fallback chain -- specific handler if set, else the generic one -- so installing a wrapper for a handler the caller never supplied would both starve that fallback and call a nil function. substitute returns nil for a nil handler, so the rule lives at one choke point instead of 22 call sites. - One generic substitute[T] rather than 22 typed wrapper methods. This corrects something asserted in the other direction earlier in the work: the handler signatures differ by type, but they share the shape func(context.Context, T), and inference resolves T through each named handler type. Measured efficiency-neutral -- per-call deltas smaller than the run-to-run noise floor of an unchanged control function, 0 allocs/op both ways, and disassembly shows the per-type and shared-shape closure bodies are byte-identical with the generic dictionary written once at construction and never read per call. - Wrappers capture the caller's handler by value rather than reading it back out of the options. Self-reference is then unconstructible rather than guarded against: the handler is evaluated before the closure that would contain it exists, so the reference chain strictly decreases. It also removes a live read of caller-owned memory at event-delivery time, which was a data race for any caller that mutated its options mid-subscription. - clientOpts stays a distinct struct, but not for the reason the first draft of this change claimed. Self-recursion is no longer the hazard; per-client nesting is. One handler is built per client, so wrapping the caller's struct in place would have the second client's wrapper wrap the first's, filtering one event against two addresses at once. Demonstrated: aliasing the struct fails TestEventsFiltersNonPrimaryClient specifically. - Completeness is enforced by the test, not the production code. The wiring is 23 explicit lines; the test walks api.EventsOpts reflectively, so a handler added later joins it automatically and fails until wired. A hand-written list would need the same manual update the production code does, and would therefore miss in exactly the case that already went wrong -- 10 of 23 handlers were never forwarded, including all seven Gloas ones. - The per-handler filter collapsed into one forwards() predicate, which also fixed an unmeasured cost: the old form called log.With()...Logger() on every event, and zerolog's With() allocates a 500-byte context buffer with no level check. Benchmarked at the default level, where Trace is disabled: 512 B/op and ~100ns become 0 B/op and ~5ns. The same defensive clone at the two newActiveHandler call sites is dropped for the same reason -- With() always allocates fresh, so sharing a logger context cannot corrupt it. Files changed: - modified: multi/events.go (the fix; 354 lines to 227, wiring 23 handlers instead of 13), multi/events_test.go (6 tests, one with 23 reflective subtests) 2 files, 371 insertions, 229 deletions. Verified: go build, go vet and gofmt clean; gosilent test ./... 2383 green; -race clean on multi, and clean over -count=20; ./custom-gcl 0 issues repo-wide. Note the lint gate is ./custom-gcl, not golangci-lint, which exits 3 here. Against the live devnet the caller's handlers receive real events at one delivery per slot with two nodes subscribed, and the generic handler is correctly starved for topics carrying a specific one -- which is the only check that exercises the real fallback dispatch, since the mock does not implement it. Coverage is mutation-proven, 9 mutations each failing exactly its own tests: restoring the recursion kills the binary outright; dropping either nil guard fails 24 and 23 tests; aliasing the caller's struct fails only the non-primary filter test; omitting Topics or Common fails only the options test; dropping one handler's wiring fails only that handler's subtest; inverting the filter fails 28; and cross-wiring the one same-signature pair, ExecutionPayload against ExecutionPayloadGossip, fails 3 -- that pair compiles silently, so the test is the only thing standing between it and a nil deref. Blockers / notes for next iteration: - Reviewers found two pre-existing defects that are captured separately rather than fixed here: mock assigns a package-level logger from New() while live mocks read it, which is a data race reproducible at the previous commit; and multi.Events returns nil even when no client subscribed successfully, which is what made this bug invisible from the outside. - Also pre-existing and untouched: the inactive-client poller sleeps 5s without selecting on ctx.Done(), so a cancelled context does not stop it promptly, and Events appends to a slice header copied from s.inactiveClients after releasing the read lock, so concurrent calls can write the same backing-array slot. - One unexplained observation, recorded rather than resolved: a reviewer saw the two tests asserting the primary address fail together in a single verbose run. It did not reproduce in roughly 360 further runs across two machines-worth of attempts, sequential, parallel and under -race, and there is no mechanism -- the rechecking monitor first fires at 30s, well after these tests finish.
attgo's current-year analyzer resolves its changed-file set by shelling out to `git diff --name-status origin/master...HEAD`. Under actions/checkout@v4's default fetch-depth of 1 there is no origin/master ref and no origin/HEAD, so gitDefaultBranch() errors, the analyzer warns to stderr and falls back to checking every file in the repo instead of the ones the branch touches. The fallback is observable in the message it emits: the "new or modified files" wording comes from the fileStatusUnchanged branch, which is only reachable when git resolution failed. With only-new-issues filtering an all-files run, an arbitrary subset leaked through as failures on files this branch never touched. Fetching full history lets the three-dot diff resolve, so the analyzer checks the branch's own files and nothing else.
…ed version execution_payload_availability is a Bitvector[SLOTS_PER_HISTORICAL_ROOT], which beacon-APIs types/primitive.yaml defines as a single hex string. The codec had it as []string, and both directions were wrong independently, for different reasons. Marshal ranged over the bytes and emitted one decimal string each. Unmarshal decoded straight into the []uint8 field, which gets encoding/json's []byte special case -- base64, not hex -- so a real node's own output was rejected outright. That is the reported failure: "illegal base64 data at input byte 16", reproduced byte-for-byte by the new test. Byte 16 is a fingerprint rather than a coincidence: "0xbffbffffffffffff" is 18 chars all of which happen to be in the base64 alphabet, so the decode clears four quanta and only then chokes on the 2-char remainder. The fix is the justification_bits form three fields earlier in the same file, and the sibling beaconstate_yaml.go was already correct for this very field, so the JSON codec was the lone outlier rather than a new pattern being invented. Key decisions: - The field-by-field audit the work item asked for found a second defect and it is fixed here rather than filed: Builder.unpack decoded version into its intermediate struct and never assigned it, so every builder read over JSON silently claimed version 0. Fixed because the item's own round-trip criterion is unmeetable while it stands -- a real state contains builders -- and because version is hashed, so a JSON-decoded state computed a different state root than the node's. That last point came from review and is a stronger reason than the one the change was originally made on. - Builder.MarshalYAML omitted version from its literal too, so the read fix alone left the type half-repaired: YAML could read the field but never wrote it. Also fixed. Version lives in three independently hand-written composite literals -- MarshalJSON, unpack, MarshalYAML -- and drift between hand-copied field lists is the actual mechanism behind this whole family. Each literal now has exactly one test guarding it, which is what the mutation run below demonstrates. - Nothing else was folded in. Four further findings are filed instead, three of them escalated by review beyond how they first looked: Builder.version is emitted as a JSON number where primitive.yaml's Uint8 is a string, and that is a hard decode failure rather than a cosmetic mismatch, since an untagged uint8 cannot accept a JSON string at all; BuilderPendingPayment and BuilderPendingWithdrawal carry no JSON tags at all and serialise with PascalCase Go field names; the bitvector marshal emits "" rather than "0x" for an empty value, violating the pattern this change's own test asserts; and the per-element fmt.Sprintf loops in this file cost ~79ms per mainnet-state marshal, ~1500x what this fix saves. The first two are latent consensus-spec-vector failures that will surface the moment those vectors exist. - No length or width check was added. That is deliberate: the policy is owned elsewhere and actively disputed, and justification_bits has none either. - The two audit instruments are deliberately different, because a round-trip is structurally blind to a field wrong the same way in both directions. Shapes are therefore checked against the spec rather than against our own output. Demonstrated, not asserted: making ptc_window hex in both directions passes the round-trip and fails only the shape test. The same run also refutes the work item's guess that ptc_window was the next bad field -- it is a Vector of ValidatorIndex, so decimal-per-entry is correct there, and the test now pins that against being "fixed" to match its neighbour. - The bug reached live validation because the pre-existing fixture leaves this field nil, and an empty array round-trips clean. Nil it again with the bug restored and the round-trip still passes, which is the mechanism exactly. Hence a fixture populating all 46 fields; it is also what caught the builder defect. - The type change repairs the YAML read path as a side effect worth knowing about. BeaconState.UnmarshalYAML deliberately routes through beaconStateJSON, so the JSON struct's field type governs YAML too: the correct hex string from the YAML marshaller met a []string and failed with "string was used where sequence is expected". gloas.BeaconState YAML round-tripping was broken outright, and the consensus-spec tests that would have caught it skip without vectors on disk. Files changed: - new: spec/gloas/beaconstate_test.go, spec/gloas/builder_test.go - modified: spec/gloas/beaconstate_json.go (field type, marshal, unmarshal), spec/gloas/builder.go (2 assignments), spec/gloas/jsonlength_test.go (BeaconState round-trip row + the amended note its widened scope needs) 5 files, 1 struct field retyped, 4 production lines changed. Verified: go build clean; gosilent test ./... 2390 green, up from 2383; -race clean on spec; go vet and gofmt clean; go generate ./spec/gloas leaves the generated siblings byte-identical; ./custom-gcl 0 issues repo-wide. Note the lint gate is ./custom-gcl, not golangci-lint, which exits 3 here. Live validation is deferred rather than skipped quietly: no Gloas endpoint is reachable, and http/ tests skip entirely without HTTP_ADDRESS, so the fix is proved by captured fixture instead. The audit was driven off the upstream spec at v1.7.0-alpha.12 and beacon-APIs master, both re-fetched during review rather than recalled. All 46 top-level fields are present, in spec order, with the correct shape class; execution_payload_availability was the only byte-slice field in spec/gloas mis-declared, on the mechanical criterion that a dynssz-size of SOMETHING/8 marks a packed bitvector. Coverage is mutation-proven, 6 mutations each failing exactly its own tests, and two of them exist only because a reviewer found them: reverting the unmarshal fails the JSON test, the YAML test and the table row; dropping the 0x prefix fails the JSON and shape tests but not the round-trip, since both directions still agree; ptc_window hex both ways fails only the shape test; and version dropped from each of the three literals fails a different single test in each case. Efficiency measured the fix at mainnet width rather than the test's minimal one: 65,405ns and 1,003 allocs become 13,959ns and 20, with the wire form 2.78x smaller. That ratio also explains the silence -- the old cost was one allocation per byte, so at the 8-byte minimal preset the tests use it sat inside the noise floor, and only 1024 bytes makes it visible. Blockers / notes for next iteration: - The include-a-real-node half of the acceptance criteria stays unticked. Nothing here has been seen by a Gloas node. - Whether Builder.version should be a quoted string on the wire cannot be settled without one. Prysm may well emit a bare number today, in which case conforming to the spec breaks against prysm; accepting both on read is the likely landing, and it is filed rather than guessed at. - The duplicated, mislabelled execution-address length check a few lines below the fix in builder.go is untouched. It is pre-existing and already owned by another work item, so fixing it here would poach that item's scope. - Reviewers independently reproduced two pre-existing hazards outside this diff: HashTreeRoot's zero-padding writes past len into a caller-supplied backing array, unreachable from here because hex.DecodeString always returns cap == len; and response bodies are still read unbounded before any codec runs, which is why the unvalidated bitvector width cannot allocate anything not already committed.
submitProposalSSZ marshalled with each fork container's generated codec, which has the mainnet preset compiled in. A block body holds a preset-sized sync committee bitvector -- 512 bits at mainnet, 32 at minimal -- and SSZ addresses the variable-size fields after it by offsets, so at any other preset the whole body shifts by 60 bytes and the node cannot read it. It failed silently, which is the part that made it expensive to diagnose. The generated codec does not reject a short bitvector; it pads it out to the mainnet length and returns no error. Measured: a block whose SyncCommitteeBits is 4 bytes and one whose bits are 64 bytes marshal to byte-identical output, 1132 bytes for electra and 924 for gloas, err=nil both times. Nothing local can observe the mismatch, so the first sign of it is a server-side rejection far from the cause. The fix routes every fork arm through marshalRequestBody, which already picks JSON vs SSZ and encodes SSZ with the request-scoped dynamic codec. That is the helper the two newer submitters use, so submitproposal.go was the last one still on the generated codec. Key decisions: - The two duplicated eight-arm version switches collapse into one. This is cause, not tidiness: the duplication is what let the SSZ arm drift from the JSON one, and the Gloas arm carried a five-line comment apologising for a defect it could not fix locally. The JSON/SSZ choice now exists once repo-wide. - The switch is inlined in submitProposalData rather than extracted, matching submitexecutionpayloadbid and submitexecutionpayloadenvelope, and the local is named container rather than block -- three of the eight arms are SignedBlockContents wrappers, not bare blocks, and this is a distinction the rest of the codebase names carefully. - The default arm stays although it is unreachable: AssertPresent runs first and rejects every version the switch does not name, which is why the new UnknownVersion test asserts "unsupported version" and not the switch's own error. It is kept so the two switches cannot drift apart silently, and now says so. - Nothing was folded in. Three findings are filed instead, two of them about the shared codec helper rather than this endpoint: it rebuilds its codec per call and throws away the type cache, measured at ~178us and 2381 allocs against ~596ns and 2 for a reused instance, and >99% of that is construct-and-discard that happens whether or not the preset actually diverges; node-supplied spec values now size an outbound body with no bound past uint32, where SYNC_COMMITTEE_SIZE=4294967295 yields a 536MB body with err=nil; and AssertPresent accepts a blinded Bellatrix-through-Fulu proposal while the switch reaches for the nil non-blinded arm, publishing a zero-valued block. The last is pre-existing and byte-for-byte unchanged here. - No bound was added on the spec values, deliberately. The helper is shared by four call sites and the width-check policy is owned by an existing family of items and actively disputed, so fixing it here would poach that scope. What this change alters is breadth rather than kind: three ePBS routes already drove encoding off node-supplied spec values, and this puts the universal publish path on the same footing. Files changed: - modified: http/submitproposal.go (two switches to one, 89 lines deleted), http/submitproposal_internal_test.go (2 new tests, the existing one rewritten to drive submitProposalData now that the two encoders it called are gone) 2 files, 156 insertions, 108 deletions. 4 production lines do the work. Verified live, before and after, against a minimal-preset node with gloas active, reproduced twice as the devnet's canonical endpoint moved. Before: 924 bytes, HTTP 400, "could not decode request body into gloas consensus block: Block: Body: invalid ssz encoding. first variable element offset indexes into fixed value data" -- the node's own words for the 60-byte shift. After: 864 bytes, HTTP 500, "could not get block's prestate: could not reconstruct parent state", so the body parses and is rejected on consensus grounds because the fixture has a zero parent root. Getting past decode is the signal. Also verified: go build, go vet and gofmt clean; ./custom-gcl 0 issues repo-wide (note the gate is ./custom-gcl, not golangci-lint, which exits 3 here); -race clean. The full suite's failing set is byte-identical to a detached worktree at the previous commit, so none of it is attributable here; a later run found two further pre-existing failures and some of them are non-deterministic, the devnet having degraded to UNHEALTHY mid-session. Coverage is mutation-proven. Cross-wiring the electra arm to fulu fails the parity test on electra alone, with the byte diff landing exactly on the slot field, 0x258 against 0x2bc; dropping the AssertPresent guard fails MissingArm, MarkedBlinded and UnknownVersion, the last by emitting the switch's error instead of the presence check's, which is what pins which guard runs first. The preset test's own RED was the node-side error reproduced in-process, "first offset 396 does not match expected 336". The parity test earns its place separately from the fix: it pins that the dynamic codec leaves the mainnet wire format alone. Global dynssz output is byte-identical to the generated codec for all eight arms, so the default path is a zero-wire-change refactor, and each arm is given a slot unique to its fork so a mis-wire encodes a different valid block rather than a nil one -- two empty containers could otherwise encode identically and let it pass. Blockers / notes for next iteration: - The fix is inert unless the caller sets WithCustomSpecSupport(true). Without a spec to work from the client has nothing to encode against, so the honest claim is that a custom-spec client can now publish, not that any non-mainnet client can. The impact line as originally written overstated this. - A nil arm marshals to a zero-valued block with err=nil under both codecs rather than panicking, because the generated code carries nil-receiver guards. The bytes are not all zero, since the SSZ offsets are not. - Not verified: that a genuinely valid block is accepted. That needs a real signed block for the current slot, which no synthetic fixture provides.
…end the token
Two internal test helpers built their service with a bare New() and no
Authorization header, so they failed against any node requiring auth. New() pings
via CheckConnectionState and returns client.ErrNotActive on failure, so the whole
diagnosis a reader gets is "client is not active", which names neither auth nor a
header.
The cause is a package boundary rather than carelessness. newTestService, which
centralises address, timeout and token, lives in main_test.go in package http_test;
the internal tests are package http and cannot see it. Every one of them therefore
re-derived New()'s parameters by hand, and two of the four got it wrong. The
events_internal_test.go pair had the correct if/else all along, which is what makes
this a consistency failure and not a knowledge one.
The fix is one constructor for package http, mirroring the one the external package
already has, with all four call sites moved onto it.
Key decisions:
- The helper returns *Service, not (client.Service, error). Being inside the package
it can, and three call sites were each asserting service.(*Service) afterwards --
one of them checking an assertion that cannot fail, since New only ever returns
that type. Swallowing the error matches the two helpers that already existed here.
- It is named newTestService, identical to its package http_test counterpart. Same
directory, different packages, different signatures. The duplication is forced by
Go's package rules, and the shared name is the strongest available signal that one
is the other's counterpart; timeout already sits in both files under one name.
- fmt.Sprintf("Bearer %s", token) is kept over concatenation, and the relocated
timeout var is kept over inlining. Both were raised as simplifications and both
are declined for the same reason: this file's value is being a faithful mirror,
and the line doing the security-relevant work is the last place to introduce a
gratuitous difference. timeout is also pre-existing code that merely moved.
- The token test is a table with one row per constructor, deliberately redundant
while all three route through the helper. Reviewed as over-engineering and
defended on evidence: re-hand-rolling customSpecService fails its own row and
leaves the other two green, so the rows catch a single call site drifting back
off the helper -- the exact regression this commit exists to prevent, and one
that already got past human review once. The comment says so, so it is not
deleted later for the same reason it was questioned.
- The two guards are separate tests because they cover different arms and a mutation
each proves it: dropping the token block fails the table and not the omit test;
making the header unconditional fails the omit test and not the table.
- The four audited sites and the query that found them are named in the helper's doc
comment, not only here, because a future reader asking whether the sweep was
complete is reading the code.
- Nothing else was folded in. Three findings are filed instead: a live node sends
ptc_window as a JSON object where the gloas BeaconState codec expects an array,
contradicting an earlier fixture-derived conclusion about that field;
TestBeaconBlockRootTimeout is flaky, failing 3 of 5 runs at the previous commit
because it asserts a 1ms timeout beats a localhost round trip; and
beaconstate_test.go still re-hand-rolls the token branch that the external
newTestService already provides, correct today but the same shape of duplication.
Files changed:
- new: http/main_internal_test.go (helper, relocated timeout, 2 tests)
- modified: http/epbsguardwiring_internal_test.go, http/epbsproposal_internal_test.go,
http/events_internal_test.go (call sites; 46 lines and 3 imports deleted)
4 files, 166 insertions, 46 deletions. The helper is the only new logic.
Verified before and after against a genuinely authenticated endpoint, which is the
point: these tests already passed against an unauthenticated node, so that
configuration proves nothing. A reverse proxy requiring a bearer token in front of
the devnet supplied one. Before, from a detached worktree at the previous commit,
all three named tests failed with "client is not active"; after, all three pass. The
proxy logged three 401s on /eth/v1/node/syncing with an empty Authorization header
during the before run and none during the after run, which is the mechanism end to
end. A reviewer reproduced this independently with its own proxy, port and token.
TestSubmitProposalDataAtTheNodesPreset was fixed too, unnamed in the report but
routed through the same customSpecService.
Also verified: go build, go vet, gofmt clean on all four files (24 other files carry
pre-existing gofmt noise, identical in a baseline worktree); ./custom-gcl 0 issues
repo-wide, and note the gate is ./custom-gcl rather than golangci-lint, which exits 3
here; -race clean on the affected tests; gosilent test ./... 2390 green. Both
configurations pass, authenticated and not: with no HTTP_BEARER_TOKEN the table
synthesises one, and the node ignores it.
The full-suite failing set was diffed against a detached worktree at the previous
commit on the same endpoint. It removes exactly the three named tests plus
TestSubmitProposalDataAtTheNodesPreset. Two entries differ for reasons that are not
this change: TestBeaconBlockRootTimeout and TestAggregateAttestation/NotFound are
both non-deterministic, the former demonstrated at 3 failures in 5 baseline runs.
Blockers / notes for next iteration:
- The devnet went down late in the session, so the last edits, which are comments
only, were gated on build, vet, gofmt, lint and the full no-node suite rather than
on a live re-run. The table test has not executed since; the omit test has, since
it needs no node.
- That omit test is the only one here that runs without a node. It works by pointing
at a closed port with WithAllowDelayedStart, and it runs at all only because
TestMain merely requires HTTP_ADDRESS to be non-empty.
- The anti-drift guard does run in CI: http-tests.yml runs on every pull request with
HTTP_ADDRESS from a repository variable and HTTP_BEARER_TOKEN from a secret. It does
not run in the default test job, where TestMain returns before m.Run() and this
directory contributes nothing.
- A caller passing its own WithExtraHeaders would silently reinstate this bug, since
that option replaces the header map rather than merging into it. Nothing does today
and no guard was added; it is documented on the helper.
…point http-tests was permanently red: 19 FAIL lines against a node on fulu, from tests written against a Gloas devnet with no way to leave themselves out. A permanently red check gates nothing, because it cannot tell 19 known failures from 19 known plus a regression without someone diffing logs by hand. The 19 are FAIL lines, not tests. Go reports a parent and each subtest separately, so they are 10 top-level tests, and they do not share a precondition: most need the chain to be on Gloas, one needs only that the node implements the endpoints, and three were never a fork problem at all. Each fork-dependent test now asks the node at runtime and skips, and CI points the same suite at two endpoints. Key decisions: - Buckets were assigned from the error text, never the test name, and the CI log overrode the summary they were filed with. TestPTCDuties, TestPayloadAttestationPool and TestSubmitPayloadAttestationMessages each failed in one subtest only -- their NilOpts/NoIndices/NoMessages siblings passed, being client-side refusals that reach no network. Gating those parents would have buried five working subtests, which is the failure this change exists to prevent, arriving by the front door. Only TestEPBSProposal, whose four subtests all failed, is gated whole. - Two levels, not one. KnowsGloas reads GLOAS_FORK_EPOCH from the spec and gates the pre-Gloas-behaviour test; OnGloas reads the head block's fork and gates the rest. PreGloasSlotIsNotSentinel asks for an envelope at slot 1 and asserts a 400, so its only precondition is that an endpoint exists to refuse: keyed on the head's fork it would stay dark on a mainnet-lineage node forever, when it should start running there the day that node's client learns Gloas, years before activation. - PastSlotIsSentinel is gated although it passed. It passed for the wrong reason: a node without the endpoint 404s, and that maps to the very sentinel it asserts. - Presence, not arithmetic, for KnowsGloas: a client that knows the fork publishes a far-future epoch, and that counts as knowing. OnGloas reads the head's own version rather than comparing epochs -- one call, it is what the gated tests depend on since they all operate on head, and it cannot disagree with reality when a node's config and its chain diverge. - HeadVersion is a third helper so that OnGloas and the skip message are answered by one piece of code rather than two that can disagree. It reports "unknown" when the node cannot be asked, which keeps "head is unknown" readable; a mutation to "" prints "head is )". - HTTP_REQUIRE_GLOAS turns skip into fail, read in TestMain. gate takes an interface rather than *testing.T so that choice is testable at all: the real Skip and Fatalf end the calling goroutine, so a test cannot observe what was done to it. - The second job is advisory but cannot be quietly green. A missing address is fatal rather than skipped, because without one TestMain returns before running anything and the job would pass having executed no tests. - Neither job filters by name. Selection in YAML drifts against test names, and the first new ePBS test whose name misses the pattern lands in the gating job silently. Files changed: - new: testclients/forkutil.go (3 helpers), testclients/forkutil_test.go, http/gloasgate_test.go (2 gates, the inversion, and their guards) - modified: .github/workflows/http-tests.yml (second job), http/main_test.go (one knob), and 7 test files carrying 8 gate call sites 12 files, 561 insertions, no deletions. 4 helper bodies do the work. Verified live against a minimal-preset devnet with gloas active. All 8 gated tests run there with zero skips, which is the property that matters most: the gates open on a Gloas node. The inversion is proven end to end rather than by inspection. Same endpoint, two runs differing only in the variable, every gated test flipping SKIP to FAIL and each message naming its own level. The bucket-B assignment is proven against a node that knows Gloas but is not on it, since that is the only shape telling the two levels apart -- on any real node both predicates agree, which is why getting this wrong is invisible. A proxy supplied it. With the correct gate PreGloasSlotIsNotSentinel runs; rewired to OnGloas it skips, and both runs report green, so nothing but the SKIP line distinguishes them. A second proxy, on gloas but with the fork stripped from its config, showed the two levels are independent: PastSlotIsSentinel passes while its neighbour skips. Also verified: go build, go vet, gofmt clean; ./custom-gcl 0 issues repo-wide, and note the gate is ./custom-gcl rather than golangci-lint, which exits 3 here; -race clean with no data races, and no test in the package calls t.Parallel, so the one package-level variable is not shared under a race. The full suite's failing set was diffed against a detached worktree at the previous commit on the same endpoint: nothing fails that did not fail before, and no test gained a SKIP. 2771 tests against 2761, the 10 new ones being these. Coverage is mutation-proven, 6 mutations each failing its own tests: KnowsGloas pinned true fails only the absent row, pinned false only the present row and the live test; HeadVersion's fallback emptied fails the degradation test and the message test; OnGloas pointed at the wrong fork fails the live test; gate reduced to a no-op fails both arms of the inversion. One test earned its keep during the writing. The absent-spec row passed on the first run for the wrong reason -- Spec was returning "client is not active", so the predicate answered false from its error branch and asserted nothing about the key. A predicate returning false both for "key missing" and "could not ask" cannot be tested by its false case alone, so the present row is what makes the absent row mean anything. Finding out cost two iterations discovering that a service needs both /eth/v1/node/syncing and /eth/v1/node/version to come up active. Blockers / notes for next iteration: - http-tests-gloas needs two repository settings before it does anything: HTTP_ADDRESS_GLOAS, without which it now fails loudly by design, and optionally the HTTP_BEARER_TOKEN_GLOAS secret. Until the variable exists the job is red rather than misleading. - Not verified: that http-tests is green against the regression endpoint. That needs its address and token, which are repository settings not available here. The auth defect behind the three non-fork failures has landed, so the expectation is green, but it is an expectation. - OnGloas reads the head block, so the service must be able to decode one. At a non-mainnet preset that means custom spec support, without which the head reads as "unknown" and the gates close for a reason that is not the node's. The suite's shared service already has it. Documented on the helper. - The validation devnet will still show unrelated failures, block production there being v4-only. That is why the job is advisory.
…rves no route The first CI run of the fork gates found the bucket-B premise to be false on the regression endpoint. Seven of the eight gates skipped as intended and the three non-fork failures were gone, leaving exactly one failure: ExecutionPayloadEnvelope/PreGloasSlotIsNotSentinel, which did not skip. It did not skip because KnowsGloas was true. That node already publishes GLOAS_FORK_EPOCH in its configuration and has no ePBS route behind it, so the assumption the gate was built on -- that a client advertising the fork also serves its endpoints -- does not hold. Every request to the endpoint is a bare 404 with no pre-fork refusal inside it, so there is nothing for the test to assert, and it failed on the 404 mapping onto the sentinel it is checking against. The gate itself behaved correctly; what was wrong was reading "knows the fork" as "implements the endpoints". Key decisions: - The escape is withheld once the chain is on Gloas, which is the part that makes this safe rather than a hole in the test. On a Gloas node a 404 for a pre-fork slot is the very defect being guarded against -- it is what maps onto the sentinel -- so treating 404 as "no route yet" everywhere would have deleted the guard while appearing to preserve it. Off Gloas the same status can only mean the route is absent. - KnowsGloas is left exactly as it was, and no third predicate was added. The weaker gate is still correct for this test and still lights it up the day the regression endpoint's client ships the route; what was missing is that a node can sit between the two states for a long time, and that is a property of this one endpoint rather than of the fork question. - It routes through the same gate helper as the fork levels, so HTTP_REQUIRE_GLOAS turns it into a failure too. On the validation devnet the route is served and this branch is unreachable; if it ever fires there, a node that knows Gloas and has withdrawn the endpoint is worth failing over. - The api.Error extraction moved above the sentinel assertion, since the status has to be known before deciding whether to assert at all. No assertion was removed. Files changed: - modified: http/executionpayloadenvelope_test.go (one conditional, one import) Verified against three endpoints, the last two synthesised because no real node occupies the states they represent: - the Gloas devnet, route served: passes, escape not taken, and its neighbour PastSlotIsSentinel still passes - the fork stripped from the config: skips on the KnowsGloas gate as before, so the new branch has not displaced the old one - knows the fork, head withheld, route 404 -- the endpoint's situation reproduced: skips naming the route, and fails instead under HTTP_REQUIRE_GLOAS Also verified: go build, go vet, gofmt clean; ./custom-gcl 0 issues; 2395 tests green with no address; zero skips among the gated tests against the devnet. Blockers / notes for next iteration: - This weakens what the gating job proves about the ePBS surface until that endpoint's client ships the routes: the test will skip rather than run, and the skip message says which of the two reasons applies. That is visible rather than silent, which is the most this endpoint can offer today. - Worth reconsidering at ADR level whether the bucket-B level should be "serves the route" rather than "knows the fork", now that the two are known to be separated by a long interval on a real node. Filed rather than decided here.
Contributor
Author
|
The advisory Gloas job ended in "panic: test timed out after 10m0s" with TestValidators 4m12s in and a couple of tests still to run, so the report it produced was silent about those rather than red or green about them. The cause is scale, not a hang. That devnet carries 503,477 validators, so the validator set is a 222MB response -- measured at 47.7s and 4.7MB/s from a workstation -- and the debug state is 81MB. A runner's link is several times slower again: TestBeaconState took 82s there against roughly a quarter of that locally. Nothing is stuck; the suite simply cannot pull that much inside 10m. Key decisions: - 25m, and only on the advisory job. The other endpoint finishes in 2m49s and keeps its 10m. That one is also the gating job, where a genuine hang should still fail the pull request quickly rather than after a further quarter hour. - The comment says the number is a bandwidth budget, so that a later reader does not tighten it as though it guarded against hangs, and names the validator count as the thing that will invalidate it. - Nothing else was folded in. The job stays red, for reasons that are now itemised in a comment on the pull request: of 15 failing subtests one is a defect in this library (the gloas Builder version wire form), three are an optional header Prysm omits from SSZ v4 proposal responses, six are node-side 500s, and five are expected on any Gloas network. - The whole-collection tests were left alone. Narrowing them to explicit ids would cut the transfer but interacts with HTTP_REQUIRE_GLOAS, which exists precisely so that a skip on this endpoint is always a bug, and it trades away the mainnet-scale decode coverage this endpoint uniquely provides. That is a judgement call about someone else's shared infrastructure, not a timeout fix. Files changed: - modified: .github/workflows/http-tests.yml (one value, plus the comment that explains it) Verified: the YAML parses and both jobs survive it; only the advisory job's timeout moved; the gating job keeps 10m and its continue-on-error setting is untouched. Blockers / notes for next iteration: - 25m is inferred, not observed. The suite has never completed end to end on a runner, so the figure comes from where the 10m run died plus the measured payload sizes. The next run on this branch is what confirms it, and if TestValidators is slower than estimated this will need raising again. - The transfer cost per pull request is now several hundred MB from shared infrastructure. Raising the timeout accepts that cost rather than answering it.
…esolves TestValidators' Genesis, Head, Finalized and Justified rows each fetched every validator on the network, and the shared assertion block never looks at the set's size: those rows leave expectedValidators unset, so all they check is that the state id resolved and the response is well formed. The whole-set fetch was incidental to what they prove, and on a mainnet-scale network it is what the test spends its time on. Measured against the mainnet-preset endpoint, where the validator set is 135.8MB gzipped: the four rows were 94.4s of the test's 121.7s. Three of them now ask for five indices instead, and those three go from 69.43s to 0.70s. The same three on the Gloas endpoint are 0.25s to 2.33s against 23.10s for the row still fetching everything -- same process, same session, same upstream, so roughly ninety-fold. Key decisions: - Head keeps the whole set, deliberately. It is the only place in the suite that decodes a validator list at mainnet scale, around 1.1M entries, and that is worth one fetch per run. Narrowing all four would have deleted that coverage silently, which is a different decision from cutting waste; the comment says so in the file, so it is not "finished off" later by someone tidying up. - Index-narrowing rather than skipping. A skip would have been cheaper still, but the Gloas endpoint's job sets HTTP_REQUIRE_GLOAS precisely so that a skip there is always a bug, and adding a legitimate skip category to that endpoint turns a bright line into a judgement call. A narrowed request produces no skip at all. - Five indices, not the 500-odd that ManyValidatorIndices uses. That row already covers a large explicit index list; these three only need enough to prove the state resolved. - ExitedValidators is untouched. Its cost is a server-side state filter, which is the thing it exists to test. Files changed: - modified: http/validators_test.go (three rows, plus the comment recording the measurements and why Head is the exception) Verified live against both endpoints the CI workflow uses, before and after. Mainnet preset, Lighthouse v8.2.1: 121.69s to 71.70s, all subtests still passing, including both network-pinned pubkey rows, which proves network detection is unaffected. Gloas endpoint, Prysm v7.1.7: 43.11s, all passing or skipped as before. gofmt and go vet clean -- vet matters here because the lint gate skips test files. Blockers / notes for next iteration: - Do not read the 121.69s to 71.70s delta as the saving. Head was 13.6s slower in the second run than the first, because that endpoint's wall clock varies independently of anything in this diff. The defensible figure is the 69.43s to 0.70s on the three rows that changed. - The same whole-collection pattern very likely exists elsewhere in the suite. TestValidatorBalances pulls 503,477 balances on the Gloas endpoint and was not examined; the debug beacon state, at 81MB, has no narrowing available at all.
The comment added with that timeout blamed runner bandwidth: "a runner's link is several times slower than a workstation's". Measurement does not support it. The other endpoint in this same workflow serves a validator set 2.7x larger -- 135.8MB gzipped against 50.0MB -- fetches it four times, and its job finishes in 2m49s. If runner bandwidth were the constraint, that job would be the one in trouble. The actual cause is which upstream the load balancer assigns. That address fronts ~47 nodes with IP-sticky sessions, and the same commit against it 23 minutes apart spent 1m34s on a single TestValidators subtest and exceeded a 10m budget, then ran the whole test in 11.37s and the suite in 103s. Two things rule out the alternatives: head advanced continuously across both runs, so the devnet had not respun, and the x-dugtrio-endpoint-name response header changed between them, from prysm-erigon-1 to prysm-reth-1. Key decisions: - The 25m value is unchanged. The reasoning was wrong, not the number, and the conclusion it supports is stronger under the real cause: variance that large is exactly what a generous ceiling is for. - The comment now names the header to check before investigating a slow job, which is the actionable part a future reader needs and the old text did not give. - The mistake is recorded rather than quietly overwritten. A reader who saw the old reasoning should be able to tell it was tested and dropped, not lost in an edit. - The 222MB figure is gone. It was measured without compression, and Go's HTTP client requests gzip, so the client only ever saw ~50MB. Files changed: - modified: .github/workflows/http-tests.yml (comment only) Verified: the YAML parses and both jobs survive it, with the advisory job still at 25m and continue-on-error, and the gating job still at 10m. No behaviour changes. Blockers / notes for next iteration: - The variance is unexplained beyond attributing it to the upstream. Which node is slow, and why, is not established -- a request-level record of that header across a whole run would be needed, and the job does not capture one.
Nothing in the suite's output named the server, and on the Gloas validation devnet the server is not a fixed thing. That address is a load balancer over ~47 nodes spanning six consensus implementations, and the caller cannot choose its upstream: neither ?client=prysm, ?endpoint=..., X-Dugtrio-Endpoint nor X-Dugtrio-Client changes where a request lands. The implementations disagree about which routes they serve and how they word errors, so the same commit against the same address produced 15 failing subtests on one run and a different 20 on another, with the chain healthy throughout -- finalized two epochs behind head, 58 peers. A reader diffing those two runs could not tell a regression from a different client having answered. This prints one line before any test runs: endpoint identity: status=200 version="teku/v26.7.0+33-g..." upstream=teku-ethrex-1 Key decisions: - It runs before the service is constructed, not after, so the address still identifies itself when the client cannot come up against it -- which is when knowing what is there matters most. - Best effort, always. Every failure is printed and then ignored, and the timeout is 10s: a diagnostic must never decide whether the suite runs, nor add meaningfully to a run that is about to fail anyway. - A raw net/http request rather than the client's own NodeVersion provider, which is the reason for the nethttp import alias. The provider returns the version but not response headers, and the header is the half that identifies the upstream. - The absent-header case reads "none (direct connection)" rather than being left blank, because that is a fact worth stating: on such an address the client cannot change between runs, so this whole class of confusion does not apply. Files changed: - modified: http/main_test.go (one function, its call site, two imports) Verified against all four branches. Load-balanced: names teku/v26.7.0 at teku-ethrex-1, which is also the direct evidence that this pool serves the suite a non-prysm client. Direct node with a token: Lighthouse/v8.2.1, "none (direct connection)". Dead address: reports connection refused, suite still ok, 0.303s, no hang. No token against an endpoint that needs one: status=404 version="unknown", which incidentally surfaces an auth failure that otherwise appears only as "client is not active", because that endpoint answers 404 rather than 401 when unauthenticated. gofmt and go vet clean; vet matters here because the lint gate skips test files. Blockers / notes for next iteration: - Verifying this needs -count=1. Without it, go test served runs 2 and 3 from cache and replayed run 1's stdout, so a changed HTTP_ADDRESS appeared to produce the identical identity line and a dead address appeared to answer in 0.186s. Any future check of live-endpoint behaviour in this package should assume the same trap. - This makes the non-determinism visible; it does not fix it. Pinning a client needs either a per-client load balancer hostname from the devnet operators or harness support for the HTTP Basic auth the per-node URLs require, since the token path here formats a Bearer header.
assertEPBSProposalMatchesRequest compared execution_payload_included against the requested include_payload for equality, but per the v4 propose-block spec that parameter only governs self-building: a node serving an external builder's bid always returns the payload excluded, regardless of what was asked. The symmetric check rejected that legitimate response as inconsistent, so any caller requesting include_payload=true (required for multi-node, DV and failover setups) could never complete a builder-bid slot. Narrowed the check to the one direction that is actually a fault: a node including a payload that was not requested. A node excluding one is now accepted either way. Updated the doc comments on the check and on EPBSProposalOpts.IncludePayload (both branches) to describe the corrected, asymmetric semantics, since a self-building request can now also come back excluded when an external builder wins the slot. TestEPBSProposalRejectsADisagreeingNode's fixture provoked the guard using the direction the fix legalizes, so it now provokes the remaining invalid direction instead. Added BuilderBidExcludesRequestedPayload, decoding a real wire response to prove the newly-accepted case survives the whole path, not just the check in isolation. Files changed: http/epbsproposal.go, http/epbsproposal_internal_test.go, http/epbsguardwiring_internal_test.go, api/epbsproposalopts.go Verified with gosilent test ./... and a live-devnet run showing no regression versus a HEAD baseline (identical failure set plus the one new passing test); go build, go vet and the repo's lint gate all clean.
ExecutionBlockHash() and its internal helper assertExecutionPayloadPresent() had no DataVersionGloas arm and fell through to ErrUnsupportedVersion, so the accessor errored for every post-Gloas proposal. Added the Gloas arm to both, mirroring the pattern already used by the file's other five fork-dispatch methods (no blinded branch, since there is no blinded proposal post-Gloas). Also adds api/versionedsignedproposal_test.go, which did not exist before this change, covering the success path and every link of the new assertExecutionPayloadPresent nil chain. Files changed: - api/versionedsignedproposal.go - api/versionedsignedproposal_test.go (new) Note for next iteration: assertExecutionPayloadPresent's Fulu-blinded arm has a separate, pre-existing nil-deref (missing .Body/.ExecutionPayloadHeader checks its sibling arms have) — out of scope here, filed as a follow-up.
A proposer needs these three to sign an ePBS proposal, but the versioned wrapper only exposed Slot/RandaoReveal/Value/contents accessors. All three funnel through the existing block() helper so the version, arm-selection and nil-checks can't drift from the other accessors: ParentRoot/StateRoot read the fields directly, BodyRoot takes the HTR of the block body (not the block) since that's what the proposer signature covers.
block()/contents()'s genuinely-unrecognized-version branch returned an ad-hoc "unknown version" string, unlike every other Versioned* wrapper in this package, which returns the shared ErrUnsupportedVersion sentinel for this case. Leaves the rest of the file's descriptive nil-arm errors as is.
verifyAttestationData required data.Index to be 0 for every slot at or after the electra fork. Gloas repurposes that field as a one-bit vote on whether the attester sees the attested block's execution payload in the canonical chain, so the check discarded every payload-FULL answer a conformant node gave and left a validator client unable to attest on those slots, with no way to opt out. At or after GLOAS_FORK_EPOCH the assertion is now the bound that process_attestation itself applies, data.Index < 2. Before the fork nothing changes: the electra-era clamp to 0 is untouched, and an index of 1 at a pre-gloas slot is still an inconsistent result. Decisions: - The fork is answered as a predicate on a slot, isGloasSlot, rather than as the fork's first slot beside the existing calculateElectraSlot. The epoch and whether the node has one at all are only meaningful together, and an unknown fork has no first slot to return, so a caller comparing against a slot without consulting a companion flag would admit every slot rather than none. - A node without GLOAS_FORK_EPOCH in its configuration answers false rather than failing. A client publishes the epoch only once it has one, so treating the missing key the way calculateElectraSlot treats its own would break this endpoint on every node that predates the fork. - The committee_index query parameter is left as it is. beacon-APIs says it SHOULD be omitted from gloas onward, but lighthouse 400s without it, so sending it is the interoperable choice until a node is seen accepting the omission. Verified by a table-driven test that answers from its own httptest server: a node cannot be asked for a payload-FULL vote on demand, since which vote it returns is decided by its own fork choice, and the pre-fork half of the gate cannot be asked of the same node at all. Eight consecutive live requests to a gloas devnet answered 0, which is the same branch every pre-gloas answer takes. Each of the five rows was checked to be load-bearing by mutating the guard and confirming exactly that row failed. Files changed: - http/attestationdata.go: the gloas arm of the index assertion, and isGloasSlot - http/attestationdata_test.go: the synthetic-node regression test Notes for the next iteration: isGloasSlot and calculateElectraSlot each read the cached spec separately, so a pre-gloas verification now takes two cached reads where it took one. Folding them into one read means changing calculateElectraSlot's signature, which the additive-only constraint wants left alone for a saving of nanoseconds on a path with no I/O.
Bez625
reviewed
Aug 4, 2026
| func (i *IndexedAttestation) unpack(indexedAttestationJSON *indexedAttestationJSON) error { | ||
| var err error | ||
| // Spec tests contain indexed attestations with empty attesting indices. | ||
| // if indexedAttestationJSON.AttestingIndices == nil { |
Contributor
There was a problem hiding this comment.
Why is this commented code here?
Contributor
Author
There was a problem hiding this comment.
Good catch. This was disabled because the spec vectors include empty attesting-index lists, which are valid for the Gloas progressive list. The commented code no longer explains a useful decision, so I have removed it.
| type Attestation struct { | ||
| AggregationBits bitfield.Bitlist `ssz-index:"0" ssz-type:"progressive-bitlist"` | ||
| Data *phase0.AttestationData `ssz-index:"1"` | ||
| Signature phase0.BLSSignature `ssz-index:"2" ssz-size:"96"` |
Contributor
There was a problem hiding this comment.
I'm not sure why we need to change Attestation - it seems the only diff between spec.electra.Attestation is the additional metadata around ssz-index and ssz-type.
Can we not apply this to electra?
Contributor
Author
There was a problem hiding this comment.
Yes, this needs to be a separate Gloas type. The Go fields and JSON happen to look the same, but the SSZ schema does not: Gloas makes Attestation a ProgressiveContainer and aggregation_bits a ProgressiveBitlist. That gives it a different hash-tree root, which feeds the aggregate signing root. Changing Electra would break Electra roots; reusing Electra would sign the wrong Gloas root. The ssz-index and ssz-type tags are therefore load-bearing, not just metadata. The spec calls out both changes: https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.12/specs/gloas/beacon-chain.md#attestation
|
|
||
| // Attestation is the Ethereum 2 attestation structure. | ||
| type Attestation struct { | ||
| AggregationBits bitfield.Bitlist `ssz-index:"0" ssz-type:"progressive-bitlist"` |
Contributor
There was a problem hiding this comment.
The ssz-max bound seems to have been removed as part of this. Is that a change to the spec or the bound has been removed?
Contributor
Author
There was a problem hiding this comment.
Yes. This is an intentional spec change, not an omitted check. Gloas defines AggregationBits as ProgressiveBitlist rather than the Electra Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT], so the old 131072 bound is no longer part of the SSZ type. The generated Gloas codec therefore must not retain the Electra ssz-max check. See the Gloas type definition and modified Attestation container: https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.12/specs/gloas/beacon-chain.md#types
Contributor
Author
Gloas inherited-type auditI reviewed the pre-existing containers that needed a Gloas counterpart rather than reuse of an older fork type. Directly changed by the Gloas spec:
Replicated because they contain those changed types:
The direct changes are listed in the official Gloas spec: https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.12/specs/gloas/beacon-chain.md#modified-containers
Remaining confirmation limits: the official |
Add PySpec-derived roots and byte-stable SSZ round trips for inherited Gloas containers and their aggregate, slashing, and block paths.\n\nExercise minimal-preset dynamic decoding alongside the mainnet path.\n\nFiles: spec/gloas/sszroot_test.go\n\nNote: official ssz_static vector validation remains separately tracked.
Contributor
Author
Integration reference — do not mergeThis PR remains the unchanged integration reference for the original Gloas port. Please review and merge only the copied stack, in order:
The copied stack includes one approved compatibility correction: |
AntiD2ta
marked this pull request as draft
August 17, 2026 10:58
This was referenced Aug 28, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Glamsterdam / Gloas support
Additively ports Gloas (Glamsterdam devnet-7) support from the
ethpandaops fork into
attestantio, imports rewritten
ethpandaops→attestantio, additive only.Target spec: consensus
v1.7.0-alpha.12. Base branch:gloas(never master).Two slices accumulate on this branch and ship as one PR: the types layer and
the transport layer. Both are now in.
Slice 1 — types layer
Additively ports the Gloas type surface.
What landed
spec/versionrestructure:DataVersion/BuilderVersionmoved to a leafspec/versionpackage with an alias shim in packagespec(spec/version.go).Added
DataVersionGloas. Heze omitted (out of scope).spec/gloaspackage (92 source files) — types + JSON/YAML/SSZ codecs +ported roundtrip tests. SSZ regenerated with
dynssz-gen, byte-identical to thefork output.
Versioned*wrappers (additive only).VersionedExecutionPayloadEnvelope,VersionedSignedExecutionPayloadEnvelope,VersionedSignedExecutionPayloadBid,VersionedExecutionRequests.api/v1/gloas:SignedExecutionPayloadEnvelopeContents.Key decisions
spec/version.gouses const aliases (preserving prior const semantics) andpreserves
spec.DataVersionFromString(the fork drops it) — additive.ssz-indexstruct tags: they are load-bearing fordynssz-gen(removing them changes the generated SSZ — verified empirically).BaseFeePerGas→BaseFeePerGasLErename;VersionedSignedBeaconBlock.ExecutionRequests()keepsits
*electra.ExecutionRequestsreturn (gloas arm returns an explicit error).//nolint:gocycloon version-switch methods that crossed the complexitythreshold once the gloas case was added (matches existing repo convention); a
scoped
.golangci.ymlline-length exclusion for the gloas type packages (theirePBS
dynsszsize-expression tags exceed 132 chars once gofmt-aligned andcannot be wrapped).
Slice 2 — transport layer (ePBS endpoints, events, client parity)
Additively ports the ePBS transport surface.
What landed
http/):SignedExecutionPayloadEnvelope(GET),SubmitExecutionPayloadBid(POST),SubmitExecutionPayloadEnvelope(POST,stateless Contents form), with
api/opts types and 3 provider interfaces(
ExecutionPayloadProvider,ExecutionPayloadEnvelopeSubmitter,ExecutionPayloadBidSubmitter).execution_payload,execution_payload_available,execution_payload_bid,execution_payload_gossip,fast_confirmation,payload_attestation_message,proposer_preferences); 3 newapi/v1event payload types(
ExecutionPayloadEvent,ExecutionPayloadAvailableEvent,FastConfirmationEvent) + anunmarshalVersionedEventDatahelper that acceptsboth
{version,data}-wrapped and bare payloads.api/v1event round-trip tests, and
TestEventHandlerextended with the 7 new topics(wrapped + bare payloads).
WithCustomSpecSupport(true)(theinterim devnet runs the minimal preset).
Key decisions
spec/allAgnostic*endpoint/opts/interface variants(spec/all not adopted) and all Heze arms (out of scope); the non-agnostic
variants use the versioned wrappers from slice 1.
dynSSZForRequesthelper mirroring the repo's existing inlinedynssz pattern (
GetGlobalDynSszfor mainnet,NewDynSsz(spec)for custompresets) rather than the fork's Service-cached version, which depends on
fields this tree lacks.
live beacon-APIs spec (verified against upstream
master): thesubmit-envelope header is
Eth-Blob-Data-Included: true(the fork used theremoved
Eth-Execution-Payload-Blinded: false), andFastConfirmationEventcarries
current_slot(the fork omitted it; parsed tolerantly).