eth, eth/fetcher: bound witness page count by the gas-derived ceiling instead of a cross-peer vote - #2417
Merged
Conversation
… instead of a cross-peer vote The witness page-count verification polled random peers for a majority page count and, on disagreement, dropped and jailed the reporting peer. Witness page counts are non-deterministic across honest nodes — a valid witness can round to one more 15 MiB page on one node than on another — so the vote both false-positively disconnected and jailed honest peers whose witness was one page larger than the sampled majority, and failed open when the sampled peers did not have the witness. Replace the cross-peer vote with a local bound: a page count within the gas-derived threshold (which already carries large headroom over the real per-block witness size) is accepted; a count above it is larger than the block gas limit can plausibly produce, so it is refused for that peer — the fetch simply tries another peer — without disconnecting or jailing. - CheckWitnessPageCount no longer takes peer-query closures and no longer votes or jails; it accepts <= threshold and refuses above. - Remove verifyWitnessPageCountSync, getConsensusPageCountWithOriginal, the witnessManager jail hook, the BlockFetcher jailPeer plumbing that only fed it (down to the NewBlockFetcher signature), and the now-unused verification meters and constants. Structural per-response page checks (invalid page number, inconsistent TotalPages within a single peer's stream) are unchanged.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2.10.2-candidate #2417 +/- ##
====================================================
Coverage ? 55.84%
====================================================
Files ? 922
Lines ? 167589
Branches ? 0
====================================================
Hits ? 93590
Misses ? 68533
Partials ? 5466
🚀 New features to boost your workflow:
|
lucca30
marked this pull request as ready for review
September 16, 2026 12:16
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
Author
|
@claude review |
Contributor
|
Reviewed the diff and traced the refusal path end-to-end. The direction is right (the cross-peer vote misclassifies honest ±1-page rounding as lying), but the "refuse without disconnecting or jailing" semantics don't hold yet, because the caller in What happens on In
Before this PR that was masked: Repro Mock peer via the existing
So the observed behavior is "download the whole thing from the peer we just refused, then jail it for an unrelated reason", and sometimes import it. Suggested fix
Otherwise: builds, vets, and |
9 tasks
CheckWitnessPageCount now refuses an oversized page count without dropping or jailing, but receiveWitnessPage still surfaced the refusal as an error. The call site discards that error, and the deferred retry handler then marked page 0 as retryable and rebuilt requests from witTotalPages — which the same page had just populated — so every remaining page of the refused witness was requested from the peer we had just refused, page 0 was retried, the duplicate tripped the "more pages than TotalPages" violation and jailed the peer for an unrelated reason, and the reconstructed witness could still be delivered. Before the vote was removed this was masked by the synchronous peer drop. - On refusal: pause the hash, discard its pages, log, and continue — never return an error, so the retry path cannot re-queue the refused download. - Discard any page arriving for an already-paused hash instead of accumulating it (late in-flight pages could otherwise complete a refused witness). - buildWitnessRequests takes downloadPaused and skips paused hashes for both fresh pages and retries. - Bound the page count before storing the page, and take the inconsistent- TotalPages pause under mapsMu like the other writers. - Replace the stale "dropping peer" log and "trigger peer drop" comment. Tests: TestRequestWitnessesWithVerification_RefusedPageCountStopsDownload drives RequestWitnessesWithVerification end-to-end and asserts only page 0 is requested, the jail callback never fires, and the request resolves empty (fails against the previous behaviour with pages 0,1,2,3,0 requested); TestBuildWitnessRequests_SkipsPausedHash pins the builder. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Brme9KQBd7fZBMnMVEhAZU
lucca30
added a commit
that referenced
this pull request
Sep 18, 2026
#2417 changes the NewBlockFetcher call directly above the WIT2 striker wiring; editing the adjacent comment here made the two branches conflict when merged together even though each merges cleanly into the candidate. Leave the striker comment as it was and document the size-oracle extension (import-failure strike + source exclusion) in its own block below. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Brme9KQBd7fZBMnMVEhAZU
lucca30
added a commit
that referenced
this pull request
Sep 22, 2026
…itnesses within a signed size band (#2416) * eth/protocols/wit, eth: WIT2 size oracle — accept non-deterministic witnesses within a signed size band Witnesses are not deterministic across nodes: BlockSTM speculative reads make honest nodes collect different-but-valid trie-node sets, so a valid witness routinely hashes differently from the BP-signed WitnessHash. WIT2's fetch-time verifyAgainstSignedHash treated any hash divergence as a fault (reject the bytes, strike the serving peer, fall back to WIT1 after two distinct servers), so a valid witness that hashes differently is rejected and its serving peer struck. Replace exact-hash equality with a size oracle: - Sign the witness size. SignedWitnessAnnouncement gains a WitnessSize field and the announce signing pre-image commits to it (keccak(domain || blockHash || blockNumber || witnessHash || witnessSize)). Producers set it from their own witness length. - Accept within a band for import. verifyAgainstSignedHash accepts for import any witness whose encoded size is <= min(3*signedSize, gas-derived absolute ceiling), regardless of hash; content-correctness is still arbitrated by import-time state-root execution. A within-band witness is re-served/relayed only when byte-identical to the BP's (hash match); a valid non-deterministic variant imports locally but is not re-served, so the signed hash stays a faithful identifier of the bytes on the serving/relay fast-path. Blame for content rests with the producer that signed the announcement, not a relaying or serving peer — preserving WIT2's property of relaying a trusted witness before self-validating. - Bound the size. A witness beyond the band is rejected and the serving peer struck (first occurrence per (peer, block), reusing the distinct-server bookkeeping); distinct servers exceeding the band for the same block fall back to WIT1. The retained gas-derived ceiling keeps the accepted size bounded even when the signed size is implausibly large. No WIT2 is deployed yet, so the signed-announce format changes in place. Scope: WIT2 signed-path only. The WIT1 page-count cross-peer verification is a separate change, handled in a follow-up. * eth/fetcher: fix stale byte-correctness comment on witness verification The comment above verifyAgainstSignedHash still described the pre-oracle byte-correctness invariant (hash must match). This PR's size-oracle rework now accepts hash divergence within the signed-size band, so the comment misdescribed the actual trust boundary an incident responder would rely on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * eth, eth/fetcher: charge size-oracle import failures, bound the signed size, relax the broadcast gates Review follow-ups for the WIT2 size oracle. Import-time consequence. A witness accepted on the size oracle alone (hash differs from the BP-signed one) that then fails import was logged at debug and forgotten, so a peer relaying the valid BP announce could serve up to the size band in unusable bytes per block at no cost — the WIT1-class weakness WIT2 was closing. Now the witness manager records the serving peer, the divergence and the fetch closure on the import op (verifyAgainstSignedHash returns diverged; enqueueOp keeps the op instead of rebuilding it from parts), and on insertChain failure importBlocks strikes the server, excludes it as a witness source for that block (new SetWitnessSourceExcluder hook → handler witnessSourceExclusions, honoured by resolveWitnessFetchPeer at every tier and released on import), and hands the block back to the witness manager for a re-fetch from another peer (new witnessRetry loop case → retryAfterImportFailure), bounded by maxWitnessImportRetries. A BP-identical witness that fails import is still the BP's fault and is handled as before. Signed size sanity. signedSize*3 could wrap for a hostile size and a zero WitnessSize yielded a zero ceiling; both struck honest servers. The band now saturates and a zero size falls back to the absolute cap, and acceptSignedAnnouncement refuses (and strikes the sender for) a WitnessSize of zero or above the gas-derived absolute cap before deferral, so the announcer is the one charged. Broadcast gates. acceptSignedBroadcast and acceptDeferredBroadcast now apply the same size oracle as the fetch path: a within-band body is accepted for import (sender marked as body-holder) regardless of hash, so a pusher's own post-import witness (flushWitnessWaitersForImported) is no longer rejected downstream; only byte-identical bytes are cached for pre-import serving, and an oversized body is dropped. fetchAndVerifyWitness (relay fetch, serve-only) keeps requiring the BP's bytes by design and now documents that limitation. Also refreshes every remaining "byte-correctness" comment to the size-oracle semantics (fetcher, handler, peerset, wit protocol), and renames the broadcast byte-mismatch meter to broadcast_oversize with new hash_divergence, implausible_size, import_failure and import_retry meters. Tests: end-to-end import-failure re-fetch through the real fetcher loop (TestImportFailureWithDivergedWitnessRefetchesFromAnotherPeer — caught the provenance loss in enqueue), charge/retry unit tests, degenerate-size and saturating-multiply cases, implausible announce size (0 and cap+1 struck, cap accepted), divergent/oversized broadcast on both the signed and deferred paths, source exclusion in resolveWitnessFetchPeer, and the exclusion set lifecycle. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Brme9KQBd7fZBMnMVEhAZU * eth: keep the WIT2 fetcher wiring comment off the lines #2417 touches #2417 changes the NewBlockFetcher call directly above the WIT2 striker wiring; editing the adjacent comment here made the two branches conflict when merged together even though each merges cleanly into the candidate. Leave the striker comment as it was and document the size-oracle extension (import-failure strike + source exclusion) in its own block below. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Brme9KQBd7fZBMnMVEhAZU * eth/fetcher: charge a size-oracle import failure only when the witness could have caused it chargeDivergedWitnessImportFailure struck and excluded the serving peer on any insertChain error. "Diverged" is the normal case on a stateless node (every node persists its own generated witness), so every import failure — including a contract bytecode missing from local disk, which the downloader heals and the witness never carried, or an interrupted insert — would have struck two honest witness sources per block until the node had none left. Gate the charge on a positive allowlist of witness-attributable failures: a missing trie node (incomplete witness), and execution against the witness disagreeing with the header (ErrStatelessStateRootMismatch, ErrGasUsedMismatch, ErrReceiptRootMismatch, ErrBloomMismatch, ErrRequestsHashMismatch, and the fmt-built "invalid merkle root" / stateless self-validation mismatch messages). Everything else — missing code, interrupted or stopped chain, whitelist mismatch, header or DB errors, unknown errors — is not charged. Also: drop the unreachable absBytes == 0 guard in acceptableWitnessSizeCeiling (the page threshold floors at one page); make the deferred-broadcast hash-match test use a size band that would reject the body, so only the hash match admits it; cover the handler size-oracle helpers without a fetcher and the deferred size-band binding's boundary and TTL. Tests: TestChargeDivergedWitnessImportFailureIgnoresNonWitnessErrors, TestIsWitnessAttributableImportError, TestSizeOracleHelpersWithoutFetcher, TestDeferredAnnounceCacheHasWitnessSizeWithin; existing charge/re-fetch tests now use attributable errors. * eth/fetcher: never charge a witness server for a contract bytecode missing from local disk #2401 surfaces a missing contract bytecode on the stateless path as core.ErrStatelessIncompleteState wrapping a *state.MissingCodeError, the same sentinel that wraps a missing trie node. Witnesses carry no code, so the server could not have supplied it and the downloader's self-heal fetches the blob; charging the server would strike and exclude honest witness sources on every block that touches the contract until the node had none left. Check for *state.MissingCodeError first in isWitnessAttributableImportError and return false explicitly, so the wrapped cause — not the shared sentinel — decides attribution: missing node → incomplete witness → charged; missing code → local gap → not charged; the bare sentinel → cause unknown → not charged. Tests: the wrapped MissingCodeError case in TestChargeDivergedWitnessImportFailureIgnoresNonWitnessErrors (no strike, no exclusion, no re-fetch, no budget consumed) and both wrappings of ErrStatelessIncompleteState in TestIsWitnessAttributableImportError. * eth/fetcher: cover the exported size ceiling and the re-fetch guard branches Exercise AcceptableWitnessSizeCeiling from its own package (the handler is its only production caller) and the two retryAfterImportFailure early exits — block already known locally, witness marked unavailable — so the new code is fully covered where it lives. * eth/fetcher, eth: split importBlocks under the complexity gate; pin the sampled mutation survivors Diffguard flagged importBlocks at complexity 21 (threshold 15) after the witness-retry plumbing. Move the goroutine body into runBlockImport, which returns whether the failed import should be retried with a witness from another peer, and the block-tracker log into logTrackedImport; importBlocks now only routes the result to witnessRetry or done. Behaviour is unchanged. Pin the five sampled survivors: assert the diverged flag on both the exact-match and divergent returns of verifyAgainstSignedHash and on the WIT1-only (nil lookup) return; add the exact-boundary case to the saturating multiply (MaxUint64/2 * 2 must multiply, not saturate); and pin that the fetcher's striker and source-excluder callbacks are wired into the handler (StrikeWitnessServer / ExcludeWitnessSource are exported for that test). * eth, eth/fetcher: charge pushed witnesses on import failure; stricter quarantine clear and announce conflict Review follow-ups (pratikspatil024, 2026-09-21). The broadcast path accepted a within-band divergent body for import but could not charge it: handleBroadcast set only op.witness, and both cached-witness attach sites rebuilt the op without provenance, so chargeDivergedWitnessImportFailure returned on its first guard and a NewWitness push was the free way to deliver unusable bytes — and the easier one, since the first witness to arrive is the one attached. acceptSignedBroadcast/acceptDeferredBroadcast now report the divergence, InjectWitness carries it with the pusher, handleBroadcast records witnessPeer/witnessDiverged and the announce's fetch closure, and the before-the-block cache keeps peer+diverged so both attach sites build the op via cachedWitness.injectFor with full provenance. An import failure of pushed bytes now strikes and excludes the pusher and re-fetches from another peer, as on the fetch path; a pusher excluded for a block has its further pushes for that block dropped (broadcast_excluded_source_drop) so it cannot beat every honest re-fetch with the same body. verifyAgainstSignedHash cleared the oversize/quarantine state on every in-band acceptance; a divergent in-band body proves nothing about the signed size, so a server alternating oversized and in-band bodies could keep a block out of quarantine indefinitely. Only BP-identical bytes clear it now (the pending-removal exits and TTL sweep still bound the maps). signedWitnessCache.putIfNewer keyed conflicts on WitnessHash alone; the signed WitnessSize decides the accept band, so a same-hash announce with another size is now a conflict too (first commitment wins) instead of replacing the band once the relay window lapses. Tests: push-path import-failure e2e for both attach sites through the real fetcher loop, provenance bit from both accept functions, excluded pusher dropped, quarantine kept across a divergent acceptance, size conflict rejected before and after the relay window. * eth/fetcher: move the import-failure charge next to its error predicate Diffguard's file-size gate allows a file already over 800 lines to grow by 10% of its base size; block_fetcher.go had reached +138 (10.4%) on the candidate base. chargeDivergedWitnessImportFailure and maxWitnessImportRetries move to witness_import_errors.go, which already holds isWitnessAttributableImportError, the predicate the charge is gated on. No behaviour change. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
What
The witness page-count check polled random peers for a majority page count and, on disagreement, dropped and jailed the reporting peer.
Witness page counts are non-deterministic across honest nodes: a valid witness can round to one more 15 MiB page on one node than on another (the trie-node set collected during execution varies). So the cross-peer vote both:
How
Replace the vote with a local bound: a page count within the gas-derived threshold (already ~10× the real per-block witness size) is accepted; a count above it exceeds what the block gas limit can plausibly produce, so it is refused for that peer — the fetch tries another peer — without disconnecting or jailing.
CheckWitnessPageCountno longer takes peer-query closures and no longer votes or jails; it accepts ≤ threshold and refuses above.verifyWitnessPageCountSync,getConsensusPageCountWithOriginal, thewitnessManagerjail hook, theBlockFetcherjailPeerplumbing that only fed it (down to theNewBlockFetchersignature), and the now-unused verification meters/const.TotalPageswithin a single peer's stream) are unchanged.Review follow-up: making the refusal actually stop the download (a27aff4)
Review found that
receiveWitnessPagestill surfaced a refusal as an error. The call site discards that error, and the deferred retry handler then marked page 0 retryable and rebuilt requests fromwitTotalPages— which the refusing page had just populated — so every remaining page of the refused witness was requested from the peer we had just refused, page 0 was retried, the duplicate tripped the "more pages than TotalPages" violation and jailed the peer for an unrelated reason, and the reconstructed witness could still be delivered. Before the vote was removed this was masked by the synchronous peer drop.continue— never return an error, so the retry path cannot re-queue the refused download.buildWitnessRequeststakesdownloadPausedand skips paused hashes for both fresh pages and retries.TotalPagespause is taken undermapsMulike the other writers.Relationship to the WIT2 signed path
This is the WIT1 page-count layer. It is independent of, and complementary to, the WIT2 signed-size acceptance change (#2416): together they make both witness-delivery gates tolerant of non-deterministic witnesses. Both branches merge cleanly into
v2.10.2-candidate, and cleanly with each other in either order (verified withgit merge-tree).Tests
TestCheckWitnessPageCountAcceptsWithinThresholdandTestCheckWitnessPageCountRefusesAboveThresholdWithoutDrop.TestConcurrentWitnessVerificationto the new signature (still a race test).TestRequestWitnessesWithVerification_RefusedPageCountStopsDownloaddrivesRequestWitnessesWithVerificationend-to-end with a mock peer claimingTotalPages=4and a refusing bound, and asserts only page 0 is requested, the jail callback never fires, and the request resolves empty. Against the previous code it fails with exactly the reviewer's repro (pages0,1,2,3,0requested).TestBuildWitnessRequests_SkipsPausedHashpins the request builder.Test plan
go build ./...go vet ./eth/ ./eth/fetcher/golangci-lint run ./eth/...(0 issues)go test ./eth/fetcher/go test ./eth/ -run 'Witness|Wit2|PageCount|Announce|Broadcast|Relay|RequestWitnesses|BuildWitnessRequests'go test -race ./eth/ -run 'RequestWitnessesWithVerification|BuildWitnessRequests' -count=3