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

EVMHost: Support warm coinbase #14126

Merged
merged 1 commit into from
Apr 17, 2023
Merged

EVMHost: Support warm coinbase #14126

merged 1 commit into from
Apr 17, 2023

Conversation

axic
Copy link
Member

@axic axic commented Apr 15, 2023

Part of #14073.

@cameel
Copy link
Member

cameel commented Apr 17, 2023

Is it normal that EIP-3651: Warm COINBASE is still in the "last call" state even after the fork? Should we ping someone to switch the status?

Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks reasonable and straightforward.

Is there a way to add a test for its effects though? As is, I trust that you tested that manually.

@axic
Copy link
Member Author

axic commented Apr 17, 2023

Is there a way to add a test for its effects though?

I think so, but we likely don't even have specific tests for the other two warm addresses.

The test probably has to measure gas and:

function f() {
  // call cold address once
}

function g() {
  // call cold address twice
}

function h() {
  // call warm address once
}

function i() {
  // call warm address twice
}

@cameel
Copy link
Member

cameel commented Apr 17, 2023

Ah, ok. I assumed we'd have to somehow know a coinbase address and have it hard-coded. But now I realized we can of course use block.coinbase. I think it would be a good idea to have these things tested at least in a minimal way.

@ekpyron ekpyron merged commit 6d847e4 into develop Apr 17, 2023
@ekpyron ekpyron deleted the warm-coinbase branch April 17, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants