Add zkApp command range query - #162
Conversation
23d7ca3 to
5a4b75a
Compare
|
@Hebilicious thanks for contribution! I just briefly looked at it.. will review this PR fully shortly. Looks like our CI has some issueswith running pr on forks |
| import { BlockStatusFilter } from '../../../blockchain/types.js'; | ||
| import type { ZkappCommandDatabaseRow } from './types.js'; | ||
|
|
||
| export function getZkappCommandsQuery( |
There was a problem hiding this comment.
This 250-line query isn't executed by any test, from what i see. The unit tests only cover the pure rowsToZkappCommands and the guard functions, and the integration fixture has only failed zkApp commands (tests/integration/integration.test.ts:10), so with the status <> 'failed' filter it returns empty here.
Can you add test ?
| ON b.id = bzkc.block_id | ||
| AND bzkc.status <> 'failed' | ||
| JOIN zkapp_commands zkc ON bzkc.zkapp_command_id = zkc.id | ||
| JOIN zkapp_fee_payer_body fpb ON zkc.zkapp_fee_payer_body_id = fpb.id |
There was a problem hiding this comment.
zkapp_fee_payer_body is the one table this query JOINs that isn't in USED_TABLES (src/db/sql/events-actions/queries.ts:427). Please add it to USED_TABLES so a missing table is caught at boot rather than first query.
| WHERE | ||
| b.height >= ${from} | ||
| AND b.height < ${to} | ||
| ${ |
There was a problem hiding this comment.
Hm.. This filters purely on b.height and b.chain_status, unlike events/actions which run the recursive pending_chain walk to resolve the true chain. For CANONICAL that's fine, but for the default ALL and for PENDING, competing/orphaned blocks at the same height all come through. Either use canonical resolution logic or document this on the schema field.
| FROM block_range b | ||
| JOIN blocks_zkapp_commands bzkc | ||
| ON b.id = bzkc.block_id | ||
| AND bzkc.status <> 'failed' |
There was a problem hiding this comment.
Here you are filtering out failed commands, so the status is always 'applied' and failureReason always null. The two output fields are effectively constant and looks like there is no point returning them . Given the PR scope is "successful" commands, simplest is to drop both fields.
| tracingState: new TracingState(graphQLSpan), | ||
| }); | ||
| }, | ||
| zkappCommands: async (_, { input }, context) => { |
There was a problem hiding this comment.
events/block-details are gated behind ENABLE_BLOCK_TRANSACTION_DETAILS; this heavier query is exposed ungated. is it intentional?
| """ | ||
| Mina block height to filter zkApp commands to, exclusive | ||
| """ | ||
| to: Int! |
There was a problem hiding this comment.
NIT: from before to reads more naturally.
|
@Hebilicious overall looking good! Just couple comments |
|
@dkijania thanks for the review, I will make the changes you suggest and we will test this on our archive internally. |
5a4b75a to
3d391d9
Compare
|
Really nice addition — requiring explicit One coupling worth reconsidering, building on @dkijania's gating question: the query is gated behind Would a dedicated, default-off flag be cleaner, so the two are independent? const ENABLE_ZKAPP_COMMANDS_QUERY =
process.env.ENABLE_ZKAPP_COMMANDS_QUERY === 'true';
// ...
if (!ENABLE_ZKAPP_COMMANDS_QUERY) {
enabledQueries = enabledQueries.filter((q) => q !== 'zkappCommands');
}That keeps operators who enable block-transaction details for the Explorer from unintentionally exposing the range query, while still letting you opt in explicitly. (Also +1 on @dkijania's coverage note — the added integration test only hits the |
|
Verdict: DRAFT — not assessed for merge Second-pass review, deliberately additive: I'm not re-litigating the six existing threads, only recording what the force-push actually landed and then covering the angle none of them touch — downstream compatibility with Prior review points — status at head
|
| Thread | Status at head |
|---|---|
zkapp_fee_payer_body missing from USED_TABLES |
Done — plus 7 more (public_keys, tokens, zkapp_updates, zkapp_account_precondition, zkapp_network_precondition, zkapp_global_slot_bounds, zkapp_states_nullable) at src/db/sql/events-actions/queries.ts:427-450 |
ALL/PENDING doesn't do the recursive pending_chain walk |
Done — the non-canonical branch of blockRangeCte (src/db/sql/zkapp-commands/queries.ts:35-144) is the same RECURSIVE pending_chain + full_chain shape as blockCTE in events-actions/queries.ts:15-72; orphans are excluded |
status / failureReason are constants, drop them |
Done — neither is selected in SQL nor present on ZkappCommandOutput (schema.graphql:193-201) |
| Should the heavier query be gated? | Done — gated on ENABLE_BLOCK_TRANSACTION_DETAILS at src/resolvers.ts:74-77. @SanabriaRusso's follow-up (give it its own flag so enabling block-transaction details for the Explorer doesn't also expose the range query) is still open |
from before to nit |
Done — schema.graphql:79,83 |
Nothing executes getZkappCommandsQuery |
Partially — tests/integration/setup.ts:91-144 now inserts one applied command and integration.test.ts:405-429 asserts on it; still canonical-only / one account update / empty events, so the ALL+PENDING path and the action/event/appState/precondition extraction remain uncovered |
Both red checks are fork-permission infra, not your code (Check Schema: Resource not accessible by integration creating the check run; build-and-deploy: couldn't find remote ref branch/ext-21-…). Consequence worth knowing: graphql-inspector never actually ran a breaking-change check on this PR, so nobody has an automated verdict on the schema delta. Manual one follows.
Schema delta vs main — purely additive, no consumer break
I diffed schema.graphql@3d391d97 against main. +77 / -0, and every addition is a new coordinate:
- New input
ZkappCommandFilterOptionsInput(schema.graphql:74-93) - New types
ZkappFieldArray,ZkappAccountPrecondition,ZkappGlobalSlotBounds,ZkappNetworkPrecondition,ZkappAccountUpdateOutput,ZkappCommandOutput(schema.graphql:158-201) - One new root field
zkappCommands(input: ZkappCommandFilterOptionsInput!): [ZkappCommandOutput!]!(schema.graphql:295)
No type/field/argument renamed or removed, no nullability tightened, no existing input field made required, no enum value dropped. Confirmed specifically:
ZkAppCommandis untouched. The new type isZkappCommandOutput(schema.graphql:193), a different name from the existingZkAppCommand(schema.graphql:261).ZkAppCommand.feePayeris stillString!(schema.graphql:263) andstatus/failureReasonare still top-level. This is the thing I most expected a new-feature PR to break, and it doesn't: the FLAT shape both consumers depend on survives —mina-explorer/src/services/api/blocks.ts:142-149(zkappCommands { hash feePayer fee memo status failureReason }, thenblocks.ts:359-369readscmd.feePayeras an address string), andmina-explorer-api/app/upstream/archive.py:11-12whose docstring pins the same contract.BlockTransactions.zkappCommandsstill resolves to[ZkAppCommand!]!— a different schema coordinate from the new root field, so the explorer's nested query is unaffected.BlockQueryInput/EventFilterOptionsInput/ActionFilterOptionsInputuntouched, soinBestChaincapability detection (mina-explorer/src/services/api/bestChainFilter.ts,mina-explorer-api/app/upstream/archive.py) and o1js'sfetchActionsare unaffected.- Nice side effect of the gate: with
ENABLE_BLOCK_TRANSACTION_DETAILSunset the field is stripped from the printed SDL (src/resolvers.ts:88-101), so a client asking for it gets verbatimCannot query field "zkappCommands" on type "Query"— which is exactlySCHEMA_ERROR_MARKERSinmina-explorer-api/app/upstream/graphql.py:32. Graceful, no masking. (The sixZkapp*types stay in the SDL as orphans when disabled — harmless, just cosmetic in introspection.)
The one substantive finding: the account-update guard doesn't bound work when a filter is set
ZKAPP_COMMAND_RANGE_SIZE=1000 and the preflight ZKAPP_COMMAND_ACCOUNT_UPDATE_LIMIT=5000 are a good design, and the unfiltered count path is genuinely cheap — SUM(cardinality(zkc.zkapp_account_updates_ids)) reads the array length off zkapp_commands without expanding anything (src/db/sql/zkapp-commands/queries.ts:323-333). That's the right idea.
But the filtered branch (queries.ts:335-359) loses that property twice over:
- It does the full
unnest … WITH ORDINALITY+zkapp_account_update→zkapp_account_update_body→account_identifiers→public_keys→tokensexpansion across the whole block range before applyingaccount_update_pk.value = $1. So the "cheap preflight" costs the same order of magnitude as the query it is guarding — the request pays the expansion twice (count, then main). - It returns the post-filter count. For a public key with few or no account updates in the range, the count comes back ~0, the guard never fires, and
ZKAPP_COMMAND_ACCOUNT_UPDATE_LIMITbounds results rather than work.
Concretely: zkappCommands(input: { from: H-1000, to: H, accountPublicKey: "B62q…<key that appears nowhere>" }) returns [] instantly to the caller while expanding every account update in 1000 mainnet blocks, twice, server-side. Cheap to issue, expensive to serve, and it passes every existing guardrail. There's no rate limiting merged yet and no statement_timeout anywhere in the repo (grep -rn statement_timeout src docs → nothing), so the client-side 20s abort in both consumers doesn't stop the query — it keeps running on the pool.
This isn't helped by indexes: zkapp_account_update_body.account_identifier_id has an FK but no index (tests/integration/fixtures/archive_db.sql:6194 is the constraint; there's no matching CREATE INDEX), so the planner can't drive the filter from the public_keys.value unique index backwards. The range side is fine — idx_blocks_height, idx_chain_status, idx_blocks_zkapp_commands_block_id all exist, and every other join is a PK lookup.
Suggested fix, small and self-contained: always run the cheap cardinality count to bound the raw expansion, and only then run the filtered count if a filter is present.
// src/services/zkapp-commands-service/zkapp-commands-service.ts
const [{ count: rawCount }] = await getZkappCommandAccountUpdateCountQuery(
this.client, blockStatus, range.to, range.from // no filter args -> cardinality path
);
assertZkappCommandAccountUpdateLimit(Number(rawCount));
if (accountPublicKey || tokenId) {
const [{ count: filteredCount }] = await getZkappCommandAccountUpdateCountQuery(
this.client, blockStatus, range.to, range.from,
accountPublicKey?.toString(), tokenId?.toString()
);
assertZkappCommandAccountUpdateLimit(Number(filteredCount));
}That makes the guard O(commands in range) in all cases and bounds work, not just output. A regression test would be: filtered request over a range whose raw expansion exceeds the limit but whose filtered count is 0 → must throw the block-range error, not return [].
Coordination with #183 (query-cost limits)
I checked the new type against the in-flight defaults (GRAPHQL_MAX_DEPTH=10, MAX_ALIASES=15, MAX_TOKENS=1000, MAX_COST=5000) — no conflict:
- Deepest selection is
zkappCommands → accountUpdates → accountPrecondition → state → fields= depth 5, same as the existing deepest query. P0: Add GraphQL query-cost controls via graphql-armor (#164) #183's "~5 levels" claim stays true after this PR. - Full-fat single-operation cost under graphql-armor's default heuristic works out to ~135, so even 15 aliases lands near ~2k, under 5000.
Only real interaction is GRAPHQL_MAX_TOKENS=1000: a client selecting the entire ZkappCommandOutput tree is ~60-70 tokens, so plenty of headroom, but whichever of these two lands second should re-run the other's worst-case query once. Worth a note in #183's description either way.
Non-blocking nits
src/resolvers.ts:15-16re-readsprocess.env.ENABLE_BLOCK_TRANSACTION_DETAILSeven thoughsrc/server/server.tsalready exports the parsed constant. Importing it would keep one source of truth (and would matter if the parsing ever gains a fallback).Number(process.env.ZKAPP_COMMAND_RANGE_SIZE) || 1000(server.ts:17-18) silently maps0→1000and accepts negatives. P0: Add GraphQL query-cost controls via graphql-armor (#164) #183 explicitly handles malformed env by falling back to the default; matching that here (const n = Number(...); const v = Number.isInteger(n) && n > 0 ? n : 1000;) would make the three range knobs behave consistently.- Adding 8 tables to
USED_TABLESwidens the boot-time hard fail incheckSQLSchema(src/db/archive-node-adapter/archive-node-adapter.ts:89-95) for a feature that is off by default — an archive on an older dump missing e.g.zkapp_states_nullablewould now refuse to start where it previously ran fine. All 8 are in the standard Berkeley+ schema so this is low risk, but gating the extra names onENABLE_BLOCK_TRANSACTION_DETAILSwould keep the failure proportional to what's actually enabled. events/actionsreturn[EventOutput]!while the new field returns[ZkappCommandOutput!]!. The new one is strictly better — just noting the inconsistency in case you want a follow-up to tighten the old two.
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.
3d391d9 to
edddf4f
Compare
|
Reviewed for downstream compatibility only — this is a draft, so nothing here is a merge gate. Schema is purely additive, re-confirmed at Worth calling out: gating The one actionable finding still stands, unchanged at this head. In
So in the filtered case the guard measures the result size rather than the work. A request for a public key with no matching account updates over a 1000-block range counts Concrete fix — make the guard measure range work, not result size. The expansion cost over the range is the same regardless of the predicate, so the filtered branch shouldn't exist at all. Collapse the function to the unfiltered form and drop the two now-unused parameters: export function getZkappCommandAccountUpdateCountQuery(
dbClient: postgres.Sql,
status: BlockStatusFilter,
to: number,
from: number
) {
return dbClient<{ count: string }[]>`
WITH ${blockRangeCte(dbClient, status, to, from)}
SELECT COALESCE(SUM(cardinality(zkc.zkapp_account_updates_ids)), 0)::text AS count
FROM block_range b
JOIN blocks_zkapp_commands bzkc
ON b.id = bzkc.block_id
AND bzkc.status <> 'failed'
JOIN zkapp_commands zkc ON bzkc.zkapp_command_id = zkc.id;
`;
}and drop the two trailing arguments at the call site in Pair it with the supporting index, which the filtered path needs regardless: CREATE INDEX CONCURRENTLY IF NOT EXISTS
zkapp_account_update_body_account_identifier_id_idx
ON zkapp_account_update_body (account_identifier_id);CI note — worse than at the last review. No checks have run against the current head; the head SHA has zero check-runs. All six workflow runs from today sit in For context on why the additive property matters here: |
e31e139 to
72b8820
Compare
72b8820 to
3d9f97f
Compare
SanabriaRusso
left a comment
SanabriaRusso
left a comment
There was a problem hiding this comment.
Round-4 review — comment only (this is a draft)
Re-verified from scratch at head 3d9f97fa. Nothing from round 3 is carried forward on trust: the head moved three times since then (edddf4ff → e31e1395 → 72b8820f → 3d9f97fa) and main moved by eleven merges, so every claim below was re-executed against the current head.
1. Rebase state: clean. No action needed.
git merge-base origin/main refs/remotes/pr/162 -> 3fd33794 (== origin/main HEAD)
git rev-list --count origin/main..pr/162 -> 2
git rev-list --count pr/162..origin/main -> 0
git merge-tree origin/main pr/162 -> 7e4c9980, exit 0, no conflict markers
The branch already contains all of main, including graphql-yoga 5, Node 22, rate limiting, metrics, readiness, graceful shutdown, explicit error masking and docs/versioning.md. It builds and lints against that main:
npm run build -> clean (tsc, engines node >=22.12.0, graphql-yoga 5.21.2)
npm run lint -> clean
npm run test:unit -> 175 assertions, 0 failures, incl. 12 new ZkappCommandsService tests
mergeStateStatus: BLOCKED is the draft status plus the missing checks, not a merge conflict.
2. Schema additivity: mechanically confirmed at 3d9f97fa. 0 breaking changes.
schema.graphql is +77 / -0 — zero deletions. Because CI has never evaluated this branch (see §3), I ran the breaking-change analysis by hand, implementing every category docs/versioning.md names as breaking (type/field/arg/enum-value removal, field type change, output T!→T, input T→T!, required arg added to an existing field, root type rename) over the two built schemas:
OLD: origin/main:schema.graphql NEW: pr/162:schema.graphql
BREAKING (0):
ADDITIONS (8):
+ field added: Query.zkappCommands: [ZkappCommandOutput!]!
+ type added: ZkappCommandFilterOptionsInput (input)
+ type added: ZkappFieldArray, ZkappAccountPrecondition, ZkappGlobalSlotBounds,
ZkappNetworkPrecondition, ZkappAccountUpdateOutput, ZkappCommandOutput
ZkAppCommand (capital A) and its flat feePayer: String! are untouched — schema.graphql:261-268 in this branch is byte-identical to main. BlockTransactions.zkappCommands: [ZkAppCommand!]! is untouched. Query.zkappCommands is a genuinely distinct coordinate from BlockTransactions.zkappCommands; they share a name but not a type, which is legal and does not confuse a Cannot query field probe.
2b. The flag gating and the resolvers.ts rewrite are behaviour-preserving. Verified by execution, not by reading.
I built both trees and printed the served schema under six env combinations:
| case | ENABLED_QUERIES |
ENABLE_BLOCK_TRANSACTION_DETAILS |
main Query fields | PR Query fields |
|---|---|---|---|---|
| A | unset | unset | actions, blocks, events, networkState | identical |
| B | unset | true |
actions, blocks, events, networkState | + zkappCommands |
| C | events,actions |
unset | actions, events | identical |
| D | events,actions |
true |
actions, events | identical |
| E | …,zkappCommands |
unset | actions, blocks, events, networkState | identical (flag wins — fails closed) |
| F | "" |
unset | (empty) | identical |
Case E is the one that matters for the policy: even when an operator explicitly lists zkappCommands in ENABLED_QUERIES, the flag being off still removes it. Good — it fails closed.
Diffing the two served SDLs in case A gives BREAKING (0), ADDITIONS (7) — the 7 new types remain present as unreachable types, but Query is field-identical to main. So a default deployment of this branch serves exactly main's query surface.
2c. Error-message contract (docs/versioning.md → "Error messages and validation behaviour"): unchanged, verified string-for-string.
Ran validate(schema, parse(q)) against both trees, both flag states:
| probe | main | PR (flag off) | PR (flag on) |
|---|---|---|---|
{ zkappCommands(input:{from:1,to:2}){hash} } |
Cannot query field "zkappCommands" on type "Query". |
same | (valid) |
explorer flat fallback blocks{transactions{zkappCommands{hash feePayer fee memo}}} |
valid | valid | valid |
explorer nested probe …zkappCommands{zkappCommand{memo}} |
Cannot query field "zkappCommand" on type "ZkAppCommand". |
same | same |
events(input:…, bogus: 1) |
Unknown argument "bogus" on field "Query.events". |
same | same |
query Q($x: NoSuchType!) |
Unknown type "NoSuchType". |
same | same |
blocks(query:{inBestChain:true}) |
valid | valid | valid |
All three of mina-explorer-api's SCHEMA_ERROR_MARKERS (app/upstream/graphql.py:33-42) are produced verbatim and unchanged. The new BLOCK_RANGE_ERROR messages contain none of those three substrings, so they cannot poison that consumer's capability cache.
Downstream impact: none. mina-explorer reads blocks{transactions{zkappCommands}} at src/pages/ZkAppsPage.tsx:35,66, src/services/api/blocks.ts:70,84, src/components/dashboard/RecentBlocks.tsx:19 and src/components/blocks/BlockDetail.tsx:218 — all on BlockTransactions, none on Query, none touched here. The "Cannot query field" probes at transactions.ts:488,556,698,977 and ZkAppsPage.tsx:183 still fire on exactly the same inputs with exactly the same text.
3. ⚠️ Check Schema has never evaluated this branch — and it structurally cannot, for any fork PR. This is a repo bug, not a PR bug.
Round 3 reported zero check-runs. That is still true today:
gh pr checks 162 -> "no checks reported on the branch"
gh api .../commits/3d9f97fa/check-runs -> {"total_count":0,"check_runs":[]}
gh api .../commits/3d9f97fa/status -> state "pending", 0 statuses
All seven workflow runs at this SHA (2026-08-27T20:40:05Z) sit at conclusion: action_required, awaiting a maintainer's "Approve and run workflows". Same for e31e1395, 72b8820f and edddf4ff.
But approving them is not sufficient, and this is the new finding. The one time Check Schema did run on this branch — run 28152520334, at 3d391d97, 2026-07-03 — it failed, and not on the schema:
GraphQL Inspector started
Creating a check named "GraphQL Inspector"
##[error]Resource not accessible by integration
- https://docs.github.com/rest/checks/runs#create-a-check-run
kamilkisiela/graphql-inspector@master reports by creating a check-run. .github/workflows/graphql-inspector.yaml declares no permissions: block, and a pull_request event from a fork gets a read-only GITHUB_TOKEN regardless of the repo default (which is write here). So the action cannot post, and the job fails.
Corroborated twice, and it is still live today:
| branch | fork? | Check Schema |
|---|---|---|
feat/structured-logging, docs/security-deployment (same-repo) |
no | success |
dependabot/github_actions/… @ f2700c6b, 2026-09-02 |
read-only token | failure — same Resource not accessible by integration |
branch/ext-21-zkapp-command-range-query (this PR) |
yes | never ran |
feat/verification-key-updates (#225) |
yes | never ran |
So docs/versioning.md → "Enforcement" ("Schema changes are checked in CI by graphql-inspector… A change it flags as breaking fails the build") does not hold for external contributions — precisely the population where it matters most. Both open schema-changing PRs are external, and neither has ever been checked.
Suggested repo fix (needs no token privileges at all — drop the check-run API call and let the job's exit status be the signal):
# .github/workflows/graphql-inspector.yaml
permissions:
contents: read
jobs:
test:
name: Check Schema
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with: { fetch-depth: 0 }
- uses: actions/setup-node@v4
with: { node-version: '22' }
- name: Diff schema against main
run: |
git fetch origin main --depth=1
git show origin/main:schema.graphql > /tmp/base.graphql
npx --yes @graphql-inspector/cli diff /tmp/base.graphql schema.graphql@graphql-inspector/cli diff exits non-zero on a breaking change, which fails the job directly. Keep the expected-breaking-change escape hatch by guarding that step with if: !contains(github.event.pull_request.labels.*.name, 'expected-breaking-change').
Until then, my §2 result is the only breaking-change analysis this branch has ever had.
4. docs/versioning.md compliance
| rule | verdict |
|---|---|
| Schema additions only → minor | ✅ 0 breaking, 8 additions (§2) |
| Flag-gating: new exposed query ships default-off behind an env flag | ✅ default-off verified (§2b case A) — |
| Validation/parse error text verbatim | ✅ all six probes byte-identical (§2c) |
| Error masking covers thrown runtime errors only | ✅ untouched; throwBlockRangeError → BLOCK_RANGE_ERROR / 400 with errors[] present |
| No env var removed/renamed, no default changed | ✅ three new vars, all documented in README.md, docs/getting-started.md:189-190, .env.example.compose |
| No endpoint/runtime change | ✅ |
| Deprecations | n/a |
Net: this is a minor-shaped change and complies, with the one caveat in Finding 2.
Finding 1 (open, carried from round 3 — partially addressed, and the fix introduced a new and larger problem)
For a development AI agent picking this up.
What changed since round 3
Commit 3d9f97fa ("fix(sql): prefilter zkapp command account-update counts") addressed half of it. Credit where due: the second full expansion is gone. Both the count query and the data query now prefilter, so the zero-match case no longer unnests every account-update array in the range twice. That part is genuinely fixed.
What is still open, and what got worse
1a. The guard still measures RESULT SIZE, not RANGE WORK.
src/db/sql/zkapp-commands/queries.ts:394-405 — the filtered branch counts matching account updates. src/services/zkapp-commands-service/zkapp-commands-service.ts:95-103 then compares that to ZKAPP_COMMAND_ACCOUNT_UPDATE_LIMIT. A filter can only ever reduce the output; it cannot reduce the work of walking the range. So the guard does not bound what it exists to bound.
Note that the unfiltered branch at queries.ts:382-391 already gets this exactly right — SUM(cardinality(zkc.zkapp_account_updates_ids)), no joins, no expansion. The correct fix is to use that form in all cases.
1b. NEW — target_account_updates is not bounded by the block range at all.
queries.ts:171-177:
target_account_updates AS (
SELECT zkau.id AS account_update_id
FROM matching_account_identifiers mai
JOIN zkapp_account_update_body zkub ON zkub.account_identifier_id = mai.id
JOIN zkapp_account_update zkau ON zkau.body_id = zkub.id
)There is no from/to anywhere in it. This selects every account update in the entire archive that touches the matching account identifiers, and only then intersects with the range via tau.account_update_id = ANY(zkc.zkapp_account_updates_ids) (queries.ts:195-196). Two consequences:
- For a hot zkApp account, the cost scales with the account's whole chain history, not with the requested range. A one-block query can trigger it. The
ZKAPP_COMMAND_RANGE_SIZEguard is bypassed entirely for this path. - Worse,
tokenIdalone:matching_account_identifiers(queries.ts:158-169) filterstokens.value, and the default MINA tokenwSHV2S4qX9jFsLjQo8r1BsMLH2ZRKsZx6EJd1sbozGPieEC4Jfis carried by essentially every account in the ledger. SozkappCommands(input: {from: 1, to: 2, tokenId: "wSHV2S4qX9jFsLjQo8r1BsMLH2ZRKsZx6EJd1sbozGPieEC4Jf"})— a two-block window — resolves that CTE to the whole account-identifier table and walks all ofzkapp_account_update_body. That is a one-line unauthenticated request that pins a connection untilPG_STATEMENT_TIMEOUT.
And = ANY(array) is not a hash-joinable condition, so the planner's options are a materialised scan of the whole set or a parameterised nested loop; there is no MATERIALIZED / NOT MATERIALIZED hint to steer it and no measurement in the PR. I want to be straight about the limits of my evidence here: the pathological plan is reasoned from the SQL, not measured — I have no mainnet archive to EXPLAIN against, and the 135-row test fixture would seq-scan either way. What is not reasoned is the absence of a range bound in that CTE; that is plainly visible at queries.ts:171-177. Acceptance criterion 4 below asks you to settle it with a real EXPLAIN (ANALYZE, BUFFERS).
1c. No index backs any of this. I listed every index in the canonical archive schema (tests/integration/fixtures/archive_db.sql:5727-5874 — 22 of them). None is on zkapp_account_update_body.account_identifier_id; the table carries only PRIMARY KEY (id) (line 5444-5448). There is an FK on that column (line 6190-6194) but Postgres does not index FK columns. Same story for zkapp_account_update.body_id. See §6 — this is a shared ask with #225, not a #162-only one.
Suggested fix
Patch A — one guard, measuring range work, for every case. Delete the filtered branch of getZkappCommandAccountUpdateCountQuery and its two filter parameters:
export function getZkappCommandAccountUpdateCountQuery(
dbClient: postgres.Sql,
status: BlockStatusFilter,
to: number,
from: number
// No accountPublicKey / tokenId: a filter reduces the OUTPUT, never the RANGE
// WORK this guard exists to bound. Guarding on the unfiltered cardinality is
// both cheaper (no joins at all) and the quantity we actually care about.
) {
return dbClient<{ count: string }[]>`
WITH ${blockRangeCte(dbClient, status, to, from)}
SELECT COALESCE(SUM(cardinality(zkc.zkapp_account_updates_ids)), 0)::text AS count
FROM block_range b
JOIN blocks_zkapp_commands bzkc
ON b.id = bzkc.block_id
AND bzkc.status <> 'failed'
JOIN zkapp_commands zkc ON bzkc.zkapp_command_id = zkc.id;
`;
}and drop the two extra arguments at the call site, zkapp-commands-service.ts:95-102.
Patch B — keep the block range as the sole driver; apply the filter as a semi-join. Delete accountUpdateRowsJoin (queries.ts:181-205) entirely and reduce accountUpdateFilterCtes to just the identifier set:
function accountUpdateFilterCte(
dbClient: postgres.Sql,
accountPublicKey?: string,
tokenId?: string
) {
if (!accountPublicKey && !tokenId) return dbClient``;
return dbClient`
,
matching_account_identifiers AS MATERIALIZED (
SELECT ai.id
FROM account_identifiers ai
JOIN public_keys pk ON ai.public_key_id = pk.id
JOIN tokens t ON ai.token_id = t.id
WHERE 1 = 1
${accountPublicKey ? dbClient`AND pk.value = ${accountPublicKey}` : dbClient``}
${tokenId ? dbClient`AND t.value = ${tokenId}` : dbClient``}
)
`;
}In getZkappCommandsQuery, use the unfiltered join form unconditionally and push the filter onto the body row:
JOIN LATERAL unnest(zkc.zkapp_account_updates_ids)
WITH ORDINALITY AS account_update_ids(account_update_id, account_update_order) ON true
JOIN zkapp_account_update zkau ON zkau.id = account_update_ids.account_update_id
JOIN zkapp_account_update_body zkub ON zkau.body_id = zkub.id
${accountPublicKey || tokenId
? dbClient`AND zkub.account_identifier_id IN (SELECT id FROM matching_account_identifiers)`
: dbClient``}Then delete the now-redundant AND account_update_pk.value = … and AND t.value = … predicates on the public_keys / tokens joins (queries.ts:262-271) — the semi-join subsumes both. target_account_updates disappears; the range is the only driver; = ANY(array) is gone; and the ordinality-recovery LATERAL is no longer needed because the ordinality never left.
This is the shape #225 already uses (see §6).
Acceptance criteria
getZkappCommandAccountUpdateCountQueryno longer acceptsaccountPublicKeyortokenId, and its generated SQL matches none of/zkapp_account_update_body/,/account_identifiers/,/public_keys/,/tokens/.getZkappCommandsQuerycontains exactly oneunnest(zkc.zkapp_account_updates_ids) WITH ORDINALITY, and the string= ANY(zkc.zkapp_account_updates_ids)appears nowhere insrc/db/sql/zkapp-commands/queries.ts.- No CTE in either query selects from
zkapp_account_update_bodywithout a predicate tying it toblock_range. EXPLAIN (ANALYZE, BUFFERS)output pasted into the PR for three cases against a real archive (devnet is fine):- (a) a full
ZKAPP_COMMAND_RANGE_SIZE-block range withaccountPublicKeyset to a key with no account updates —shared readmust not exceed the same range with no filter. - (b) the same range with
tokenId=wSHV2S4qX9jFsLjQo8r1BsMLH2ZRKsZx6EJd1sbozGPieEC4Jfand noaccountPublicKey— must complete insidePG_STATEMENT_TIMEOUT, and the plan must contain no scan ofzkapp_account_update_bodythat is not parameterised by the range. - (c) a one-block range with that same token id —
shared readmust be within roughly one block's worth, i.e. it must scale with the range, not with the account's history. Today (b) and (c) are the failure cases; (c) is the sharp one.
- (a) a full
- The three tests in
tests/services/zkapp-commands-service/zkapp-commands-service.test.tsunderSQL account-update limit preflightcurrently pintarget_account_updatesand= ANY(…)and will fail after this change — that is expected. Rewrite them to assert the invariant rather than the shape: count-query text must not match/zkapp_account_update_body/; data-query text must contain exactly one/unnest\(zkc\.zkapp_account_updates_ids\)/. (Aside:makeSqlTextClientinterpolates values with bareString(v), so what those tests inspect is not whatpostgres.jsactually sends — real queries are parameterised, so there is no injection here, but do not treat that helper's output as evidence about escaping.) npm run build,npm run lint,npm run test:unitandnpm run test:integrationall pass.
Finding 2 (new this round) — the new query is gated by the wrong flag, which forces it on for every mina-explorer deployment
For a development AI agent picking this up.
src/resolvers.ts:15-16,74-78 gates Query.zkappCommands behind ENABLE_BLOCK_TRANSACTION_DETAILS. That flag already means something else and is already load-bearing for a downstream consumer:
- On main it gates block-detail output only —
src/services/blocks-service/blocks-service.ts:101(populatingtransactions.{userCommands,zkappCommands,feeTransfers}) and:397(parentHash). Documented atdocs/getting-started.md:191as "IncludeuserCommands/zkappCommands/feeTransfers". mina-explorerneeds it on.src/pages/ZkAppsPage.tsx:57-75(ZKAPP_ACTIVITY_QUERY_FLAT) readsblocks{transactions{zkappCommands{hash feePayer fee memo}}}and extracts at:156-169;src/services/api/blocks.ts:70,84andsrc/components/blocks/BlockDetail.tsx:218do the same. With the flag off the query still validates and simply returns empty arrays, so the zkApps page silently shows nothing.
So any deployment serving mina-explorer must set ENABLE_BLOCK_TRANSACTION_DETAILS=true — and, with this PR, thereby also exposes a brand-new range query with the unbounded-work path of Finding 1, whether or not the operator wants it. My case-B/case-E table in §2b is the proof: flipping that one variable is what adds zkappCommands to the served Query. ENABLED_QUERIES is an escape hatch, but it defaults to "everything", so the default for a mina-explorer deployment is exposed.
docs/versioning.md → "Flag-gating behaviour changes" asks each such change to ship behind an env flag; overloading an existing one means the operator cannot take one capability without the other. And neither README.md, docs/getting-started.md:191 nor .env.example.compose was updated to say the flag now also exposes a query — an operator reading the docs cannot discover this.
Suggested fix
Give it its own flag, defaulting off, in src/server/server.ts next to the others:
const ENABLE_ZKAPP_COMMAND_QUERY =
process.env.ENABLE_ZKAPP_COMMAND_QUERY === 'true';and in src/resolvers.ts replace
const ENABLE_BLOCK_TRANSACTION_DETAILS =
process.env.ENABLE_BLOCK_TRANSACTION_DETAILS === 'true';
…
if (!ENABLE_BLOCK_TRANSACTION_DETAILS) {with
const ENABLE_ZKAPP_COMMAND_QUERY =
process.env.ENABLE_ZKAPP_COMMAND_QUERY === 'true';
…
if (!ENABLE_ZKAPP_COMMAND_QUERY) {Add ENABLE_ZKAPP_COMMAND_QUERY?: string; to src/envionment.d.ts and document it in the three env tables. Leave the ENABLE_BLOCK_TRANSACTION_DETAILS row exactly as it is.
Acceptance criteria
Query.zkappCommandsis gated byENABLE_ZKAPP_COMMAND_QUERY(defaultfalse) and by nothing else.- With
ENABLE_BLOCK_TRANSACTION_DETAILS=trueandENABLE_ZKAPP_COMMAND_QUERYunset:printSchema(schema)does not containzkappCommands(, andblocks{transactions{zkappCommands{hash feePayer fee memo}}}still validates. Both halves must hold — the second is the mina-explorer path. - With
ENABLE_ZKAPP_COMMAND_QUERY=trueandENABLE_BLOCK_TRANSACTION_DETAILSunset:Query.zkappCommandsis present. ENABLED_QUERIESstill fails closed: withENABLED_QUERIESlistingzkappCommandsbutENABLE_ZKAPP_COMMAND_QUERYunset, the field is absent (this is case E in §2b — preserve that behaviour).README.md,docs/getting-started.mdand.env.example.composedocument the new flag; theENABLE_BLOCK_TRANSACTION_DETAILSrows are unchanged.- A unit test in
tests/unit/asserts 2, 3 and 4.
5. Non-blocking (one line each, per the "ship it" bar — do not gold-plate these)
- Filtered results return a partial command. With
accountPublicKey/tokenIdset,ZkappCommandOutput.accountUpdatescontains only the matching updates, not the whole command — but each carriescallDepth, so a consumer can easily mis-reconstruct the call tree from a subset. One sentence in theZkappCommandFilterOptionsInputdescription would close it. - Three new env vars where main has one.
ZKAPP_COMMAND_RANGE_SIZE(1000) andZKAPP_COMMAND_ACCOUNT_UPDATE_LIMIT(5000) are a third and fourth copy of thevalidateRangepattern already inevents-service.ts:72-77andactions-service.ts:146-151. #225 reuses main'sBLOCK_RANGE_SIZEand message text instead. ReusingBLOCK_RANGE_SIZE, or lifting one sharedassertBlockRange(from, to, max)helper, would leave one knob to reason about rather than two. (Patch A makesZKAPP_COMMAND_ACCOUNT_UPDATE_LIMITgenuinely meaningful, so that one earns its keep.) USED_TABLESis a boot gate.src/db/sql/events-actions/queries.ts:543-566adds 8 tables, andsrc/db/archive-node-adapter/archive-node-adapter.ts:106-112throwsMissing table Xand refuses to start if any is absent — unconditionally, even with the feature flag off. I verified all 8 (public_keys,tokens,zkapp_fee_payer_body,zkapp_updates,zkapp_account_precondition,zkapp_network_precondition,zkapp_global_slot_bounds,zkapp_states_nullable) exist in the canonical archive schema, so the practical risk is nil — noted only so nobody is surprised.- 7 orphan types are served when the flag is off (§2b). Harmless — introspection is off by default and
Cannot query fieldstill fires correctly — but if you want the served schema to match the enabled surface exactly, prune unreachable types in the same AST visit. - The
CHANGES_REQUESTEDfrom @dkijania (2026-06-11) is stale: @dkijania has since authored the head commit3d9f97faon this branch. Worth re-reviewing or dismissing so it stops reading as an outstanding objection.
6. Coordination with #225 ("Add bounded verification-key update query")
I reviewed #162 alongside #225 so the two authors get one story instead of two.
- No schema collision. #225 adds
VerificationKeyUpdateFilterInput,VerificationKeyUpdate,Query.verificationKeyUpdates(+25/-0); #162 adds the 7 coordinates in §2 (+77/-0). Zero name overlap, zero semantic overlap. The only conflicts are textual and append-only — theQueryblock, the insertion point afterActionFilterOptionsInput, plussrc/resolvers.ts,src/db/sql/events-actions/queries.ts,archive-node-adapter{,.interface}.tsandtests/integration/integration.test.ts. Resolve by taking both sides. - #225 already solves Finding 1's problem, and #162 should copy it. In
src/db/sql/verification-key-updates/queries.ts, #225 resolves the hash to an id-set intarget_updates AS MATERIALIZED (…), then drives the join from the bounded block range and applies the set as a semi-join predicate (zaub.update_id IN (SELECT id FROM target_updates)). Work stays bounded by the range; theMATERIALIZEDis deliberate and carries a measurement in the comment (405 ms inlined vs 199 ms materialised on a 681k-block devnet archive over a 10 000-block range). Patch B above is that same shape. #225 also reuses main'sthrowBlockRangeErrorand main'sBLOCK_RANGE_SIZErather than inventing new knobs. - The index ask is one ask, not two. From
tests/integration/fixtures/archive_db.sql:5727-5874, none of these is indexed:zkapp_account_update_body.account_identifier_id(#162),zkapp_account_update_body.update_id(#225),zkapp_account_update.body_id(both),zkapp_updates.verification_key_id(#225). Both queries sequential-scanzkapp_account_update_bodytoday. This should go to whoever owns archive-node schema as a single migration naming all four columns, credited to both PRs — not as two competing requests. - Both PRs are blocked by the same CI defect (§3): both are fork PRs, both have zero check-runs, and
Check Schemawould fail on either even after approval.
I have a message out to the #225 reviewer on two open questions — whether the 199 ms MATERIALIZED measurement was taken with a large target_updates set (if not, #225 has a milder version of the same unboundedness and I would soften the recommendation above), and whether #225 is content sharing BLOCK_RANGE_SIZE. No reply yet at the time of writing, so treat those two sub-points as provisional; everything else in this section I verified directly from the two heads.
7. What would make this mergeable — in order
Maintainer (blocking, and nobody else can do these):
- Click "Approve and run workflows" on this PR. Seven runs have been sitting at
action_requiredsince 2026-08-27. #225 needs the same. - Fix
.github/workflows/graphql-inspector.yaml(§3) soCheck Schemacan pass on a fork PR at all. Without this, step 1 buys you a red Check Schema for a PR whose schema is provably fine — worse than no signal. This also unblocks #225 and stops the recurring Dependabot failure. - Route the four-column index migration (§6) to the archive-node schema owner, as one request covering #162 and #225.
Author (blocking):
- Finding 1 — Patches A and B, plus the
EXPLAIN (ANALYZE, BUFFERS)evidence for the three cases. This is the one that would otherwise ship a one-line unauthenticated request that walks the wholezkapp_account_update_bodytable. - Finding 2 — move
Query.zkappCommandsto its ownENABLE_ZKAPP_COMMAND_QUERYflag, default off, and document it. - Update the three
SQL account-update limit preflighttests, which pin the SQL shape Patch B removes.
Author (cheap, do them in the same push):
- One sentence in the
ZkappCommandFilterOptionsInputdescription saying that a filter narrowsaccountUpdatesto the matching updates only. - Decide
ZKAPP_COMMAND_RANGE_SIZEvs reusingBLOCK_RANGE_SIZE, ideally matching whatever #225 settles on.
Then:
- Mark ready for review, and ask @dkijania to refresh or dismiss the June
CHANGES_REQUESTED(he has since authored the head commit).
Once 1, 2, 4 and 5 land, I do not see anything else standing between this and merge. The schema half of this PR is in good shape — it is provably additive, it fails closed, and it does not touch a single string, field or argument either downstream consumer depends on. The SQL half is where the remaining risk is, and it is concentrated in one CTE.
How to reproduce everything above
# rebase state
git merge-base origin/main refs/remotes/pr/162 # -> 3fd33794 == origin/main
git merge-tree origin/main refs/remotes/pr/162 # -> single tree oid, exit 0
# build / lint / unit against current main's toolchain
git archive refs/remotes/pr/162 | tar -x -C /tmp/pr162 && cd /tmp/pr162
npm run build && npm run lint && npm run test:unit
# served-schema probe, per env combination
ENABLED_QUERIES= ENABLE_BLOCK_TRANSACTION_DETAILS= \
node -e "import('./build/src/resolvers.js').then(async m => {
const {printSchema}=await import('graphql');
console.log(Object.keys(m.schema.getQueryType().getFields()).sort());
})"
# validation error strings
node -e "import('./build/src/resolvers.js').then(async m => {
const {parse,validate}=await import('graphql');
console.log(validate(m.schema, parse('{ zkappCommands(input:{from:1,to:2}){hash} }')).map(e=>e.message));
})"
# CI state
gh pr checks 162
gh api repos/o1-labs/Archive-Node-API/commits/3d9f97fa/check-runs
gh run view 28152520334 --repo o1-labs/Archive-Node-API --log-failed | grep -i "not accessible"
# indexes present in the archive schema
grep -n '^CREATE INDEX' tests/integration/fixtures/archive_db.sql
SanabriaRusso
left a comment
SanabriaRusso
left a comment
There was a problem hiding this comment.
Follow-up: the two provisional points in §6 are now answered, plus one correction to my own comment
The #225 reviewer came back on both open questions. Replacing the provisional wording in §6 of my comment above.
1. No softening needed — #225 stands as the model, with one caveat #162 should inherit. The MATERIALIZED measurement was not taken on a large target_updates set, but a large one does not exist in the data: on the current hourly devnet dump (691,103 blocks, 176,348 zkapp_updates rows), zkapp_updates is hash-consed — across all 1,593 distinct verification keys the maximum rows for any single vk hash is 564. EXPLAIN (ANALYZE, BUFFERS) on the real SQL at the full 10,000-block range: 196–265 ms for the most common vk, 277–457 ms for a vk with 260 hits, and 9–125 ms for a hash matching nothing.
That last number is the point. In #225 the miss case is the cheapest. In #162 the miss case is the pathological one — a filter that matches nothing still resolves target_account_updates over the whole archive. Exact opposite shape.
The caveat: #225 measured a synthetic scaling curve confirming cost is linear in |target set| and uncapped by the range guard (1k → 22 ms, 100k → 2.2 s, 1M → 24.5 s). It is non-blocking there only because the cardinality cannot be reached — zkapp_updates is hash-consed. zkapp_account_update_body is not. So when copying #225's shape (Patch B), copy the range-driven semi-join, not the "large set is fine" conclusion: on this table the set genuinely can be large, which is precisely why Patch B keeps block_range as the sole driver rather than relying on the filter set being small.
2. BLOCK_RANGE_SIZE: #225 shares it, explicitly — verification-key-updates-service.ts:10 imports it unchanged (used at :66-69) from server.js and adds no knob. That settles non-blocking item 2 in §5: ZKAPP_COMMAND_RANGE_SIZE should be dropped in favour of BLOCK_RANGE_SIZE and the shared throwBlockRangeError message text. (ZKAPP_COMMAND_ACCOUNT_UPDATE_LIMIT still earns its keep once Patch A makes it measure range work.)
3. Indexes: withdrawing my version. #225's reviewer has taken ownership of the joint four-column ask, attributed to both PRs, proposed for docs/runbook.md. Use §6 of the #225 review as the single source; ignore my §6 phrasing so there is one request, not two. They independently confirmed zkapp_updates.verification_key_id is unindexed and measured a 21.5 ms range-independent floor from it.
4. Correction to my own comment. In §4 I wrote that throwBlockRangeError yields "BLOCK_RANGE_ERROR / 400". The 400 is misleading: src/errors/error.ts:10-16 puts it in extensions.status, and graphql-yoga only honours extensions.http.status — so the wire status is HTTP 200 with a populated errors[] and extensions.code, verified against a live server by the #225 reviewer.
This is better news than what I wrote. mina-explorer-api (app/upstream/graphql.py) opens its per-endpoint circuit breaker on a 4xx with no errors[]; a 200 with errors[] cannot trip it. #162 inherits this automatically since it uses the same throwBlockRangeError. So a client hammering zkappCommands with an over-limit range degrades gracefully instead of taking the archive tier dark. Nothing to change — I just described the mechanism wrongly.
For the record: #225 was approved, with a strictly additive schema change (0 removed lines in schema.graphql and src/resolvers-types.ts). Everything else in my comment above stands as written.
3 participants
Closes #161.
Adds
Query.zkappCommandsfor fetching successful zkApp commands over an explicit block range, including block info, fee payer data, and ordered account updates with actions, events, app state, and preconditions.Guardrails:
fromandto; there is no implicit latest-range scanZKAPP_COMMAND_RANGE_SIZEfor this heavier query, defaulting to1000ZKAPP_COMMAND_ACCOUNT_UPDATE_LIMIT, defaulting to5000accountPublicKey/tokenIdfiltered countsAlso adds service tests for the range and account-update guardrails.