Update and sync-up the recursive verifier #1575
Conversation
fix: cleanup and restore security parameters chore: lower num of fri queries and small edits chore: cleanup chore: cleanup
Al-Kindi-0
force-pushed
the
al-update-recursive-verifier
branch
from
7088eb4
to
2d04c4c
Compare
|
Is this ready for review? Or still a draft for now? |
|
Yes, it is ready for review |
processor/src/operations/comb_ops.rs
Outdated
| @@ -75,7 +75,7 @@ where | |||
| // --- compute the updated accumulator values --------------------------------------------- | |||
| let v0 = self.stack.get(7); | |||
| let tx = QuadFelt::new(v0, ZERO); | |||
| let [p_new, r_new] = [p + alpha * (tx - tz), r + alpha * (tx - tgz)]; | |||
| let [r_new, p_new] = [r + alpha * (tx - tz), p + alpha * (tx - tgz)]; | |||
There was a problem hiding this comment.
nit: can we swap the order to [p_new, r_new] so that it's consistent with line 73 above?
I think the ideal would be to put r before p on the stack (so that the order is always consistent), but just this small swap is probably fine
processor/src/operations/comb_ops.rs
Outdated
| r.to_base_elements()[0], | ||
| r.to_base_elements()[1], | ||
| p.to_base_elements()[0], | ||
| p.to_base_elements()[1], | ||
| r.to_base_elements()[0], | ||
| r.to_base_elements()[1], |
There was a problem hiding this comment.
Lines 334 and 335 are still incorrect (i.e. p should use tgz and r should use tz). Then I think the diff here should be reverted (since the stack is being built from bottom to top, so r comes before p)
| let proof_context = proof.context.to_elements(); | ||
| let trace_len: Felt = proof_context[1]; | ||
| let num_queries = proof_context[7]; | ||
| let blowup = proof_context[6]; | ||
| let grinding_factor = proof_context[5]; |
There was a problem hiding this comment.
Why do we call to_elements() and then access the serialized buffer? Could we not access the Context fields directly? This is a bit painful to read, as I have to go understand how to_elements() works to make sure that this is correct. And if to_elements() changes, then this test will start to fail.
| (trace_len.as_int() as usize).ilog2() as u64, | ||
| ]; | ||
|
|
||
| // build a seed for the public coin; the initial seed is the hash of public inputs and proof | ||
| // context, but as the protocol progresses, the coin will be reseeded with the info received | ||
| // from the prover | ||
| let mut tape = vec![]; |
There was a problem hiding this comment.
nit: can we rename this to advice_stack? (also in VerifierData)
| const.OOD_TRACE_PTR=4294900000 | ||
| const.OOD_CONSTRAINT_EVALS_PTR=4294900081 | ||
| const.OOD_CONSTRAINT_EVALS_PTR=4294900077 |
There was a problem hiding this comment.
Is there an assumption in the code that OOD_TRACE_PTR is right next to OOD_CONSTRAINT_EVALS_PTR? If not, we could put them far away enough such that we don't need to come fix this value everytime we make a change to the shape of the trace.
Similar comment applies to all constants in this file
| drop | ||
| #=> [trace_length, num_queries, blowup, grinding] | ||
|
|
||
| # Construct the proof context | ||
|
|
||
| ##trace layout info | ||
| push.1208027408 | ||
|
|
||
| push.1174472464 |
There was a problem hiding this comment.
Can we use constants for these magic numbers, and a bit of docs? I'm guessing that we're pushing a value related to Context.trace_info, but I don't understand how 1174472464 encodes the entire TraceInfo?
| export.generate_constraint_composition_coefficients | ||
|
|
||
| push.236 | ||
| push.224 |
There was a problem hiding this comment.
Similarly, can we use a constant for this?
|
|
||
| push.92 | ||
| # note that 88 is the next number after 85 divisible by 4 | ||
| push.88 |
There was a problem hiding this comment.
Same as above. It makes it easier to go back and adjust these parameters later (instead of finding them in the code)
As the title says.
We now use the
rcomb_baseand this leads to faster testing times. Also updated is how thercomb_baseis defined so that the accumulator for the(x - z)quotient is deeper in the stack than the accumulator for the(x - gz)quotient.Other than that, there are some minor cleanups and re-organization for the testing code.