cl: complete errcheck rollout across cl package and fix violations - #23577
Conversation
…nsition, fix review findings Closes the gap the interface change opened: cl/transition could still silently discard the persistence-hook errors now returned by abstract.BeaconState setters. Also fixes partial-mutation bugs in SetSlot/ResetEpochParticipation on hook failure, and replaces a panic on disk-write failure in fork_graph.NewForkGraphDisk with a returned error routed through the existing startup error path.
- solid: bound-check memberIndexInCommittee in ToAttestation; the dev validator feeds it straight from a beacon-API duties response, so an out-of-range value panicked one line below the new committee-index check. - raw/setters: mark ValidatorsLeafIndex after the event hook, not before. The new early return on hook error left the leaf dirty for a write that never landed, forcing a full validators-subtree re-hash. - historical_states_reader: propagate SetSlot/SetCurrentSyncCommittee/ SetNextSyncCommittee errors instead of discarding them. - forkchoice: propagate the versionedHashes RangeErr; a truncated list made the EL blob check pass and skipped data availability. - state: panic in New if InitBeaconState fails instead of ignoring it. - raw: drop init's unused error return; the two `_ =` swallows go with it. - synced_data, handler: no require from a goroutine holding the manager mutex, and bound the ViewHeadState retry loop. - state: hoist require.NoError out of b.Loop in the root and shuffling benchmarks. - upgrade_test: key the balance hook on the QueueExcessActiveBalance write so the test cannot pass via the earlier zeroing loop. - services, devvalidator: raise the pending-envelope failure to Warn and log the aggregate-build inputs.
…rigon into linter/enable-errcheck-cl
…fe require.NoError
- ssz_snappy.EncodeAndWrite discarded both flush errors, so a send that never reached the peer still returned nil; the buffered writes it now checks almost never fail on their own. - ProcessPendingConsolidations resolved the target balance only while crediting it, so a bad target index drained the source and credited nothing. Resolve it before touching the source. - sentinel_requests_test compares the read error with errors.Is, like the other handlers' tests.
… block - runDownload never returned anything but nil, so both new callers' error branches were dead. Drop the return value instead of pretending. - getFlagsTotalBalances' worker panicked on a bad validator index, so the error return added for it could never fire. Return the error. - httpreqresp logs a failed SetDeadline like cl/sentinel/handlers does, instead of a 400 that costs the peer its connection over a stream reset. - handlers.go keeps only SetDeadline; it already covers both directions. - Extract dropPeer/closePeer: the RemovePeer/RemovePeer/ClosePeer block was copied nine times across service.go and discovery.go. - GetUnslashedIndiciesSet ran a one-worker ParallellForLoop whose error was discarded; a plain loop is the same thing without the ignored error. - Bench loops use b.Fatal, not testify, so the recorded ns/op stay comparable.
There was a problem hiding this comment.
Pull request overview
Completes the errcheck rollout across the cl/* tree by converting previously ignored errors into explicit handling/propagation, and updates .golangci.yml to stop excluding cl/* from errcheck. This touches consensus state transition logic, CL networking, persistence, and a broad set of tests/benchmarks to keep the package tree lint-clean.
Changes:
- Removed
errcheckexclusions forcl/*and updated CL code paths to return, log, or assert on errors rather than discarding them. - Propagated worker-pool execution errors and made some state transition helpers return errors (e.g., pending consolidations).
- Updated many tests/benchmarks to check new/previously ignored error returns.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cl/utils/bls/cache_test.go | Benchmarks/tests now check cache-load and aggregate-verify errors. |
| cl/transition/impl/eth2/statechange/process_sync_committee_update_test.go | Test updated to assert state mutation calls succeed. |
| cl/transition/impl/eth2/statechange/process_rewards_and_penalties.go | Worker-pool execution errors are now propagated to callers. |
| cl/transition/impl/eth2/statechange/process_pending_consolidations.go | Function now returns error and checks balance mutations. |
| cl/transition/impl/eth2/statechange/process_epoch.go | Epoch processing now handles consolidation errors and avoids unchecked parallel loop return. |
| cl/transition/impl/eth2/statechange/process_epoch_test.go | Test harness simplified to pass error-returning functions directly. |
| cl/transition/impl/eth2/statechange/finalization_and_justification.go | Worker-pool execution errors are now returned instead of dropped. |
| cl/transition/impl/eth2/operations_bls_change_test.go | Test now asserts validator addition succeeds. |
| cl/spectest/consensus_tests/epoch_processing.go | Spectest wiring updated to use error-returning functions directly. |
| cl/sentinel/service/service.go | ClosePeer errors are now logged instead of ignored. |
| cl/sentinel/sentinel_requests_test.go | Tests now handle ReadByte errors (including EOF) explicitly. |
| cl/sentinel/httpreqresp/server.go | Deadline-set errors are now checked and surfaced via HTTP error responses. |
| cl/sentinel/handlers/rate_limiter_integration_test.go | Integration tests now assert stream deadlines are set successfully. |
| cl/sentinel/handlers/light_client_test.go | Tests now check stream reads and tolerate EOF explicitly. |
| cl/sentinel/handlers/heartbeats.go | Explicitly discards (documents) a best-effort send error. |
| cl/sentinel/handlers/handlers.go | Deadline-set errors on streams are now checked and logged. |
| cl/sentinel/handlers/blocks_by_root_test.go | Test now checks tx.Commit and stream read errors. |
| cl/sentinel/handlers/blocks_by_range_test.go | Test now checks stream read errors. |
| cl/sentinel/handlers/blobs_test.go | Test now checks stream read errors. |
| cl/sentinel/discovery.go | ClosePeer errors are now logged instead of ignored during pruning/connection handling. |
| cl/sentinel/communication/ssz_snappy/encoding.go | EncodeAndWrite now checks buffered writes and flush errors to avoid silent short writes. |
| cl/rpc/rpc.go | BanPeer RPC call errors are now logged instead of ignored. |
| cl/persistence/state/validator_events_test.go | Test now asserts ReplayEvents returns nil. |
| cl/persistence/state/historical_states_reader/historical_states_reader_test.go | Test now asserts OnHeadState succeeds. |
| cl/persistence/state/historical_states_reader/gloas_roundtrip_test.go | Tests now assert BitVector SetBitAt errors are handled. |
| cl/persistence/blob_storage/bucket_store.go | Temp-file cleanup now checks/remediates remove errors after write failures. |
| cl/p2p/p2p_discovery.go | Peer connect/close failures are now logged instead of ignored. |
| cl/merkle_tree/merkle_tree_test.go | Test helpers now assert MerkleRoot computations succeed. |
| cl/das/peer_das.go | Cleanup/scheduling paths now log/handle errors instead of dropping them. |
| cl/antiquary/state_prune_test.go | Test now asserts OnHeadState succeeds. |
| cl/antiquary/state_prune_reader_test.go | Test now asserts OnHeadState succeeds. |
| cl/antiquary/state_antiquary_test.go | Tests now assert OnHeadState succeeds. |
| cl/antiquary/antiquary_test.go | Test now asserts etl.Collector Load succeeds. |
| cl/aggregation/pool_test.go | Tests now check bitvector set errors and (mostly) AddAttestation errors. |
| .golangci.yml | Removes cl/* from the errcheck exclusion bootstrap list. |
Suppressed comments (1)
cl/utils/bls/cache_test.go:91
- Using testify/require inside the benchmark hot loop adds non-trivial overhead and can skew the measured performance. Prefer a simple branch with b.Fatal on error.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if applyErr != nil { | ||
| return applyErr | ||
| } |
There was a problem hiding this comment.
Fixed. IncreaseBalance failing now restores the source balance to its pre-debit value, so a failed move cannot destroy balance. (SetValidatorBalance runs its antiquary hook before mutating, so the failed credit itself leaves nothing behind.) The wider point — earlier consolidations in the same Range are already applied when a later one fails — is inherent to the epoch transition, which is not atomic; the caller discards the state on error.
| // Some testcases intentionally add a subset attestation | ||
| // (e.g. "skip att1_1"), which AddAttestation rejects with | ||
| // ErrIsSuperset by design; only the final merged state matters here. | ||
| _ = pool.AddAttestation(tc.atts[i]) |
There was a problem hiding this comment.
Fixed. The loop now allows only ErrIsSuperset; any other AddAttestation error fails the test.
|
Reviewed and pushed the fixes directly to the branch (two commits). Summary of what the review turned up. Fixed: the error checks that could never fireThe errcheck pass added
Fixed: errors that were still being swallowed
Fixed: behaviour and duplication
Test-side
Not changed — worth a decision
Verified after the changes: |
|
|
||
| responsePacket = append(responsePacket, responseChunk) | ||
| if _, err := r.ReadByte(); err != nil && err != io.EOF { | ||
| if _, err := r.ReadByte(); err != nil && !errors.Is(err, io.EOF) { |
There was a problem hiding this comment.
Do not convert this sentinel check to errors.Is.
Using errors.Is(err, rlp.EOL) (or errors.Is(err, io.EOF)) causes severe decoding bugs.
errors.Is unwraps the error chain. If a nested element decoder encounters malformed or truncated input and returns a wrapped error (e.g. fmt.Errorf("decode field: %w", io.EOF)), errors.Is unwraps it, matches EOL/EOF, and incorrectly treats the data corruption as a clean end of list, swallowing the error and returning nil.
The bare identity check (err == rlp.EOL with //nolint:errorlint) must be preserved so that wrapped decoding errors propagate properly rather than being swallowed.
There was a problem hiding this comment.
Reverted — both lines are back to err != io.EOF. It was a consistency change, not a correctness one, so your call on your PR.
Two things for the record, though, because the stated rationale does not hold at this call site.
r here is bytes.NewReader(w.Bytes()), and (*bytes.Reader).ReadByte has exactly two outcomes:
func (r *Reader) ReadByte() (byte, error) {
r.prevRune = -1
if r.i >= int64(len(r.s)) {
return 0, io.EOF
}
b := r.s[r.i]
r.i++
return b, nil
}Bare io.EOF or nil. There is no underlying reader, no nested element decoder, and nothing that could wrap. err != io.EOF and errors.Is(err, io.EOF) are provably identical here, so neither form can swallow a corruption error. (And there is no rlp.EOL in this file — the RLP stream-decoder hazard you describe is real, but it is a different package and a different failure mode.)
Second: this PR makes the opposite choice four times, in blobs_test.go, blocks_by_range_test.go, blocks_by_root_test.go and light_client_test.go, all switched to errors.Is(err, io.EOF). Those read from a libp2p network.Stream obtained via host1.NewStream(...), which can return wrapped errors — so if the unwrapping hazard applies anywhere in this diff, it applies there and not here. Worth picking one form for all five rather than leaving them split.
For what it is worth, errorlint is enabled in .golangci.yml but does not flag the bare comparison — I checked with golangci-lint run --enable-only errorlint ./cl/sentinel/..., which is clean either way. So no //nolint is needed.
…Attestation SetValidatorBalance can fail in its antiquary hook, so a target credit can fail after the source debit landed. Restore the source instead of returning with the balance destroyed. The aggregation pool test now allows only ErrIsSuperset, so an unrelated AddAttestation failure fails the test instead of being masked.
Reverts my errors.Is change at the author's request. The two forms are equivalent here, and errorlint does not ask for either.
| n, err := io.ReadFull(stream, code) | ||
| synthesizedEmptySuccess := false | ||
| if errors.Is(err, io.EOF) && n == 0 && communication.IsMultiChunkProtocol(topic) { | ||
| if err == io.EOF && n == 0 && communication.IsMultiChunkProtocol(topic) { //nolint:errorlint // intentional bare sentinel check |
There was a problem hiding this comment.
This reverts #23303. Commit 6217798d87e ("all: resolve errorlint linter findings") changed this line from err == io.EOF to errors.Is(err, io.EOF); the //nolint:errorlint puts it back.
Production p2p read path, and the PR body doesn't mention it. If the bare sentinel is right here, the reasoning belongs in the description — an errcheck rollout silently undoing a merged errorlint decision is how the next errorlint pass re-flips it.
There was a problem hiding this comment.
Confirmed - reverts #23303, which mechanically converted every bare ==/!= error check repo-wide to errors.Is with no per-site analysis.
Checked the actual Read internals of both muxers this repo negotiates - go-yamux/v5 and go-libp2p-mplex - and neither wraps io.EOF; both return the bare sentinel. So at this line the two forms behave identically today.
Kept bare anyway: that equivalence relies on an unenforced assumption about third-party muxer internals that a dependency bump or a later refactor could silently break, with no compiler or lint signal. The bare form has no such fragility, and it matches the err == rlp.EOL / //nolint:errorlint convention already used for this exact class of check in execution/types/ and p2p/enr/enr.go.
| } | ||
| if applyErr = state.IncreaseBalance(s, c.TargetIndex, sourceEffectiveBalance); applyErr != nil { | ||
| // Put the source back: a half-applied move destroys balance. | ||
| if err := s.SetValidatorBalance(int(c.SourceIndex), vBalance); err != nil { |
There was a problem hiding this comment.
This rollback branch has no test. The only coverage for ProcessPendingConsolidations is the spectest vector PendingConsolidationTest (cl/spectest/consensus_tests/epoch_processing.go:114), which never makes state.IncreaseBalance fail, so the restore never executes.
It is the one path here where getting it wrong destroys balance, and it was added in review rather than by the rollout itself. A unit test with a mutator that fails IncreaseBalance would pin it.
| if err := sw.Flush(); err != nil { | ||
| return err | ||
| } | ||
| return wr.Flush() |
There was a problem hiding this comment.
Removing the two defers here makes cl/persistence/blob_storage/bucket_store.go:130 false. It still says "EncodeAndWrite flushes in a defer and discards that error, so a short write is only observable on the writer it was handed", which is the stated justification for the errWriter shim on the line below it.
Both flush errors now reach the caller, so w.err != nil at bucket_store.go:136 is only reachable when EncodeAndWrite already returned the same error at 133. Worth fixing the comment and dropping the shim here, since this is the change that invalidated it.
| } | ||
|
|
||
| // pruneExcessPeers disconnects excess peers while ensuring no subnet becomes empty | ||
| func (s *Sentinel) closePeer(pid peer.ID) { |
There was a problem hiding this comment.
This was inserted between pruneExcessPeers' doc comment and pruneExcessPeers, so Go now attaches "disconnects excess peers while ensuring no subnet becomes empty" to closePeer — a three-line wrapper that knows nothing about subnets. pruneExcessPeers at 312 has no doc at all.
Move the comment down to 312, or give closePeer its own.
Completes the errcheck rollout for #22538 across the entire
cl/*tree. Started ascl/beacon,cl/cltypes,cl/phase1,cl/validator; this PR adds every remainingcl/*package:cl/aggregation,cl/antiquary,cl/das,cl/merkle_tree,cl/p2p,cl/persistence/blob_storage,cl/persistence/state(+historical_states_reader),cl/rpc,cl/sentinel(+communication/ssz_snappy,handlers,httpreqresp,service),cl/spectest/consensus_tests,cl/transition/impl/eth2(+statechange),cl/utils/bls. Only non-cl/* packages remain excluded for future PRs.Notable fixes beyond lint-satisfying:
errors.Is/bare-io.EOFdistinction on network streams vsbytes.Reader, thedropPeer/closePeerdedup (9x repeated block), two error returns that could never fire (runDownload, the one-workerParallellForLoop), and theAddAttestationtest now only toleratingErrIsSupersetspecifically instead of masking anything.Everything else is peer/cleanup operations logging on failure instead of discarding silently, or straightforward test fixes.