feat(ethereum-forks): remove total difficulty for hardfork check #13362

aroralanuk · 2024-12-12T17:38:36Z

fixes Remove extra Total difficulty argument for execution #13174

aroralanuk · 2024-12-12T17:41:13Z

@mattsse, is this how you were thinking of removing total difficulty and relying solely on the block number (given Paris is always the merge fork)?

mattsse · 2024-12-12T19:29:56Z

crates/ethereum-forks/src/forkcondition.rs

-    /// The fork is activated after a total difficulty has been reached.
-    TTD {
-        /// The block number at which TTD is reached, if it is known.
-        ///
-        /// This should **NOT** be set unless you want this block advertised as [EIP-2124][eip2124]
-        /// `FORK_NEXT`. This is currently only the case for Sepolia and Holesky.
-        ///
-        /// [eip2124]: https://eips.ethereum.org/EIPS/eip-2124
-        fork_block: Option<BlockNumber>,
-        /// The total difficulty after which the fork is activated.
-        total_difficulty: U256,
-    },


hmm, this has implications on the forkid.

I think the way we should go about this is keeping this variant but requiring that it knows the block number

@Rjected @joshieDo I can't remember what the implications of the fork_block field are because we set this.
I think we can't just set the fork_block field because this messes with the forkid, for example for mainnet we must set this to none,

so to enforce a block number for ttd we should add a new field

yes, we need to keep one eip-2124 fork block at least. We would want another blocknum field, that is used for checking if the merge is active, but not for the forkid.

mattsse · 2024-12-14T08:58:52Z

@aroralanuk so the way we should solve this is, we keep the TTD variant but add a new mandatory activation_block_number field, because we assume there won't be any more total difficulty hardforks and all activation numbers are known

aroralanuk · 2024-12-15T13:21:17Z

crates/chainspec/src/spec.rs

@@ -607,6 +607,7 @@ impl From<Genesis> for ChainSpec {
                hardforks.push((
                    EthereumHardfork::Paris.boxed(),
                    ForkCondition::TTD {
+                        activation_block_number: genesis.config.merge_netsplit_block.expect("Merge netsplit block is required"),


@mattsse @Rjected, do I need to update merge_netsplit_block to not be optional in the genesis file?

aroralanuk · 2024-12-15T13:28:33Z

crates/consensus/common/src/calc.rs

 ) -> Option<u128> {
-    if chain_spec.fork(EthereumHardfork::Paris).active_at_ttd(total_difficulty, block_difficulty) {
+    if chain_spec.fork(EthereumHardfork::Paris).active_at_ttd(block_number) {


should I keep the active_at_ttd function as it is and add a new active_at_merge method instead?

let's keep this for now to keept changes small

mattsse

cool, this looks good overall,

just need some additional time to take a closer look

mattsse · 2024-12-16T16:39:05Z

crates/blockchain-tree/src/blockchain_tree.rs

        if !self
            .externals
            .provider_factory
            .chain_spec()
            .fork(EthereumHardfork::Paris)
-            .active_at_ttd(parent_td, U256::ZERO)
+            .active_at_ttd(block.number)


I believe this should use the parent block number

mattsse · 2024-12-16T16:40:27Z

crates/consensus/common/src/calc.rs

 ) -> Option<u128> {
-    if chain_spec.fork(EthereumHardfork::Paris).active_at_ttd(total_difficulty, block_difficulty) {
+    if chain_spec.fork(EthereumHardfork::Paris).active_at_ttd(block_number) {


let's keep this for now to keept changes small

emilianobonassi · 2024-12-17T17:54:16Z

not sure how much is related but would be great to remove ttd from the response in the block

looks like execution api ratified that and geth recently implemented (seen some slow rollout these days by rpc providers)

ethereum/go-ethereum#30386

aroralanuk · 2024-12-20T00:21:11Z

thanks @Rjected

mattsse

amazing, this also unlocks a few nice followups

mattsse · 2024-12-20T08:04:53Z

crates/ethereum-forks/src/forkcondition.rs

+        /// The activation block number for the fork.
+        activation_block_number: BlockNumber,


can we add a one liner that this is mandatory because we expect that all difficulty hardforks are finalized and total difficulty is deprecated?

mattsse · 2024-12-20T08:07:56Z

crates/ethereum/evm/src/lib.rs

            &Head {
                number: header.number,
                timestamp: header.timestamp,
                difficulty: header.difficulty,
-                total_difficulty,
+                // NOTE: this does nothing within revm_spec
+                total_difficulty: U256::MIN,
                hash: Default::default(),
            },


we can simplify this a lot in a followup because we only need number+timstamp now

mattsse · 2024-12-20T08:08:15Z

crates/evm/execution-types/src/execute.rs

so much better lol

mattsse · 2024-12-20T08:09:49Z

crates/evm/src/state_change.rs

@@ -27,8 +26,6 @@ where
    if let Some(base_block_reward) = calc::base_block_reward(
        chain_spec,
        block.header().number(),
-        block.header().difficulty(),


mattsse · 2024-12-20T08:10:15Z

crates/optimism/evm/src/execute.rs

@@ -110,8 +110,8 @@ where
    /// Configures a new evm configuration and block environment for the given block.
    ///
    /// Caution: this does not initialize the tx environment.
-    fn evm_env_for_block(&self, header: &Header, total_difficulty: U256) -> EnvWithHandlerCfg {
-        let (cfg, block_env) = self.evm_config.cfg_and_block_env(header, total_difficulty);
+    fn evm_env_for_block(&self, header: &Header) -> EnvWithHandlerCfg {


mattsse · 2024-12-20T08:10:50Z

crates/storage/provider/src/providers/consistent.rs

-            .header_td_by_number(header.number())?
-            .ok_or_else(|| ProviderError::HeaderNotFound(header.number().into()))?;
-        Ok(evm_config.cfg_and_block_env(header, total_difficulty))
+        Ok(evm_config.cfg_and_block_env(header))


we can also get rid of this provider trait entirely now

Rjected · 2024-12-20T08:48:13Z

note that we will have a bit of redundancy in:

EthereumHardfork::Paris.mainnet_activation_block(),
The new activation_block field as part of the TTD fork, and
The paris_block_and_final_difficulty field on ChainSpec

So we can probably simplify even more things w.r.t our hardfork traits / structs

codspeed-hq · 2024-12-20T14:38:12Z

CodSpeed Performance Report

Merging #13362 will degrade performances by 10.82%

_{Comparing aroralanuk:rm-ttd-execution (1574376) with main (4e9f8c2)}

Summary

❌ 1 regressions
✅ 72 untouched benchmarks
🆕 4 new benchmarks
⁉️ 4 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`aroralanuk:rm-ttd-execution`	Change
🆕	`range` lookup`	N/A	611.6 µs	N/A
🆕	`any` lookup`	N/A	20.1 ms	N/A
🆕	`prefix set	preload size: 1000	input size: 1000	`Vec` with binary search lookup`
🆕	`prefix set	preload size: 1000	input size: 1000	`Vec` with custom cursor lookup`
⁉️	`range` lookup`	610.2 µs	N/A	N/A
⁉️	`any` lookup`	19.8 ms	N/A	N/A
⁉️	`prefix set	preload size: 1000	input size: 998	`Vec` with binary search lookup`
⁉️	`prefix set	preload size: 1000	input size: 998	`Vec` with custom cursor lookup`
❌	`receipts root	size: 10	HashBuilder`	106.5 µs

mattsse requested changes Dec 12, 2024

View reviewed changes

mattsse added the C-enhancement New feature or request label Dec 13, 2024

aroralanuk commented Dec 15, 2024

View reviewed changes

mattsse reviewed Dec 16, 2024

View reviewed changes

mattsse marked this pull request as ready for review December 16, 2024 16:42

mattsse requested review from fgimenez, emhane, rkrasiuk, joshieDo and rakita as code owners December 16, 2024 16:42

ratankaliani mentioned this pull request Dec 17, 2024

RPC Deserialization Issues anton-rs/kona#874

Open

Rjected requested review from shekhirin, onbjerg and gakonst as code owners December 19, 2024 17:29

aroralanuk added 3 commits December 19, 2024 19:30

init

53e9c90

add activation_block_number

a0a6062

fix test

8d214e4

mattsse approved these changes Dec 20, 2024

View reviewed changes

chore: remove total difficulty from execution types

171537d

Rjected force-pushed the rm-ttd-execution branch from 51b6ad5 to 171537d Compare December 20, 2024 08:50

Rjected added 2 commits December 20, 2024 10:55

chore: use paris block helper fn

5b02786

change paris active on EthereumHardforks to use activation block

0650773

Rjected force-pushed the rm-ttd-execution branch from 1898bba to 0650773 Compare December 20, 2024 10:15

add docs and remove condition for DisplayHardforks

461f250

fix doctests

ea11efe

Rjected enabled auto-merge December 20, 2024 10:40

fix hardfork tests

1574376

Rjected disabled auto-merge December 20, 2024 14:27

Rjected added this pull request to the merge queue Dec 20, 2024

Merged via the queue into paradigmxyz:main with commit 82af170 Dec 20, 2024
42 of 43 checks passed

mattsse mentioned this pull request Dec 22, 2024

chore: rm evmenvprovider trait #13503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ethereum-forks): remove total difficulty for hardfork check #13362

feat(ethereum-forks): remove total difficulty for hardfork check #13362

aroralanuk commented Dec 12, 2024 •

edited

Loading

aroralanuk commented Dec 12, 2024

mattsse Dec 12, 2024

Rjected Dec 12, 2024

mattsse commented Dec 14, 2024

aroralanuk Dec 15, 2024

aroralanuk Dec 15, 2024

mattsse Dec 16, 2024

mattsse left a comment

mattsse Dec 16, 2024

mattsse Dec 16, 2024

emilianobonassi commented Dec 17, 2024

aroralanuk commented Dec 20, 2024

mattsse left a comment

mattsse Dec 20, 2024

mattsse Dec 20, 2024

mattsse Dec 20, 2024

mattsse Dec 20, 2024

mattsse Dec 20, 2024

mattsse Dec 20, 2024

Rjected commented Dec 20, 2024

codspeed-hq bot commented Dec 20, 2024

		/// The activation block number for the fork.
		activation_block_number: BlockNumber,

feat(ethereum-forks): remove total difficulty for hardfork check #13362

feat(ethereum-forks): remove total difficulty for hardfork check #13362

Conversation

aroralanuk commented Dec 12, 2024 • edited Loading

aroralanuk commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse commented Dec 14, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilianobonassi commented Dec 17, 2024

aroralanuk commented Dec 20, 2024

mattsse left a 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

Rjected commented Dec 20, 2024

codspeed-hq bot commented Dec 20, 2024

Merging #13362 will degrade performances by 10.82%

Summary

Benchmarks breakdown

aroralanuk commented Dec 12, 2024 •

edited

Loading