fix: only touch coinbase after successful transaction in state tests #9865
+44
−13
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flcl42
approved these changes
Dec 2, 2025
deffrian
requested changes
Dec 2, 2025
LukaszRozmej
approved these changes
Dec 2, 2025
deffrian
approved these changes
Dec 2, 2025
Contributor
|
@bshastry Legacy tests are completely broken. Please fix |
Contributor
Author
Could you please paste the error so I can investigate? |
bshastry
pushed a commit
to bshastry/nethermind
that referenced
this pull request
Dec 3, 2025
This analysis documents the root causes of legacy test failures reported in PR NethermindEth#9865. Key findings: - PR combines coinbase bug fix with test infrastructure restructuring - Directory change from Constantinople to Cancun breaks many tests - Missing test categories in Cancun: stChangedEIP150, vmBlockInfoTest, etc. - Incompatible fork specifications between the two test sets - Different expected state roots due to pre-state differences Recommended fix: Split the PR to separate the coinbase fix from the test restructuring.
bshastry
pushed a commit
to bshastry/nethermind
that referenced
this pull request
Dec 3, 2025
This analysis documents the root causes of legacy test failures reported in PR NethermindEth#9865. Key findings: - The PR only contains the coinbase bug fix (3 commits) - Coinbase creation moved from InitializeTestState to after successful tx - This is semantically correct (matches geth behavior) - Legacy tests break because their expected hashes were computed with the buggy behavior (coinbase always touched) Recommended fixes: 1. Regenerate legacy test expected hashes 2. Add separate code path for legacy tests 3. Skip affected legacy tests
When a transaction fails validation (e.g., insufficient balance), the state test runner should not modify state at all. Previously, the coinbase account was created in InitializeTestState before transaction execution, causing state root divergence from geth when transactions failed validation. This fix moves coinbase creation to after successful transaction execution, matching geth's behavior: - Only touch coinbase when txResult == Ok - When validation fails, state remains unchanged from pre-state This was a consensus bug in the test framework (not production code) introduced in PR NethermindEth#9225. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The InitializeTestState method signature was changed to remove the coinbase parameter. T8nExecutor already creates the coinbase account separately (line 54), so we just need to update the call to match the new 3-argument signature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When a transaction fails validation (e.g., insufficient balance), the state test runner should not modify state at all. Previously, the coinbase account was created in InitializeTestState before transaction execution, causing state root divergence from geth when transactions failed validation. This fix moves coinbase creation to after successful transaction execution, matching geth's behavior: - Only touch coinbase when txResult == Ok - When validation fails, state remains unchanged from pre-state This was a consensus bug in the test framework (not production code) introduced in PR NethermindEth#9225. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…tate tests Legacy tests expected coinbase to be created before transaction execution, which was a buggy behavior baked into their expected state roots. This adds an IsLegacy flag to preserve backward compatibility while new tests use the correct behavior of creating coinbase only after successful tx. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
bshastry
force-pushed
the
fix/statetest-coinbase-consensus-bug
branch
from
e42de6a to
7a91d49
Compare
Contributor
Author
|
I tried to fix it by adding a flag for legacy tests. Hope it works! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a transaction fails validation (e.g., insufficient balance), the state test runner should not modify state at all. Previously, the coinbase account was created in InitializeTestState before transaction execution, causing state root divergence from geth when transactions failed validation.
This fix moves coinbase creation to after successful transaction execution, matching geth's behavior:
This was a consensus bug in the test framework (not production code) introduced in PR #9225.
🤖 Generated with Claude Code
fix: only touch coinbase after successful transaction in state tests
Changes
GeneralTestBase.csInitializeTestStatemethodTypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Manually verified with EEST-generated state test
CALL_Bounds2a[fork_Cancun-state_test--g0]that previously produced divergent state roots:0x823cc91f768e8afb9e1d4cfcdd1bf85e35781fbbb2122020783586da1f42ca300xd919d764bbc4f34ed6b03dbe84b9ef094a75146c08390a209b349c4c9156f629(matches geth)The test involves a transaction that fails validation due to insufficient sender balance (sender has 0 wei, needs 1,500,001 wei for gas * price + value). Previously, Nethermind was creating the coinbase account before transaction execution, causing the state root to differ from geth which only touches coinbase after successful execution.
Note: This bug only affects the test framework (
Ethereum.Test.Base), not production code. The productionTransactionProcessorcorrectly only touches coinbase inPayFeesafterBuyGasvalidation passes.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
This bug was introduced in commit
f48e3d8e6b4(PR #9225) which moved coinbase creation before transaction execution. The original code had a comment from @winsvega about adding a 0-wei reward for retesteth compatibility, but this should only happen after successful transaction execution, not when the transaction fails validation.The same bug was independently discovered in Besu's state test runner (fixed in parallel). Both bugs are isolated to test frameworks only - production consensus code is not affected.