Implement SSTORE net gas metering in aleth-interpreter #5238
Conversation
gumb0
force-pushed
the
net-sstore-interpreter
branch
from
e14077b to
ccc5e72
Compare
Codecov Report
@@ Coverage Diff @@
## master #5238 +/- ##
==========================================
+ Coverage 60.9% 61.01% +0.11%
==========================================
Files 338 338
Lines 27589 27649 +60
Branches 3197 3203 +6
==========================================
+ Hits 16802 16871 +69
+ Misses 9664 9649 -15
- Partials 1123 1129 +6 |
gumb0
force-pushed
the
net-sstore-interpreter
branch
from
ccc5e72 to
33d0276
Compare
| @@ -1356,22 +1356,19 @@ void VM::interpretCases() | |||
| if (m_message->flags & EVMC_STATIC) | |||
| throwDisallowedStateChange(); | |||
|
|
|||
| static_assert( | |||
There was a problem hiding this comment.
For the sake of simplicity I got rid of the trick to charge sstoreSetGas before set_storage`.
To keep it I think we would need to handle pre-Constantinople as an additional special case. In Constantinople we can't know before actually calling set_storage whether the change will require DB write, or will be just a dirty write (unless we decide to provide access to "original storage values" to EVM)
There was a problem hiding this comment.
Now I'm not sure if this is much simpler because it turns out we still need to handle Constantinople here differently (difference in the cost of X->X)
Here's the version with Constantinople as a separate case and keeping the old optimization: #5240
There was a problem hiding this comment.
I think to be safe we should charge more/less the SLOAD cost, so sstoreUnchangedGas is good candidate and handles both pre and post Constantinople cases.
We want to make sure the sstoreUnchangedGas is the smallest from all SSTORE possible costs. Would be nice to use std::min here but it started be a constexpr function in C++14.
libevm/ExtVMFace.cpp
Outdated
| evmc_result evmcResult = {}; | ||
| evmcResult.status_code = result.status; | ||
| evmcResult.gas_left = static_cast<int64_t>(gas); | ||
| evmcResult.release = nullptr; |
libevm/ExtVMFace.cpp
Outdated
| o_result->output_data = nullptr; | ||
| o_result->output_size = 0; | ||
| evmcResult.create_address = toEvmC(result.address); | ||
| evmcResult.output_data = nullptr; |
| @@ -1356,22 +1356,19 @@ void VM::interpretCases() | |||
| if (m_message->flags & EVMC_STATIC) | |||
| throwDisallowedStateChange(); | |||
|
|
|||
| static_assert( | |||
There was a problem hiding this comment.
I think to be safe we should charge more/less the SLOAD cost, so sstoreUnchangedGas is good candidate and handles both pre and post Constantinople cases.
We want to make sure the sstoreUnchangedGas is the smallest from all SSTORE possible costs. Would be nice to use std::min here but it started be a constexpr function in C++14.
Upgrade EVMC to unreleased development version that support Constantinople SSTORE statuses.
chfast
force-pushed
the
net-sstore-interpreter
branch
from
6b50a69 to
02746a1
Compare
| status = EVMC_STORAGE_ADDED; | ||
| else if (value == 0) | ||
| u256 const originalValue = env.originalStorageValue(index); | ||
| if (originalValue == currentValue || !schedule.eip1283Mode) |
There was a problem hiding this comment.
I think this should be simplified. We should report EVMC_STORAGE_MODIFIED_DIRTY also in pre-Constantinople. It will be simply handled together with EVMC_STORAGE_MODIFIED.
Closes #5226
Includes the changes due to EVMC API change (in the separate commit)