Add initial integration test with circuits

[ed255]

Add an integration tests for the evm and state circuits, using the input
generated by the circuit input builder and data queried from geth.

bus-mapping: Introduce BuilderClient which wraps a GethClient and
contains methods to generate the complete circuit inputs by doing all
the necessary steps: query geth, build partial state_db, etc. Use this
BuilderClient in both the circuits and circuit_input_builder
integration tests.

---

For the state circuit I've used the pub StateCircuit.

For the evm circuit, I didn't find a pub circuit, so I copied the TestCircuit used in the evm circuit tests. I don't know if this is the best approach, please let me know if you have a better idea!

I haven't included the state_circuit integration tests into the CI because, even though the evm_circuit is passing, the state_circuit is not working.
The test tries to build the proof based on the block in which the Greeter.sol contract is deployed.
Here's a log that includes the generated memory, stack and storage ops, as well as the verification error that the MockProver outputs: https://gist.github.com/ed255/ba4991427bb4cee0fa909f498e9e5786
Is the state circuit supposed to fail? Does anybody have an idea of what may be failing, based on the generated ops by the circuit input builder?

The state circuit integration test can be reproduced following these steps:

Setup

cd integration-tests
./run.sh --steps "setup"
./run.sh --steps "gendata"

Test

RUST_LOG=circuits=trace cargo test --profile release --test circuits --features circuits test_state_circuit_block_a -- --nocapture

---

With the addition of the circuit integration tests, the integration tests github action take between 16m an 20m. For this reason I have disabled the trigger on pull request and left only the trigger on push to main.

[han0110]

LGTM!

