From e346a6a2fdf3c61b4ed3b557c55426086353e02a Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Fri, 28 Aug 2026 10:47:11 +1000 Subject: [PATCH 1/5] fix(stateless): never return a wrong result from an incomplete witness 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) --- build.zig | 4 + src/evm/context/journal.zig | 97 +++++++++- src/evm/context/main.zig | 8 + src/evm/interpreter/host.zig | 79 +++++++- src/stateless/db/main.zig | 39 +++- src/stateless/db/test.zig | 257 ++++++++++++++++++++++++++ src/stateless/executor/transition.zig | 12 ++ 7 files changed, 486 insertions(+), 10 deletions(-) diff --git a/build.zig b/build.zig index 2923e39..dc4e7b5 100644 --- a/build.zig +++ b/build.zig @@ -535,6 +535,10 @@ pub fn build(b: *std.Build) void { .{ .m = mods.precompile, .name = "precompile" }, .{ .m = mods.interpreter, .name = "interpreter" }, .{ .m = mods.handler, .name = "handler" }, + // `context` owns journal.zig. Tests only run for the module passed to + // addTest, not for its imported modules, so without this entry nothing in + // src/evm/context/ is covered by `zig build test`. + .{ .m = mods.context, .name = "context" }, .{ .m = mods.mpt, .name = "mpt" }, .{ .m = mods.rlp_decode, .name = "rlp_decode" }, .{ .m = mods.executor, .name = "executor" }, diff --git a/src/evm/context/journal.zig b/src/evm/context/journal.zig index c47d471..8be04a6 100644 --- a/src/evm/context/journal.zig +++ b/src/evm/context/journal.zig @@ -404,6 +404,19 @@ pub const JournaledAccount = struct { /// /// Spec Id is a essential information for the Journal. pub const JournalInner = struct { + /// First database error swallowed by a code path that has no error channel. + /// + /// Parts of the interpreter (notably setupCreateCore) return plain result + /// unions, so a DB failure there can only be turned into "the operation + /// failed". For a stateless witness that is a wrong execution result rather + /// than a legitimate outcome: an incomplete witness would let a CREATE + /// succeed or fail on fabricated information. Those sites record the error + /// here instead, and the per-transaction driver rejects the block after + /// execution (see stateless/executor/transition.zig). + /// + /// Sticky and first-write-wins: the first error is the informative one, and + /// execution after it is meaningless anyway. + witness_error: ?anyerror = null, /// The current evm_state evm_state: state.EvmState, /// Transient storage that is discarded after every transaction. @@ -455,6 +468,7 @@ pub const JournalInner = struct { pub fn new() JournalInner { return .{ + .witness_error = null, .evm_state = state.EvmState.init(alloc_mod.get()), .transient_storage = state.TransientStorage.init(alloc_mod.get()), .logs = std.ArrayList(primitives.Log).empty, @@ -1652,11 +1666,38 @@ pub fn Journal(comptime DB: type) type { /// Returns true if the address has any non-zero storage in the DB. /// Used by CREATE collision check; returns false for DB types without this method. - pub fn hasNonZeroStorageForAddress(self: *const @This(), addr: primitives.Address) bool { - if (comptime @hasDecl(DB, "hasNonZeroStorageForAddress")) return self.getDb().hasNonZeroStorageForAddress(addr); + /// + /// A DB that can fail (a stateless witness) is not allowed to have its + /// failure silently become "no storage here": that would let a CREATE + /// succeed where the reference rejects it. The error is recorded so the + /// block is rejected after execution, and `true` is returned meanwhile so + /// the in-flight CREATE fails closed rather than proceeding on a guess. + pub fn hasNonZeroStorageForAddress(self: *@This(), addr: primitives.Address) bool { + if (comptime @hasDecl(DB, "hasNonZeroStorageForAddress")) { + const result = self.getDb().hasNonZeroStorageForAddress(addr); + if (comptime @typeInfo(@TypeOf(result)) == .error_union) { + return result catch |err| { + self.recordWitnessError(err); + return true; + }; + } + return result; + } return false; } + /// Record a database error raised where no error can be returned. Sticky: + /// keeps the first error. See JournalInner.witness_error. + pub fn recordWitnessError(self: *@This(), err: anyerror) void { + if (self.inner.witness_error == null) self.inner.witness_error = err; + } + + /// The first swallowed database error, if any. The per-transaction driver + /// checks this after execution and rejects the block. + pub fn witnessError(self: *const @This()) ?anyerror { + return self.inner.witness_error; + } + /// Check whether an address is already in the EVM state cache (was loaded before /// this call). Used to avoid un-tracking addresses that were legitimately accessed /// earlier in the same transaction. @@ -1671,3 +1712,55 @@ pub fn Journal(comptime DB: type) type { } }; } + +// ─── Tests: swallowed-database-error channel ────────────────────────────────── + +// A stub DB whose hasNonZeroStorageForAddress always fails, standing in for a +// stateless witness that cannot prove the account. Only the members Journal +// touches on this path are provided. +const FailingStorageDb = struct { + pub fn hasNonZeroStorageForAddress(_: *const @This(), _: primitives.Address) !bool { + return error.InvalidWitness; + } +}; + +// The CREATE collision check must not read a DB failure as "no storage here": +// that would let a CREATE succeed at an address the reference rejects. The error +// has to be recorded so the block is rejected, and the in-flight CREATE must fail +// closed (true) rather than proceed on a guess. +test "a failing hasNonZeroStorageForAddress is recorded and fails closed" { + var j = Journal(FailingStorageDb).new(.{}); + defer j.deinit(); + + try std.testing.expect(j.witnessError() == null); + const answer = j.hasNonZeroStorageForAddress(@splat(0x11)); + + try std.testing.expect(answer); // fail closed, not a fabricated "false" + try std.testing.expectEqual(@as(?anyerror, error.InvalidWitness), j.witnessError()); +} + +// An infallible DB (InMemoryDB returns plain bool) must keep working unchanged, +// and must never set the error channel. +test "an infallible DB is unaffected by the error channel" { + const PlainDb = struct { + pub fn hasNonZeroStorageForAddress(_: *const @This(), _: primitives.Address) bool { + return true; + } + }; + var j = Journal(PlainDb).new(.{}); + defer j.deinit(); + + try std.testing.expect(j.hasNonZeroStorageForAddress(@splat(0x22))); + try std.testing.expect(j.witnessError() == null); +} + +// Sticky and first-write-wins: later errors must not mask the first, which is the +// informative one. +test "witness_error keeps the first error" { + var j = Journal(FailingStorageDb).new(.{}); + defer j.deinit(); + + j.recordWitnessError(error.InvalidWitness); + j.recordWitnessError(error.OutOfMemory); + try std.testing.expectEqual(@as(?anyerror, error.InvalidWitness), j.witnessError()); +} diff --git a/src/evm/context/main.zig b/src/evm/context/main.zig index 482f67a..bdffaa5 100644 --- a/src/evm/context/main.zig +++ b/src/evm/context/main.zig @@ -117,3 +117,11 @@ pub const testing = struct { std.debug.print("Context tests passed.\n", .{}); } }; + +// `zig build test` only collects tests from a module's root file, so tests living +// in the other files of this module are invisible unless referenced here. Without +// this block, everything in journal.zig — including the swallowed-database-error +// channel — is silently untested even when `context` is in build.zig's test list. +test { + _ = @import("journal.zig"); +} diff --git a/src/evm/interpreter/host.zig b/src/evm/interpreter/host.zig index c2b47de..cef0bac 100644 --- a/src/evm/interpreter/host.zig +++ b/src/evm/interpreter/host.zig @@ -924,7 +924,13 @@ fn setupCreateCore( } } - _ = js.loadAccount(new_addr) catch return .{ .failed = CreateResult.preExecFailure(gas_limit) }; + // Record before failing: with a stateless witness this error means the CREATE + // target could not be resolved, so "create failed" is a fabricated outcome and + // the block must be rejected (see JournalInner.witness_error). + _ = js.loadAccount(new_addr) catch |err| { + js.recordWitnessError(err); + return .{ .failed = CreateResult.preExecFailure(gas_limit) }; + }; // EIP-8037 (Amsterdam+): was the target already alive (pre-funded) before creation? // Captured before createAccountCheckpoint transfers value / bumps nonce. A deployable @@ -955,6 +961,10 @@ fn setupCreateCore( } } + // Safe to swallow: createAccountCheckpoint's error set is exactly TransferError + // (OutOfFunds, OverflowPayment, CreateCollision), all of which are legitimate + // CREATE failures rather than "outcome could not be determined". It cannot + // surface a database error, so there is nothing to record here. const checkpoint = js.createAccountCheckpoint(caller, new_addr, value, spec_id) catch { return .{ .failed = CreateResult.failure() }; }; @@ -1172,3 +1182,70 @@ pub fn create2Address(sender: primitives.Address, salt: primitives.U256, init_co @memcpy(&addr, hash[12..32]); return addr; } + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +// A database that can resolve one address and fails for every other, standing in +// for a stateless witness that is missing the CREATE target account. +const MissingTargetDb = struct { + pub const CALLER: primitives.Address = @splat(0xC0); + + pub fn basic(_: *@This(), address: primitives.Address) !?state_mod.AccountInfo { + if (std.mem.eql(u8, &address, &CALLER)) { + var info = state_mod.AccountInfo.default(); + info.balance = 1_000_000; + return info; + } + return error.InvalidWitness; + } + + pub fn codeByHash(_: *@This(), _: primitives.Hash) !bytecode_mod.Bytecode { + return bytecode_mod.Bytecode.newLegacy(&.{}); + } + + pub fn storage(_: *@This(), _: primitives.Address, _: primitives.StorageKey) !primitives.StorageValue { + return error.InvalidWitness; + } + + pub fn blockHash(_: *@This(), _: u64) !primitives.Hash { + return error.InvalidWitness; + } +}; + +// setupCreateCore cannot return an error (it yields a plain CreateSetupResult), so +// a database failure while loading the CREATE target can only become "create +// failed". With a stateless witness that outcome is fabricated, so the error must +// be recorded on the journal for the block to be rejected afterwards. Deleting the +// recordWitnessError call must not go unnoticed. +test "setupCreateCore records a witness error when the CREATE target cannot be loaded" { + var ctx = context_mod.Context(MissingTargetDb).new(.{}, primitives.SpecId.prague); + defer ctx.journaled_state.deinit(); + + var host = Host.init(MissingTargetDb, &ctx, null); + + // The caller is read from evm_state, not the DB, so load it first: we want the + // CREATE to fail on the *target*, not on its own caller lookup. + _ = try ctx.journaled_state.loadAccount(MissingTargetDb.CALLER); + + try std.testing.expect(ctx.journaled_state.witnessError() == null); + + const setup = setupCreateCore( + &ctx.journaled_state, + &host, + MissingTargetDb.CALLER, + 0, + &[_]u8{0x00}, + 100_000, + false, + 0, + false, + 0, + true, + ); + + switch (setup) { + .failed => {}, + else => return error.ExpectedCreateToFail, + } + try std.testing.expect(ctx.journaled_state.witnessError() != null); +} diff --git a/src/stateless/db/main.zig b/src/stateless/db/main.zig index 5bd20ad..d6fdf8a 100644 --- a/src/stateless/db/main.zig +++ b/src/stateless/db/main.zig @@ -119,11 +119,18 @@ pub const WitnessDatabase = struct { else => return DbError.InvalidWitness, }; + // These puts must propagate rather than `catch {}`. storageRootFor() is a + // bare cache `get()`, so a dropped entry is indistinguishable from "this + // account was never loaded", and the post-execution batch trie update + // treats that as "no pre-state storage" and rebuilds the storage trie from + // only the touched slots (see executor/output.zig:188). Swallowing the + // failure would therefore turn an allocation failure into a silently wrong + // state root reported as success. const as = account_state orelse { - self.storage_root_cache.put(address, EMPTY_TRIE_HASH) catch {}; + try self.storage_root_cache.put(address, EMPTY_TRIE_HASH); return null; }; - self.storage_root_cache.put(address, as.storage_root) catch {}; + try self.storage_root_cache.put(address, as.storage_root); return state.AccountInfo{ .balance = as.balance, .nonce = as.nonce, @@ -165,16 +172,29 @@ pub const WitnessDatabase = struct { address, self.node_index, ) catch |err| switch (err) { - error.InvalidProof => break :blk EMPTY_TRIE_HASH, + // InvalidProof means the witness lacks the node needed to prove + // anything here — genuine absence is reported as a null + // account_state below ("valid non-inclusion" in mpt/main.zig). + // Defaulting to EMPTY_TRIE_HASH would make every subsequent slot + // read on this account return 0, i.e. a wrong execution result + // from an incomplete witness. Matches basic()'s handling. + error.InvalidProof => return DbError.InvalidWitness, else => return DbError.InvalidWitness, }; const root = if (account_state) |as| as.storage_root else EMPTY_TRIE_HASH; - self.storage_root_cache.put(address, root) catch {}; + // Propagate, for the same reason as in basic(): a dropped entry later + // reads as "no pre-state storage" and yields a wrong state root. + try self.storage_root_cache.put(address, root); break :blk root; }; const slot = u256ToHash(index); const value = mpt.verifyStorageIndexed(storage_root, slot, self.node_index) catch |err| switch (err) { - error.InvalidProof => return 0, + // A slot that is genuinely unset yields null from verifyStorageIndexed + // (valid non-inclusion), which becomes 0 without an error. Reaching + // InvalidProof means the storage node is missing from the witness, so + // returning 0 would fabricate a value — the slot's real contents are + // unknown and may be non-zero. + error.InvalidProof => return DbError.InvalidWitness, else => return DbError.InvalidWitness, }; return value; @@ -182,7 +202,12 @@ pub const WitnessDatabase = struct { // ── hasNonZeroStorageForAddress ───────────────────────────────────────── - pub fn hasNonZeroStorageForAddress(self: *const Self, address: primitives.Address) bool { + /// Fallible: this feeds the CREATE collision check, so "I cannot prove it" + /// must not collapse into "no storage here". Doing so would let a CREATE + /// succeed at an address the reference rejects — a consensus-level wrong + /// result from an incomplete witness. A genuinely absent account still + /// resolves to null (valid non-inclusion) and returns false without error. + pub fn hasNonZeroStorageForAddress(self: *const Self, address: primitives.Address) !bool { if (self.storage_root_cache.get(address)) |root| { return !std.mem.eql(u8, &root, &EMPTY_TRIE_HASH); } @@ -190,7 +215,7 @@ pub const WitnessDatabase = struct { self.pre_state_root, address, self.node_index, - ) catch return false; + ) catch return DbError.InvalidWitness; const as = account_state orelse return false; return !std.mem.eql(u8, &as.storage_root, &EMPTY_TRIE_HASH); } diff --git a/src/stateless/db/test.zig b/src/stateless/db/test.zig index d074146..3229a83 100644 --- a/src/stateless/db/test.zig +++ b/src/stateless/db/test.zig @@ -382,3 +382,260 @@ test "blockHash returns InvalidWitness for missing hash" { defer wdb.deinit(); try std.testing.expectError(error.InvalidWitness, wdb.blockHash(12345678)); } + +// ─── Regression: storage-root cache must not be silently dropped ────────────── + +// `storageRootFor()` is a bare `storage_root_cache.get()`, so a missing entry is +// indistinguishable from "this account was never loaded". The post-execution +// batch trie update relies on that distinction: for a null pre-state root, +// computeStorageRootBatch (src/stateless/executor/output.zig:188) rebuilds the +// storage trie from only the slots execution touched, as if the account had no +// pre-state storage. For an account that *does* have pre-state storage, that +// silently drops every untouched slot and yields a WRONG state root, reported as +// success. +// +// So an allocation failure while caching the root must never be swallowed: the +// three `storage_root_cache.put(...)` sites in db/main.zig must propagate, not +// `catch {}`. This test asserts the invariant that makes the downstream +// assumption safe: +// +// basic() succeeded => storageRootFor() knows the account's storage root +// +// It drives every allocation index in turn, so it does not depend on how many +// allocations basic() happens to make. A wrong state root returned as success is +// the worst failure mode for a proving system, hence pinning it here. +test "basic must not succeed while silently dropping the storage-root cache entry" { + var address: primitives.Address = @splat(0x00); + address[19] = 0x33; + const key_hash = mpt.keccak256(&address); + + // A non-empty storage root: this is what must not be lost. + const storage_root: primitives.Hash = @splat(0x5a); + + var account_rlp: [200]u8 = undefined; + const account_len = buildAccountRlp(&account_rlp, 1, 500, storage_root, KECCAK_EMPTY); + var leaf_node: [512]u8 = undefined; + const leaf_len = buildLeafNode(&leaf_node, key_hash, account_rlp[0..account_len]); + const leaf_bytes = leaf_node[0..leaf_len]; + const state_root = mpt.keccak256(leaf_bytes); + + const w = input.StateWitness{ + .state_root = state_root, + .nodes = &[_][]const u8{leaf_bytes}, + .codes = &.{}, + .keys = &.{}, + .headers = &.{}, + }; + + // The node index is built with the real allocator; only the database's own + // allocations (which include the storage-root cache) are made to fail. + var idx = try mpt.buildNodeIndex(ALLOC, w.nodes); + defer idx.deinit(); + + var fail_index: usize = 0; + while (fail_index < 16) : (fail_index += 1) { + var failing = std.testing.FailingAllocator.init(ALLOC, .{ .fail_index = fail_index }); + var wdb = db_mod.WitnessDatabase.init( + failing.allocator(), + &idx, + w.state_root, + w.codes, + &.{}, + ) catch continue; // init itself ran out of memory: nothing to check + defer wdb.deinit(); + + const info = wdb.basic(address) catch continue; // propagated: correct + if (info == null) continue; // account absent: no root to cache + + // basic() reported success, so the cached root must be present and right. + const cached = wdb.storageRootFor(address); + if (cached == null) { + std.debug.print( + "\nfail_index={d}: basic() succeeded but storageRootFor() is null;\n" ++ + "the batch trie update will rebuild storage from scratch and\n" ++ + "produce a wrong state root with no error raised.\n", + .{fail_index}, + ); + return error.StorageRootSilentlyDropped; + } + try std.testing.expectEqualSlices(u8, &storage_root, &cached.?); + } +} + +// ─── Regression: an unprovable slot must not read as zero ───────────────────── + +// The MPT layer distinguishes two outcomes precisely (src/stateless/mpt/main.zig): +// `null` means *proved absent* — the comments there read "valid non-inclusion" — +// whereas `error.InvalidProof` is returned by +// `findNodeInIndex(...) orelse return error.InvalidProof`, i.e. the witness does +// not contain the node needed to prove anything at all. +// +// So InvalidProof means "cannot verify", not "absent". basic() already treats it +// that way (it maps InvalidProof to InvalidWitness, with a documented +// SYSTEM_ADDRESS carve-out), but storage() maps it to `return 0` +// (db/main.zig:186) and the storage-root lookup maps it to EMPTY_TRIE_HASH +// (:175). An incomplete witness therefore reads as "slot is zero" instead of +// failing, which lets a wrong state transition be executed and proved. +// +// Here slot 3 genuinely holds 0xabcd, but its storage leaf is withheld from the +// witness, so a zero result is demonstrably wrong rather than merely unproven. +test "storage must fail, not return 0, when the witness cannot prove the slot" { + var address: primitives.Address = @splat(0x00); + address[19] = 0x77; + const slot_key: u256 = 3; + + var slot_hash: primitives.Hash = @splat(0); + { + var n = slot_key; + var si: usize = 32; + while (si > 0) { + si -= 1; + slot_hash[si] = @intCast(n & 0xff); + n >>= 8; + } + } + const storage_key_hash = mpt.keccak256(&slot_hash); + const rlp_value = &[_]u8{ 0x82, 0xab, 0xcd }; // slot 3 = 0xabcd + var storage_leaf: [256]u8 = undefined; + const storage_leaf_len = buildLeafNode(&storage_leaf, storage_key_hash, rlp_value); + const storage_root = mpt.keccak256(storage_leaf[0..storage_leaf_len]); + + const acc_key_hash = mpt.keccak256(&address); + var account_rlp: [200]u8 = undefined; + const account_len = buildAccountRlp(&account_rlp, 0, 0, storage_root, KECCAK_EMPTY); + var acc_leaf: [512]u8 = undefined; + const acc_leaf_len = buildLeafNode(&acc_leaf, acc_key_hash, account_rlp[0..account_len]); + const acc_leaf_bytes = acc_leaf[0..acc_leaf_len]; + const state_root = mpt.keccak256(acc_leaf_bytes); + + // The account leaf is present; the storage leaf is deliberately withheld, so + // the storage trie cannot be walked at all. + const w = input.StateWitness{ + .state_root = state_root, + .nodes = &[_][]const u8{acc_leaf_bytes}, + .codes = &.{}, + .keys = &.{}, + .headers = &.{}, + }; + var idx: mpt.NodeIndex = undefined; + var wdb = try makeWdb(w, &idx); + defer idx.deinit(); + defer wdb.deinit(); + + const result = wdb.storage(address, slot_key); + if (result) |value| { + std.debug.print( + "\nstorage() returned {d} for an unprovable slot whose true value is 0xabcd;\n" ++ + "an incomplete witness reads as 'slot is zero' instead of failing.\n", + .{value}, + ); + return error.UnprovableSlotReadAsZero; + } else |_| { + // Any error is acceptable here; the point is that it must not succeed. + } +} + +// ─── Regression: CREATE collision check must not guess ──────────────────────── + +// hasNonZeroStorageForAddress feeds the CREATE collision check +// (src/evm/interpreter/host.zig, setupCreateCore). It used to `catch return +// false`, so a witness that cannot prove the target account read as "no storage +// here" and allowed a CREATE that the reference rejects — a consensus-level wrong +// result. It is now fallible; the journal wrapper records the error so the block +// is rejected after execution, and fails the CREATE closed in the meantime. +// +// Note a genuinely absent account is NOT an error: it resolves via valid +// non-inclusion to false, which the second half of this test pins so the fix +// cannot be "just always return an error". +test "hasNonZeroStorageForAddress must fail when the account cannot be proven" { + var address: primitives.Address = @splat(0x00); + address[19] = 0x88; + + // A state root whose account node is absent from the witness: nothing can be + // proven about this address one way or the other. + const unprovable_root: primitives.Hash = @splat(0x9e); + const w = input.StateWitness{ + .state_root = unprovable_root, + .nodes = &.{}, + .codes = &.{}, + .keys = &.{}, + .headers = &.{}, + }; + var idx: mpt.NodeIndex = undefined; + var wdb = try makeWdb(w, &idx); + defer idx.deinit(); + defer wdb.deinit(); + + if (wdb.hasNonZeroStorageForAddress(address)) |answer| { + std.debug.print( + "\nhasNonZeroStorageForAddress returned {} for an unprovable account;\n" ++ + "the CREATE collision check would proceed on a fabricated answer.\n", + .{answer}, + ); + return error.UnprovableAccountAnsweredAnyway; + } else |_| { + // Correct: the caller is told we cannot determine this. + } +} + +test "hasNonZeroStorageForAddress reports false for a provably absent account" { + var address: primitives.Address = @splat(0x00); + address[19] = 0x99; + + // An empty branch root proves non-inclusion for every address, so the answer + // is knowable and must be `false`, not an error. + var branch: [18]u8 = undefined; + const branch_len = buildEmptyBranchNode(&branch); + const branch_bytes = branch[0..branch_len]; + const state_root = mpt.keccak256(branch_bytes); + + const w = input.StateWitness{ + .state_root = state_root, + .nodes = &[_][]const u8{branch_bytes}, + .codes = &.{}, + .keys = &.{}, + .headers = &.{}, + }; + var idx: mpt.NodeIndex = undefined; + var wdb = try makeWdb(w, &idx); + defer idx.deinit(); + defer wdb.deinit(); + + try std.testing.expect(!(try wdb.hasNonZeroStorageForAddress(address))); +} + +// Covers the OTHER InvalidProof site in storage(): the storage-root lookup +// (db/main.zig, the verifyAccountIndexed call inside storage()). Here the ACCOUNT +// node is missing, not the storage node, so the account's storage root cannot be +// determined. Defaulting to EMPTY_TRIE_HASH would make every slot on this account +// read as 0. Distinct from the test above, which withholds the storage leaf and +// exercises the verifyStorageIndexed site. +test "storage must fail when the account's storage root cannot be proven" { + var address: primitives.Address = @splat(0x00); + address[19] = 0xaa; + + // Empty node pool: the account node behind this root is absent, so nothing + // about the account (including its storage root) can be proven. + const unprovable_root: primitives.Hash = @splat(0x7c); + const w = input.StateWitness{ + .state_root = unprovable_root, + .nodes = &.{}, + .codes = &.{}, + .keys = &.{}, + .headers = &.{}, + }; + var idx: mpt.NodeIndex = undefined; + var wdb = try makeWdb(w, &idx); + defer idx.deinit(); + defer wdb.deinit(); + + // Cache is empty, so this goes through the storage-root lookup path. + if (wdb.storage(address, 3)) |value| { + std.debug.print( + "\nstorage() returned {d} for an account whose storage root is unprovable;\n" ++ + "every slot on this account would read as 0.\n", + .{value}, + ); + return error.UnprovableAccountStorageReadAsZero; + } else |_| {} +} diff --git a/src/stateless/executor/transition.zig b/src/stateless/executor/transition.zig index 3d5fafa..8feb002 100644 --- a/src/stateless/executor/transition.zig +++ b/src/stateless/executor/transition.zig @@ -1073,6 +1073,12 @@ pub fn transitionWithContext( return err; }; + // Parts of the interpreter cannot return an error (setupCreateCore returns a + // plain result union), so a database failure there was turned into "the + // operation failed" and recorded instead. With a stateless witness that + // outcome is fabricated, so reject the block rather than carry on with it. + if (ctx.journaled_state.witnessError()) |witness_err| return witness_err; + if (ctx.tx.data) |*d| d.deinit(alloc_mod.get()); ctx.tx.data = null; ctx.tx.access_list.deinit(); @@ -1203,6 +1209,12 @@ pub fn transitionWithContext( // Detect changes from mining reward + withdrawals + post-block calls (all at BAI=N+1) if (tracker) |*t| t.detectAndRecord(txs.len + 1, ctx, txs.len); + // Final backstop for swallowed database errors. The per-transaction check above + // covers the common case, but system calls also swallow execution errors and a + // block may have no transaction after them, so re-check once here — before the + // state root is computed — so a recorded error can never escape as a valid block. + if (ctx.journaled_state.witnessError()) |witness_err| return witness_err; + // ── Extract post-state ──────────────────────────────────────────────────── const post_alloc = try extractPostState(arena, pre_alloc_in, ctx, spec); From f36e6903ad593f1969d45ca315be2651c8935829 Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Fri, 28 Aug 2026 10:59:33 +1000 Subject: [PATCH 2/5] test(stateless): cover the witness-error rejection path end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/stateless/executor/transition.zig | 104 ++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/src/stateless/executor/transition.zig b/src/stateless/executor/transition.zig index 8feb002..9237c71 100644 --- a/src/stateless/executor/transition.zig +++ b/src/stateless/executor/transition.zig @@ -1521,3 +1521,107 @@ fn extractPostState( return post; } + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +const TEST_SENDER: input.Address = @splat(0x11); +const TEST_COINBASE: input.Address = @splat(0x22); +const TEST_RECIPIENT: input.Address = @splat(0x33); + +// A database that resolves only the accounts a block legitimately needs and fails +// for anything else, standing in for a stateless witness that is missing the +// CREATE target account. Mirrors WitnessDatabase's duck-typed surface. +const PartialWitnessDb = struct { + fn known(address: input.Address) bool { + return std.mem.eql(u8, &address, &TEST_SENDER) or + std.mem.eql(u8, &address, &TEST_COINBASE) or + std.mem.eql(u8, &address, &TEST_RECIPIENT); + } + + pub fn basic(_: *@This(), address: input.Address) !?state_mod.AccountInfo { + if (!known(address)) return error.InvalidWitness; + var info = state_mod.AccountInfo.default(); + info.balance = 1_000_000_000_000; + return info; + } + + pub fn codeByHash(_: *@This(), _: primitives.Hash) !bytecode_mod.Bytecode { + return bytecode_mod.Bytecode.newLegacy(&.{}); + } + + pub fn storage(_: *@This(), _: primitives.Address, _: primitives.StorageKey) !primitives.StorageValue { + return 0; + } + + pub fn blockHash(_: *@This(), _: u64) !primitives.Hash { + return @splat(0); + } +}; + +fn testEnv() input.Env { + return .{ .coinbase = TEST_COINBASE, .gas_limit = 30_000_000, .base_fee = 0 }; +} + +fn runTestBlock(arena: std.mem.Allocator, txs: []input.TxInput) !TransitionResult { + var ctx = context_mod.Context(PartialWitnessDb).new(.{}, primitives.SpecId.shanghai); + const pre_alloc: std.AutoHashMapUnmanaged(input.Address, input.AllocAccount) = .{}; + return transitionWithContext( + arena, + &ctx, + pre_alloc, + testEnv(), + txs, + primitives.SpecId.shanghai, + 1, + 0, + &.{}, + ); +} + +// This is the integration counterpart to the unit tests in db/test.zig and +// host.zig: those prove the error is raised and recorded, this proves the block is +// actually rejected. setupCreateCore cannot return an error, so a database failure +// while loading the CREATE target is recorded on the journal and checked after +// execution — deleting either check in transitionWithContext must not go +// unnoticed, since a fabricated "create failed" would otherwise be accepted as a +// valid block. +test "a CREATE whose target is absent from the witness rejects the block" { + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + + // to == null makes this a CREATE. The target address is derived from the + // sender and nonce, so it is not one of the addresses the stub DB can resolve. + var txs = [_]input.TxInput{.{ + .from = TEST_SENDER, + .to = null, + .nonce = 0, + .gas = 200_000, + .gas_price = 1, + .data = &[_]u8{ 0x60, 0x00, 0x60, 0x00, 0xf3 }, // RETURN empty + }}; + + if (runTestBlock(arena_state.allocator(), &txs)) |_| { + return error.BlockAcceptedDespiteUnprovableCreateTarget; + } else |_| { + // Correct: the block is rejected rather than being given a fabricated result. + } +} + +// Control: the same stub DB and block shape, but a plain transfer to an address +// the witness *can* prove. Without this, the test above would also pass if +// transitionWithContext rejected every block for an unrelated reason. +test "a block whose accounts are all provable is accepted" { + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + + var txs = [_]input.TxInput{.{ + .from = TEST_SENDER, + .to = TEST_RECIPIENT, + .nonce = 0, + .gas = 100_000, + .gas_price = 1, + .value = 1, + }}; + + _ = try runTestBlock(arena_state.allocator(), &txs); +} From c763c62b87c4c95b660bed2b002db6f9546040a8 Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Fri, 28 Aug 2026 12:06:52 +1000 Subject: [PATCH 3/5] refactor(stateless): use the existing ctx_error channel, not a second 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) --- src/evm/context/journal.zig | 77 ++++++++------------------- src/evm/interpreter/host.zig | 36 ++++++++----- src/stateless/executor/transition.zig | 59 ++++++++++---------- 3 files changed, 73 insertions(+), 99 deletions(-) diff --git a/src/evm/context/journal.zig b/src/evm/context/journal.zig index 8be04a6..69d7c1c 100644 --- a/src/evm/context/journal.zig +++ b/src/evm/context/journal.zig @@ -404,19 +404,6 @@ pub const JournaledAccount = struct { /// /// Spec Id is a essential information for the Journal. pub const JournalInner = struct { - /// First database error swallowed by a code path that has no error channel. - /// - /// Parts of the interpreter (notably setupCreateCore) return plain result - /// unions, so a DB failure there can only be turned into "the operation - /// failed". For a stateless witness that is a wrong execution result rather - /// than a legitimate outcome: an incomplete witness would let a CREATE - /// succeed or fail on fabricated information. Those sites record the error - /// here instead, and the per-transaction driver rejects the block after - /// execution (see stateless/executor/transition.zig). - /// - /// Sticky and first-write-wins: the first error is the informative one, and - /// execution after it is meaningless anyway. - witness_error: ?anyerror = null, /// The current evm_state evm_state: state.EvmState, /// Transient storage that is discarded after every transaction. @@ -468,7 +455,6 @@ pub const JournalInner = struct { pub fn new() JournalInner { return .{ - .witness_error = null, .evm_state = state.EvmState.init(alloc_mod.get()), .transient_storage = state.TransientStorage.init(alloc_mod.get()), .logs = std.ArrayList(primitives.Log).empty, @@ -1672,32 +1658,18 @@ pub fn Journal(comptime DB: type) type { /// succeed where the reference rejects it. The error is recorded so the /// block is rejected after execution, and `true` is returned meanwhile so /// the in-flight CREATE fails closed rather than proceeding on a guess. - pub fn hasNonZeroStorageForAddress(self: *@This(), addr: primitives.Address) bool { + /// Propagates rather than reporting "no storage here" on a database error: + /// this feeds the CREATE collision check, so a swallowed failure would let + /// a CREATE succeed where the reference rejects it. The caller (which owns + /// ctx_error) is responsible for marking the block invalid. Databases that + /// cannot fail (InMemoryDB returns a plain bool) are unaffected. + pub fn hasNonZeroStorageForAddress(self: *const @This(), addr: primitives.Address) !bool { if (comptime @hasDecl(DB, "hasNonZeroStorageForAddress")) { - const result = self.getDb().hasNonZeroStorageForAddress(addr); - if (comptime @typeInfo(@TypeOf(result)) == .error_union) { - return result catch |err| { - self.recordWitnessError(err); - return true; - }; - } - return result; + return self.getDb().hasNonZeroStorageForAddress(addr); } return false; } - /// Record a database error raised where no error can be returned. Sticky: - /// keeps the first error. See JournalInner.witness_error. - pub fn recordWitnessError(self: *@This(), err: anyerror) void { - if (self.inner.witness_error == null) self.inner.witness_error = err; - } - - /// The first swallowed database error, if any. The per-transaction driver - /// checks this after execution and rejects the block. - pub fn witnessError(self: *const @This()) ?anyerror { - return self.inner.witness_error; - } - /// Check whether an address is already in the EVM state cache (was loaded before /// this call). Used to avoid un-tracking addresses that were legitimately accessed /// earlier in the same transaction. @@ -1725,23 +1697,19 @@ const FailingStorageDb = struct { }; // The CREATE collision check must not read a DB failure as "no storage here": -// that would let a CREATE succeed at an address the reference rejects. The error -// has to be recorded so the block is rejected, and the in-flight CREATE must fail -// closed (true) rather than proceed on a guess. -test "a failing hasNonZeroStorageForAddress is recorded and fails closed" { +// that would let a CREATE succeed at an address the reference rejects. The journal +// propagates instead, leaving the caller (which owns ctx_error) to mark the block +// invalid. +test "a failing hasNonZeroStorageForAddress propagates instead of answering false" { var j = Journal(FailingStorageDb).new(.{}); defer j.deinit(); - try std.testing.expect(j.witnessError() == null); - const answer = j.hasNonZeroStorageForAddress(@splat(0x11)); - - try std.testing.expect(answer); // fail closed, not a fabricated "false" - try std.testing.expectEqual(@as(?anyerror, error.InvalidWitness), j.witnessError()); + try std.testing.expectError(error.InvalidWitness, j.hasNonZeroStorageForAddress(@splat(0x11))); } -// An infallible DB (InMemoryDB returns plain bool) must keep working unchanged, -// and must never set the error channel. -test "an infallible DB is unaffected by the error channel" { +// An infallible DB (InMemoryDB returns a plain bool) must keep working unchanged — +// the @hasDecl/duck-typed path must not require an error union. +test "an infallible DB still answers hasNonZeroStorageForAddress directly" { const PlainDb = struct { pub fn hasNonZeroStorageForAddress(_: *const @This(), _: primitives.Address) bool { return true; @@ -1750,17 +1718,14 @@ test "an infallible DB is unaffected by the error channel" { var j = Journal(PlainDb).new(.{}); defer j.deinit(); - try std.testing.expect(j.hasNonZeroStorageForAddress(@splat(0x22))); - try std.testing.expect(j.witnessError() == null); + try std.testing.expect(try j.hasNonZeroStorageForAddress(@splat(0x22))); } -// Sticky and first-write-wins: later errors must not mask the first, which is the -// informative one. -test "witness_error keeps the first error" { - var j = Journal(FailingStorageDb).new(.{}); +// A DB without the method at all falls back to false, as before. +test "a DB without hasNonZeroStorageForAddress reports false" { + const NoDeclDb = struct {}; + var j = Journal(NoDeclDb).new(.{}); defer j.deinit(); - j.recordWitnessError(error.InvalidWitness); - j.recordWitnessError(error.OutOfMemory); - try std.testing.expectEqual(@as(?anyerror, error.InvalidWitness), j.witnessError()); + try std.testing.expect(!(try j.hasNonZeroStorageForAddress(@splat(0x33)))); } diff --git a/src/evm/interpreter/host.zig b/src/evm/interpreter/host.zig index cef0bac..3c19c93 100644 --- a/src/evm/interpreter/host.zig +++ b/src/evm/interpreter/host.zig @@ -924,11 +924,12 @@ fn setupCreateCore( } } - // Record before failing: with a stateless witness this error means the CREATE - // target could not be resolved, so "create failed" is a fabricated outcome and - // the block must be rejected (see JournalInner.witness_error). - _ = js.loadAccount(new_addr) catch |err| { - js.recordWitnessError(err); + // Mark the block invalid before failing: with a stateless witness this error + // means the CREATE target could not be resolved, so "create failed" would be a + // fabricated outcome. Note js.loadAccount is a Journal call and so bypasses the + // Host accessors that already set ctx_error on a database error. + _ = js.loadAccount(new_addr) catch { + host.ctx_error.* = context_mod.ContextError.database_error; return .{ .failed = CreateResult.preExecFailure(gas_limit) }; }; @@ -955,7 +956,15 @@ fn setupCreateCore( { const storage_wiped = if (js.inner.evm_state.get(new_addr)) |acct| acct.status.storage_wiped else false; if (!storage_wiped) { - if (js.hasNonZeroStorageForAddress(new_addr)) { + // A database error here means we cannot tell whether the target has + // storage. Treating that as "no storage" would let the CREATE proceed + // at an address the reference rejects, so mark the block invalid and + // fail the CREATE closed. + const has_storage = js.hasNonZeroStorageForAddress(new_addr) catch { + host.ctx_error.* = context_mod.ContextError.database_error; + return .{ .failed = CreateResult.failure() }; + }; + if (has_storage) { return .{ .failed = CreateResult.failure() }; } } @@ -1214,10 +1223,12 @@ const MissingTargetDb = struct { // setupCreateCore cannot return an error (it yields a plain CreateSetupResult), so // a database failure while loading the CREATE target can only become "create -// failed". With a stateless witness that outcome is fabricated, so the error must -// be recorded on the journal for the block to be rejected afterwards. Deleting the -// recordWitnessError call must not go unnoticed. -test "setupCreateCore records a witness error when the CREATE target cannot be loaded" { +// failed". With a stateless witness that outcome is fabricated, so ctx_error must +// be marked -- the block-level driver (stateless/executor/main.zig) turns a +// non-ok ctx_error into InvalidWitness. Note js.loadAccount is a Journal call and +// bypasses the Host accessors that already do this, which is why it needs its own +// marking here. +test "setupCreateCore marks ctx_error when the CREATE target cannot be loaded" { var ctx = context_mod.Context(MissingTargetDb).new(.{}, primitives.SpecId.prague); defer ctx.journaled_state.deinit(); @@ -1227,7 +1238,7 @@ test "setupCreateCore records a witness error when the CREATE target cannot be l // CREATE to fail on the *target*, not on its own caller lookup. _ = try ctx.journaled_state.loadAccount(MissingTargetDb.CALLER); - try std.testing.expect(ctx.journaled_state.witnessError() == null); + try std.testing.expectEqual(context_mod.ContextError.ok, ctx.ctx_error); const setup = setupCreateCore( &ctx.journaled_state, @@ -1247,5 +1258,6 @@ test "setupCreateCore records a witness error when the CREATE target cannot be l .failed => {}, else => return error.ExpectedCreateToFail, } - try std.testing.expect(ctx.journaled_state.witnessError() != null); + // The block must be rejected rather than accepting the fabricated failure. + try std.testing.expectEqual(context_mod.ContextError.database_error, ctx.ctx_error); } diff --git a/src/stateless/executor/transition.zig b/src/stateless/executor/transition.zig index 9237c71..1c595e7 100644 --- a/src/stateless/executor/transition.zig +++ b/src/stateless/executor/transition.zig @@ -1073,11 +1073,6 @@ pub fn transitionWithContext( return err; }; - // Parts of the interpreter cannot return an error (setupCreateCore returns a - // plain result union), so a database failure there was turned into "the - // operation failed" and recorded instead. With a stateless witness that - // outcome is fabricated, so reject the block rather than carry on with it. - if (ctx.journaled_state.witnessError()) |witness_err| return witness_err; if (ctx.tx.data) |*d| d.deinit(alloc_mod.get()); ctx.tx.data = null; @@ -1209,12 +1204,6 @@ pub fn transitionWithContext( // Detect changes from mining reward + withdrawals + post-block calls (all at BAI=N+1) if (tracker) |*t| t.detectAndRecord(txs.len + 1, ctx, txs.len); - // Final backstop for swallowed database errors. The per-transaction check above - // covers the common case, but system calls also swallow execution errors and a - // block may have no transaction after them, so re-check once here — before the - // state root is computed — so a recorded error can never escape as a valid block. - if (ctx.journaled_state.witnessError()) |witness_err| return witness_err; - // ── Extract post-state ──────────────────────────────────────────────────── const post_alloc = try extractPostState(arena, pre_alloc_in, ctx, spec); @@ -1562,12 +1551,15 @@ fn testEnv() input.Env { return .{ .coinbase = TEST_COINBASE, .gas_limit = 30_000_000, .base_fee = 0 }; } -fn runTestBlock(arena: std.mem.Allocator, txs: []input.TxInput) !TransitionResult { - var ctx = context_mod.Context(PartialWitnessDb).new(.{}, primitives.SpecId.shanghai); +fn makeTestCtx() context_mod.Context(PartialWitnessDb) { + return context_mod.Context(PartialWitnessDb).new(.{}, primitives.SpecId.shanghai); +} + +fn runTestBlock(arena: std.mem.Allocator, ctx: anytype, txs: []input.TxInput) !TransitionResult { const pre_alloc: std.AutoHashMapUnmanaged(input.Address, input.AllocAccount) = .{}; return transitionWithContext( arena, - &ctx, + ctx, pre_alloc, testEnv(), txs, @@ -1578,14 +1570,13 @@ fn runTestBlock(arena: std.mem.Allocator, txs: []input.TxInput) !TransitionResul ); } -// This is the integration counterpart to the unit tests in db/test.zig and -// host.zig: those prove the error is raised and recorded, this proves the block is -// actually rejected. setupCreateCore cannot return an error, so a database failure -// while loading the CREATE target is recorded on the journal and checked after -// execution — deleting either check in transitionWithContext must not go -// unnoticed, since a fabricated "create failed" would otherwise be accepted as a -// valid block. -test "a CREATE whose target is absent from the witness rejects the block" { +// Integration counterpart to the unit tests in db/test.zig and host.zig: those +// prove the database error is raised and that setupCreateCore marks ctx_error, +// this proves it survives a whole block execution. transitionWithContext does not +// itself reject -- the block-level driver (executor/main.zig) turns a non-ok +// ctx_error into InvalidWitness -- so the contract asserted here is that the block +// is *marked* invalid rather than completing as if the CREATE legitimately failed. +test "a CREATE whose target is absent from the witness marks the block invalid" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); @@ -1600,17 +1591,20 @@ test "a CREATE whose target is absent from the witness rejects the block" { .data = &[_]u8{ 0x60, 0x00, 0x60, 0x00, 0xf3 }, // RETURN empty }}; - if (runTestBlock(arena_state.allocator(), &txs)) |_| { - return error.BlockAcceptedDespiteUnprovableCreateTarget; - } else |_| { - // Correct: the block is rejected rather than being given a fabricated result. - } + var ctx = makeTestCtx(); + // Whether this returns a value or an error is not the contract; the mark is. + _ = runTestBlock(arena_state.allocator(), &ctx, &txs) catch {}; + + try std.testing.expectEqual( + context_mod.ContextError.database_error, + ctx.ctx_error, + ); } // Control: the same stub DB and block shape, but a plain transfer to an address -// the witness *can* prove. Without this, the test above would also pass if -// transitionWithContext rejected every block for an unrelated reason. -test "a block whose accounts are all provable is accepted" { +// the witness *can* prove. Without this, the test above would also pass if every +// block were marked invalid for an unrelated reason. +test "a block whose accounts are all provable is not marked invalid" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); @@ -1623,5 +1617,8 @@ test "a block whose accounts are all provable is accepted" { .value = 1, }}; - _ = try runTestBlock(arena_state.allocator(), &txs); + var ctx = makeTestCtx(); + _ = try runTestBlock(arena_state.allocator(), &ctx, &txs); + + try std.testing.expectEqual(context_mod.ContextError.ok, ctx.ctx_error); } From 01a9a70efec36747ef5489db3b70a59c923b5819 Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Fri, 28 Aug 2026 12:22:56 +1000 Subject: [PATCH 4/5] fmt --- src/stateless/executor/transition.zig | 1 - 1 file changed, 1 deletion(-) diff --git a/src/stateless/executor/transition.zig b/src/stateless/executor/transition.zig index 1c595e7..25f9b5d 100644 --- a/src/stateless/executor/transition.zig +++ b/src/stateless/executor/transition.zig @@ -1073,7 +1073,6 @@ pub fn transitionWithContext( return err; }; - if (ctx.tx.data) |*d| d.deinit(alloc_mod.get()); ctx.tx.data = null; ctx.tx.access_list.deinit(); From 974a8b52195e369420cc5cab1438deeac835a5fd Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Fri, 28 Aug 2026 15:11:40 +1000 Subject: [PATCH 5/5] fix(evm): mark ctx_error in the Host accessors that swallowed DB errors Reported by Cursor Bugbot on #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) --- src/evm/interpreter/host.zig | 123 +++++++++++++++++++++++++++++++++-- 1 file changed, 118 insertions(+), 5 deletions(-) diff --git a/src/evm/interpreter/host.zig b/src/evm/interpreter/host.zig index 3c19c93..f289c86 100644 --- a/src/evm/interpreter/host.zig +++ b/src/evm/interpreter/host.zig @@ -360,7 +360,10 @@ pub const Host = struct { /// Load account info. Returns null on database error. pub fn accountInfo(self: *Host, addr: primitives.Address) ?struct { balance: primitives.U256, is_cold: bool, is_empty: bool } { - const load = self.js_vtable.accountInfo(self.js, addr) catch return null; + const load = self.js_vtable.accountInfo(self.js, addr) catch { + self.ctx_error.* = context_mod.ContextError.database_error; + return null; + }; return .{ .balance = load.info.balance, .is_cold = load.is_cold, @@ -425,12 +428,18 @@ pub const Host = struct { } pub fn sload(self: *Host, addr: primitives.Address, key: primitives.U256) ?struct { value: primitives.U256, is_cold: bool } { - const load = self.js_vtable.sload(self.js, addr, key) catch return null; + const load = self.js_vtable.sload(self.js, addr, key) catch { + self.ctx_error.* = context_mod.ContextError.database_error; + return null; + }; return .{ .value = load.data, .is_cold = load.is_cold }; } pub fn sstore(self: *Host, addr: primitives.Address, key: primitives.U256, val: primitives.U256) ?struct { original: primitives.U256, current: primitives.U256, new: primitives.U256, is_cold: bool } { - const result = self.js_vtable.sstore(self.js, addr, key, val) catch return null; + const result = self.js_vtable.sstore(self.js, addr, key, val) catch { + self.ctx_error.* = context_mod.ContextError.database_error; + return null; + }; return .{ .original = result.data.original_value, .current = result.data.present_value, @@ -452,7 +461,10 @@ pub const Host = struct { } pub fn selfdestruct(self: *Host, addr: primitives.Address, target: primitives.Address) ?SelfDestructLoadResult { - const result = self.js_vtable.selfdestruct(self.js, addr, target) catch return null; + const result = self.js_vtable.selfdestruct(self.js, addr, target) catch { + self.ctx_error.* = context_mod.ContextError.database_error; + return null; + }; return .{ .had_value = result.data.had_value, .target_exists = result.data.target_exists, @@ -847,7 +859,10 @@ fn recordCreateTargetCore( accel.keccak256(init_code, &init_hash); break :blk create2Address(caller, salt, init_hash); } else createAddress(caller, nonce); - const load = js.loadAccount(new_addr) catch return null; + const load = js.loadAccount(new_addr) catch { + host.ctx_error.* = context_mod.ContextError.database_error; + return null; + }; // is_account_alive: reference generic_create charges NEW_ACCOUNT only when the target // leaf does not already exist (balance/nonce/code present). const info = load.data.info; @@ -1261,3 +1276,101 @@ test "setupCreateCore marks ctx_error when the CREATE target cannot be loaded" { // The block must be rejected rather than accepting the fabricated failure. try std.testing.expectEqual(context_mod.ContextError.database_error, ctx.ctx_error); } + +// Reported by Cursor Bugbot on #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. Same fabricated success, different mask than the zero it +// replaced. These accessors now mark ctx_error like their siblings +// (blockHash, codeInfo) already did. +test "Host.sload marks ctx_error when the slot cannot be proven" { + var ctx = context_mod.Context(MissingTargetDb).new(.{}, primitives.SpecId.prague); + defer ctx.journaled_state.deinit(); + var host = Host.init(MissingTargetDb, &ctx, null); + + _ = try ctx.journaled_state.loadAccount(MissingTargetDb.CALLER); + try std.testing.expectEqual(context_mod.ContextError.ok, ctx.ctx_error); + + const result = host.sload(MissingTargetDb.CALLER, 1); + + try std.testing.expect(result == null); + try std.testing.expectEqual(context_mod.ContextError.database_error, ctx.ctx_error); +} + +test "Host.sstore marks ctx_error when the slot cannot be proven" { + var ctx = context_mod.Context(MissingTargetDb).new(.{}, primitives.SpecId.prague); + defer ctx.journaled_state.deinit(); + var host = Host.init(MissingTargetDb, &ctx, null); + + _ = try ctx.journaled_state.loadAccount(MissingTargetDb.CALLER); + try std.testing.expectEqual(context_mod.ContextError.ok, ctx.ctx_error); + + const result = host.sstore(MissingTargetDb.CALLER, 1, 42); + + try std.testing.expect(result == null); + try std.testing.expectEqual(context_mod.ContextError.database_error, ctx.ctx_error); +} + +// The remaining accessors that used to swallow database errors. All five are the +// same defect: a stateless witness that cannot answer becomes a plain `null`, the +// caller turns that into an EVM-level failure, and the block is accepted with a +// fabricated result. Each is pinned separately so reverting any one site alone +// fails a test. +test "Host.accountInfo marks ctx_error when the account cannot be proven" { + var ctx = context_mod.Context(MissingTargetDb).new(.{}, primitives.SpecId.prague); + defer ctx.journaled_state.deinit(); + var host = Host.init(MissingTargetDb, &ctx, null); + + const unknown: primitives.Address = @splat(0xE1); + try std.testing.expectEqual(context_mod.ContextError.ok, ctx.ctx_error); + + const result = host.accountInfo(unknown); + + try std.testing.expect(result == null); + try std.testing.expectEqual(context_mod.ContextError.database_error, ctx.ctx_error); +} + +test "Host.selfdestruct marks ctx_error when the target cannot be proven" { + var ctx = context_mod.Context(MissingTargetDb).new(.{}, primitives.SpecId.prague); + defer ctx.journaled_state.deinit(); + var host = Host.init(MissingTargetDb, &ctx, null); + + _ = try ctx.journaled_state.loadAccount(MissingTargetDb.CALLER); + const unknown: primitives.Address = @splat(0xE2); + try std.testing.expectEqual(context_mod.ContextError.ok, ctx.ctx_error); + + const result = host.selfdestruct(MissingTargetDb.CALLER, unknown); + + try std.testing.expect(result == null); + try std.testing.expectEqual(context_mod.ContextError.database_error, ctx.ctx_error); +} + +// recordCreateTargetCore mirrors setupCreateCore's pre-checks to decide whether to +// charge NEW_ACCOUNT gas, and loads the CREATE target the same way. It takes +// `js: anytype`, so the same stub injection works. +test "recordCreateTargetCore marks ctx_error when the CREATE target cannot be loaded" { + // Amsterdam: recordCreateTargetCore returns null immediately on earlier specs + // because EIP-7928 BAL recording is Amsterdam+ only, so it would never reach + // the target load and the test would pass for the wrong reason. + var ctx = context_mod.Context(MissingTargetDb).new(.{}, primitives.SpecId.amsterdam); + defer ctx.journaled_state.deinit(); + var host = Host.init(MissingTargetDb, &ctx, null); + + _ = try ctx.journaled_state.loadAccount(MissingTargetDb.CALLER); + try std.testing.expectEqual(context_mod.ContextError.ok, ctx.ctx_error); + + const result = recordCreateTargetCore( + &ctx.journaled_state, + &host, + MissingTargetDb.CALLER, + 0, + &[_]u8{0x00}, + false, + 0, + 0, + ); + + try std.testing.expect(result == null); + try std.testing.expectEqual(context_mod.ContextError.database_error, ctx.ctx_error); +}