Align EEST validation mappings and fixture coverage - #12717
Conversation
|
Claude finished @flcl42's task in 4m 3s —— View job Review: Map nonce overflow transaction test errors
VerdictThe fix is correct and minimal. One Medium follow-up: the change closes the gap on the transaction-test side while leaving the mirror-image gap open on the blockchain-test side. What I verifiedThe mapping is right. All 15 fork variants are genuinely covered. Zero regression risk. The change adds a new dictionary key. Regression coverage is real. FindingsMedium — token coverage diverges between the two mapping tablesAfter this change:
Nethermind cannot distinguish the two: Two lines close the class of bug rather than one instance:
This is squarely within the PR's own stated goal ("Align transaction-test exception handling with the existing blockchain-test mapping"). Worth running Low — mirrored tables are the root cause (follow-up, not blocking)The comment at Low — hardcoded
|
| ["TransactionException.TYPE_3_TX_INVALID_BLOB_VERSIONED_HASH"] = ["InvalidBlobVersionedHashVersion"], | ||
| ["TransactionException.TYPE_3_TX_CONTRACT_CREATION"] = ["blob transaction of type create"], | ||
| ["TransactionException.INSUFFICIENT_MAX_FEE_PER_BLOB_GAS"] = ["max fee per blob gas less than block blob gas fee"], | ||
| ["TransactionException.NONCE_OVERFLOW"] = ["NonceTooHigh"], |
There was a problem hiding this comment.
Medium — the two mapping tables now diverge in opposite directions.
The mapping itself is correct: NonceCapTxValidator (TxValidator.cs:415-422, EIP-2681) returns TxErrorMessages.NonceTooHigh = "NonceTooHigh: Nonce exceeds max nonce", so the "NonceTooHigh" fragment substring-matches at line 90.
But after this change:
TransactionTestBaseknowsNONCE_OVERFLOW, notNONCE_IS_MAXBlockchainTestBase.cs:581knowsNONCE_IS_MAX, notNONCE_OVERFLOW
Nethermind cannot distinguish the two cases — transaction.Nonce < ulong.MaxValue rejects both nonce == 2^64-1 (NONCE_IS_MAX) and nonce > 2^64-1 (NONCE_OVERFLOW) with the identical message. So each table will spuriously fail whichever of the two tokens it doesn't list. EEST commonly emits both blockchain-test and transaction-test formats from the same source test, so a NONCE_OVERFLOW blockchain fixture in the same release is likely already failing (or being masked by the BAL-mismatch escape hatch at BlockchainTestBase.cs:562).
Listing both tokens on both sides costs one line each and closes the class of bug rather than one instance:
| ["TransactionException.NONCE_OVERFLOW"] = ["NonceTooHigh"], | |
| ["TransactionException.NONCE_OVERFLOW"] = ["NonceTooHigh"], | |
| ["TransactionException.NONCE_IS_MAX"] = ["NonceTooHigh"], |
...plus ("TransactionException.NONCE_OVERFLOW", "NonceTooHigh"), next to BlockchainTestBase.cs:581.
Worth also running blockTest against the same EEST release to confirm whether the blockchain side is currently failing.
There was a problem hiding this comment.
Addressed in 4b0b3a1. I removed the broad INVALID_DEPOSIT_EVENT_LAYOUT aliases for InvalidRequestsHash and InvalidBlockLevelAccessListHash. The swapped-offset fixture exposed a real consensus-side gap: the deposit ABI decoder validated field sizes but accepted non-canonical offsets. ExecutionRequestsProcessor now enforces fixed EIP-6110 offsets (160, 256, 320, 384, 512), with a focused regression test; the mapper accepts only the canonical DepositsInvalid diagnostic. Release ExecutionProcessorTests pass 4/4.
…8' into glam-nonce-overflow-mapping
Changes
TransactionException.NONCE_TOO_BIGand currentTransactionException.NONCE_IS_MAXmax-nonce cases to Nethermind'sNonceTooHighvalidation error in transaction and blockchain fixtures.NonceOverflowerror, and map EESTTransactionException.NONCE_OVERFLOWto that error instead of aliasing it to the max-nonce case.ExecutionRequestsProcessor, so the EEST swapped-offset deposit fixture receives the intendedDepositsInvalidvalidation error at the consensus boundary.tests-zkevm@v0.6.2execution until a fixture release compatible with the current Amsterdam gas schedule is available. The reusable workflow and manual dispatch support remain available.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
NONCE_TOO_BIG,NONCE_IS_MAX, andNONCE_OVERFLOWTxDecoderTests: 120/120, including 9-byte and 33-byte nonce-overflow regressionsExecutionProcessorTests: 4/4, including the non-canonical deposit-offset regressionEthereumTests.slnxbuilds in Release with warnings treated as errors and zero warnings or errorsDocumentation
Requires documentation update
Requires explanation in Release Notes
Remarks
EEST distinguishes an exactly-maximum nonce (
NONCE_IS_MAX, with the older fixture spellingNONCE_TOO_BIG) from a nonce that does not fit in 64 bits (NONCE_OVERFLOW). Nethermind now returnsNonceTooHighonly for the former and rejects the latter at RLP deserialization withNonceOverflow.The EEST
test_invalid_layout_with_swapped_decodable_offsetsfixture uses valid field lengths but non-canonical ABI offsets. The previous mapper aliases accepted downstreamInvalidRequestsHashor BAL-hash diagnostics asINVALID_DEPOSIT_EVENT_LAYOUT, which could hide a real consensus defect. The client now rejects those offsets directly and reportsDepositsInvalid: Invalid deposit event layout.tests-zkevm@v0.6.2was published before the current Amsterdam gas schedule. Its fixture corpus contains 25,994 Amsterdam cases and 110 Amsterdam-transition cases; 15,504 now produce a different stateless success result. There is no compatible official release yet, so continuing to run this corpus automatically reports expected fixture drift rather than current client correctness.