Alchemy - Code Fix

[svlachakis]

Moved the persisted-code hint cache from StateProvider onto the ICodeDb itself, where the underlying storage actually lives.

Before: StateProvider._persistedCodeInsertFilter was a single long-lived in-memory cache that recorded "this code hash has been flushed to disk". It was set after every CommitCodeAsync, regardless of whether that commit went to durable production storage or a transient overlay storage.

After: the hint cache lives on the codeDb instance via ICodeDb.ContainsCode / MarkCodePersisted. Two implementations:

• Production codeDb (isPersistent: true) — keeps the hint cache, gets the optimization for normal block sync where factory contracts redeploy popular bytecode.
• Overlay codeDb (isPersistent: false, used by debug_traceCall / eth_call / state overrides) — hint cache is null. ContainsCode always returns false, MarkCodePersisted is a no-op. Overlay writes can no longer poison anything.

The bug:

1. debug_traceCall runs through a scoped overlay — a temporary in-memory layer wrapping the production codeDb. Writes during the trace land in the overlay's temp buffer.
2. On commit, CommitCodeAsync writes the just-deployed bytecode into the overlay and then calls _persistedCodeInsertFilter.Set(codeHash) — marking it "persisted."
3. The trace ends. The overlay's temp buffer is discarded (ClearTempChanges). Production codeDb never received those bytes.
4. _persistedCodeInsertFilter lived on StateProvider, outliving the overlay. So the next trace on the same pooled GethStyleTracer instance saw a "persisted" hint for code that does not exist in production.
5. InsertCode consulted the hint, said "already there, skip", and didn't put the bytes into _codeBatch. The account's codeHash was still set to 0xb44f…ea55.
6. ProcessDiffState then called GetCode(addr) → codeHash 0xb44f… → not in _codeBatch, not in overlay, not in production → throw.

The root issue was a lifetime mismatch: the hint cache's lifetime (long-lived on StateProvider) didn't match the lifetime of the storage it claimed to describe (short-lived overlay).

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Added regression test which fails on master.

[svlachakis]

@stdevMac needs to be cherry picked in 1.38.0

[claude]

Claude finished @svlachakis's task in 6m 30s —— View job

---

Code Review

• Gather context and understand the request
• Read coding standards and rules
• Read all changed files
• Analyze for correctness, edge cases, security, and performance
• Post inline comments for specific issues
• Post final review summary

---

Summary

The root-cause analysis is accurate and the fix is well-targeted. The bug was a lifetime mismatch: the persisted-code hint cache lived on the long-lived StateProvider but was invalidated by short-lived overlay scopes (used by debug_traceCall / eth_call). Moving the cache onto the ICodeDb instance ties its lifetime to the actual storage it describes.

Verdict: Approve with minor comments. No correctness regressions found. The three findings below are low-to-medium; none block merge.

---

Findings

Medium — IWorldStateScopeProvider.ICodeDb is a public interface (breaking change)

Adding ContainsCode and MarkCodePersisted to the ICodeDb nested interface is technically a breaking change for any external code that implements it, even though the PR is labelled "non-breaking bugfix". Within the Nethermind codebase there is only one concrete implementation (KeyValueWithBatchingBackedCodeDb), so there are no in-tree breakages. However, any plugin or library targeting Nethermind.Evm that implements ICodeDb will fail to compile after this change.

Adding default implementations with the overlay semantics (always-false / no-op) would eliminate the break entirely and also self-document that the safe default is the overlay behaviour:

bool ContainsCode(in ValueHash256 codeHash) => false;
void MarkCodePersisted(in ValueHash256 codeHash) { }

See inline comment on IWorldStateScopeProvider.cs:92.

Low — OverridableWorldStateManager silently relies on codeDbIsPersistent = false default

OverridableWorldStateManager (not modified by this PR) creates:

WorldState = new TrieStoreScopeProvider(overlayTrieStore, readOnlyDbProvider.CodeDb, logManager);

This is one of the affected overlay paths — it is correct precisely because the new default is false. But it is silent about its intent. Consider adding codeDbIsPersistent: false explicitly (as a follow-up commit) so future readers know this is a deliberate overlay path, not an oversight. Fix this →

