Skip to content
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

Merged
merged 1 commit into from
Feb 13, 2023
Merged

state: Implement EIP-3651: Warm COINBASE #560

merged 1 commit into from
Feb 13, 2023

Conversation

chfast
Copy link
Member

@chfast chfast commented Feb 10, 2023

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #560 (fe540d3) into master (791b42b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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           
Flag Coverage Δ
blockchaintests 76.96% <ø> (ø)
statetests 71.44% <100.00%> (+0.03%) ⬆️
unittests 92.78% <50.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/state/state.cpp 98.92% <100.00%> (+0.02%) ⬆️

Copy link
Member

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

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

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.

Copy link
Member Author

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.

Copy link
Member

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?

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 changed the comment.

@chfast
Copy link
Member Author

chfast commented Feb 10, 2023

Any unit test for this?

We don't have unit tests for State/Host 😬. We mostly relay on state tests.

@axic axic merged commit 676027e into master Feb 13, 2023
@axic axic deleted the warm_coinbase branch February 13, 2023 13:46
@chfast
Copy link
Member Author

chfast commented Feb 13, 2023

We don't have unit tests for State/Host grimacing. We mostly relay on state tests.

Actually, we have some in test/integration/statetest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants