feat: add pre-block EIP-4788 beacon root contract call #4457

Rjected · 2023-09-01T20:47:27Z

This is #4438 but using the new revm API (#3512). Adds a new method apply_pre_block_call which applies the cancun beacon block root contract call.

crates/revm/Cargo.toml

codecov · 2023-09-05T14:36:05Z

Codecov Report

Merging #4457 (ae9c5cc) into main (394a3a9) will increase coverage by 0.10%.
The diff coverage is 83.26%.

Files Changed	Coverage Δ
crates/consensus/auto-seal/src/lib.rs	`0.00% <ø> (ø)`
crates/interfaces/src/executor.rs	`100.00% <ø> (ø)`
crates/payload/basic/src/lib.rs	`0.00% <0.00%> (ø)`
crates/primitives/src/constants/mod.rs	`100.00% <ø> (ø)`
crates/revm/src/state_change.rs	`73.00% <85.36%> (+13.67%)`	⬆️
crates/revm/src/processor.rs	`81.76% <92.80%> (+14.91%)`	⬆️
crates/revm/revm-primitives/src/config.rs	`91.27% <100.00%> (+0.11%)`	⬆️
crates/revm/revm-primitives/src/env.rs	`48.36% <100.00%> (+7.89%)`	⬆️

... and 10 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.81% <0.00%> (-0.12%)`	⬇️
unit-tests	`63.34% <83.26%> (+0.14%)`	⬆️

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

Components	Coverage Δ
reth binary	`31.97% <ø> (ø)`
blockchain tree	`83.59% <ø> (ø)`
pipeline	`88.54% <ø> (ø)`
storage (db)	`73.47% <ø> (ø)`
trie	`94.70% <ø> (-0.04%)`	⬇️
txpool	`49.44% <ø> (ø)`
networking	`77.19% <ø> (-0.07%)`	⬇️
rpc	`57.48% <ø> (-0.01%)`	⬇️
consensus	`62.58% <ø> (ø)`
revm	`28.04% <92.64%> (+8.38%)`	⬆️
payload builder	`8.56% <0.00%> (-0.50%)`	⬇️
primitives	`86.51% <ø> (+0.07%)`	⬆️

mattsse · 2023-09-14T09:52:14Z

crates/consensus/auto-seal/src/lib.rs

+        // TODO: there isn't really a parent beacon block root here, so not sure whether or not to
+        // call the 4788 beacon contract


yeah I think we need to figure out what to do here, but no blocker for this pr

mattsse · 2023-09-14T09:55:54Z

crates/revm/revm-primitives/src/env.rs

+///  * the call does not follow the EIP-1559 burn semantics - no value should be transferred as
+///  part of the call
+///  * if no code exists at `BEACON_ROOTS_ADDRESS`, the call must fail silently
+pub fn fill_tx_env_with_beacon_root_contract_call(env: &mut Env, parent_beacon_block_root: H256) {


this is great

mattsse · 2023-09-14T09:56:53Z

crates/revm/src/processor.rs

+    ///
+    /// If cancun is not activated or the block is the genesis block, then this is a no-op, and no
+    /// state changes are made.
+    pub fn apply_pre_block_call(&mut self, block: &Block) -> Result<(), BlockExecutionError> {


it's not obvious that this is the eip-4788 call,
in favour of changing the function names accordingly

changed to apply_beacon_root_contract_call

crates/revm/src/state_change.rs

mattsse · 2023-09-14T10:01:58Z

crates/payload/basic/src/lib.rs

+        ..Default::default()
+    };
+
+    // apply pre-block EIP-4788 contract call
+    let mut evm_pre_block = revm::EVM::with_env(env);
+    evm_pre_block.database(db);
+
+    // initialize a block from the env, because the pre block call needs the block itself
+    match apply_pre_block_call(
+        chain_spec,
+        attributes.timestamp,
+        block_number,
+        attributes.parent_beacon_block_root,
+        &mut evm_pre_block,


the txenv is still uninitialized here?

Changed fill_tx_env_with_beacon_root_contract_call to explicitly set each field in the TxEnv

mattsse · 2023-09-14T10:03:06Z

crates/payload/basic/src/lib.rs

@@ -967,6 +990,49 @@ fn commit_withdrawals<DB: Database<Error = Error>>(
    })
 }

+/// TODO: docs
+fn pre_block_contract_call<DB>(


this will be required for any kind of payload building so I think we should move this to the payload/builder crate

Let's track in new good-first-issue

mattsse · 2023-09-14T10:03:51Z

crates/payload/basic/src/lib.rs

@@ -967,6 +990,49 @@ fn commit_withdrawals<DB: Database<Error = Error>>(
    })
 }

+/// TODO: docs
+fn pre_block_contract_call<DB>(
+    db: State<DB>,


I think this could be Database + DatabaseCommit, so it no longer requires a concrete type,
all of them are also implemented on &mut
so we don't need to return it

same in separate good first issue

was an easy change, it's all &mut now

onbjerg · 2023-09-14T19:25:39Z

crates/revm/src/processor.rs

@@ -234,15 +249,14 @@ impl<'a> EVMProcessor<'a> {
        total_difficulty: U256,
        senders: Option<Vec<Address>>,
    ) -> Result<(Vec<Receipt>, u64), BlockExecutionError> {
+        self.init_env(&block.header, total_difficulty);


does this need to happen even if the block is empty? if not, should prob move back to where it was before

yes, this happens on every block, regardless of empty / non empty

onbjerg · 2023-09-14T19:26:24Z

crates/revm/src/processor.rs

+
+    /// Returns the beacon root contract code
+    fn beacon_root_contract_code() -> Bytes {
+        Bytes::from_str("0x3373fffffffffffffffffffffffffffffffffffffffe14604457602036146024575f5ffd5b620180005f350680545f35146037575f5ffd5b6201800001545f5260205ff35b6201800042064281555f359062018000015500").unwrap()


nit: can be a lazy static?

onbjerg · 2023-09-14T19:27:50Z

crates/revm/src/state_change.rs

+        if block_number == 0 {
+            if block_parent_beacon_block_root != Some(H256::zero()) {


nit: can be rolled into 1 statement

I don't think so, because if the parent root is Some(H256::zero()) on genesis, then we don't want to run the beacon root contract call, per the spec. If we rolled the statements into one, then we would run the contract call on a genesis with parent_beacon_block_root = Some(H256::zero()).

gakonst

LGTM. Let's open issues for the nits/todos from the PR and get this in, unless any objections. We'll merge Revm State PR first, and then we'll need to do a smol rebase against main before getting this in main.

gakonst · 2023-09-15T20:40:00Z

crates/payload/basic/src/lib.rs

@@ -967,6 +990,49 @@ fn commit_withdrawals<DB: Database<Error = Error>>(
    })
 }

+/// TODO: docs
+fn pre_block_contract_call<DB>(


Let's track in new good-first-issue

gakonst · 2023-09-15T20:40:08Z

crates/payload/basic/src/lib.rs

@@ -967,6 +990,49 @@ fn commit_withdrawals<DB: Database<Error = Error>>(
    })
 }

+/// TODO: docs
+fn pre_block_contract_call<DB>(
+    db: State<DB>,


same in separate good first issue

crates/primitives/src/constants/mod.rs

gakonst · 2023-09-15T20:44:45Z

crates/revm/src/state_change.rs

+where
+    <DB as Database>::Error: Debug,
+{
+    if chain_spec.is_cancun_activated_at_timestamp(block_timestamp) {


you can de-indent twice here if you collapse the ifs / invert this and return early, but nbd

gakonst · 2023-09-15T20:45:56Z

crates/revm/src/state_change.rs

+            // get previous env
+            let previous_env = evm.env.clone();


nit: can you not instead call this beacon_roots_env and do the fill_tx call with that, to avoid having to re-set the evm.env at the end of the function?

no, because the env would have to be set in the evm anyways, for it to be used during transact. We could initialize a new Env and swap them, but I don't see how that's different than cloning here, since we have to initialize a second Env anyways.

We could get rid of the reset by just telling the caller (in docs) to make sure the block and tx env are initialized properly before the next call to transact, and make sure to do that in the processor, payload builder, etc.

gakonst · 2023-09-15T20:47:31Z

crates/revm/src/state_change.rs

+            // TODO: re-evaluate this after 4788 implementers call, this is to prevent EIP161 state
+            // clearing of the system address, since it is touched during transact. The pyspec
+            // tests currently require the state to remain empty if it started empty, which
+            // somewhat contradicts EIP161, and clients still need consensus on what to do here.
+            state.remove(&SYSTEM_ADDRESS);
+            state.remove(&evm.env.block.coinbase);


I think this current way makes sense? My default way of doing this EIP would've been w/o a smart contract but with direct state surgery, and that would involve no system address. So my intuition is that the system address should not be anywhere outside of this kind of weird functions.

mattsse

lgtm

Rjected added C-enhancement New feature or request S-blocked This cannot more forward until something else changes A-execution Related to the Execution and EVM M-eip This change relates to the implementation of an EIP E-cancun Related to the Cancun network upgrade labels Sep 1, 2023

Rjected requested review from mattsse and gakonst as code owners September 1, 2023 20:47

Rjected requested a review from rakita September 1, 2023 20:47

Rjected mentioned this pull request Sep 1, 2023

feat: add pre-block EIP-4788 beacon root contract call #4438

Closed

3 tasks

Rjected force-pushed the dan/eip-4788-contract-call-squashed branch from 206c353 to fed5b0e Compare September 4, 2023 13:19

mattsse reviewed Sep 4, 2023

View reviewed changes

crates/revm/Cargo.toml Outdated Show resolved Hide resolved

Rjected force-pushed the dan/eip-4788-contract-call-squashed branch from fed5b0e to 2378263 Compare September 5, 2023 14:22

Rjected removed the S-blocked This cannot more forward until something else changes label Sep 5, 2023

Rjected marked this pull request as draft September 5, 2023 23:49

Rjected force-pushed the dan/eip-4788-contract-call-squashed branch 2 times, most recently from 83c600d to f81d404 Compare September 7, 2023 14:29

Rjected mentioned this pull request Sep 7, 2023

EIP-4788: Beacon block root in the EVM bluealloy/revm#525

Closed

Rjected marked this pull request as ready for review September 13, 2023 17:57

mattsse requested changes Sep 14, 2023

View reviewed changes

onbjerg reviewed Sep 14, 2023

View reviewed changes

gakonst approved these changes Sep 15, 2023

View reviewed changes

Base automatically changed from rakita/revm_state to main September 16, 2023 11:13

Rjected requested review from rkrasiuk, joshieDo and shekhirin as code owners September 16, 2023 11:13

Rjected force-pushed the dan/eip-4788-contract-call-squashed branch 2 times, most recently from 72fe343 to 4d487f2 Compare September 18, 2023 14:39

Rjected requested a review from mattsse September 18, 2023 19:35

mattsse approved these changes Sep 18, 2023

View reviewed changes

Rjected added 23 commits September 19, 2023 00:02

assert exact error on cancun block without beacon block root

6e2c9d4

apply pre block contract call in payload builder

434103b

disable block base fee check and set block gas limit

2c0291d

stop using optional base fee feature

66abd72

init env correctly

6936f3e

fix usages of State

30b6320

fix beacon root contract code

66b29ee

set system address nonce to zero

3788bf7

re-set beacon roots address

f0af29b

remove system address from output

2f6970c

remove beneficiary account from result state

a8d3c14

change apply_pre_block_call to apply_beacon_root_contract_call

8d05d0b

use cancun helper fn

0865e7a

add note about EIP161 state clear

0034849

rename pre block call method in payload builder

bafd150

use Database + DatabaseCommit instead of returning State

f854021

remove old contract address

1b539c4

add test for no nonce increments on empty system account

8858761

add docs for pre_block_beacon_root_contract_call

493482a

explicitly set every TxEnv field before beacon root contract call

37d2e1a

fix tests

63d8d48

set blob fields in beacon root txenv

71d6a17

remove todo for client consensus

ae9c5cc

Rjected force-pushed the dan/eip-4788-contract-call-squashed branch from 50c9996 to ae9c5cc Compare September 19, 2023 04:02

Rjected added this pull request to the merge queue Sep 19, 2023

Merged via the queue into main with commit a80b720 Sep 19, 2023
22 checks passed

Rjected deleted the dan/eip-4788-contract-call-squashed branch September 19, 2023 15:29

mattsse added the M-changelog This change should be included in the changelog label Sep 19, 2023

onbjerg mentioned this pull request May 5, 2024

Document external state transitions for EIPs bluealloy/revm#1382

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add pre-block EIP-4788 beacon root contract call #4457

feat: add pre-block EIP-4788 beacon root contract call #4457

Rjected commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

mattsse Sep 14, 2023

mattsse Sep 14, 2023

mattsse Sep 14, 2023

Rjected Sep 15, 2023

mattsse Sep 14, 2023

Rjected Sep 15, 2023

mattsse Sep 14, 2023

gakonst Sep 15, 2023

mattsse Sep 14, 2023

gakonst Sep 15, 2023

Rjected Sep 15, 2023

onbjerg Sep 14, 2023

Rjected Sep 15, 2023

onbjerg Sep 14, 2023

onbjerg Sep 14, 2023

Rjected Sep 15, 2023

gakonst left a comment •

edited

Loading

gakonst Sep 15, 2023

gakonst Sep 15, 2023

gakonst Sep 15, 2023

gakonst Sep 15, 2023

Rjected Sep 18, 2023

gakonst Sep 15, 2023

mattsse left a comment

		// TODO: there isn't really a parent beacon block root here, so not sure whether or not to
		// call the 4788 beacon contract

		if block_number == 0 {
		if block_parent_beacon_block_root != Some(H256::zero()) {

feat: add pre-block EIP-4788 beacon root contract call #4457

feat: add pre-block EIP-4788 beacon root contract call #4457

Conversation

Rjected commented Sep 1, 2023 • edited Loading

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Rjected commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

gakonst left a comment •

edited

Loading