chore: Accumulated ports to next#24964
Open
AztecBot wants to merge 6 commits into
Open
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
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
3 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)
END_COMMIT_OVERRIDE