Submit attester slashings via go-eth2-client's versioned submitter - #229
Submit attester slashings via go-eth2-client's versioned submitter#229parithosh wants to merge 4 commits into
Conversation
f7c8ca6 to
4544769
Compare
The V1 pool endpoint (/eth/v1/beacon/pool/attester_slashings) was deprecated in the Electra release of the beacon APIs and removed from the spec (ethereum/beacon-APIs#549). generate_slashings still used it, and the legacy fallback only fired on a 404 and hardcoded Eth-Consensus-Version: electra even on fulu/gloas networks. - add BeaconClient.SubmitAttesterSlashingV2: V2 primary path with the caller-supplied consensus version, falling back to the legacy V1 submission on 404 (pre-Electra nodes) - generate_slashings now submits via V2 with the version derived from the chain spec (new ChainSpec.ConsensusForkVersionAtSlot helper) - simplify the legacy SubmitAttesterSlashing to the plain V1 POST; its nested v2 fallback with the hardcoded electra header is superseded
4544769 to
0a4c843
Compare
Follow-up to the V2 submission change: drop assertoor's hand-rolled V2 POST and route the submission through go-eth2-client's AttesterSlashingSubmitterV2, merged in ethpandaops/go-eth2-client#46. - BeaconClient.SubmitAttesterSlashing now takes a spec.VersionedAttesterSlashing and delegates to the library, matching the shape of SubmitBLSToExecutionChanges / SubmitVoluntaryExits. The library derives Eth-Consensus-Version from the slashing's version, so the header can no longer drift from the payload. Both the bespoke V2 POST and the legacy V1 helper are gone. - ChainSpec.ConsensusForkVersionAtSlot becomes DataVersionAtSlot and returns a spec.DataVersion, which selects the versioned arm and drives the header in one value instead of a bare header string. - generate_slashings wraps the generated phase0 slashing in the arm matching the fork at the slot it was built for; electra/fulu need electra.AttesterSlashing and gloas needs gloas.AttesterSlashing, since EIP-7549 and the progressive-list migration each gave IndexedAttestation its own Go type.
|
Reworked on top of the merged ethpandaops/go-eth2-client#46 ( Two consequences worth calling out explicitly:
Dependency is pinned to the master pseudo-version Test coverage is now end-to-end rather than unit-only: Separately: the root cause of that whole class of bug is that go-eth2-client has no compile-time conformance assertions anywhere in the module. I've opened that as its own PR upstream against attestantio. |
Review follow-up. DataVersionAtSlot floored at electra, so on a chain that schedules electra at a later epoch a pre-electra slot produced an electra-shaped slashing and an Eth-Consensus-Version: electra POST to a v2 pool endpoint the node does not serve. The resulting HTTP error is easy to misdiagnose as a client bug rather than an unsupported fork. DataVersionAtSlot now returns (spec.DataVersion, error) and reports ErrPreElectraFork with the current and activation epochs when electra is positively scheduled at a later epoch. The floor stays for the ambiguous case: a fork epoch of 0 means "not scheduled" under this ChainSpec's convention, so a genesis-activated electra cannot be told apart from one an older node never reported, and treating that as pre-electra would break every genesis-electra devnet. Also fold the duplicated fork-activation arithmetic into isForkActive, which IsGloasActive now uses too; it divides instead of multiplying, so a FAR_FUTURE fork epoch cannot overflow the comparison.
|
Thanks — taking the suggestion. Pushed On the 🟡: deliberate, but you're right that the failure mode was the bad part. I could only make that positive in one direction, and it's worth being explicit about why. Under this On the playbooks: the fork-generic consumers running Two incidental things in the same commit:
Build, vet, gofmt, full test suite and |
Review follow-up, net -83 lines. DataVersionAtSlot had three different activation checks in one function and only named electra, fulu and gloas. It is now a single ladder over isForkActive covering every fork spec.DataVersion knows, phase0 through heze, and IsGloasActive is the same helper. That ladder needs a fork epoch of 0 to mean genesis rather than "not scheduled", which is the reading the old > 0 guards used. The two cases were conflated because both arrive as the Go zero value: a beacon node reports FAR_FUTURE_EPOCH for a known-but-unscheduled fork and omits the key entirely for a fork it predates. NewChainSpec now seeds every fork epoch with FarFutureEpoch before smapping fills it, so an omitted key stays unscheduled and a reported 0 keeps its literal meaning. Without that seeding a node that has never heard of gloas reports GLOAS at genesis, which TestSetClientSpecsForkDefaults pins. Also add HEZE_FORK_EPOCH so the ladder covers the full DataVersion range. generate_slashings now builds an all.AttesterSlashing and calls ToVersioned instead of rewrapping phase0 types into electra/gloas ones by hand: no per-fork switch, and future forks need no change here. That removes versionedAttesterSlashing and its four converters. The pre-electra guard moves to the submitter as a single ordered comparison, since it is a property of the versioned pool endpoint rather than of any one fork, and it is likewise fork-independent.
|
Both fair —
The three activation checks: guilty. One thing that had to change for the uniform ladder to be safe, and it's the part worth your eyes:
A uniform ladder needs So Knock-on: Pre-electra guard (from the earlier round) is no longer a per-fork thing either — it's one ordered Tests consolidated to where the behaviour now lives: Build, vet, gofmt, full suite and |
Summary
Submit attester slashings through the beacon-APIs V2 pool endpoint, using go-eth2-client's
AttesterSlashingSubmitterV2rather than a bespoke HTTP call.BeaconClient.SubmitAttesterSlashingnow takes a*spec.VersionedAttesterSlashingand delegates tobc.clientSvc.(eth2client.AttesterSlashingSubmitterV2), in the same shape as the neighbouringSubmitBLSToExecutionChanges/SubmitVoluntaryExits. Both the hand-rolled V2 POST and the legacy V1 helper are deleted; assertoor no longer builds this request itself.ChainSpec.ConsensusForkVersionAtSlotbecomesChainSpec.DataVersionAtSlotand returns aspec.DataVersioninstead of a bare header string. One value now selects the versioned arm and drives theEth-Consensus-Versionheader, so the two cannot disagree.generate_slashingsbuilds anall.AttesterSlashingand callsToVersioned(), so the fork-specific types stay inside go-eth2-client and a future fork needs no change here.go-eth2-clientbumped tov0.1.7-0.20260804142719-11c20aff398e(master, the merge commit of ethpandaops/go-eth2-client#46). Happy to re-pin to a release tag once one is cut.Motivation
The V1 pool endpoint was deprecated in the electra release of the beacon APIs and removed from the spec (ethereum/beacon-APIs#549); Prysm already drops it. On fulu/gloas devnets the old code therefore either hit a removed endpoint or sent the wrong consensus-version header.
The original version of this PR fixed that with its own V2 submission function. Now that ethpandaops/go-eth2-client#46 is merged, the library owns the endpoint, the header derivation and the fork-specific marshalling, and assertoor should not keep a second implementation of the same request.
bc.clientSvcis already ago-eth2-client/http.Service, so this is a delegation, not a new dependency.Behavioural change: the V1 fallback is gone
The earlier revision fell back to a V1 POST on any V2 failure. That is dropped, deliberately:
electra/gloasAttesterSlashing, so retrying it against the V1 endpoint sends a post-electra body to a pre-electra endpoint.Pre-electra chains fail on the fork, not on the endpoint
The versioned pool endpoint only exists from electra. Rather than letting a pre-electra chain produce an opaque 404/400,
SubmitAttesterSlashingrejects the submission up front withErrPreElectraSlashing. It is one ordered comparison onspec.DataVersion, not a per-fork list, and it lives on the submitter because it is a property of the endpoint rather than of any task.playbooks/stable/validator-lifecycle-test-v2.yamlandplaybooks/stable/kurtosis/validator-slashing-test.yamlare the fork-generic consumers that runslashingType: attester; both now get a named fork error instead of an opaque pool-endpoint failure if pointed at a pre-electra chain.Fork resolution
DataVersionAtSlotis a single ladder over oneisForkActivehelper covering every forkspec.DataVersionknows, phase0 through heze;IsGloasActiveis now the same helper.HEZE_FORK_EPOCHis added toChainSpecso the ladder spans the full range.That ladder needs a fork epoch of
0to mean genesis rather than "not scheduled". The two were conflated because both arrive as the Go zero value: a beacon node reportsFAR_FUTURE_EPOCHfor a known-but-unscheduled fork and omits the key entirely for a fork it predates.NewChainSpecnow seeds every fork epoch withFarFutureEpochbeforesmappingfills it, so an omitted key stays unscheduled and a reported0keeps its literal meaning.This is load-bearing, not cosmetic: without the seeding, a node that has never heard of gloas reports
GLOAS_FORK_EPOCHabsent, and a uniform ladder would then resolve every slot to gloas.Scope
Still a spec-correctness fix for the submission path. It does not claim to change slashing-inclusion behaviour on any specific CL client: how a node parses the JSON body server-side is a separate, server-side question.
Unchanged and intentionally out of scope:
beaconapi.go:573still hardcodesEth-Consensus-Version: electrainSubmitAttestations(the v2 attestations endpoint used bygenerate_attestations). Same defect class;DataVersionAtSlotis the same fix, but it wants its own PR, and go-eth2-client'sSubmitAttestationswould be the right thing to delegate to there too.Test plan
go build ./...,go vet ./...,gofmt -l .clean; fullgo test ./...passesgolangci-lint runon the touched packages reports no new findings (two pre-existinggoconsthits on"head"inbeaconapi.go/beaconstream.goare unchanged from master)rpc.TestSubmitAttesterSlashing— hermetichttptestend-to-end through the realgo-eth2-client/http.Service, as a table over electra/fulu/gloas plus a pre-electra case. Asserts the v2 pool path, thatEth-Consensus-Versiontracks the fork rather than a constant, and that the body carries a non-null fork-specificattestation_1/attestation_2(an arm/version mismatch marshals tonullunder a valid header). The pre-electra case assertsErrPreElectraSlashingand that no request reaches the node. Inputs are built throughall.AttesterSlashing.ToVersioned()exactly as the task does, so the construction is covered too. Verified to fail if the guard is replaced withif false.consensus.TestDataVersionAtSlot— every rung of the ladder and its boundary, plus genesis-activated forks and unscheduled ones.consensus.TestSetClientSpecsForkDefaults— the load-bearing case: a spec response that omitsFULU/GLOAS/HEZEleaves them unscheduled while a reported0stays genesis. Verified to fail if the seeding is dropped, with the exact symptom it prevents:DataVersionAtSlot = fulu, want electra.