Low — _persistedHint cache size comment

The new _persistedHint = new AssociativeKeyCache<ValueHash256>(1_024) uses 1 024 entries but there is no comment explaining the sizing. The _blockCodeInsertFilter comment explains its 256-entry limit ("guarding hot duplicates within the current block"). The 1 024 entry cross-block hint would benefit from similar justification. See inline comment on TrieStoreScopeProvider.cs:278.

---

Correctness analysis

Scenario	Before	After
debug_traceCall deploys code → overlay reset → second call same code	Throws Code 0x… missing	✓ Returns code
Production block processing: same factory bytecode deployed many blocks	Skips DB write (cross-block hint) ✓	Same — _persistedHint on the durable codeDb ✓
Same code deployed twice in same block	Skips second _codeBatch insert ✓	Same — _blockCodeInsertFilter ✓
WorldStateManager.CreateResettableWorldState() (read-only)	N/A	codeDbIsPersistent: false default → no hint cache ✓
HealingWorldStateScopeProvider	Was using old single filter	codeDbIsPersistent: true passed explicitly ✓

Thread safety: AssociativeKeyCache uses lock-free seqlock reads and Volatile.Write for mutations — it is safe for concurrent access from the main thread (ContainsCode) and the Task.Run background thread (MarkCodePersisted).

_codeDb null guard in InsertCode: _codeDb?.ContainsCode(codeHash) ?? false is fail-open (false-negative), so code is redundantly re-inserted rather than silently skipped if _codeDb is null — correct direction.

Regression tests: Both KeyValueWithBatchingBackedCodeDb_ContainsCode_respects_isPersistent_flag (unit) and Same_code_can_be_redeployed_across_overlay_resets (integration) directly exercise the failure path. The integration test correctly relies on worldState.BeginScope dispose calling Reset() (which clears _blockCodeInsertFilter) between the two overlay passes.

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 3m 24s —— View job

---

Code Review

• Gather context and understand the request
• Read coding standards and rules
• Read all changed files
• Analyze for correctness, edge cases, security, and performance
• Post inline comments for specific issues
• Post final review summary

---

Summary

This is a re-review after the previous round of feedback. All three findings from the first review have been addressed:

Previous finding	Status
Medium — ICodeDb interface breaking change for external implementors	✅ Resolved — default implementations added (=> false / { })
Low — OverridableWorldStateManager silently relying on codeDbIsPersistent = false default	✅ Resolved — now passes codeDbIsPersistent: false explicitly
Low — _persistedHint cache size comment missing	✅ Resolved — comment now explains the 1 024-entry rationale

---

Correctness analysis

Scenario	Result
debug_traceCall deploys code → overlay reset → second call same code	✓ Fixed: overlay _persistedHint is null, ContainsCode always false
Production block sync: same factory bytecode across many blocks	✓ _persistedHint on durable codeDb still short-circuits redundant writes
Same code deployed twice in the same block	✓ _blockCodeInsertFilter (256-entry, intra-block) prevents duplicate inserts
OverridableWorldStateManager (debug/call overlays)	✓ codeDbIsPersistent: false explicit
HealingWorldStateScopeProvider (production)	✓ codeDbIsPersistent: true explicit
FlatScopeProvider read-only path	✓ isPersistent: !isReadOnly correct
NullCodeDb in benchmark stubs	✓ Explicitly overrides with correct no-op semantics (redundant but not wrong)

Thread safety: MarkCodePersisted runs inside the background Task.Run(PersistCodeBatch). The main thread calls codeFlushTask.GetAwaiter().GetResult() before the next block's InsertCode can run — no race on _persistedHint.

_codeDb null guard in InsertCode: _codeDb?.ContainsCode(codeHash) ?? false is fail-open (false-negative), so code is re-inserted rather than silently skipped when _codeDb is null — correct direction.

---

Remaining finding

Low — Session-specific test comment

StateProviderTests.cs:305–306 still says:

