Implement BYTE opcode + Code reuse utilities

[Brechtpd]

Spec: privacy-scaling-explorations/zkevm-specs#46

Also contains a couple of helper classes and utilities to more easily reuse frequently used code which should also improve readability. Currently these are only used for the BYTE opcode implementation because I'm not sure if people will like this, so let me know what you think.

[spartucus]

Love this code reuse utilities PR.

[ChihChengLiang]

LGTM!
The ConstraintBuilder is a life saver! I like it so much! It makes the circuit code so readable and auditable.

[miha-stopar]

Really nice work! I like the utilities too! I would only perhaps slightly modify the tests to have the variables which are then passed into ExecutionSteps and Operations (because now changing the value to be tested requires changes in multiple places).

[han0110]

LGTM! Nice work!

Only got one concern about the require_in_set, which might use degree up to set.len(), I am thinking maybe we would like to use lookup with some prebuilt tables for those who has too large set.len() like responsible opcodes, constant gas cost and stack overflow/underflow range.

Would it possible to set some max_degree on ConstraintBuilder to panic when the function makes the degree too high?

[Brechtpd]

Really nice work! I like the utilities too! I would only perhaps slightly modify the tests to have the variables which are then passed into ExecutionSteps and Operations (because now changing the value to be tested requires changes in multiple places).

I kind of neglected the tests because I hoped to be able to use the tracer test tool for that, but yeah doesn't seem that will be very soon so should have put in a bit more time there regardless. I have updated the test code.

Would it possible to set some max_degree on ConstraintBuilder to panic when the function makes the degree too high?

I have added a check on the set length, because I think that was kind of the main idea of the check. But I wonder how useful that check really is. The expressions that are passed in could be a higher degree already so perhaps better to add a check on the degree of the final expression that is built? Although even that may not really work as expected always because a high degree expression could be passed in as a single element set, which I guess would be a valid use of the function as it could not be done more efficiently with a lookup directly? What do you think?

[han0110]

The expressions that are passed in could be a higher degree already so perhaps better to add a check on the degree of the final expression that is built

Ah right, but actually we can peek Expression's degree by its function degree, so maybe we should assert it is less than max_degree in add_expression or some internal function. Then with panic trace we still know which line of code leads to degree too high.

Although even that may not really work as expected always because a high degree expression could be passed in as a single element set, which I guess would be a valid use of the function as it could not be done more efficiently with a lookup directly?

We would definitely want a max_degree on all expressions in the end, cause once the degree exceed next power of 2, then the circuit's capacity also halved. If some verification just requires such high degree, we could probably just add some extra intermediate witness to make it fit in the max_degree. So ConstraintBuilder should always guard the max_degree in my opinion.

[Brechtpd]

The expressions that are passed in could be a higher degree already so perhaps better to add a check on the degree of the final expression that is built

Ah right, but actually we can peek Expression's degree by its function degree, so maybe we should assert it is less than max_degree in add_expression or some internal function. Then with panic trace we still know which line of code leads to degree too high.

Should we do < max_degree or <= max_degree? I did <= max_degree because for some reason that makes more sense to me but I don't know why so let me know what makes the most sense to you. :)

We would definitely want a max_degree on all expressions in the end, cause once the degree exceed next power of 2, then the circuit's capacity also halved. If some verification just requires such high degree, we could probably just add some extra intermediate witness to make it fit in the max_degree. So ConstraintBuilder should always guard the max_degree in my opinion.

I'm assuming we should also guard the degree of expressions used in lookups in the same way? Added those checks but not sure if needed (or these may even need a bit stricter requirements because they may increase in degree in the end, I don't really know how these actually end up being used).

[han0110]

<= max_degree makes sense to me :)

I'm assuming we should also guard the degree of expressions used in lookups in the same way.

For sure, cause the lookup currently is composed in a strange way, which might have a high degree of input. I think we probably want a simpler lookup in the future... For example a selector with some advice columns, and when one wants to enable the lookup, it constrain the input in these columns, and also the selector to be enabled. It seems a more reasonable way to construct lookup (we make use of it on much more rows), and then we don't need to worry about lookup's degree anymore.

Regarding to lookup's degree, it also has a required_degree depends on input and table. So if to check lookup's degree, it would be like assert!(expr.degree() + 3 <= self.max_degree) if we assume table has max degree 1 (we might gain extra 1 degree if we disable the zk feature).

BTW, IIUC all of gate or arguments' required degree would be subtracted by one due to divide_by_vanishing_poly, so our DEFAULT_MAX_DEGREE could be 2**n + 1 like 9.

[Brechtpd]

Alright, thanks for the info and help! I think I have added these checks correctly now.

[ChihChengLiang]

Instead of leaving dead codes here, can we create an issue and put this code snippet there?

[Brechtpd]

Marked the exhaustive test as ignored so it doesn't run by default, but all ignored tests can be run when doing cargo test -- --ignored. Does that sound good to you? No more dead code and the test code is still nicely available next to the other code, and all future slow tests can do the same so there's just one command that can be run to all test these as well.

[ChihChengLiang]

@Brechtpd Yes, that sounds great!

[ChihChengLiang]

Great work @Brechtpd! I also checked with @miha-stopar and @han0110, they are all good with the updates on feedbacks.