feat: feynman base fee #273

greged93 · 2025-07-04T21:33:24Z

Adds the Feynman L2 base fee computation to Reth and introduces it into the transaction pool.

Fixes #234.

Signed-off-by: Gregory Edison <[email protected]>

codspeed-hq · 2025-07-04T21:40:46Z

CodSpeed Performance Report

Merging #273 will not alter performance

_{Comparing feat/feynman-base-fee (2e7b149) with scroll (aad1706)}

Summary

✅ 77 untouched benchmarks

greged93 · 2025-07-04T21:41:20Z

crates/scroll/alloy/rpc-types/src/transaction.rs

+            // TODO: should we get the pool base fee in the case where the transaction is a pending
+            // transaction here?


This is one of the points that I'm not sure about: should the base fee from the transaction pool be fetched and used here?

For context, this type is used in the RpcConverter and builds a RPC transaction from a recovered tx and the scroll transaction info (which contains the L1 fee and the block context info). For now, afaiu, the RpcConverter uses empty block context for pending pool transaction (see here) but from what I understand in l2geth, the pending base fee is used to compute the effective_gas_price for pool transactions.

Where is the original value (tx_info.inner.base_fee) taken from, the latest or pending block?

I don't think it's very important to consider overhead here, and this will just be a transient discrepancy.

It's taken from here and with thus be None.

Signed-off-by: Gregory Edison <[email protected]>

greged93 · 2025-07-14T10:57:15Z

crates/scroll/alloy/hardforks/src/hardfork.rs

