BaseFold: all open functions accept Arc instead of DenseMultilinearExtension
#563
Conversation
matthiasgoergens
left a comment
matthiasgoergens
left a comment
There was a problem hiding this comment.
The unification looks like a reasonable thing to do.
Though I wonder if it's possible to break the PR into even more manageable chunks? (It's good you broke this one out at all, it's already pretty beefy on its own!)
mpcs/benches/basefold.rs
Outdated
| .iter() | ||
| .map(Evaluation::value) | ||
| .map(|x| *x) | ||
| .copied() |
There was a problem hiding this comment.
That's an unrelated improvement, isn't it? (It's small enough that we can leave it in this PR, however.)
mpcs/benches/basefold.rs
Outdated
| .map(Evaluation::value) | ||
| .map(|x| *x) | ||
| .copied() | ||
| .collect::<Vec<E>>(); |
There was a problem hiding this comment.
(Not part of your PR: if we specify the type in line 187 (on the let), we don't need to also specify it here, too.)
| * E::from(1 << (num_vars - poly.num_vars)) | ||
| inner_product(poly, build_eq_x_r_vec(point).iter()) | ||
| * scalar | ||
| * E::from(1 << (num_vars - log2_strict(poly.len()))) |
There was a problem hiding this comment.
This seems almost like an unrelated change?
matthiasgoergens
left a comment
matthiasgoergens
left a comment
There was a problem hiding this comment.
Looks mostly ok. I left a question about the change to logarithms.
Also a small suggestion:
diff --git a/mpcs/benches/basefold.rs b/mpcs/benches/basefold.rs
index 1aa55ddb..4b37d729 100644
--- a/mpcs/benches/basefold.rs
+++ b/mpcs/benches/basefold.rs
@@ -166,8 +166,8 @@ fn bench_batch_commit_open_verify_goldilocks<Pcs: PolynomialCommitmentScheme<E>>
transcript.append_field_element_exts(values.as_slice());
let transcript_for_bench = transcript.clone();
let polys = polys
- .iter()
- .map(|poly| ArcMultilinearExtension::from(poly.clone()))
+ .into_iter()
+ .map(ArcMultilinearExtension::from)
.collect::<Vec<_>>();
let proof =
Pcs::batch_open(&pp, &polys, &comms, &points, &evals, &mut transcript).unwrap();
diff --git a/mpcs/src/lib.rs b/mpcs/src/lib.rs
index ac1690e7..438101c4 100644
--- a/mpcs/src/lib.rs
+++ b/mpcs/src/lib.rs
@@ -531,8 +531,8 @@ pub mod test_util {
transcript.append_field_element_exts(values.as_slice());
let polys = polys
- .iter()
- .map(|poly| ArcMultilinearExtension::from(poly.clone()))
+ .into_iter()
+ .map(ArcMultilinearExtension::from)
.collect_vec();
let proof =
Extracted from #294
Currently, only
simple_batch_openis requiringArcfor polys in API while other open functions are acceptingDenseMultilinearExtension. This PR unifies them.