feat: merge-train/spartan-v5#24576
Merged
Merged
Conversation
…24563) ## Context `getWorldState` backs most node RPC tree/witness queries, and had three related problems: - World-state sync failures were swallowed (logged and ignored), so queries were served from whatever state the node had — stale reads with no signal to the caller. - For tag and number queries, the target block was resolved *after* syncing, so an archiver tip advancing mid-request (routine during catch-up) made valid queries fail spuriously with "not yet synced". - Resolution pinned only a block *height*, never a *fork*: a reorg replacing the block at that height between resolution, sync, and snapshot read was served silently — e.g. a `proven` query could be answered with a same-height block from a new fork that is not proven. ## Approach - Every query variant (number, hash, archive, tag) now resolves up front to a concrete (block number, block hash) via the archiver's `getBlockData`, and the sync is driven to that exact block and hash so the synchronizer barriers on it and detects fork mismatches. - A new `WorldStateSynchronizer.getVerifiedSnapshot(blockNumber, blockHash)` verifies the snapshot's own archive view matches the requested fork before handing it out (block 0 is checked against the initial header hash, since its snapshot predates the genesis archive leaf), closing the remaining window between sync and snapshot read. - Sync and fork-verification failures surface as `WorldStateSynchronizerError` and are retried a few times with the query re-resolved each time, so prunes and fork flips heal onto the current chain instead of failing or serving wrong-fork state. Hash and archive resolution misses stay terminal with a clear reorg error; tag and number misses are treated as transient. - `proposed`/`latest` queries keep their "latest on the current fork" semantics: plain sync plus the committed db, unverified. ## API changes - `WorldStateSynchronizer` gains `getVerifiedSnapshot(blockNumber, blockHash)`, implemented by `ServerWorldStateSynchronizer` (fork-verified) and `TXESynchronizer` (passthrough, no reorgs). Other `getSnapshot` callers are unchanged. - Node RPC queries at a block now fail instead of silently returning stale state when world state cannot sync, and the error messages for unknown or unsyncable blocks have changed accordingly. Fixes A-1339
…ndle pg pool errors (A-1313, A-1314) (#24556) Hardens the validator HA signing path against an honest-validator double-sign and a Postgres-induced crash. Both issues live entirely in the HA signer. ## Context **A-1313 (stuck-duty cleanup race → slashable double-sign).** `signWithProtection` discarded the boolean from `recordSuccess` and returned the signature regardless. If the background cleanup loop deleted our `SIGNING` protection row while the (remote) signer was slow, the signature was broadcast with no protection record in place — so a later proposal for the same slot with different data could sign freely, i.e. slashable equivocation by an honest validator. The race is live today: `SequencerClient.start()` starts the validator client, which starts the cleanup loop. (The issue text claimed this was masked by #1111 never starting the cleanup loop; that is stale and no longer true.) **A-1314 (pg.Pool without an 'error' listener crashes HA validators).** `createHASigner` built a `new Pool(...)` with no `pool.on('error')`. pg re-emits idle-client errors (e.g. a Postgres restart severing an idle connection) on the pool; with no listener Node escalates to `uncaughtException` and crashes the process — taking down every HA replica sharing the DB at once. ## Approach Four defense-in-depth fixes for A-1313 (part 1 alone closes the slashable broadcast; parts 2-3 remove the ways the race arises) plus the independent A-1314 liveness fix: - Treat `recordSuccess === false` as a signing failure: throw a new `SigningLockLostError` and never return/broadcast the signature. The duty is not deleted, since we no longer own the row. This is the hard safety backstop: even under pathological timer behavior the worst case is a failed duty, never a broadcast double-sign. - Bound each signer call with a hard timeout (`executeTimeout`), configured via `signerCallTimeoutMs` (default 30s) and clamped at construction to `maxStuckDutiesAgeMs / 2` (default 144s / 2). Since `signWithProtection` is the only writer of `SIGNING` rows and every path through it is bounded by this clamped timeout, an in-flight signing always times out and releases its row well before stuck-duty cleanup could reclaim it — so cleanup can never race a live signing. A timeout takes the existing failure path (release the lock, then throw), and the orphaned signer promise is discarded. - Add a request timeout (`AbortSignal.timeout`) to both Web3Signer key-store fetches, surfaced as a clear timeout error instead of hanging. The keystore is only constructed directly in tests today (production uses node-keystore's remote signer), so the timeout is a constructor option defaulting to 30s. - Register a `pool.on('error', ...)` structured-log handler in `createHASigner` (covering both the created and injected pool), and add the same handler to the e2e HA fixture pools. Only message/code is logged, never the raw error. ## API changes - New env var `VALIDATOR_SIGNER_CALL_TIMEOUT_MS` (default 30000) controls the per-call signing timeout (effective value clamped to `maxStuckDutiesAgeMs / 2`), exposed as config field `signerCallTimeoutMs`. - The confusable existing config field `signingTimeoutMs` (how long to wait for a *peer* node's in-progress signing, default 3s) is renamed to `peerSigningTimeoutMs` so each name says whose wait it bounds. Its env var `VALIDATOR_HA_SIGNING_TIMEOUT_MS` is already shipped and is kept unchanged; the derived CLI flag follows the field rename (`--sequencer.signingTimeoutMs` → `--sequencer.peerSigningTimeoutMs`). Each fix follows red/green with unit tests in `validator-ha-signer` (errors, signer, slashing-protection service, LMDB + Postgres backends, factory) and `validator-client` (Web3Signer key store). Fixes A-1313 Fixes A-1314
## Context `readMessage` on the requesting side accumulated **all** response chunks into memory before the snappy `maxSizeKb` validation ran (that guard lives in `SnappyTransform.inboundTransformData`, which only executes after the full compressed stream has been buffered). A peer we dialed could send a valid `SUCCESS` status chunk and then stream data up to the request timeout, forcing the requesting node to buffer arbitrarily much (~1.2 GB/response at 1 Gbps within the 10s timeout) → remote-triggerable OOM, amplified by concurrent in-flight requests. ## Approach - Track a running byte total in the `readMessage` loop and abort as soon as it exceeds a bound derived from `maxSizeKb`, before buffering more chunks. The bound is `maxSizeKb * 1024 * 2`: snappy can expand incompressible input to ~1.17x, so 2x sits comfortably above the worst case (a legitimate max-size response is never rejected) while capping buffered memory at twice the permitted post-decompression size. - Uses `chunk.byteLength` rather than `chunk.subarray().length` — calling `subarray()` on a `Uint8ArrayList` consolidates its backing buffers into a copy, which would materialize the very chunk we are trying to reject. - Throws a dedicated `ResponseSizeLimitExceededError`, penalized as a `LowToleranceError` via the existing `categorizeResponseError`/`handleResponseError` path, so the offending peer's stream is torn down and it is scored down. - The existing decompressed-size preamble check in `inboundTransformData` still guards the decompression-bomb variant; this closes the residual reception-side buffering gap. Per-request bound only — aggregate in-flight amplification across peers is tracked separately. Fixes A-1399
…24554) A reqresp request payload must fit in a single muxer frame: yamux splits writes larger than 64KiB (minus the 12-byte frame header) into multiple frames, each of which arrives at the responder as a separate chunk, and the responder never reassembles a request from multiple chunks. A request type growing past that limit would surface as a confusing decoding error on the responder instead of failing at the sender. - Assert the payload size in `sendRequestToPeer` before dialing, throwing a descriptive `OversizedReqRespRequestError` locally. The check runs before the generic error handling so the remote peer is not penalized for a local bug. - Pin `maxMessageSize` on the yamux muxer to its library default (64KiB) so a dependency upgrade cannot silently change the limit the assertion relies on. - Tests: an oversized payload is rejected without dialing the peer and without penalizing it; a payload at exactly the limit round-trips successfully. Related to #24552. ## No current subprotocol can exceed the limit The limit is 65,524 bytes (64KiB yamux frame minus the 12-byte header). **No reqresp subprotocol today can produce a request anywhere near it**, so this assertion cannot fire on legitimate traffic: - `GOODBYE`: 1 byte (the reason code). - `PING`: a few bytes. - `STATUS`: a `StatusMessage` (component versions string, block numbers, block hash) — tens of bytes. - `AUTH`: an `AuthRequest` (`StatusMessage` plus a 32-byte challenge) — tens of bytes. - `TX`: the request type is `TxHashArray` (4 + 32·n bytes), but this subprotocol currently has no sender — only the server-side handler remains. - `BLOCK_TXS`: the only request that scales with anything. Serialized as `archiveRoot` (32 bytes) + tx-indices `BitVector` (4 + ⌈N/8⌉ bytes, N = tx count of the proposal) + tx-hashes commitment (32 bytes) + optional full-hash vector (4 + 32·H bytes). Pinned-peer and smart-peer requests send indices only (H = 0). Dumb-peer requests include full hashes but chunk them to `txBatchSize`, which is always the default 8 in production (`BatchTxRequester` is constructed without opts, and the option is not wired to any config). N is capped at deserialization by `MAX_TXS_PER_BLOCK = 2^16`, so even a maliciously large proposal yields a worst-case request of ~8.5KB — about 13% of the limit. A realistic 32-tx block produces requests of a few hundred bytes. Breaching the limit would take a proposal with ~460k txs (which `BitVector.fromBuffer` rejects at 2^16 anyway) or a `txBatchSize` over ~1,800 with full hashes enabled — neither is reachable today. The assertion exists so that if a future change makes a request type scale past the limit, it fails loudly at the sender instead of as a decoding error at the responder.
#24552) ## Context The reqresp rate limiter is consulted once per inbound stream (`streamHandler`), but the sub-protocol handler was invoked once per chunk read from that stream (`processStream`). Req/resp is one-request-one-response and an honest sender writes a single payload before half-closing, but a malicious peer can open one stream (costing a single rate-limit token) and then push many request frames on it — each frame arrives as its own chunk and drives a full handler invocation (mempool / block / tree lookups). Per-peer and global rate limits are bypassed by the fan-out factor. ## Approach Make `processStream` handle exactly one request per stream: after emitting the response for the first chunk, the pipeline generator returns instead of looping over the rest of the source. Extra frames a peer queued on the same stream are discarded when the stream closes, so work is bounded to one request per token. This does not regress legitimate traffic — no code path pipelines multiple requests on a single stream, and the one-chunk-per-request framing is already in force. The alternative (per-invocation rate checks inside the loop) was rejected: nothing pipelines, and mid-stream status signalling is broken on the requester side, which parses the first chunk as status and concatenates the rest as data. Adds a unit test that drives `processStream` with three frames on one stream and asserts the handler runs once and the sink receives a single SUCCESS + response pair. Fixes A-1324
…-1315, A-1317, A-1318) (#24565) ## Context A store that must never silently become empty — the single-node signing-protection LMDB, and (for the ordering fix) world-state — could be wiped or bypassed by the `DatabaseVersionManager` / store-creation path. - **A-1315**: `writeVersion()` ran *before* `onOpen()`, and was a plain non-atomic `writeFile`. A crash in between left a "valid" marker over an empty/partial data dir; on restart the self-healing reset was skipped forever (a stable wedge for world-state, which opens several native stores in `onOpen`). - **A-1318**: any non-ENOENT read/parse failure of the version file (EACCES, EIO, truncation) fell back to `DatabaseVersion.empty()`, which unconditionally triggered a reset — so a transient permissions/disk error at startup rm-rf'd the signing-protection DB. The `schemaVersionMismatchPolicy: 'throw'` added by A-1029 only guarded the numeric-mismatch branches, not this one. - **A-1317**: a missing `dataDirectory` silently selected a fresh ephemeral tmp store on every start, giving no double-signing protection across restarts, with only a `debug` log. ## Approach - **A-1315**: `DatabaseVersionManager.open()` now opens the database *before* writing the version marker, making the marker a post-commit record — a crash before a durable open leaves no marker, so the next start re-runs the reset. `writeVersion` is now an atomic durable write (temp file → fsync → rename → best-effort directory fsync). The marker is only (re)written when it would actually change (first boot, reset, upgrade), which also avoids leaking a freshly opened DB if the write fails and drops the per-boot fsync. - **A-1318**: new `versionFileReadFailurePolicy` (`'reset'` default, preserving existing behavior for archiver/p2p/world-state; `'throw'` for signing protection). On `'throw'`, an unreadable version file fails startup with an operator-actionable error and leaves data untouched. Threaded through the kv-store `createStore` options. - **A-1317**: `createLocalSignerWithProtection` now throws when no data directory is configured, unless `allowEphemeralSigningProtection` (env `VALIDATOR_ALLOW_EPHEMERAL_SIGNING_PROTECTION`, default false) is set, in which case it warns loudly. The local network sets the flag by default; the production default is strict fail-fast. The rollup-address-change reset is intentionally left as-is: the LMDB slashing DB relies on it (see the `cleanupOutdatedRollupDuties` no-op). Reviewed by Codex and a second model; both confirmed the atomic-write mechanics, ENOENT branching, migration idempotency, and the schema-composition decision (a cross-field Zod refine was avoided because it would break the downstream `.merge()`/`.extend()` chain, so the invariant is enforced at the factory). Operator-facing changes are documented in the v5 changelog. Fixes A-1315 Fixes A-1317 Fixes A-1318
…#24329) ## What Adds `keepFinalizedTxsForSlots` to the v2 tx pool. Instead of deleting a finalized tx's data at the finalized tip, the pool keeps it for a configurable number of slots behind finality. Default is `0` (current behaviour). Prover nodes raise it automatically. ## Why A prover node fetches a checkpoint's txs from its tx pool (`TxProvider` → pool first, reqresp fallback) and re-reads them for failure upload. ## How - New `keepFinalizedTxsForSlots` config (`P2PConfig` + env `P2P_KEEP_FINALIZED_TXS_FOR_SLOTS`, default `0`). - `handleFinalizedBlock` resolves the slot margin to a block cutoff: target slot = `finalizedSlot − margin`, find the checkpoint at or before it, and use that checkpoint's last block as the deletion cutoff. Applied to both the active-pool deletion and the `DeletedPool` finalize path. Rounds to a checkpoint boundary, so it can retain slightly more than the configured margin but never less, and never deletes past the finalized block. A margin of `0` short-circuits to the previous behaviour. - The "checkpoint at or before the target slot" lookup is a single reverse range query, not a slot-by-slot scan: a new `{ fromSlot, limit, reverse }` variant on `CheckpointsQuery` (backed by `BlockStore.getCheckpointsBySlot`, a one-pass walk of the existing slot index) returns the nearest checkpoint at or before the slot with `limit: 1, reverse: true`. All `CheckpointsQuery` resolution now lives in `getCheckpointsData`. - Prover nodes: `createAztecNodeService` floors `keepFinalizedTxsForSlots` at `(proofSubmissionEpochs + 1) × epochDuration` (read from the rollup), taking the max with any operator-configured value and warning when it raises it. This matches the prover-node catch-up window. ## Testing - New unit tests in `tx_pool_v2.test.ts`: the retain/delete boundary and the checkpoint resolution when the target slot falls in a gap. - New `getCheckpointsBySlot` tests in `block_store.test.ts`: reverse exact-hit, gap walk-back, before-genesis, multi nearest-first, and the forward direction. - `fromSlot` schema round-trip added to the archiver and aztec-node interface tests. - Full `tx_pool_v2` (254), `block_store` (159), and interface (120) suites pass. Closes A-1274.
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
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
4 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.
BEGIN_COMMIT_OVERRIDE
fix(node): fork-aware getWorldState that fails closed on sync errors (#24563)
fix(validator): prevent double-sign on stuck-duty cleanup race and handle pg pool errors (A-1313, A-1314) (#24556)
fix(p2p): bound reqresp response buffering before size check (#24553)
fix(p2p): reject reqresp requests larger than a single muxer frame (#24554)
fix(p2p): process one request per reqresp stream to enforce rate limit (#24552)
fix(validator): fail closed on signing-protection persistence gaps (A-1315, A-1317, A-1318) (#24565)
feat(p2p): retain finalized txs a configurable margin behind finality (#24329)
END_COMMIT_OVERRIDE