core, crypto, params: implement CREATE2 evm instrction #17196
Conversation
| @@ -209,6 +209,7 @@ const ( | |||
| CALLCODE | |||
| RETURN | |||
| DELEGATECALL | |||
| CREATE2 | |||
There was a problem hiding this comment.
Please skim through this file, there are 1 or 2 more instances where you need to add the opcode (the string to opcode and opcode to string mappings).
core/vm/evm.go
Outdated
| func (evm *EVM) Create(caller ContractRef, code []byte, gas uint64, value *big.Int) (ret []byte, contractAddr common.Address, leftOverGas uint64, err error) { | ||
|
|
||
| // create creates a new contract using code as deployment code. | ||
| func (evm *EVM) create(caller ContractRef, code []byte, gas uint64, value *big.Int, address common.Address) (ret []byte, contractAddr common.Address, leftOverGas uint64, err error) { |
There was a problem hiding this comment.
Please remote the contractAddr from the returns. Since you're passing it in as as argument, it just makes things confusing.
|
Small issues, otherwise LGTM |
|
Within op Create2, no need to check for eip150 or homestead. There is no create2 prior to Constantinople. |
|
I did think about that, but I don't think we have code that auto-enables
previous forks if you enable a specific one. We should implement it though
imho.
…On Wed, Jul 18, 2018, 22:59 Martin Holst Swende ***@***.***> wrote:
Within op Create2, no need to check for eip150 or homestead. There is no
create2 prior to Constantinople.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#17196 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAH6GWY28N7osHAHlyS8pC8HuyXmDZuvks5uH5OigaJpZM4VUgXN>
.
|
|
Can you use a fixed size buffer for salt instead of slice? It's defined by the eip, right? |
|
@holiman Sorry i didn't find the salt definition in the EIP |
crypto/crypto.go
Outdated
| // TODO(rjl493456442) considering address collision, should we add the 0xff as the input prefix? | ||
| func CreateAddress2(b common.Address, salt []byte, code []byte) common.Address { | ||
| data, _ := rlp.EncodeToBytes([]interface{}{b, salt, code}) | ||
| return common.BytesToAddress(Keccak256(data)[12:]) |
There was a problem hiding this comment.
I'm unsure whether this method is correct or not. The spec seems to state sha3(msg.sender ++ salt ++ init_code)[12:]. Given that sender, salt and code are all bytes, I'd interpret the formula as simple append, not RLP encoding. Might be worth a clarification on the EIP.
There was a problem hiding this comment.
For the rlp encoding, no matter address is common.Address or byte slice, the result should be same.
And RLP or simple Append, yes we should give some feedbacks to EIP and make a clarification
core/vm/instructions.go
Outdated
| input = memory.Get(offset.Int64(), size.Int64()) | ||
| gas = contract.Gas | ||
| ) | ||
| if evm.ChainConfig().IsEIP150(evm.BlockNumber) { |
There was a problem hiding this comment.
This is 'speculative' -- how would Create2 work before EIP150? That's not defined, so I'd vote to remove this.
core/vm/instructions.go
Outdated
| // homestead we must check for CodeStoreOutOfGasError (homestead only | ||
| // rule) and treat as an error, if the ruleset is frontier we must | ||
| // ignore this error and pretend the operation was successful. | ||
| if evm.ChainConfig().IsHomestead(evm.BlockNumber) && suberr == ErrCodeStoreOutOfGas { |
There was a problem hiding this comment.
Same as above. Yes, it's (post) homestead
|
We'll need to extend the call tracer to be able to handle this new create. But lets do that in a followup pr. |
|
@karalabe Will do! |
|
Lgtm! |
* core, crypto, params: implement CREATE2 evm instrction * core/vm: add opcode to string mapping * core: remove past fork checking * core, crypto: use option2 to generate new address
Implements EIP1014