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.

Sign up for GitHub

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

Jump to bottom

feat: replace DIFFICULTY with PREVRANDAO after Merge/EIP-4399 #162

Merged
merged 1 commit into from
May 21, 2023
Merged

feat: replace DIFFICULTY with PREVRANDAO after Merge/EIP-4399 #162

merged 1 commit into from
May 21, 2023

Conversation

vimpunk
Copy link
Contributor

@vimpunk vimpunk commented Apr 26, 2023 •
edited
Loading

EIP-4399 that has been introduced in the Merge hasn't yet been implemented in sputnik, so this PR does it.

The EIP changed the DIFFICULTY opcode to PREVRANDAO, that returns random data from a consensus node post Merge, rather than the block difficulty, which after the Merge is always 0.

Changes:

  • Configuration for the Merge hardfork.
  • In the config, has_prevrandao specifies whether this EIP is enabled.
  • A new field, block_randomness has been added to MemoryVicinity, so EVM hosts can specify the randomness they wish to use for the current execution of the EVM. If has_prevrandao is enabled then this value is returned by opcode 0x44.
  • Last commit on master removing Config from Runtime was reverted as this config is actually needed in this implementation.

Note: depends on rust-blockchain/evm-tests#19 to be merged first because CI here is failing as evm-tests is out-of-date.

@vimpunk vimpunk changed the title feat: replace DIFFICULTY with PREVRANDAO after merge/EIP-4399 feat: replace DIFFICULTY with PREVRANDAO after Merge/EIP-4399 Apr 26, 2023
@vimpunk vimpunk force-pushed the feat/eip-4399 branch 4 times, most recently from a3712ca to 5068df6 Compare April 27, 2023 12:28
@vimpunk vimpunk marked this pull request as ready for review April 27, 2023 14:52
sorpaas
sorpaas reviewed May 2, 2023
View reviewed changes
Copy link
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

This PR currently reverted #161, which I don't think is a good thing to do.

The reason we moved config out of runtime because it's actually just not really used. There are huge amount of parameters, yet we just need two of them.

I would appreciate one of the following alternative way:

  • Just don't set the has_prevrandao config. Instead, if Handler::block_randomness is Some, use randao behavior. Otherwise, use difficulty behavior`. I would prefer this.
  • Create a separate RuntimeConfig struct that just has the three parameters needed stack_limit, memory_limit, and has_randao.

@vimpunk vimpunk force-pushed the feat/eip-4399 branch 2 times, most recently from 07d297d to d19423b Compare May 3, 2023 09:36
@vimpunk
Copy link
Contributor Author

vimpunk commented May 3, 2023

@sorpaas I've addressed your comment. Can you take another look please?

sorpaas
sorpaas reviewed May 3, 2023
View reviewed changes
src/backend/memory.rs Outdated
@@ -110,6 +115,9 @@ impl<'vicinity> Backend for MemoryBackend<'vicinity> {
fn block_difficulty(&self) -> U256 {
self.vicinity.block_difficulty
}
fn block_randomness(&self) -> Option<U256> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn block_randomness(&self) -> Option<U256> {
fn block_randomness(&self) -> Option<H256> {

And you can avoid the conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it but this means EVM hosts (including evm-test) will have to take care of the endianness conversion. Isn't that error-prone?

core/src/error.rs Outdated Show resolved Hide resolved
@sorpaas
Copy link
Member

sorpaas commented May 3, 2023

LGTM. Please update the test PR.

@vimpunk
Copy link
Contributor Author

vimpunk commented May 3, 2023

Tests PR has been updated to convert endianness: https://github.com/rust-blockchain/evm-tests/pull/19/files#diff-bf42ab7deb71f94483835a445200880b13b91fc19cf43ad526262e4ea4d7b7d3R74-R84

@vimpunk
Copy link
Contributor Author

vimpunk commented May 3, 2023

I think we actually need to merge this first, then I'll be able to update the evm git submodule in the tests PR to the latest master here, and then that can be merged too.

@sorpaas
Copy link
Member

sorpaas commented May 3, 2023

I think we actually need to merge this first, then I'll be able to update the evm git submodule in the tests PR to the latest master here, and then that can be merged too.

In the evm-tests repo, go to the evm submodule folder, and then just normally check out your branch locally. You can then develop against your branch. Then just update the PR without updating the submodule pin. We can then merge that.

We don't have any CIs in the test repo, only the main one.

@vimpunk
Copy link
Contributor Author

vimpunk commented May 3, 2023 •
edited
Loading

In the evm-tests repo, go to the evm submodule folder, and then just normally check out your branch locally. You can then develop against your branch. Then just update the PR without updating the submodule pin.

Yes, that's how I've been developing.

We can then merge that.

I resolved the conflict on that PR. Let me know what to do next.

@vimpunk
Copy link
Contributor Author

vimpunk commented May 8, 2023

@sorpaas ping

@vimpunk vimpunk mentioned this pull request May 8, 2023
Merged
@vimpunk
Copy link
Contributor Author

vimpunk commented May 15, 2023

@sorpaas jsontests won't run I believe, I still need to send one more PR for the two repos to build together. Can we merge this as is and I'll send the follow up PR soon that should fix everything?

This was referenced May 15, 2023
Merged
Merged
@vimpunk
Copy link
Contributor Author

vimpunk commented May 16, 2023

That PR has been sent btw: #163

@sorpaas sorpaas mentioned this pull request May 21, 2023
Merged
@sorpaas sorpaas merged commit 21972c2 into rust-ethereum:master May 21, 2023
@vimpunk vimpunk deleted the feat/eip-4399 branch May 22, 2023 10:33
zjb0807 added a commit to AcalaNetwork/Acala that referenced this pull request Jan 22, 2024
@zjb0807
add eip-4399 ref: rust-ethereum/evm#162
05f6418
zjb0807 added a commit to AcalaNetwork/Acala that referenced this pull request Jan 29, 2024
@zjb0807
Update evm and switch to shanghai hardfork (#2691)
97bfc23 
* update PrecompileHandle ref: rust-ethereum/evm#122

* update fee calculation ref: rust-ethereum/evm#132

* add code_size/code_hash fn in StackState trait ref: rust-ethereum/evm#140

* update evm call stack ref: rust-ethereum/evm#136

* update evm call stack ref: rust-ethereum/evm#155

* add shanghai eips 3651, 3855, 3860 ref: rust-ethereum/evm#152

* update is_precompile ref: rust-ethereum/evm#157

* fix eip-3860 ref: rust-ethereum/evm#160

* update runtime config ref: rust-ethereum/evm#161

* add eip-4399 ref: rust-ethereum/evm#162

* fix eip-2618 ref: rust-ethereum/evm#163

* fix nonce back to U256 ref: rust-ethereum/evm#166

* remove exit_substate in create functions ref: rust-ethereum/evm#168

* record external cost ref: rust-ethereum/evm#170

* add record_external_operation ref: rust-ethereum/evm#171

* add storage_growth ref: rust-ethereum/evm#173

* update evm

* switch to shanghai hardfork

* update ecrecover ref: polkadot-evm/frontier#964 (#2696)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorpaas sorpaas sorpaas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vimpunk @sorpaas