Extend storage status #661
Conversation
chfast
force-pushed
the
extend_storage_status
branch
from
911af80 to
2767330
Compare
chfast
force-pushed
the
extend_storage_status
branch
from
2767330 to
03dc899
Compare
Codecov Report
@@ Coverage Diff @@
## master #661 +/- ##
==========================================
+ Coverage 93.08% 93.12% +0.04%
==========================================
Files 25 25
Lines 3674 3770 +96
Branches 379 391 +12
==========================================
+ Hits 3420 3511 +91
- Misses 144 148 +4
- Partials 110 111 +1 |
|
I have Client-side implementation of this using only 4 checks:
https://github.com/ethereum/evmone/blob/state/test/state/host.cpp#L99-L162 (unstable link). Then you can get the gas cost and refund per revision from a lookup table. |
chfast
force-pushed
the
extend_storage_status
branch
from
03dc899 to
b0ee9fc
Compare
include/evmc/evmc.h
Outdated
| * This corresponds to the EIP-2200 rule 2 where: | ||
| * c == v. | ||
| */ | ||
| EVMC_STORAGE_MODIFIED_AGAIN = 0, |
There was a problem hiding this comment.
Why EVMC_STORAGE_MODIFIED_AGAIN rather than EVMC_STORAGE_UNCHANGED?
There was a problem hiding this comment.
It felt more correct to me: in general we modify a value, just gas cost does not care. The "unchanged" strongly suggests that current == value what is not always the case for this group. But I can change it to EVMC_STORAGE_UNCHANGED if people think this is fine.
There was a problem hiding this comment.
What is this rule 2 of EIP-2200 referenced in the doc? "If current value equals new value (this is a no-op), SLOAD_GAS is deducted."? Or something else?
There was a problem hiding this comment.
Oh, this is a mistake. This a group of this rule (2) and 3.2 without matching any subrules of 3.2. If you can parse my DSL, this can be e.g. X → Y → Z. Because these have the same cost, I decided to use single enum value for it.
If you think this is too confusing, I can assign separate values to these. But this suggests these have different behavior what is not the case.
There was a problem hiding this comment.
I'd prefer separate EVMC_STORAGE_UNCHANGED and EVMC_STORAGE_MODIFIED_AGAIN for extra clarity, even if they are priced the same.
There was a problem hiding this comment.
Just throwing in another idea: EVMC_STORAGE_ASSIGNED_AGAIN
There was a problem hiding this comment.
REASSIGNED?
Time for twitter pool.
There was a problem hiding this comment.
Based on the comments from @gumb0 and my preferences, can we leave it as EVMC_STATUS_MODIFIED_AGAIN? Also documentation is updated.
There was a problem hiding this comment.
Changed to EVMC_STORAGE_ASSIGNED.
chfast
force-pushed
the
extend_storage_status
branch
from
b0ee9fc to
c9f1100
Compare
include/evmc/evmc.h
Outdated
| * this status should be assigned to all remaining cases. | ||
| * | ||
| * This corresponds to the EIP-2200 rule 2 or rule 3.2 without sub-rules, where: | ||
| * c == v or (c != v, o != c, (o == 0 or (o != 0, c != 0, v != 0)), o != v) |
There was a problem hiding this comment.
This is hard to parse, and rule numbers like 3.2.1.1 are not useful because don't exist in EIP.
I perfectly understand notation like X -> Y -> 0, why do you need this new one?
There was a problem hiding this comment.
This is redundant. I try to explain what EIP rules it matches with the ordered list on condition to reach the particular rules in the spec tree in the EIP.
Any ideas how to improve this?
There was a problem hiding this comment.
Do you think it is better to remove it? I can have separate doxygen chapter (md document) that describes all matching details using big tables.
There was a problem hiding this comment.
I would rather remove them. The document looks good.
chfast
force-pushed
the
extend_storage_status
branch
2 times, most recently
from
ab3a03b to
a980e44
Compare
chfast
force-pushed
the
extend_storage_status
branch
3 times, most recently
from
9dc1a64 to
6d9c8e6
Compare
chfast
force-pushed
the
extend_storage_status
branch
from
df7f718 to
c7f5282
Compare
Expand the number of storage statuses to cover all possible EVM behaviors. These provide enough information to VM to compute not only gas costs but also refunds. Including legacy SSTORE costs before net gas metering.
chfast
force-pushed
the
extend_storage_status
branch
from
c7f5282 to
b91b723
Compare
Expand the number of storage statuses to cover all possible EVM
behaviors. These provide enough information to VM to compute not only
gas costs but also refunds. Including legacy SSTORE costs before net
gas metering.