-
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, tests: implement Metropolis EIP 684 #15039
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = crypto.Keccak256Hash(nil) | ||
|
||
type ( | ||
CanTransferFunc func(StateDB, common.Address, *big.Int) bool | ||
TransferFunc func(StateDB, common.Address, common.Address, *big.Int) | ||
|
@@ -307,13 +311,17 @@ func (evm *EVM) Create(caller ContractRef, code []byte, gas uint64, value *big.I | |
if !evm.CanTransfer(evm.StateDB, caller.Address(), value) { | ||
return nil, common.Address{}, gas, ErrInsufficientBalance | ||
} | ||
|
||
// Create a new account on the state | ||
// Ensure there's no existing contract already at the designated address | ||
nonce := evm.StateDB.GetNonce(caller.Address()) | ||
evm.StateDB.SetNonce(caller.Address(), nonce+1) | ||
|
||
snapshot := evm.StateDB.Snapshot() | ||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. But yeah, I do see your point. |
||
return nil, common.Address{}, 0, ErrContractAddressCollision | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point, the caller There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds like not incrementing nonce (?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well yes, but the nonce is retrieved already in |
||
} | ||
// Create a new account on the state | ||
snapshot := evm.StateDB.Snapshot() | ||
evm.StateDB.CreateAccount(contractAddr) | ||
if evm.ChainConfig().IsEIP158(evm.BlockNumber) { | ||
evm.StateDB.SetNonce(contractAddr, 1) | ||
|
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