Challenge in EVM circuit WIP

[adria0]

Uploaded the version I'm working now of evm circuit with challenge api

Uses
https://github.com/privacy-scaling-explorations/halo2/tree/tmp/challenge-evm-circuit

Current status, and stuff I'll be working on

• Not passing copy_circuit_valid_sha3
• Its like 50% slower for now (anyway, I reduced from 300% slower to 50% slower replacing Expression::identifier by https://github.com/privacy-scaling-explorations/halo2/blob/f090828bb829ac817d20b3446b3676bc63af8d3b/halo2_proofs/src/plonk/circuit.rs#L930 ) seems that performance now is the same.
• Lookup RLC challenge in Phase3 using lookup_input challenge
• fix split_expression
• Rebase

Anyway, if anybody wants to take a look (not asking for a detailed review now, only for general comments), will be nice because I'm blocking other issues.

[han0110]

Had a review on the major changes and every opcode today, here are some my thoughts:

• Since we have 3 phases, the witness assignment is called 3 times, so the slow down seems to make sense. In future PR we will need to address this issue by not doing unnecessary computation when the phase is not matching.
• The lookup RLC challenge is still using the same challenge evm_word, I think we need to use another challenge lookup_input usable after second phase, then we'd need more third phase columns in ColumnType::Lookup (or need more phase2 columns if we manage to remove phase3, see next bullet).
• I think we could remove third phase by witnessing the is_zero of RLC value in first phase, since as far as I see there is only is_zero(word_rlc1 - word_rlc2) requiring the third phase
  Edit: I think we still need columns in ColumnType::Lookup to be in third phase, since no matter what those tables just contain the RLC value :(

[adria0]

Had a review on the major changes and every opcode today, here are some my thoughts:

Thanks! :)

• Since we have 3 phases, the witness assignment is called 3 times, so the slow down seems to make sense. In future PR we will need to address this issue by not doing unnecessary computation when the phase is not matching.

But is this happening with MockProver? I added a dbg! of the challange values in the EvmCircuit::synthesize and I'm only getting one print, with the values set. Should I expect here at least two rounds, one with Value::unknown and the other with Value::known ?

running 1 test
[zkevm-circuits/src/evm_circuit.rs:344] challenges = Challenges {
    evm_word: Value {
        inner: Some(
            0x2cc67045d9924a297606aa509ddcf7235ee11f77683a7d127b4cb83d74c12519,
        ),
    },
    keccak_input: Value {
        inner: Some(
            0x1f00d576567535824a93454a0f4679c99b1052a5728d21c5e03b4877c99f12f1,
        ),
    },
}
test evm_circuit::execution::gas::test::gas_gadget_simple ... ok

[han0110]

But is this happening with MockProver? I added a dbg! of the challange values in the EvmCircuit::synthesize and I'm only getting one print, with the values set. Should I expect here at least two rounds, one with Value::unknown and the other with Value::known ?

Ahhh sorry, you are completely right, it should just be one synthesize called in MockProver, only when real prover we will do multiple times synthesize. Sorry for confusing.

[adria0]

@han0110, for thelookup_input challenge, you you think that's enough with 8120bd4 ?

[han0110]

for thelookup_input challenge, you you think that's enough with 8120bd4 ?

It looks good to me! Just left 2 more thoughts.

[han0110]

This seems need to be stored in LookupPhase3 since it contains challenge after phase 2

[adria0]

Oh, sure! I miss this!

[han0110]

It seems we only need 12 here at most for CopyTable.