debug_traceCallMany: strip filler blocks from overrides result

[grubbyTimes]

debug_traceCallMany overrides path returned traces for every simulated block, including the filler blocks SimulateTxExecutor inserts when BlockOverride.Number has gaps. Result count no longer matched the input bundles.

Compute each bundle's target block number (same overrideNumber ?? lastBlockNumber + 1 rule the executor uses) and filter simulationResult.Data down to that set. Regression test covers a non-contiguous gap between two bundles.

[LukaszRozmej]

@claude review

[claude]

Claude finished @LukaszRozmej's task in 4m 59s —— View job

---

Code Review

• Gather context and read changed files
• Read relevant rules files
• Analyze correctness, edge cases, and regressions
• Check security and performance
• Post review findings

---

Summary

The fix is logically correct and addresses a real bug. The bundleBlockNumbers pre-computation faithfully mirrors the executor's overrideNumber ?? lastBlockNumber + 1 formula, and the Where filter correctly strips filler blocks while preserving result order (since both the simulation output and the input bundles are in ascending block-number order). The regression test covers the critical gap scenario.

---

Findings

Medium — Logic coupling: duplicated block-number-resolution formula

File: src/Nethermind/Nethermind.JsonRpc/Modules/DebugModule/DebugRpcModule.cs:488-496

TraceCallManyWithOverrides now manually re-implements the same formula that SimulateTxExecutor.Execute uses to assign block numbers (lines 131-170 of SimulateTxExecutor.cs). There is no compile-time or runtime enforcement that keeps the two in sync. If the executor's numbering logic ever changes (e.g., a new filler condition, different handling of BlockOverrides == null), this filter will silently produce wrong results — either dropping real bundle traces or including filler ones — with no error surfaced to the caller.

The PR description acknowledges the formula is the same as the executor's, but doesn't address the maintenance risk. The right long-term fix is to either:

• Expose the bundle-to-block-number mapping as a shared helper (e.g., SimulateTxExecutor.ComputeBundleBlockNumbers), or
• Add a marker property to SimulateBlockResult<TTrace> (e.g., bool IsFiller) so callers don't need to know the numbering formula at all.

For now the duplication is safe, but this should be either addressed or explicitly acknowledged with rationale.

---

Low — No completeness check after filtering

IEnumerable<IEnumerable<GethLikeTxTrace>> bundleTraces = simulationResult.Data
    .Where(blockResult => blockResult.Number is long n && bundleBlockNumbers.Contains(n))
    .Select(blockResult => blockResult.Traces);

If (due to a future bug) a bundle's target block is absent from simulationResult.Data after a successful simulation, the caller silently receives fewer results than the number of input bundles — no error, no warning. A debug assertion or a post-filter count check would make the contract explicit:

// After filtering:
Debug.Assert(bundleTraces.Count() == bundles.Length, "Unexpected result count after filler-block filter");

This is low-severity because the current executor guarantees all requested blocks appear in the output, but defensive code here would catch regressions early.

---

Low — Test gap: only covers gap = 4 filler blocks

Debug_traceCallMany_with_block_number_gap_returns_one_entry_per_bundle uses headNumber + 1 → headNumber + 5, creating 3 filler blocks. It would be marginally more thorough to also test gap = 1 (i.e., headNumber + 1 → headNumber + 3, one filler block), since that boundary (givenNumber = lastBlockNumber + 2) is the smallest case that exercises the filler-insertion path. That said, the existing test is sufficient to validate the fix.

---

What's good

• The bundleBlockNumbers HashSet is sized to bundles.Length upfront — avoids rehashing.
• BlockParameter concreteBlockParameter = new(header.Number) pins the simulation to the already-validated header, avoiding a potential TOCTOU between the header lookup and the simulation call.
• The .Where(blockResult => blockResult.Number is long n && ...) pattern correctly handles the long? nullability of BlockForRpc.Number.
• The mutation side-effect (SimulateTxExecutor writes blockToSimulate.BlockOverrides.Number = givenNumber on the shared BlockOverride reference) does not affect correctness because bundleBlockNumbers is computed before Execute is called.

[grubbyTimes]

Added the gap=1 boundary case for "Test gap"; skipping "Logic coupling" (extracting a helper just relocates the duplication unlessSimulateTxExecutor also calls it — better as a separate IsFiller refactor PR) and "No completeness check" (Debug.Assert compiles out in Release, so it wouldn't guard prod).

[svlachakis]

@claude review

[claude]

Claude finished @svlachakis's task in 2m 0s —— View job

---

Code Review

• Read changed files and related executor code
• Review correctness, edge cases, and regressions
• Evaluate author responses to prior findings
• Check security and performance
• Post final findings

---

Summary

The fix is correct and the test improvements address the prior review's outstanding gaps. No new issues found.

---

Formula correctness — verified

The filter's block-number formula matches SimulateTxExecutor exactly:

Location	Formula
SimulateTxExecutor.cs:131	blockToSimulate.BlockOverrides.Number ?? (ulong)lastBlockNumber + 1
DebugRpcModule.cs:492-493	overrideNumber is not null ? (long)overrideNumber.Value : lastBlockNumber + 1

Both iterate over bundles in order and advance lastBlockNumber identically. The pre-computation runs before Execute is called, so the executor's mutation of blockToSimulate.BlockOverrides.Number at line 155 does not affect the pre-computed values. Correctness holds.

---

Author responses to prior findings — accepted

Finding	Response	Assessment
Medium — Logic coupling	Declined: extracting a helper relocates duplication unless the executor also calls it; prefers a separate IsFiller refactor PR	Technically sound. The coupling is real but fixing it properly requires a deeper change. The rationale is explicit and on-record.
Low — No completeness check	Declined: Debug.Assert compiles out in Release	Correct — [Conditional("DEBUG")] means it provides no prod guard. Valid rejection.
Low — Test gap (gap=1)	Added secondBundleOffset = 3 test case	Correct coverage. headNumber+1 → headNumber+3 produces exactly 1 filler block at headNumber+2, the minimum filler-insertion path.

---

Minor nit (non-blocking)

Debug_traceCallMany_with_minimum_block_number_gap_returns_one_entry_per_bundle — the "gap" here refers to 2 block numbers apart (1 filler), which could be confused with a 1-block-number gap (no filler). A name like with_one_filler_block or with_minimum_filler_block would be more precise, but this is purely cosmetic.

---

Verdict

All prior findings have been addressed or acknowledged with clear rationale. The fix is correct, safe, and well-tested.