From 462b3e39c77145c25e804222ccc1b50b9e1cc7b3 Mon Sep 17 00:00:00 2001 From: Bhargava Shastry Date: Tue, 2 Dec 2025 15:11:26 +0100 Subject: [PATCH 1/6] fix: only touch coinbase after successful transaction in state tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #9225. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../Ethereum.Test.Base/GeneralTestBase.cs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs index 39dbc1517905..6157df18d08c 100644 --- a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs +++ b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs @@ -89,7 +89,7 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer) using IDisposable _ = stateProvider.BeginScope(null); ITransactionProcessor transactionProcessor = mainBlockProcessingContext.TransactionProcessor; - InitializeTestState(test.Pre, test.CurrentCoinbase, stateProvider, specProvider); + InitializeTestState(test.Pre, stateProvider, specProvider); BlockHeader header = new( test.PreviousHash, @@ -154,6 +154,15 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer) { stateProvider.Commit(specProvider.GetSpec((ForkActivation)1)); stateProvider.CommitTree(1); + + // '@winsvega added a 0-wei reward to the miner, so we had to add that into the state test execution phase. He needed it for retesteth.' + // This must only happen after successful transaction execution, not when tx fails validation. + if (!stateProvider.AccountExists(test.CurrentCoinbase)) + { + stateProvider.CreateAccount(test.CurrentCoinbase, 0); + } + + stateProvider.Commit(specProvider.GetSpec((ForkActivation)1)); stateProvider.RecalculateStateRoot(); } else @@ -176,7 +185,7 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer) return testResult; } - public static void InitializeTestState(Dictionary preState, Address coinbase, IWorldState stateProvider, ISpecProvider specProvider) + public static void InitializeTestState(Dictionary preState, IWorldState stateProvider, ISpecProvider specProvider) { foreach (KeyValuePair accountState in preState) { @@ -194,13 +203,6 @@ public static void InitializeTestState(Dictionary preStat stateProvider.Commit(specProvider.GenesisSpec); stateProvider.CommitTree(0); stateProvider.Reset(); - - if (!stateProvider.AccountExists(coinbase)) - { - stateProvider.CreateAccount(coinbase, 0); - stateProvider.Commit(specProvider.GetSpec((ForkActivation)1)); - stateProvider.RecalculateStateRoot(); - } } private List RunAssertions(GeneralStateTest test, IWorldState stateProvider) From 6133ed1cb43657ac94eca7047d6eff49c2a93ba6 Mon Sep 17 00:00:00 2001 From: Lukasz Rozmej Date: Tue, 2 Dec 2025 16:07:30 +0100 Subject: [PATCH 2/6] Update src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs --- src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs index 6157df18d08c..2a1a433853b1 100644 --- a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs +++ b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs @@ -157,10 +157,7 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer) // '@winsvega added a 0-wei reward to the miner, so we had to add that into the state test execution phase. He needed it for retesteth.' // This must only happen after successful transaction execution, not when tx fails validation. - if (!stateProvider.AccountExists(test.CurrentCoinbase)) - { - stateProvider.CreateAccount(test.CurrentCoinbase, 0); - } + stateProvider.CreateAccountIfNotExists(test.CurrentCoinbase, UInt256.Zero); stateProvider.Commit(specProvider.GetSpec((ForkActivation)1)); stateProvider.RecalculateStateRoot(); From 891e69c4458054eca532df3b4c9d2dab3ed84c91 Mon Sep 17 00:00:00 2001 From: Bhargava Shastry Date: Tue, 2 Dec 2025 20:15:57 +0100 Subject: [PATCH 3/6] fix: update T8nExecutor to use new InitializeTestState signature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tools/Evm/T8n/T8nExecutor.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/Evm/T8n/T8nExecutor.cs b/tools/Evm/T8n/T8nExecutor.cs index 140c962a31aa..2ac9ed6f9cd1 100644 --- a/tools/Evm/T8n/T8nExecutor.cs +++ b/tools/Evm/T8n/T8nExecutor.cs @@ -52,7 +52,7 @@ public static T8nExecutionResult Execute(T8nCommandArguments arguments) _logManager); stateProvider.CreateAccount(test.CurrentCoinbase, 0); - GeneralStateTestBase.InitializeTestState(test.Alloc, test.CurrentCoinbase, stateProvider, test.SpecProvider); + GeneralStateTestBase.InitializeTestState(test.Alloc, stateProvider, test.SpecProvider); Block block = test.ConstructBlock(); var withdrawalProcessor = new WithdrawalProcessor(stateProvider, _logManager); From 5f1d9e5ed218c6edb000a4a2e10861ae08a1942a Mon Sep 17 00:00:00 2001 From: Bhargava Shastry Date: Tue, 2 Dec 2025 15:11:26 +0100 Subject: [PATCH 4/6] fix: only touch coinbase after successful transaction in state tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #9225. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs index 2a1a433853b1..aadd5152efe4 100644 --- a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs +++ b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs @@ -158,7 +158,6 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer) // '@winsvega added a 0-wei reward to the miner, so we had to add that into the state test execution phase. He needed it for retesteth.' // This must only happen after successful transaction execution, not when tx fails validation. stateProvider.CreateAccountIfNotExists(test.CurrentCoinbase, UInt256.Zero); - stateProvider.Commit(specProvider.GetSpec((ForkActivation)1)); stateProvider.RecalculateStateRoot(); } From 7a91d49b533ad938fa22a8d258798cee399c0f78 Mon Sep 17 00:00:00 2001 From: Bhargava Shastry Date: Wed, 3 Dec 2025 12:24:09 +0100 Subject: [PATCH 5/6] fix: add IsLegacy flag for backward-compatible coinbase handling in state tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Ethereum.Test.Base/GeneralStateTest.cs | 6 +++++ .../Ethereum.Test.Base/GeneralTestBase.cs | 26 +++++++++++++++++-- .../LoadLegacyGeneralStateTestsStrategy.cs | 9 +++++-- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/Nethermind/Ethereum.Test.Base/GeneralStateTest.cs b/src/Nethermind/Ethereum.Test.Base/GeneralStateTest.cs index 800ff17e8437..098b7cc86821 100644 --- a/src/Nethermind/Ethereum.Test.Base/GeneralStateTest.cs +++ b/src/Nethermind/Ethereum.Test.Base/GeneralStateTest.cs @@ -13,6 +13,12 @@ namespace Ethereum.Test.Base { public class GeneralStateTest : EthereumTest { + /// + /// When true, uses legacy coinbase behavior (create before tx) for backward compatibility + /// with old test expectations that were computed with buggy coinbase timing. + /// + public bool IsLegacy { get; set; } + public IReleaseSpec? Fork { get; set; } public string? ForkName { get; set; } public Address? CurrentCoinbase { get; set; } diff --git a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs index aadd5152efe4..186da698ac37 100644 --- a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs +++ b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs @@ -91,6 +91,16 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer) InitializeTestState(test.Pre, stateProvider, specProvider); + // Legacy tests expect coinbase to be created BEFORE transaction execution + // (old buggy behavior that was baked into expected state roots). + // Modern tests correctly create coinbase only after successful tx. + if (test.IsLegacy && test.CurrentCoinbase is not null) + { + stateProvider.CreateAccountIfNotExists(test.CurrentCoinbase, UInt256.Zero); + stateProvider.Commit(specProvider.GetSpec((ForkActivation)1)); + stateProvider.RecalculateStateRoot(); + } + BlockHeader header = new( test.PreviousHash, Keccak.OfAnEmptySequenceRlp, @@ -157,13 +167,25 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer) // '@winsvega added a 0-wei reward to the miner, so we had to add that into the state test execution phase. He needed it for retesteth.' // This must only happen after successful transaction execution, not when tx fails validation. - stateProvider.CreateAccountIfNotExists(test.CurrentCoinbase, UInt256.Zero); + // For legacy tests, coinbase was already created before tx execution. + if (!test.IsLegacy) + { + stateProvider.CreateAccountIfNotExists(test.CurrentCoinbase, UInt256.Zero); + } stateProvider.Commit(specProvider.GetSpec((ForkActivation)1)); stateProvider.RecalculateStateRoot(); } else { - stateProvider.Reset(); + // For legacy tests with failed tx, we need to recalculate root since coinbase was created + if (test.IsLegacy) + { + stateProvider.RecalculateStateRoot(); + } + else + { + stateProvider.Reset(); + } } List differences = RunAssertions(test, stateProvider); diff --git a/src/Nethermind/Ethereum.Test.Base/LoadLegacyGeneralStateTestsStrategy.cs b/src/Nethermind/Ethereum.Test.Base/LoadLegacyGeneralStateTestsStrategy.cs index 93670a28c31b..1c3a2309ad27 100644 --- a/src/Nethermind/Ethereum.Test.Base/LoadLegacyGeneralStateTestsStrategy.cs +++ b/src/Nethermind/Ethereum.Test.Base/LoadLegacyGeneralStateTestsStrategy.cs @@ -50,9 +50,14 @@ private IEnumerable LoadTestsFromDirectory(string testDir, string { FileTestsSource fileTestsSource = new(testFile, wildcard); var tests = fileTestsSource.LoadTests(TestType.State); - foreach (EthereumTest blockchainTest in tests) + foreach (EthereumTest ethereumTest in tests) { - blockchainTest.Category = testDir; + ethereumTest.Category = testDir; + // Mark legacy tests to use old coinbase behavior for backward compatibility + if (ethereumTest is GeneralStateTest generalStateTest) + { + generalStateTest.IsLegacy = true; + } } testsByName.AddRange(tests); From a00f2b711a0ef78a176ac5415123085a1b46ab30 Mon Sep 17 00:00:00 2001 From: Bhargava Shastry Date: Mon, 15 Dec 2025 15:23:46 +0100 Subject: [PATCH 6/6] Update src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs Co-authored-by: Alexey Osipov --- src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs index 186da698ac37..e402b1716b7e 100644 --- a/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs +++ b/src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs @@ -180,6 +180,7 @@ protected EthereumTestResult RunTest(GeneralStateTest test, ITxTracer txTracer) // For legacy tests with failed tx, we need to recalculate root since coinbase was created if (test.IsLegacy) { + stateProvider.CommitTree(0); stateProvider.RecalculateStateRoot(); } else