cl, sentinel: quiet data column sidecar misses - #21686
Conversation
b6c4709 to
40a122f
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the CL sentinel + DAS interaction for data column sidecar downloads by ensuring “misses” return a structured req/resp resource unavailable response (instead of silently closing/resetting streams), and by introducing typed error values so DAS can treat expected misses differently from real failures.
Changes:
- Add
httpreqresptyped errors (HTTPError,PeerResponseError) and sharedResponseCodedefinitions. - Update data column sidecar handlers to emit
ResourceUnavailablePrefixwhen Fulu isn’t active or when no sidecars are available, while preserving real errors if nothing was written. - Update DAS peer download loop to downgrade expected
resource unavailablemisses to trace; add unit test coverage for the new classification helper.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cl/sentinel/service/service.go | Switch to typed req/resp errors and centralized ResponseCode; affects peer error handling on non-success response codes. |
| cl/sentinel/httpreqresp/errors.go | New shared typed error + response code utilities, including snappy error-message decoding. |
| cl/sentinel/handlers/data_cloumn_sidecar.go | Ensure sidecar miss paths return resource unavailable responses and preserve underlying errors when nothing was written. |
| cl/sentinel/handlers/data_column_sidecar_test.go | New tests validating resource unavailable prefix for missing/before-fork cases. |
| cl/das/peer_das.go | Treat resource unavailable peer responses as expected misses (trace) instead of debug-level failures. |
| cl/das/peer_das_test.go | New unit test for isExpectedColumnDownloadMiss. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (c *ConsensusHandlers) dataColumnSidecarsByRangeHandler(s network.Stream) error { | ||
| curEpoch := c.ethClock.GetCurrentEpoch() | ||
| if curEpoch < c.beaconConfig.FuluForkEpoch { | ||
| return nil | ||
| return ssz_snappy.EncodeAndWrite(s, &emptyString{}, ResourceUnavailablePrefix) |
yperbasis
left a comment
yperbasis
left a comment
There was a problem hiding this comment.
A few changes requested:
-
Don't drop peers on code-3 misses. In
requestPeer(service.go), any non-success response code still triggersRemovePeer/ClosePeerwhen at max peer count, so the misses this PR teaches the client to recognize as benign still churn healthy peers (Copilot flagged this too). Please skip removal forResponseCodeResourceUnavailable— otherwise we log the miss at Trace but disconnect the peer anyway. -
By-range:
ResourceUnavailablefor legitimately-empty ranges. Per the Fulu spec, code 3 is for ranges the node is unable to serve withindata_column_serve_range; a range whose blocks carry no blobs, an all-future range, or an all-pre-Fulu range should produce an empty success response. Other clients request columns by range blindly alongside blocks during range sync, so erroring on blobless stretches risks failed-batch handling / downscoring of erigon peers. The all-future and all-pre-Fulu cases are cheaply distinguishable (no storage lookup) and should return empty success; for the remaining blobless-vs-pruned ambiguity, please sanity-check behavior against lighthouse/prysm on a mixed-client devnet before merge. -
Missing success-path test. Nothing pins the key invariant of the new
count/responseErrtail logic: a found sidecar must produce a success chunk with no trailing code-3 chunk. Please add that test. Also consider a wrapped-error case (fmt.Errorf("...: %w", peerErr)) inTestIsExpectedColumnDownloadMissso future wrapping in the call chain can't silently break classification. -
Dead code: the four
ResponseCode*const aliases in service.go are unused (only the type aliases are referenced) — drop them. -
Document the limitation. Peers that reply with spec-canonical empty responses (non-erigon clients, pre-upgrade erigons) still surface as
HTTPError 400 "Read Code: EOF"at Debug, so the flood is only quieted for upgraded erigon peers. Worth a note in the description and a tracking issue for the client-side fix (treating a zero-chunk response as valid empty in httpreqresp).
Nits: errors.go is missing the license header its package-mate server.go has, and the moved ErrorMessage lost the one-line comment explaining the varint-skip-before-snappy decoding — worth keeping.
bc1d1fb to
50868ea
Compare
|
Addressed the review feedback in
Validation:
|
50868ea to
73f7f3f
Compare
|
Follow-up after another adversarial pass: fixed two by-root boundary cases in
Added tests for both empty by-root cases and for the invalid-column path not being reported as resource unavailable. Validation remains:
|
73f7f3f to
52c9360
Compare
|
Second follow-up after another adversarial pass, now in
Revalidated:
|
52c9360 to
d5f535b
Compare
|
Third follow-up after another split adversarial review, now in
Revalidated:
|
d5f535b to
05e3ca9
Compare
|
Fourth follow-up after another split subagent review, now in
Revalidated:
|
05e3ca9 to
5cdf92b
Compare
|
Fifth follow-up after an AGENTS.md-style adversarial review, now in
Risk matrix checked in this pass:
Revalidated:
|
|
Consolidated update for the review feedback: the latest pushed commit is Addressed items:
Adversarial review residuals I intentionally did not broaden in this PR:
Validation:
|
yperbasis
left a comment
yperbasis
left a comment
There was a problem hiding this comment.
Round-1 items are addressed. Remaining:
-
Bare
0x00empty-success is a truncated chunk to other clients. Per the req/resp spec an empty response is zero chunks (zero bytes on the wire); a result byte followed by EOF is a truncated chunk. Our blobs/blocks handlers close with zero bytes for empty results. Lighthouse's outbound codec buffers a lone byte (if src.len() <= 1 { return Ok(None) }inrpc/codec.rs) and doesn't overridedecode_eof, so tokio-util's default turns the leftover byte at stream close intoErr("bytes remaining on stream")→RPCError::IoError→PeerAction::HighToleranceErrorplus a failed request. Every blobless/pruned/backfilling by-range we serve to a Lighthouse peer would fail with a penalty instead of parsing as a clean empty; Prysm's chunk reader errors on the same pattern. Please revert thewriteDataColumnSidecarsEmptySuccesspaths to plainreturn nil(zero-byte close). The by-root code-3 responses are complete chunks and should stay — verified penalty-free in Lighthouse'shandle_rpc_errorforDataColumnsByRoot. -
The #21670 flood is mostly out of scope as written — fix the client side. The flood is
Read Code: EOF, readBytes=0on by-root: peers (mostly non-erigon) sending spec-canonical zero-byte empty responses that our client turns into HTTP 400. Code-3 quieting only helps against upgraded erigon peers. Inhttpreqresp.NewRequestHandler, whenio.ReadFull(stream, code)returnsio.EOFwith 0 bytes read andcommunication.IsMultiChunkProtocol(topic), synthesize success (setREQRESP-RESPONSE-CODE: 0, hand back the empty body).parseResponseDataalready treats an empty body as zero chunks and the DAS loop already skips zero sidecars. Keep erroring for single-chunk protocols and for stream resets (network.ErrReset≠io.EOF). This is coupled with point 1: reverting the0x00alone would reintroduce erigon↔erigon by-range EOF noise. With both, this PR can sayCloses #21670— please add the reference either way. -
Description wording. "Preserve real server/storage/write errors when no response chunk was written" — the code returns
responseErrregardless of whether chunks were written (the wrapper then resets, discarding the partial response). Behavior is fine; fix the wording. Also worth one line that over real gRPC (separate sentinel process)*PeerResponseErroris flattened, so misses stay at Debug there.
Nits:
- License headers missing on
data_column_sidecar_test.goandpeer_das_test.go. - Pre-Fulu by-root: malformed requests get code-3 instead of code-1 (fork gate precedes validation).
ErrorMessage: consider bounding the varint loop (≤10 bytes) and the decoded message (spec capsErrorMessageat 256 bytes) rather than relying on the per-topic cap.- Validation section lists
./cl/sentinel/service, which has no test files.
| // Consume additional rate-limit tokens: slots × columns per slot, capped at config max. | ||
| if cost := min(int(req.Count)*req.Columns.Length(), int(c.beaconConfig.MaxRequestDataColumnSidecars)) - 1; !c.consumeRateLimit(s, cost) { | ||
| return nil | ||
| } |
5cdf92b to
01b086c
Compare
| for i := 0; i < 10; i++ { | ||
| b, err := rawReader.ReadByte() | ||
| if err != nil { | ||
| return "", err | ||
| } |
| func writeDataColumnSidecarsEmptySuccess(s network.Stream) error { | ||
| return nil | ||
| } |
| "github.com/libp2p/go-libp2p/core/network" | ||
| ) | ||
|
|
||
| var errInvalidDataColumnIndex = errors.New("invalid column index") |
yperbasis
left a comment
yperbasis
left a comment
There was a problem hiding this comment.
Approving — prior review items are addressed; targeted tests and golangci-lint pass locally. A few non-blocking nits:
- Before merge: by-root returns code-3 (
ResourceUnavailable) on a miss — verified penalty-free in Lighthouse, but please sanity-check Prysm on a mixed-client devnet, since the downside is peer downscoring during range sync. - Filename typo: the handler is still
data_cloumn_sidecar.gowhile the new test isdata_column_sidecar_test.go; agit mvto fix the spelling would make them match and grep cleanly. - Service-layer coverage: the new "keep peer on code-3" branch in
service.goisn't unit-tested, and the Validation command still lists./cl/sentinel/service, which has no test files — drop it from the list or add the coverage. httpreqresp/server.go: prefererrors.Is(err, io.EOF)overerr == io.EOFfor the synthesized-empty-success check (robust to any reader wrapping).- Cosmetic: the in-loop
GetCurrentStateVersion(epoch) < FuluVersionskip in the by-range handler is now unreachable (startSlot >= fuluStartSlot);ResponseCode.ErrorMessagereturns""for code-1 without reading the body (preserved behavior, just noting).
A "Merge origin/main" resolution on this branch accidentally reverted #21686 ("quiet data column sidecar misses"), deleting cl/sentinel/httpreqresp/errors.go, the data_column_sidecar / peer_das / server tests, isExpectedColumnDownloadMiss, etc. — ~908 lines that are on main. It compiled clean so CI didn't flag it. Restore all eight cl/das + cl/sentinel files to origin/main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Fixes the data column sidecar miss path so expected misses no longer produce noisy download logs, while keeping the wire behavior compatible with peers that signal an empty multi-chunk response by closing without a response code.
Closes #21670
Details
resource unavailableresponses for by-root misses and valid pre-Fulu by-root requests, while validating malformed pre-Fulu by-root requests first so they returninvalid request.io.EOFbefore any response-code byte. Single-chunk EOFs, partial reads, and stream errors still surface as HTTP 400.responseErrpropagation for real storage/write errors. Across the Sentinel HTTP/gRPC boundary those transport failures can still be flattened by gRPC, while structured peer response codes are surfaced as typedPeerResponseErrorvalues.Validation
go test ./cl/sentinel/httpreqresp ./cl/sentinel/service ./cl/sentinel/handlers ./cl/das -count=1make lint