chore: forward-port v5 node fixes to next - #25247
Open
spalladino wants to merge 9 commits into
Open
Conversation
When the p2p client starts while there are blocks to sync, the libp2p service start is deferred to `startServiceIfSynched`, which runs inside the block stream event handler. That handler catches and only logs errors, so a TCP bind failure (e.g. `ERR_NO_VALID_ADDRESSES` after a restart) was swallowed. On top of that, the sync promise awaited by node creation was resolved before the service was started, so startup succeeded regardless and the node kept running with a dead p2p stack and zero peers. The service is now started before the sync promise settles, and a failure logs at fatal level and rejects the stored sync promise (rather than throwing, which the block stream would eat) so node creation fails and the process exits, letting the orchestrator restart it. Part of A-1701. --- Related PRs from the same incident (all independent, all targeting `merge-train/spartan-v5`): #25177 (fail startup when the p2p service fails to start), #25185 (connectivity signal + slasher/proposer/health/sendTx gates), #25183 (periodic zero-peers warning). (cherry picked from commit c22df34)
) A validator could run for hours with a dead libp2p stack - zero peers and zero gossip - without anything in its logs flagging the condition. The failure was only reconstructable after the fact from the absence of activity, rather than from any explicit signal. The peer manager heartbeat now tracks consecutive zero-peer heartbeats. After 3 of them (so startup and transient dips stay quiet) it logs a warning that the node has no connected peers and can neither gossip nor propagate txs, with the zero-peer heartbeat count, the heartbeat interval, time since a peer was last connected, and the cached peer count. The warning then repeats about once a minute, derived from peerCheckIntervalMS, instead of on every heartbeat. When peers come back, a single info log reports connectivity restored and the new peer count. Connected peers are counted via libp2p getPeers(), matching what getP2PConnectivity reports. A peer count gauge already existed (PEER_MANAGER_PEER_COUNT, recorded in discover()), so no metric was added - this change is logs only. Part of A-1701. --- Related PRs from the same incident (all independent, all targeting `merge-train/spartan-v5`): #25177 (fail startup when the p2p service fails to start), #25185 (connectivity signal + slasher/proposer/health/sendTx gates), #25183 (periodic zero-peers warning). (cherry picked from commit e9ee6ea)
…25202) Validators were aborting checkpoint proposals on eth-mainnet with a generic `L1RpcError: L1 RPC request failed` during pre-broadcast header validation, e.g. `insufficient funds for gas * price + value: have 169461140054989709 want 432138191814787072`. With this PR we now skip any balance checks during simulate calls for header validation by removing fee-per-gas settings, and also just in case we fake a lot of ETH as balance. Created A-1712 to gate block-building on publisher balance. `L1TxUtils.simulate` attached production fee fields (competitive P75 priority fee, ~26 gwei during the incident) together with the worst-case `MAX_L1_TX_LIMIT` gas cap (16.7M) to every `eth_simulateV1` call, and `validateBlockHeader` overrode the multicall3 sender's balance with the validator's real balance. Supplying fee fields makes the node enforce the EIP-1559 upfront funds check (balance >= gasLimit x maxFeePerGas, ~0.43 ETH at those numbers) before executing anything, so any validator holding less than that failed header validation and skipped its proposal — even though the actual publish tx costs a small fraction of that. With fee fields omitted, the node defaults them to zero, so the check only fired because we supplied them. - Omit `maxFeePerGas`/`maxPriorityFeePerGas` from simulated calls entirely, making the upfront check vacuous for all `simulate()` callers. - `validateBlockHeader` now always uses the ample 10 ETH multicall3 balance override (previously fisherman-mode only) instead of the real sender balance, as compatibility with providers that still apply an upfront check. - While at it, the third commit renames `SequencerPublisher.validateBlockHeader` to `validateCheckpointHeader`: it takes a `CheckpointHeader` and simulates the rollup's `validateHeaderWithAttestations`, so it never validated a *block* header. Mechanical rename of the method, its `trackSpan` label, and all call sites and doc references; contract-side names, the `header-validation-failed` event key, and metric labels are untouched. - An insufficient-funds rejection of the simulation request (code -38014, with a message-match fallback for clients and gateways that report it differently) now surfaces as a descriptive error carrying the sender and the provider's have/want message, instead of a generic RPC error. - `_simulate` is restructured so only the `simulateBlocks` transport call sits inside the classifying try/catch; decoding of failed calls and the success path happen after it. - **The core semantic claim**, verified against client and library sources: viem 2.38.2's `simulateBlocks` forwards call objects verbatim (no fee filling, no type inference — unlike `simulateCalls`/`sendTransaction`, so re-verify on a viem bump), and geth, reth, anvil (>= v1.1.0), and nethermind all default omitted fee fields to a zero gas price and zero the block base fee under `validation: false`. The upfront funds check still runs in all four but degenerates to `balance >= value`; every call we simulate is zero-value, so it passes regardless of sender balance. Nothing in the simulated paths reads `tx.gasprice`. All four production `simulate()` callers were audited (`validateCheckpointHeader`, `simulateInvalidateCheckpoint`, the multicall aggregate helper, L1 contract deploy simulations): they consume only `gasUsed`/return data, both unaffected by gas price. - **The insufficient-funds classifier keeps a message fallback deliberately**: geth reports the rejection as spec code -38014, but reth uses -32003 (TransactionRejected) and anvil did too before its spec-conformance fixes, so matching on the code alone would miss those clients. - **The catch boundary in `readonly_l1_tx_utils.ts`** (second commit): only the RPC request is inside the try, so an execution revert whose message mentions "insufficient funds" (e.g. a plain `Error("insufficient funds")` revert string) can no longer be misclassified as an RPC rejection — this was a real bug in the first commit, caught in review and pinned by the new regression test. Also check the `MethodNotFound`/`fallbackGasEstimate` early-return path survived the restructure unchanged. - **Loss of signal**: dropping the real-balance override removes an (unintentional) affordability check. It never carried a real signal — wrong sender (multicall3), worst-case gas cap, inflated fees — and no other affordability check exists today: publisher selection only requires balance > 0, with the optional funding loop and balance metrics covering the rest operationally. An explicit post-simulation cost-vs-balance warning is a candidate follow-up. - **Tests are correct by inspection only** — this branch has not been built or run locally; CI is the first real execution. A port to `next` will follow; it needs the `getGasPrice` -> `getFeesPerGas` rename accounted for. Fixes A-1706 (cherry picked from commit 6289a9a)
…osal type (#25222) Follow-up to #25207. That PR removed the duplicate downstream re-validation of inbound block and checkpoint proposals, leaving p2p ingress (gossipsub topic validation) as the single place proposals are validated. The downstream handlers document that precondition in prose; this PR enforces it with the type system. - Adds branded `ValidatedBlockProposal` and `ValidatedCheckpointProposalCore` types (plus their minting functions) in `stdlib/src/p2p/validated_proposal.ts`, following the existing `Branded<T, Brand>` convention used by `BlockNumber` and friends. - The p2p received-proposal callbacks (`P2PBlockReceivedCallback`, `P2PCheckpointReceivedCallback`) and the downstream consumers (`ValidatorClient.validateBlockProposal` / `attestToCheckpointProposal`, `ProposalHandler.handleBlockProposal` / `handleCheckpointProposal`, and the `Validator` interface) now take the branded types, so a raw inbound `BlockProposal` / `CheckpointProposalCore` cannot reach them. - The brands are minted only in `libp2p_service.ts`, at the three points where the topic validator has already returned `Accept`: the block-proposal topic path, the checkpoint-embedded block path (`processBlock` is only set after Accept), and the checkpoint path. - Purely a compile-time marker: no runtime validation is added and there is no behavior change. The only non-type edits are in tests, which mint validated proposals from constructed ones. Related to A-1703. (cherry picked from commit 9118206)
Fixes two independent validation gaps around checkpoint shape. There is no circuit for proving a non-first block with zero transactions. The zero-tx block-root circuit (`rollup_block_root_first_empty_tx`) exists only so that an epoch can be proven when there are no transactions at all, and it is pinned to index 0: it starts a fresh sponge blob and carries a non-zero `in_hash`, both of which the block-merge continuity checks forbid at any later index. Validation methods had upper bounds on block contents but no lower bound, so a checkpoint carrying an empty block past the first would pass every node-side check and then be unprovable. **Fix**: `validateCheckpointStructure` now requires every block after the first to carry at least one tx, and the sequencer floors `minValidTxs` at 1 for any block past index 0 so it never builds one. The archiver passes `allowEmptyNonFirstBlocks` on the L1-ingest path, since such a checkpoint is already final on L1 and refusing to ingest it would only stall sync. Network configuration allowed `maxBlocksPerCheckpoint` to be set higher than the attestable limit, which would produce block indices that p2p and checkpoint validation reject. It is now capped at `MAX_ATTESTABLE_BLOCKS_PER_CHECKPOINT`. Fixes A-1614 (cherry picked from commit dd95801)
…25206) Fixes a race where `worldState.syncImmediate(N)` failed with `block_not_available` even though the archiver had block N, alerting `Eviction rule FeePayerBalanceEviction failed` on production nodes. ## Context `EventDrivenL2BlockStream.sync()` goes through `RunningPromise.trigger()`, whose pending-request slot was only cleared after the pass serving it completed. Since the world-state stream is event-driven, nearly every block-processing pass serves a request, so a `sync()` arriving mid-pass attached to a pass that started before the archiver committed the target block and resolved at N-1. Additionally, a trigger coalescing onto a pending request silently dropped its event payload, so the next pass could run armed with an older event's hot-block cache, which capped `getL2Tips()` below the archiver's real tip. Both mechanisms produce the observed error; the 100ms fallback poll self-healed right after. ## Approach - `RunningPromise` now captures and clears the request slot when a pass starts, so `trigger()` resolves only after a full run that started after the call; unserved requests are rejected when the loop exits instead of hanging; the per-trigger argument is removed entirely. - `EventDrivenL2BlockStream` is reduced to a doorbell: the archiver's aggregate event just triggers an immediate reconciliation pass, and every pass reads tips and blocks authoritatively from the source. The `HotBlockSourceAdapter` fast path and the event's `blocksAdded` payload are deleted — their arming conditions were the source of the race, and their only real saving was block-body re-hydration on event-triggered passes. - The latency win that motivated the event-driven stream (#24317) is preserved; if the removed body-read saving ever shows up in profiles, it should be recovered inside the archiver with a content-addressed block cache instead (tracked separately). Fixes A-1659 (cherry picked from commit 6f01032)
…tivity (#25185) Gates p2p-dependent operations on actual gossip connectivity, so a node running with a dead libp2p stack (zero peers) can no longer file false slashing accusations, burn its proposer slots, or silently blackhole txs. Follows an incident where a validator whose TCP listener failed to bind kept running peerless and voted `DATA_WITHHOLDING` against entire committees. One commit per concern (squashed on merge): 1. **`feat(p2p): expose p2p connectivity`** — new `getP2PConnectivity(): { enabled, connectedPeers }` on the p2p service, client, and node RPC API. The dummy (p2p-disabled) implementation reports `enabled: false`, so sandbox and single-node setups are vacuously healthy for every gate below. 2. **`fix(slasher): do not file data-withholding offenses while peerless`** — the data-withholding watcher infers offenses from the *absence* of txs in the local pool, which is invalid evidence without gossip connectivity. It now permanently skips (never backfills) slots probed while p2p is enabled with zero peers — after reconnecting, mined txs may already be evicted from the pool, so a late probe would still false-positive. 3. **`fix(sequencer): skip proposing when node has no connected peers`** — a peerless proposer broadcasts into the void (burns the slot) or, with enough own committee seats, publishes a checkpoint whose data nobody has. It now skips building below `minPeersToPropose` (default 1, `SEQ_MIN_PEERS_TO_PROPOSE`, 0 disables) while keeping L1 duties (votes, prune, invalidation) via the existing fallback. 4. **`feat(node): report per-component health on GET /status`** — `/status` now returns `{ ok, components: { p2p: { healthy, enabled, connectedPeers }, ... } }`. The `p2pHealthMinPeers` floor (`P2P_HEALTH_MIN_PEERS`) defaults to 0 (report-only) so fresh-network bootstrap nodes aren't failed by default; operators opt in. 5. **`fix(node): reject sendTx when node has no peers`** — instead of accepting a tx and gossiping it to nobody, the node rejects immediately so callers can retry against a healthy node. 6. **`test: keep single-node mock-gossip e2e working`** — `registerPhantomGossipPeer` test helper so the one single-node e2e test on the in-memory mock gossip bus presents one connected peer to the gates. 7. **`fix(slasher): do not file data-withholding offenses when p2p is disabled`** — a node with no p2p stack at all sees strictly less than a peerless one, so it must not slash either. The runtime gate drops `enabled` (any node with zero peers skips), and the watcher isn't constructed when `p2pEnabled` is false. Unlike the other gates, this one infers guilt from *absence*, which is never valid without gossip; the propose/`/status`/`sendTx` gates keep treating `enabled: false` as vacuously fine because they gate the ability to *act*, which a deliberately p2p-less node has by design. Strongly suggest to **review commit by commit**. Related PRs (independent, same incident): #25177 (fail node startup when the p2p service fails to start), #25183 (periodic zero-peers warning). Part of A-1701. (cherry picked from commit a6ce449)
) The node's five L1 event watchers (slasher rotation, slashing votes/rounds, checkpoint invalidation, slash events) used viem's `watchContractEvent`, which installs a server-side filter and polls `eth_getFilterChanges`. Server-side filters die in the field: load balancers route polls to backends that never saw the filter, and providers report purged filters with error codes viem does not treat as "filter gone" (reth -32602, Alchemy -32600; erigon returns an empty result with no error). Watchers then either churn recreating filters or hammer a dead filter id at the poll rate — operators reported 30-40 req/s of spam — while silently missing slashing and governance events. Replace the viem watchers with a custom poller, `watchContractEvent` in `@aztec/ethereum`. An earlier revision of this PR instead excluded the filter RPC methods at the transport level to force viem into its built-in getLogs fallback, but that fallback retries filter creation on every tick and issues a single unbounded `eth_getLogs` when catching up after an outage, so we now own the loop: - Each tick polls `eth_blockNumber` (viem caches it per client at the client's polling interval, so concurrent watchers share one request) and fetches new logs with `eth_getLogs` via `client.getContractEvents`, never touching filter RPCs. Requests never span more than a configurable `maxBlockRange` (default 100) blocks: catch-up after downtime is chunked into multiple bounded requests, and the cursor advances per successful chunk so a mid-catch-up failure retries from where it left off instead of losing blocks. - The helper is strongly typed on the event via viem's `ContractEventName` / `GetContractEventsReturnType` helpers (including `strict` decoding), and is built on foundation's `RunningPromise`, so ticks never overlap and unwatching interrupts a catch-up loop. - Per-log callback isolation (a throwing callback or a rejected returned promise is logged without dropping the rest of the batch), and every failed poll logs a warning. - Poll intervals per event: `SlasherUpdated` polls every 60s since slasher rotations are rare governance operations; the other four watchers poll at the client's polling interval as before, and only issue `eth_getLogs` when the head actually advances. All `listen*` methods accept an options override. - Tests cover baseline semantics (only events mined after subscription), chunk boundaries, failed-chunk retry from the same cursor, callback isolation, stop-on-unwatch, and an anvil integration test behind a request-recording proxy asserting events are delivered with zero filter RPCs on the wire. Known trade-offs, documented in the helper's jsdoc: delivery is best-effort with possible duplicates on reorgs (no `removed` notifications; production consumers treat events as triggers and re-read chain state), and events mined within ~1 polling interval of subscribing may be missed (filter mode had nearly the same window). Fixes A-1695 (cherry picked from commit 81fc589)
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.
Forward-ports the fixes merged into
merge-train/spartan-v5over the last two weeks onto thenextline. One commit per original PR, each carrying acherry picked fromtrailer.Ported
Commits are ordered as they merged into the v5 line, since several build on each other (#25177 → #25183 → #25185 and #25207 → #25222 → #25229).
Not ported
release-image/Dockerfile.dockerignore.Conflict resolutions worth a look
Four commits conflicted; all others applied clean.
ethereum/src/l1_tx_utils/l1_tx_utils.ts(fix: spurious insufficient-funds rejection when simulating L1 calls #25202) — the rename fix: spurious insufficient-funds rejection when simulating L1 calls #25202 called out in its own description:getGasPriceisgetFeesPerGason this line. Took the fix's side (fee fields dropped from the simulated call), so the localfeesPerGasread is gone along with them.archiver/src/modules/data_store_updater.ts(fix: checkpoint block-shape and block-count validation gaps #25229) — the v5 version of this method also passesmaxBlocksPerCheckpoint: MAX_CAPACITY_BLOCKS_PER_CHECKPOINTon the ingest path, which is a separate v5-only change that has not been ported. Only fix: checkpoint block-shape and block-count validation gaps #25229's actual change is taken here: the sharedvalidateOptsobject plusallowEmptyNonFirstBlocks: true. Ingest on this line therefore still validates against the attestable limit; raising it to the capacity limit is a separate port.ethereum/src/contracts/slashing_proposer.ts(fix: poll getLogs instead of eth_newFilter for L1 event watchers #25176) —listenToVoteCastcarries avoteIndexargument on this line (from feat(slasher): warn on own-validator slash votes using VoteCast vote index #25068), which the v5 version predates. Combined both: the newwatchContractEventpolling mechanism with thevoteIndexcallback argument retained.safe_json_rpc_server.test.tskeepsimport httpas a value import, since feat: configure RPC server timeouts #25243 added a runtimehttp.createServer()call to that file.stdlib/src/interfaces/p2p.test.tstakes onlyP2PConnectivity; theTxHash/GetTxByHashOptionsimports on the v5 side belong to v5-only API methods and would be unused here.Verification
yarn buildproduces a byte-identical error set toorigin/merge-train/spartanbuilt in the same working tree (46 errors, all from stale cross-line generated artifacts locally — Noir circuit artifacts, verification-key lengths,withWalleton contract types). No new compile errors from the port.Unit tests for every package with a hand-resolved conflict pass:
stdlib(52),ethereum(61),foundation(47),slasher(140),validator-client(39). Two suites (archiver/data_store_updater.test.ts,sequencer-client/checkpoint_proposal_job.test.ts) fail to load in this working tree on stale cross-line artifacts; the archiver one was confirmed to fail identically with the base version of the file, so CI is the first real run for those two.Labeled
ci-no-squashto preserve one commit per ported PR.