Fix/some simulate call by asdacap · Pull Request #8989 · NethermindEth/nethermind

asdacap · 2025-07-15T11:25:36Z

Require #8980

Eth simulate changes some state, and then call BranchProcessor (previously BlockProcessor) which does its own state handling including calling SetBaseBlock.
This PR changes that by calling BlockProcessor directly, which does not manipulate the state (as in does not CommitTree or change current state).
Also fix other issue so that some hive test can pass.
Also remove some unnecessary code.
Reduce hive test failure from 76 to 32 (jsonrpc.GasCap need to be set to 50mil).

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
Refactoring

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Notes on testing

Hive test failure down from 76 to 33.

rubo

Hive test failure down from 76 to 33.

It's down from 76 to 70. Not 33 but still good. Did you forget to push some changes?

asdacap · 2025-07-18T22:54:58Z

Did you set gaslimit to 50 mil?

rubo · 2025-07-18T23:38:04Z

Did you set gaslimit to 50 mil?

Nope. How?

asdacap · 2025-07-18T23:39:37Z

Change hive code to set jsonrpc.haslimit to 500000000

rubo · 2025-07-19T17:54:58Z

Change hive code to set jsonrpc.haslimit to 500000000

Are you suggesting a permanent change to Hive settings for Nethermind? Could you elaborate why this affects Hive tests?

asdacap · 2025-07-19T22:50:58Z

Same limit as geth. Its used as the default tx max gas.

Copilot

Pull Request Overview

This PR fixes issues with the eth_simulate endpoint by switching from BranchProcessor to BlockProcessor to avoid unwanted state manipulation and improves compatibility with Hive tests.

Changes transaction simulation to use BlockProcessor instead of BranchProcessor to prevent unintended state handling
Adds proper gas cap limit handling and transaction gas management during simulation
Fixes transaction hash calculation and nonce handling for better test compatibility

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
SimulateTxExecutor.cs	Adds gas cap parameter and fixes SystemUser address handling
EthRpcModule.cs	Updates simulator constructor to include secondsPerSlot parameter
SimulateBridgeHelper.cs	Major refactor switching to BlockProcessor with improved state and gas management
SimulateTransactionProcessorAdapter.cs	Adds gas tracking and transaction hash recalculation
SimulateRequestState.cs	Adds gas tracking fields
SimulateBlockValidationTransactionsExecutor.cs	Improves gas calculation and receipt handling
Test files	Updates expected hash values after simulation changes
StateOverridesExtensions.cs	Separates commit logic for state overrides

Comments suppressed due to low confidence (1)

src/Nethermind/Nethermind.Facade/Simulate/SimulateBridgeHelper.cs:97

[nitpick] The variable name globalGasCap is misleading as it's actually the effective gas limit after applying the cap. Consider renaming to effectiveGasLimit or cappedGasLimit.

        long globalGasCap = parent.GasLimit;

Copilot · 2025-07-26T13:09:14Z


                        Transaction tx = callTransactionModel.ToTransaction();
+
+                        // The RPC set SystemUser as default, but we want to set it to zero to follow hive test.


This workaround to set SystemUser to Zero for Hive test compatibility should include a more detailed comment explaining why this change is necessary and its implications.

Suggested change

// The RPC set SystemUser as default, but we want to set it to zero to follow hive test.

// The RPC sets SystemUser as the default sender address. However, for Hive test compatibility,

// we need to set the sender address to Address.Zero. Hive tests are designed to validate

// Ethereum client behavior in a controlled environment, and they require transactions to

// originate from Address.Zero to ensure consistent and predictable results. This workaround

// ensures compliance with Hive test requirements but may differ from the default behavior

// expected in other contexts. Developers should be cautious when modifying this logic, as it

// could impact test outcomes or introduce inconsistencies in environments outside of Hive tests.

Copilot · 2025-07-26T13:09:14Z

+            // For some reason, the logs from geth when processing the block is missing but not in the output from tracer.
+            // this cause the receipt root to be different than us. So we simulate it here.


This forced clearing of logs to match Geth behavior needs a more comprehensive comment explaining why this workaround is necessary and its potential side effects.

Suggested change

// For some reason, the logs from geth when processing the block is missing but not in the output from tracer.

// this cause the receipt root to be different than us. So we simulate it here.

// The logs from Geth when processing the block are missing, but they are present in the output from the tracer.

// This discrepancy causes the receipt root to differ between Geth and our implementation. To ensure compatibility

// with Geth's behavior, we clear the logs here by setting them to an empty array. This workaround ensures that

// the receipt root matches Geth's, which is critical for maintaining consistency in environments where Geth is

// the reference implementation.

//

// Potential side effects:

// 1. Clearing the logs might impact downstream processes that rely on transaction logs for auditing, debugging,

// or other purposes. Developers should ensure that this behavior is acceptable in their specific use case.

// 2. This workaround assumes that the absence of logs in Geth's output is intentional and not a bug. If Geth's

// behavior changes in the future, this code might need to be revisited.

Copilot · 2025-07-26T13:09:15Z

        foreach (Address address in senders.Union(targets).Where(static t => t is not null))
        {
-            stateProvider.CreateAccountIfNotExists(address, 0, 1);
+            stateProvider.CreateAccountIfNotExists(address, 0, 0);


The nonce parameter is set to 0, but this differs from the previous implementation which used 1. This change could affect account state initialization and should be verified for correctness.

Suggested change

stateProvider.CreateAccountIfNotExists(address, 0, 0);

stateProvider.CreateAccountIfNotExists(address, 1, 0);

Its deliberate

…rocessorAdapter.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…Tracer.cs Co-authored-by: Ruben Buniatyan <rubo@users.noreply.github.com>

…sactionProcessorAdapter.cs" This reverts commit bfe522d.

…/some-simulate-call

rubo

Change hive code to set jsonrpc.haslimit to 500000000

Setting JsonRpc.GasCap to 500000000 doesn't have any effect. Still 70 failing.

asdacap · 2025-08-04T04:59:27Z

@rubo check this. ethereum/hive#1326 . Dont forget --docker.pull.

rubo · 2025-08-05T18:37:01Z

@asdacap 53 passed, 39 failed. That's now a big difference. Should we open a PR to their repo?

asdacap · 2025-08-08T00:36:49Z

Yes, this, ethereum/hive#1326

asdacap marked this pull request as ready for review July 16, 2025 13:09

asdacap force-pushed the fix/some-simulate-call branch from 078a882 to beac11a Compare July 16, 2025 13:11

LukaszRozmej requested a review from rubo July 18, 2025 15:34

rubo reviewed Jul 18, 2025

View reviewed changes

Comment thread src/Nethermind/Nethermind.Facade/Simulate/SimulateBlockMutatorTracer.cs Outdated

NethermindEth deleted a comment from asdacap Jul 19, 2025

asdacap force-pushed the feature/separate-branch-and-block-processor branch from 603fb6d to e8b5c4e Compare July 21, 2025 13:44

asdacap force-pushed the fix/some-simulate-call branch from beac11a to ca90ec9 Compare July 21, 2025 13:44

asdacap force-pushed the feature/separate-branch-and-block-processor branch from e8b5c4e to c2e847b Compare July 23, 2025 12:26

asdacap force-pushed the fix/some-simulate-call branch from ca90ec9 to c114ef4 Compare July 23, 2025 12:26

asdacap mentioned this pull request Jul 23, 2025

Remove use of blockchain processor in RPC #9021

Merged

7 tasks

benaadams requested a review from Copilot July 26, 2025 13:08

Copilot AI reviewed Jul 26, 2025

View reviewed changes

asdacap force-pushed the feature/separate-branch-and-block-processor branch from c2e847b to 9295fce Compare July 27, 2025 01:30

Base automatically changed from feature/separate-branch-and-block-processor to master July 29, 2025 07:18

asdacap and others added 2 commits July 29, 2025 15:57

Move eth simulate from branch processor to block processor

7e5ba4b

Update src/Nethermind/Nethermind.Facade/Simulate/SimulateTransactionP…

bfe522d

…rocessorAdapter.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

asdacap force-pushed the fix/some-simulate-call branch from 0b14818 to bfe522d Compare July 29, 2025 07:58

asdacap and others added 4 commits August 1, 2025 10:14

Update src/Nethermind/Nethermind.Facade/Simulate/SimulateBlockMutator…

f200f6c

…Tracer.cs Co-authored-by: Ruben Buniatyan <rubo@users.noreply.github.com>

Revert "Update src/Nethermind/Nethermind.Facade/Simulate/SimulateTran…

a669737

…sactionProcessorAdapter.cs" This reverts commit bfe522d.

Merge remote-tracking branch 'origin/fix/some-simulate-call' into fix…

a347c2a

…/some-simulate-call

Merge branch 'master' into fix/some-simulate-call

79518fd

rubo approved these changes Aug 1, 2025

View reviewed changes

asdacap merged commit 932648e into master Aug 1, 2025
77 of 78 checks passed

asdacap deleted the fix/some-simulate-call branch August 1, 2025 12:04

benaadams added refactoring bug fix + reliability labels Aug 4, 2025


		Transaction tx = callTransactionModel.ToTransaction();

		// The RPC set SystemUser as default, but we want to set it to zero to follow hive test.

-                        // The RPC set SystemUser as default, but we want to set it to zero to follow hive test.
+                        // The RPC sets SystemUser as the default sender address. However, for Hive test compatibility,
+                        // we need to set the sender address to Address.Zero. Hive tests are designed to validate
+                        // Ethereum client behavior in a controlled environment, and they require transactions to
+                        // originate from Address.Zero to ensure consistent and predictable results. This workaround
+                        // ensures compliance with Hive test requirements but may differ from the default behavior
+                        // expected in other contexts. Developers should be cautious when modifying this logic, as it
+                        // could impact test outcomes or introduce inconsistencies in environments outside of Hive tests.

		// For some reason, the logs from geth when processing the block is missing but not in the output from tracer.
		// this cause the receipt root to be different than us. So we simulate it here.

-            // For some reason, the logs from geth when processing the block is missing but not in the output from tracer.
-            // this cause the receipt root to be different than us. So we simulate it here.
+            // The logs from Geth when processing the block are missing, but they are present in the output from the tracer.
+            // This discrepancy causes the receipt root to differ between Geth and our implementation. To ensure compatibility
+            // with Geth's behavior, we clear the logs here by setting them to an empty array. This workaround ensures that
+            // the receipt root matches Geth's, which is critical for maintaining consistency in environments where Geth is
+            // the reference implementation.
+            //
+            // Potential side effects:
+            // 1. Clearing the logs might impact downstream processes that rely on transaction logs for auditing, debugging,
+            //    or other purposes. Developers should ensure that this behavior is acceptable in their specific use case.
+            // 2. This workaround assumes that the absence of logs in Geth's output is intentional and not a bug. If Geth's
+            //    behavior changes in the future, this code might need to be revisited.

	stateProvider.CreateAccountIfNotExists(address, 0, 0);
	stateProvider.CreateAccountIfNotExists(address, 1, 0);

Conversation

asdacap commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Uh oh!

rubo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asdacap commented Jul 18, 2025

Uh oh!

rubo commented Jul 18, 2025

Uh oh!

asdacap commented Jul 18, 2025

Uh oh!

rubo commented Jul 19, 2025

Uh oh!

asdacap commented Jul 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

asdacap Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

rubo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asdacap commented Aug 4, 2025

Uh oh!

rubo commented Aug 5, 2025

Uh oh!

asdacap commented Aug 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asdacap commented Jul 15, 2025 •

edited

Loading

rubo left a comment •

edited

Loading