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(bb): towards reduced polynomial memory usage #7990

Merged
merged 165 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 66 commits
Commits
Show all changes
165 commits
Select commit Hold shift + click to select a range
88cb0b4
feat(bb): debug log
ludamad0 Dec 8, 2023
3714e2e
Debug log
ludamad0 Dec 11, 2023
3ddc166
Merge remote-tracking branch 'origin/master' into ad/feat/bb-debug-log
ludamad0 Dec 11, 2023
67de114
Update debug_log.cpp
ludamad Dec 11, 2023
9421093
Update debug_log.cpp
ludamad Dec 11, 2023
b2cb0d8
format
ludamad0 Dec 11, 2023
ac72179
debugs
ludamad0 Dec 11, 2023
e3b8713
empty polys
ludamad0 Dec 11, 2023
f9ce86b
fix(ci): correctly run bb tests with asserts
ludamad Jul 25, 2024
d528cc0
Merge branch 'master' into ad/bb/enable/asserts
ludamad Aug 13, 2024
428f0de
remove unneeded assert
ludamad Aug 13, 2024
30c8e5c
pass on polynomial memory representation
ludamad Aug 14, 2024
496fdae
Merge branch 'master' into ad/mem-offsets
ludamad Aug 14, 2024
f9c7931
passing tests
ludamad Aug 14, 2024
1552197
Merge remote-tracking branch 'origin/ad/mem-offsets' into ad/mem-offsets
ludamad Aug 14, 2024
4f1e519
more refactoring
ludamad Aug 14, 2024
eaa6a34
start of actually using new polynomial structure
ludamad Aug 14, 2024
50f59fe
progress
ludamad Aug 14, 2024
d79914e
poly iter class
ludamad Aug 14, 2024
d83219e
more polynomial related boilerplate...
ludamad Aug 14, 2024
cdeedc6
polynomial span checkpoint
ludamad Aug 16, 2024
24b35f3
poly arith fix
ludamad Aug 16, 2024
a3d18fb
checkpoint
ludamad Aug 19, 2024
23ff433
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Aug 20, 2024
45ee50d
progress
ludamad Aug 20, 2024
b3868e2
Merge branch 'master' into ad/bb/enable/asserts
ludamad Aug 21, 2024
b6f6968
Merge remote-tracking branch 'origin/master' into ad/bb/enable/asserts
ludamad Aug 21, 2024
f3a7f60
assert-friendly tests: AztecIVC
ludamad Aug 21, 2024
8e4ece7
ensure converted points are on curve
ludamad Aug 21, 2024
35fa784
poly test fix
ludamad Aug 21, 2024
5f347ca
fix the last of the asserts hopefully
ludamad Aug 21, 2024
da1baa9
Merge remote-tracking branch 'origin/ad/bb/enable/asserts' into ad/bb…
ludamad Aug 21, 2024
d0b3918
Merge remote-tracking branch 'origin/master' into ad/bb/enable/asserts
ludamad Aug 21, 2024
99d672a
constify pippenger
ludamad Aug 21, 2024
ebf2a90
Merge branch 'ad/bb/enable/asserts' into ad/mem-offsets
ludamad Aug 22, 2024
6a6795e
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Aug 22, 2024
5561f67
Try to get ultrahonk tests working
ludamad Aug 22, 2024
109146b
push to see tests
ludamad Aug 22, 2024
bbc3b84
Merge branch 'master' into ad/mem-offsets
ludamad Aug 22, 2024
55ded5b
debug
ludamad Aug 22, 2024
549edb2
asan fixes for big woops's
ludamad Aug 22, 2024
98bc35c
Merge remote-tracking branch 'origin/ad/feat/bb-debug-log' into ad/me…
ludamad Aug 22, 2024
abff423
Progress
ludamad Aug 23, 2024
7fba8bd
Progress
ludamad Aug 23, 2024
a8023b0
more logs
ludamad Aug 23, 2024
0869084
ultra honk tests working
ludamad Aug 23, 2024
70d194e
wires
ludamad Aug 23, 2024
6932aac
Merge remote-tracking branch 'origin/ad/mem-offsets' into ad/mem-offsets
ludamad Aug 23, 2024
195d845
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Aug 23, 2024
8698df4
cleanup
ludamad Aug 26, 2024
15d9184
eliminate .set()
ludamad Aug 26, 2024
44bd924
all compiling
ludamad Aug 26, 2024
e682488
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Aug 26, 2024
88c0c28
refactor
ludamad Aug 26, 2024
ede7f41
eccvm fix
ludamad Aug 26, 2024
44c7f78
zero initialization
ludamad Aug 26, 2024
fdd2b29
tedious test updates
ludamad Aug 26, 2024
e0e0153
finall translator working again
ludamad Aug 26, 2024
95022c2
Merge branch 'master' into ad/mem-offsets
ludamad Aug 26, 2024
81503b3
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Aug 26, 2024
9623ecf
building again
ludamad Aug 26, 2024
5e99416
building
ludamad Aug 26, 2024
2770bcc
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Aug 27, 2024
65f76b0
avm building
ludamad Aug 27, 2024
6ce46c4
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Aug 27, 2024
d3a4498
revert
ludamad Aug 27, 2024
d63fea1
remove unshifted and pray
ludamad Aug 28, 2024
73fa590
comment on shfits
ludamad Aug 28, 2024
5616be3
remove redundant pippenger param
ludamad Aug 29, 2024
bce13aa
Merge remote-tracking branch 'origin/ad/pippenger-edge-case-smoothing…
ludamad Aug 29, 2024
57d4d81
speed up non powers of 2 in pippenger
ludamad Aug 29, 2024
956a794
Merge remote-tracking branch 'origin/ad/pippenger-edge-case-smoothing…
ludamad Aug 29, 2024
cac72b1
rounding up to power of 2
ludamad Aug 29, 2024
bb6b7cc
fix commitmentkey: rounding up to power of 2
ludamad Aug 29, 2024
d6d08d0
comment
ludamad Aug 30, 2024
4a871b2
comment and use new pippenger
ludamad Aug 30, 2024
cf62795
comment and use new pippenger
ludamad Aug 30, 2024
01efa1f
working
ludamad Aug 30, 2024
5c8418a
revert
ludamad Aug 30, 2024
cb8f5e6
revert
ludamad Aug 30, 2024
536ebbe
std span
ludamad Aug 30, 2024
528142a
revert
ludamad Aug 30, 2024
c371e37
Update scalar_multiplication.cpp
ludamad Aug 30, 2024
069c007
Merge remote-tracking branch 'origin/ad/pippenger-edge-case-smoothing…
ludamad Aug 30, 2024
b1d4b79
merge
ludamad Aug 30, 2024
84d7b74
random poly compile issue
ludamad Aug 30, 2024
94e6df3
Update scalar_multiplication.cpp
ludamad Aug 30, 2024
7dc942b
Update scalar_multiplication.cpp
ludamad Aug 30, 2024
25f64be
Update scalar_multiplication.cpp
ludamad Aug 30, 2024
bf21caf
Update scalar_multiplication.hpp
ludamad Aug 30, 2024
1b16007
compile fix
ludamad Aug 30, 2024
55758d0
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Aug 30, 2024
485d627
Merge branch 'master' into ad/pippenger-edge-case-smoothing
ludamad Aug 30, 2024
39a9525
fix scalar mul edge cases. woops, tried to be too clever
ludamad Aug 30, 2024
47c7736
Merge remote-tracking branch 'origin/ad/pippenger-edge-case-smoothing…
ludamad Aug 30, 2024
84cb602
Merge remote-tracking branch 'origin/ad/pippenger-edge-case-smoothing…
ludamad Aug 30, 2024
00981cb
remote lock improvement
ludamad Aug 30, 2024
686c4a5
Merge branch 'master' into ad/pippenger-edge-case-smoothing
ludamad Aug 30, 2024
c3e37c5
speculative fix
ludamad Aug 30, 2024
ac15f5b
Merge remote-tracking branch 'origin/ad/pippenger-edge-case-smoothing…
ludamad Aug 30, 2024
7dae750
better error
ludamad Aug 30, 2024
a9c2617
fix edge case causing srs size too big
ludamad Aug 30, 2024
49fa0ad
Merge remote-tracking branch 'origin/ad/pippenger-edge-case-smoothing…
ludamad Aug 30, 2024
e791c97
revert
ludamad Aug 30, 2024
2be10cd
revert
ludamad Aug 30, 2024
6386e95
format
ludamad Aug 30, 2024
34425ea
Merge branch 'master' into ad/mem-offsets
ludamad Aug 30, 2024
70f03c5
format
ludamad Aug 30, 2024
7e518f8
Merge branch 'ad/pippenger-edge-case-smoothing' into ad/mem-offsets
ludamad Aug 30, 2024
80d568b
Merge remote-tracking branch 'origin/ad/mem-offsets' into ad/mem-offsets
ludamad Aug 30, 2024
06c61cd
adjustment
ludamad Aug 30, 2024
dd7e461
fix gcc complain
ludamad Aug 30, 2024
d720c55
fix size compare
ludamad Aug 30, 2024
e55bb85
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Sep 3, 2024
1d306e8
merge and notes
ludamad Sep 3, 2024
52027b3
Merge branch 'ad/mem-offsets-structure-spike' into ad/mem-offsets
ludamad Sep 3, 2024
782f472
use format
ludamad Sep 3, 2024
971a388
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Sep 5, 2024
7dbd1d9
format
ludamad Sep 5, 2024
f5514c3
compile fix
ludamad Sep 5, 2024
41c01c6
Merge remote-tracking branch 'origin/ad/mem-offsets' into ad/mem-offsets
ludamad Sep 5, 2024
7f4ea77
[skip ci]
ludamad Sep 5, 2024
e5bcec1
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Sep 6, 2024
d1c7702
updates
ludamad Sep 6, 2024
1457a3c
Merge local stash
ludamad Sep 6, 2024
e307d13
work on structured polys
ludamad Sep 6, 2024
5bbff94
reverts
ludamad Sep 6, 2024
c70ef6f
niceness
ludamad Sep 6, 2024
0f0b86f
Basic index_values() test
ludamad Sep 6, 2024
c81ec3b
remove merge markers
ludamad Sep 6, 2024
4192629
fix zip view
ludamad Sep 6, 2024
0e6c1b6
ultra honk test fixes
ludamad Sep 6, 2024
200a243
Merge branch 'master' into ad/mem-offsets
ludamad Sep 6, 2024
9573c93
Merge
ludamad Sep 6, 2024
2c6a6c6
Merge branch 'ad/fake-base-mem-offsets' into ad/mem-offsets
ludamad Sep 6, 2024
4a2e3a8
Merge remote-tracking branch 'origin/ad/mem-offsets' into ad/mem-offsets
ludamad Sep 6, 2024
f5b430e
revert
ludamad Sep 6, 2024
27f1d36
revert
ludamad Sep 6, 2024
2646dbc
revert
ludamad Sep 6, 2024
f51042a
Merge branch 'ad/fake-base-mem-offsets' into ad/mem-offsets
ludamad Sep 6, 2024
44dd297
revert
ludamad Sep 6, 2024
e72cdbd
repair
ludamad Sep 6, 2024
95e1c5e
repair
ludamad Sep 6, 2024
60d111d
built
ludamad Sep 6, 2024
8bb094d
more test fixes
ludamad Sep 6, 2024
4e9e400
avm fix
ludamad Sep 6, 2024
6047855
better verbosity
ludamad Sep 9, 2024
e16f609
avm changes, get_row investigation
ludamad Sep 9, 2024
7befd6d
at() usage in get_row
ludamad Sep 9, 2024
f0e9ecd
test
ludamad Sep 9, 2024
964384c
fix
ludamad Sep 9, 2024
b2a5423
remove unneeded leftover BB_UNUSED
ludamad Sep 9, 2024
bd0b6dc
remove copypasta test
ludamad Sep 9, 2024
a4f34cb
commitment_schemes_tests working
ludamad Sep 9, 2024
e1d8f4d
commitment recursion tests
ludamad Sep 9, 2024
8f5af24
remove const_at, vm tests green
ludamad Sep 9, 2024
0b69ca6
last fixes(?)
ludamad Sep 10, 2024
5efcd7e
Merge remote-tracking branch 'origin/master' into ad/mem-offsets
ludamad Sep 10, 2024
be4a79e
avm redo
ludamad Sep 10, 2024
8adb0b6
fix relation tests
ludamad Sep 10, 2024
213cc28
fix plonk_honk_shared_tests
ludamad Sep 10, 2024
a45ab52
try fix
ludamad Sep 10, 2024
72d286c
format [skip ci]
ludamad Sep 10, 2024
92926cf
revert
ludamad Sep 10, 2024
0ffcee2
Crs fix
ludamad Sep 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -455,10 +455,10 @@ void pippenger(State& state)
size_t num_cycles = 1 << static_cast<size_t>(state.range(0));
Polynomial<Fr> pol(num_cycles);
for (size_t i = 0; i < num_cycles; i++) {
*(uint256_t*)&pol[i] = engine.get_random_uint256();
pol[i].self_reduce_once();
pol[i].self_reduce_once();
pol[i].self_reduce_once();
*(uint256_t*)&pol.at(i) = engine.get_random_uint256();
pol.at(i).self_reduce_once();
pol.at(i).self_reduce_once();
pol.at(i).self_reduce_once();
}

