feat: create feehandler system by tac0turtle · Pull Request #48 · evstack/ev-reth

tac0turtle · 2025-09-03T08:19:16Z

Description

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Performance improvement
Refactoring

Related Issues

Fixes #(issue)

Checklist

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published

Testing

Additional Notes

damiannolan

I will have to review and look into this in much more depth but from what I understand applying changes to the state here outside of the EVM and recomputing the root would be a problem for us in our zk prover, as we depend on RSP which applies transactions using a block builder and computes new roots based on that.

So to me, that would suggest that this implementation would fall outside the scope of that and lead to a divergence of state roots

jonas089 · 2025-09-23T15:43:37Z

I will have to review and look into this in much more depth but from what I understand applying changes to the state here outside of the EVM and recomputing the root would be a problem for us in our zk prover, as we depend on RSP which applies transactions using a block builder and computes new roots based on that.

So to me, that would suggest that this implementation would fall outside the scope of that and lead to a divergence of state roots

I share this concern. Manually mutating the state root rather than taking the resulting state root from the execution of the EVM transactions could make it impossible to replay the transactions in ZK, unless all of the operations can be re-applied and the same state root can be derived.

Also I wonder about the implications this has for eth_getProof?

Edit: Or is the original state root still exposed and used for eth_getProof?

tac0turtle · 2025-09-23T16:36:25Z

still looking into how to change this without manipulating state outside the evm level

liamsi

Just some nitpicks / typos while reading through the code

crates/ev-revm/src/config.rs

damiannolan

I did not review the ev-revm code in detail. I don't have intimate knowledge of evm internals but given the ADR and my higher level understanding this all computes and makes sense to me.

I think this approach leveraging reward_beneficiary() is much better than the previous, first drafted design and integrates better overall with the prover arch we have been working with.

We will have to allocate time to dedicate to testing this once its merged and ev-reth is happy with it.

Is it possible to get a git tag cut with the ev-reth code before this PR is merged? If its just an alpha or beta tag that's totally fine.

damiannolan · 2025-10-06T13:03:33Z

crates/node/src/builder.rs

+    /// EVM configuration (potentially wrapped with base fee redirect)
+    pub evm_config: WrappedEthEvmConfig,


Would it make more sense to call this type alias EvolveEvmConfig ?

Suggested change

/// EVM configuration (potentially wrapped with base fee redirect)

pub evm_config: WrappedEthEvmConfig,

/// EVM configuration (with optional base fee redirect)

pub evm_config: EvolveEvmConfig,

damiannolan · 2025-10-06T13:25:37Z

etc/ev-reth-genesis.json

    "terminalTotalDifficulty": 0,
