Skip to content

Commit

Permalink
fix: Reallocate commitment key to avoid pippenger error (#11249)
Browse files Browse the repository at this point in the history
Fixes recent bug introduced by the SmallSubgroupIPA work which added an
edge case where we always commit to polynomials of some fixed degree (of
259 or whatever). Pippenger was set up to work for circuit_size amount
of points, which could be lower than the SmallSubgroupIPA poly sizes, so
it led to buffer overflows.

Fixes it by reallocating commitment key if necessary in SmallSubgroupIPA
prover. Also adds an assert to commit() to check for any potential
overflows.
  • Loading branch information
lucasxia01 authored Jan 16, 2025
1 parent 66f2014 commit 8fc2011
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 1 deletion.
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))
{}

/**
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.");
// 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) {
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

0 comments on commit 8fc2011

Please sign in to comment.