Scroll review

[frisitano]

Feedback from scroll colleague:

overall it looks great, very clean. here's some initial feedback:

• seems like extcodesize is the same as revm's original, why override it then? if the thinking is we can later add a version optimized for zkVM setup (loading code size from state instead of calling code.len()), then we can add that later.
• for L1BlockInfo some of the terminology is not what we normally use. I guess you just followed the Optimism example here.
• for fees I'm not sure we should use saturating_mul, etc. In the unlikely scenario where these overflow we should error instead of failing silently.
• just want to confirm, is Context re-created for each transaction? asking because l1_block_info might change in the middle of a block.
• precompile_not_implemented seems a bit different from how we disabled these in l2geth. not sure if this would break compatibility though.
• fees: currently scroll uses a hardcoded "beneficiary", we either need to make sure to pass this in block.coinbase or need to consider this in the code. also our 1559 fee deduction is a bit different, as not ETH is burnt.
• I see the Optimism version overrides some other things (validate_env, validate_tx_against_state, etc.), we don't need these changes?

[frisitano]

• seems like extcodesize is the same as revm's original, why override it then? if the thinking is we can later add a version optimized for zkVM setup (loading code size from state instead of calling code.len()), then we can add that later.

From my initial observation it looked like the gas calculation was different as seen below:

upstream:

    if SPEC::enabled(BERLIN) {
        gas!(interpreter, warm_cold_cost_with_delegation(load));
    } else if SPEC::enabled(TANGERINE) {
        gas!(interpreter, 700);
    } else {
        gas!(interpreter, 20);
    }

scroll:

gas!(interpreter, warm_cold_cost(load.state_load.is_cold));

This materialises into a difference of this additional conditional related to delegated account:

    if let Some(is_cold) = load.is_delegate_account_cold {
        gas += warm_cold_cost(is_cold);
    }

I'm not sure how this has been implemented in l2geth but my concern was that this would cause a divergence, so I just followed the implementation used in scroll's revm fork.

• for L1BlockInfo some of the terminology is not what we normally use. I guess you just followed the Optimism example here.

I took this data structure directly from scroll's revm fork. Please feel free to open a pull request to address this or alternatively point me to a spec or reference implementation so we can look to converge on the standard.

• for fees I'm not sure we should use saturating_mul, etc. In the unlikely scenario where these overflow we should error instead of failing silently.

Good catch. Lets create an issue to evaluate error handling here. I had just lifted the code from scroll's revm fork.

• just want to confirm, is Context re-created for each transaction? asking because l1_block_info might change in the middle of a block.

Yes, the pre_exec.load_accounts handler is invoked at the start of every transaction and as such the Context will be recreated for each transaction.
It would be great if you could provide a description of how the L1 fee transaction works that would help aid my understanding.

• precompile_not_implemented seems a bit different from how we disabled these in l2geth. not sure if this would break compatibility though.

How are they disabled in l2geth?

• fees: currently scroll uses a hardcoded "beneficiary", we either need to make sure to pass this in block.coinbase or need to consider this in the code. also our 1559 fee deduction is a bit different, as not ETH is burnt.

I would say that this should be configured via the client as opposed to hardcoding a beneficiary in the scroll-revm implementation, I'm not sure exactly how this is done but it should be possible. The reason I think this is preferable is because we may have other consumers of scroll-revm that may want to use a different beneficiary to the one that scroll uses.

• I see the Optimism version overrides some other things (validate_env, validate_tx_against_state, etc.), we don't need these changes?

I had not looked into these handlers it's possible they may be needed.