fix(stateless): audit of swallowed database errors that fabricate execution results - #99
Open
Gabriel-Trintinalia wants to merge 5 commits into
Conversation
Three places in WitnessDatabase turned a database error into a
semantically meaningful default, so an incomplete witness produced a
wrong execution result reported as success rather than a rejected block.
The MPT layer already distinguishes the two outcomes precisely: `null`
means proved absent (mpt/main.zig calls this "valid non-inclusion"), while
error.InvalidProof comes from `findNodeInIndex(...) orelse
return error.InvalidProof` -- the node is missing from the witness. So
InvalidProof means "cannot verify", never "absent", and defaulting on it
fabricates state. basic() already handled this correctly (InvalidProof ->
InvalidWitness, with a documented SYSTEM_ADDRESS carve-out); the rest did
not.
1. storage(): `error.InvalidProof => return 0` made an unprovable slot read
as zero, and the storage-root lookup's
`error.InvalidProof => break :blk EMPTY_TRIE_HASH` made every slot on an
unprovable account read as zero. Both now return InvalidWitness.
2. hasNonZeroStorageForAddress(): `catch return false` fed the CREATE
collision check (interpreter/host.zig, setupCreateCore), so an
unprovable target let a CREATE succeed where the reference rejects it --
a consensus-level wrong result. Now fallible.
3. storage_root_cache.put(...) `catch {}` silently dropped an entry, and
storageRootFor() is a bare get(), so a dropped entry is
indistinguishable from "never loaded". computeStorageRootBatch
(executor/output.zig) reads that as "no pre-state storage" and rebuilds
the storage trie from only the touched slots -- a wrong state root. Now
propagates.
setupCreateCore returns a plain CreateSetupResult and so cannot report an
error, so JournalInner gains a sticky `witness_error`: the affected sites
record there and transition.zig rejects the block after execution, with a
second check before the state root is computed (system calls also swallow
execution errors and a block may have no transaction after them). The
in-flight CREATE fails closed meanwhile rather than proceeding on a guess.
Infallible databases (InMemoryDB) are unaffected -- the journal detects a
fallible one via @typeinfo, so duck-typing is preserved.
Left deliberately unchanged: createAccountCheckpoint's `catch` in
setupCreateCore. The compiler rejected an else prong there, proving its
error set is exactly TransferError (OutOfFunds, OverflowPayment,
CreateCollision) -- all legitimate CREATE outcomes, no database error
possible. Documented in place.
Also fixes test discovery, without which most of the above would be
untested: the `context` module (journal.zig, ~1700 lines) was absent from
build.zig's test list, and addTest only collects tests from a module's root
file, so a `test { _ = @import("journal.zig"); }` reference is required
too. Verified with a canary: before this, a `try expect(false)` in
journal.zig passed the suite.
Coverage was checked by mutation -- each change reverted individually to
confirm a test fails. Six of seven are covered. The two transition.zig
call sites are not: "this check is called from the right place" is not a
unit-testable property, and every zkevm fixture has a complete witness, so
that path needs an integration test with a deliberately incomplete one.
Verified: zkevm 23994/23994 (100%, the stateless-witness suite),
blockchain-tests 0 fail / 48 skip, unit 399/399, and state-tests
144 fail + 15 crash with a failing-test list byte-identical to a stashed
baseline (all pre-existing and unrelated).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two witnessError() checks in transitionWithContext were the one part of the previous commit that mutation testing showed to be uncovered: both could be deleted with the suite still green. "This check is called from the right place" is not a unit-testable property, so it needs a block driven through transitionWithContext. No witness fixture is required: ctx is anytype, so a stub database that resolves only the sender, coinbase and recipient stands in for a witness missing the CREATE target. A tx with to == null derives its target from the sender and nonce, so that address is one the stub cannot resolve — the error is recorded in setupCreateCore and must surface as a rejected block rather than a fabricated "create failed". Paired with a control that runs the same stub and block shape as a plain transfer to a provable address and asserts the block is accepted. Without it the first test would also pass if transitionWithContext rejected everything for an unrelated reason. Verified by mutation: removing both checks fails the CREATE test. Note the two checks are redundant by design, so removing either one alone still passes — the block-level backstop exists for a database error raised by a system call in a block with no following transaction, which no test currently reaches. It is defence in depth rather than separately covered. Coverage after this commit: six of seven changes killed by unit tests, the seventh (the check pair) by this integration test. Verified: unit 401/401, zkevm 23994/23994 (100%). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… one
The previous commit added JournalInner.witness_error to carry a database
error out of setupCreateCore, which returns a plain CreateSetupResult and
so cannot report one. That duplicated a mechanism the codebase already
had: ctx_error, set to .database_error at seven sites in
interpreter/host.zig ("On any database error, marks ctx_error so the block
is rejected") and checked in stateless/executor/main.zig, which turns a
non-ok ctx_error into InvalidWitness.
Replaces witness_error with ctx_error, which is strictly less code:
- JournalInner.witness_error, recordWitnessError() and witnessError()
are gone.
- Journal.hasNonZeroStorageForAddress now propagates instead of
swallowing-and-recording, so it is back to *const and no longer needs
the @typeinfo error-union probe -- a plain-bool database (InMemoryDB)
coerces, and one without the method at all still yields false. Both
are pinned by tests.
- The two callers in setupCreateCore mark ctx_error themselves, matching
the seven existing sites. js.loadAccount is a Journal call and so
bypasses the Host accessors that already do this, which is why that
site needs its own marking.
- Both witnessError() checks in transitionWithContext are removed; the
pre-existing check at the block level covers them.
The gaps the previous commit fixed were real and remain fixed: verified
that Journal.loadAccount never touches ctx_error, and
hasNonZeroStorageForAddress returns a plain bool with no channel at all.
Tests follow the contract that now holds. transitionWithContext does not
itself reject -- it marks -- so the integration tests assert on ctx_error
rather than on a returned error, with the control asserting it stays .ok.
This does mean they stop one level short of the block-level rejection:
executeBlockStateless hardcodes Context(WitnessDatabase), so covering that
last hop needs a real witness fixture rather than an injected stub. The
zkevm suite exercises it.
Verified by mutation: dropping the ctx_error marking in setupCreateCore
fails two tests; re-swallowing in hasNonZeroStorageForAddress no longer
compiles.
Verified: unit 401/401, zkevm 23994/23994 (100%), blockchain-tests 0 fail /
48 skip, state-tests 144 fail + 15 crash with a failing-test list
byte-identical to baseline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 01a9a70. Configure here.
| } | ||
| // The block must be rejected rather than accepting the fabricated failure. | ||
| try std.testing.expectEqual(context_mod.ContextError.database_error, ctx.ctx_error); | ||
| } |
There was a problem hiding this comment.
CREATE host tests never actually run
Medium Severity
The new setupCreateCore ctx_error test lives in host.zig, but the interpreter test root only pulls in a few opcode files. addTest collects tests from a module root and from files referenced inside a test { _ = @import(...) } block, so this case never runs — the same silent gap this PR just fixed for journal.zig.
Reviewed by Cursor Bugbot for commit 01a9a70. Configure here.
Reported by Cursor Bugbot on Consensys-Incorporated#99. Making WitnessDatabase.storage() return InvalidWitness was not sufficient on its own: Host.sload swallowed it with `catch return null`, and opSload turns null into halt(.invalid_opcode) -- a consensus-visible EVM failure -- so the transaction failed and the block was still ACCEPTED. That is the same fabricated success the PR set out to remove, just wearing a different mask than the zero it replaced. Auditing the pattern found five such sites, not the two reported: accountInfo, sload, sstore, selfdestruct, and the loadAccount in recordCreateTargetCore All five now mark ctx_error before returning null, matching the sibling accessors (blockHash, codeInfo, loadAccountWithCode) that already did. `grep -c "catch return null;"` in host.zig is now 0. Each site has its own test at the Host boundary, and each was verified by mutation: reverting any one site alone fails exactly one test. Two things that would have made a test pass for the wrong reason and are guarded against in comments: recordCreateTargetCore returns early on pre-Amsterdam specs (EIP-7928 BAL recording is Amsterdam+), so that test runs on Amsterdam; and setupCreateCore reads the caller from evm_state rather than the database, so the caller must be pre-loaded or the CREATE fails on its own caller lookup instead of the target. Bugbot's second finding -- that these host.zig tests never run -- does not hold. A canary (try expect(false)) in host.zig fails the suite and the count moves 406 -> 407. Test inclusion is transitive: opcodes/host_ops_tests.zig is referenced from interpreter/main.zig's test block and imports ../host.zig. journal.zig was invisible for a different reason -- `context` was absent from build.zig's test list entirely. Verified: unit 406/406, zkevm 23994/23994 (100%), blockchain-tests 0 fail / 48 skip, state-tests 144 fail + 15 crash with a failing-test list byte-identical to baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.


