feat: merge-train/barretenberg (#22147)

BEGIN_COMMIT_OVERRIDE
chore: bench phase breakdown + thread sweep for MSM reduction (#21885)
chore: minor fixes pt. 2 (#22138)
chore: minor fixes pt. 3 (#22181)
fix: satisfy Chonk num_circuits >= 4 assertion in mock IVC creation
(#22188)
END_COMMIT_OVERRIDE

Author: iakovenkos

barretenberg/cpp/src/barretenberg/bbapi/bbapi.test.cpp

@@ -1,4 +1,6 @@
 #include "barretenberg/bbapi/bbapi.hpp"
+#include "barretenberg/api/file_io.hpp"
+#include "barretenberg/bbapi/bbapi_shared.hpp"
 #include "barretenberg/common/serialize.hpp"
 #include "barretenberg/common/utils.hpp"
 #include "barretenberg/serialize/test_helper.hpp"
@@ -42,3 +44,72 @@ TYPED_TEST(BBApiMsgpack, DefaultConstructorRoundtrip)
     EXPECT_EQ(actual_response, expected_response);
     std::cout << msgpack_schema_to_string(command) << " " << msgpack_schema_to_string(response) << std::endl;
 }
+
+// Regression tests for input validation at API boundaries.
+// These ensure non-canonical field encodings and trailing bytes are rejected.
+
+TEST(BBApiInputValidation, NonCanonicalPublicInputRejected)
+{
+    using Flavor = UltraFlavor;
+    // A value >= BN254 scalar field modulus should be rejected
+    uint256_t non_canonical = fr::modulus + 1;
+    std::vector<uint256_t> bad_public_inputs = { non_canonical };
+    std::vector<uint256_t> proof = { uint256_t(0) };
+
+    EXPECT_THROW(bbapi::concatenate_proof<Flavor>(bad_public_inputs, proof), std::runtime_error);
+}
+
+TEST(BBApiInputValidation, NonCanonicalProofElementRejected)
+{
+    using Flavor = UltraFlavor;
+    // The modulus itself is non-canonical (valid range is [0, modulus))
+    uint256_t non_canonical = fr::modulus;
+    std::vector<uint256_t> public_inputs = { uint256_t(42) };
+    std::vector<uint256_t> bad_proof = { non_canonical };
+
+    EXPECT_THROW(bbapi::concatenate_proof<Flavor>(public_inputs, bad_proof), std::runtime_error);
+}
+
+TEST(BBApiInputValidation, CanonicalValuesAccepted)
+{
+    using Flavor = UltraFlavor;
+    // modulus - 1 is the largest canonical value
+    uint256_t max_canonical = fr::modulus - 1;
+    std::vector<uint256_t> public_inputs = { uint256_t(0), max_canonical };
+    std::vector<uint256_t> proof = { uint256_t(1) };
+
+    EXPECT_NO_THROW(bbapi::concatenate_proof<Flavor>(public_inputs, proof));
+}
+
+TEST(BBApiInputValidation, TrailingBytesInBinaryInputRejected)
+{
+    // A buffer that is not a multiple of 32 bytes should be rejected
+    std::vector<uint8_t> buf(32 + 1, 0); // 33 bytes = 1 field element + 1 trailing byte
+    EXPECT_THROW(many_from_buffer_exact<uint256_t>(buf, "test input"), std::runtime_error);
+}
+
+TEST(BBApiInputValidation, ExactBinaryInputAccepted)
+{
+    // A buffer that is exactly 2 field elements should parse fine
+    std::vector<uint8_t> buf(64, 0);
+    EXPECT_NO_THROW(many_from_buffer_exact<uint256_t>(buf, "test input"));
+    auto result = many_from_buffer_exact<uint256_t>(buf, "test input");
+    EXPECT_EQ(result.size(), 2UL);
+}
+
+TEST(BBApiInputValidation, VkWithTrailingBytesRejectedOnProveSide)
+{
+    using VK = UltraFlavor::VerificationKey;
+    const size_t expected_size = VK::calc_num_data_types() * sizeof(bb::fr);
+    // One extra byte beyond the expected VK size
+    std::vector<uint8_t> bad_vk(expected_size + 1, 0);
+    EXPECT_THROW(bbapi::validate_vk_size<VK>(bad_vk), std::runtime_error);
+}
+
+TEST(BBApiInputValidation, VkWithCorrectSizeAccepted)
+{
+    using VK = UltraFlavor::VerificationKey;
+    const size_t expected_size = VK::calc_num_data_types() * sizeof(bb::fr);
+    std::vector<uint8_t> good_vk(expected_size, 0);
+    EXPECT_NO_THROW(bbapi::validate_vk_size<VK>(good_vk));
+}

barretenberg/cpp/src/barretenberg/bbapi/bbapi_ultra_honk.cpp

@@ -72,6 +72,7 @@ CircuitProve::Response _prove(std::vector<uint8_t>&& bytecode,
         info("WARNING: computing verification key while proving. Pass in a precomputed vk for better performance.");
         vk = std::make_shared<VerificationKey>(prover_instance->get_precomputed());
     } else {
+        validate_vk_size<VerificationKey>(vk_bytes);
         vk = std::make_shared<VerificationKey>(from_buffer<VerificationKey>(vk_bytes));
     }
 

barretenberg/cpp/src/barretenberg/benchmark/pippenger_bench/pippenger.bench.cpp

@@ -5,6 +5,7 @@
  * Batch config modeled after AVM: ~2618 wire polys of size 2^21, committed in batches of 32.
  */
 #include "barretenberg/common/assert.hpp"
+#include "barretenberg/common/thread.hpp"
 #include "barretenberg/ecc/curves/bn254/bn254.hpp"
 #include "barretenberg/ecc/scalar_multiplication/scalar_multiplication.hpp"
 #include "barretenberg/polynomials/polynomial_arithmetic.hpp"
@@ -82,6 +83,43 @@ BENCHMARK_DEFINE_F(PippengerBench, BatchMSM)(benchmark::State& state)
     }
 }
 
+/**
+ * @brief Batch MSM benchmark variant added to investigate `AztecProtocol/barretenberg#1656`.
+ *
+ * The issue is concerned about the single-threaded final reduction step in
+ * `MSM::batch_multi_scalar_mul(...)` when the work is split across a large number of threads,
+ * e.g. `2^16` points with `256` threads.
+ *
+ * We run a single MSM (num_polys = 1) and sweep thread counts and MSM sizes to make the
+ * final reduction overhead visible in the phase breakdown reported via `BB_BENCH` counters.
+ */
+BENCHMARK_DEFINE_F(PippengerBench, BatchMSM_1656)(benchmark::State& state)
+{
+    const size_t num_threads = static_cast<size_t>(state.range(0));
+    const size_t msm_size = static_cast<size_t>(state.range(1));
+
+    std::vector<Fr> msm_scalars(msm_size);
+    for (auto& s : msm_scalars) {
+        s = Fr::random_element(&engine);
+    }
+
+    std::vector<std::span<Fr>> scalar_spans;
+    std::vector<std::span<const G1>> point_spans;
+    scalar_spans.emplace_back(msm_scalars);
+    point_spans.emplace_back(srs->get_monomial_points().subspan(0, msm_size));
+
+    // This is thread-local: restore after the benchmark so other cases in this binary are unaffected.
+    const size_t original_concurrency = bb::get_num_cpus();
+    bb::set_parallel_for_concurrency(num_threads);
+
+    for (auto _ : state) {
+        GOOGLE_BB_BENCH_REPORTER(state);
+        bb::scalar_multiplication::MSM<Curve>::batch_multi_scalar_mul(point_spans, scalar_spans, false);
+    }
+
+    bb::set_parallel_for_concurrency(original_concurrency);
+}
+
 // ===================== Registration =====================
 
 // Single MSM: 2^14 to 2^20
@@ -97,6 +135,12 @@ BENCHMARK_REGISTER_F(PippengerBench, BatchMSM)
     ->Args({ 32, 1 << 19 })
     ->Args({ 32, 1 << 21 });
 
+// Issue #1656 target: {threads=256, msm_size}
+BENCHMARK_REGISTER_F(PippengerBench, BatchMSM_1656)
+    ->Unit(benchmark::kMillisecond)
+    ->Args({ 256, 1 << 16 })
+    ->Args({ 256, 1 << 20 });
+
 } // namespace
 
 BENCHMARK_MAIN();

barretenberg/cpp/src/barretenberg/chonk/chonk.cpp

@@ -25,7 +25,7 @@ namespace bb {
 Chonk::Chonk(size_t num_circuits)
     : num_circuits(num_circuits)
 {
-    BB_ASSERT_GT(num_circuits, 0UL, "Number of circuits must be specified and greater than 0.");
+    BB_ASSERT_GTE(num_circuits, 4UL, "Number of circuits must be at least 4 (get_queue_type uses num_circuits - 3).");
 }
 
 /**

barretenberg/cpp/src/barretenberg/chonk/chonk_proof.cpp

@@ -43,7 +43,9 @@ ChonkProof_<IsRecursive> ChonkProof_<IsRecursive>::from_field_elements(const std
 
     // MegaZK Oink proof size = total - all other fixed-size components.
     // This correctly accounts for any ACIR public inputs prepended to the oink portion.
-    const size_t mega_zk_oink_length = fields.size() - merge_size - eccvm_size - ipa_size - joint_size;
+    constexpr size_t fixed_total = merge_size + eccvm_size + ipa_size + joint_size;
+    BB_ASSERT_GTE(fields.size(), fixed_total, "ChonkProof::from_field_elements: proof too short");
+    const size_t mega_zk_oink_length = fields.size() - fixed_total;
 
     auto it = fields.begin();
 

barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder_elliptic.test.cpp

@@ -276,3 +276,30 @@ TEST_F(UltraCircuitBuilderElliptic, ChainedOperationsDoubleFailure)
     // Should fail because the middle operation (doubling) has an invalid result
     EXPECT_FALSE(CircuitChecker::check(builder));
 }
+
+// Demonstrates that a doubling gate with off-curve input (0,0) does not constrain the output.
+// The elliptic relation substitutes x1^3 = y1^2 - b, which is invalid for off-curve points.
+// When (x1,y1) = (0,0), both the x- and y-coordinate subrelations become 0 = 0 for any output.
+// Production code (cycle_group::dbl) prevents this by substituting y1 = 1 for points at infinity.
+// See CycleGroupTest.TestDblWitnessPoints (cycle_group.test.cpp) for the complementary test
+// that verifies the mitigation works correctly.
+TEST_F(UltraCircuitBuilderElliptic, DoubleOffCurveOriginUnconstrainedOutput)
+{
+    // (0,0) is not on grumpkin (y^2 = x^3 - 17), so this is an off-curve input
+    auto x1_val = bb::fr(0);
+    auto y1_val = bb::fr(0);
+    // Claim an arbitrary output point
+    auto x3_val = bb::fr::random_element();
+    auto y3_val = bb::fr::random_element();
+
+    UltraCircuitBuilder builder;
+    auto x1 = builder.add_variable(x1_val);
+    auto y1 = builder.add_variable(y1_val);
+    auto x3 = builder.add_variable(x3_val);
+    auto y3 = builder.add_variable(y3_val);
+    builder.create_ecc_dbl_gate({ x1, y1, x3, y3 });
+
+    // The circuit checker passes because the relation is trivially satisfied for (0,0) input.
+    // This is NOT a bug — production code never lets (0,0) reach a doubling gate.
+    EXPECT_TRUE(CircuitChecker::check(builder));
+}

barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp

@@ -48,6 +48,10 @@ MegaCircuitBuilder_<bb::fr> UltraCircuitChecker::prepare_circuit<MegaCircuitBuil
 
 template <typename Builder> bool UltraCircuitChecker::check(const Builder& builder_in)
 {
+    if (builder_in.failed()) {
+        info("CircuitChecker: circuit contains invalid witnesses: ", builder_in.err());
+    }
+
     Builder builder = UltraCircuitChecker::prepare_circuit(builder_in);
 
     // Construct a hash table for lookup table entries to efficiently determine if a lookup gate is valid

barretenberg/cpp/src/barretenberg/commitment_schemes/gemini/gemini.hpp

@@ -267,8 +267,7 @@ template <typename Curve> class GeminiProver_ {
 
     static std::vector<Polynomial> compute_fold_polynomials(const size_t log_n,
                                                             std::span<const Fr> multilinear_challenge,
-                                                            const Polynomial& A_0,
-                                                            const bool& has_zk = false);
+                                                            const Polynomial& A_0);
 
     static std::vector<Claim> construct_univariate_opening_claims(const size_t log_n,
                                                                   Polynomial&& A_0_pos,

barretenberg/cpp/src/barretenberg/commitment_schemes/gemini/gemini_impl.hpp

@@ -66,12 +66,12 @@ std::vector<typename GeminiProver_<Curve>::Claim> GeminiProver_<Curve>::prove(
     Polynomial A_0 = polynomial_batcher.compute_batched(rho);
 
     // Construct the d-1 Gemini foldings of A₀(X)
-    std::vector<Polynomial> fold_polynomials = compute_fold_polynomials(log_n, multilinear_challenge, A_0, has_zk);
+    std::vector<Polynomial> fold_polynomials = compute_fold_polynomials(log_n, multilinear_challenge, A_0);
 
     // If virtual_log_n >= log_n, pad the fold commitments with dummy group elements [1]_1.
     for (size_t l = 0; l < virtual_log_n - 1; l++) {
         std::string label = "Gemini:FOLD_" + std::to_string(l + 1);
-        // When has_zk is true, we are sending commitments to 0. Seems to work, but maybe brittle.
+        // Virtual-round fold polynomials are constant; their commitments are zeroed by the verifier.
         transcript->send_to_verifier(label, commitment_key.commit(fold_polynomials[l]));
     }
     const Fr r_challenge = transcript->template get_challenge<Fr>("Gemini:r");
@@ -108,7 +108,7 @@ std::vector<typename GeminiProver_<Curve>::Claim> GeminiProver_<Curve>::prove(
  */
 template <typename Curve>
 std::vector<typename GeminiProver_<Curve>::Polynomial> GeminiProver_<Curve>::compute_fold_polynomials(
-    const size_t log_n, std::span<const Fr> multilinear_challenge, const Polynomial& A_0, const bool& has_zk)
+    const size_t log_n, std::span<const Fr> multilinear_challenge, const Polynomial& A_0)
 {
     BB_BENCH_NAME("Gemini::compute_fold_polynomials");
     BB_ASSERT_GTE(log_n, size_t(2), "Gemini folding requires at least 4-element polynomials");
@@ -156,25 +156,23 @@ std::vector<typename GeminiProver_<Curve>::Polynomial> GeminiProver_<Curve>::com
         A_l = A_l_fold;
     }
 
-    // Perform virtual rounds.
-    // After the first `log_n - 1` rounds, the prover's `fold` univariates stabilize. With ZK, the verifier multiplies
-    // the evaluations by 0, otherwise, when `virtual_log_n > log_n`, the prover honestly computes and sends the
-    // constant folds.
+    // Virtual rounds (indices log_n .. virtual_log_n - 1).
+    // After real folding, the fold polynomials are constant. Since each constant polynomial evaluates to its own
+    // value at every point, (f(X) - f(x)) / (X - x) = 0, so these contribute nothing to the Shplonk quotient Q(X).
+    // On the verifier side, padding_indicator_array zeros their contributions independently.
     const auto& last = fold_polynomials.back();
     const Fr u_last = multilinear_challenge[log_n - 1];
     const Fr final_eval = last.at(0) + u_last * (last.at(1) - last.at(0));
     Polynomial const_fold(1);
-    // Temporary fix: when we're running a zk proof, the verifier uses a `padding_indicator_array`. So the evals in
-    // rounds past `log_n - 1` will be ignored. Hence the prover also needs to ignore them, otherwise Shplonk will fail.
-    const_fold.at(0) = final_eval * Fr(static_cast<int>(!has_zk));
+    const_fold.at(0) = final_eval;
     fold_polynomials.emplace_back(const_fold);
 
     // FOLD_{log_n+1}, ..., FOLD_{d_v-1}
     Fr tail = Fr(1);
     for (size_t k = log_n; k < virtual_log_n - 1; ++k) {
         tail *= (Fr(1) - multilinear_challenge[k]); // multiply by (1 - u_k)
         Polynomial next_const(1);
-        next_const.at(0) = final_eval * tail * Fr(static_cast<int>(!has_zk));
+        next_const.at(0) = final_eval * tail;
         fold_polynomials.emplace_back(next_const);
     }
 

barretenberg/cpp/src/barretenberg/commitment_schemes/shplonk/shplemini.hpp

@@ -368,8 +368,10 @@ template <typename Curve, bool HasZK = false> class ShpleminiVerifier_ {
                 libra_evaluations, gemini_evaluation_challenge, multivariate_challenge, libra_univariate_evaluation);
         }
 
