-
Notifications
You must be signed in to change notification settings - Fork 293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
state: Implement EIP-3651: Warm COINBASE #560
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #560 +/- ##
=======================================
Coverage 97.03% 97.03%
=======================================
Files 66 66
Lines 6136 6138 +2
=======================================
+ Hits 5954 5956 +2
Misses 182 182
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Any unit test for this?
test/state/state.cpp
Outdated
@@ -156,6 +156,10 @@ std::variant<TransactionReceipt, std::error_code> transition( | |||
for (const auto& key : storage_keys) | |||
storage[key].access_status = EVMC_ACCESS_WARM; | |||
} | |||
// EIP-3651: Warm COINBASE. | |||
// We cannot touch it here because it may create an account in pre Spurious Dragon. |
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.
Hm? The condition is >=Shanghai, so not sure how Spurious Dragon is related.
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.
This refers to the implementation detail that the coinbase is touched after the transaction execution (see some lines below). This gives 2 coinbase account lookups, but it looks we cannot do better because we are not allowed to create the account here for pre Spurious Dragon revisions.
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.
because we are not allowed to create the account here for pre Spurious Dragon revisions.
Since the condition is >=Shanghai, this can never happen. Or the point of the comment is to explain the condition?
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 changed the comment.
We don't have unit tests for State/Host 😬. We mostly relay on state tests. |
064893a
to
daca228
Compare
daca228
to
fe540d3
Compare
Actually, we have some in test/integration/statetest. |
Spec: https://eips.ethereum.org/EIPS/eip-3651
Tested with: ethereum/tests#1082.