fix: minor issues in bigfield (#22216)

Fixes minor issues found by claudebox in bigfield. Mostly tightens
things around range constraints and adds/updates comments.

Author: suyash67

barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh

@@ -21,7 +21,7 @@ script_path="$root/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_cha
 # - Generate a hash for versioning: sha256sum bb-chonk-inputs.tar.gz
 # - Upload the compressed results: aws s3 cp bb-chonk-inputs.tar.gz s3://aztec-ci-artifacts/protocol/bb-chonk-inputs-[hash(0:8)].tar.gz
 # Note: In case of the "Test suite failed to run ... Unexpected token 'with' " error, need to run: docker pull aztecprotocol/build:3.0
-pinned_short_hash="aafc0a7e"
+pinned_short_hash="7f8e5859"
 pinned_chonk_inputs_url="https://aztec-ci-artifacts.s3.us-east-2.amazonaws.com/protocol/bb-chonk-inputs-${pinned_short_hash}.tar.gz"
 
 function update_pinned_hash_in_script {

barretenberg/cpp/src/barretenberg/dsl/acir_format/gate_count_constants.hpp

@@ -36,7 +36,7 @@ template <typename Builder> inline constexpr size_t AES128_ENCRYPTION = 1559 + Z
 // contain only 2 of the values set for ECCVM (hence the difference of two gates between Ultra and Mega builders).
 template <typename Builder> inline constexpr size_t ECDSA_SECP256K1 = 42837 + ZERO_GATE;
 template <typename Builder>
-inline constexpr size_t ECDSA_SECP256R1 = 72612 + ZERO_GATE + (IsMegaBuilder<Builder> ? 2 : 0);
+inline constexpr size_t ECDSA_SECP256R1 = 72611 + ZERO_GATE + (IsMegaBuilder<Builder> ? 2 : 0);
 
 template <typename Builder> inline constexpr size_t BLAKE2S = 2952 + ZERO_GATE + MEGA_OFFSET<Builder>;
 template <typename Builder> inline constexpr size_t BLAKE3 = 2158 + ZERO_GATE + MEGA_OFFSET<Builder>;
@@ -55,7 +55,7 @@ template <typename Builder> inline constexpr size_t ASSERT_EQUALITY = ZERO_GATE
 // Honk Recursion Constants
 // ========================================
 
-inline constexpr size_t ROOT_ROLLUP_GATE_COUNT = 12904952;
+inline constexpr size_t ROOT_ROLLUP_GATE_COUNT = 12904885;
 
 template <typename RecursiveFlavor>
 constexpr std::tuple<size_t, size_t> HONK_RECURSION_CONSTANTS(
@@ -67,18 +67,18 @@ constexpr std::tuple<size_t, size_t> HONK_RECURSION_CONSTANTS(
     if constexpr (std::is_same_v<RecursiveFlavor, bb::UltraRecursiveFlavor_<UltraCircuitBuilder>>) {
         switch (mode) {
         case PredicateTestCase::ConstantTrue:
-            return std::make_tuple(681794, 0);
+            return std::make_tuple(681762, 0);
         case PredicateTestCase::WitnessTrue:
         case PredicateTestCase::WitnessFalse:
-            return std::make_tuple(682851, 0);
+            return std::make_tuple(682819, 0);
         }
     } else if constexpr (std::is_same_v<RecursiveFlavor, bb::UltraZKRecursiveFlavor_<UltraCircuitBuilder>>) {
         switch (mode) {
         case PredicateTestCase::ConstantTrue:
-            return std::make_tuple(703951, 0);
+            return std::make_tuple(703917, 0);
         case PredicateTestCase::WitnessTrue:
         case PredicateTestCase::WitnessFalse:
-            return std::make_tuple(705104, 0);
+            return std::make_tuple(705070, 0);
         }
     } else if constexpr (std::is_same_v<RecursiveFlavor, bb::UltraRecursiveFlavor_<MegaCircuitBuilder>>) {
         switch (mode) {
@@ -100,7 +100,7 @@ constexpr std::tuple<size_t, size_t> HONK_RECURSION_CONSTANTS(
         if (mode != PredicateTestCase::ConstantTrue) {
             bb::assert_failure("Unhandled mode in MegaZKRecursiveFlavor.");
         }
-        return std::make_tuple(781933, 0);
+        return std::make_tuple(781910, 0);
     } else {
         bb::assert_failure("Unhandled recursive flavor.");
     }
@@ -113,7 +113,7 @@ constexpr std::tuple<size_t, size_t> HONK_RECURSION_CONSTANTS(
 // ========================================
 
 // Gate count for Chonk recursive verification (Ultra with RollupIO)
-inline constexpr size_t CHONK_RECURSION_GATES = 1491513;
+inline constexpr size_t CHONK_RECURSION_GATES = 1491408;
 
 // ========================================
 // Hypernova Recursion Constants
@@ -147,7 +147,7 @@ inline constexpr size_t HIDING_KERNEL_ULTRA_OPS = 127;
 // ========================================
 
 // Gate count for ECCVM recursive verifier (Ultra-arithmetized)
-inline constexpr size_t ECCVM_RECURSIVE_VERIFIER_GATE_COUNT = 224206;
+inline constexpr size_t ECCVM_RECURSIVE_VERIFIER_GATE_COUNT = 224162;
 
 // ========================================
 // Goblin AVM Recursive Verifier Constants

barretenberg/cpp/src/barretenberg/relations/non_native_field_relation.hpp

@@ -92,7 +92,7 @@ template <typename FF_> class NonNativeFieldRelationImpl {
 
         // Bigfield Product Gate 2 (selected by q_2 * q_4):
         // Computes cross-term contributions in limb multiplication.
-        // Formula: (w_1 * w_2') + (w_1' * w_2) + (w_1 * w_4 + w_2 * w_3 - w_3') * 2^68 - w_3 - w_4' = 0
+        // Formula: (w_1 * w_2') + (w_1' * w_2) + (w_1 * w_4 + w_2 * w_3 - w_3') * 2^68 - w_4' = 0
         // where primed values (') denote shifted wires from the next row.
         auto limb_subproduct = w_1_m * w_2_shift_m + w_1_shift_m * w_2_m;
         auto non_native_field_gate_2_m = (w_1_m * w_4_m + w_2_m * w_3_m - w_3_shift_m);
@@ -103,7 +103,7 @@ template <typename FF_> class NonNativeFieldRelationImpl {
 
         // Bigfield Product Gate 1 (selected by q_2 * q_3):
         // Accumulates limb products with 2^68 scaling for high-order terms.
-        // Formula: (w_1 * w_2') + (w_1' * w_2) * 2^68 + (w_1' * w_2') - w_3 - w_4 = 0
+        // Formula: (w_1 * w_2' + w_1' * w_2) * 2^68 + (w_1' * w_2') - w_3 - w_4 = 0
         limb_subproduct *= LIMB_SIZE;
         limb_subproduct += (w_1_shift_m * w_2_shift_m);
         auto non_native_field_gate_1_m = limb_subproduct;

barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield_impl.hpp

@@ -273,7 +273,7 @@ template <typename Builder, typename T> bigfield<Builder, T>::bigfield(const byt
     const auto res = bigfield::unsafe_construct_from_limbs(limb0, limb1, limb2, limb3, true);
 
     const auto num_last_limb_bits = 256 - (NUM_LIMB_BITS * 3);
-    res.binary_basis_limbs[3].maximum_value = (uint64_t(1) << num_last_limb_bits);
+    res.binary_basis_limbs[3].maximum_value = (uint64_t(1) << num_last_limb_bits) - 1;
     *this = res;
     set_origin_tag(bytes.get_origin_tag());
 }
@@ -1992,14 +1992,14 @@ void bigfield<Builder, T>::unsafe_assert_less_than(const uint256_t& upper_limit,
         r2.get_witness_index(),
         r3.get_witness_index(),
         static_cast<size_t>(NUM_LIMB_BITS),
-        static_cast<size_t>(NUM_LIMB_BITS),
+        static_cast<size_t>(NUM_LAST_LIMB_BITS),
         msg == "bigfield::unsafe_assert_less_than" ? "bigfield::unsafe_assert_less_than: r2 or r3 too large" : msg);
 }
 
 // check elements are equal mod p by proving their integer difference is a multiple of p.
 // This relies on the minus operator for a-b increasing a by a multiple of p large enough so diff is non-negative
 // When one of the elements is a constant and another is a witness we check equality of limbs, so if the witness
-// bigfield element is in an unreduced form, it needs to be reduced first. We don't have automatice reduced form
+// bigfield element is in an unreduced form, it needs to be reduced first. We don't have automatic reduced form
 // detection for now, so it is up to the circuit writer to detect this
 template <typename Builder, typename T>
 void bigfield<Builder, T>::assert_equal(const bigfield& other, std::string const& msg) const
@@ -2010,13 +2010,16 @@ void bigfield<Builder, T>::assert_equal(const bigfield& other, std::string const
         BB_ASSERT_EQ(get_value(), other.get_value(), "We expect constants to be less than the target modulus");
         return;
     } else if (other.is_constant()) {
-        // NOTE(https://github.com/AztecProtocol/barretenberg/issues/998): This can lead to a situation where
-        // an honest prover cannot satisfy the constraints, because `this` is not reduced, but `other` is, i.e.,
-        // `this` = kp + r  and  `other` = r
-        // where k is a positive integer. In such a case, the prover cannot satisfy the constraints
-        // because the limb-differences would not be 0 mod r. Therefore, an honest prover needs to make sure that
-        // `this` is reduced before calling this method. Also `other` should never be greater than the modulus by
-        // design. As a precaution, we assert that the circuit-constant `other` is less than the modulus.
+        // NOTE(https://github.com/AztecProtocol/barretenberg/issues/998): This does a limb-wise integer
+        // comparison, so `this` must already be in reduced form (value in [0, p)) before calling this method.
+        // If `this = kp + r` and `other = r`, the limbs differ and an honest prover cannot satisfy the
+        // constraints. Callers are responsible for calling self_reduce() first when necessary; we omit it
+        // here to avoid adding spurious gates in the common case where `this` is already reduced.
+        // `other` should never exceed the modulus by design; we assert this as a precaution.
+        BB_ASSERT_LT(get_value(),
+                     modulus_u512,
+                     "bigfield::assert_equal: 'this' is not reduced (value >= p). Call self_reduce() before comparing "
+                     "against a constant.");
         BB_ASSERT_LT(other.get_value(), modulus_u512);
         field_t<Builder> t0 = (binary_basis_limbs[0].element - other.binary_basis_limbs[0].element);
         field_t<Builder> t1 = (binary_basis_limbs[1].element - other.binary_basis_limbs[1].element);
@@ -2072,8 +2075,10 @@ void bigfield<Builder, T>::assert_equal(const bigfield& other, std::string const
 
 // construct a proof that points are different mod p, when they are different mod r
 // WARNING: This method doesn't have perfect completeness - for points equal mod r (or with certain difference kp
-// mod r) but different mod p, you can't construct a proof. The chances of an honest prover running afoul of this
-// condition are extremely small (TODO: compute probability) Note also that the number of constraints depends on how
+// mod r) but different mod p, you can't construct a proof. The failure probability is at most
+// (L + R + 1) / r where L = floor(a.max / p), R = floor(b.max / p), r = native field size (~2^254).
+// With max bounded by 2^256 - 1 and p >= 2^249, we get L,R <= 127, so probability < 2^{-246}.
+// Note also that the number of constraints depends on how
 // much the values have overflown beyond p e.g. due to an addition chain The function is based on the following.
 // Suppose a-b = 0 mod p. Then a-b = k*p for k in a range [-R,L] for largest L and R such that L*p>= a, R*p>=b.
 // And also a-b = k*p mod r for such k. Thus we can verify a-b is non-zero mod p by taking the product of such values
@@ -2147,7 +2152,7 @@ template <typename Builder, typename T> void bigfield<Builder, T>::self_reduce()
 
     BB_ASSERT_LT((uint1024_t(1) << maximum_quotient_bits) * uint1024_t(modulus_u512) + DEFAULT_MAXIMUM_REMAINDER,
                  get_maximum_crt_product());
-    quotient.binary_basis_limbs[0] = Limb(quotient_limb, uint256_t(1) << maximum_quotient_bits);
+    quotient.binary_basis_limbs[0] = Limb(quotient_limb, (uint256_t(1) << maximum_quotient_bits) - 1);
     quotient.binary_basis_limbs[1] = Limb(field_t<Builder>::from_witness_index(context, context->zero_idx()), 0);
     quotient.binary_basis_limbs[2] = Limb(field_t<Builder>::from_witness_index(context, context->zero_idx()), 0);
     quotient.binary_basis_limbs[3] = Limb(field_t<Builder>::from_witness_index(context, context->zero_idx()), 0);

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.test.cpp

@@ -356,7 +356,7 @@ TYPED_TEST(stdlib_field_conversion, GateCountScalarDeserialization)
 TYPED_TEST(stdlib_field_conversion, GateCountBigfieldDeserialization)
 {
     // Deserializing a single bigfield element is expensive due to creating new ranges for range constraints
-    this->template check_deserialization_gate_count<fq<TypeParam>>([] { return bb::fq::random_element(); }, 3515);
+    this->template check_deserialization_gate_count<fq<TypeParam>>([] { return bb::fq::random_element(); }, 3513);
 }
 
 /**
@@ -365,7 +365,7 @@ TYPED_TEST(stdlib_field_conversion, GateCountBigfieldDeserialization)
  */
 TYPED_TEST(stdlib_field_conversion, GateCountMultipleBigfieldDeserialization)
 {
-    this->template check_deserialization_gate_count<fq<TypeParam>>([] { return bb::fq::random_element(); }, 3914, 10);
+    this->template check_deserialization_gate_count<fq<TypeParam>>([] { return bb::fq::random_element(); }, 3913, 10);
 }
 
 /**
@@ -389,7 +389,7 @@ TYPED_TEST(stdlib_field_conversion, GateCountMultipleBN254PointDeserialization)
 {
     using Builder = TypeParam;
 
-    constexpr uint32_t expected = std::is_same_v<Builder, bb::UltraCircuitBuilder> ? 5751 : 0;
+    constexpr uint32_t expected = std::is_same_v<Builder, bb::UltraCircuitBuilder> ? 5746 : 0;
     this->template check_deserialization_gate_count<bn254_element<Builder>>(
         [] { return curve::BN254::AffineElement::random_element(); }, expected, 10);
 }