Skip to content

fix: do not create trie account for movePrecompileToAddress-only state overrides#12045

Open
DarkLord017 wants to merge 4 commits into
masterfrom
simulate_hive_fix
Open

fix: do not create trie account for movePrecompileToAddress-only state overrides#12045
DarkLord017 wants to merge 4 commits into
masterfrom
simulate_hive_fix

Conversation

@DarkLord017

Copy link
Copy Markdown
Contributor

Fixes #12044

Changes

  • AccountOverride: added HasStateChanges property
  • StateOverridesExtensions: skip CreateAccount when HasStateChanges is false, preventing empty accounts from being injected into the trie for movePrecompileToAddress-only overrides
  • Added regression test as well as unit test

Types of changes

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

Testing

Requires testing

  • Yes

If yes, did you write tests?

  • Yes

Notes on testing

Covers the Hive test case multicall-move-two-accounts-to-same-38023. The regression test asserts that the simulated block's stateRoot equals the parent's when the only overrides are movePrecompileToAddress.

Documentation

Requires documentation update

  • No

Requires explanation in Release Notes

  • No

@claude

claude Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @DarkLord017's task in 2m 1s —— View job


PR Review: do not create account when state is empty in overrides

  • Gather context and read changed files
  • Review AccountOverride.cs changes
  • Review StateOverridesExtensions.cs changes
  • Review test files
  • Post findings

Summary

The fix is correct and well-scoped. The root cause was that StateOverridesExtensions.ApplyStateOverridesNoCommit unconditionally called state.CreateAccount(...) for any address that didn't already exist in state, even when the only override was MovePrecompileToAddress. Since precompile addresses (e.g. 0x01, 0x02) don't have state accounts, this injected empty accounts into the trie, altering the state root.

The HasStateChanges guard is semantically precise — MovePrecompileToAddress is correctly excluded because it is processed entirely by IOverridableCodeInfoRepository.MovePrecompile (no trie writes for the source address). The UpdateCode and UpdateState paths still execute unconditionally, which is fine: UpdateCode handles the precompile move without touching state, and UpdateState is a no-op when both State and StateDiff are null (both of which are in HasStateChanges).

The regression test in EthSimulateTestsSimplePrecompiles correctly exercises the two-precompile-to-same-address case and confirms state-root preservation.


Findings

Severity Count
Critical 0
High 0
Medium 0
Low 2

Low — HasStateChanges missing XML doc comment (view →)
The property name is self-explanatory but documenting the deliberate exclusion of MovePrecompileToAddress would make the design intent explicit for future maintainers. See inline suggestion.

Low — Missing complementary negative unit test (view →)
StateOverridesTests gained a positive case (override_with_balance_creates_account_in_state) but not the matching negative case (no account created for a move-only override). The integration test covers it, but a unit-level assertion would be a more direct spec. See inline suggestion.


Both are optional; neither blocks merge.

@DarkLord017 DarkLord017 changed the title do not create account when state is empty in overrides fix: do not create trie account for movePrecompileToAddress-only state overrides Jun 17, 2026
Comment thread src/Nethermind/Nethermind.Evm/AccountOverride.cs
Comment thread src/Nethermind/Nethermind.Evm.Test/StateOverridesTests.cs
@DarkLord017 DarkLord017 marked this pull request as ready for review June 17, 2026 17:19
@claude

claude Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @DarkLord017's task in 2m 33s —— View job


PR Review: do not create trie account for movePrecompileToAddress-only state overrides

  • Gather context and read changed files
  • Review AccountOverride.cs changes
  • Review StateOverridesExtensions.cs changes
  • Review test files
  • Post findings

Summary

The fix is correct and safe to merge. The root cause was that ApplyStateOverridesNoCommit unconditionally called state.CreateAccount(...) for any address absent from state, even when the only override was MovePrecompileToAddress. Since canonical precompile addresses (e.g. 0x01, 0x02) carry no trie account, this injected an empty account, dirtying the state root.

Correctness verification:

All three execution paths are sound:

Scenario CreateAccount? Trie dirty?
New addr, move-only (HasStateChanges=false) ❌ skipped ❌ No
New addr, state fields set (HasStateChanges=true) ✅ called ✅ Intended
Existing addr (any override) ❌ skipped (else branch) Only if balance/nonce change

UpdateCode is called unconditionally in all cases — correct, because MovePrecompile operates entirely on the code repository (no trie writes). UpdateState is a no-op when both State and StateDiff are null, so the guard composes cleanly.

Edge cases checked:

  • Code-only override on new address → HasStateChanges=trueCreateAccount called ✓
  • StateDiff-only override on new address → HasStateChanges=true
  • MovePrecompileToAddress + Balance on new address → HasStateChanges=true → both trie and code-repo updated ✓
  • Fully empty AccountOverride{}HasStateChanges=false → no account created ✓

Performance: HasStateChanges is a 5-null-check property expression — zero allocation, computed at most once per override entry. Not a concern.


Findings

Severity Count
Critical 0
High 0
Medium 0
Low 2

Low — HasStateChanges summary omits the rationale for excluding MovePrecompileToAddress
The existing doc explains what is included but not why MovePrecompileToAddress is excluded. A future reader adding a new routing-only field might mistakenly include it. Recommend adding a <remarks> line or extending the summary. Fix this →

Low — Unit test uses fully-empty override, not a MovePrecompileToAddress-only override
override_with_no_state_fields_does_not_create_account passes new AccountOverride() (all fields null, including MovePrecompileToAddress). This validates the HasStateChanges=false guard but does not exercise the UpdateCodeMovePrecompile path at the unit level. The integration test Test_eth_simulate_two_precompiles_moved_to_same_address_does_not_change_state_root closes this gap end-to-end, so this is not a blocking concern — just a note for future maintainers.

Neither finding blocks merge.

@DarkLord017 DarkLord017 requested a review from svlachakis June 17, 2026 17:20
@github-actions

Copy link
Copy Markdown
Contributor

EVM Opcode Benchmark Diff

Aggregated runs: base=3, pr=3
Noisy rerun opcodes: ADD, MULMOD, PUSH30

No significant regressions or improvements detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

eth_simulateV1: moving two precompiles to the same address produces wrong stateRoot and block hash

2 participants