chore: Accumulated ports to next - #25053
Merged
Merged
Conversation
…25028) ## Context `SiblingPath.deserialize` read a 32-bit length prefix and then both pre-allocated an array of that size and looped that many times, with nothing tying the declared length to the bytes actually present. Its element reader was built on `Buffer.slice`, which clamps past the end of a buffer instead of throwing — so the loop never ran out of input and ran the full declared count regardless of how much data arrived. A 4-byte field was therefore enough to build a million-element path, or to walk a client's V8 heap into a fatal out-of-memory abort. That abort is not catchable, so a `try`/`catch` around RPC decoding does not contain it. This matters because sibling paths reach clients as RPC *outputs* (witness lookups, which the PXE fetches automatically during private execution), so it is the node that can crash the client, not the other way round. ## Approach Sibling paths now deserialize through `BufferReader.readVector`, which bounds the declared length three ways: an explicit `maxSize` (`MAX_SIBLING_PATH_LENGTH`, 128 — the deepest protocol tree is 42 levels and the stacked path for L2-to-L1 message inclusion spans four unbalanced trees, so real paths are well under it), a new check that the size does not exceed the bytes left after the prefix, and `readBytes`' own per-element range check. The last of those also closes the truncation half of the problem, where a path declaring more elements than its payload holds used to deserialize into undersized buffers instead of failing. The cap has to live in `foundation`, which cannot import `@aztec/constants` since `constants` depends on it, so a test in `stdlib` asserts the cap stays above every generated `*_HEIGHT` constant. Enumerating them rather than listing the deep ones means a future deeper tree fails that test instead of silently failing to deserialize. `deserializeArrayFromVector`, the primitive behind the old code, had exactly one caller and duplicated `readVector` minus all of those checks, so it is deleted rather than fixed. The remaining-bytes bound lands in `readVector` itself, which covers its other callers too: every element consumes at least one byte, so any size beyond the bytes remaining is unsatisfiable and can be rejected before allocating or looping. It was not exploitable there in the same way (its item deserializers all range-check as they read), but the guard makes the failure explicit and cheap rather than dependent on every element reader. That bound assumes a minimum of one byte per element, so I checked for anything that deserializes a count of sub-byte items: `BitVector` is the only bit-packed reader, and it converts its bit count to `ceil(length / 8)` bytes itself and reads through the range-checked `readBytes`, so it never reaches the bound with a bit-denominated length. Every other `readVector` element reader in the tree advances by whole bytes. One existing `readVector` test changed: it asserted that a 32-byte buffer declaring 65537 elements yields 65537 elements, using a deserializer that consumed no bytes. That amplification is what the bound removes, so the happy path is now exercised against a well-formed vector and the oversized case asserts the throw. Fixes A-1522
Two public node RPC inputs reached expensive work with no length cap.
`simulatePublicCalls` accepted an unbounded `overrides.publicStorage`
array (and an unbounded
`overrides.contracts` record). Each storage override becomes a leaf
inserted into a forked public
data tree before the simulated transaction runs, so a small request
could force a large amount of
Merkle work and memory that the transaction's gas limit does not meter.
`getCheckpointsData` range queries validated `limit` with only `min(1)`,
while the sibling
`getCheckpoints` capped it. The effective upper bound was the node's
contiguous checkpoint history
rather than a page size, so one request could ask the archiver to read
and serialize the whole chain.
## Approach
Both caps live in `stdlib/src/interfaces/api_limit.ts` next to the
existing `MAX_RPC_*` limits and are
enforced in the zod schemas, so they reject at the RPC boundary before
any handler runs:
- `MAX_RPC_PUBLIC_STORAGE_OVERRIDES_LEN` (200) and
`MAX_RPC_CONTRACT_OVERRIDES_LEN` (50) on
`SimulationOverrides.schema`. Internal callers of the override path use
a handful of entries at
most (`fastforwardContractUpdate` writes one delayed-public-mutable
value), so there is ample
headroom.
- `MAX_RPC_CHECKPOINTS_DATA_LEN` (200) on both range variants of
`CheckpointsQuerySchema`
(`{ from, limit }` and `{ fromSlot, limit }`). Checkpoint data carries
no attestations or block
bodies, hence a larger page than a full checkpoint response. The widest
internal caller is the
prover node's catch-up fetch, bounded by `(proofSubmissionEpochs + 1) *
epochDuration`.
The existing `MAX_RPC_TXS_LEN`, `MAX_RPC_BLOCKS_LEN` and
`MAX_RPC_CHECKPOINTS_LEN` page caps stay at
50 and are unchanged. Two ideas from earlier revisions were dropped:
raising those caps to 100 while
adding a tighter `MAX_RPC_HEAVY_LEN` for reads whose `include*` options
attach a tx body or proof to
every entry, which needed a refine on four schemas plus a divergent
default for `getPendingTxs`; and
capping the per-slot attestation arrays at the committee size, which is
not actually an upper bound
since equivocation can produce more than one attestation per validator
per slot.
Tests cover each cap at the schema level (`SimulationOverrides`,
`CheckpointsQuerySchema`) and as
round-trips through the JSON-RPC client and server in
`aztec-node.test.ts`.
Fixes A-1524
Fixes A-1523
…25029) ## Context `Tx.schema` accepted `contractClassLogFields` and `publicFunctionCalldata` arrays of any length, and the `HashedValues.schema` those calldata entries are built from accepted any number of values. Three unauthenticated node RPC methods take a full tx — `sendTx`, `isValidTx` and `simulatePublicCalls` — so until now the only thing bounding those arrays at the RPC boundary was `RPC_MAX_BODY_SIZE`. A caller could post a tx whose calldata array is orders of magnitude larger than any real tx, and the node would deserialize all of it and compute the tx hash before protocol-level validation rejected it. Follow-up to #25026, which capped simulation overrides and checkpoint-data range queries. ## Approach Each array is capped at what the protocol can actually produce: - `publicFunctionCalldata` → `MAX_ENQUEUED_CALLS_PER_TX + 1` (33). One entry per enqueued call, plus the teardown call. `MAX_ENQUEUED_CALLS_PER_TX` is a whole-tx bound rather than a per-phase one: the revertible and non-revertible call request arrays are each sized `MAX_ENQUEUED_CALLS_PER_TX` in the circuit ABI, but `split_to_public` in the private tail fills them by *partitioning* a single `ClaimedLengthArray<_, MAX_ENQUEUED_CALLS_PER_TX>`, so their lengths sum to 32 rather than reaching it each. The teardown request is a separate field that "can only be set once", hence the `+ 1`. `mockTx` with its defaults and a teardown call produces exactly 33. - `contractClassLogFields` → `MAX_CONTRACT_CLASS_LOGS_PER_TX` (1). Same partitioning argument: `getNonEmptyContractClassLogsHashes` concatenates both accumulated data sets, but they are filled from one array of that size. - The fields within a calldata entry → `MAX_FR_CALLDATA_TO_ALL_ENQUEUED_CALLS` (16000). A tx cannot spend more than its whole calldata budget on a single call. That also clears every producer by a wide margin: the largest payload the client builds is packed bytecode for a contract class publication, bounded by `MAX_PACKED_PUBLIC_BYTECODE_SIZE_IN_FIELDS` (3000). The last of those is applied through a new `HashedValues.schemaFor(maxValues)` rather than baked into `HashedValues.schema`, because that schema is shared with `TxExecutionRequest.argsOfCalls`, `PrivateExecutionResult` and the wallet's `extraHashedArgs` — a public calldata budget is not the right bound for private call arguments or authwit arguments. `Tx.schema` is the only caller that opts in. ## Note for reviewers `Tx.schema` is also used to deserialize txs from the tx file store, so an over-tight bound would reject previously-valid stored data rather than just bad RPC input. The values above are protocol ceilings, so stored and gossiped txs stay parseable; the only behavior change is that a tx exceeding them is now rejected while parsing instead of later, by `DataTxValidator`. Note that gossip and the reqresp protocols go through `Tx.fromBuffer`, not the schema, so validator coverage there is unchanged. Fixes A-1527
Collaborator
Author
|
🤖 Auto-merge enabled after 8 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
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): bound deserialized sibling path and vector lengths (#25028)
fix(rpc): cap simulation overrides and checkpoint queries (#25026)
fix(stdlib): cap the unbounded arrays in Tx and HashedValues schemas (#25029)
END_COMMIT_OVERRIDE