fix: resolve fromActionState/endActionState by chain position, not zkapp_field interning key - #209
Conversation
Update: the fix is now in this PR (commit
|
| checkpoints checked | wrong suffix returned | |
|---|---|---|
before (e353bbc) |
61 | 48 |
| after (this branch) | 61 | 0 |
For the account behind the original incident:
before fromActionState = checkpoint -> 32 entries, 1 chain-link break, block 303203 missing
after fromActionState = checkpoint -> 33 entries, 0 breaks, nothing missing
and querying the formerly-dropped state now correctly starts at block 303203 rather than
returning block 303199, which is before the checkpoint.
Tests
action-state-ordering: 36 pass, 0 fail, 0 todo (was 18 pass / 15 fail / 1 todo). The
todo is resolved and replaced by three explicit cases: rejected when actions were missed,
answered normally when nothing was missed, and endActionState below the range rejected with
no exception.
Existing suites unchanged: 83 unit, 25 integration. npm run lint clean.
Related
o1-labs/o1js#2905 — createActionsList cannot detect a dropped block, because it never
compares actionStateOne[i] with actionStateTwo[i+1]. That is why this fault was silent on
the client. Worth fixing independently of this PR: it would make any archive fault of this
class loud, on any endpoint, for every zkApp.
Update: nightly invariant suite on real data (commit
|
| Fixture suite (per PR) | Nightly suite | |
|---|---|---|
| Data | generated, deterministic | a real dump, different every run |
| Question | does the known defect stay fixed? | do the rules hold on real data shapes? |
| Builds its own adversarial case | yes — inverts the interning order on purpose | no — takes data as it comes |
Designed to need no upkeep
The dump rotates hourly, so anything pinned to its contents rots within a day. Four rules:
- No expected values. Every assertion is an invariant the data implies about itself,
with the unfiltered list as the reference:fromActionStatereturns exactly the suffix,
endActionStateexactly the prefix, consecutive entries link, another account's action
state is rejected. Nothing to update when the data changes. - Subjects are discovered, never named, in a deterministic order, accounts with an
interning-order inversion first. NoORDER BY random()— a nightly failure that cannot be
reproduced on the same dump is one nobody can act on. - Finding nothing to test is a failure. A suite that silently tests nothing reports
success, which is worse than no suite. The last test asserts the run covered something. - Failures are reproducible without the dump.
It has teeth — verified both ways
Run against a dump with known interning-order inversions with the old ActionsService
injected, it fails and points straight at the incident:
fromActionState returned the wrong set of blocks.
account : B62qmMvzQNSCnZ4qH1N9uJByov9EyGushzv1jV7Cqg8UwS6BrRgHGzk
checkpoint : 27078409473756655551477733610137394255970846757871033008217988809531431111245 (block 303199)
entries : expected 133, got 132
diverges at: index 1 (expected block 303203, got 303207)
missing : [303203]
unexpected : none
reproduce : { actions(input: { address: "B62q…", fromActionState: "27078…" }) { … } }
With the fixed service: passes over 12 accounts and 40 checkpoints. A suite that cannot fail
proves nothing, so that check is documented in tests/devnet-dump/README.md as the way to
re-verify it later.
Real accounts have hundreds of action states, so the message reports the first divergence and
the missing blocks rather than printing both lists in full.
Note on the workflow
The postgres service is pinned to 17 to match the major version the production dumps are
taken from — an older server rejects statements the newer pg_dump emits. This is the first
time this suite will run in CI at all, so the first nightly may need a nudge (disk space for
the dump is the other likely snag; the job frees a few GB up front).
PR-gated suites are unaffected: action-state 36/36, integration 25/25, unit 83, lint clean.
|
Verdict: MERGEABLE ✅ Second pass, focused on downstream compatibility. I verified the premise, both boundary semantics, ordering, and the new error path against the code at What I checked
The one behaviour change callers will noticeA previously-answered query can now 400 with Non-blocking nits1. // src/db/sql/events-actions/queries.ts, in resolveActionStateBoundary
- const fromAsNum = from ? Number(from) : null;
- const toAsNum = to ? Number(to) : null;
+ // `|| null` so "0" collapses to null, matching fullChainCTE's `if (fromAsNum)`
+ // truthiness check. Number(undefined) is NaN, which is also falsy.
+ const fromAsNum = Number(from) || null;
+ const toAsNum = Number(to) || null;and the test that pins it, alongside the existing out-of-range cases: test('from: 0 does not bypass the out-of-range guard', async () => {
const account = fixture.accounts.control;
const actions = await actionsService.getActions(
{ address: account.address, fromActionState: account.states[0].value, from: 0 },
nullOptions
);
// window_start must be derived the same way as fullChainCTE, so `from: 0`
// behaves exactly like sending no `from` at all.
const noFrom = await actionsService.getActions(
{ address: account.address, fromActionState: account.states[0].value },
nullOptions
);
assert.deepStrictEqual(heightsOf(actions), heightsOf(noFrom));
});Cleanest long-term shape is to export one 2. 3. Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.
Two things this approval does not mean:
- It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
- It does not by itself mean the branch is ready to merge.
mainrequires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.
…r defect
`getActionsQuery` filters `fromActionState` / `endActionState` on
`zkapp_field.id`. That is the interning key of the value: it records when
the value was first written to the archive, not where it sits on the
chain. When an archive is filled out of chain order (bulk import,
hard-fork migration, bootstrap) the two orders disagree, and the filter
silently drops real actions or returns actions from before the
checkpoint.
Clients that fold an action state from a checkpoint (o1js
`Reducer.getActions({ fromActionState })`) then compute a value that can
never match the chain, and every transaction they build fails with
`Account_action_state_precondition_unsatisfied`.
Measured on the mesa-rc-1 archive dump of 2026-08-11: 1 271 damaged
checkpoints across 1 003 of 2 778 zkApp accounts.
A local network writes blocks in chain order and cannot reproduce this,
so the fixture inverts the interning order on purpose. It holds three
accounts with identical actions in identical blocks that differ only in
that order: `inverted`, `adjacent` (the shape measured in production),
and `control` (natural order). `control` must pass before and after any
fix — it proves the suite fails because of the interning order and not
because the expected behaviour of the API changed.
No production code is changed here. Against current `master` the suite
is 18 pass / 15 fail / 1 todo, and every `control` case passes.
Adds:
- tests/integration/fixtures/generate-action-state-fixture.mjs and its
two generated outputs (.sql fixture, .json expectations, read by the
tests so the two cannot drift apart)
- tests/integration/action-state-ordering.test.ts
- tests/integration/action-state-setup.ts (own database, so the added
blocks do not disturb the counts integration.test.ts asserts on)
- assertActionChainIsLinked / heightsOf in tests/test-helpers.ts
`getActionsQuery` filtered `fromActionState` / `endActionState` with
AND zkf0.id >= (SELECT id FROM zkapp_field WHERE field = <state>)
`zkapp_field` is the interning table for every field value in the archive.
Its `id` records when a value was first written, not where it sits on the
chain. Those orders disagree on any archive filled out of chain order — a
bulk import, a hard-fork migration, a bootstrap — and the filter then
silently dropped real actions or returned actions from before the
checkpoint.
Filter on the block height of the checkpoint instead, resolved for the
requested account before the main query runs. Greater-or-equal keeps the
checkpoint's own block in the answer, because o1js strips it in
createActionsList by comparing actionStateOne with the requested value.
`checkActionState` is replaced by `resolveActionStateBoundary`. The old
check only asked whether the value existed anywhere in `zkapp_field`, so
it accepted action data, app state, or another account's action state,
and the server answered with unrelated data instead of raising an error.
The new lookup is scoped to the account and to the chain.
That lookup is deliberately not limited to the queried block range. A
checkpoint is a position, not data. Resolving it inside the window would
make an out-of-range checkpoint resolve to NULL, and `height >= NULL`
selects no rows — the server would answer with an empty list, which a
reducer reads as "no actions to fold".
Being below the window does not by itself make the answer wrong. If the
account emitted no action between the checkpoint and the start of the
window, every action in the window is still the complete answer, and
that is what clients get today. Rejecting in that case would break the
common case of a quiet zkApp folding from its genesis action state — the
default o1js path, since o1js sends no from/to. So the answer is returned
when it is provably complete and rejected with a new
`ACTION_STATE_OUT_OF_RANGE` error, naming the `from` to use, when actions
really are missing. `endActionState` gets no such exception: it bounds
the request from above, so a checkpoint below the window puts the whole
requested span below it.
Validation on a mesa-rc-1 archive dump (2026-08-11), over 20 accounts
known to have an interning-order inversion, checking every checkpoint
against the unfiltered list:
before 61 checkpoints, 48 returned the wrong suffix
after 61 checkpoints, 0 returned the wrong suffix
For the account that produced the original failed transaction, the
answer goes from 32 entries with a chain-link break and block 303203
missing, to 33 entries with no break and nothing missing.
Tests: action-state-ordering 36/36 (was 18/34 with 15 failures and 1
todo). Existing suites unchanged: 83 unit, 25 integration.
The fixture suite in tests/integration is the per-pull-request gate: it is
deterministic and builds its own adversarial data. This suite answers a
different question — do the same rules hold on real data, whose shapes a
fixture does not model? Many actions in one block, many account updates in
one transaction, pending and orphaned branches, accounts with hundreds of
action states, archives assembled by a real ingestion pipeline.
It reuses the existing tests/devnet-dump download and load, so no new data
infrastructure is added. It is registered from devnet-dump.test.ts rather
than living in its own file, because node:test gives each file its own
process and so its own multi-minute dump load.
The dump rotates hourly, so anything pinned to its contents would rot
within a day. Four rules keep the suite from needing upkeep:
1. No expected values. Every assertion is an invariant the data implies
about itself, with the unfiltered action list as the reference:
fromActionState returns exactly the suffix, endActionState exactly
the prefix, consecutive entries link, and another account's action
state is rejected.
2. Subjects are discovered, never named, in a deterministic order, with
accounts carrying an interning-order inversion first. No
ORDER BY random(): a nightly failure nobody can reproduce is a
failure nobody can act on.
3. Finding nothing to test fails. A suite that silently tests nothing
reports success, which is worse than having no suite.
4. Failures are reproducible without the dump. Messages carry the
account, a compact difference, and a GraphQL query to paste
anywhere. Real accounts have hundreds of action states, so the
message reports the first divergence and the missing or unexpected
blocks instead of printing both lists.
Verified to have teeth: run against the mesa-rc-1 dump with the old
ActionsService injected it fails and names the missing block 303203 and
the account from the original incident; with the fixed service it passes
over 12 accounts and 40 checkpoints.
Adds a nightly workflow for it (03:30 UTC, plus manual dispatch). Nothing
runs the devnet-dump suite automatically today. The postgres service is
pinned to 17 to match the major version the production dumps come from.
5f686ae to
5b09c5b
Compare
Fixes a defect in the
actionsquery that silently removes real actions from afromActionStateresponse, which makes any client folding an action state from thatcheckpoint compute a value that can never match the chain.
Three commits, reviewable independently:
05d06627514bed5f686aeThe defect
getActionsQueryresolved the checkpoint like this(
src/db/sql/events-actions/queries.ts,emittedActionStateCTE):zkapp_fieldis the interning table for every field value in the archive. Itsidrecordswhen a value was first written, not where it sits on the chain. The filter assumed those
two orders are the same. They are not: any archive filled out of chain order — a bulk import
through
mina-archive-blocks, a hard-fork migration, a bootstrap — writes a de-duplicated setof new field values and loses their block order.
When the orders disagree the filter drops real actions, or returns actions from before the
requested checkpoint.
endActionStatehad the same defect.A client that folds from a checkpoint (o1js
Reducer.getActions({ fromActionState })) thencomputes a value that never matches, and every transaction it builds fails with
Account_action_state_precondition_unsatisfied— deterministically, for that checkpoint, onevery call. Not a race, not archive lag.
Evidence
On a dump of the mesa-rc-1 archive (2026-08-11), the two interning keys for one affected
account are adjacent and swapped:
The checkpoint sits on the earlier block but holds the larger id, so
zkf0.id >= 491052removes the action at block 303203 — a real, canonical, confirmed action.
Scale on that archive: 1 271 damaged checkpoints across 1 003 of 2 778 zkApp accounts. On
the account that failed in production the rate is 1 in 78, which is why this went unnoticed:
almost every checkpoint works, and the damaged one fails every time.
Running this repository's own
ActionsServiceate353bbc, unmodified, against that dump:The third line is the proof that this is key order and not an off-by-one: asking for the state
of block 303203 returns an entry from block 303199, before the checkpoint. Two opposite
errors from one filter.
The fix
Filter on the block height of the checkpoint, resolved for the requested account before the
main query runs. Greater-or-equal keeps the checkpoint's own block in the answer, because o1js
strips it in
createActionsListby comparingactionStateOnewith the requested value —excluding it here would change what existing clients receive.
checkActionStateis replaced byresolveActionStateBoundary, scoped to the account and tothe chain. The old check only asked whether the value existed anywhere in
zkapp_field, so itaccepted action data, app state, or another account's action state, and the server answered
with unrelated data instead of raising an error. That is the second reason this was invisible.
The one design decision worth your attention
The boundary lookup is deliberately not limited to the queried block range. A checkpoint is
a position, not data, so resolving it does not need its block inside the window. Scoped to the
window, an out-of-range checkpoint would resolve to NULL, and
height >= NULLselects no rows— the server would answer with an empty list, which a reducer reads as "no actions to
fold". That would be worse than the current behaviour.
Being below the window does not by itself make the answer wrong:
action in the window is still the complete answer, and that is what clients get today. This
is the common case for a quiet zkApp folding from its genesis action state — the default
o1js path, because o1js sends no
from/to;So the query is answered when provably complete, and rejected with a new
ACTION_STATE_OUT_OF_RANGEerror naming thefromto use when it is not.endActionStategets no such exception: it bounds the request from above, so a checkpoint below the window puts
the whole requested span below it, and an empty answer would be silently wrong.
Happy to change this to "always reject" — it is a small change, and tests for both behaviours
are already written.
Tests
A local network writes blocks in chain order and therefore cannot reproduce this. The
fixture inverts the interning order on purpose. Three accounts dispatch identical actions in
identical blocks and differ only in that order:
invertedcontroladjacentcontrolpassing is what makes the suite trustworthy: any change that makes the other two passby weakening the expectations breaks
controltoo. The failing set was derived by hand fromthe interning ids before the tests were first run, and all 34 outcomes matched.
At commit 1 the suite is 18 pass / 15 fail / 1 todo. At commit 2 it is 36 pass / 0 fail / 0
todo — the todo is resolved and replaced by three explicit cases for the decision above.
Commit 3 adds the real-data counterpart under
tests/devnet-dump, reusing the existing dumpdownload so no new data infrastructure is introduced, plus the nightly workflow (nothing runs
test:devnet-dumpautomatically today). Because the dump rotates hourly it asserts invariantsonly — no expected values, subjects discovered rather than named, deterministic ordering, and
finding nothing to test is a failure rather than a silent pass. Verified to have teeth: with
the old
ActionsServiceinjected it fails and names the missing block; with the fixed one itpasses over 12 accounts and 40 checkpoints.
Validation on real data
Every action state of 20 accounts with a known inversion, used as a checkpoint and compared
against the unfiltered list:
e353bbc)For the account behind the original incident: 32 entries with a chain-link break and block
303203 missing becomes 33 entries with no break and nothing missing.
Existing suites unchanged: 83 unit tests, 25 integration tests.
npm run lintclean. All CIchecks pass.
Note for reviewers
The postgres service in the new nightly workflow is pinned to 17 to match the major version
the production dumps are taken from — an older server rejects statements the newer
pg_dumpemits. This is the first time the devnet-dump suite will run in CI at all, so the first
scheduled run may need attention; disk space for the dump is the other likely snag, and the job
frees a few GB up front.
Related
o1-labs/o1js#2905 —
createActionsListnever comparesactionStateOne[i]withactionStateTwo[i+1], so it re-seeds its fold from each block and cannot detect a droppedblock. That is why this fault was silent on the client side. Worth fixing independently of this
PR: it would make any archive fault of this class loud, on any endpoint, for every zkApp.