// One long-lived overridable scope reused across two BeginScope calls — mirrors
// OverridableEnv reusing _worldState across BuildAndOverride.

OverridableEnv and BuildAndOverride are not identifiers in the codebase. The comment describes PR context, not a stable invariant. Inline suggestion posted.

---

Verdict: Approve. The bug fix is correct and well-targeted. The interface change is non-breaking. All call sites have the right isPersistent / codeDbIsPersistent value. Regression tests directly exercise the failure path. The single remaining item (test comment) is cosmetic and does not block merge.

[claude]

Claude finished @svlachakis's task in 4m 56s —— View job

---

Code Review

• Gather context and understand the request
• Read coding standards and rules
• Read all changed files
• Analyze for correctness, edge cases, security, and performance
• Post final review summary

---

Summary

This is a complete re-review. All findings from both prior review rounds have been addressed. The implementation is correct, well-tested, and safe.

---

Previously flagged findings — all resolved

Finding	Status
Medium — ICodeDb interface breaking change	✅ Default implementations added (=> false / { })
Low — OverridableWorldStateManager implicit false	✅ codeDbIsPersistent: false now explicit
Low — _persistedHint cache size undocumented	✅ Comment explains 1 024-entry rationale
Low — Session-specific test comment ("OverridableEnv", "BuildAndOverride")	✅ Removed; no such comment exists in the current file

---

Unmodified TrieStoreScopeProvider instantiation sites — all verified correct

The search turned up four sites not touched by this PR. Each is correctly using the default codeDbIsPersistent: false:

Site	Code db	Why false is correct
WorldStateManager.CreateResettableWorldState()	_readaOnlyCodeCb — ReadOnlyDb(…, true) overlay	Temp-overlay writes are discarded on reset; true would re-introduce the same lifetime bug for block producers
FlatOverridableWorldScope	new ReadOnlyDb(codeDb, true) overlay	Same overlay semantics
StatelessBlockProcessingEnv	witness.CreateCodeDb() — witness-backed, not RocksDB	No persistent store to cache against
WitnessGeneratingBlockProcessingEnvFactory	readOnlyDbProvider.CodeDb — ReadOnlyDb overlay	Temp overlay; writes discarded

---

Correctness analysis

Scenario	Result
debug_traceCall deploys code → overlay reset → second call same code	✓ Fixed: _persistedHint is null on overlay codeDb; ContainsCode always false
Production block sync: same factory bytecode across many blocks	✓ _persistedHint (1 024-entry) on durable codeDb still short-circuits redundant writes
Same code deployed twice in the same block	✓ _blockCodeInsertFilter (256-entry, intra-block, cleared on Reset) prevents duplicate inserts
OverridableWorldStateManager (debug/call overlays)	✓ codeDbIsPersistent: false explicit
HealingWorldStateScopeProvider (production)	✓ codeDbIsPersistent: true hardcoded in constructor
Non-healing production path (PruningTrieStateFactory)	✓ codeDbIsPersistent: true explicit
FlatScopeProvider	✓ isPersistent: !isReadOnly — write paths durable, read-only paths overlay

Thread safety: MarkCodePersisted runs in Task.Run(PersistCodeBatch). The main thread blocks via codeFlushTask.GetAwaiter().GetResult() before the next block's InsertCode can run — no race on _persistedHint. AssociativeKeyCache seqlock reads and Volatile.Write mutations are safe for this access pattern.

_codeDb null guard in InsertCode: _codeDb?.ContainsCode(codeHash) ?? false is fail-open (false-negative direction) — code is re-inserted rather than silently skipped when _codeDb is null. Correct.

MarkCodePersisted ordering: Called after the using (ICodeSetter batch = codeDb.BeginCodeWrite()) block completes — code is durably written before the hint is set. Correct.

Regression tests: KeyValueWithBatchingBackedCodeDb_ContainsCode_respects_isPersistent_flag (parameterized unit) and Same_code_can_be_redeployed_across_overlay_resets (integration, both trie and flat backends) directly exercise the failure path. The integration test correctly disposes the scope (triggering Reset() which clears _blockCodeInsertFilter) and calls ResetOverrides() between the two overlay passes.

