execution/p2p: improve bal fetcher reliability - #22715
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves best-effort EIP-7928 Block Access List (BAL) fetching over eth/71 by making BAL retrieval more resilient to partial/invalid peer responses and by spreading requests across peers to avoid redundant truncated-prefix downloads. It also centralizes the “header has a non-empty BAL commitment” check in types.Header.
Changes:
- Add
Header.HasBAL()and use it in BAL request building. - Rework BAL fetching to shard large batches across peers initially, then broadcast unresolved requests in later rounds; keep valid entries even if some response entries are invalid, while still penalizing the peer.
- Introduce per-peer “BAL miss” tracking (with TTL) and add new downloader timeouts/config and tests around BAL deficit behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| execution/types/block.go | Adds Header.HasBAL() helper for checking a non-empty BAL commitment. |
| execution/p2p/bbd.go | Refreshes peer set per window, improves BAL peer selection, adds partial-miss logging, and switches BAL request building to HasBAL(). |
| execution/p2p/bbd_test.go | Adds a regression test ensuring blocks are still delivered even when BALs are persistently unavailable. |
| execution/p2p/bbd_options.go | Adds separate BAL batch/request timeouts and increases body batch timeout. |
| execution/p2p/bal_peer_misses.go | Adds per-peer BAL miss tracking with TTL to avoid querying peers for ranges they likely don’t retain. |
| execution/p2p/bal_peer_misses_test.go | Adds unit test coverage for per-peer BAL miss tracking behavior. |
| execution/p2p/bal_fetcher.go | Updates BAL fetcher API/signature, adds sharded/broadcast multi-round fetching, increases parallelism, and integrates miss tracking + partial-validation retention. |
| execution/p2p/bal_fetcher_test.go | Updates/extends tests to cover partial-invalid response retention and new multi-round fetch behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mh0lt
left a comment
There was a problem hiding this comment.
Reviewed at opus/high effort — LGTM, approving. Focused on the concurrency/networking rework since that is the highest-risk part.
Concurrency: no data race and no goroutine/channel leak found. Each round's workers in fetchAcrossPeers write only their own results[i] slot and the merge is serial after eg.Wait() (strictly safer than the old shared-map-under-mutex); balPeerMisses access is fully under b.mu; the bbd.go goroutines only read peer state (peersExcept) with mutations kept on the serial loop; errgroup ctx is derived from the batch timeout and cancelled via defer.
Verified correct: sharding/broadcast/termination (disjoint shards partition remaining exactly, remainder recomputed each round, no-progress broadcast round breaks — no dropped-and-never-retried request, no infinite loop); the partial-invalid path preserves valid entries while still penalizing the peer, and each entry is independently keccak-validated against the header-committed hash so a malicious peer can't poison neighbors; the miss heuristic can't permanently blacklist (TTL re-probe in mayHave + allPeers fallback when plausible is empty); best-effort fallback masks nothing consensus-critical (blocks are still delivered BAL-less); HasBAL migration is correctly scoped.
Nits / one question (all non-blocking):
balPeerMisses.mark: the TTL branch looks dead — when it enters viatime.Since(mark.at) > balMissTTL,blockNum <= maxMissing, somax(...)leaves the watermark unchanged; the real re-probe already lives inmayHave. Consider dropping the|| TTLclause frommark.balPeerMisses.mhas no eviction and grows with peer churn (entries never removed on disconnect). Not a functional leak (tiny entries, TTL-ignored inmayHave), but a prune-on-markor disconnect hook would bound it. Fine as a follow-up.- Question:
downloadBlocksForHeadersnow re-queries plausible peers per window and returnsno peers availablewhen empty. If the miss-tracker transiently reports zero plausible peers for a window mid-span, does the whole backward download abort rather than back off/retry? Worth confirming a momentary peer-set dip can't kill a long span (non-pinned peerId==nil path only).
Test coverage is good (prefix-truncation, single-peer straggler, termination bound, parallelism limit, both partial-invalid variants, miss-tracker, and BAL-less delivery).
…igontech#22190) ## execution/state, execution/vm: zero re-executions and aborts for BAL-fed parallel execution; remove `VersionMap.HasBAL` ## Why EIP-7928 block access lists pre-populate the parallel executor's versionMap with per-field value cells, so every read on a BAL-fed block can resolve deterministically — in principle making optimistic execution conflict-free. In practice, residual races and validation blind spots still caused re-executions and dependency aborts: reads recorded from signals the BAL structurally cannot carry (the SELFDESTRUCT flag, account existence, incarnation metadata), version churn on unchanged values, and probes recorded as observations when they were only guesses. Each re-execution is wasted work and — worse — non-determinism. This PR drives BAL-fed parallel execution to **literal zero re-executions and zero aborts**, verified from genesis to tip on glamsterdam-devnet-6 (and previously bal-devnet-7), and removes `VersionMap.HasBAL` entirely: BAL and no-BAL blocks run through one uniform, value/cell-evidence-based validation — pre-populated cells simply make reads deterministic when present, with no flag-gated semantics anywhere. ## What Net delta vs main: ~1,618 insertions / 179 deletions over 20 files (execution/state, execution/stagedsync, execution/vm, common/dbg), 29 new tests. The eth/71 BAL fetcher is NOT in this PR — it was extracted and landed separately (erigontech#22715). ### Validation semantics (versionmap.go) - `ValidateVersion` gains an `eip161 bool`: nil-vs-EIP-161-empty record dead-equivalence applies only post-Spurious-Dragon (pre-161, existing-empty is gas-observable, so the strict existence-only form is kept). Deadness is assembled from sub-field floors (`accountLiveAt`): a creation-time-empty record next to a funded Balance/Nonce/Code cell is live (the "created then funded" phantom that once produced a wrong BAL at bal-devnet-7 block 115,591). - AddressPath record reads validate existence-only; every sub-field is recorded and value-validated as its own read (records churn per fee-paying tx; their values are what matter). - Value tiebreakers unified across the code trio: Code (byte compare), CodeHash, and CodeSize (upgraded here) all survive a value-matching cold-read/flush collision and still invalidate on a real change. - Sub-field reads with no dedicated cell fold onto the AddressPath record for validation; the fold now carries a `recordField`/`matchesRecord` value tiebreaker — fee-merge record re-stamps with an unchanged field no longer invalidate readers (coinbase nonce churn). - `destroyedAndUnrevived` relaxes the created-account incarnation check: a nil record read of a destroyed, unrevived account is correct on every fork. ### Read-path determinism (read_paths.go, intra_block_state.go) - Value-aware read-time relaxation (`readValueUnchanged`) at the RD/WR dependency checks: version-only churn with an unchanged value (existence-only for records, with the same sub-field-aware dead case as validation) is not a dependency — no abort. - `ProvisionalRead`: a mid-account-load nil record probe is marked provisional; a cell flushed between probe and re-probe is adopted (the nil was never exposed to the EVM) instead of false-positive aborting. Provisional markers are demoted to definitive reads at every absent-conclusion exit, so they never outlive their load. - `synthesizeCreatedAccountBase`: an account absent from both the AddressPath map and the DB resolves its existence from BAL-prepopulated sub-field cells (Done cells only; bails on SD floors, estimates, and EIP-161-empty results) — removing the read-after-create race with the creator's flush. Synthesis refuses to fire once the tx has consumed a definitive absence (a recorded non-provisional nil read): adopting cells then would fork the tx's view mid-execution invisibly to validation; commit-time validation re-executes instead. - Recorded-read reconciliation: a DB-resolved account load reconciles the nil map-read marker it recorded (otherwise a later record cell spuriously invalidates the nil against a live account), including for recordRead=false flows. - Read-set-served probes are never laundered into recorded reads: the cold account-field path resolves a `ReadSetRead` source back to the underlying entry's source/version, and `accountRead` skips re-recording a read-set-served read. (Without this, validation deterministically rejects the synthetic source and the tx livelocks — "too many validator-invalid retries".) - DB-loaded records are stamped `UnknownVersion` (pre-block state) so lower- versioned BAL cells overlay them instead of being shadowed. ### SELFDESTRUCT signal handling (vm/operations_acl.go, intra_block_state.go) The SD flag is the one signal a BAL cannot carry, so recorded SD reads race the destroyer's flush. Everywhere its consequences are already pinned by value-validated reads, the probe is derived without recording: - The SELFDESTRUCT refund gas probe runs only when `refundsEnabled` (pre-London): post-EIP-3529 the result is discarded, so the recorded read was pure race surface. - `CreateAccount` (CREATE2 re-creation flow) derives `destructed` from its own write set → cached state object → direct map probe, unrecorded; the value-carrying synthetic incarnation/balance reads it stamps make any stale conclusion invalidate by value. - Account-load SD gates decide deletion via cells-evidenced revival (`AccountLifecycle`: any cell after the destruct index proves life; same-tx re-creation uses >= on AddressPath). ### Incarnation is metadata, not an observation (read_paths.go) The no-cell incarnation refresh no longer records a read: with no cell, the "current" incarnation is pre-block state or a synthesis guess, it is EVM-invisible, staleness is caught by the real field reads, and write normalization resolves the final incarnation from the versionMap (`SetAccountFieldFromMap`). The decisive repro was the fund-then-deploy CREATE2 pattern (target funded a block earlier, deployed onto, read by the next tx). ### BAL pre-population (versionmap.go) `WriteChanges` derives `CodeSize` and `CodeHash` cells from each BAL code change: EXTCODESIZE/EXTCODEHASH readers resolve from the map instead of falling through to the DB and racing the creator's flush. ### Instrumentation (env-gated, off by default) `ERIGON_TRACE_REEXEC` (classified validation-invalid/abort dumps: VINV reason=, DEP-*, ABORT-*, TOOEARLY) and `ERIGON_TRACE_BAL_FEED` (per-block feed/miss + per-cell pre-population) — the measurement tooling behind every number below. ## Verification Every fix is TDD'd (red→green); 29 new tests pin the classes. Full batteries on the final tree: **EEST 355,601 / 0 failed** across five shards (blocktests-devnet 86,931; blocktests-stable-parallel and -sequential 70,864 each; statetests-devnet 66,976; statetests-stable 59,966), `GOGC=80 make test-all` exit 0, `make lint` 0 issues, plus main's erigontech#22409 lifecycle/SD/revival test battery. On-devnet acceptance (fresh datadir, from genesis to tip, external CL): | run | code state | span | post-Amsterdam re-execs / aborts | |---|---|---|---| | bal-devnet-7 baseline | pre-fix | 0 → 43.8k | 1,188 / 31 | | bal-devnet-7 final | fix set | 0 → 407,538 (tip) | **0 / 0** (0 bare blocks) | | gd6 merge baseline | post-main-merge, pre-fix | 0 → 15k | 78 / 0 | | gd6 acceptance | + 5 gd6 fixes | 0 → 149,990 (tip) | **0 / 0** | | gd6 re-acceptance | post-erigontech#22409 merge + seam fixes | 0 → 171,182 (tip) | **0 / 0** | Each acceptance also had 0 blocks executed without a BAL, 0 BAL-hash mismatches/bad blocks, and a canonical hash match at tip. For contrast, the pre-Amsterdam blocks of the same syncs (no BALs by protocol) ran ordinary Block-STM at a 46.8% re-execution rate on gd6's conflict-heavy fuzz traffic — that is the cost the BAL eliminates. The pre-Spurious-Dragon strictness and all no-BAL behavior changes are covered by the stable sequential+parallel EEST shards; a mainnet from-0 parallel re-execution remains the deferred final verification before enabling anything by default. ## Not in this PR - The eth/71 BAL fetcher hardening (extracted; landed as erigontech#22715 — this branch now consumes it from main). - Any spec change: EIP-7928 does not carry account existence, the SD flag, or incarnation; the residual classes those caused are eliminated by derivation/relaxation, not by extending the BAL.
Summary
Extracting p2p improvements from wider PR #22190 about 0 BAL re-execs. Tested on bal-devnet-7 and glamsterdam-devnet-6. We were missing bal downloads for some blocks. Consistently downloaded all bals for all blocks after the below improvements.
Improve best-effort Block Access List fetching over P2P by making peer usage more efficient and resilient to incomplete responses.
types.Header.HasBALto centralize the check for a non-empty EIP-7928 BAL commitment.Testing
go test ./execution/p2p ./execution/types -count=1