o1vm/mips: use batch_inversion for the witness generation #2813
Conversation
dannywillems
changed the title
dannywillems
force-pushed
the
o1vm/batch-inversion
branch
from
00087d6
to
86aa2a5
Compare
dannywillems
changed the base branch from
master
to
o1vm/mips/interpreter-remove-inverse-or-zero
dannywillems
force-pushed
the
o1vm/batch-inversion
branch
2 times, most recently
from
f384222
to
e6169b1
Compare
dannywillems
force-pushed
the
o1vm/batch-inversion
branch
2 times, most recently
from
799172e
to
b45339e
Compare
dannywillems
force-pushed
the
o1vm/batch-inversion
branch
from
b45339e
to
30c9354
Compare
dannywillems
force-pushed
the
o1vm/batch-inversion
branch
2 times, most recently
from
4122619
to
8be102a
Compare
| @@ -121,3 +125,31 @@ fn test_small_circuit() { | |||
| ); | |||
| assert!(verif, "Verification fails"); | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
Feel free to disagree with these changes. It is a single commit we can simply delete. I only want to keep a regression test in case arkworks changes.
There was a problem hiding this comment.
These are good to visualize the behavior when zeroes are involved.
| @@ -50,10 +50,17 @@ pub const NUM_INSTRUCTION_LOOKUP_TERMS: usize = 5; | |||
| pub const NUM_LOOKUP_TERMS: usize = | |||
| NUM_GLOBAL_LOOKUP_TERMS + NUM_DECODING_LOOKUP_TERMS + NUM_INSTRUCTION_LOOKUP_TERMS; | |||
| // TODO: Delete and use a vector instead | |||
| // FIXME: since the introduction of the scratch size inverse, the value below | |||
There was a problem hiding this comment.
This is fixed in #2815.
| scratch_state: [Fp::from(0); SCRATCH_SIZE], | ||
| scratch_state_inverse: [Fp::from(0); SCRATCH_SIZE_INVERSE], |
There was a problem hiding this comment.
In a future PR, as suggested by @mrmr1993, we can try to use BigInt/BigUInt directly to avoid computation in Montgomery representation. I do not know if it would make it better.
| // MIPS + hash_counter + byte_counter + eof + num_bytes_read + chunk + bytes | ||
| // + length + has_n_bytes + chunk_bytes + preimage | ||
| pub const SCRATCH_SIZE: usize = 98; | ||
|
|
||
| /// Number of columns used by the MIPS interpreter to keep values to be |
There was a problem hiding this comment.
To verify it was the minimal value, I did try to use 11, and it failed.
From there, I run for some millions instructions the op-program, and it never failed.
dannywillems
force-pushed
the
o1vm/batch-inversion
branch
from
8be102a
to
4d82c40
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2813 +/- ##
==========================================
+ Coverage 72.10% 72.15% +0.04%
==========================================
Files 256 256
Lines 59946 60042 +96
==========================================
+ Hits 43227 43326 +99
+ Misses 16719 16716 -3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
These fields will be used to keep track of the elements that should be inverted while generating the proof.
Checking as a regression tests that the batch inversion works with zeroes
dannywillems
force-pushed
the
o1vm/batch-inversion
branch
from
4d82c40
to
a11a350
Compare
| @@ -106,6 +115,12 @@ impl<Fp: Field, PreImageOracle: PreImageOracleT> InterpreterEnv for Env<Fp, PreI | |||
| Column::ScratchState(scratch_idx) | |||
| } | |||
|
|
|||
| fn alloc_scratch_inverse(&mut self) -> Self::Position { | |||
There was a problem hiding this comment.
It seems like this function for the mips witness is the same as the one in the mips constraints. I wonder if it could be generically defined in the interpreter itself, but probably not directly given that it uses the fact that it accesses a self.scratch_state_idx_inverse. Maybe environments can be defined to implement a trait that returns it, but not sure if it is worth the code duplication.
There was a problem hiding this comment.
I do think it is fine at the moment. Agree it is a bit "annoying"/"ugly" to have duplicated code. But lgtm for the moment.
| let inv_or_zero = if *x == 0 { | ||
| Fp::zero() | ||
| let pos = self.alloc_scratch_inverse(); | ||
| if *x == 0 { |
There was a problem hiding this comment.
I am not sure why there's this if-else distinction if it always writes in the inverse scratch state. Is it to avoid a conversion in case Fp::zero() is faster than Fp:from(0)? Either way I suppose the batch inversion algorithm will just ignore zeros, I believe I checked that in the arkworks code.
There was a problem hiding this comment.
No reason. Seems to be an ugly code 😅. For a follow-up.
| @@ -36,8 +39,9 @@ pub struct ColumnEnvironment<'a, F: FftField> { | |||
| } | |||
|
|
|||
| pub fn get_all_columns() -> Vec<Column> { | |||
| let mut cols = Vec::<Column>::with_capacity(SCRATCH_SIZE + 2 + N_MIPS_SEL_COLS); | |||
| for i in 0..SCRATCH_SIZE + 2 { | |||
| let mut cols = | |||
There was a problem hiding this comment.
Perhaps use N_MIPS_REL_COLS, or you want to be super explicit here?
There was a problem hiding this comment.
No, you are right about using N_MIPS_REL_COLS.
| @@ -121,3 +125,31 @@ fn test_small_circuit() { | |||
| ); | |||
| assert!(verif, "Verification fails"); | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
These are good to visualize the behavior when zeroes are involved.
These fields will be used to keep track of the elements that should be inverted while generating the proof.
Speed from ~250kHz (commit 82464fb) to ~420kHz (head of this branch, i.e. o1vm/batch-inversion).
In a follow-up PR, we should take a look at mrmr1993 idea here to get the same expected performances. Also, we should decrease the number of columns in scratch_state as stated in the FIXME.
On commit 82464fb, getting:
On the head of this branch: