Update halo2 dependency to v2023_04_20

[ed255]

Description

Update halo2 to the latest dependency, which contains some changes affecting the ff trait.

Issue Link

Resolve #1366

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Rationale

I've extended the Field trait we define in eth_types to contain the common traits used in different places, to avoid having a custom list of traits in each function.

[ed255]

Currently many tests are failing at a new assertion from halo2 mock prover assignment here:
https://github.com/privacy-scaling-explorations/halo2/blob/17e9765c199670534c0299c96128d0464a188d0b/halo2_proofs/src/dev.rs#L466
This assert panics when a cell is re-assigned with a different value. I have observed panics in three places:

• evm_circuit: The assignment is coming from StoredExpression / CachedRegion. I need to look further why cells are re-assigned with different values
• state_circuit: Negative tests do re-assignment of different values on purpose, so it's not clear how this can be fixed
• tx_circuit: I haven't checked it yet

[CPerezz]

Need help with that?

[ed255]

Need help with that?

I think I do 🥲
So far I have solved the failures in the tx_circuit. They were unrelated to cell re-assignment.
I've also checked the evm_circuit failure. So here's a summary of where we are:

In privacy-scaling-explorations/halo2#129 Han introduced a check for the mock prover advice assignment with the following comment: Inconsistent assignment between different phases.. As far as I see the check also applies when doing inconsistent assignment in the same phase. Now we have:

• evm_circuit: The assignment is coming from StoredExpression / CachedRegion. To assign each execution step, we do an optimistic assignment of the next step because sometimes expressions in the current step need values of the next one. I say optimistic because there are some values of the next step that are not known yet, so we first do an assignment that may not be correct, and then re-do it with the correct values.
• state_circuit: Negative tests do re-assignment of different values on purpose, so it's not clear how this can be fixed

Both cases seem reasonable to me, and they are not friendly to the re-assignment with different value check. So I would like help on ideas to proceed!

I have thought about using the Cached region to first store all the assignments (where we can re-assign freely), and after everything is assigned, we do the real assignment. I think this would add a lot of unneeded complexity. Moreover, the assignment function returns an AssignedCell which can't be constructed outside of halo2

The other option that comes to my mind is to disable the re-assignment check, maybe with a toggle in the MockProver, or something like that, and then we can disable it for the EVM and state circuits tests. Nevertheless that means we lose the original feature.

A final option is to redo the feature to make the MockProver complain on reassignment with different values, only when the new assignment is in a different phase. For that we would need to store the phase of each assignment, which would add some overhead.

[CPerezz]

I have thought about using the Cached region to first store all the assignments (where we can re-assign freely), and after everything is assigned, we do the real assignment. I think this would add a lot of unneeded complexity. Moreover, the assignment function returns an AssignedCell which can't be constructed outside of halo2

Agree this is not a way.

The other option that comes to my mind is to disable the re-assignment check, maybe with a toggle in the MockProver, or something like that, and then we can disable it for the EVM and state circuits tests. Nevertheless that means we lose the original feature.

A final option is to redo the feature to make the MockProver complain on reassignment with different values, only when the new assignment is in a different phase. For that we would need to store the phase of each assignment, which would add some overhead.

Do we really want the check?? It just seems there's no good solution for the state circuit case. Let's just get ridd of it for now?

[han0110]

LGTM! Nice work! Just some nits that could be ignored, and also a question about halo2 dependency.

[han0110]

All LGTM now!

[davidnevadoc]

All LGTM 👍