Skip to content

Optimise CREATE(2) by removing redundant check#9580

Merged
Marchhill merged 2 commits into
masterfrom
optimise-create-already-deployed
Oct 28, 2025
Merged

Optimise CREATE(2) by removing redundant check#9580
Marchhill merged 2 commits into
masterfrom
optimise-create-already-deployed

Conversation

@Marchhill

@Marchhill Marchhill commented Oct 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Changes

  • Remove redundant check for dead account to clear storage. This is safe to do as we implement EIP-7610, so the contract deployment would have already failed if the address had storage
  • Microoptimisations

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

@Marchhill Marchhill requested a review from flcl42 October 27, 2025 17:34
@flcl42

flcl42 commented Oct 28, 2025

Copy link
Copy Markdown
Contributor

Any way to test it?

tanishqjasoria
tanishqjasoria suggested changes Oct 28, 2025
View reviewed changes
Comment thread src/Nethermind/Nethermind.Evm/Instructions/EvmInstructions.Create.cs
marcindsobczak
marcindsobczak approved these changes Oct 28, 2025
View reviewed changes

@marcindsobczak marcindsobczak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, state.ClearStorage() is already unreachable if state is non-empty, so this PR is removing indeed redundant check.

The better question is, why our archive sync is working fine when we already have this kind of bug? Clearing non-empty storage never happened in history?

@Marchhill

Marchhill commented Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

From what I understand, state.ClearStorage() is already unreachable if state is non-empty, so this PR is removing indeed redundant check.

The better question is, why our archive sync is working fine when we already have this kind of bug? Clearing non-empty storage never happened in history?

I think it probably hasn't happened as would require a hash collision so extremely unlikely

@Marchhill

Marchhill commented Oct 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Any way to test it?

Not really as the code is unreachable

@Marchhill Marchhill merged commit b4fd430 into master Oct 28, 2025
80 checks passed
@Marchhill Marchhill deleted the optimise-create-already-deployed branch October 28, 2025 14:13
@LukaszRozmej

LukaszRozmej commented Oct 28, 2025

Copy link
Copy Markdown
Member

please check if no regressions in hive!

@flcl42

flcl42 commented Oct 29, 2025

Copy link
Copy Markdown
Contributor

Any way to test it?

Not really as the code is unreachable

I can't find any collision tests, might be good to have some

asdacap added a commit that referenced this pull request Nov 17, 2025
@asdacap
Revert "Optimise CREATE(2) by removing redundant check (#9580)"
d2d08af 
This reverts commit b4fd430.
@asdacap asdacap mentioned this pull request Nov 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@marcindsobczak marcindsobczak marcindsobczak approved these changes

@benaadams benaadams Awaiting requested review from benaadams benaadams is a code owner

@Demuirgos Demuirgos Awaiting requested review from Demuirgos

@flcl42 flcl42 Awaiting requested review from flcl42

+1 more reviewer

@tanishqjasoria tanishqjasoria tanishqjasoria approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

performance is good

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Marchhill @flcl42 @LukaszRozmej @tanishqjasoria @marcindsobczak