Move external_tracer from bus mapping to new crate only for test

[scroll-dev]

Close scroll-tech#61
Close #272

Summary

1. Move common types of external-tracer to etc-types/src/geth_types.rs.
2. Move test code to a new crate external-tracer.
3. Separate bus_mapping::mock::BlockData to test implement (commented with #[cfg(test)]) and common implement (exported for test of zkevm-circuit).

[silathdiir]

I add a new struct GethData which need geth_utils::trace to generate. It is used for test code of both bus-mapping and zkevm-circuits. Both bus-mapping and zkevm-circuits need to add new crate external-tracer to dev-dependencies.

There are two places of impl BlockData in bus-mapping/src/mock.rs. One is commented with #[cfg(test)], it is used for test cases in bus-mapping. And another impl BlockData contains code which is exported for test code of zkevm-circuits. Since code with comment #[cfg(test)] cannot be exported outside of crate.

[ed255]

After taking a look at the changes I want to share some thoughts:
Originally, the mock module was found in the bus-mapping and it contained:

• BlockData: a struct to put all the different pieces that are needed to generate the witness of a block without future geth interactions
• Functions to generate mock data (block, tx, etc.)
• Functions to BlockData with real geth traces for different situations.

These are used both in the bus-mapping and the circuits for testing. In the current form of this PR the mock functionality is split into different parts (bus-mapping, bus-mapping with cfg test, external-tracer, geth_types). I think it's useful to keep them together, so what about moving the complete original bus-mapping::mock to a new crate? This way both the bus-mapping and the zkevm-circuits can import it as a dev-dependency and everything is defined in one place. This way the zkevm-circuits can use mock functions the same way as the bus-mapping:
Like this

    let block_trace =
        bus_mapping::mock::BlockData::new_single_tx_trace_code_gas(
            &bytecode,
            Gas(config.gas_limit),
        )
        .unwrap();

Instead of

    let accounts = [bus_mapping::mock::new_tracer_account(&bytecode)];
    let geth_data = external_tracer::create_tx_by_accounts(
        &accounts,
        Gas(config.gas_limit),
    )
    .unwrap();
    let block_trace =
        bus_mapping::mock::BlockData::new_single_tx_trace(&accounts, geth_data)
            .unwrap();

Also I think mock data shouldn't belong in geth_types (MOCK_CHAIN_ID, MOCK_GAS, etc). Also the structs originally found at external-tracer::{BlockConstants, Transaction} are types that should only be used with the external-tracer (they are used to setup the environment and output of the external-tracer), so they shouldn't be moved to a different place, they should stay with the external-tracer.

Finally, I think that by moving the full mock module to it's own crate, there will be no need to introduce the GethData.

What do you think about moving the complete original mock module to a new crate?

[silathdiir]

@ed255 Yes. I think it should be better to move mod mock to a new crate which could be used as a dev-dependency for bus-mapping and zkevm-circuits. I will give a try. Thanks.

And I have a question about mod bytecode, state_db and circuit_input_builder which are used by mod mock. It seems that circuit_input_builder should not be moved out of bus-mapping. So I should leave mock::new_circuit_input_builder as a test function in bus-mapping.

But if I create a new mock crate, both bytecode and state_db should be used for this new crate and bus-mapping. Should I move mod bytecode and state_db to a common crate (eth-types?)? What do you think? Thanks.

[ed255]

And I have a question about mod bytecode, state_db and circuit_input_builder which are used by mod mock. It seems that circuit_input_builder should not be moved out of bus-mapping. So I should leave mock::new_circuit_input_builder as a test function in bus-mapping.

But if I create a new mock crate, both bytecode and state_db should be used for this new crate and bus-mapping. Should I move mod bytecode and state_db to a common crate (eth-types?)? What do you think? Thanks.

Ah, you're completely right! It's not straight forward to move the complete mock outside of the bus-mapping because it would create circular dependencies. I think I now understand why you introduced GethData :)

So here's a new proposal:

1. Extend GethData with an accounts: Vec<Account> field.
2. In bus-mapping mock only leave the declaration of BlockData with the methods new_circuit_input_builder and a method to build a BlockData from a GethData (similar to BlockData::new_single_tx_trace but only takes geth_data as input, and maybe rename it to new_from_geth_data or something like that).
3. Put all the methods slice_from_code_start, new_single_tx_trace_accounts, new_single_tx_trace_code that currently are in bus-mapping mock, into the new mock crate. They will return GethData.

So now we can have a mock crate imported only as testing, and a very small BlockData with just a few methods. Now the zkevm-circuits can import mock as dev-dependency, and bus-mapping as dependency and do something like this:

    let block_trace =
        bus_mapping::mock::BlockData::new_from_geth_data(
            mock::geth_data::new_single_tx_trace_code_gas(
                &bytecode,
                Gas(config.gas_limit),
            ).unwrap()
        );

Unfortunately the GethData can't be defined in mock, because it would imply that bus-mapping imports mock without dev-dependency. So I think it's ok to define GethData in eth-types.

Nevertheless I still think that all the mock constants (currently in eth-types::geth_types) can be moved to the mock crate. The rest of types currently defined in eth-types::geth_types (that is Account, BlockConstants, Transaction), I think it's OK to leave them there, as they are used by GethData.

So in the end the mock crate would only contain methods to generate GethData using the external-tracer with various settings. Do you think this makes sense?

[silathdiir]

Hi @ed255 , according to your code review, I make the below fixes:

1. Move functions for generating GethData to a new crate mock. So both bus-mapping and zkevm-circuits use mock as dev-dependencies.
2. Move bus_mapping::bytecode to eth_types. Since the type Bytecode is an argument of a generating GethData function in new crate mock.
3. Update bus_mapping::mock only contains function new_circuit_input_builder and new_from_geth_data.
4. Move MOCK_* to crate mock.
5. Update test cases.

Please help review when you have time. Thanks.

[ed255]

Great work! LGTM!

[ed255]

@silathdiir everything looks good! Once you bring this branch up to date with main and resolve the conflicts, we can merge this :)