GKR backend for LogUp-GKR

[Al-Kindi-0]

Builds on #295 and implements the GKR backend for the LogUp-GKR argument.

[plafer]

not a full review yet

[plafer]

I don't think it is necessary to store the oracles here

[Al-Kindi-0]

I think it is needed for the s-column, which is done in another PR building on this one

[plafer]

Suggested change

	pub fn prove_gkr<E: FieldElement>(
	pub fn prove_logup_gkr<E: FieldElement>(

I wonder if we should stick with logup_gkr instead of gkr everywhere. While it's clear to us that "gkr" stands for "gkr over the LogUp circuit", I'm not sure it would be so clear to someone first reading the codebase

[Al-Kindi-0]

I see your point. On the other hand, some of the structs (e.g., GkrCircuitProof) as well many of the comments fit better with the current name.
No strong preference though

[plafer]

Couldn't we merge build_query() and evaluate_query() into a single evaluate_fractions(), which internally builds & evaluates the query? I would see it as simplifying the interface by having one less method, as well as removing the risk that get_oracles() and build_query() be inconsistent.

[Al-Kindi-0]

build_query is useful for building the input layer and the OOD evaluation. For the first one we can merge the two but for the second one the evaluate_query logic is not needed.
One the other hand, evaluate_query is used without the build_query logic in the high degree sum-check.

[plafer]

Good point, I missed that.

[plafer]

(I can't comment on sum_check_prove_higher_degree() since it wasn't modified in this file, so commenting here instead).

Two equations in the docstring don't render properly (I believe due to a bug in Katex). The workaround is to write \\B instead of \mathbb{B}:

/// $$
/// p_{n-1}\left(v_2, \cdots, v_{\mu}, x_1, \cdots, x_{\nu}\right) =
/// \sum_{y\in\\B_{\nu}} G(y_{1}, ..., y_{\nu})
/// $$
///
/// and
///
/// $$
/// q_{n-1}\left(v_2, \cdots, v_{\mu}, x_1, \cdots, x_{\nu}\right) =
/// \sum_{y\in\\B_{\nu}} H\left(y_1, \cdots, y_{\nu} \right)
/// $$

and update /.cargo/katex-header.html with a new macro:

            macros: {
                "\\B": "\\mathbb{B}",
            },

[Al-Kindi-0]

Fixed, thank you!

[plafer]

I think that we'll run into an issue when our oracles are the current row and next row. In this case, at the last row, we don't want to call build_query() on the evaluation frame, since the frame contains current_row: <last row> and next_row: <first row>.

Ultimately we want numerators equal to 0 (and whatever non-zero denominators) to be inserted for that last row, and somehow also encode this information in the GkrOracles. Have you thought about how we could fix this already?

[Al-Kindi-0]

My thinking was that, at the "application" level, the selectors will be designed so as to be zero for such edge cases. Thus I didn't give much thought to a choice we can make with respect to NextRow oracles, namely either we choose a circular shift, as we do now, or we use a plain shift where we will get zero at the last row as a result.

[plafer]

namely either we choose a circular shift, as we do now, or we use a plain shift where we will get zero at the last row as a result.

Right, we currently do a circular shift. But we actually want a plain shift, and I don't see how this would be possible with the current API. For example, when x represents the last row, how does evaluate_query() know that ultimately it should output 0s for the numerators and 1s for the denominator (since all it gets is the evaluations of the oracles at x)?

I'm currently not seeing it, but if you're confident that it's possible, then we can move on.

[Al-Kindi-0]

For example, the numerators could be an expression involving selectors which are zero once a HALT instruction has been initiated. For this row, and all subsequent rows assuming that a HALT can only be followed by a HALT, the numerators will be zero. The denominators are not important as far as I can see, of course as long as they are not zero, and this is guaranteed with high probability by the LogUp randomness.
I might be missing something extra but we can, as you suggested, fix it in a subsequent targeted PR.

[plafer]

We should export a DummyLogUpGkrEval<BaseField, PublicInputs> can be used when LogUp-GKR is disabled (instead of reimplementing it everytime)

[Al-Kindi-0]

Good idea!
Implemented and exported

[plafer]

LGTM!

I'm still not sure if the LogUpGkrEvaluator API is flexible enough to properly support the use of the NextRow GkrOracle, but if it isn't we can always fix it in a subsequent PR

[irakliyk]

Thank you! Looks good! Not a full review yet - but I left some comments/questions inline.

[irakliyk]

Looks good! Thank you! Still not a full review (though I think I covered 80% of the code by now) - but I left some more comments inline.

[irakliyk]

Looks good! Thank you! I left some more comments inline. Most of them are pretty minor and we can address them either here or in the subsequent PRs.

[irakliyk]

I wonder if this should also be an error rather than expect(). With expect(), I think a maliciously crafted proof could crush the verifier, right?

[Al-Kindi-0]

If the deserialization fails then this should return an error but probably you are thinking about something else