-
Notifications
You must be signed in to change notification settings - Fork 20.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core/state: remove account reset operation #28666
Conversation
df805c4
to
c9adc7b
Compare
This comment was marked as spam.
This comment was marked as spam.
Benchmark 05 is running a full sync since genesis using this branch. |
Looks like the state tests are failing, need to investigate it. |
The failure of state tests reflects a behavior difference between master code and this branch. EVM supports the contract code deployment to an existent account, if the account has 0 nonce and empty contract code. The existent account can be divided into two categories:
The definition of empty account is: The semantic of code deployment to existent account is a bit unclear to me, we can interpret it with different definitions:
For the latter one, the leftover storage slots will be dropped while for the former one the leftover storage slots are kept. It's the actual behavior difference. Personally, I am inclined to the first option, which is to simply add the contract code. However, my gut feeling is that we have been using the latter option for a very long time, and it is impossible or unreasonable to change the behavior now. |
A few analysis can be done:
After iterating the latest ethereum mainnet state, I did find a few deployable accounts with non-empty storage
|
c9adc7b
to
dc677ad
Compare
For these redeployable contracts with storages. They must be created before the EIP158 with 0 nonce. So they are created via a normal creation transaction or It's only possible to redeploy these accounts if hash collision happens:
Either of them happens is a huge disaster for the entire network. So the conclusion is: there are some redeployable accounts with storage slots and can theoretically trigger the behavior difference by making deployment, but it's impossible in practice unless hash collision. |
b169a09
to
87e175b
Compare
Gary's analysis seems correct to, but has missed a subtle point, which makes this complicated. The KECCAK collision required is only 160 bits, not the full 256 bits. 160 bit collisions are acheivable and cost around ~$10 billion. Someone could have set a trap that this PR is walking into. I think the most likely scenario is:
All of this is ludicrously unlikely, but is it unlikely enough that we can declare it impossible. I think the 2016 deadline for step 3 means this can be ruled impossible, but we need to agree on this. Assuming it is the decided that this is an impossible situation, the following state tests need to be deleted, before this PR is merged:
These cases are already controversial, |
@petertdavies thanks for pointing out that the KECCAK collision is 160bits. I totally missed that. You are right it's technically possible to trigger it. However, my point is: The behavioral definition of redeployment to existent account is very vague. Even the KECCAK collision happens, the correct behavior should be adding the new deployed code into this account, instead of resetting the whole thing. Namely the storage of the account shouldn't be wiped if it's not empty. In order to achieve this, it's becoming a complicated thing that we need to make sure all the client implementations have the same behavior for handling it. EDIT: after reading the conversation in py-evm's ticket, I believe the behavior for this case really need to be defined clearly. I will advocating for not having account reset at all. |
And for KECCAK collision, the result of the collision is that the attacker inherits the balance of the specified account. But compared to the cost of 10b, this benefit should be completely insufficient. We can say, it's technically possible, but the economy incentive is insufficient. |
if it is technically possible, consider the L2s that have tottaly different states. why not to cover all mathmatic solutions of this account reset/collision function? |
My point is: it's technically possible to trigger, but we need to align the behavior about it between different clients. Re the behavior definition: I would vote to just add the new contract code to the existent account, instead of resetting the whole account. |
I am all aboard for merging this.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but we should have a call about it, with @karalabe too
core/state/statedb.go
Outdated
@@ -47,6 +47,15 @@ type revision struct { | |||
journalIndex int | |||
} | |||
|
|||
type objectState struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, the naming... we're in statedb, and we're dealing with stateobjects, and for every state-object we have an object-state...?
How about lifecycle
, or phase
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you referring t exactly
core/state/statedb.go
Outdated
@@ -47,6 +47,15 @@ type revision struct { | |||
journalIndex int | |||
} | |||
|
|||
type objectState struct { | |||
typ int // 0 => update; 1 => delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use an enum for this? Or, if we know it's only two types, perhaps a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly dont know how to do that but i have an enum
Needs a rebase |
core/state/statedb.go
Outdated
state.stateObjectsDirty[addr] = struct{}{} | ||
// Deep copy the object state markers. | ||
for addr, obj := range s.stateObjectsState { | ||
state.stateObjectsState[addr] = obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this actually a shallow copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like modifications to the original state would trickle into the copied state. It would be good to have a testcase that detects this, and not just fix it here and now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a test that shows this bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, three lines down:
// Deep copy the destruction markers.
for addr, value := range s.stateObjectsDestruct {
state.stateObjectsDestruct[addr] = value
}
It also just copies over *types.StateAccount
into a map, not deep-copy. The actual usage of the items within stateObjectsDestruct
is a bit murky to me, we don't use it a lot, but we do use it in handleDestruction
. Please look it over.
@rjl493456442 is that the exhaustive list? |
From teh bottom
|
We also need to ensure to skip the tests that fail on CI, before we merge this. |
Currently, I believe the behaviour is mostly aligned on doing the account reset. Reth and EELS both intentionally implement a version of the account reset solely to pass the tests. Py-evm declares the situation impossible and doesn't implement the account reset. I can't speak to other clients, but I assume they implement the account reset rather than marking the tests invalid. Currently the tests are filled by Geth, so this PR will confusingly make other clients testsuites start failing when the tests are refilled with the new behaviour. |
@rjl493456442 don't forget this, it needs some love |
213b9f8
to
3f22e79
Compare
3f22e79
to
1afc158
Compare
234f9ba
to
b3fc6be
Compare
@rjl493456442 Hi I wonder if in once tx, an account call twice |
This scenario described by the test is impossible to happen, unless the hash collision. If we have EIP7610 included, this test case should be modified. |
@rjl493456442 Hi |
It's impossible to create a contract at the designed address twice within the same transaction, even the former created one is self-destructed. Before the deployment, this check will be conducted: // Ensure there's no existing contract already at the designated address
contractHash := evm.StateDB.GetCodeHash(address)
if evm.StateDB.GetNonce(address) != 0 || (contractHash != (common.Hash{}) && contractHash != types.EmptyCodeHash) {
if evm.Config.Tracer != nil && evm.Config.Tracer.OnGasChange != nil {
evm.Config.Tracer.OnGasChange(gas, 0, tracing.GasChangeCallFailedExecution)
}
return nil, common.Address{}, 0, ErrContractAddressCollision
} The nonce of former-created one will remain as non-zero within the whole transaction execution. |
b0322c8
to
f0353de
Compare
@@ -1407,3 +1355,19 @@ func copy2DSet[k comparable](set map[k]map[common.Hash][]byte) map[k]map[common. | |||
} | |||
return copied | |||
} | |||
|
|||
func (s *StateDB) markDelete(addr common.Address) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a docstring about when/why this method should be invoked (same for the one below)
306f7bc
to
d68fda5
Compare
Please don't merge this pull request, as it violates the EIP-6780. In EIP-6780, the contract deployed within the same transaction can be self-destructed. While a special case supported by protocol is the account was previously existent with non-zero balance, zero-nonce, zero-code and empty contract. It's deployed with contract code within the transaction and is eligible for self-destruction. In this pull request, if the account was present, the The potential fix is to explicitly set the |
That is quirky indeed.
So essentially it is possible to I'm not sure which I prefer, but we need to be super-clear on the semantics of the events. |
Can't we just set |
Essentially, the contract can be self-destruct-6780'd IFF the runtime code is non-empty, namely Interesting idea, i will think about it. Sound like it's more semantic correct. |
Actually flagging the Therefore, the flag must be set before init-code execution and must be revoked at the end of the transaction. The flag could be named as |
What testcase is that? I'll play around a bit with alternative fixes after the commit |
Close it in favor of #29520 |
This pull request gets rid of the Account Reset operation in statedb, serving as a pre-requisite steps for further statedb simplification.
Originally account reset is used to handle the scenario that an account was pre-funded and then be deployed with a contract code. In which the original account with pre-funded balance is be "reset" and the remaining balance would be transferred to the new account with the same address
However, in order to handle this specific scenario, account reset is not necessary. We can just add the contract code to this account instead, e.g.
It's a really sensitive change involves in EVM spec definition, let's firstly figure out what kind of account is deployable.
According to this ticket ethereum/EIPs#684,
If a contract creation is attempted, due to either a creation transaction or the CREATE (or future CREATE2) opcode, and the destination address already has either nonzero nonce, or nonempty code, then the creation throws immediately
.An address is regarded as deployable if nonce is 0 and contract code is empty. Specifically
account.nonce == 0 && (account.CodeHash == empty || account.CodeHash == keccak256(nil))
.Then we can list all the potential possibilities for contract deployment:
Apparently (a) and (b) are valid, and we will discuss why (c) and (d) are impossible.
In order to prove (c) and (d) are impossible to occur unless hash collision, we have to make sure that storage can only be created through contract as the first step, namely the storage of an account must be created by the account(contract) itself either in contract construction stage or after the deployment. It is impossible to set storage directly without running the contract, or to set up the storage through other contracts.
Therefore we can say if the storage is already present, then a contract creation may have happened either via (1) tx creation (2)
create
opcode (3)create2
opcode.Before EIP158, the nonce of created contracts is still left as 0 and it's set to 1 after EIP158 fork. Therefore the first conclusion we have is: For all accounts/contracts created after EIP158 forks, they are no longer deployable unless they are destructed first
What about the accounts created before EIP158?
Their nonce are zero and the stored contract code can be nil if the runtime code is empty. It's totally possible to have this kind account
nonce = 0 && code == empty && storage == non-empty
in the state. (Actually there are 28 accounts in mainnet in this case).So is it possible to deploy something to these account? These accounts are all created via tx creation or
create
opcode as there is nocreate2
at that time. It's impossible to compute a same address with rules unless hash collision.Keccak256(rlp{sender, nonce})[12:]
Keccak256([]byte{0xff}, sender, salt, inithash)[12:]
Therefore the second conclusion we have is The existent deployable accounts with storage are not possible to be redeployed unless hash collision.
In a summary we can prove (c) and (d) are impossible to occur.
Finally with this change, the only scenario we need to handle is pre-funded accounts deployment and it's apparently handled!
This branch can do full sync on mainnet from Genesis!