privacy-scaling-explorations / zkevm-circuits

https://privacy-scaling-explorations.github.io/zkevm-circuits/
Other
817 stars 856 forks source link

[Integration Tests] Evm & Super circuit test (ERC20 Transfer) fail for sub_mock_prover #1703

Open AronisAt79 opened 11 months ago

AronisAt79 commented 11 months ago

What command(s) is the bug in?

No response

Describe the bug

https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/7113665859
failures:
    sub_mock_prover::serial_test_evm_circuit_erc20_openzeppelin_transfer_fail
    sub_mock_prover::serial_test_evm_circuit_erc20_openzeppelin_transfer_succeed
    sub_mock_prover::serial_test_evm_circuit_multiple_erc20_openzeppelin_transfers
    sub_mock_prover::serial_test_evm_circuit_multiple_transfers_0
    sub_mock_prover::serial_test_super_circuit_erc20_openzeppelin_transfer_fail
    sub_mock_prover::serial_test_super_circuit_erc20_openzeppelin_transfer_succeed
    sub_mock_prover::serial_test_super_circuit_multiple_erc20_openzeppelin_transfers
    sub_mock_prover::serial_test_super_circuit_multiple_transfers_0

thread 'sub_mock_prover::serial_test_evm_circuit_erc20_openzeppelin_transfer_fail' panicked at 'circuit fixed columns are not constant for different witnesses', /github/runner/_work/zkevm-circuits/zkevm-circuits/integration-tests/src/integration_test_circuits.rs:351:13

Concrete steps to reproduce the bug. If it's able reproduce via testool, please share test_id from jenkins report

Manually trigger the integration tests GH Action worfklow on main with option: sub_mock_prover

AronisAt79 commented 9 months ago

Further error description: failure happens due to fn test_variadic; for evm tests, different witness inputs result in non equal circuit fixed-columns. Differences in "fixed" matrix happen always at rows 1 and 2. Cell values are shown in attached file test_mock_variadic_EVM.log . (for each row, a list is printed with elements: (column index, value of cell for fixed, value of cell for prev_fixed)).

for example: ROW 1: cells fixed[1][10-16] are assigned value 0x0000000000000000000000000000000000000000000000000000000000000009 while prev_fixed cells are unassigned

ROW 2: cells fixed[2][10-14] are assigned values 0x000000000000000000000000000000000000000000000000000000000000000[1-5] while prev_fixed cells are unassigned

test_mock_variadic_EVM.log]

ps. error does not occur when test_variadic runs on witness input from the same block in sequence

ed255 commented 9 months ago

I assume you swapped "Column" by "Row" in your description, so I will assume the following:

So first we need to figure out what column 1 and 2 are, and then what those rows in those columns contain. A straight forward way to do that would be to use a patched version of halo2 that adds the following code to assign_fixed at https://github.com/privacy-scaling-explorations/halo2/blob/73408a140737d8336490452193b21f5a7a94e7de/halo2_proofs/src/circuit.rs#L335

if column.index == 1 && offset = 10 {
    panic!("Assignment to [1][10]");
}

Then we run the test with the debug profile and with RUST_BACKTRACE=1 and we will get a backtrace that shows us where the fixed cell is assigned; from that it should be easy to figure out the column, and what that assignment is supposed to mean.

Nevertheless I didn't go to that route, instead I did the following: Column indexes are assigned sequentially, so we can see the configuration of the EvmCircuit and count which columns are 1 and 2. The configure / configure_with_params is the method that creates the columns: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3151efd65744eea26bac12f8f0c1d009fc45ff1c/zkevm-circuits/src/evm_circuit.rs#L378-L385

Now we check each table, who many fixed columns it has to see which are columns 1 and 2. We find the following:

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3151efd65744eea26bac12f8f0c1d009fc45ff1c/zkevm-circuits/src/table/block_table.rs#L33-L37

So now we now that the affected columns are BlockTable.tag and BlockTable.index.

Next we check the code that assigns the values to the BlockTable: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3151efd65744eea26bac12f8f0c1d009fc45ff1c/zkevm-circuits/src/evm_circuit.rs#L442 https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3151efd65744eea26bac12f8f0c1d009fc45ff1c/zkevm-circuits/src/table/block_table.rs#L53 https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3151efd65744eea26bac12f8f0c1d009fc45ff1c/zkevm-circuits/src/table/block_table.rs#L61-L62 Row 0 has zeros, and the next ones have the output of: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3151efd65744eea26bac12f8f0c1d009fc45ff1c/zkevm-circuits/src/witness/block.rs#L184 Now we count up to row 10 and find this: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3151efd65744eea26bac12f8f0c1d009fc45ff1c/zkevm-circuits/src/witness/block.rs#L237-L249 And there we find the assignment at row 10 and onwards on columns index, value (which are columns 1 and 2)

We observe that on tag the assigned value is BlockContextFieldTag::BlockHash as u64 which is 9, and that matches the test logs. And on the index the assigned values are self.number - len_history + idx which would be sequential numbers, this also matches the logs.

We see that this assignment loops over history_hashes: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3151efd65744eea26bac12f8f0c1d009fc45ff1c/zkevm-circuits/src/witness/block.rs#L238-L241

So we found the root of the problem. The vector history_hashes has variable length, and this leads to variable assignment to the fixed tag and index columns of the BlockTable. This issue was triggered by the integration tests because we start proving early blocks, so there are not yet 256 previous blocks.

The solution is to make sure that the history_hashes of the block is always padded to the same length (256), and that the padded entries have a number field set such that self.number - len_history + idx in the assignment loop always returns the sequence 0..255

AronisAt79 commented 8 months ago

This issue is now on hold. A fix is required to rerun integration tests. See #1787 @ed255