@@ -37,7 +37,7 @@ impl ScrollHardfork {
            (Self::Euclid, ForkCondition::Timestamp(1744815600)),
            (Self::EuclidV2, ForkCondition::Timestamp(1745305200)),
            // TODO: update
-            (Self::Feynman, ForkCondition::Timestamp(u64::MAX)),
+            (Self::Feynman, ForkCondition::Timestamp(6000000000)),


This is done in order to be able to add test cases with pre and post Feynman chainspec.

Signed-off-by: Gregory Edison <[email protected]>

Thegaram · 2025-07-15T12:51:06Z

crates/scroll/chainspec/src/constants.rs

 use alloy_primitives::{address, b256, Address, B256};

 /// The transaction fee recipient on the L2.
 pub const SCROLL_FEE_VAULT_ADDRESS: Address = address!("5300000000000000000000000000000000000005");

+/// The system contract on L2 mainnet.
+pub const SCROLL_MAINNET_SYSTEM_CONTRACT_ADDRESS: Address =


Suggested change

pub const SCROLL_MAINNET_SYSTEM_CONTRACT_ADDRESS: Address =

pub const SCROLL_MAINNET_L2_SYSTEM_CONFIG_CONTRACT_ADDRESS: Address =

A bit verbose, but "system contract" made me think of Ethereum's system contracts.

Thegaram · 2025-07-15T12:51:45Z

crates/scroll/chainspec/src/constants.rs

+/// The system contract on devnet.
+pub const SCROLL_DEV_SYSTEM_CONTRACT_ADDRESS: Address =
+    address!("0000000000000000000000000000000000000000");


We can add this as a predeploy in devnet genesis if needed.

Thegaram · 2025-07-15T12:54:59Z

crates/scroll/chainspec/src/genesis.rs

+    /// The address of the L2 system contract.
+    pub l2_system_contract_address: Address,


Please make sure that genesis format is consistent with l2geth: https://github.com/scroll-tech/sre-helm-charts/blob/main/config/genesis/genesis.mainnet.json

This should be l2SystemConfigAddress.

I just realized that I erroneously added this to l1Config. We should fix that, but for now it seems safer to maintain consistency with l2geth.

Thegaram · 2025-07-15T12:57:44Z

crates/scroll/chainspec/src/genesis.rs

@@ -108,6 +112,8 @@ pub struct ScrollChainConfig {
    /// This is an optional field that, when set, specifies where L2 transaction fees
    /// will be sent or stored.
    pub fee_vault_address: Option<Address>,


Unrelated to this PR, but how come we only have one l1_message_queue_address? In EuclidV2 we added a new message queue, both should be indexed

We missed this update to the genesis, but we don't ever use the message queue address in Reth, which is way it was missed.

Rollup-node does not use the genesis config for this?

hmmm no but indeed it would make sense to have the chain spec in the L1 watcher and read config from there.

Thegaram · 2025-07-15T12:59:39Z

crates/scroll/chainspec/src/scroll_sepolia.rs

@@ -24,6 +26,9 @@ pub static SCROLL_SEPOLIA: LazyLock<Arc<ScrollChainSpec>> = LazyLock::new(|| {
            ),
            genesis,
            hardforks: SCROLL_SEPOLIA_HARDFORKS.clone(),
+            base_fee_params: BaseFeeParamsKind::Variable(


How does BaseFeeParamsKind::Variable behave before Feynman?

It will return these values, which are also the default values we use (8 for max change denominator and 2 for elasticity multiplier).

Thegaram · 2025-07-15T13:07:33Z

crates/scroll/evm/src/base_fee.rs

+        );
+
+        if chain_spec.is_feynman_active_at_timestamp(ts) {
+            Ok(feynman_base_fee(chain_spec, parent_header, ts, overhead.saturating_to()))


Any risk about dropping back to u64 here? I guess no since the result of next_block_base_fee must fit into u64 anyway?

L2geth does not have this limitation, so if we set overhead incorrectly the behavior would diverge. But normally this value should be quite small.

If the overhead > u64::MAX, then you would end up with base_fee = MAX_L2_BASE_FEE; at the end right? Since your comment below says we also need to bound the Feynman base fee.

Thegaram · 2025-07-15T13:10:38Z

crates/scroll/evm/src/base_fee.rs

+    if base_fee > MAX_L2_BASE_FEE {
+        base_fee = MAX_L2_BASE_FEE;
+    }


Max fee cap also needs to be applied in feynman_base_fee.

Another clarification is in l2geth if the base fee is bounded excluding the overhead part here: https://github.com/scroll-tech/go-ethereum/blob/c604c901598fd401f1816ac5eff83b5f97bf8b7d/consensus/misc/eip1559.go#L163, would it fail the check here: https://github.com/scroll-tech/go-ethereum/blob/c604c901598fd401f1816ac5eff83b5f97bf8b7d/consensus/misc/eip1559.go#L98?

Thegaram · 2025-07-15T13:11:51Z

crates/scroll/payload/src/builder.rs

@@ -382,14 +394,10 @@ where
        builder_config: &ScrollBuilderConfig,
    ) -> Result<impl BlockBuilder<Primitives = Evm::Primitives> + 'a, PayloadBuilderError> {
        // get the base fee for the attributes.
-        let base_fee: u64 = if self.chain_spec.is_curie_active_at_block(self.parent().number + 1) {


I guess this means we don't support building pre-Curie blocks anymore? (Which is fine.)

I kind of came around on this yes and decided to just remove support for pre-Curie blocks

colinlyguo · 2025-07-16T08:53:56Z

crates/scroll/evm/src/base_fee.rs

+    let eip_1559_base_fee = if chainspec.is_feynman_active_at_timestamp(parent_header.timestamp()) {
+        // extract the eip 1559 base fee from parent header by subtracting overhead from it.
+        let parent_eip_1559_base_fee =
+            parent_header.base_fee_per_gas().expect("Feynman active").saturating_sub(overhead);


Seems the result will be max(0, parent base fee - overhead). But it's OK iiuc since parent base fee will always >= overhead. Or maybe in some blocks right are transition, this will happen, l2geth should also add underflow protection.

That's actually a good point, I think there might be an edge case where we increase ovearhead and that leads to underflow or undefined behavior in l2geth.

Signed-off-by: Gregory Edison <[email protected]>

frisitano

left a couple of comments inline.

frisitano · 2025-07-17T06:31:42Z

crates/scroll/evm/src/base_fee.rs

+    const CURIE_PARAMS_TEST_CASES: [(u64, u64); 8] = [
+        (0u64, 15680000u64),
+        (1000000000, 15714000),
+        (2000000000, 15748000),
+        (100000000000, 19080000),
+        (111111111111, 19457777),
+        (2164000000000, 89256000),
+        (644149677419355, 10000000000),
+        (0x1c3c0f442u64, 15937691),
+    ];
+
+    const OVERWRITTEN_PARAMS_TEST_CASES: [(u64, u64); 7] = [
+        (0, 1),
+        (1000000000, 1),
+        (2000000000, 1),
+        (100000000000, 2),
+        (111111111111, 2),
+        (2164000000000, 22),
+        (644149677419355, 6442),
+    ];
+
+    const EIP_1559_TEST_CASES: [(u64, u64, u64, u64); 3] = [
+        (1000000000, 20000000, 10000000, 1000000000), // usage == target
+        (1000000001, 20000000, 9000000, 987500001),   // usage below target
+        (1000000001, 20000000, 11000000, 1012500001), // usage above target
+    ];
+


Where do these test cases come from?

they come from l2geth: https://github.com/scroll-tech/go-ethereum/blob/develop/consensus/misc/eip1559_test.go#L111

frisitano · 2025-07-17T06:32:03Z

crates/engine/tree/src/tree/mod.rs

+        let backfill_height = if let ControlFlow::Unwind { bad_block, target } = &ctrl {
            warn!(target: "engine::tree", invalid_block=?bad_block, "Bad block detected in unwind");
            // update the `invalid_headers` cache with the new invalid header
            self.state.invalid_headers.insert(**bad_block);

            // if this was an unwind then the target is the new height
-            backfill_height = Some(*target);
-        }
+            Some(*target)
+        } else {
+            // backfill height is the block number that the backfill finished at
+            ctrl.block_number()
+        };


why are we making changes to the core reth logic for this PR?

The clippy CI was failing, I just added the changes from this PR on Reth in order to have it pass.

georgehao · 2025-07-17T09:38:50Z

crates/scroll/evm/src/base_fee.rs

+const L1_BASE_FEE_OVERHEAD_SLOT: U256 = U256::from_limbs([101, 0, 0, 0]);
+
+/// The default base fee overhead, in case the L2 system contract isn't deployed or
+/// initialized.
+pub const DEFAULT_L1_BASE_FEE_OVERHEAD: U256 = U256::from_limbs([15_680_000, 0, 0, 0]);
+
+/// The base fee scalar slot.
+const L1_BASE_FEE_SCALAR_SLOT: U256 = U256::from_limbs([102, 0, 0, 0]);


L2BaseFeeOverheadSlot = common.BigToHash(big.NewInt(101)) L2BaseFeeScalarSlot = common.BigToHash(big.NewInt(102))

The constant name is wrong or the slot number is wrong

I think the name is wrong

georgehao · 2025-07-17T09:45:44Z

crates/net/network/src/manager.rs

@@ -842,20 +842,20 @@ impl<N: NetworkPrimitives> NetworkManager<N> {
                    "Session disconnected"
                );

-                let mut reason = None;
-                if let Some(ref err) = error {
+                let reason = if let Some(ref err) = error {


What are the benefits of this change

none really, it's just to make clippy happy, see here. I think clippy flags this as it is more idiomatic in Rust to use the let var = if let Some(..) = error { ... } else { ... } pattern.

Signed-off-by: Gregory Edison <[email protected]>

greged93 added 3 commits July 4, 2025 13:01

feat: add base fee params and l2 system contract address

23205d7

Signed-off-by: Gregory Edison <[email protected]>

feat: update base fee to Feynman

a35e0ae

Signed-off-by: Gregory Edison <[email protected]>

feat: propagate the base fee provider in the pool

6506202

Signed-off-by: Gregory Edison <[email protected]>

greged93 requested review from Thegaram, frisitano and colinlyguo July 4, 2025 21:33

greged93 commented Jul 4, 2025

View reviewed changes

greged93 added 4 commits July 14, 2025 10:59

Merge branch 'scroll' into feat/feynman-base-fee

f585780

fix: lints

9a5f05e

Signed-off-by: Gregory Edison <[email protected]>

fix: make clippy happy

f91e166

Signed-off-by: Gregory Edison <[email protected]>

fix: riscv

8aa3a87

Signed-off-by: Gregory Edison <[email protected]>

greged93 commented Jul 14, 2025

View reviewed changes

greged93 added 2 commits July 14, 2025 13:25

test: fix

ca4f14e

Signed-off-by: Gregory Edison <[email protected]>

fix: doc tests

bd9782b

Signed-off-by: Gregory Edison <[email protected]>

greged93 requested a review from georgehao July 14, 2025 12:15

Thegaram reviewed Jul 15, 2025

View reviewed changes

colinlyguo reviewed Jul 16, 2025

View reviewed changes

fix: answer comments

9780398

Signed-off-by: Gregory Edison <[email protected]>

greged93 requested a review from Thegaram July 16, 2025 13:46

frisitano reviewed Jul 17, 2025

View reviewed changes

georgehao reviewed Jul 17, 2025

View reviewed changes

fix: constant naming

2e7b149

Signed-off-by: Gregory Edison <[email protected]>

georgehao approved these changes Jul 18, 2025

View reviewed changes

		// TODO: should we get the pool base fee in the case where the transaction is a pending
		// transaction here?

	pub const SCROLL_MAINNET_SYSTEM_CONTRACT_ADDRESS: Address =
	pub const SCROLL_MAINNET_L2_SYSTEM_CONFIG_CONTRACT_ADDRESS: Address =

		/// The address of the L2 system contract.
		pub l2_system_contract_address: Address,

feat: feynman base fee #273

Are you sure you want to change the base?

feat: feynman base fee #273

Uh oh!

Conversation

greged93 commented Jul 4, 2025 • edited by Thegaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #273 will not alter performance

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greged93 Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colinlyguo Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frisitano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgehao Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greged93 Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

greged93 commented Jul 4, 2025 •

edited by Thegaram

Loading

codspeed-hq bot commented Jul 4, 2025 •

edited

Loading

greged93 Jul 14, 2025 •

edited

Loading

colinlyguo Jul 16, 2025 •

edited

Loading

georgehao Jul 17, 2025 •

edited

Loading

greged93 Jul 17, 2025 •

edited

Loading