refactor(eval): new design for `trait Eval`

[cyphersnake]

The current trait Eval design looks like this:

pub trait Eval<F: PrimeField, POLY>: GetDataForEval<F> {
    fn eval(&self, poly: &POLY, row_index: usize) -> Result<F, Error>;
}

We have:

• What we are going to evaluate the data for. Earlier it was represented as MultiPolynomial, now it will be represented as Expression (which we take from GroupedPoly). Let's call it Poly.

• Where we are going to get the data for calculation. In #159, a separate trait GetDataForEval was created. Let's call it Data.

• There is also a type that implements the logic of calculation, such as GraphEvaluator. It stores intermediate data for calculation. Let's call it Evaluator.

Usage

• In the cross-term computation, we use rayon to evaluate all rows in parallel and stack them inside the collection
• Same in lookup module
• As part of the IVC correctness check, we compute specific row and compare it with expected value

Problem 1

For some reason, trait Eval is implemented for Data, although it is not implied that there can be a different type of calculation for different types of Data, since the logic (both old and new) does not depend on the way the data is fetched, but on the algorithm.

Thus trait does not fulfill the task of isolating the computation algorithm, but only complicates the structure of generics. Such a design will not allow us, for example, to replace the current algorithm on the fly with another `Evalutor' and compare the efficiency.

Problem 2

The current design, when Evalutor instance is local, within a single row computation (inside the eval fn), we cannot overuse the cache (the computation tree inside GraphEvalutor), so current versions of the PR code do NOT use trait at all to compute cross-term, because otherwise we have to call GraphEvaluator::new on every line and this will cause overhead, given the large number of lines.

Decision

Refactoring

trait in this case should perform the task of isolating a particular eval algorithm, so should be implemented for Evaluator rather than Data, but that would cause a huge number of generics throughout our codebase (I tried it, it came out verbose).

Refactoring and adding fn new and a separate implementation of fn eval_all_row that will take over the creation of Evaluator and reusing it will make the trait Eval design meaningful, however, verbose.

In that case we should add generics on the PlonkStructure & Argument (and all related structs) itself or its methods with no obvious benefit at this stage, due to no plans yet to even implement another algorithm evalution of expression for now.

However, the latter approach will allow us to change Evaluator implementations on the fly in the future and make measurements on one version of the code, which is much better than the problems we encountered in the #159 implementation, where we have to make comparisons on different commits.

Alternative

I suggest we take a step back and abandon this isolation at this point, given the lack of plans to implement and support multiple Evalutors. This will cause us to use GraphEvalutor directly in a few places (commit_cross_term + issat*), however, the number of such places is small and we can always revert to this isolation without much refactoring.

Summarize

Options:

1. Remove trait, work with its single instance of the algorithm in five places
2. Refactor trait in such a way that it does not create overhead from its use when calculating all rows, and also it was possible to change Evaluator implementations on the fly
3. Leave it as it is and have a strong overhead on traversing the Expression tree to build the Calculation tree on each of the rows in the table

The latter option seems terrible to me, I'm in favor of the former.

[cyphersnake]

I did PR #245 with the first option, I also want to point out that within the third option, the overhead is also in lookup module, because there is also recreated Evaluator for each line.

[chaosma]

I like the first option as well. It makes everything simpler. data.eval(poly) and poly.eval(data) are essentially same. We only need one of them in this case. Since we already have graph_evaluator, it is natural to remove the old Eval trait.