-
Notifications
You must be signed in to change notification settings - Fork 19.9k
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, tests: implement Metropolis EIP 684 #15039
Conversation
core/vm/evm.go
Outdated
@@ -25,6 +25,10 @@ import ( | |||
"github.com/ethereum/go-ethereum/params" | |||
) | |||
|
|||
// emptyCodeHash is used by create to ensure deployment is disallowed to already | |||
// deployed contract addresses (relevant after the account abstraction). | |||
var emptyCodeHash = common.BytesToHash(crypto.Keccak256(nil)) |
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.
Keccak256Hash
contractAddr = crypto.CreateAddress(caller.Address(), nonce) | ||
contractHash := evm.StateDB.GetCodeHash(contractAddr) | ||
if evm.StateDB.GetNonce(contractAddr) != 0 || (contractHash != (common.Hash{}) && contractHash != emptyCodeHash) { |
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.
Why is this not env.StateDB.Exist
?
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.
That results in a lot of test failures.
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'm guessing it's some complexity with suicides within create.
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.
Does it work with !env.StateDB.Empty(contractAddr)
? Trying to avoid the complicated condition here.
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.
Empty also requires balance == 0. That's not the case here, balance is allowed to be > 0.
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.
right
f8a125e
to
08f2742
Compare
contractAddr = crypto.CreateAddress(caller.Address(), nonce) | ||
contractHash := evm.StateDB.GetCodeHash(contractAddr) | ||
if evm.StateDB.GetNonce(contractAddr) != 0 || (contractHash != (common.Hash{}) && contractHash != emptyCodeHash) { |
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.
If you check the nonce first, you may not even need to get the contractHash
from the db.
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.
Well, true, but the nonce and hash retrieval should cost the same. I.e. retrieving the nonce pulls in the account node from the trie, which also contains the contract hash, so it shouldn't matter much. But that's just my 2c.
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.
Still, I prefer the less assuming kind of defensive programming:
contractAddr = crypto.CreateAddress(caller.Address(), nonce)
if existingHash := evm.StateDB.GetCodeHash(contractAddr); (existingHash != (common.Hash{}) && existingHash != emptyCodeHash) {
if evm.StateDB.GetNonce(contractAddr) != 0 {
return nil, common.Address{}, 0, ErrContractAddressCollision
}
}
But I can live with it anyway...
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's a bit messier than that. Your code requires both the nonce and the code hash to be zero for the error to trigger. The true intent is either cases triggers it.
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.
But yeah, I do see your point.
contractAddr = crypto.CreateAddress(caller.Address(), nonce) | ||
contractHash := evm.StateDB.GetCodeHash(contractAddr) | ||
if evm.StateDB.GetNonce(contractAddr) != 0 || (contractHash != (common.Hash{}) && contractHash != emptyCodeHash) { | ||
return nil, common.Address{}, 0, ErrContractAddressCollision |
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.
At this point, the caller nonce
is already incremented. Is that correct behaviour? Just want to double-check.
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.
in exactly the same way as it would fail if the sender address did not have enough balance to create the contract with the given initial wei value.
Sounds like not incrementing nonce (?)
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.
Well yes, but the nonce is retrieved already in nonce
. If you're asking whether a failure should increment the nonce or not, currently the tests expect it incremented. Not incrementing it would mean that Create transactions might have a new invalidity possibility which the txpool needs to handle.
Implements ethereum/EIPs#684.
Note, this PR is retroactively activated for all past transactions too. Please take care reviewing it.