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

fix: Reallocate commitment key to avoid pippenger error #11249

Merged
merged 7 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -77,6 +77,7 @@ template <class Curve> class CommitmentKey {
CommitmentKey(const size_t num_points, std::shared_ptr<srs::factories::ProverCrs<Curve>> prover_crs)
: pippenger_runtime_state(num_points)
, srs(prover_crs)
, dyadic_size(get_num_needed_srs_points(num_points))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

used to not be set for plonk, led to errors because the assert would fail.

{}

/**
Expand All @@ -90,6 +91,7 @@ template <class Curve> class CommitmentKey {
PROFILE_THIS_NAME("commit");
// We must have a power-of-2 SRS points *after* subtracting by start_index.
size_t dyadic_poly_size = numeric::round_up_power_2(polynomial.size());
ASSERT(dyadic_poly_size <= dyadic_size && "Polynomial size exceeds commitment key size.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now we throw an error if we try to commit to too large of a polynomial

// Because pippenger prefers a power-of-2 size, we must choose a starting index for the points so that we don't
// exceed the dyadic_circuit_size. The actual start index of the points will be the smallest it can be so that
// the window of points is a power of 2 and still contains the scalars. The best we can do is pick a start index
Expand Down Expand Up @@ -211,6 +213,7 @@ template <class Curve> class CommitmentKey {
{
PROFILE_THIS_NAME("commit_structured");
ASSERT(polynomial.end_index() <= srs->get_monomial_size());
ASSERT(polynomial.end_index() <= dyadic_size && "Polynomial size exceeds commitment key size.");

// Percentage of nonzero coefficients beyond which we resort to the conventional commit method
constexpr size_t NONZERO_THRESHOLD = 75;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ template <typename Flavor> class SmallSubgroupIPAProver {
const std::vector<FF>& multivariate_challenge,
const FF claimed_ipa_eval,
std::shared_ptr<typename Flavor::Transcript> transcript,
std::shared_ptr<typename Flavor::CommitmentKey> commitment_key)
std::shared_ptr<typename Flavor::CommitmentKey>& commitment_key)
: interpolation_domain(zk_sumcheck_data.interpolation_domain)
, concatenated_polynomial(zk_sumcheck_data.libra_concatenated_monomial_form)
, libra_concatenated_lagrange_form(zk_sumcheck_data.libra_concatenated_lagrange_form)
Expand All @@ -135,6 +135,11 @@ template <typename Flavor> class SmallSubgroupIPAProver {
, batched_quotient(QUOTIENT_LENGTH)

{
// Reallocate the commitment key if necessary. This is an edge case with SmallSubgroupIPA since it has
// polynomials that may exceed the circuit size.
if (commitment_key->dyadic_size < SUBGROUP_SIZE + 3) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose this is safer than catching it in pippenger - but can we also add a check there? Approving though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean? the assert in commit() should serve that purpose right

commitment_key = std::make_shared<typename Flavor::CommitmentKey>(SUBGROUP_SIZE + 3);
}
// Extract the evaluation domain computed by ZKSumcheckData
if constexpr (std::is_same_v<Curve, curve::BN254>) {
bn_evaluation_domain = std::move(zk_sumcheck_data.bn_evaluation_domain);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,9 @@ typename Curve::Element pippenger_internal(std::span<const typename Curve::Affin
bool handle_edge_cases)
{
PROFILE_THIS();

ASSERT(scalars.start_index + scalars.size() <= state.num_points / 2 &&
"Pippenger runtime state is too small to support this many points");
// multiplication_runtime_state state;
compute_wnaf_states<Curve>(state.point_schedule, state.skew_table, state.round_counts, scalars, num_initial_points);
organize_buckets(state.point_schedule, num_initial_points * 2);
Expand All @@ -923,6 +926,9 @@ typename Curve::Element pippenger(PolynomialSpan<const typename Curve::ScalarFie
using Group = typename Curve::Group;
using Element = typename Curve::Element;

ASSERT(scalars_.start_index + scalars_.size() <= state.num_points / 2 &&
"Pippenger runtime state is too small to support this many points");

// our windowed non-adjacent form algorthm requires that each thread can work on at least 8 points.
// If we fall below this theshold, fall back to the traditional scalar multiplication algorithm.
// For 8 threads, this neatly coincides with the threshold where Strauss scalar multiplication outperforms
Expand Down Expand Up @@ -984,6 +990,9 @@ typename Curve::Element pippenger_unsafe_optimized_for_non_dyadic_polys(
{
PROFILE_THIS();

ASSERT(scalars.start_index + scalars.size() <= state.num_points / 2 &&
"Pippenger runtime state is too small to support this many points");

// our windowed non-adjacent form algorthm requires that each thread can work on at least 8 points.
const size_t threshold = get_num_cpus_pow2() * 8;
// Delegate edge-cases to normal pippenger_unsafe().
Expand Down Expand Up @@ -1018,6 +1027,8 @@ typename Curve::Element pippenger_unsafe(PolynomialSpan<const typename Curve::Sc
std::span<const typename Curve::AffineElement> points,
pippenger_runtime_state<Curve>& state)
{
ASSERT(scalars.start_index + scalars.size() <= state.num_points / 2 &&
"Pippenger runtime state is too small to support this many points");
return pippenger(scalars, points, state, false);
}

Expand All @@ -1030,6 +1041,8 @@ typename Curve::Element pippenger_without_endomorphism_basis_points(
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1135): We don't need start_index more scalars here.
std::vector<typename Curve::AffineElement> G_mod((scalars.start_index + scalars.size()) * 2);
ASSERT(scalars.start_index + scalars.size() <= points.size());
ASSERT(scalars.start_index + scalars.size() <= state.num_points / 2 &&
"Pippenger runtime state is too small to support this many points");
bb::scalar_multiplication::generate_pippenger_point_table<Curve>(
points.data(), &G_mod[0], scalars.start_index + scalars.size());
return pippenger(scalars, G_mod, state, false);
Expand Down
Loading