---

Verdict: Approve. No outstanding findings. The fix is minimal, correct, and well-tested.

[svlachakis]

@claude review my regression tests we should have one unit, one in state module and one full end to end on rpc layer

[claude]

Claude finished @svlachakis's task in 4m 24s —— View job

---

Regression Test Review

• Gather context and understand the request
• Read coding standards and test rules
• Read all changed test files
• Identify unit test, state module test, and RPC end-to-end test
• Analyze coverage, correctness, and gaps
• Post final review summary

---

Coverage map

All three layers are present:

Layer	Test	File
Unit	KeyValueWithBatchingBackedCodeDb_ContainsCode_respects_isPersistent_flag	Nethermind.State.Test/StateProviderTests.cs:274
State module	Same_code_can_be_redeployed_across_overlay_resets	Nethermind.State.Test/StateProviderTests.cs:287
RPC end-to-end	Debug_traceCall_CREATE_replayed_back_to_back_does_not_throw_code_missing	Nethermind.JsonRpc.Test/Modules/DebugRpcModuleTests.cs:248

---

Findings

Medium — Unit test runs 4× instead of 2×

StateProviderTests is [TestFixture(false)][TestFixture(true)]. The unit test (KeyValueWithBatchingBackedCodeDb_ContainsCode_respects_isPersistent_flag) does not use useFlat at all, so NUnit runs it twice redundantly — once per fixture variant. Combined with its own [TestCase] parameterization it executes 4 times. Consider moving it to a dedicated [TestFixture] CodeDbTests class. See inline comment.

Low — RPC test has no content assertion

Debug_traceCall_CREATE_replayed_back_to_back_does_not_throw_code_missing only asserts BeOfType<JsonRpcSuccessResponse> — it doesn't verify the deployed bytecode appears in the prestate diff. A regression where the trace silently returns wrong data (no exception, wrong code) would slip past. See inline comment.

---

What each test exercises

Unit (KeyValueWithBatchingBackedCodeDb_ContainsCode_respects_isPersistent_flag)
Directly instantiates KeyValueWithBatchingBackedCodeDb with isPersistent: true/false, calls MarkCodePersisted, and asserts ContainsCode matches. No state layer involved. Directly locks in the invariant at the lowest level. ✓

State module (Same_code_can_be_redeployed_across_overlay_resets)
Creates an IOverridableWorldScope, deploys code (triggering CommitCodeAsync which flushes the code batch and calls MarkCodePersisted), resets the overlay, then redeploys the same hash in a fresh scope. Runs against both flat and trie backends via the [TestFixture] parameterization. Correctly reproduces the lifetime-mismatch scenario. ✓

RPC end-to-end (Debug_traceCall_CREATE_replayed_back_to_back_does_not_throw_code_missing)
Sends the same CREATE transaction to debug_traceCall three times against the same pooled IDebugRpcModule. The comment explains why 3 iterations are used — the bug first surfaces on call 2, the third makes it deterministic. The init code is carefully crafted so the deployed runtime is non-empty (codeHash != keccak("")), which is the path that actually hits the codeDb on GetCode. ✓

---

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 4m 38s —— View job

---

Code Review

• Gather context and understand the request
• Read coding standards and rules
• Read all changed files
• Analyze for correctness, edge cases, security, and performance
• Post final review summary

---

Summary

This is a full re-review of the current branch state. All findings from the three prior review rounds have been resolved. The implementation is correct, well-tested, and safe.

---

Previously flagged findings — all resolved

Finding	Status
Medium — ICodeDb interface breaking change for external implementors	✅ Default implementations added (=> false / { })
Low — OverridableWorldStateManager implicit codeDbIsPersistent: false	✅ Now explicit
Low — _persistedHint cache size undocumented	✅ Comment explains 1 024-entry rationale
Low — Session-specific test comment ("OverridableEnv", "BuildAndOverride")	✅ Removed
Medium — Unit test KeyValueWithBatchingBackedCodeDb_… running 4×	✅ Extracted to CodeDbTests class (no fixture parameterization)
Low — RPC test has no content assertion	✅ result.Should().Contain("\"code\":\"0x00\"") added

