Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Use storage status from EVMC #5171

Merged
merged 1 commit into from
Aug 9, 2018
Merged

Use storage status from EVMC #5171

merged 1 commit into from
Aug 9, 2018

Conversation

chfast
Copy link
Member

@chfast chfast commented Aug 8, 2018

Depends on ethereum/evmc#52.

@chfast chfast requested a review from gumb0 August 8, 2018 14:30
@codecov-io
Copy link

codecov-io commented Aug 8, 2018

Codecov Report

Merging #5171 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            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

@chfast chfast force-pushed the evmc-storage-status branch from 48f21d5 to 45785cd Compare August 9, 2018 10:32
Copy link
Member

@gumb0 gumb0 left a 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!

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.
Copy link
Member Author

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?

Copy link
Member

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.

@chfast chfast force-pushed the evmc-storage-status branch from 45785cd to da0e578 Compare August 9, 2018 14:58
Copy link
Member Author

@chfast chfast left a 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?

@chfast chfast merged commit 5b8745f into master Aug 9, 2018
@chfast chfast deleted the evmc-storage-status branch August 9, 2018 15:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants