Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: BlockCapturer for Created Accounts #1291

Conversation

DavePearce
Copy link
Collaborator

The block capturer had an issue capturing accounts which were created during the execution of the conflation being captured. The problematic scenario required an account to exist at the beginning of the conflation, but which was then the subject of a CREATE/CREATE2 contract creation. This is permitted in certain situations, such as when the account only has a balance (but no storage).

To resolve this case, we have to be a little more careful when creating storage snapshots. Specifically, when an account only partially exists (i.e. has no storage, but only a balance) then we never create storage snapshots.

Finally, this also adds a debugging mode to ReplayExecutionEnvironment to assist debugging the BlockCapturer.

The block capturer had an issue capturing accounts which were created
during the execution of the conflation being captured.  The problematic
scenario required an account to exist at the beginning of the
conflation, but which was then the subject of a CREATE/CREATE2
contract creation.  This is permitted in certain situations, such as
when the account only has a balance (but no storage).

To resolve this case, we have to be a little more careful when creating
storage snapshots.  Specifically, when an account only partially exists
(i.e. has no storage, but only a balance) then we never create storage
snapshots.

Finally, this also adds a debugging mode to ReplayExecutionEnvironment
to assist debugging the BlockCapturer.
@DavePearce DavePearce linked an issue Sep 23, 2024 that may be closed by this pull request
// for accounts which are created *during* the conflation. Such accounts may have technically
// existed before the conflation (e.g. they had a non-zero balance) but could still have been
// "created" during the conflation. In such case, this snapshot would be simply assigning 0x0
// to the given storage locations. However, we don't want to create a storage snapshot in such
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does taking a storage snapshot insert a storage value ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, at least according to the current EYP, storage does not matter when it comes to checking the possibility of doing a CREATE2

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, at least according to the current EYP

Yeah, but at the end of the day, we are not measuring against the EYP. We are comparing against execution on MAINNET using BESU. What I'm looking at is what BESU does. If that deviates from the EYP ... well, we still have to follow BESU.

However, I agree that it does seem wierd and I can follow up with BESU to see why they do it that way (or even it could be a bug).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does taking a storage snapshot insert a storage value ?

Yes, in the replay environment it does. See ReplayExecutionEnvironment.initWorld(). One option might be to drop StorageSnapshots which assign 0x0 at the beginning of the conflation. It has the same effect. Maybe you prefer this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But does taking a storage snapshot change the world state ? If so that is a massive problem.

Copy link
Collaborator

@OlivierBBB OlivierBBB Sep 24, 2024

Choose a reason for hiding this comment

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

In any case we do want to define storage snapshots even if the target account has empty storage or doesn't even exist yet. Simply because it's possible in Ethereum and we will want to test that, see #1205, but it should be side-effect free

@DavePearce DavePearce enabled auto-merge (squash) September 23, 2024 15:59
}
}

private static boolean accountExists(final Account account) {
Copy link
Collaborator

@OlivierBBB OlivierBBB Sep 24, 2024

Choose a reason for hiding this comment

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

AFAIU the EYP defines for an account $a$

  • existence as existence in the state, i.e. $\sigma[a] \neq \varnothing$
  • EMPTY as having nontrivial nonce, balance or code
  • DEAD as either not existing or being empty
  • prohibited from being deployed at as existing and either having nontrivial nonce, code or storage
image image

along with EIP-7610

So maybe a more accurate name would be accountMayNotBeDeployedAt / deploymentIsProhibitedAt or so.

Copy link
Collaborator

@OlivierBBB OlivierBBB left a comment

Choose a reason for hiding this comment

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

LGTM though there were parts where I'm not sure what's going on (e.g. the executeFrom)

@DavePearce DavePearce merged commit 9aab20e into arith-dev Sep 24, 2024
5 checks passed
@DavePearce DavePearce deleted the fix/issue-1289/fix-blockcapturer-handling-of-contract-creation branch September 24, 2024 10:15
@DavePearce DavePearce mentioned this pull request Sep 24, 2024
3 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix BlockCapturer Handling of Contract Creation
2 participants