The reason module evm_circuit doesn't expose a directly usable circuit is because we haven't decide how to connect all circuits together. A direct approach is to replace the rw_table with StateCircuit and bytecode_table with BytecodeCircuit and force them to implement the LookupTable trait. But not sure if this is a desired approach (it hasn't taken aggregation into consideration).

But if the "integration" is for integration of bus-mapping and circuits, I think using mocking advice table is enough.

[ed255]

The reason module evm_circuit doesn't expose a directly usable circuit is because we haven't decide how to connect all circuits together. A direct approach is to replace the rw_table with StateCircuit and bytecode_table with BytecodeCircuit and force them to implement the LookupTable trait. But not sure if this is a desired approach (it hasn't taken aggregation into consideration).

Thanks for the explanation! I've moved the TestCircuitConfig and TestCircuit in a module inside the test so that it's easier to see which part of the integration test is temporary and copied from the evm_circuit module.

But if the "integration" is for integration of bus-mapping and circuits, I think using mocking advice table is enough.

I wanted to see how far the pieces we have could be connected between them at this stage. The truth is that I copied the run_test_circuit and TestCircuit from evm_circuit without paying to much attention, and I didn't realize it was using mock advice. I think it would be much better to use the advice provided by the circuit input builder as much as possible, but I'm not exactly sure how to proceed to achieve that.

[han0110]

I think it would be much better to use the advice provided by the circuit input builder as much as possible.

The mocking advice table is using the witness from circuit input builder, the only different is that they are not enforced with some relation (read/write consistency of rw_table, validity of hash of bytecode_table), that is implemented in another circuit.

I think after #170 is merged, we can try to connect it here, and State circuit might need some more extra effort for integration.

[lispc]

The test of the evm citcuit should not pass too. There was a bug due to which evm circuit tests can never fail. After the fix, the evm circuit test here will output thread 'test_evm_circuit_block_a' panicked at 'not implemented: unimplemented opcode CALLVALUE', zkevm-circuits/src/evm_circuit/witness.rs correctly.

More details:
the evm trace being tested here is the deploy related bytecodes. The YUL representation of the bytecode executed in this integration test is this constructor YUL IR. Opcode like CALLVALUE is not implemented in evm circuit, so the evm circuit test should fail.

[miha-stopar]

Sorry, I totally overlooked this review was assigned to me. But it looks all good to me! I think it can be merged once conflicts and what @lispc mentioned are resolved.

[ed255]

With @miha-stopar we analyzed why the state circuit test was failing. The data generated by the circuit input builder for StackOps and MemoryOps contained some cases of a READ following a WRITE at the same address with different values.

StackOps

After analyzing the cases I realized that this is expected in the current state because we haven't implemented all opcodes in the bus-mapping, so by skipping an opcode that writes to the stack, we skip generating the StackOp that does the WRITE. If a following step does a READ we will get a READ which doesn't match the previous generated WRITE.

MemoryOps

I found out that the traces returned by geth never returned memory contents. After some digging I found that we were affected by this issue ethereum/go-ethereum#24146 which I fixed by introducing a configuration parameter for the relevant rpc methods to request memory dumpss in the traces. With that fixes the state circuit still fails on MemoryOps constraints: this is again because we don't have all the opcodes implemented in the bus-mapping that touch memory. In particular, in our trace we have a CODECOPY, which writes to memory, but we don't have it implemented, so we generate a later READ that doesn't match the previous WRITE.

[lispc]

The test of the evm citcuit should not pass too. There was a bug due to which evm circuit tests can never fail. After the fix, the evm circuit test here will output thread 'test_evm_circuit_block_a' panicked at 'not implemented: unimplemented opcode CALLVALUE', zkevm-circuits/src/evm_circuit/witness.rs correctly.

More details: the evm trace being tested here is the deploy related bytecodes. The YUL representation of the bytecode executed in this integration test is this constructor YUL IR. Opcode like CALLVALUE is not implemented in evm circuit, so the evm circuit test should fail.

@ed255 Hi~ Just curious the above comment I found without some opcode like callvalue, the evm circuit test using a block with contract constructor should have failed. But i think finally the test runs well when being merged. So i wonder maybe some updates/changes are applied here? Can you give me some information? Thank you!

[ed255]

@ed255 Hi~ Just curious the above comment I found without some opcode like callvalue, the evm circuit test using a block with contract constructor should have failed. But i think finally the test runs well when being merged. So i wonder maybe some updates/changes are applied here? Can you give me some information? Thank you!

I just ran an integration test on main with a block that runs a contract deployment. I did it like this:

cd integration-tests
$ RUST_LOG=circuits,bus_mapping=trace cargo test --profile release --test circuits --features circuits test_evm_circuit_block_deploy_greeter -- --nocapture

This is the result:

    Finished release [optimized] target(s) in 0.25s
     Running tests/circuits.rs (/home/dev/git/zkevm/zkevm-circuits/target/release/deps/circuits-eb262837322ee84e)

running 1 test
[2022-03-01T12:11:12Z WARN  bus_mapping::evm::opcodes] Using dummy gen_associated_ops for opcode CODESIZE
[2022-03-01T12:11:12Z WARN  bus_mapping::evm::opcodes] Using dummy gen_associated_ops for opcode CODECOPY
[2022-03-01T12:11:12Z WARN  bus_mapping::evm::opcodes] Using dummy gen_associated_ops for opcode SSTORE
[2022-03-01T12:11:12Z WARN  bus_mapping::evm::opcodes] Using dummy gen_associated_ops for opcode CODECOPY
[2022-03-01T12:11:12Z WARN  bus_mapping::evm::opcodes] Using dummy gen_associated_ops for opcode RETURN
thread 'test_evm_circuit_block_deploy_greeter' panicked at 'not implemented', zkevm-circuits/src/evm_circuit/witness.rs:1109:26
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test test_evm_circuit_block_deploy_greeter ... FAILED

failures:

failures:
    test_evm_circuit_block_deploy_greeter

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 5 filtered out; finished in 0.04s

error: test failed, to rerun pass '--test circuits'

The warnings come from the bus_mapping because there are opcodes which are not implemented, so their associated ops are not generated.
And then the panic comes from the witness converter found in the evm circuit: it doesn't yet support code source from tx input (only from account).

So currently the integration test for evm circuit with contract deployment is not passing.

On the other hand, all the integration tests with circuits are currently disabled in the Github Actions: https://github.com/appliedzkp/zkevm-circuits/blob/a07200fb2675cada12bbcc3abc5fdaf21d101b55/.github/workflows/integration.yml#L62 So they are not triggered in PRs.

[lispc]

ok got it. I thought the tests could pass. Thank you!