-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5171 +/- ##
==========================================
- Coverage 59.9% 59.88% -0.03%
==========================================
Files 337 337
Lines 27350 27353 +3
Branches 3179 3181 +2
==========================================
- Hits 16385 16379 -6
- Misses 9880 9888 +8
- Partials 1085 1086 +1 |
48f21d5
to
45785cd
Compare
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.
Looks good, nice simplification!
libaleth-interpreter/VM.cpp
Outdated
m_context->fn_table->set_storage(m_context, &m_message->destination, &key, &value); | ||
m_runGas = (status == EVMC_STORAGE_ADDED) ? VMSchedule::sstoreSetGas : | ||
VMSchedule::sstoreResetGas; | ||
updateIOGas(); // Update the gas after the change when the full cost is known. |
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 see one problem here. When we have 0 gas left, we will do a contract storage lookup (which should cost at least 200). Maybe we should charge sstoreResetGas
up front, and then optionally VMSchedule::sstoreSetGas - VMSchedule::sstoreResetGas
?
BTW, wasn't the cost of deletion 0 in Frontier?
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.
Sounds reasonable, charging upfront shouldn't hurt...
I think store costs didn't change since Frontier, only sload cost changed once.
45785cd
to
da0e578
Compare
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.
Updated. Can you review again?
Depends on ethereum/evmc#52.