---

Correctness analysis

Scenario	Result
debug_traceCall deploys code → overlay reset → second call same code	✓ Fixed: overlay _persistedHint is null; ContainsCode always false
Production block sync: same factory bytecode across many blocks	✓ _persistedHint (1 024-entry) on durable codeDb still short-circuits redundant writes
Same code deployed twice in the same block	✓ _blockCodeInsertFilter (256-entry, cleared on Reset) prevents duplicate inserts
OverridableWorldStateManager	✓ codeDbIsPersistent: false explicit
HealingWorldStateScopeProvider	✓ codeDbIsPersistent: true hardcoded
PruningTrieStateFactory production path	✓ codeDbIsPersistent: true explicit
FlatScopeProvider	✓ isPersistent: !isReadOnly — write paths durable, read-only paths transient
WorldStateManager.CreateResettableWorldState()	✓ codeDbIsPersistent: false (default) — _readaOnlyCodeCb is a ReadOnlyDb overlay; writes are transient
FlatOverridableWorldScope	✓ codeDbIsPersistent: false (default) — _codeDbOverlay is new ReadOnlyDb(codeDb, true); writes are transient

CommitCodeAsync ordering in test: worldState.Commit(spec) invokes the extension method with commitRoots: true, which calls CommitCodeAsync. The background Task.Run(PersistCodeBatch) completes before scope disposal (blocked by GetAwaiter().GetResult()). MarkCodePersisted is called inside PersistCodeBatch after the batch write completes — code is on disk before the hint is set. ✓

Reset() clears _blockCodeInsertFilter: On scope disposal, Reset(resetBlockChanges: true) clears both _blockCodeInsertFilter and _codeBatch. So in the second scope, the only filter consulted is _codeDb.ContainsCode(codeHash), which returns false (overlay, _persistedHint is null). InsertCode adds to _codeBatch. GetCode finds it there. ✓

Thread safety: MarkCodePersisted runs in Task.Run(PersistCodeBatch); the main thread blocks via GetAwaiter().GetResult() before the next block's InsertCode can run — no race on _persistedHint. AssociativeKeyCache seqlock reads and Volatile.Write mutations are safe for concurrent access. ✓

_codeDb null guard: _codeDb?.ContainsCode(codeHash) ?? false is fail-open (false-negative) — code is re-inserted rather than silently skipped when _codeDb is null. ✓

---

Remaining notes (pre-existing, unmodified files — not blocking)

Two call sites in unmodified files still use the implicit isPersistent: false default rather than the explicit named parameter:

• FlatOverridableWorldScope.cs:47: new TrieStoreScopeProvider.KeyValueWithBatchingBackedCodeDb(_codeDbOverlay) — _codeDbOverlay is new ReadOnlyDb(codeDb, true), so false is correct, just implicit.
• WorldStateManager.cs:64 CreateResettableWorldState(): new TrieStoreScopeProvider(_readOnlyTrieStore, _readaOnlyCodeCb, _logManager) — _readaOnlyCodeCb is a ReadOnlyDb overlay, so false is correct, just implicit.

These are harmless and pre-date this PR; worth a follow-up pass to make intent explicit, but do not block merge.

---

Verdict: Approve. No outstanding findings. The fix is minimal, correct, and well-tested across all three layers (unit, state module, RPC end-to-end).

[LukaszRozmej]

not needed as there is default implementation in interface?

[LukaszRozmej]

same here

[LukaszRozmej]

IMO more readable:

Suggested change

	if (!_blockCodeInsertFilter.Get(codeHash) && !(_codeDb?.ContainsCode(codeHash) ?? false))
	if (!_blockCodeInsertFilter.Get(codeHash) && !(_codeDb?.ContainsCode(codeHash) == true))

[LukaszRozmej]

Does this supersede #11712? Should we close that one?

[svlachakis]

@LukaszRozmej I've left #11712 in case we wanted to release 1.37.3 because it's based of from release and for the customer image diff, looks like not so I'm closing it