fix(rpc): emit spec message for eth_simulateV1 -38020#11665
Conversation
The execution-apis spec defines error -38020 (`InvalidInputBlocksOutOfOrder`)
with a fixed message: "Block number in sequence did not increase". Nethermind
was returning a verbose `$"Block number out of order {given} is <= than previous
block number of {prev}!"` that fails strict spec conformance tests.
Add `SimulateErrorMessages.BlockNumberNotIncreasing` and use it at the
validation site so the test class continues to be the single source of truth
for canonical eth_simulateV1 error messages.
Regression test asserts both the error code (-38020) and the verbatim spec
message on a SimulatePayload whose second BlockStateCall has a strictly lower
Number than the first.
Fixes NethermindEth#11219
|
@claude review |
|
Claude finished @LukaszRozmej's task in 1m 30s —— View job Review
The fix is minimal, correct, and well-structured. The verbose, value-interpolated message is replaced with the spec-mandated constant, and the new constant is co-located with the other spec-mandated messages in Low — Assertion style inconsistency in new testThe new test at line 274–275 uses FluentAssertions ( // New test (lines 274-275) – FluentAssertions
result.ErrorCode.Should().Be(ErrorCodes.InvalidInputBlocksOutOfOrder);
result.Result!.Error.Should().Be(SimulateErrorMessages.BlockNumberNotIncreasing);
// Existing error test (lines 310-311) – NUnit
Assert.That(result.Result!.Error, Is.EqualTo(SimulateErrorMessages.InsufficientFunds));
Assert.That(result.ErrorCode, Is.EqualTo(ErrorCodes.InsufficientFunds));Low — Sibling error at line 161 has the same verbose-message pattern
// SimulateTxExecutor.cs:161
return ResultWrapper<...>.Fail(
$"Block timestamp out of order {blockToSimulate.BlockOverrides.Time} is <= than given base timestamp of {lastBlockTime}!",
ErrorCodes.BlockTimestampNotIncreased);If the execution-apis spec mandates a fixed message for Overall: clean fix, no blocking issues. |
…urce Collapse the standalone -38020 regression test plus the five existing per-error tests into one parameterized eth_simulateV1_returns_spec_error, fed by static payload factories. Drops ~125 lines of duplicated chain- setup + assertion boilerplate while keeping per-case names visible in the test runner. sender_is_not_eoa kept separate (needs a custom EIP-3607 chain). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
svlachakis
left a comment
There was a problem hiding this comment.
this is possible to change in the spec, but let's merge for now
Fixes #11219
Changes
SimulateTxExecutor.Executereturned-38020(InvalidInputBlocksOutOfOrder) with a verbose, value-interpolated message:$"Block number out of order {given} is <= than previous block number of {prev}!". The execution-apis spec mandates a fixed message:"Block number in sequence did not increase".SimulateErrorMessages.BlockNumberNotIncreasingso this canonical string sits next to the other spec-mandated messages (IntrinsicGas,FeeCapBelowBaseFee,InsufficientFunds) already shared with the test suite.SimulateTxExecutor.cs:137).Test_eth_simulate_returns_spec_message_when_block_numbers_not_increasinginEthSimulateTestsBlocksAndTransactions. Builds aSimulatePayloadwith a secondBlockStateCallwhoseNumberis strictly less than the first and asserts bothErrorCode == -38020andError == SimulateErrorMessages.BlockNumberNotIncreasing.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
EthSimulateTestsBlocksAndTransactionsexercises the error path end-to-end throughSimulateTxExecutor.Execute.EthSimulateTestsBlocksAndTransactionstests pass.dotnet format whitespace --folderclean.Documentation
Requires documentation update
Requires explanation in Release Notes