EVM-C v3 #112
EVM-C v3 #112
Conversation
axic
force-pushed
the
evmc-v3
branch
2 times, most recently
from
07a2223 to
084db80
Compare
| @@ -19,19 +20,21 @@ struct evm_uint160be address(struct evm_env* env) | |||
| static void query(union evm_variant* result, | |||
| struct evm_env* env, | |||
| enum evm_query_key key, | |||
| const union evm_variant* arg) { | |||
There was a problem hiding this comment.
Why is it better not to have this union?
There was a problem hiding this comment.
Previously this function was used to get other data (e.g. EXTCODESIZE -- the argument was the address, now address is explicit argument, BLOCKHASH -- the argument was int64 number of the block). Now the only possible argument is the storage key for SLOAD, so the type has changed from evm_variant to evm_uint256be.
Remove items from evm_query already available in evm_message
| @@ -41,6 +44,7 @@ static void query(union evm_variant* result, | |||
|
|
|||
| static void update(struct evm_env* env, | |||
| enum evm_update_key key, | |||
| const struct evm_uint160be* addr, | |||
There was a problem hiding this comment.
So update callback supports SSTORE (arg1: key, arg2: value), LOG (arg1: topics, arg2: data) and SELFDESTRUCT (arg1: address). See example implementation in cpp-ethereum: https://github.com/ethereum/cpp-ethereum/blob/4cb9b1085a11a04d5e6f6429df3681eaab2480fd/libevm/JitVM.cpp#L92-L112.
Because logs are not state changes per se, we can create another callback function for logs only (I forgot I wasn't happy about this). But in the end SSTORE and SELFDESTRUCT needs different types of arguments so evm_variant will be needed...
There was a problem hiding this comment.
But that means two evm_variant args should be enough?
There was a problem hiding this comment.
No. For SSTORE you have to provide address, key and value because the address of the currently executed account is only in the message. The "host" application does not need to remember what account EVM is going to change. This is abstraction what is introduced with message / tx data / state separation. Go and Python have internal API that reflects this already.
There was a problem hiding this comment.
Do we have any recommendations how the VMs should ensure that unauthorised SSTORE doesn't happen?
There was a problem hiding this comment.
VM should use the address from a message in this case, unless it wants to implement internal calls by itself...
There was a problem hiding this comment.
Maybe in that case we should also pass the current call context? Talking a bit blindly here, as haven't done the client side yet.
There was a problem hiding this comment.
All we need (at the moment) to manipulate or query the state (read: the map of all accounts) it an account address. All other data in a message (call context) is volatile and has not use for the state, i.e. value, input data, gas limit, etc.
| evm_call_fn call_fn; | ||
| evm_get_tx_context_fn get_tx_context_fn; | ||
| evm_get_block_hash_fn get_block_hash_fn; |
There was a problem hiding this comment.
Why is the block hash not part of the state? Because it is being offloaded to a contract?
There was a problem hiding this comment.
No. BLOCKHASH is not query about the state exactly and it does not match the query() function callback where you always have to provide the address of an account you query about. In the end it is not transaction data, nor query about accounts... so it got its own callback function.
Metropolis changes are no going to help because we are not going to remove (or deprecate) BLOCKHASH opcode. However, EVM implementation could actually use query() function to get needed information, instead of relying on dedicated callback function.
There was a problem hiding this comment.
Sounds reasonable.
(Note, the opcode will actually be a call to the contract.)
| @@ -41,11 +45,49 @@ struct evm_uint256be { | |||
| }; | |||
|
|
|||
| /// Big-endian 160-bit hash suitable for keeping an Ethereum address. | |||
| /// TODO: Rename to "address". | |||
There was a problem hiding this comment.
After we finish with functional changes I plan to do some renaming here and there...
libevmjit/RuntimeManager.cpp
Outdated
|
|
||
| llvm::Value* RuntimeManager::getSender() | ||
| { | ||
| return m_dataElts[RuntimeData::Caller]; |
There was a problem hiding this comment.
Can we use the same name, either sender or caller?
|
I like the split of state / tx context / call context, but not sure which is better for VMs. |
|
The documentation in Most of these fields were removed. |
|
Docs updated. |
|
@gumb0 approved cpp-ethereum side. Is this one ready to land as well? |
PoC of #110
In cpp-ethereum ethereum/aleth#3510 the state test running time goes down by ~2-5%.