Origin
This started as an audit, not a bug report. The invariant under test was simply a block execution must never return a wrong result, applied to the stateless witness path.
The audit looked for one specific shape: an error turned into a semantically meaningful default. It found 11 sites — 6 in
WitnessDatabaseand 5 in theHostaccessors — where a "cannot verify" answer became a fabricated one (0,false,EMPTY_TRIE_HASH, or a failed operation) and the block was accepted anyway. Cursor Bugbot's review of the first round surfaced a further instance of the same class, which is what fix 4 below covers.Sites that looked like instances but were verified not to be are called out as deliberately unchanged, with the reasoning, so the audit's negative results are reviewable too.
Problem
Several places turned a database error into a semantically meaningful default, so an incomplete witness produced a wrong execution result reported as success instead of a rejected block.
The MPT layer already distinguishes the two outcomes precisely:
nullmeans proved absent (mpt/main.zigcalls this "valid non-inclusion"), whereaserror.InvalidProofcomes fromfindNodeInIndex(...) orelse return error.InvalidProof— the node is missing from the witness. SoInvalidProofmeans "cannot verify", never "absent", and defaulting on it fabricates state.basic()already handled this correctly (InvalidProof→InvalidWitness, with a documentedSYSTEM_ADDRESScarve-out). The rest did not. This PR finishes that existing, deliberate design rather than introducing a new policy.The fixes
1.
storage()fabricated zeros.error.InvalidProof => return 0made an unprovable slot read as zero, and the storage-root lookup'serror.InvalidProof => break :blk EMPTY_TRIE_HASHmade every slot on an unprovable account read as zero. Demonstrated by a test whose slot genuinely holds0xabcd.2.
hasNonZeroStorageForAddress()could let an invalid CREATE succeed.catch return falsefed the CREATE collision check insetupCreateCore, so an unprovable target allowed a CREATE the reference rejects — consensus-level.3. A dropped cache entry silently changed the state root.
storage_root_cache.put(...) catch {}dropped entries, andstorageRootFor()is a bareget(), so a dropped entry is indistinguishable from "never loaded".computeStorageRootBatchreads that as "no pre-state storage" and rebuilds the storage trie from only the touched slots.4. Five
Hostaccessors swallowed the error anyway — found by Cursor Bugbot's review of this PR (thanks). Fixingstorage()was not sufficient on its own:Host.sloadswallowed the result withcatch return null, andopSloadturnsnullintohalt(.invalid_opcode)— a consensus-visible EVM failure — so the transaction failed and the block was still accepted. Same fabricated success, wearing a different mask than the zero it replaced.Auditing the pattern found five such sites, not the two reported:
accountInfo,sload,sstore,selfdestruct, and theloadAccountinrecordCreateTargetCore. All now markctx_errorlike their siblings (blockHash,codeInfo,loadAccountWithCode) already did.grep -c "catch return null;"inhost.zigis now 0.setupCreateCorereturns a plainCreateSetupResultand cannot report an error, so its sites markctx_error— the mechanism already used at seven sites ininterpreter/host.zigand checked instateless/executor/main.zig. Infallible databases (InMemoryDB) are unaffected.Deliberately unchanged:
createAccountCheckpoint'scatchinsetupCreateCore. The compiler rejected anelseprong there, proving its error set is exactlyTransferError— all legitimate CREATE outcomes, no database error possible. Documented in place.