Small changes for EVM circuit updates

[Brechtpd]

For privacy-ethereum/zkevm-circuits#417

• Lazy evaluation in MockProver: More rows need to be checked again, but because a lot of expressions are disabled by selectors, big parts of expressions can simply be skipped when the selector is zero.
• Column index is made public
• An expression identifier is added to make is possible to easily verify if expressions calculate the same thing

[han0110]

LGTM. Only got a question on docs.

[han0110]

... but the expressions don't need to be exactly equal in how they are composed e.g. 1 + 2 and 2 + 1 can have the same identifier

At first glance at the docs I thought this method helps to organize expression to make 1 + 2 and 2 + 1 have the same identifier, but it seems not. Does this aim to info developer to use more identical expression as possible (to reuse stored expression as much as possible)?

[Brechtpd]

I had some naive code to try and do the reorganization, but chose to remove it for now because it didn't really work well enough and didn't make any difference. I think it's not super easy to do this well elegantly (or at least I haven't found one yet, but haven't really tried much to be honest). But yeah the goal is indeed that this identifier can directly be used to detect expressions that are interchangeable (so in our use case to reuse stored expressions when possible).

So for now the docs share how the function could behave in the future, and so e.g. should not be used to compare expressions if strict equality is required.

[han0110]

I see, I also tried once to normalize expression, but I found it's really ugly (use some kinds of map as key of map) and not sure if it works, can feel that it's not easy to do.

[CPerezz]

Since HashMap in std rust now relies on hashbrown which is pretty damn fast.
Have we tried to store as ID the map key containing ([values, involved], Expression::Type)? With arrays we can check quickly with contains and similar things. Same in Iterators.

Expression::Type here simply means that for any set of inputs (independent of the order), if we perform the same op we will get the same result. (So we don't need to store the result in the keys and only in the values).

Is there anything painful on using HashMap?

[CPerezz]

Brilliant work as always @Brechtpd

Just left some comments with ideas :)

Also, seems that rustdoc is complaining for intra_doc_links lint.

halo2_gadgets/src/lib.rs#L7

lint `broken_intra_doc_links` has been renamed to `rustdoc::broken_intra_doc_links`

[CPerezz]

Since HashMap in std rust now relies on hashbrown which is pretty damn fast.
Have we tried to store as ID the map key containing ([values, involved], Expression::Type)? With arrays we can check quickly with contains and similar things. Same in Iterators.

Expression::Type here simply means that for any set of inputs (independent of the order), if we perform the same op we will get the same result. (So we don't need to store the result in the keys and only in the values).

Is there anything painful on using HashMap?

[Brechtpd]

Also, seems that rustdoc is complaining for intra_doc_links lint.

halo2_gadgets/src/lib.rs#L7

lint `broken_intra_doc_links` has been renamed to `rustdoc::broken_intra_doc_links`

I renamed them in https://github.com/appliedzkp/halo2/pull/44/files, but I think that is straight from halo2 upstream so I'm not sure why they were not renamed there already. I don't know if there's some version difference for something.

[CPerezz]

@Brechtpd any plans to try #54 (comment) ?

Or should we merge directly?

[Brechtpd]

@Brechtpd any plans to try #54 (comment) ?

Or should we merge directly?

I would prefer to merge it as is. It's not easy to get something that catches all similar expressions. I do have something that tries to make more of an effort, but at least currently it doesn't make a difference. I would prefer to postpone a better method to a later PR.