-    "terminalTotalDifficultyPassed": true
+    "terminalTotalDifficultyPassed": true,
+    "ev_reth": {


nit, all configuration fields are camelCase in these genesis files and this is snake_case.

Maybe we could consider something else, e.g. just evolve

damiannolan · 2025-10-06T14:06:57Z

docs/adr/ADR-0001-base-fee-redirect.md

This ADR is helpful, thank you! 🙏🏻

So this just deals strictly with the base fee, correct? The "tip" still goes to a potentially different or potentially the same coinbase/beneficiary still?

total_fee = gas_price * gas_used base_fee = block.baseFeePerGas * gas_used tip = total_fee - base_fee base fee -> burn || reward_beneficiary(baseFeeSink) tip -> coinbase/beneficiary

damiannolan · 2025-10-06T14:32:15Z

crates/node/src/builder.rs

-use tracing::debug;
+use tracing::{debug, info};
+
+type WrappedEthEvmConfig = EthEvmConfig<ChainSpec, EvEvmFactory<EthEvmFactory>>;


I believe this is what we need in order to operate succinct/rsp. As this should implement the following trait: https://github.com/paradigmxyz/reth/blob/main/crates/evm/evm/src/lib.rs#L184

From what I've looked at this is what matters - as this has the full set of API calls wired with the essential overrides. It would require a definition similar to this but with the EvEvmFactory wired in: https://github.com/succinctlabs/rsp/blob/main/crates/executor/client/src/executor.rs#L151-L161

We will have to test from our side but this seems to be the integration point I was looking for.

liamsi · 2025-10-06T14:24:59Z

crates/ev-revm/src/handler.rs

+        let gas = exec_result.gas();
+        let spent = gas.spent_sub_refunded();
+
+        if let (Some(redirect), true) = (self.redirect, spent != 0) {


Should we log sth if redirect is None or spent is 0? Or is the expectation that there will always be gas spent or a redirect in place? either way, I think it would be good to improve the introspecatibility here.

liamsi · 2025-10-06T14:40:35Z

crates/node/src/config.rs


    /// Validates the configuration
    pub const fn validate(&self) -> Result<(), ConfigError> {
        Ok(())


Are we sure the extra added sink can't be invalid in some way (like an invalid address)? Shouldn't this be checked here?

Thinking about this further: if setting the addr to 0xffffffffffffffffffffffffffffffffffffffff or other special addresses this might defeat the purpose. I think we should do further validation on the sink addr.

why would it defeat the purpose? setting it to a predefined special address should be fine

crates/ev-revm/src/factory.rs

liamsi

There is a few issues with factory.rs when trying to get this PR to compile and run tests locally. I think this should be fixed before approving this PR. I left a few other rather non-blocking comments. The design makes sense to me. The code base is new to me though and I can't really say much about prover compatibility but if @damiannolan says this works for the prover part, that's good.

damiannolan · 2025-10-06T15:07:44Z

Once this branch is compiling and we can run an ev-reth instance with it using the base fee redirect then I'm happy to allocate time to testing it with the prover based on what I mentioned in #48 (comment)

liamsi

I think a few more tests and this can be merged.

liamsi · 2025-10-07T12:20:57Z

crates/ev-revm/src/handler.rs

+        self.inner.reimburse_caller(evm, exec_result)
+    }
+
+    fn reward_beneficiary(


Ideally, we would add a test that this method behaves as expected in the happy and unhappy paths or rather that the integration works correctly. I see the tests in for redirect.apply in base_fee.rs but would it be possible to test the redirect itself without too much hassle? (Like some minimal Context::mainnet() with EmptyDB or sth?) Please ignore if this is too complex. As mentioned I'm not too familiar with the code base.

liamsi · 2025-10-07T12:33:52Z

docs/adr/ADR-0001-base-fee-redirect.md

+## Test Cases
+
+* Unit tests cover the redirect logic inside `BaseFeeRedirect` and verify the handler credits both the sink and block beneficiary as expected (`crates/ev-revm/src/base_fee.rs`, `crates/ev-revm/src/factory.rs`).
+* Integration smoke tests should execute payload-building runs with and without the `config.ev_reth.baseFeeSink` field present to confirm state roots differ only when the redirect is active.


If I understand the tests in common.rs correctly, the config and the resulting fixture there does not use base_fee_sink. I think it would be good to add this param there too.

docs/adr/ADR-0001-base-fee-redirect.md

crates/node/src/config.rs

crates/ev-revm/src/factory.rs

docs/adr/ADR-0001-base-fee-redirect.md

liamsi · 2025-10-07T13:18:31Z

docs/adr/ADR-0001-base-fee-redirect.md

+
+We introduced a new `ev-revm` crate that mirrors Optimism’s `op-revm` layout but adds an `EvEvm` wrapper. The wrapper:
+
+* Stores the optional `BaseFeeRedirect` policy and a flag indicating whether inspection is active.


Question regarding this flag:

ev-reth/crates/ev-revm/src/evm.rs

Lines 305 to 309 in 52ddc52

if self.inspect {

InspectEvm::inspect_tx(self, tx)

} else {

ExecuteEvm::transact(self, tx)

}

The redirection of fees works in both cases, right?

liamsi · 2025-10-08T14:42:56Z

crates/ev-revm/src/base_fee.rs

+        }
+
+        let amount = U256::from(base_fee) * U256::from(gas_used);
+        if amount.is_zero() {


Can this even be zero after the check above?

if gas_used == 0 || base_fee == 0 { return Ok(U256::ZERO); }

liamsi · 2025-10-08T14:49:29Z

crates/node/src/builder.rs

                        i, gas_used
                    );
                }
                Err(err) => {


I'm not sure if this is a dumb question but do we need to make sure that the sink is also credited here? I would assume this is not necessary as this code existed before and did not require a particular burn for fees instead.

liamsi · 2025-10-08T15:00:57Z

crates/ev-revm/src/base_fee.rs

+            .load_account(self.fee_sink)
+            .map_err(BaseFeeRedirectError::Database)?;
+        journal
+            .balance_incr(self.fee_sink, amount)


Question: What is the expected behaviour if the sender is equal to the sink? (Just trying to think of any edge cases).

it would be a free transaction as the fee would just be moved from the users wallet back to the users wallet

liamsi · 2025-10-08T15:04:09Z

crates/node/src/config.rs


    /// Validates the configuration
    pub const fn validate(&self) -> Result<(), ConfigError> {
        Ok(())


Thinking about this further: if setting the addr to 0xffffffffffffffffffffffffffffffffffffffff or other special addresses this might defeat the purpose. I think we should do further validation on the sink addr.

tac0turtle · 2025-10-14T13:52:50Z

marking it for r4r tested locally, ill address the issues and try to identify possible issues

liamsi

🚀

create feehandler system

eead581

This comment was marked as resolved.

Sign in to view

tac0turtle added 3 commits September 23, 2025 14:27

Merge branch 'main' into feat/basefee-redirect

9b4ac85

add fee handling

a6e5a6f

modify system and add docs

4697d19

damiannolan reviewed Sep 23, 2025

View reviewed changes

tac0turtle added 4 commits September 26, 2025 10:33

custom evm

8d48eda

remove feehandler

d691de2

make it work with chainspec and update docs

46a2f65

remove feehandler doc

c53e17a

liamsi reviewed Oct 6, 2025

View reviewed changes

crates/ev-revm/src/config.rs Outdated Show resolved Hide resolved

crates/ev-revm/src/config.rs Outdated Show resolved Hide resolved

crates/ev-revm/src/config.rs Outdated Show resolved Hide resolved

crates/ev-revm/src/config.rs Outdated Show resolved Hide resolved

damiannolan approved these changes Oct 6, 2025

View reviewed changes

liamsi reviewed Oct 6, 2025

View reviewed changes

liamsi requested changes Oct 6, 2025

View reviewed changes

tac0turtle added 2 commits October 7, 2025 12:00

fix clippy and test imports

6efb4a9

Merge branch 'main' into feat/basefee-redirect

200b376

liamsi reviewed Oct 7, 2025

View reviewed changes

rename

52ddc52

liamsi reviewed Oct 7, 2025

View reviewed changes

docs/adr/ADR-0001-base-fee-redirect.md Outdated Show resolved Hide resolved

liamsi reviewed Oct 7, 2025

View reviewed changes

tac0turtle added 4 commits October 7, 2025 19:16

tests, e2e still needs loving

c7b45ac

snakecase and write more tests

deabe94

build docker images on pull requests

56702b0

rename to evolve

cf03540

liamsi reviewed Oct 9, 2025

View reviewed changes

pipe fees through the executpr

b838928

tac0turtle marked this pull request as ready for review October 14, 2025 13:52

tac0turtle requested a review from a team as a code owner October 14, 2025 13:52

tac0turtle added 2 commits October 14, 2025 15:59

clippy

eb3c6fb

if no tip address is set use the sink address

8ee3cc7

liamsi approved these changes Oct 14, 2025

View reviewed changes

cleanup

6fb0eef

tac0turtle force-pushed the feat/basefee-redirect branch from c19b45e to 6fb0eef Compare October 16, 2025 08:03

tac0turtle mentioned this pull request Oct 16, 2025

test: refactor ev-reth and node #60

Closed

modify adr

53dc218

tac0turtle merged commit f176ff1 into main Oct 16, 2025
14 checks passed

tac0turtle deleted the feat/basefee-redirect branch October 16, 2025 08:29

damiannolan mentioned this pull request Nov 27, 2025

Test support for EvolveClientExecutor with custom evm config celestiaorg/celestia-zkevm#317

Open

		/// EVM configuration (potentially wrapped with base fee redirect)
		pub evm_config: WrappedEthEvmConfig,


		We introduced a new `ev-revm` crate that mirrors Optimism’s `op-revm` layout but adds an `EvEvm` wrapper. The wrapper:

		* Stores the optional `BaseFeeRedirect` policy and a flag indicating whether inspection is active.

	if self.inspect {
	InspectEvm::inspect_tx(self, tx)
	} else {
	ExecuteEvm::transact(self, tx)
	}

Comments

Conversation

tac0turtle commented Sep 3, 2025

Description

Type of Change

Related Issues

Checklist

Testing

Additional Notes

Uh oh!

This comment was marked as resolved.

damiannolan left a comment

Choose a reason for hiding this comment

Uh oh!

jonas089 commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tac0turtle commented Sep 23, 2025

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

damiannolan 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

damiannolan commented Oct 6, 2025

Uh oh!

liamsi 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!

Uh oh!

Uh oh!

Uh oh!

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!

tac0turtle commented Oct 14, 2025

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

jonas089 commented Sep 23, 2025 •

edited

Loading