chore: Accumulated ports to next - #24964
Merged
Merged
Conversation
…24840) ## Problem The safe JSON-RPC server built success responses as `{ jsonrpc, id, result }`. When a handler returns `undefined`, `JSON.stringify` drops the `result` key, producing `{"jsonrpc":"2.0","id":N}` — a response with **neither `result` nor `error`**, which violates JSON-RPC 2.0 and leaves raw/external callers unable to tell "not found" from a malformed reply. Surfaced via `node_getContract` on an undeployed (but valid) address: the response had no `result` field at all. ## Fix One line in the shared server (`safe_json_rpc_server.ts`): coerce an `undefined` return to `null` (`result: result ?? null`, so `0`/`false`/`''` are preserved). The `result` key is now always present. ## Blast radius Framework-level, so it fixes every method that can return `undefined` — ~20 on the node interface (`getContract`, `getContractClass`, `getTxEffect`, `getTxByHash`, `getBlock`, the witness getters, synced slot/epoch, validator stats, …) plus every other server on this framework (PXE, prover-node, archiver). Void methods now also return `result: null`. ## Why it's safe for existing clients The TS client short-circuits `null`/`undefined`/`'null'`/`'undefined'` **before** schema validation, so it never runs `null` through an `.optional()` schema — omitted vs `result: null` both resolve to `undefined` client-side. Only raw/external callers change, and they now get a spec-compliant response. ## Testing - Added a test: a handler returning `undefined` now yields `{ jsonrpc, result: null }` (red before, green after). - Updated the existing void-method (`clear`) assertions, single and batch, to the new shape. - Added integration tests (`test/integration.test.ts`) pinning that optional result schemas tolerate `null` responses: - end-to-end: the wire response for an undefined return carries an explicit `result: null`, and the safe client resolves it to `undefined`; - client-isolated: a stubbed fetch returning `result: null` verbatim resolves to `undefined` against an `.optional()` schema instead of throwing a `ZodError`. Both go red if the client's null short-circuit is removed (verified: `Invalid input: expected object, received null`). - Full `json-rpc` suite: 102 passed; `foundation` lint clean.
PhilWindle
approved these changes
Jul 24, 2026
PhilWindle
enabled auto-merge
July 24, 2026 14:49
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 24, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 24, 2026
…port #24667) (#24977) ## Summary Port of #24667 to `next` via `port-to-next-staging`. - Added `unfinalizedTaggingIndexesWindowEnd` and used it for sender pending bounds plus sender/recipient scan windows. - Preserved the current `next` window length and recipient sync structure while adapting fresh-secret bounds to `[0, WINDOW_LEN)`. ## Conflicts resolved - `constants.ts`: kept `next`'s `UNFINALIZED_TAGGING_INDEXES_WINDOW_LEN = MAX_PRIVATE_LOGS_PER_TX` and added the shared helper. - Recipient sync and tests were resolved against the current `next` implementation, without pulling in source-branch-only constrained probe changes. - Sender store tests were adapted to assert exactly `WINDOW_LEN` fresh pending indexes and rejection at `WINDOW_LEN`. ## Testing - `JEST_MAX_WORKERS=1 yarn workspace @aztec/pxe test src/storage/tagging_store/sender_tagging_store.test.ts src/tagging/recipient_sync/sync_tagged_private_logs.test.ts` passed: 61 tests. - `yarn build` was attempted but this partial checkout is missing generated dependency outputs such as `@aztec/l1-artifacts`; the failure was in setup dependencies, not the PXE changes. --- *Created by [claudebox](https://claudebox.work/v2/sessions/0ca34c3ad2537dfa/jobs/1) · group: `slackbot` · [Slack thread](https://aztecprotocol.slack.com/archives/C0AGN2WT3CP/p1784928640004259?thread_ts=1784928640.004259&cid=C0AGN2WT3CP)* --------- Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
…24754) `getL2ToL1MembershipWitness` assembled its witness from several non-atomic archiver reads (`getTxEffect`, then the epoch blocks, target block and checkpoint metadata read inside `computeL2ToL1MembershipWitness`). A store commit landing between those reads could splice together two different chain states and surface a spurious "message does not exist" error even when the tx-effect index was healthy. Wrap the witness assembly in a single `store.transactionAsync` so every archiver read binds to one write transaction and observes a consistent snapshot (the store serializes writers, so no commit can interleave). The L1 Outbox roots fetch stays outside the transaction to avoid holding the archiver's writer lock across a network round-trip, and the tx effect is re-read inside the snapshot so the receipt's block number and tx index stay consistent with the block data. Cost: witness assembly briefly serializes with archiver writers (it holds the store's writer queue while it reads), but the work under the lock is pure store reads plus an in-memory tree build — no network I/O. Context: this came out of the A-1426 investigation. The incident symptom itself turned out to be a downstream client bug following a v5 behavior change in `L2BlockSynchronizer` — not this race, and not archiver index corruption. This PR is therefore hardening against a real but so-far-unobserved torn-read window, not the incident fix. See #24765 for the companion store hardening from the same investigation. Related to A-1426
…24764) ## Context A mainnet sequencer kept casting governance signals for a payload whose proposal had already been executed: the outer Multicall3 tx succeeds but the inner `signalWithSig` reverts (swallowed by `allowFailure`), wasting ~100k gas per slot. Two client-side bugs allowed this: executed proposals were treated like any other terminal state, so the payload was re-signalled every round; and the TS `ProposalState` enum was missing `Droppable` (present in `IGovernance.sol` since July 2025), so decoding an `Expired` proposal threw, and the publisher failed open and signalled anyway. Additionally, since the executed payload upgraded the canonical rollup, proposers of the old rollup kept signalling on a `GovernanceProposer` now keyed to a different instance, where their signals always revert. ## Approach - Fix the `ProposalState` enum to match `IGovernance.sol` (9 states including `Droppable`) and drive the sweep from an explicit `LIVE_PROPOSAL_STATES` set. - Replace the boolean `hasActiveProposalWithPayload` sweep with `getPayloadProposalStatus` returning `'live' | 'executed' | 'none'`. The publisher stops signalling a payload whose proposal was executed, unless the new `GOVERNANCE_PROPOSER_FORCE_PAYLOAD_VOTE` config is set (L1 permits re-proposing an executed payload, and some payloads are designed to be re-executed). Live proposals take precedence over executed ones, executed verdicts are memoized in-process, and the sweep remains a bounded lookback capped by the protocol-wide proposal lifetime. - Add a canonicality guard to `enqueueCastSignalHelper`: resolve the canonical rollup via `getRollupAddress()` and skip the signal when the configured rollup is not canonical. Only the canonical rollup's current proposer can signal, so a non-canonical node would otherwise waste gas on a reverting signal. Because the guard returns early on a mismatch, round accounting and the EIP-712 digest use the configured (verified-canonical) rollup address. - The proposal-status guard fails open on RPC errors — at worst a duplicate signal the contract simply counts alongside others in the round — so a flaky L1 endpoint cannot silence governance participation. A failure to resolve the canonical rollup instead skips the signal for that slot and is retried on the next one. ## API changes `ReadOnlyGovernanceContract.hasActiveProposalWithPayload` and the `GovernanceProposerContract` wrapper are replaced by `getPayloadProposalStatus(payload)`. The redundant `GovernanceProposerContract.getInstance()` wrapper is removed in favour of the existing `getRollupAddress()`. New optional sequencer config `governanceProposerForcePayloadVote` (env `GOVERNANCE_PROPOSER_FORCE_PAYLOAD_VOTE`, default `false`). To be forward-ported to `merge-train/spartan` after landing on the v5 line. Fixes A-1422
…eck tx-effect deletes (#24765) ## Problem Two defects in `BlockStore` block removal could corrupt the tx-effect index (txHash → owning block hash/position) if the store ever held two blocks sharing a tx: 1. **Blind delete** — `deleteBlock` removed `#txEffects` entries by txHash without checking the entry still pointed at the block being deleted. If another stored block contained the same tx, that block's entry was destroyed collaterally, making the block unreadable (`Could not find tx effect for tx…`). 2. **Skip-on-unreadable leak** — `removeBlocksAfter` did `getBlock(bn) === undefined → warn + continue`, so an unreadable block's row, tx effects, and indices were never released. A later `addCheckpoints` overwrites the leaked row in place ("L1 data is authoritative"), leaving stale tx-effect entries pointing at coordinates now occupied by different txs — `getTxReceipt` then reports positions the chain no longer has, and `getL2ToL1MembershipWitness` resolves the wrong tx and throws `The L2ToL1Message you are trying to prove inclusion of does not exist`. This is hardening, not an incident fix. No honest code path produces two stored blocks sharing a tx: duplicate nullifiers are rejected at block building (double-spend validation against the build fork), at validator re-execution, and at world-state sync (nullifier leaves are non-updateable). The incident that prompted the investigation turned out to be a downstream client bug following a v5 behavior change in `L2BlockSynchronizer`, unrelated to the store. The store should still not amplify a duplicated-tx state into silent corruption if it ever appears — e.g. via a future upstream bug, or byzantine tx effects that repeat a txHash with distinct nullifiers (invisible to every nullifier-based check). ## Fix - `removeBlocksAfter` cleans up from the raw storage row, so blocks whose bodies can no longer be fully loaded still release their row, tx effects (enumerated from `#blockTxs`), and hash/archive indices. - `deleteBlock` only deletes a tx-effect entry still owned by the block being removed (compares the entry's leading block hash). - Both anomalies now log a warning (an entry owned by another block, or a missing entry). These states should be unreachable, so a warning in production logs is direct evidence of an upstream bug worth investigating — previously the ownership skip was silent, hiding exactly that signal. ## Cost The added reads are confined to prune/reorg paths: one `#blockTxs` read plus one tx-effect point read per tx of each *removed* block (roughly doubling point reads on block removal). Block ingestion and query paths are untouched. ## Verification - Regression test from #24732: two proposed blocks sharing a tx effect; asserts removal releases both blocks fully with no residue (fails on the pre-fix implementation). - Full `block_store.test.ts` suite passes (160 tests). Recreates #24732 with the original commit cherry-picked and authorship preserved (thanks @benesjan). On top of it, this PR updates the description and code comments to match what the follow-up investigation established about how a duplicated-tx state can and cannot arise, and adds the anomaly warnings. Supersedes #24732. Related to A-1426
## Context An RPC node logged `Gossip validation for checkpoint_attestation took 15533ms, approaching mcache eviction window of 8400ms` shortly after startup. Investigation (see A-1425) showed the validations were not slow — the Node.js event loop was fully blocked for 12–15s, and the attestations were queued behind it. The blocking call is `getKzg()`, which lazily builds the `@crate-crypto/node-eth-kzg` precomputation tables synchronously (~2s locally, 12–15s under the pod's CPU limits) on first use. On a plain RPC node that first use is the archiver reconstructing blobs during sync, or the proposal handler uploading blobs right after a checkpoint arrives — both immediately adjacent to gossip traffic, so the stalled loop overruns the gossipsub mcache window (8.4s) and attestation forwarding is skipped. Sequencers already avoid this via `Sequencer.init()`, so only non-sequencer nodes were affected. ## Approach Warm the KZG singleton in `createAztecNodeService`, right after the existing bb.js WASM singleton init and before any subsystem runs. The cost is unchanged (every full node reconstructs blobs and pays it eventually) — it just moves off the gossip path to a point where blocking the loop is harmless. `getKzg()` is an idempotent per-process singleton, so this is a no-op for sequencers and for tests that pre-warm via `warmBlobKzg` (all e2e setups do), and adds no work for pure unit tests, which never construct a full node. Fixes A-1425
…l for failure upload (A-1216) (#24983) ## What Stops the prover-node from holding every checkpoint's `Tx` objects in memory for the whole proof-submission window. - Drops `CheckpointProver.txs` (an instance map that was populated at gather and never cleared). `executeCheckpoint` now consumes the gathered txs locally and lets them go out of scope. - The only reader of the cached txs *after* execution was failure upload. `SessionManager.buildProvingData` now re-fetches each checkpoint's txs from the tx pool (via a new `CheckpointProver.getTxsForUpload`, concurrently across checkpoints) instead of reading the cached map. It becomes `async`, and the two `prover-node` upload call sites `await` it. ## Why A live heap snapshot of a prover-node under load showed thousands of retained input `Tx` objects (client proofs + public inputs) — one set per registered `CheckpointProver`, held until the 100-epoch submission window expired. The tx pool already stores these txs durably on disk (kept until L1 finality, with A-1274's retention margin covering a lagging prover), so the in-memory copy is pure duplication of data the pool holds. Re-fetch is safe: at failure time the txs were used for proving moments ago, so they are almost always still resident in the local pool (reqresp covers the rest); the A-1274 margin guarantees the durable case. Re-fetch is best-effort — a tx the pool can no longer supply is logged and omitted rather than failing the post-mortem, since a partial snapshot is still useful. Note the rerun/replay path (`rerun-epoch-proving-job.ts`) is unaffected — it rebuilds from the already-uploaded `jobData.txs`, not from live prover state. ## Testing - `yarn build`, full `@aztec/prover-node` suite (184/184). - New tests: a `CheckpointProver` caches no txs and re-fetches from the pool on demand; failure upload rebuilds complete `EpochProvingJobData` by re-fetching. ## Depends on A-1274 (tx-pool retention margin behind finality) — guarantees the pool keeps txs long enough for a lagging prover's failure-upload re-fetch.
…ool (#25000) ## Problem When the block builder flags a tx as failed, it hard-deletes it from the local mempool via `handleFailedExecution`. That is terminal for the tx — it will never be retried by that node — but the **reason** is not visible at the log level nodes actually run. There are two ways a tx lands in `failed`: - **Execution threw** — already logged at `warn` (`Failed to process tx <hash>: <error>`). - **Pre-process validation rejected it** — logged at **`debug`** only (`Rejecting tx <hash> due to pre-process validation fail: <reason>`). The second is the common case: `createTxValidatorForBlockBuilding` runs timestamp, phases, block-header, double-spend and gas checks before execution, and a rejection there is fast and silent. The drop itself (`Dropping failed txs <hashes>`) is `verbose`, and the pool-side confirmation (`Deleted N failed txs`) is `info` but carries hashes with no reasons. Net effect on a node running at `info`: transactions disappear from the mempool with no recorded cause. This came up while investigating mainnet transactions that sat unincluded — the logs showed `Processed 0 successful txs and 6 failed txs` immediately followed by `Deleted 6 failed txs`, and the actual rejection reason for those txs was unrecoverable from production logs. ## Change - `public_processor.ts` — pre-process validation rejection moves from `debug` to `warn`, with `txHash`, `reason` and `blockNumber` as structured context. - `checkpoint_proposal_job.ts` — `dropFailedTxsFromP2P` moves from `verbose` to `warn` and now includes the per-tx reason (`fail.error.message`) alongside the hash, plus `slot` / `checkpointNumber` context. - `automine_sequencer.ts` — same change to its copy of `dropFailedTxsFromP2P`, so local/dev runs behave the same. After this, every tx removed from the mempool by the builder leaves a `warn` line naming the tx and why. ## Trade-offs This raises log volume during a period of mass tx invalidation — e.g. after a reorg, when many txs are rejected with `Block header not found` in a short window. That is deliberate: those are exactly the windows where the reason matters most, and the alternative is losing the information. The drop path already deletes in batches, so it is one line per batch, not one per tx. ## Not included `invalid_txs_after_reorg_rule.ts` logs `Evicting N txs from pool due to referencing pruned blocks` with no tx hashes and no structured context, so eviction victims are still unnamed. That is a separate path from the builder's failed-tx drop and is left alone here. ## Testing The repo in this container has no `node_modules` and the `noir` submodule is not checked out (`yarn install` fails resolving the `@aztec/noir-acvm_js` portal dependency), so a full `yarn build` was not possible locally — CI is the verification for compilation. The edits were checked statically: `this.globalVariables` is already used for the same `blockNumber` context at `public_processor.ts:410`, `this.targetSlot` / `this.checkpointNumber` are existing fields on `CheckpointProposalJob`, and `FailedTx.error` is typed `Error`. No behavior outside logging is touched. --- *Created by [claudebox](https://claudebox.work/v2/sessions/b14cb9eb6114c0bf/jobs/4) · group: `slackbot` · requested by Santiago Palladino · [Slack thread](https://aztecprotocol.slack.com/archives/C0AU8BULZHC/p1785158724697589?thread_ts=1785158724.697589&cid=C0AU8BULZHC)* --------- Co-authored-by: Santiago Palladino <santiago@aztec-labs.com>
…mory (A-1215) (#24990) ## What The proving broker kept a full in-memory copy of every job's inputs (`jobsCache.inputsUri`) and every settled result (`resultsCache.value`), duplicating what LMDB already persists. With the inline default `ProofStore`, those URIs embed the full circuit inputs and full proofs — so the broker held every input and every proof in RAM *and* again on disk. Under load this was the largest single consumer in the prover stack (broker peaked ~7.6 GiB in a 5 TPS bench run). Now the broker keeps only scheduling metadata + settled status in memory and reads the large payloads from LMDB on demand: - **`jobsCache`** → `{id, type, epochNumber}` metadata; `inputsUri` is read from the DB when a job is dispatched to an agent (new `getProvingJobInputs`). The DB write commits before a job becomes dispatchable, so the read always hits. - **`resultsCache`** → settled *status* only (plus the small rejected `reason` / aborted). The fulfilled proof `value` is read from the DB in `getProvingJobStatus`, via a tiny transient `pendingResults` hold that covers the settle→commit window (and is *kept* on a DB-write failure, preserving the old "works even if the DB breaks" behaviour for that edge). - **`promises`** carry no payload. New DB getters `getProvingJobInputs` / `getProvingJobResult` search the (few) resident epoch databases by id, independent of the write-time epoch key. ## Concurrency The enqueue lock and the abort-revive span are unchanged: `jobsCache` and `resultsCache` remain synchronous in-memory maps, set before any `await`, so no DB read is inserted into that span. `getProvingJob` claims `inProgress` synchronously *before* reading inputs from the DB, and the enqueue-return "status at start" is computed without awaiting before the gate. The identity check on a duplicate id is now metadata-level (ids are content-addressed, so different content yields a different id). This trades the previous in-memory result resilience for memory (results are served from LMDB) — a deliberate, signed-off decision. ## Testing - `yarn build`, full `@aztec/prover-client` `proving_broker/` suite (158/158), including a new test asserting inputs and the fulfilled result are served from the DB after the in-memory payload is dropped, and the existing abort/revive concurrency test.
…out a failed-proof store (A-1517) (#25027) ## What `BrokerCircuitProverFacade` (prover-node side) kept each in-flight job's `inputsUri` in its `jobs` map from enqueue until the job settled. With the default `InlineProofStore` that URI embeds the **full circuit inputs**, and the only thing that reads it after enqueue is `backupFailedProofInputs` on failure. So when no `failedProofStore` is configured, the facade pinned every in-flight job's full inputs in memory for no purpose. This only retains `inputsUri` on the long-lived job when a `failedProofStore` is configured. The broker still receives the URI on enqueue regardless — this governs only what the facade holds onto. ## Why it matters here `PROVER_PROOF_STORE` is unset in every environment, so the facade always uses `InlineProofStore` (URI == payload). `PROVER_FAILED_PROOF_STORE` is: - **unset** on `bench-inclusion-sweep` and the staging environments → the retained inline inputs were pure dead weight; this change reclaims that memory in the environments we benchmark memory in; - **set** (GCS) on mainnet / mainnet-v4 / testnet / next-net → unchanged: those keep retaining the URI and still back up failed-proof inputs. ## Scope Follow-up to A-1215 (broker-side), flagged by @alexghr in review of #24990 (producer side keeps its own copy). This is the minimal, safe slice: it does not change prod behaviour. Bounding the in-flight inputs footprint on networks that *do* set a failed-proof store (via a file-store `PROVER_PROOF_STORE`, or fetching inputs by id at failure) remains possible future work. ## Testing - New unit test: with no `failedProofStore`, the facade does not retain `inputsUri` for an in-flight job (while the broker still receives it), and the job still completes normally. - The existing "handles proof errors" test (which configures a failed-proof store) still asserts the failed-proof backup runs. - Full `@aztec/prover-client` `proving_broker/` suite: 159/159. Build, format, lint clean.
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(foundation): always include a result key in json-rpc responses (#24840)
fix(pxe): cap fresh secret pending tag indexes to the probed window (port #24667) (#24977)
fix(archiver): resolve L2-to-L1 witness from a single store snapshot (#24754)
fix(sequencer): stop signalling already-executed governance payloads (#24764)
fix(archiver): clean up removed blocks from raw rows and ownership-check tx-effect deletes (#24765)
fix(node): warm KZG trusted setup at startup (#24775)
feat(prover-node): stop caching checkpoint txs; re-fetch from the pool for failure upload (A-1216) (#24983)
fix(sequencer): log tx failure reason at warn when dropping from mempool (#25000)
feat(prover-client): stop duplicating broker job inputs/results in memory (A-1215) (#24990)
fix(prover-client): don't retain inline job inputs in the facade without a failed-proof store (A-1517) (#25027)
END_COMMIT_OVERRIDE