-        // Currently, only used in ECCVM
+        // Used in ECCVM and BatchedHonkTranslator. The nu power offset in batch_sumcheck_round_claims
+        // assumes ZK claims (NUM_SMALL_IPA_EVALUATIONS) precede sumcheck round claims in the batching order.
         if (committed_sumcheck) {
+            BB_ASSERT(HasZK, "committed sumcheck requires ZK for correct nu power indexing");
             batch_sumcheck_round_claims(commitments,
                                         scalars,
                                         constant_term_accumulator,
@@ -513,18 +515,28 @@ template <typename Curve, bool HasZK = false> class ShpleminiVerifier_ {
         }
 
         // Erase the duplicate entries (higher-index range first to preserve lower indices)
-        auto erase_range = [&](size_t start, size_t count) {
+        auto erase_range = [&](size_t duplicate_start, size_t original_start, size_t count) {
             for (size_t i = 0; i < count; ++i) {
-                scalars.erase(scalars.begin() + static_cast<std::ptrdiff_t>(start));
-                commitments.erase(commitments.begin() + static_cast<std::ptrdiff_t>(start));
+                // Verify the commitment being erased matches its original (native only).
+                // Each erase shifts elements down, so duplicate_start always points to the
+                // next duplicate; the original at original_start + i is unaffected since
+                // we erase higher-index ranges first.
+                if constexpr (!Curve::is_stdlib_type) {
+                    BB_ASSERT(commitments[duplicate_start] == commitments[original_start + i],
+                              "remove_repeated_commitments: commitment mismatch at duplicate index " +
+                                  std::to_string(duplicate_start) + " vs original index " +
+                                  std::to_string(original_start + i));
+                }
+                scalars.erase(scalars.begin() + static_cast<std::ptrdiff_t>(duplicate_start));
+                commitments.erase(commitments.begin() + static_cast<std::ptrdiff_t>(duplicate_start));
             }
         };
         if (second_duplicate_start > first_duplicate_start) {
-            erase_range(second_duplicate_start, r2.count);
-            erase_range(first_duplicate_start, r1.count);
+            erase_range(second_duplicate_start, second_original_start, r2.count);
+            erase_range(first_duplicate_start, first_original_start, r1.count);
         } else {
-            erase_range(first_duplicate_start, r1.count);
-            erase_range(second_duplicate_start, r2.count);
+            erase_range(first_duplicate_start, first_original_start, r1.count);
+            erase_range(second_duplicate_start, second_original_start, r2.count);
         }
     }