Logup for o1vm #2757
Logup for o1vm #2757
Conversation
Fizzixnerd
force-pushed
the
fizzixnerd/logup-pickles
branch
from
b285710 to
0c17f3b
Compare
Fizzixnerd
force-pushed
the
fizzixnerd/logup-pickles
branch
from
0c17f3b to
1ec45d7
Compare
| /// All fixed lookup tables values, indexed by their ID | ||
| pub(crate) fixed_tables: BTreeMap<ID, T>, | ||
| pub fixed_tables: BTreeMap<ID, T>, |
There was a problem hiding this comment.
This can be reverted.
| pub scratch: [G; SCRATCH_SIZE], | ||
| pub instruction_counter: G, | ||
| pub error: G, | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
Needed to do some evil clones, will likely need optimizing
There was a problem hiding this comment.
Indeed, let's keep it for a follow-up
|
E2E tests pass! 🎉 |
o1vm/src/pickles/prover.rs
Outdated
| @@ -184,6 +276,26 @@ where | |||
| absorb_commitment(&mut fq_sponge, comm) | |||
| } | |||
|
|
|||
| let lookup_env = if !logups.is_empty() { | |||
There was a problem hiding this comment.
In a follow up I'd remove this if statement, our lookup are never empty
| // FIXME: use a proper Challenge structure | ||
| let challenges = BerkeleyChallenges { | ||
| alpha, | ||
| // No permutation argument for the moment | ||
| // TODO: No permutation argument for the moment |
There was a problem hiding this comment.
Actually it' not a TODO, the RAMLookup arg is sufficient, we do not need a perm arg
| lookup_terms_evals_d8: &lookup_env.lookup_terms_evals_d8, | ||
| lookup_aggregation_evals_d8: &lookup_env.lookup_aggregation_evals_d8, | ||
| lookup_counters_evals_d8: &lookup_env.lookup_counters_evals_d8, | ||
| fixed_tables_evals_d8: &lookup_env.fixed_lookup_tables_evals_d8, |
There was a problem hiding this comment.
For a follow up, this computation, and other related to fixed tables should be done at setup time
| pub struct Proof<G: KimchiCurve, ID: LookupTableID> { | ||
| pub commitments: WitnessColumns<PolyComm<G>, [PolyComm<G>; N_MIPS_SEL_COLS], ID>, | ||
| pub zeta_evaluations: WitnessColumns<G::ScalarField, [G::ScalarField; N_MIPS_SEL_COLS], ID>, | ||
| pub zeta_omega_evaluations: | ||
| WitnessColumns<G::ScalarField, [G::ScalarField; N_MIPS_SEL_COLS], ID>, |
There was a problem hiding this comment.
Should we reorganize the proof to have less fields, by putting all the commitments in one field (which can have more structure itself), and same for evals ?
This might also help in how we order polys in the opening proof
Just a suggestion, no strong opinion on my side
|
Expecting failures. |
| @@ -90,6 +96,8 @@ pub struct Env<Fp, PreImageOracle: PreImageOracleT> { | |||
| pub preimage_key: Option<[u8; 32]>, | |||
| pub keccak_env: Option<KeccakEnv<Fp>>, | |||
| pub hash_counter: u64, | |||
| pub lookup_fixed_tables: FixedTableMap<LookupTableIDs, Fp>, | |||
There was a problem hiding this comment.
As discussed, I would recommend using a fixed size array for efficiency, as we know how many, and which, tables are used in the MIPS interpreter. We focus on efficiency for the witness builder. The reason of using a fixed size array is to mostly use the cache of the CPU and to avoid hitting the memory by using pointers.
| // No-op, constraints only | ||
| // TODO: keep track of multiplicities of fixed tables here as in Keccak? | ||
| fn add_lookup(&mut self, lookup: Lookup<Self::Variable>) { | ||
| if let Some(idx) = LookupTable::is_in_table(&lookup.table_id, lookup.value) { |
There was a problem hiding this comment.
I would recommend using an "assert" as we know exactly which table is used. We don't need this way to do a lookup in a table.
No description provided.