auto ck = std::make_shared<CommitmentKey<curve::BN254>>(num_cycles);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ void ipa_open(State& state) noexcept
// Construct the polynomial
Polynomial poly(n);
for (size_t i = 0; i < n; ++i) {
poly[i] = Fr::random_element(&engine);
poly.at(i) = Fr::random_element(&engine);
}
auto x = Fr::random_element(&engine);
auto eval = poly.evaluate(x);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ template <typename FF> Polynomial<FF> sparse_random_poly(const size_t size, cons

for (size_t i = 0; i < num_nonzero; i++) {
size_t idx = engine.get_random_uint32() % size;
polynomial[idx] = FF::random_element();
polynomial.at(idx) = FF::random_element();
}

return polynomial;
Expand Down Expand Up @@ -55,7 +55,7 @@ template <typename Curve> void bench_commit_sparse(::benchmark::State& state)

auto polynomial = Polynomial<Fr>(num_points);
for (size_t i = 0; i < num_nonzero; i++) {
polynomial[i] = 1;
polynomial.at(i) = 1;
}

for (auto _ : state) {
Expand All @@ -74,7 +74,7 @@ template <typename Curve> void bench_commit_sparse_preprocessed(::benchmark::Sta

auto polynomial = Polynomial<Fr>(num_points);
for (size_t i = 0; i < num_nonzero; i++) {
polynomial[i] = 1;
polynomial.at(i) = 1;
}

for (auto _ : state) {
Expand Down Expand Up @@ -121,8 +121,8 @@ template <typename Curve> void bench_commit_random(::benchmark::State& state)
auto key = create_commitment_key<Curve>(MAX_NUM_POINTS);

const size_t num_points = 1 << state.range(0);
auto polynomial = Polynomial<Fr>(num_points);
for (auto& coeff : polynomial) {
Polynomial<Fr> polynomial{ num_points };
for (auto& coeff : polynomial.coeffs()) {
coeff = Fr::random_element();
}
for (auto _ : state) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
* simplify the codebase.
*/

#include "barretenberg/common/debug_log.hpp"
#include "barretenberg/common/op_count.hpp"
#include "barretenberg/ecc/scalar_multiplication/scalar_multiplication.hpp"
#include "barretenberg/numeric/bitop/pow.hpp"
#include "barretenberg/polynomials/polynomial.hpp"
#include "barretenberg/polynomials/polynomial_arithmetic.hpp"
#include "barretenberg/srs/factories/crs_factory.hpp"
#include "barretenberg/srs/factories/file_crs_factory.hpp"
Expand Down Expand Up @@ -67,7 +69,7 @@ template <class Curve> class CommitmentKey {
* @param polynomial a univariate polynomial p(X) = ∑ᵢ aᵢ⋅Xⁱ
* @return Commitment computed as C = [p(x)] = ∑ᵢ aᵢ⋅Gᵢ
*/
Commitment commit(std::span<const Fr> polynomial)
Commitment commit(PolynomialSpan<const Fr> polynomial)
{
BB_OP_COUNT_TIME();
const size_t degree = polynomial.size();
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC size() just returns the size of the memory block so the check below needs to be updated accordingly right?

Expand All @@ -78,8 +80,16 @@ template <class Curve> class CommitmentKey {
srs->get_monomial_size());
ASSERT(false);
}
return scalar_multiplication::pippenger_unsafe<Curve>(
const_cast<Fr*>(polynomial.data()), srs->get_monomial_points(), degree, pippenger_runtime_state);

// Extract the precomputed point table (contains raw SRS points at even indices and the corresponding
// endomorphism point (\beta*x, -y) at odd indices). We offset by polynomial.start_index * 2 to align
// with our polynomial span.
G1* point_table = srs->get_monomial_points() + polynomial.start_index * 2;
ludamad marked this conversation as resolved.
Show resolved Hide resolved
DEBUG_LOG_ALL(polynomial.span);
Commitment point = scalar_multiplication::pippenger_unsafe<Curve>(
polynomial.data(), point_table, degree, pippenger_runtime_state);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should actually update the name of the variable degree as it is now decidedly not the degree (and in fact never was)

DEBUG_LOG(point);
return point;
};

/**
Expand All @@ -92,19 +102,20 @@ template <class Curve> class CommitmentKey {
* @param polynomial
* @return Commitment
*/
Commitment commit_sparse(std::span<const Fr> polynomial)
Commitment commit_sparse(PolynomialSpan<const Fr> polynomial)
{
BB_OP_COUNT_TIME();
const size_t degree = polynomial.size();
ASSERT(degree <= srs->get_monomial_size());
const size_t poly_size = polynomial.size();
ASSERT(poly_size + polynomial.start_index * 2 <= srs->get_monomial_size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR but there really should be an srs->size() that returns the size you'd expect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at least now you can do srs->get_monomial_points.size()


// Extract the precomputed point table (contains raw SRS points at even indices and the corresponding
// endomorphism point (\beta*x, -y) at odd indices).
G1* point_table = srs->get_monomial_points();
// endomorphism point (\beta*x, -y) at odd indices). We offset by polynomial.start_index * 2 to align
// with our polynomial spann.
G1* point_table = srs->get_monomial_points() + polynomial.start_index * 2;

// Define structures needed to multithread the extraction of non-zero inputs
const size_t num_threads = degree >= get_num_cpus_pow2() ? get_num_cpus_pow2() : 1;
const size_t block_size = degree / num_threads;
const size_t num_threads = poly_size >= get_num_cpus_pow2() ? get_num_cpus_pow2() : 1;
const size_t block_size = poly_size / num_threads;
std::vector<std::vector<Fr>> thread_scalars(num_threads);
std::vector<std::vector<G1>> thread_points(num_threads);

Expand All @@ -115,7 +126,7 @@ template <class Curve> class CommitmentKey {

for (size_t idx = start; idx < end; ++idx) {

const Fr& scalar = polynomial[idx];
const Fr& scalar = polynomial.span[idx];

if (!scalar.is_zero()) {
thread_scalars[thread_idx].emplace_back(scalar);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ template <typename Curve> class CommitmentTest : public ::testing::Test {
{
Polynomial p(n);
for (size_t i = 0; i < n; ++i) {
p[i] = Fr::random_element(engine);
p.at(i) = Fr::random_element(engine);
}
return p;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,13 @@ template <class Curve> class GeminiTest : public CommitmentTest<Curve> {
const size_t num_unshifted = multilinear_polynomials.size();
const size_t num_shifted = multilinear_polynomials_to_be_shifted.size();
for (size_t i = 0; i < num_unshifted; ++i) {
batched_unshifted.add_scaled(multilinear_polynomials[i], rhos[i]);
batched_unshifted.add_scaled({ /*start index*/ 0, multilinear_polynomials[i] }, rhos[i]);
batched_commitment_unshifted += multilinear_commitments[i] * rhos[i];
}
for (size_t i = 0; i < num_shifted; ++i) {
size_t rho_idx = num_unshifted + i;
batched_to_be_shifted.add_scaled(multilinear_polynomials_to_be_shifted[i], rhos[rho_idx]);
batched_to_be_shifted.add_scaled({ /*start index*/ 0, multilinear_polynomials_to_be_shifted[i] },
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hopeful that we can in general make tradeoffs on methods like add_scaled() that maximize clarity/user experience at the expense of generality. For example could we simply enforce that the mem block of the RHS is strictly contained within the mem block of the LHS? This might not always be true but in the cases where it is not the user may simply have to redefine the LHS to make it true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I understand here, there is that check, but everything has start indices now whereas 0 was implicit before so I passed it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I get it. Will experiment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have made it more strict as discussed

rhos[rho_idx]);
batched_commitment_to_be_shifted += multilinear_commitments_to_be_shifted[i] * rhos[rho_idx];
}

Expand Down Expand Up @@ -119,7 +120,7 @@ TYPED_TEST(GeminiTest, Single)

// Collect multilinear polynomials evaluations, and commitments for input to prover/verifier
std::vector<Fr> multilinear_evaluations = { eval };
std::vector<std::span<Fr>> multilinear_polynomials = { poly };
std::vector<std::span<Fr>> multilinear_polynomials = { poly.coeffs() };
std::vector<std::span<Fr>> multilinear_polynomials_to_be_shifted = {};
std::vector<GroupElement> multilinear_commitments = { commitment };
std::vector<GroupElement> multilinear_commitments_to_be_shifted = {};
Expand All @@ -145,15 +146,15 @@ TYPED_TEST(GeminiTest, SingleShift)

// shiftable polynomial must have 0 as last coefficient
auto poly = this->random_polynomial(n);
poly[0] = Fr::zero();
poly.at(0) = Fr::zero();

auto commitment = this->commit(poly);
auto eval_shift = poly.evaluate_mle(u, true);

// Collect multilinear polynomials evaluations, and commitments for input to prover/verifier
std::vector<Fr> multilinear_evaluations = { eval_shift };
std::vector<std::span<Fr>> multilinear_polynomials = {};
std::vector<std::span<Fr>> multilinear_polynomials_to_be_shifted = { poly };
std::vector<std::span<Fr>> multilinear_polynomials_to_be_shifted = { poly.coeffs() };
std::vector<GroupElement> multilinear_commitments = {};
std::vector<GroupElement> multilinear_commitments_to_be_shifted = { commitment };

Expand Down Expand Up @@ -187,7 +188,7 @@ TYPED_TEST(GeminiTest, Double)

// Collect multilinear polynomials evaluations, and commitments for input to prover/verifier
std::vector<Fr> multilinear_evaluations = { eval1, eval2 };
std::vector<std::span<Fr>> multilinear_polynomials = { poly1, poly2 };
std::vector<std::span<Fr>> multilinear_polynomials = { poly1.coeffs(), poly2.coeffs() };
std::vector<std::span<Fr>> multilinear_polynomials_to_be_shifted = {};
std::vector<GroupElement> multilinear_commitments = { commitment1, commitment2 };
std::vector<GroupElement> multilinear_commitments_to_be_shifted = {};
Expand All @@ -212,8 +213,7 @@ TYPED_TEST(GeminiTest, DoubleWithShift)
auto u = this->random_evaluation_point(log_n);

auto poly1 = this->random_polynomial(n);
auto poly2 = this->random_polynomial(n);
poly2[0] = Fr::zero(); // necessary for polynomial to be 'shiftable'
auto poly2 = this->random_polynomial(n).unshifted(); // make 'shiftable'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how I feel about this unshifted() method. Seems only to be used in tests at the moment. I see you have a WORKTODO in it so maybe its going to go away? My concern is that it's either 'throwing away' the last coefficient or resulting a poly that has degree +1 the 'nominal' size of other polynomials.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a test-only method - I can make sure it has the right size

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving this functionality to random


auto commitment1 = this->commit(poly1);
auto commitment2 = this->commit(poly2);
Expand All @@ -224,8 +224,8 @@ TYPED_TEST(GeminiTest, DoubleWithShift)

// Collect multilinear polynomials evaluations, and commitments for input to prover/verifier
std::vector<Fr> multilinear_evaluations = { eval1, eval2, eval2_shift };
std::vector<std::span<Fr>> multilinear_polynomials = { poly1, poly2 };
std::vector<std::span<Fr>> multilinear_polynomials_to_be_shifted = { poly2 };
std::vector<std::span<Fr>> multilinear_polynomials = { poly1.coeffs(), poly2.coeffs() };
std::vector<std::span<Fr>> multilinear_polynomials_to_be_shifted = { poly2.coeffs() };
std::vector<GroupElement> multilinear_commitments = { commitment1, commitment2 };
std::vector<GroupElement> multilinear_commitments_to_be_shifted = { commitment2 };

Expand Down
11 changes: 6 additions & 5 deletions barretenberg/cpp/src/barretenberg/commitment_schemes/ipa/ipa.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ template <typename Curve_> class IPA {

// Step 4.
// Set initial vector a to the polynomial monomial coefficients and load vector G
auto a_vec = polynomial;
// Ensure the polynomial copy is fully-formed
auto a_vec = polynomial.expand(polynomial.start_index());
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a couple of minutes to understand what this was doing. I'm still not totally sure I understand. Seems like it will make a copy of the poly but ensure that the mem block extends all the way to zero. What about the end of the poly? Do we not need to do the same there? I wonder if in some of these situations we can just do something a bit dumber but easier to follow like auto a_vec = polynomial.full(); or something where it just returns a poly with memory allocated for the full virtual size. You have a lot more context on all of this than I do at this point so I'm just thinking out loud here. Maybe I'm missing a subtlety.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.full() is a good shout - but I took up the conversation on slack if this shiftable model is even worth it. It sounded conceptually nice to have shiftable polys just have start_index = 1, but not so much in practice

auto* srs_elements = ck->srs->get_monomial_points();
std::vector<Commitment> G_vec_local(poly_length);

Expand Down Expand Up @@ -212,16 +213,16 @@ template <typename Curve_> class IPA {
}, thread_heuristics::FF_ADDITION_COST * 2 + thread_heuristics::FF_MULTIPLICATION_COST * 2);
// Sum inner product contributions computed in parallel and unpack the std::pair
auto [inner_prod_L, inner_prod_R] = sum_pairs(inner_prods);
// Step 6.a (using letters, because doxygen automaticall converts the sublist counters to letters :( )
// Step 6.a (using letters, because doxygen automatically converts the sublist counters to letters :( )
// L_i = < a_vec_lo, G_vec_hi > + inner_prod_L * aux_generator
L_i = bb::scalar_multiplication::pippenger_without_endomorphism_basis_points<Curve>(
&a_vec[0], &G_vec_local[round_size], round_size, ck->pippenger_runtime_state);
a_vec.data(), &G_vec_local[round_size], round_size, ck->pippenger_runtime_state);
L_i += aux_generator * inner_prod_L;

// Step 6.b
// R_i = < a_vec_hi, G_vec_lo > + inner_prod_R * aux_generator
R_i = bb::scalar_multiplication::pippenger_without_endomorphism_basis_points<Curve>(
&a_vec[round_size], &G_vec_local[0], round_size, ck->pippenger_runtime_state);
&a_vec.data()[round_size], &G_vec_local[0], round_size, ck->pippenger_runtime_state);
R_i += aux_generator * inner_prod_R;

// Step 6.c
Expand Down Expand Up @@ -257,7 +258,7 @@ template <typename Curve_> class IPA {
parallel_for_heuristic(
round_size,
[&](size_t j) {
a_vec[j] += round_challenge * a_vec[round_size + j];
a_vec.at(j) += round_challenge * a_vec[round_size + j];
b_vec[j] += round_challenge_inv * b_vec[round_size + j];
}, thread_heuristics::FF_ADDITION_COST * 2 + thread_heuristics::FF_MULTIPLICATION_COST * 2);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ TEST_F(IPATest, CommitOnManyZeroCoeffPolyWorks)
constexpr size_t n = 4;
Polynomial p(n);
for (size_t i = 0; i < n - 1; i++) {
p[i] = Fr::zero();
p.at(i) = Fr::zero();
}
p[3] = Fr::one();
p.at(3) = Fr::one();
GroupElement commitment = this->commit(p);
auto* srs_elements = this->ck()->srs->get_monomial_points();
GroupElement expected = srs_elements[0] * p[0];
Expand Down Expand Up @@ -156,8 +156,8 @@ TEST_F(IPATest, AIsZeroAfterOneRound)
size_t n = 4;
auto poly = Polynomial(n);
for (size_t i = 0; i < n / 2; i++) {
poly[i] = Fr::random_element();
poly[i + (n / 2)] = poly[i];
poly.at(i) = Fr::random_element();
poly.at(i + (n / 2)) = poly[i];
}
auto [x, eval] = this->random_eval(poly);
auto commitment = this->commit(poly);
Expand Down Expand Up @@ -249,7 +249,7 @@ TEST_F(IPATest, GeminiShplonkIPAWithShift)
const auto mle_opening_point = this->random_evaluation_point(log_n); // sometimes denoted 'u'
auto poly1 = this->random_polynomial(n);
auto poly2 = this->random_polynomial(n);
poly2[0] = Fr::zero(); // this property is required of polynomials whose shift is used
poly2.at(0) = Fr::zero(); // this property is required of polynomials whose shift is used
Copy link
Contributor

Choose a reason for hiding this comment

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

So I guess if I tried to write poly2[0] = Fr::zero(); the compiler would complain since r-value assignment has been deleted implicitly for fields?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes


GroupElement commitment1 = this->commit(poly1);
GroupElement commitment2 = this->commit(poly2);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ template <typename Curve_> class KZG {
{
Polynomial quotient = opening_claim.polynomial;
OpeningPair<Curve> pair = opening_claim.opening_pair;
quotient[0] -= pair.evaluation;
quotient.at(0) = quotient[0] - pair.evaluation;
// Computes the coefficients for the quotient polynomial q(X) = (p(X) - v) / (X - r) through an FFT
quotient.factor_roots(pair.challenge);
auto quotient_commitment = ck->commit(quotient);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ TYPED_TEST(KZGTest, GeminiShplonkKzgWithShift)
// point.
const auto mle_opening_point = this->random_evaluation_point(log_n); // sometimes denoted 'u'
auto poly1 = this->random_polynomial(n);
auto poly2 = this->random_polynomial(n);
poly2[0] = Fr::zero(); // this property is required of polynomials whose shift is used
auto poly2 = this->random_polynomial(n).unshifted(); // shiftable

GroupElement commitment1 = this->commit(poly1);
GroupElement commitment2 = this->commit(poly2);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ template <typename Curve> class ShplonkProver_ {

// Compute individual claim quotient tmp = ( fⱼ(X) − vⱼ) / ( X − xⱼ )
tmp = claim.polynomial;
tmp[0] -= claim.opening_pair.evaluation;
tmp.at(0) = tmp[0] - claim.opening_pair.evaluation;
tmp.factor_roots(claim.opening_pair.challenge);

// Add the claim quotient to the batched quotient polynomial
Expand Down Expand Up @@ -103,7 +103,7 @@ template <typename Curve> class ShplonkProver_ {
for (const auto& claim : opening_claims) {
// tmp = ρʲ ⋅ ( fⱼ(X) − vⱼ) / ( r − xⱼ )
tmp = claim.polynomial;
tmp[0] -= claim.opening_pair.evaluation;
tmp.at(0) = tmp[0] - claim.opening_pair.evaluation;
Fr scaling_factor = current_nu * inverse_vanishing_evals[idx]; // = ρʲ / ( r − xⱼ )

// G -= ρʲ ⋅ ( fⱼ(X) − vⱼ) / ( r − xⱼ )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ TYPED_TEST(CommitmentKeyTest, CommitSparse)
Polynomial poly{ num_points };
for (size_t i = 0; i < num_nonzero; ++i) {
size_t idx = (i + 1) * (i + 1) % num_points;
poly[idx] = Fr::random_element();
poly.at(idx) = Fr::random_element();
}

// Commit to the polynomial using both the conventional commit method and the sparse commitment method
Expand Down
Loading
Loading