LogUp-GKR integration into VM #1364
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
refactor: serialize usize with write_usize (#1266)
* change prover * change verifier * cleanup * Remove `VirtualBusVerifier` * Remove `VirtualBusProver` * reorganize exports * `LayerGatesInputs` * use constant * docstrings * add TODO * remove TODO * clippy * add static_assertions * add padding comment * add back `claim` in verifier
plafer
commented
Jul 8, 2024
There was a problem hiding this comment.
Initial implementation ready for review! I ran the BLAKE3 benchmarks:
nextbranch:- prover time: 42s
- verifier time: 1.7ms
- current branch:
- prover time: 68s
- verifier time: 3ms
Upon closer inspection,
- our implementation's constraint evaluation is 12 seconds slower - we could expect parallelization to shrink that down close to 0.
- Also notably, we are currently saving 1 second in auxiliary trace commitment with, but I believe this is noise, since we actually have one extra column currently (we removed 1, but added 2). Hopefully when we go from 7 to 2, we can see an actual speedup.
Find below the full benchmark output. The command ran for both branches was:
$ make exec-info
$ MIDEN_LOG=info RAYON_NUM_THREADS=16 ./target/optimized/miden example blake3 -n 200next branch output
============================================================
INFO assemble_with_opts_in_context [ 6.25ms | 100.00% ]
Generated a program to compute 200-th iteration of BLAKE3 1-to-1 hash; expected result: [3273414978, 861395390, 3406933114, 1249690799, 3478855123, 2478635211, 4040123218, 3120808209]
--------------------------------
INFO prove_program [ 42.2s | 4.46% / 100.00% ]
INFO ┝━ execute_program [ 641ms | 1.52% ]
INFO ┝━ ｉ [info]: Generated execution trace of 70 columns and 2097152 steps (49% padded) in 640 ms
INFO ┝━ build_domain [ 21.3ms | 0.05% ] trace_length: 2097152 | lde_domain_size: 16777216
INFO ┝━ commit_to_main_trace_segment [ 20.0s | 0.00% / 47.44% ]
INFO │ ┝━ extend_execution_trace [ 16.6s | 39.36% ] num_cols: 70 | blowup: 8
INFO │ ┕━ compute_execution_trace_commitment [ 3.41s | 8.08% ] tree_depth: 24
INFO ┝━ commit_to_aux_trace_segment [ 3.64s | 0.00% / 8.62% ]
INFO │ ┝━ extend_execution_trace [ 2.95s | 6.98% ] num_cols: 7 | blowup: 8
INFO │ ┕━ compute_execution_trace_commitment [ 692ms | 1.64% ] tree_depth: 24
INFO ┝━ evaluate_constraints [ 11.0s | 25.93% ] ce_domain_size: 16777216
INFO ┝━ commit_to_constraint_evaluations [ 3.28s | 0.00% / 7.75% ]
INFO │ ┝━ build_composition_poly_columns [ 275ms | 0.65% ] num_columns: 8
INFO │ ┝━ evaluate_composition_poly_columns [ 2.30s | 5.44% ]
INFO │ ┕━ compute_constraint_evaluation_commitment [ 702ms | 1.66% ] tree_depth: 24
INFO ┝━ build_deep_composition_poly [ 1.32s | 3.12% ]
INFO ┝━ evaluate_deep_composition_poly [ 133ms | 0.32% ]
INFO ┝━ compute_fri_layers [ 163ms | 0.39% ] num_layers: 5
INFO ┝━ determine_query_positions [ 1.22ms | 0.00% ] grinding_factor: 16 | num_positions: 27
INFO ┕━ build_proof_object [ 173ms | 0.41% ]
--------------------------------
Executed program in 42248 ms
Stack outputs: [3273414978, 861395390, 3406933114, 1249690799, 3478855123, 2478635211, 4040123218, 3120808209]
Execution proof size: 107 KB
Execution proof security: 96 bits
--------------------------------
INFO verify_program [ 1.70ms | 100.00% ]
Execution verified in 1 ms
current branch output
============================================================
INFO assemble_with_opts_in_context [ 6.03ms | 100.00% ]
Generated a program to compute 200-th iteration of BLAKE3 1-to-1 hash; expected result: [3273414978, 861395390, 3406933114, 1249690799, 3478855123, 2478635211, 4040123218, 3120808209]
--------------------------------
INFO prove_program [ 68.5s | 21.03% / 100.00% ]
INFO ┝━ execute_program [ 638ms | 0.93% ]
INFO ┝━ ｉ [info]: Generated execution trace of 70 columns and 2097152 steps (49% padded) in 638 ms
INFO ┝━ build_domain [ 20.5ms | 0.03% ] trace_length: 2097152 | lde_domain_size: 16777216
INFO ┝━ commit_to_main_trace_segment [ 21.3s | 0.00% / 31.06% ]
INFO │ ┝━ extend_execution_trace [ 17.0s | 24.77% ] num_cols: 70 | blowup: 8
INFO │ ┕━ compute_execution_trace_commitment [ 4.32s | 6.30% ] tree_depth: 24
INFO ┝━ commit_to_aux_trace_segment [ 2.38s | 0.00% / 3.47% ]
INFO │ ┝━ extend_execution_trace [ 1.77s | 2.59% ] num_cols: 8 | blowup: 8
INFO │ ┕━ compute_execution_trace_commitment [ 604ms | 0.88% ] tree_depth: 24
INFO ┝━ evaluate_constraints [ 23.7s | 34.53% ] ce_domain_size: 16777216
INFO ┝━ commit_to_constraint_evaluations [ 3.32s | 0.00% / 4.84% ]
INFO │ ┝━ build_composition_poly_columns [ 241ms | 0.35% ] num_columns: 8
INFO │ ┝━ evaluate_composition_poly_columns [ 2.31s | 3.38% ]
INFO │ ┕━ compute_constraint_evaluation_commitment [ 762ms | 1.11% ] tree_depth: 24
INFO ┝━ build_deep_composition_poly [ 2.32s | 3.39% ]
INFO ┝━ evaluate_deep_composition_poly [ 142ms | 0.21% ]
INFO ┝━ compute_fri_layers [ 168ms | 0.24% ] num_layers: 5
INFO ┝━ determine_query_positions [ 246µs | 0.00% ] grinding_factor: 16 | num_positions: 27
INFO ┕━ build_proof_object [ 188ms | 0.27% ]
--------------------------------
Executed program in 68527 ms
Stack outputs: [3273414978, 861395390, 3406933114, 1249690799, 3478855123, 2478635211, 4040123218, 3120808209]
Execution proof size: 128 KB
Execution proof security: 96 bits
--------------------------------
INFO verify_program [ 3.54ms | 100.00% ]
Execution verified in 3 ms
| pub use gkr_verifier::{ | ||
| verify_virtual_bus, CompositionPolyQueryBuilder, SumCheckVerifier, SumCheckVerifierError, | ||
| }; | ||
| pub mod logup_gkr; |
There was a problem hiding this comment.
Leaving this as-is, since we'll be moving it out of the VM in the next PR.
Comment on lines
+31
to
+37
| // Hack: These columns already have an assertion | ||
| CLK_COL_IDX | ||
| | FMP_COL_IDX | ||
| | STACK_TRACE_OFFSET..=STACK_TRACE_END | ||
| | B0_COL_IDX_MAIN_TRACE | ||
| | B1_COL_IDX_MAIN_TRACE | ||
| | V_COL_IDX => (), |
There was a problem hiding this comment.
How we deal with the main_trace_first_row's assertions currently is hacky. The main issue before this hack was that winterfell would panic because of overlapping assertions (i.e. this assertion being redundant with other ones set by other modules).
The real underlying issue is that all the columns listed here shouldn't actually be part of the public inputs. But we'll need to fix it properly when we move this into winterfell, so I left it as is for now.
Comment on lines
+51
to
+52
| let final_opening_claim = | ||
| verify_virtual_bus(E::ZERO, gkr_proof, vec![log_up_randomness], public_coin)?; |
There was a problem hiding this comment.
Might be missing a check after this; will double-check in the morning.
There was a problem hiding this comment.
I think all the necessary checks are there, but @Al-Kindi-0 please double-check
|
Not a review yet - just a few observations about the performance because I think something looks odd. First, the times are quite a bit higher than what I'm getting on my machine. Specifically, I'm getting 16 seconds for the Also, I usually run the benchmarks for 1M cycles. Previously, this is what we were getting for
|
| Step | next |
this |
|---|---|---|
| execute program | 0.35 | 0.33 |
| commit main trace | 2.9 | 2.9 |
| commit aux trace | 0.76 | 0.73 |
| evaluate constraints | 2.28 | 7.02 |
| commit to constraint evaluations | 0.77 | 0.76 |
| everything else | 0.66 | 1.51 |
| unaccounted | 0.34 | 5.65 |
| total | 8.06 | 18.9 |
So, the delta between the next and this branch is wholly explained by: (1) the increase in constraint evaluation step which is now taking 4.8 extra seconds, and (2) some unaccounted steps which now take an extra 5.3 seconds.
My hypothesis is that there are 2 "unaccounted" steps: building of the auxiliary trace and generating GKR proof. We should probably update Winterfell to log these steps explicitly. Also, would be nice to add "sub-items" to the constraint evaluation step logging so that we can see Lagrange kernel constraint evaluation time (and maybe something else?) explicitly as well.
|
Added instrumentation for
|
|
I re-ran the benchmark with the latest changed to Winterfell included (and also added a couple more instrumentations inside the GKR prover), and now everything seems to be accounted for. It seems like building the the GKR proof adds pretty much as much extra time as constraint evaluation and accounts for almost 5.6 seconds. Here is the breakdown: |
|
Closing in favor of #1493 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Built on top of #1307
Integrates LogUp GKR implementation into the VM.