fix(simulate): return 0 for unresolvable BLOCKHASH in eth_simulateV1 instead of throwing

[kamilchodola]

Summary

eth_simulateV1 can run transactions whose BLOCKHASH opcode targets an ancestor block whose hash can't be resolved in the simulate context. Today the shared BlockhashProvider throws InvalidDataException ("Hash cannot be found when executing BLOCKHASH operation"), which fails the entire eth_simulateV1 request with -32603.

eth_simulateV1 is best-effort. Per EVM semantics, BLOCKHASH returns 0 for out-of-window/unavailable blocks, so the simulate-only decorator (SimulateBlockhashProvider) now catches that case and returns null (→ the EVM pushes 0) instead of failing the whole request.

Scope / safety

• Touches only SimulateBlockhashProvider (the eth_simulateV1-only decorator). Canonical block processing is unaffected — it always has the hash available and never hits this path.
• ~16 lines, no behavior change for normal simulate calls.

Found while using eth_simulateV1 for benchmark cache-warming; it's an independent robustness improvement.

[claude]

Claude finished @kamilchodola's task in 2m 4s —— View job

---

Review

• Gather context
• Read changed files and rules
• Analyze correctness, edge cases, security, performance
• Post findings

---

The fix is correct and minimal. The root cause is clear: BlockhashProvider.GetBlockhash (line 52) throws InvalidDataException when blockhashCache.GetHash returns null — a case that only arises in the simulate context where the full hash ancestry isn't cached. Catching that exception in the simulate-only decorator and returning null is the right thing to do: the EVM will push 0, which is exactly the specified behavior for an unavailable block hash.

Medium — Missing regression test

AGENTS.md mandates: "When fixing a bug, always add a regression test." No test was added. A unit test for SimulateBlockhashProvider should cover two cases:

1. When the inner IBlockhashProvider throws InvalidDataException, GetBlockhash returns null rather than propagating.
2. The normal flow (inner provider returns a hash or null) passes through unchanged.

This is easy to write with a mock — the SimulateBlockhashProvider class has no dependencies that need a full integration setup.

Fix this →

Low — InvalidDataException catch is a contract dependency on a concrete type

The catch relies on BlockhashProvider throwing InvalidDataException specifically for cache-miss. This is an internal implementation detail of BlockhashProvider (not part of IBlockhashProvider's contract). If that implementation changes (e.g. returns null directly or throws a different exception), the catch becomes a silent no-op.

A more principled long-term fix would be to have BlockhashProvider return null for cache misses and only throw for truly unexpected states (e.g. database corruption), but that's a larger change. The comment in the code does explain the coupling clearly, which mitigates the risk.

Low — Comment accuracy

The comment says "return 0 (the EVM BLOCKHASH out-of-window result)" — but this isn't an out-of-window case (depth is valid). The hash is simply absent from the simulate cache. The out-of-window vs. cache-miss distinction matters for future readers trying to understand why null is correct here. Consider: "return 0 per EVM BLOCKHASH semantics for unavailable ancestor hashes".

---

Summary: The behavior change is correct and safe. The only blocking concern per repo rules is the missing regression test.

[kamilchodola]

@claude re-review

[LukaszRozmej]

We should inject blockhashProvider that does this internally instead of catching exceptions

[Marchhill]

could handle this by adding a TryGetBlockHash method that returns nullable result

[claude]

Claude finished @kamilchodola's task in 2m 54s —— View job

---

Re-review

• Read changed files and rules
• Analyze against prior review and LukaszRozmej's change request
• Post findings

---

Progress since first review:

• Tests added ✅ — SimulateBlockhashProviderTests.cs covers the exception path, normal flow, and the BestKnownNumber clamping. The previous medium concern is resolved.
• Comment accuracy concern (previous low) no longer applies — the catch site has no comment.

---

Remaining issues:

Medium — Unresolved CHANGES_REQUESTED (architectural coupling)

@LukaszRozmej's change request is still unaddressed. The catch (InvalidDataException) block in SimulateBlockhashProvider couples the decorator to a private implementation detail of BlockhashProvider — the ?? throw new InvalidDataException(...) on line 52 of BlockhashProvider.cs is not part of IBlockhashProvider's interface contract. If that implementation changes, the catch silently becomes dead code.

The reviewer's preferred direction: the null-safe behaviour should live in an injected IBlockhashProvider, not in the catch block of its consumer. Two clean options:

1. New null-safe decorator registered before SimulateBlockhashProvider in the DI chain (SimulateReadOnlyBlocksProcessingEnvFactory.cs line 57): a thin wrapper that catches InvalidDataException and returns null. Remove the try/catch from SimulateBlockhashProvider.
2. Change BlockhashProvider to return null instead of throwing for cache misses — treating cache miss as "hash not available" (semantically the same as out-of-window). This is simpler but changes canonical-processing behavior.

Fix this →

---

Summary: The behavior fix is correct and the tests now exist. The only blocking item is @LukaszRozmej's architectural CHANGES_REQUESTED: the exception-swallowing should live in a purpose-built injectable provider, not as a catch clause in SimulateBlockhashProvider.

[claude]

Medium — Unresolved CHANGES_REQUESTED: exception catch couples to BlockhashProvider's internals

@LukaszRozmej's concern is valid and unaddressed. SimulateBlockhashProvider is consuming an exception that is thrown by a specific implementation detail of BlockhashProvider (line 52 of BlockhashProvider.cs):

: blockhashCache.GetHash(currentBlock, (int)depth)
  ?? throw new InvalidDataException("Hash cannot be found when executing BLOCKHASH operation")

That throw is not part of IBlockhashProvider's contract — it's an internal invariant of BlockhashProvider. If the implementation changes (returns null directly, throws a different type, or the condition changes), this catch silently becomes a no-op.

The fix suggested by LukaszRozmej: inject a null-safe IBlockhashProvider into the simulate scope that returns null for cache misses rather than throwing. One clean path:

1. In SimulateReadOnlyBlocksProcessingEnvFactory, add a decorator before SimulateBlockhashProvider in the DI chain that catches InvalidDataException and returns null — keeping the exception-swallowing logic in a single dedicated type rather than spread across the decorator hierarchy.
2. Remove the try/catch from SimulateBlockhashProvider entirely.

OR: modify BlockhashProvider itself to return null for cache misses (treating them as "hash not available", semantically equivalent to out-of-window). This is the simpler option but changes canonical-processing behaviour that the current throw was presumably guarding.

[Marchhill]

could handle this by adding a TryGetBlockHash method that returns nullable result

[AnkushinDaniil]

would it be worth a if (_logger.IsTrace) _logger.Trace(...) here, like the existing out-of-window path does? Just so a real corruption case doesn't go completely silent