adding comments for clarifying accum #313
Conversation
risc0/zkp/src/prove/accum.rs
Outdated
| @@ -21,23 +21,29 @@ use crate::{ | |||
| field::{Elem, ExtElem, Field}, | |||
| }; | |||
|
|
|||
| /// Tracks plonk accumulations for an execution. | |||
| /// Tracks grand product accumulations for PLOOKUP. | |||
There was a problem hiding this comment.
My understanding was that we are using the PLONK permutation argument to verify that memory is valid. In this section of the code, I'm not sure PLOOKUP applies. I think PLOOKUP is used in the circuit to avoid having to split every bit of a byte or word into an individual column.
There was a problem hiding this comment.
I'm not sure either -- @jbruestle can you clarify whether Accum is just used for the memory-based grand product accumulation or whether this struct is also involved in the bytes-based accumulation?
There was a problem hiding this comment.
The answer to the TODO about kinds will be clarifying on this front.
There was a problem hiding this comment.
So both are permutation arguments, but and accum is used for both, PLOOKUP is too specific. Technically, since at least in our case PLOOKUP is a specialization of PLONK, I'd either call it PLONK, or just a permutation argument.
risc0/zkp/src/prove/accum.rs
Outdated
| @@ -21,23 +21,29 @@ use crate::{ | |||
| field::{Elem, ExtElem, Field}, | |||
| }; | |||
|
|
|||
| /// Tracks plonk accumulations for an execution. | |||
| /// Tracks grand product accumulations for PLOOKUP. | |||
There was a problem hiding this comment.
So both are permutation arguments, but and accum is used for both, PLOOKUP is too specific. Technically, since at least in our case PLOOKUP is a specialization of PLONK, I'd either call it PLONK, or just a permutation argument.
risc0/zkp/src/prove/accum.rs
Outdated
| @@ -57,6 +60,7 @@ impl<E: Elem> Accum<E> { | |||
| } | |||
|
|
|||
| pub struct Handler<'a, F: Field> { | |||
| // TODO what is p? | |||
There was a problem hiding this comment.
@jbruestle one TODO remaining -- can you clarify the role of p in the Handler?
There was a problem hiding this comment.
@shkoo Do you know what p refers to in this context?
There was a problem hiding this comment.
p is a mutex lock (an implementation detail so it can be used in multiple threads and as a callback) containing a "Accum" structure, that's commented as "Tracks grand product accumulations for PLONK-style permutation arguments." on line 24
There was a problem hiding this comment.
Thanks for clarifying, @shkoo
I've edited the TODO to read, "p is a mutex lock that contains an Accum structure;" lmk if you'd revise that.
|
All TODOs have been taken care of -- thanks for the help, everyone! Will land soon (Tuesday AM?) unless anyone has any last edits to suggest. |
No description provided.