-
Notifications
You must be signed in to change notification settings - Fork 279
Dory: add layout enum + AddressMajor support #1197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Introduces DoryLayout enum for row-major and column-major coefficient mapping. Includes global state management and updated commitment logic. Co-authored-by: quang.dao <[email protected]>
Co-authored-by: quang.dao <[email protected]>
Update test parameters to match and add E2E tests for Dory layouts. Co-authored-by: quang.dao <[email protected]>
This commit introduces the AddressMajor layout for Dory, enabling more efficient polynomial commitments. It includes updates to OneHotPolynomial, RLCPolynomial, and DoryGlobals to handle the new layout. The verifier is also updated to initialize DoryGlobals with the new layout. Co-authored-by: quang.dao <[email protected]>
This commit completes the AddressMajor layout implementation by updating the core algorithms in OneHotPolynomial and RLCPolynomial: - Updated OneHotPolynomial::num_rows() to be layout-aware - Implemented commit_rows_address_major() with cycle-based row indexing - Implemented vector_matrix_product_address_major() for correct VMV - Updated RLCPolynomial to handle AddressMajor for dense polynomials - Added get_dimension() and get_cycles_per_row() helper methods - Fixed test num_vars consistency to avoid generator cache conflicts The two layouts differ in how coefficients are organized: - CycleMajor: index = address * T + cycle (groups by address) - AddressMajor: index = cycle * K + address (groups by cycle)
moodlezoup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in https://github.com/a16z/jolt/pull/996/files#diff-bcd74093e182e3cebb7ddcc4f3c4c251928a4c4a53615f8c42be8f815b7a22df, I had to make changes to the opening point passed to Dory in order to support address-major matrix layout. Why do we no longer need those changes?
| // Parameters are computed from trace length as needed | ||
|
|
||
| // Initialize Dory globals for the verifier | ||
| // The verifier needs these to compute commitments during verification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is a bit misleading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to do this anyways, so I'm deleting it.
| /// Test that dense polynomials produce the same commitment regardless of layout. | ||
| /// | ||
| /// The CycleMajor vs AddressMajor distinction only affects OneHot polynomials. | ||
| /// For dense polynomials, the matrix storage is always row-major. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is true, I don't understand why this test is passing. Dense polynomials are treated as having K=1
| }) | ||
| .collect(), | ||
| MultilinearPolynomial::LargeScalars(poly) => { | ||
| if address_major && poly.Z.len() == t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the len == t checks throughout this file are unnecessary? All non-one-hot polynomials should be length T
| // AddressMajor embedding for cycle-only dense polynomials (len == T): | ||
| // use bases at stride K (col = cycle_offset * K) and chunk coefficients by cycles_per_row. | ||
| macro_rules! compute_dense_address_major_msm { | ||
| ($coeffs:expr, $msm_method:ident) => {{ | ||
| let t = DoryGlobals::get_T(); | ||
| if DoryGlobals::get_layout() == DoryLayout::AddressMajor && $coeffs.len() == t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment and name for this macro seem to be inaccurate, as it handles the CycleMajor case in the else block. can we just fold this into the existing compute_msm! macro?
| // This batching helper was originally written for CycleMajor and is not currently | ||
| // performance-critical. For AddressMajor, fall back to per-polynomial commits to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're not using this function anywhere anymore, we can just delete it
| result | ||
| } | ||
|
|
||
| /// AddressMajor lazy VMV over lazy trace iterator (experimental, re-runs tracer). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that we don't plan to use address-major on CPU (except for testing), I prefer to remove all the streaming-related codepaths. Wdyt?
Summary
This PR introduces an explicit Dory matrix layout selector and adds an AddressMajor layout implementation, while preserving existing CycleMajor behavior as the default.
Motivation
We need to support an alternate OneHot/Dory layout used by other implementations without silently changing existing proof behavior.
What’s included
DoryLayoutenum (CycleMajor,AddressMajor) + globalDoryGlobals::{get_layout,set_layout}.commit_rows)vector_matrix_product)evaluate)RLCPolynomialfor AddressMajor (CycleMajor preserved).a16z/jolt:main.Layout definition
Let
K= number of addresses,T= number of cycles.CycleMajor (current default):
idx = address * T + cyclenum_columns.AddressMajor:
idx = cycle * K + addressnum_columns.How to enable AddressMajor
Call before any Dory initialization / prover work:
Testing
cargo test -p jolt-core --lib -- dory_layout --nocapturecargo test -p jolt-core --lib -- zkvm::prover::tests::sha3_e2e_dory --exact --nocapture