Corrected error codes for eth simulate #11430
Conversation
|
Claude finished @DarkLord017's task in 2m 31s —— View job PR Review:
|
|
@claude review |
|
Claude finished @DarkLord017's task in 3m 10s —— View job Code Review:
|
| Condition | Old | New |
|---|---|---|
TransactionNonceTooLow |
-32603 (InternalError) |
-38010 (NonceTooLow) |
TransactionNonceTooHigh |
-32603 (InternalError) |
-38011 (NonceTooHigh) |
SenderHasDeployedCode |
-32602 (InvalidParams) |
-38024 (SenderIsNotEoa) |
The error code values themselves are correct per the eth_simulateV1 specification.
Findings
Medium — Missing regression tests
AGENTS.md explicitly requires: "When fixing a bug, always add a regression test." The PR checks "No testing required," but all three changes are observable API behaviour fixes.
The test infrastructure for this pattern already exists — EthSimulateTestsBlocksAndTransactions.cs contains tests that assert result.ErrorCode against specific ErrorCodes.* values (e.g. lines 274, 608, 656, 701). Three analogous tests are missing:
NonceTooLow— call with"validation": trueand a nonce below the account nonce → assertErrorCode == ErrorCodes.NonceTooLow(-38010)NonceTooHigh— call with"validation": trueand an explicit nonce above the current nonce → assertErrorCode == ErrorCodes.NonceTooHigh(-38011)SenderIsNotEoa— call whosefromaddress has deployed code (EIP-3607) with"validation": true→ assertErrorCode == ErrorCodes.SenderIsNotEoa(-38024)
Without these tests, a future refactor could silently regress the fix. Fix this →
Low — Trailing whitespace in XML doc comment
ErrorCodes.cs line 152 has a trailing space:
/// Transaction nonce is higher than the account's current nonce·
Should be trimmed. Fix this →
Low — Out-of-order error code placement
The new constants break the loose numeric ordering of the eth_simulate error code block:
-38010(NonceTooLow) and-38011(NonceTooHigh) are declared after-38015(BlockGasLimitReached)-38024(SenderIsNotEoa) is declared after-38025(MaxInitCodeSizeExceeded)
Not a functional issue, but makes the file harder to navigate. Prefer inserting constants in ascending numeric order within the group.
Verdict
The core logic is correct and the error codes match the spec. The one medium finding (missing regression tests) is a repo requirement violation per AGENTS.md. Please add tests for all three error code paths before merging.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
DarkLord017
force-pushed
the
error_code_fix
branch
from
5eaba79 to
278bf30
Compare
|
@claude re-review |
|
Claude finished @DarkLord017's task in 2m 2s —— View job Re-review:
|
| Test | Error path | Expected code |
|---|---|---|
eth_simulateV1_nonce_too_low_returns_spec_error_code |
TransactionNonceTooLow |
-38010 |
eth_simulateV1_nonce_too_high_returns_spec_error_code |
TransactionNonceTooHigh |
-38011 |
eth_simulateV1_sender_is_not_eoa_returns_spec_error_code |
SenderHasDeployedCode |
-38024 |
The sender_is_not_eoa test correctly uses TestSpecProvider { AllowTestChainOverride = false } with IsEip3607Enabled = true — this is the right pattern (confirmed by TestBlockchain.cs:274).
Low — Numeric ordering ✅ Fixed
NonceTooLow = -38010 and NonceTooHigh = -38011 are now declared before FeeCapBelowBaseFee = -38012 through BlockGasLimitReached = -38015. SenderIsNotEoa = -38024 is correctly placed before MaxInitCodeSizeExceeded = -38025.
Low — Trailing whitespace ✅ Fixed
The trailing space on the NonceTooHigh doc comment has been removed.
Core Logic
The three switch-arm changes in SimulateTxExecutor.cs are correct:
SenderHasDeployedCode→ErrorCodes.InvalidParams(-32602) →ErrorCodes.SenderIsNotEoa(-38024) ✓TransactionNonceTooHigh→ErrorCodes.InternalError(-32603) →ErrorCodes.NonceTooHigh(-38011) ✓TransactionNonceTooLow→ErrorCodes.InternalError(-32603) →ErrorCodes.NonceTooLow(-38010) ✓
All values match the eth_simulateV1 specification.
Verdict
Mergeable ✅ — No remaining findings. All three error code remappings are spec-correct, regression tests cover each changed path, and the ErrorCodes.cs constant ordering is clean.
DarkLord017
changed the title
DarkLord017
requested review from
LukaszRozmej,
benaadams,
smartprogrammer93 and
svlachakis
as code owners
|
Claude finished @DarkLord017's task in 3m 48s —— View job PR Review:
|
| Condition | Before | After |
|---|---|---|
TransactionNonceTooLow |
-32603 (InternalError) |
-38010 (NonceTooLow) ✅ |
TransactionNonceTooHigh |
-32603 (InternalError) |
-38011 (NonceTooHigh) ✅ |
SenderHasDeployedCode |
-32602 (InvalidParams) |
-38024 (SenderIsNotEoa) ✅ |
All values match the eth_simulateV1 specification. Prior review findings (numeric ordering, trailing whitespace, missing regression tests) have all been addressed.
Core Logic — SimulateTxExecutor.cs
The three switch arm changes at lines 258–262 are correct. The validation order in TransactionProcessor is:
ValidateSender→SenderHasDeployedCode(EIP-3607)BuyGas→ fee validationIncrementNonce→ nonce validation
All three conditions are on distinct code paths, so the changes are orthogonal and cannot interfere with each other.
Constants — ErrorCodes.cs
NonceTooLow = -38010 and NonceTooHigh = -38011 are correctly inserted before FeeCapBelowBaseFee = -38012. SenderIsNotEoa = -38024 is correctly placed before MaxInitCodeSizeExceeded = -38025. Numeric ordering within the eth_simulateV1 group is clean.
Regression Tests — EthSimulateTestsBlocksAndTransactions.cs
Three tests added, each asserting result.ErrorCode against the specific constant:
eth_simulateV1_nonce_too_low_returns_spec_error_code— sets account nonce to 10 via state override, sends tx with nonce 0 → expects-38010✅eth_simulateV1_nonce_too_high_returns_spec_error_code— sends tx with explicit nonce 100 against account nonce 0 → expects-38011✅eth_simulateV1_sender_is_not_eoa_returns_spec_error_code— overrides sender with deployed code underIsEip3607Enabled = true→ expects-38024✅
Low — Tests omit error message assertion
The IntrinsicGas and FeeCapBelowBaseFee regression tests (same file) assert both result.ErrorCode and result.Result!.Error. The new nonce/EOA tests only assert the error code, not the message. There is no spec-mandated message for these three error types (unlike -38013/-38012 which have defined messages), so this is acceptable. No action required.
Low — No explicit BaseFeePerGas = 0 block override
eth_simulateV1_intrinsic_gas_returns_spec_error_code_and_message sets BlockOverrides = new BlockOverride { BaseFeePerGas = UInt256.Zero } explicitly. The new nonce tests omit this and rely on the chain's genesis block having BaseFeePerGas = 0 (which it does — TestBlockchain builds genesis without an explicit base fee, defaulting to UInt256.Zero, and London base fee propagation keeps it at zero). The existing InsufficientFunds regression test follows the same omission and works. Not blocking, but adding explicit BaseFeePerGas = UInt256.Zero would make intent clearer and prevent future brittleness if genesis defaults change.
Verdict
Mergeable ✅ — All previous findings resolved. The error code mappings are spec-correct, regression tests cover each changed path, and ErrorCodes.cs constants are in proper numeric order.
|
Duplicating with #11434 |
|
@claude suggest test deduplication if possible |
|
Claude finished @LukaszRozmej's task in 1m 37s —— View job Tasks
Test Deduplication AnalysisThe two nonce tests are good candidates for merging
They can be collapsed into a single [TestCase(10ul, 0ul, ErrorCodes.NonceTooLow, TestName = "NonceTooLow")]
[TestCase(0ul, 100ul, ErrorCodes.NonceTooHigh, TestName = "NonceTooHigh")]
public async Task eth_simulateV1_nonce_validation_returns_spec_error_code(
ulong accountNonce, ulong txNonce, int expectedErrorCode)
{
TestRpcBlockchain chain = await EthRpcSimulateTestsBase.CreateChain();
SimulatePayload<TransactionForRpc> payload = new()
{
BlockStateCalls =
[
new()
{
StateOverrides = new Dictionary<Address, AccountOverride>
{
{ TestItem.AddressA, new AccountOverride { Balance = 1.Ether, Nonce = accountNonce } }
},
Calls =
[
new LegacyTransactionForRpc
{
From = TestItem.AddressA,
To = TestItem.AddressB,
Value = UInt256.Zero,
Nonce = txNonce,
GasPrice = UInt256.Zero,
Gas = 21_000
}
]
}
],
Validation = true
};
ResultWrapper<IReadOnlyList<SimulateBlockResult<SimulateCallResult>>> result =
chain.EthRpcModule.eth_simulateV1(payload, BlockParameter.Latest);
Assert.That(result.ErrorCode, Is.EqualTo(expectedErrorCode));
}Note: the original
|
|
@DarkLord017 have you run hive tests? do we need to append to #11412 and known failures? |
https://github.com/NethermindEth/nethermind/actions/runs/25162663667/job/73760828927 yep they are failing but not due to this pr |
@DarkLord017 no, master should be fine - I've added exclusions to it, merged again latest master in your PR, try again |
One is failing |
3 participants
Fixes Closes Resolves #11220 , #11221 & #11222
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs. Remove if not applicable.Requires explanation in Release Notes