erigontech / erigontech/erigon

cl/clparams: ForkSchemaMatchesSlot only discriminates the Gloas boundary despite its general name

Open
#22,820 1 comment 0 reactions 1 assignee Claimed by @domiwei View on GitHub
Caplin Glamsterdam tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

## Problem

`ForkSchemaMatchesSlot`, introduced by #22797 (not yet merged) in `cl/clparams/config.go`, is:

```go
func (b *BeaconChainConfig) ForkSchemaMatchesSlot(slot uint64, decodedVersion StateVersion) bool {
slotIsGloas := b.GetCurrentStateVersion(slot/b.SlotsPerEpoch) >= GloasVersion
return slotIsGloas == (decodedVersion >= GloasVersion)
}
```

It compares two booleans, so it answers exactly one question: do the slot-implied fork and the decoded schema agree about being Gloas-or-later. That is the right scope today — Gloas is the only boundary across which `BeaconBody` and `DataColumnSidecar` change which fields they carry — and the doc comment says as much.

The name does not. At both call sites it reads as a general check that the schema matches the slot:

```go
if !f.beaconCfg.ForkSchemaMatchesSlot(block.Block.Slot, block.Version()) {
return ErrForkSchemaSlotMismatch
}
```

If a fork after Gloas moves fields between these schemas again, the predicate keeps returning `true` for the new mismatch, and both call sites keep looking protected while no longer being so. Whoever adds that fork has no reason to look inside a function whose name already claims to cover them. With Glamsterdam in progress this is not hypothetical.

## Options

- Rename it to state the boundary it actually checks, so the next fork's author can see it does not cover them.
- Make it exact instead: `GetCurrentStateVersion(slot/SlotsPerEpoch) == decodedVersion`. A req/resp server encodes each object with the fork digest of that object's own slot, so honest traffic should satisfy equality. The risk is Erigon-internal callers that build objects with a locally chosen version, which would need auditing first — which is why #22797 did not start here.
- Keep the predicate and add a test that fails when a fork later than Gloas enters the schedule, so the question gets re-asked rather than silently answered.

Any of the three is fine; the thing to avoid is leaving a general-sounding name in front of a Gloas-only check.

## Note

Since #22797 is still open, this may be simpler to settle in the PR than to carry as a follow-up — close this if it is handled there.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.