Add bounded verification-key update query - #225
Conversation
| type Query { | ||
| events(input: EventFilterOptionsInput!): [EventOutput]! | ||
| actions(input: ActionFilterOptionsInput!): [ActionOutput]! | ||
| verificationKeyUpdates( |
There was a problem hiding this comment.
Flagging a policy question for us, not a change request for you — no action needed on your side @mellowcroc
docs/versioning.md says changes that alter "the set of exposed queries" ship "disabled by default behind an environment flag". ENABLED_QUERIES is an allowlist only when it is set, so by default this new query is exposed. Given the cost numbers in my comment on the SQL, we may decide this ships default-off at first.
Separately: a new query is a MINOR change under that same policy, so package.json needs 1.1.0. We can fold that into the release, so leave it unless we ask.
|
|
||
| return client.unsafe( | ||
| ` | ||
| WITH RECURSIVE pending_chain AS ( |
There was a problem hiding this comment.
No change needed from you — noting this so it is on record.
This is the third implementation of the pending-chain walk in the repo. The other two are fullChainCTE and getZkappsWithPendingEventsQuery in src/db/sql/events-actions/queries.ts. This one differs in two places: it seeds from MAX(height) WHERE chain_status = 'pending' rather than MAX(height), and it puts the chain_status <> 'canonical' guard on the parent rather than the child.
Both variants behave the same on normal archive data, so this is not a defect. We recently shipped #209 to fix a chain-position defect in one of the copies, so we want one shared helper - but that touches SQL currently in our merge train and would conflict with this PR. We will do it separately, after this lands. Leave yours as is.
The base archive_db.sql fixture has 227 blocks_zkapp_commands rows and every one has status 'failed'. It holds one verification-key hash and one account update that sets a verification key. No input can make getVerificationKeyUpdatesQuery return a row against it, so the only assertion available was the empty list. That gap was measured, not assumed: replacing the query body with one that returns nothing at all (AND 1=0) left all 63 integration tests green. verification_key_updates.sql adds blocks 26..32 on top of the base canonical tip, with eleven accounts that each make one distinction observable: alpha sets the target key -> returned beta sets a different key -> filtered by hash gamma sets the target key, on a CUSTOM token -> proves the token join delta sequence_no 0 at height 28 -> command order epsilon sequence_no 1 at height 28 -> command order zeta sets the target key, command FAILED -> excluded eta target hash as a PRECONDITION only -> excluded theta sets the target key in an ORPHANED block -> excluded iota sets the target key in a pending block -> pending only kappa carried by BOTH competing tips at height 32 lambda carried by fork B only eta is the important one. The archive records the key an account update SETS in zkapp_updates.verification_key_id, reached through zkapp_account_update_body.update_id, and the key it merely REQUIRES in zkapp_account_update_body.verification_key_hash_id. A query that reads the second column answers "who called this contract" instead of "who deployed it". The two tips at height 32 carry the same command, which is what a real fork looks like: on the devnet archive one zkApp command was measured in 8 blocks at a single height. Their rows agree on height, sequence_no, account-update position and zkapp_account_update.id alike. The suite now has power. Against the query as it stands: returns nothing (AND 1=0) 10 of 15 tests fail reads the precondition column instead 11 of 15 tests fail failed commands not excluded 14 of 15 tests fail Like action-state-ordering.test.ts, this runs on its own database: the fixture changes the maximum height and the pending chain, which the other integration tests assert on. The placeholder test in integration.test.ts is removed. Its one assertion, that a failed deployment is not discoverable, is now zeta. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmkAwbAi2ogrSg4XU9tYAz
…ates Ordering -------- ORDER BY was height, sequence_no, account-update position, then zkapp_account_update.id. None of those separates two competing tips at the same height, and pending_chain seeds from every block at the maximum pending height, so both tips are in the answer. When both carry the same command — the normal case during a reorg, and one command was measured in 8 blocks at a single height on the devnet archive — all four keys tie and the order was whatever the plan produced. Observed on the new fixture: fork B came back before fork A. state_hash is now the second key, so the order is total and groups by block. Cost ---- The verification-key hash was matched last, after the join tree had already expanded every account update in the range. The planner cannot drive from the hash instead, because zkapp_updates.verification_key_id and zkapp_account_update_body.update_id are both unindexed, so it hashed all of zkapp_updates (173k rows) and zkapp_account_update_body (365k rows) on every call and spilled to disk. The hash is now resolved to its (small) set of zkapp_updates rows in a MATERIALIZED CTE before any block is touched, and applied as a semi-join. Measured on a 681k-block devnet archive, over the 10 000-block maximum a client may request: before 405 ms, sequential scans, hash join spilling to 8 batches after 199 ms That is a shape improvement, not a cure. The remaining cost is inherent while those two columns are unindexed, and the indexes belong to the archive schema in the mina repository, not here. Their effect is still to be measured on a dump we can index, and mainnet is much larger than this one. Behaviour is unchanged: same parameters in the same positions, and the precondition column is still never read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmkAwbAi2ogrSg4XU9tYAz
dkijania
left a comment
dkijania
left a comment
There was a problem hiding this comment.
Just some nits. Also i created pr into your branch on your repo for things that are on us. I realised that after review of this PR
Test coverage and a total order for verificationKeyUpdates
SanabriaRusso
left a comment
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approve — backwards-compatible, and the bound is real
Independent review focused on (1) downstream backwards compatibility, (2) the new error surface,
(3) query cost / DoS, (4) SQL correctness. Everything below was executed, not read off the diff.
Head reviewed: 04bfb5d8.
1. Schema change is strictly additive — verified by count, not by eye
git diff origin/main...04bfb5d8 -- schema.graphql | grep -c '^-[^-]' → 0
git diff origin/main...04bfb5d8 -- src/resolvers-types.ts | grep -c '^-[^-]' → 0
Added: input VerificationKeyUpdateFilterInput, type VerificationKeyUpdate,
Query.verificationKeyUpdates. Removed/renamed/retyped: nothing. No nullability tightening on
any pre-existing field. resolvers-types.ts mirrors that exactly — new entries appended to
ResolversTypes, ResolversParentTypes, QueryResolvers and Resolvers, nothing dropped.
The mapper change flagged in the PR description is the one place a shape change could have hidden,
and it does not. src/blockchain/utils.ts:4-31 changes
createBlockInfo(row: ArchiveNodeDatabaseRow) → createBlockInfo(row: BlockInfoRow)where BlockInfoRow = Pick<ArchiveNodeDatabaseRow, …>. That is a widening of a parameter type
(contravariant): every existing caller still type-checks, and both function bodies are byte-identical
to main. The emitted JavaScript is unchanged. The existing events/actions integration tests that
exercise both mappers still pass (77/77, below).
Downstream impact: none. Neither mina-explorer nor mina-explorer-api references
verificationKeyUpdates, and no field either consumer selects today changed name, type or nullability.
2. BLOCK_RANGE_ERROR is not a new error code, and it does not open a circuit breaker
throwBlockRangeError (src/errors/error.ts:34, code BLOCK_RANGE_ERROR) already exists on main
and is shared with events/actions; this PR adds no new error string. Verified live against a running
server (node build/src/index.js, real Postgres):
POST / {verificationKeyUpdates(input:{verificationKeyHash:"x",from:10,to:10})}
→ HTTP/1.1 200 OK
{"data":null,"errors":[{"message":"to must be greater than from",
"extensions":{"code":"BLOCK_RANGE_ERROR","status":400}}]}
POST / {… from:0,to:20000}
→ HTTP/1.1 200 OK
{"errors":[{"message":"The block range is too large. The maximum range is 10000", …}]}
HTTP 200 with a populated errors[]. So for mina-explorer-api: _execute sees a normal
GraphQL error response, not a bare 4xx, so breaker.record_failure() is never reached. And neither
message contains "Cannot query field", "Unknown argument" or "Unknown type", so the
capability cache in app/upstream/graphql.py:33-42 is not poisoned. For mina-explorer, no
HTTP error: NNN throw path is entered.
3. Cost / DoS — measured on a real 691k-block devnet archive
I loaded the current hourly devnet dump (devnet-archive-dump-2026-09-01_1200, 1.4 GB SQL →
3.0 GB database): 691,103 blocks, max height 552,287, 176,348 zkapp_updates,
369,074 zkapp_account_update_body, 1,593 verification keys. EXPLAIN (ANALYZE, BUFFERS) on the
exact SQL from src/db/sql/verification-key-updates/queries.ts, maximum 10,000-block range:
| case | rows out | execution |
|---|---|---|
most common vk hash (564 target_updates), tip range 542287–552287 |
0 | 196–265 ms |
| same hash, range 398000–408000 (a range that actually matches) | 260 | 277–457 ms |
| hash that matches nothing at all, max range | 0 | 9–125 ms |
The nonexistent-key case is the cheapest, not the most expensive — target_updates comes back
empty and the semi-join short-circuits. This is not the defect shape from #162: the guard runs
before any SQL (verification-key-updates-service.ts:63, and the unit test asserts
calls.length === 0 on rejection), and the join is driven from the bounded block set
(Index Scan using idx_blocks_height on blocks b, Index Cond: (height >= … AND height < …)),
with the vk set applied as a semi-join predicate. Range work is genuinely bounded by from/to.
I independently reproduce the ~199 ms figure in the code comment at queries.ts:56-62.
One dimension the range guard does not bound (non-blocking, evidence below). Cost is linear in
|target_updates| — the number of zkapp_updates rows that set the requested key — and nothing
caps that. On synthetic data at realistic table ratios (3.9M zkapp_updates, 3M
zkapp_account_update) I measured, over a 48-block range returning 8 rows:
target_updates 2 → 0.8 ms
101 → 6.4 ms
1001 → 21.9 ms
10001 → 208.8 ms
100001 → 2230.7 ms
1000001 → 24481.0 ms ← planner flips to a nested loop, 10M rows discarded
Clean linear growth, ~24.5 µs per target row, entirely independent of the block range. This is
not reachable on real data: zkapp_updates is hash-consed by the archive node, so the observed
maximum across all 1,593 keys and all 691k blocks of devnet history is 564 — 177× below the
100k where it would start to hurt. I am recording the shape, not asking for a change.
Two cheap, optional hardenings, both outside this PR's blast radius:
zkapp_updates.verification_key_idhas no index (\d zkapp_updatesshows only the PK; a
Postgres FK constraint does not create one), sotarget_updatesalways sequential-scans the
table — 21.5 ms of the 265 ms above on devnet, and range-independent, so a 1-block query pays it
too.CREATE INDEX CONCURRENTLY idx_zkapp_updates_verification_key_id ON zkapp_updates (verification_key_id);
removes it. That is an archive-DB index, not a repo change — see the joint ask in §6.- #182 (
PG_STATEMENT_TIMEOUT) is the right backstop for the tail. Worth landing before this
endpoint is publicly exposed; it is not a reason to hold this PR.
4. SQL correctness — the three claims in the description hold
- Sets vs. precondition. The join is on
zaub.update_id(the key the update writes), not
zaub.verification_key_hash_id(the key it requires). Confirmed by the schema — both columns
exist onzkapp_account_update_body— and by the integration test
"a verification-key precondition is not a verification-key update". - Failed / non-applied commands excluded.
WHERE bzc.status = 'applied', covered by
"excludes a different verification key, a failed command, a precondition-only update, and an
orphaned block". - Ordering is total.
UNNEST … WITH ORDINALITYordinality is the account-update position
withinzc.zkapp_account_updates_ids;state_hashin the sort key is what disambiguates two
competing tips at one height. Covered by three separate tests including
"repeats the same order on every run". - Chain selection matches
fullChainCTEinevents-actions/queries.tsin effect
(distance_from_max_block_heightis computed identically,(SELECT max(height) FROM blocks) - height).
It differs in the pending seed, which @dkijania already reviewed and explicitly said to leave.
One small improvement over the shared helper: theALLbranch addschain_status <> 'orphaned',
which the recursive walk's<> 'canonical'guard alone would not exclude.
5. ENABLED_QUERIES — no change to what an existing deployment serves
src/resolvers.ts is untouched except for the new resolver entry, so the filter picks the query up
automatically. A deployment that sets ENABLED_QUERIES=events,actions today serves exactly what it
served before. A deployment that does not set it now additionally exposes
verificationKeyUpdates — which is precisely the docs/versioning.md:50-54 point @dkijania raised,
and my §3 numbers support shipping it default-off first: the endpoint is cheap on today's data but
runs with neither a statement timeout (#182) nor a cost limit (#183) merged.
checkSQLSchema() now also requires public_keys, tokens and zkapp_updates at startup. All
three are present in the live devnet archive schema (verified), and getTables reads
pg_catalog.pg_tables, which is grant-independent — so no startup regression.
6. Cross-PR: draft #162 ("Add zkApp command range query")
Coordinated with the reviewer of #162; folding the joint conclusion in so both authors get one story:
- No schema collision. #162 adds
ZkappCommandFilterOptionsInput,ZkappCommandOutput,
Query.zkappCommandsand friends — zero name or semantic overlap with this PR. The file-level
conflict between the two is textual and append-only (schema Query block,resolvers.ts,
USED_TABLES, the adapter pair,integration.test.ts); resolve by taking both sides. - Range guard: #162 should follow this PR, not the reverse. This PR reuses
throwBlockRangeError
and the existingBLOCK_RANGE_SIZEwith the same message text as events/actions. #162 introduces
two new env vars for the same concept. To answer that reviewer's question explicitly: this PR is
content sharingBLOCK_RANGE_SIZE—verification-key-updates-service.ts:12imports it
unchanged fromserver.jsand adds no knob of its own. No new env var is needed here, and none
should be needed there. - Joint index ask, one change not two.
tests/integration/fixtures/archive_db.sqlhas 22
indexes and none on anyzkapp_*join column either query uses. Between the two PRs the
useful set iszkapp_account_update_body(update_id),zkapp_account_update_body(account_identifier_id),
zkapp_account_update(body_id)andzkapp_updates(verification_key_id). That belongs in
docs/runbook.mdas an operator prerequisite, attributed to both PRs — not in either PR. - On whether the 199 ms was measured with a large target set: it was not, and neither was mine —
because a large one does not exist. Real devnet maximum is 564 (§3). The linear-cost shape is
real; the cardinality needed to exploit it is not present in the data.
7. @dkijania's open items
| item | status on 04bfb5d8 |
|---|---|
USED_TABLES missing public_keys / tokens |
fixed — both added |
unsafe() → tagged template (re-planning, 20.7 ms) |
fixed — no unsafe, no params array, statusClause fragment as suggested |
| SQL-text-matching unit tests | fixed — removed; the two remaining tests assert guard-before-query behaviour |
| third pending-chain copy | explicitly "leave as is"; shared helper deferred |
docs/versioning.md default-off + 1.1.0 |
policy, on the maintainers — see §5, my numbers support default-off |
Those are the author's and @dkijania's to close out; nothing there blocks from my side.
Verified by
npm ci && npm run build && npm run lint (clean) · npm run test:unit (pass) ·
npm run test:integration against PostgreSQL 16 — 77/77 pass, including all 15
verificationKeyUpdates tests · live server HTTP probes for the error surface ·
EXPLAIN (ANALYZE, BUFFERS) on a freshly downloaded 691k-block devnet archive dump ·
a synthetic scaling curve at realistic table ratios.
Nothing here is a backwards-compatibility break, a security hole, or a data-correctness bug.
Approving.
|
@mellowcroc Approved! Thanks for your contribution. We just need to solve issues with CI. I think that this is connected with running against forked repo. I will try to patch it in another pr and ask you to update your pr |
|
updating after possible ci fix |
3 participants
Closes #224.
Summary
Add a generic
verificationKeyUpdatesGraphQL query so clients can discover zkApp accounts by the verification key set in an applied account update, without direct archive PostgreSQL access.The query returns the account address and token ID together with the existing block and transaction metadata shapes.
Query contract
verificationKeyHashfromand exclusivetoblock rangeBLOCK_RANGE_ERRORALL,CANONICAL, andPENDINGstatus filterENABLED_QUERIESschema filteringImplementation
The SQL starts from the bounded best-chain block set, uses
UNNEST ... WITH ORDINALITYfor account-update IDs, and follows the archive relation fromzkapp_account_update_body.update_idthroughzkapp_updatesto the verification-key hash. This distinguishes an account update that sets a verification key from one that merely references a verification-key precondition.The schema-generated resolver types were regenerated. Shared block and transaction metadata mappers now accept the exact row fields they consume, allowing the new service to reuse them without casts to an event row.
Validation
npm run buildnpm run lintnpm run test:unitnpm run test:integration(63 tests passed, including a static-dump check that failed deployments are excluded)