Skip to content
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

feat: higher FRI arity #2

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

OsamaAlkhodairy
Copy link

No description provided.

@OsamaAlkhodairy OsamaAlkhodairy changed the title Feat: higher FRI arity feat: higher FRI arity Dec 20, 2024
fri/src/prover.rs Outdated Show resolved Hide resolved
cur_folded_len /= 2;
}

let folded_matrix = RowMajorMatrix::new(folded.clone(), arity);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be best to avoid this folded.clone() since that's an entire matrix. below, can you use leaves in place of folded? it would make the code a little more readable too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it actually makes the code uglier since leaves gives you only a reference, so the first fold will be a bit special. I'll also need to figure out how to pass that matrix reference to g.fold_matrix but with the width changed (to 2). do you know how to do that?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least for now, DenseMatrix has pub width so you could just do folded_matrix.width = 2 since the underlying storage is row major vector.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaves only give us an immutable reference

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noting we're leaving for now, but could probably use .as_view()

fri/src/verifier.rs Outdated Show resolved Hide resolved
fri/src/verifier.rs Outdated Show resolved Hide resolved
fri/src/verifier.rs Outdated Show resolved Hide resolved
fri/src/verifier.rs Outdated Show resolved Hide resolved
Copy link

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me but:

  1. there is an assumptions that two matrices don't have the same height, which is not necessarily true
  2. avoid clone of matrices
  3. add example in the code

One last thing I'm not sure about: if arity is 4 and we start with max height of 8, what happens? it will fold down to 2 and then stop? do we need the final poly height to equal 2?

fri/src/prover.rs Outdated Show resolved Hide resolved
Copy link

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants