diff --git a/barretenberg/cpp/CMakeLists.txt b/barretenberg/cpp/CMakeLists.txt index d878fb210d3f..2e8ded03c602 100644 --- a/barretenberg/cpp/CMakeLists.txt +++ b/barretenberg/cpp/CMakeLists.txt @@ -35,6 +35,7 @@ option(ENABLE_STACKTRACES "Enable stack traces on assertion" OFF) option(ENABLE_TRACY "Enable low-medium overhead profiling for memory and performance with tracy" OFF) option(ENABLE_PIC "Builds with position independent code" OFF) option(SYNTAX_ONLY "only check syntax (-fsyntax-only)" OFF) +option(ENABLE_WASM_BENCH "Enable BB_BENCH benchmarking support in WASM builds (dev only, not for releases)" OFF) option(AVM "enable building of vm2 module and bb-avm" ON) if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "arm64") @@ -97,6 +98,10 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "wasm32") set(OMP_MULTITHREADING OFF) set(ENABLE_PAR_ALGOS 0) add_compile_definitions(_WASI_EMULATED_PROCESS_CLOCKS=1) + if(ENABLE_WASM_BENCH) + message(STATUS "Enabling WASM benchmarking support (ENABLE_WASM_BENCH)") + add_compile_definitions(ENABLE_WASM_BENCH) + endif() endif() set(CMAKE_C_STANDARD 11) diff --git a/barretenberg/cpp/bootstrap.sh b/barretenberg/cpp/bootstrap.sh index 74ad41e5bf41..bb96f84c7a50 100755 --- a/barretenberg/cpp/bootstrap.sh +++ b/barretenberg/cpp/bootstrap.sh @@ -45,11 +45,15 @@ function inject_version { function build_preset() { local preset=$1 shift - local avm_transpiler_flag="" + local cmake_args=() if [ "${AVM_TRANSPILER:-1}" -eq 0 ]; then - avm_transpiler_flag="-DAVM_TRANSPILER_LIB=" + cmake_args+=(-DAVM_TRANSPILER_LIB=) fi - cmake --fresh --preset "$preset" $avm_transpiler_flag + # Auto-enable ENABLE_WASM_BENCH for wasm-threads preset on non-semver builds + if [[ "$preset" == "wasm-threads" ]] && ! semver check "$REF_NAME"; then + cmake_args+=(-DENABLE_WASM_BENCH=ON) + fi + cmake --fresh --preset "$preset" "${cmake_args[@]}" cmake --build --preset "$preset" "$@" } diff --git a/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh b/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh index 12a9b4dfba8b..e2a295232b80 100755 --- a/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh +++ b/barretenberg/cpp/scripts/test_chonk_standalone_vks_havent_changed.sh @@ -13,8 +13,8 @@ cd .. # - 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="bd98634a" -pinned_chonk_inputs_url="https://aztec-ci-artifacts.s3.us-east-2.amazonaws.com/protocol/bb-chonk-inputs-${pinned_short_hash}.tar.gz" +pinned_short_hash="abdb6bae" +pinned_chonk_inputs_url="https://aztec-ci-artifacts.s3.us-east-2.amazonaws.com/protocol/bb-civc-inputs-${pinned_short_hash}.tar.gz" function compress_and_upload { # 1) Compress the results diff --git a/barretenberg/cpp/scripts/wasmtime.sh b/barretenberg/cpp/scripts/wasmtime.sh index 439cb5a43649..3ad657d5798d 100755 --- a/barretenberg/cpp/scripts/wasmtime.sh +++ b/barretenberg/cpp/scripts/wasmtime.sh @@ -9,6 +9,7 @@ exec wasmtime run \ ${HARDWARE_CONCURRENCY:+--env HARDWARE_CONCURRENCY} \ --env HOME \ ${MAIN_ARGS:+--env MAIN_ARGS} \ + ${BB_BENCH:+--env BB_BENCH} \ --dir=$HOME/.bb-crs \ --dir=. \ "$@" diff --git a/barretenberg/cpp/src/barretenberg/bb/cli.cpp b/barretenberg/cpp/src/barretenberg/bb/cli.cpp index 040e6b7be56c..55f9e094ab50 100644 --- a/barretenberg/cpp/src/barretenberg/bb/cli.cpp +++ b/barretenberg/cpp/src/barretenberg/bb/cli.cpp @@ -559,12 +559,13 @@ int parse_and_run_cli_command(int argc, char* argv[]) debug_logging = flags.debug; verbose_logging = debug_logging || flags.verbose; slow_low_memory = flags.slow_low_memory; -#ifndef __wasm__ +#if !defined(__wasm__) || defined(ENABLE_WASM_BENCH) if (!flags.storage_budget.empty()) { storage_budget = parse_size_string(flags.storage_budget); } if (print_bench || !bench_out.empty() || !bench_out_hierarchical.empty()) { bb::detail::use_bb_bench = true; + vinfo("BB_BENCH enabled via --print_bench or --bench_out"); } #endif @@ -659,9 +660,11 @@ int parse_and_run_cli_command(int argc, char* argv[]) " (default ./ivc-inputs.msgpack)"); } api.prove(flags, ivc_inputs_path, output_path); -#ifndef __wasm__ +#if !defined(__wasm__) || defined(ENABLE_WASM_BENCH) if (print_bench) { + vinfo("Printing BB_BENCH results..."); bb::detail::GLOBAL_BENCH_STATS.print_aggregate_counts_hierarchical(std::cout); + std::cout << std::flush; } if (!bench_out.empty()) { std::ofstream file(bench_out); @@ -687,7 +690,7 @@ int parse_and_run_cli_command(int argc, char* argv[]) UltraHonkAPI api; if (prove->parsed()) { api.prove(flags, bytecode_path, witness_path, vk_path, output_path); -#ifndef __wasm__ +#if !defined(__wasm__) || defined(ENABLE_WASM_BENCH) if (print_bench) { bb::detail::GLOBAL_BENCH_STATS.print_aggregate_counts_hierarchical(std::cout); } diff --git a/barretenberg/cpp/src/barretenberg/benchmark/relations_bench/relations.bench.cpp b/barretenberg/cpp/src/barretenberg/benchmark/relations_bench/relations.bench.cpp index 3c7291591ce0..1a2acbf93f2e 100644 --- a/barretenberg/cpp/src/barretenberg/benchmark/relations_bench/relations.bench.cpp +++ b/barretenberg/cpp/src/barretenberg/benchmark/relations_bench/relations.bench.cpp @@ -49,7 +49,7 @@ template void execute_relation_for_univaria } // Ultra relations (Sumcheck prover work) -BENCHMARK(execute_relation_for_univariates>); +BENCHMARK(execute_relation_for_univariates>); BENCHMARK(execute_relation_for_univariates>); BENCHMARK(execute_relation_for_univariates>); BENCHMARK(execute_relation_for_univariates>); @@ -64,7 +64,7 @@ BENCHMARK(execute_relation_for_univariates>); // Ultra relations (verifier work) -BENCHMARK(execute_relation_for_values>); +BENCHMARK(execute_relation_for_values>); BENCHMARK(execute_relation_for_values>); BENCHMARK(execute_relation_for_values>); BENCHMARK(execute_relation_for_values>); diff --git a/barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_goblin.test.cpp b/barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_goblin.test.cpp index 0389e98c6d33..2f6230c8c97d 100644 --- a/barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_goblin.test.cpp +++ b/barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_goblin.test.cpp @@ -99,10 +99,12 @@ TEST_F(BoomerangGoblinRecursiveVerifierTests, graph_description_basic) ASSERT_TRUE(verified); } auto translator_pairing_points = output.points_accumulator; - translator_pairing_points.P0.x.fix_witness(); - translator_pairing_points.P0.y.fix_witness(); - translator_pairing_points.P1.x.fix_witness(); - translator_pairing_points.P1.y.fix_witness(); + // BIGGROUP_AUDITTODO: It seems suspicious that we have to fix these witnesses here to make this test pass. Seems to + // defeat the purpose of the test. + translator_pairing_points.P0.x().fix_witness(); + translator_pairing_points.P0.y().fix_witness(); + translator_pairing_points.P1.x().fix_witness(); + translator_pairing_points.P1.y().fix_witness(); info("Recursive Verifier: num gates = ", builder.num_gates()); auto graph = cdg::StaticAnalyzer(builder, false); auto variables_in_one_gate = graph.get_variables_in_one_gate(); diff --git a/barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_ultra_recursive_verifier.test.cpp b/barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_ultra_recursive_verifier.test.cpp index 18ce23d94581..7b19d8510372 100644 --- a/barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_ultra_recursive_verifier.test.cpp +++ b/barretenberg/cpp/src/barretenberg/boomerang_value_detection/graph_description_ultra_recursive_verifier.test.cpp @@ -118,10 +118,12 @@ template class BoomerangRecursiveVerifierTest : publi StdlibProof stdlib_inner_proof(outer_circuit, inner_proof); VerifierOutput output = verifier.template verify_proof>(stdlib_inner_proof); PairingObject pairing_points = output.points_accumulator; - pairing_points.P0.x.fix_witness(); - pairing_points.P0.y.fix_witness(); - pairing_points.P1.x.fix_witness(); - pairing_points.P1.y.fix_witness(); + // BIGGROUP_AUDITTODO: It seems suspicious that we have to fix these witnesses here to make this test pass. + // Seems to defeat the purpose of the test. + pairing_points.P0.x().fix_witness(); + pairing_points.P0.y().fix_witness(); + pairing_points.P1.x().fix_witness(); + pairing_points.P1.y().fix_witness(); if constexpr (HasIPAAccumulator) { output.ipa_claim.set_public(); outer_circuit.ipa_proof = output.ipa_proof.get_value(); diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.hpp b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.hpp index c67b1dc00fe8..2546b3efb968 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.hpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.hpp @@ -18,7 +18,7 @@ namespace bb { class UltraCircuitChecker { public: using FF = bb::fr; - using Arithmetic = UltraArithmeticRelation; + using Arithmetic = ArithmeticRelation; using Elliptic = EllipticRelation; using Memory = MemoryRelation; using NonNativeField = NonNativeFieldRelation; diff --git a/barretenberg/cpp/src/barretenberg/common/bb_bench.cpp b/barretenberg/cpp/src/barretenberg/common/bb_bench.cpp index eb60779094b0..2d7794c7c30f 100644 --- a/barretenberg/cpp/src/barretenberg/common/bb_bench.cpp +++ b/barretenberg/cpp/src/barretenberg/common/bb_bench.cpp @@ -1,7 +1,7 @@ #include "barretenberg/common/assert.hpp" #include #include -#ifndef __wasm__ +#if !defined(__wasm__) || defined(ENABLE_WASM_BENCH) #include "barretenberg/serialize/msgpack_impl.hpp" #include "bb_bench.hpp" #include @@ -182,7 +182,7 @@ void AggregateEntry::add_thread_time_sample(const TimeAndCount& stats) // Account for aggregate time and count time += stats.time; count += stats.count; - time_max = std::max(static_cast(stats.time), time_max); + time_max = std::max(stats.time, time_max); // Use Welford's method to be able to track the variance num_threads++; double delta = static_cast(stats.time) - time_mean; @@ -300,12 +300,12 @@ void GlobalBenchStatsContainer::print_aggregate_counts(std::ostream& os, size_t // Serializable structure for a single benchmark entry (msgpack-compatible) struct SerializableEntry { std::string parent; - std::size_t time; - std::size_t time_max; + uint64_t time; + uint64_t time_max; double time_mean; double time_stddev; - std::size_t count; - std::size_t num_threads; + uint64_t count; + uint64_t num_threads; MSGPACK_FIELDS(parent, time, time_max, time_mean, time_stddev, count, num_threads); }; @@ -578,7 +578,7 @@ void GlobalBenchStatsContainer::print_aggregate_counts_hierarchical(std::ostream uint64_t total_time = 0; for (const auto& [_, parent_map] : aggregated) { if (auto it = parent_map.find(""); it != parent_map.end()) { - total_time = std::max(static_cast(total_time), it->second.time_max); + total_time = std::max(total_time, it->second.time_max); } } diff --git a/barretenberg/cpp/src/barretenberg/common/bb_bench.hpp b/barretenberg/cpp/src/barretenberg/common/bb_bench.hpp index f01f5de0513f..ea858c2198ff 100644 --- a/barretenberg/cpp/src/barretenberg/common/bb_bench.hpp +++ b/barretenberg/cpp/src/barretenberg/common/bb_bench.hpp @@ -58,11 +58,11 @@ struct AggregateEntry { // For convenience, even though redundant with map store OperationKey key; OperationKey parent; - std::size_t time = 0; - std::size_t count = 0; + uint64_t time = 0; + uint64_t count = 0; size_t num_threads = 0; double time_mean = 0; - std::size_t time_max = 0; + uint64_t time_max = 0; double time_stddev = 0; // Welford's algorithm state @@ -106,19 +106,19 @@ extern GlobalBenchStatsContainer GLOBAL_BENCH_STATS; // but doesn't provide recursive parent-child relationships through the entire call stack. struct TimeStats { TimeStatsEntry* parent = nullptr; - std::size_t count = 0; - std::size_t time = 0; + uint64_t count = 0; + uint64_t time = 0; // Used if the parent changes from last call - chains to handle multiple parent contexts std::unique_ptr next; TimeStats() = default; - TimeStats(TimeStatsEntry* parent_ptr, std::size_t count_val, std::size_t time_val) + TimeStats(TimeStatsEntry* parent_ptr, uint64_t count_val, uint64_t time_val) : parent(parent_ptr) , count(count_val) , time(time_val) {} - void track(TimeStatsEntry* current_parent, std::size_t time_val) + void track(TimeStatsEntry* current_parent, uint64_t time_val) { // Try to track with current stats if parent matches // Check if 'next' already handles this parent to avoid creating duplicates @@ -138,7 +138,7 @@ struct TimeStats { private: // Returns true if successfully tracked (parent matches), false otherwise - bool raw_track(TimeStatsEntry* expected_parent, std::size_t time_val) + bool raw_track(TimeStatsEntry* expected_parent, uint64_t time_val) { if (parent != expected_parent) { return false; @@ -181,7 +181,7 @@ template struct ThreadBenchStats { struct BenchReporter { TimeStatsEntry* parent; TimeStatsEntry* stats; - std::size_t time; + uint64_t time; BenchReporter(TimeStatsEntry* entry); ~BenchReporter(); }; @@ -196,7 +196,7 @@ struct BenchReporter { #define BB_BENCH_ONLY_NAME(name) (void)0 #define BB_BENCH_ENABLE_NESTING() (void)0 #define BB_BENCH_ONLY() (void)0 -#elif defined __wasm__ +#elif defined __wasm__ && !defined ENABLE_WASM_BENCH #define BB_TRACY() (void)0 #define BB_TRACY_NAME(name) (void)0 #define BB_BENCH_TRACY() (void)0 diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.test.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.test.cpp index c97cb61ce74e..3053946f77b2 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.test.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.test.cpp @@ -4,6 +4,7 @@ #include "acir_format.hpp" #include "acir_format_mocks.hpp" +#include "acir_to_constraint_buf.hpp" #include "barretenberg/common/streams.hpp" #include "barretenberg/op_queue/ecc_op_queue.hpp" @@ -341,3 +342,82 @@ TEST_F(AcirFormatTests, TestBigAdd) EXPECT_TRUE(CircuitChecker::check(builder)); } + +// Helper function to convert a uint256_t to a 32-byte vector in big-endian format +std::vector to_bytes_be(uint256_t value) +{ + std::vector bytes(32, 0); + for (size_t i = 0; i < 32; i++) { + bytes[31 - i] = static_cast(value & 0xFF); + value >>= 8; + } + return bytes; +} + +/** + * @brief Test for bug fix where expressions with distinct witnesses requiring more than one width-4 gate + * were incorrectly processed when they initially appeared to fit in width-3 gates + * + * @details This test verifies the fix for a bug in handle_arithmetic where an expression with: + * - 1 mul term using witnesses (w0 * w1) + * - 3 additional linear terms using distinct witnesses (w2, w3, w4) + * + * Such expressions have ≤3 linear combinations and ≤1 mul term, appearing to fit in a + * poly_triple (width-3) gate. However, with all 5 witnesses distinct, serialize_arithmetic_gate + * correctly returns all zeros, indicating it cannot fit in a width-3 gate. + * + * The bug: old code would check if poly_triple was all zeros, and if so, directly add to + * quad_constraints via serialize_mul_quad_gate. But it did this inside the initial + * might_fit_in_polytriple check, so it would never properly go through the mul_quad processing + * logic that handles the general case with >4 witnesses. + * + * The fix: now uses a needs_to_be_parsed_as_mul_quad flag that is set when poly_triple fails, + * and processes through the proper mul_quad logic path, which splits into multiple gates. + * + * Expression: w0 * w1 + w2 + w3 + w4 = 10 + * With witnesses: w0=0, w1=1, w2=2, w3=3, w4=4 + * Evaluation: 0*1 + 2 + 3 + 4 = 9, but we set q_c = -9, so constraint is: 9 - 9 = 0 + */ +TEST_F(AcirFormatTests, TestArithmeticGateWithDistinctWitnessesRegression) +{ + // Create an ACIR expression: w0 * w1 + w2 + w3 + w4 - 9 = 0 + // This has 1 mul term and 3 linear terms with all 5 distinct witnesses (requires multiple width-4 gates) + Acir::Expression expr{ .mul_terms = { std::make_tuple( + to_bytes_be(1), Acir::Witness{ .value = 0 }, Acir::Witness{ .value = 1 }) }, + .linear_combinations = { std::make_tuple(to_bytes_be(1), Acir::Witness{ .value = 2 }), + std::make_tuple(to_bytes_be(1), Acir::Witness{ .value = 3 }), + std::make_tuple(to_bytes_be(1), Acir::Witness{ .value = 4 }) }, + .q_c = to_bytes_be(static_cast(fr(-9))) }; + + Acir::Opcode::AssertZero assert_zero{ .value = expr }; + + // Create an ACIR circuit with this opcode + Acir::Circuit circuit{ + .current_witness_index = 5, + .opcodes = { Acir::Opcode{ .value = assert_zero } }, + .return_values = {}, + }; + + Acir::Program program{ .functions = { circuit } }; + + // Serialize the program to bytes + auto program_bytes = program.bincodeSerialize(); + + // Process through circuit_buf_to_acir_format (this calls handle_arithmetic internally) + AcirFormat constraint_system = circuit_buf_to_acir_format(std::move(program_bytes)); + + // The key assertion: this expression should end up in big_quad_constraints, not poly_triple_constraints + // or single quad_constraints, because it needs 5 witness slots (all distinct) + EXPECT_EQ(constraint_system.poly_triple_constraints.size(), 0); + EXPECT_EQ(constraint_system.quad_constraints.size(), 0); + EXPECT_EQ(constraint_system.big_quad_constraints.size(), 1); + + // Now verify the constraint system with valid witness assignments + // We need: w0 * w1 + w2 + w3 + w4 = 9 + // Use values: w0=0, w1=1, w2=2, w3=3, w4=4, so 0*1 + 2 + 3 + 4 = 9 + WitnessVector witness{ 0, 1, 2, 3, 4 }; + AcirProgram acir_program{ constraint_system, witness }; + auto builder = create_circuit(acir_program); + + EXPECT_TRUE(CircuitChecker::check(builder)); +} diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_to_constraint_buf.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_to_constraint_buf.cpp index 84ef21dfb8c7..b0304fbf1149 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_to_constraint_buf.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_to_constraint_buf.cpp @@ -459,7 +459,9 @@ std::pair is_assert_equal(Acir::Opcode::AssertZero const& ar void handle_arithmetic(Acir::Opcode::AssertZero const& arg, AcirFormat& af, size_t opcode_index) { // If the expression fits in a polytriple, we use it. - if (arg.value.linear_combinations.size() <= 3 && arg.value.mul_terms.size() <= 1) { + bool might_fit_in_polytriple = arg.value.linear_combinations.size() <= 3 && arg.value.mul_terms.size() <= 1; + bool needs_to_be_parsed_as_mul_quad = !might_fit_in_polytriple; + if (might_fit_in_polytriple) { poly_triple pt = serialize_arithmetic_gate(arg.value); auto assert_equal = is_assert_equal(arg, pt, af); @@ -502,15 +504,14 @@ void handle_arithmetic(Acir::Opcode::AssertZero const& arg, AcirFormat& af, size // gate. This is the case if the linear terms are all distinct witness from the multiplication term. In that // case, the serialize_arithmetic_gate() function will return a poly_triple with all 0's, and we use a width-4 // gate instead. We could probably always use a width-4 gate in fact. - if (pt == poly_triple{ 0, 0, 0, 0, 0, 0, 0, 0 }) { - af.quad_constraints.push_back(serialize_mul_quad_gate(arg.value)); - af.original_opcode_indices.quad_constraints.push_back(opcode_index); - - } else { + if (pt != poly_triple{ 0, 0, 0, 0, 0, 0, 0, 0 }) { af.poly_triple_constraints.push_back(pt); af.original_opcode_indices.poly_triple_constraints.push_back(opcode_index); + } else { + needs_to_be_parsed_as_mul_quad = true; } - } else { + } + if (needs_to_be_parsed_as_mul_quad) { std::vector> mul_quads; // We try to use a single mul_quad gate to represent the expression. if (arg.value.mul_terms.size() <= 1) { diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.cpp index c7f6219a796d..caaedc23a018 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_constraints.cpp @@ -94,8 +94,11 @@ void create_ecdsa_verify_constraints(typename Curve::Builder& builder, // P is on the curve typename Curve::AffineElement default_point(Curve::g1::one + Curve::g1::one); - public_key.x = Fq::conditional_assign(predicate, public_key.x, default_point.x); - public_key.y = Fq::conditional_assign(predicate, public_key.y, default_point.y); + // BIGGROUP_AUDITTODO: mutable accessor needed for conditional_assign(). Could add a conditional_assign method + // to biggroup or could just perform these operations on the underlying fields prior to constructing the + // biggroup element. + public_key.x() = Fq::conditional_assign(predicate, public_key.x(), default_point.x()); + public_key.y() = Fq::conditional_assign(predicate, public_key.y(), default_point.y()); } else { BB_ASSERT(input.predicate.value, "Creating ECDSA constraints with a constant predicate equal to false."); } diff --git a/barretenberg/cpp/src/barretenberg/flavor/flavor.hpp b/barretenberg/cpp/src/barretenberg/flavor/flavor.hpp index 780073b5e556..1fd90d296385 100644 --- a/barretenberg/cpp/src/barretenberg/flavor/flavor.hpp +++ b/barretenberg/cpp/src/barretenberg/flavor/flavor.hpp @@ -461,7 +461,7 @@ template constexpr auto create_sumcheck_tuple_of_tuple * * @example if RelationsTuple = UltraFlavor::Relations_, then the tuple returned by the function is a tuple of length 9, * where the first element of the tuple is an array of length 2 (as the first relation in UltraFlavor::Relations_ is the - * UltraArithmeticRelation, which is made up by two subrelations). + * ArithmeticRelation, which is made up by two subrelations). * * @tparam RelationsTuple */ diff --git a/barretenberg/cpp/src/barretenberg/flavor/mega_flavor.hpp b/barretenberg/cpp/src/barretenberg/flavor/mega_flavor.hpp index 53d0a3fcd5c9..0bcd4bd41e2f 100644 --- a/barretenberg/cpp/src/barretenberg/flavor/mega_flavor.hpp +++ b/barretenberg/cpp/src/barretenberg/flavor/mega_flavor.hpp @@ -76,7 +76,7 @@ class MegaFlavor { // define the tuple of Relations that comprise the Sumcheck relation // Note: made generic for use in MegaRecursive. template - using Relations_ = std::tuple, + using Relations_ = std::tuple, bb::UltraPermutationRelation, bb::LogDerivLookupRelation, bb::DeltaRangeConstraintRelation, diff --git a/barretenberg/cpp/src/barretenberg/flavor/ultra_flavor.hpp b/barretenberg/cpp/src/barretenberg/flavor/ultra_flavor.hpp index bed7a1d34fd3..0b94fe01f934 100644 --- a/barretenberg/cpp/src/barretenberg/flavor/ultra_flavor.hpp +++ b/barretenberg/cpp/src/barretenberg/flavor/ultra_flavor.hpp @@ -81,7 +81,7 @@ class UltraFlavor { // List of relations reflecting the Ultra arithmetisation. WARNING: As UltraKeccak flavor inherits from // Ultra flavor any change of ordering in this tuple needs to be reflected in the smart contract, otherwise // relation accumulation will not match. - using Relations_ = std::tuple, + using Relations_ = std::tuple, bb::UltraPermutationRelation, bb::LogDerivLookupRelation, bb::DeltaRangeConstraintRelation, diff --git a/barretenberg/cpp/src/barretenberg/honk/relation_checker.hpp b/barretenberg/cpp/src/barretenberg/honk/relation_checker.hpp index 184cc9873d11..094a51585036 100644 --- a/barretenberg/cpp/src/barretenberg/honk/relation_checker.hpp +++ b/barretenberg/cpp/src/barretenberg/honk/relation_checker.hpp @@ -93,7 +93,7 @@ template <> class RelationChecker : public RelationChecker>(polynomials, params, "UltraArithmetic"); + Base::check>(polynomials, params, "UltraArithmetic"); Base::check>(polynomials, params, "UltraPermutation"); Base::check>(polynomials, params, "DeltaRangeConstraint"); Base::check>(polynomials, params, "Elliptic"); diff --git a/barretenberg/cpp/src/barretenberg/polynomials/backing_memory.cpp b/barretenberg/cpp/src/barretenberg/polynomials/backing_memory.cpp index 13c9ecd1c4b8..2336c95521d7 100644 --- a/barretenberg/cpp/src/barretenberg/polynomials/backing_memory.cpp +++ b/barretenberg/cpp/src/barretenberg/polynomials/backing_memory.cpp @@ -10,9 +10,19 @@ bool slow_low_memory = std::getenv("BB_SLOW_LOW_MEMORY") == nullptr ? false : std::string(std::getenv("BB_SLOW_LOW_MEMORY")) == "1"; -// Storage budget is disabled for WASM builds -#ifndef __wasm__ +// Storage budget is disabled for WASM builds unless ENABLE_WASM_BENCH is defined +#if !defined(__wasm__) || defined(ENABLE_WASM_BENCH) +#if defined(__wasm__) && defined(BB_NO_EXCEPTIONS) +// Simplified version for WASM builds without exception support +// For WASM benchmarking, we don't enforce storage budgets +size_t parse_size_string(const std::string& /* size_str */) +{ + // Return unlimited budget for WASM builds + return std::numeric_limits::max(); +} +#else +// Full version with exception handling for native builds // Parse storage size string (e.g., "500m", "2g", "1024k") size_t parse_size_string(const std::string& size_str) { @@ -55,6 +65,7 @@ size_t parse_size_string(const std::string& size_str) throw_or_abort("Invalid storage size format: '" + size_str + "'. Value out of range"); } } +#endif namespace { // Parse storage budget from environment variable (supports k/m/g suffixes like Docker) @@ -75,4 +86,4 @@ size_t storage_budget = parse_storage_budget(); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) std::atomic current_storage_usage{ 0 }; -#endif // __wasm__ +#endif // !defined(__wasm__) || defined(ENABLE_WASM_BENCH) diff --git a/barretenberg/cpp/src/barretenberg/relations/ultra_arithmetic_relation.hpp b/barretenberg/cpp/src/barretenberg/relations/ultra_arithmetic_relation.hpp index 95e1ed14f5c2..88a868eb0741 100644 --- a/barretenberg/cpp/src/barretenberg/relations/ultra_arithmetic_relation.hpp +++ b/barretenberg/cpp/src/barretenberg/relations/ultra_arithmetic_relation.hpp @@ -9,7 +9,7 @@ namespace bb { -template class UltraArithmeticRelationImpl { +template class ArithmeticRelationImpl { public: using FF = FF_; @@ -25,60 +25,45 @@ template class UltraArithmeticRelationImpl { template inline static bool skip(const AllEntities& in) { return in.q_arith.is_zero(); } /** - * @brief Expression for the Ultra Arithmetic gate. - * @details This relation encapsulates several idenitities, toggled by the value of q_arith in [0, 1, 2, 3, ...]. + * @brief Expression for the Ultra (width-4) Arithmetic gate. + * @details This relation contains two subrelations and encapsulates several identities, toggled by the value of + * q_arith in [0, 1, 2, 3]. * - * The whole formula is: + * Subrelation 1: + * q_arith * + * [ (-1/2) * (q_arith - 3) * (q_m * w_1 * w_2) + \sum_{i=1..4} q_i * w_i + q_c + (q_arith - 1) * w_4_shift] * - * q_arith * ( ( (-1/2) * (q_arith - 3) * q_m * w_1 * w_2 + q_1 * w_1 + q_2 * w_2 + q_3 * w_3 + q_4 * w_4 + q_c ) + - * (q_arith - 1)*( α * (q_arith - 2) * (w_1 + w_4 - w_1_omega + q_m) + w_4_omega) ) = 0 + * Subrelation 2: + * q_arith * (q_arith - 1) * (q_arith - 2) * (w_1 + w_4 - w_1_shift + q_m) * - * This formula results in several cases depending on q_arith: - * 1. q_arith == 0: Arithmetic gate is completely disabled + * These formulas result in several cases depending on q_arith: * - * 2. q_arith == 1: Everything in the minigate on the right is disabled. The equation is just a standard plonk - * equation with extra wires: q_m * w_1 * w_2 + q_1 * w_1 + q_2 * w_2 + q_3 * w_3 + q_4 * w_4 + q_c = 0 + * CASE q_arith == 0: Arithmetic gate is completely disabled * - * 3. q_arith == 2: The (w_1 + w_4 - ...) term is disabled. The equation is: - * (1/2) * q_m * w_1 * w_2 + q_1 * w_1 + q_2 * w_2 + q_3 * w_3 + q_4 * w_4 + q_c + w_4_omega = 0 - * It allows defining w_4 at next index (w_4_omega) in terms of current wire values + * CASE q_arith == 1: Conventional 4-wire Ultra arithmetic relation + * Subrelation 1: q_m * w_1 * w_2 + \sum_{i=1..4} q_i * w_i + q_c + * Subrelation 2: Disabled * - * 4. q_arith == 3: The product of w_1 and w_2 is disabled, but a mini addition gate is enabled. α² allows us to - * split the equation into two: + * CASE q_arith == 2: Same as above but with an additional linear term: +w_4_shift + * Subrelation 1: q_m * w_1 * w_2 + [ \sum_{i=1..4} q_i * w_i + q_c + w_4_shift ] * 2 + * Subrelation 2: Disabled + * Note: Factor of 2 on the linear term must be accounted for when constructing inputs to the relation. * - * q_1 * w_1 + q_2 * w_2 + q_3 * w_3 + q_4 * w_4 + q_c + 2 * w_4_omega = 0 - * - * w_1 + w_4 - w_1_omega + q_m = 0 (we are reusing q_m here) - * - * 5. q_arith > 3: The product of w_1 and w_2 is scaled by (q_arith - 3), while the w_4_omega term is scaled by - * (q_arith - * - 1). The equation can be split into two: - * - * (q_arith - 3)* q_m * w_1 * w_ 2 + q_1 * w_1 + q_2 * w_2 + q_3 * w_3 + q_4 * w_4 + q_c + (q_arith - 1) * w_4_omega - * = 0 - * - * w_1 + w_4 - w_1_omega + q_m = 0 - * - * The problem that q_m is used both in both equations can be dealt with by appropriately changing selector values - * at the next gate. Then we can treat (q_arith - 1) as a simulated q_6 selector and scale q_m to handle (q_arith - - * 3) at product. - * - * The relation is - * defined as C(in(X)...) = q_arith * [ -1/2(q_arith - 3)(q_m * w_r * w_l) + (q_l * w_l) + (q_r * w_r) + - * (q_o * w_o) + (q_4 * w_4) + q_c + (q_arith - 1)w_4_shift ] - * - * q_arith * - * (q_arith - 2) * (q_arith - 1) * (w_l + w_4 - w_l_shift + q_m) + * CASE q_arith == 3: + * Subrelation 1: [ \sum_{i=1..4} q_i * w_i + q_c + (2 * w_4_shift) ] * 3 + * Subrelation 2: [ w_1 + w_4 - w_1_shift + q_m ] * 6 + * Note: We are repurposing q_m here as an additive term in the second subrelation. + * Note: Factor of 2 on the w_4_shift term must be accounted for when constructing inputs to the relation. * * @param evals transformed to `evals + C(in(X)...)*scaling_factor` - * @param in an std::array containing the fully extended Univariate edges. - * @param parameters contains beta, gamma, and public_input_delta, .... + * @param in Inputs to the relation algebra + * @param parameters Unused in this relation * @param scaling_factor optional term to scale the evaluation before adding to evals. */ template inline static void accumulate(ContainerOverSubrelations& evals, const AllEntities& in, - const Parameters&, + BB_UNUSED const Parameters& params, const FF& scaling_factor) { using Accumulator = std::tuple_element_t<0, ContainerOverSubrelations>; @@ -91,6 +76,7 @@ template class UltraArithmeticRelationImpl { auto q_arith_sub_1 = q_arith_m - FF(1); auto scaled_q_arith = q_arith_m * scaling_factor; + // Subrelation 1 { using Accumulator = std::tuple_element_t<0, ContainerOverSubrelations>; @@ -111,6 +97,7 @@ template class UltraArithmeticRelationImpl { std::get<0>(evals) += (tmp0 + Accumulator(tmp1)) * Accumulator(scaled_q_arith); } + // Subrelation 2 { using ShortAccumulator = std::tuple_element_t<1, ContainerOverSubrelations>; @@ -124,5 +111,5 @@ template class UltraArithmeticRelationImpl { }; }; -template using UltraArithmeticRelation = Relation>; +template using ArithmeticRelation = Relation>; } // namespace bb diff --git a/barretenberg/cpp/src/barretenberg/relations/ultra_relation_consistency.test.cpp b/barretenberg/cpp/src/barretenberg/relations/ultra_relation_consistency.test.cpp index c96fea4e30c7..3fc636c2e254 100644 --- a/barretenberg/cpp/src/barretenberg/relations/ultra_relation_consistency.test.cpp +++ b/barretenberg/cpp/src/barretenberg/relations/ultra_relation_consistency.test.cpp @@ -108,13 +108,13 @@ class UltraRelationConsistency : public testing::Test { }; }; -TEST_F(UltraRelationConsistency, UltraArithmeticRelation) +TEST_F(UltraRelationConsistency, ArithmeticRelation) { - const auto run_test = [](bool random_inputs) { - using Relation = UltraArithmeticRelation; + const auto run_test = [](bool random_inputs, const FF& q_arith_value = FF::random_element()) { + using Relation = ArithmeticRelation; using SumcheckArrayOfValuesOverSubrelations = typename Relation::SumcheckArrayOfValuesOverSubrelations; - const InputElements input_elements = random_inputs ? InputElements::get_random() : InputElements::get_special(); + InputElements input_elements = random_inputs ? InputElements::get_random() : InputElements::get_special(); const auto& w_1 = input_elements.w_l; const auto& w_1_shift = input_elements.w_l_shift; const auto& w_2 = input_elements.w_r; @@ -127,21 +127,48 @@ TEST_F(UltraRelationConsistency, UltraArithmeticRelation) const auto& q_o = input_elements.q_o; const auto& q_4 = input_elements.q_4; const auto& q_c = input_elements.q_c; + + // Set specific q_arith value to enable testing different modes of the arithmetic relation + input_elements.q_arith = q_arith_value; const auto& q_arith = input_elements.q_arith; SumcheckArrayOfValuesOverSubrelations expected_values; static const FF neg_half = FF(-2).invert(); - // Contribution 1 - auto contribution_1 = (q_arith - 3) * (q_m * w_2 * w_1) * neg_half; - contribution_1 += (q_l * w_1) + (q_r * w_2) + (q_o * w_3) + (q_4 * w_4) + q_c; - contribution_1 += (q_arith - 1) * w_4_shift; - contribution_1 *= q_arith; - expected_values[0] = contribution_1; + FF contribution_1 = FF(0); + FF contribution_2 = FF(0); + if (q_arith == FF(1)) { + // Contribution 1 + contribution_1 = (q_m * w_2 * w_1) + (q_l * w_1) + (q_r * w_2) + (q_o * w_3) + (q_4 * w_4) + q_c; + + // Contribution 2: None + } else if (q_arith == FF(2)) { + // Contribution 1 + contribution_1 = (q_m * w_2 * w_1); + contribution_1 += ((q_l * w_1) + (q_r * w_2) + (q_o * w_3) + (q_4 * w_4) + w_4_shift + q_c) * FF(2); + + // Contribution 2: None + } else if (q_arith == FF(3)) { + // Contribution 1 + contribution_1 = (q_l * w_1) + (q_r * w_2) + (q_o * w_3) + (q_4 * w_4) + q_c; + contribution_1 += w_4_shift * FF(2); + contribution_1 *= FF(3); + + // Contribution 2 + contribution_2 = (w_1 + w_4 - w_1_shift + q_m) * FF(6); + } else { + // Contribution 1 + contribution_1 = (q_arith - 3) * (q_m * w_2 * w_1) * neg_half; + contribution_1 += (q_l * w_1) + (q_r * w_2) + (q_o * w_3) + (q_4 * w_4) + q_c; + contribution_1 += (q_arith - 1) * w_4_shift; + contribution_1 *= q_arith; + + // Contribution 2 + contribution_2 = (w_1 + w_4 - w_1_shift + q_m); + contribution_2 *= (q_arith - 2) * (q_arith - 1) * q_arith; + } - // Contribution 2 - auto contribution_2 = (w_1 + w_4 - w_1_shift + q_m); - contribution_2 *= (q_arith - 2) * (q_arith - 1) * q_arith; + expected_values[0] = contribution_1; expected_values[1] = contribution_2; const auto parameters = RelationParameters::get_random(); @@ -150,6 +177,9 @@ TEST_F(UltraRelationConsistency, UltraArithmeticRelation) }; run_test(/*random_inputs=*/false); run_test(/*random_inputs=*/true); + run_test(/*random_inputs=*/true, /*q_arith_value=*/FF(1)); + run_test(/*random_inputs=*/true, /*q_arith_value=*/FF(2)); + run_test(/*random_inputs=*/true, /*q_arith_value=*/FF(3)); }; TEST_F(UltraRelationConsistency, UltraPermutationRelation) diff --git a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa.test.cpp index 4f98f939739e..582065191e52 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa.test.cpp @@ -80,15 +80,15 @@ template class EcdsaTests : public ::testing::Test { case TamperingMode::XCoordinateOverflow: { // Invalidate the circuit by passing a public key with x >= q // Do nothing here, tampering happens in circuit - failure_msg = "ECDSA input validation: the x coordinate of the public key is bigger than the base field " - "modulus.: hi limb."; + failure_msg = "ECDSA input validation: coordinate(s) of the public key bigger than the base field modulus. " + "(x coordinate): hi limb."; break; } case TamperingMode::YCoordinateOverflow: { // Invalidate the circuit by passing a public key with y >= q // Do nothing here, tampering happens in circuit - failure_msg = "ECDSA input validation: the y coordinate of the public key is bigger than the base field " - "modulus.: hi limb."; + failure_msg = "ECDSA input validation: coordinate(s) of the public key bigger than the base field modulus. " + "(y coordinate): hi limb."; break; } case TamperingMode::InvalidR: { diff --git a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp index 7ccfe32e333a..78c3f9776f0c 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/encryption/ecdsa/ecdsa_impl.hpp @@ -90,10 +90,8 @@ bool_t ecdsa_verify_signature(const stdlib::byte_array& hashed Fr z(hashed_message); // Step 1. - public_key.x.assert_is_in_field( - "ECDSA input validation: the x coordinate of the public key is bigger than the base field modulus."); // x < q - public_key.y.assert_is_in_field( - "ECDSA input validation: the y coordinate of the public key is bigger than the base field modulus."); // y < q + public_key.assert_coordinates_in_field( + "ECDSA input validation: coordinate(s) of the public key bigger than the base field modulus."); // x < q, y < q // Step 2. public_key.validate_on_curve("ECDSA input validation: the public key is not a point on the elliptic curve."); @@ -137,19 +135,22 @@ bool_t ecdsa_verify_signature(const stdlib::byte_array& hashed bool_t(false), "ECDSA validation: the result of the batch multiplication is the point at infinity."); // Step 8. - result.x.reduce_mod_target_modulus(); - - // Transfer Fq value result.x to Fr (this is just moving from a C++ class to another) - Fr result_x_mod_r = Fr::unsafe_construct_from_limbs(result.x.binary_basis_limbs[0].element, - result.x.binary_basis_limbs[1].element, - result.x.binary_basis_limbs[2].element, - result.x.binary_basis_limbs[3].element); + // We reduce result.x() to 2^s, where s is the smallest s.t. 2^s > q. It is cheap in terms of constraints, and + // avoids possible edge cases + // BIGGROUP_AUDITTODO: mutable accessor needed for self_reduce() + result.x().reduce_mod_target_modulus(); + + // Transfer Fq value result.x() to Fr (this is just moving from a C++ class to another) + Fr result_x_mod_r = Fr::unsafe_construct_from_limbs(result.x().binary_basis_limbs[0].element, + result.x().binary_basis_limbs[1].element, + result.x().binary_basis_limbs[2].element, + result.x().binary_basis_limbs[3].element); // Copy maximum limb values from Fq to Fr: this is needed by the subtraction happening in the == operator for (size_t idx = 0; idx < 4; idx++) { - result_x_mod_r.binary_basis_limbs[idx].maximum_value = result.x.binary_basis_limbs[idx].maximum_value; + result_x_mod_r.binary_basis_limbs[idx].maximum_value = result.x().binary_basis_limbs[idx].maximum_value; } - // Check result.x = r mod n + // Check result.x() = r mod n bool_t is_signature_valid = result_x_mod_r == r; // Logging diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp index ec00b10c91b5..ffed47e71bc3 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp @@ -66,10 +66,10 @@ template class element { element val(native_val); size_t idx = 0; std::array limb_vals; - for (auto& limb : val.x.binary_basis_limbs) { + for (auto& limb : val._x.binary_basis_limbs) { limb_vals[idx++] = limb.element.get_value(); } - for (auto& limb : val.y.binary_basis_limbs) { + for (auto& limb : val._y.binary_basis_limbs) { limb_vals[idx++] = limb.element.get_value(); } BB_ASSERT_EQ(idx, PUBLIC_INPUTS_SIZE); @@ -83,8 +83,8 @@ template class element { */ uint32_t set_public() const { - const uint32_t start_idx = x.set_public(); - y.set_public(); + const uint32_t start_idx = _x.set_public(); + _y.set_public(); return start_idx; } @@ -118,13 +118,13 @@ template class element { if (input.is_point_at_infinity()) { Fq x = Fq::from_witness(ctx, NativeGroup::affine_one.x); Fq y = Fq::from_witness(ctx, NativeGroup::affine_one.y); - out.x = x; - out.y = y; + out._x = x; + out._y = y; } else { Fq x = Fq::from_witness(ctx, input.x); Fq y = Fq::from_witness(ctx, input.y); - out.x = x; - out.y = y; + out._x = x; + out._y = y; } out.set_point_at_infinity(witness_ct(ctx, input.is_point_at_infinity())); @@ -142,17 +142,20 @@ template class element { bool has_circuit_failed = get_context()->failed(); Fq b(get_context(), uint256_t(NativeGroup::curve_b)); - Fq _b = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), b); - Fq _x = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), x); - Fq _y = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), y); + Fq adjusted_b = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), b); + Fq adjusted_x = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), _x); + Fq adjusted_y = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), _y); if constexpr (!NativeGroup::has_a) { // we validate y^2 = x^3 + b by setting "fix_remainder_zero = true" when calling mult_madd - Fq::mult_madd({ _x.sqr(), _y }, { _x, -_y }, { _b }, true); + Fq::mult_madd({ adjusted_x.sqr(), adjusted_y }, { adjusted_x, -adjusted_y }, { adjusted_b }, true); } else { Fq a(get_context(), uint256_t(NativeGroup::curve_a)); - Fq _a = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), a); + Fq adjusted_a = Fq::conditional_assign(is_point_at_infinity(), Fq::zero(), a); // we validate y^2 = x^3 + ax + b by setting "fix_remainder_zero = true" when calling mult_madd - Fq::mult_madd({ _x.sqr(), _x, _y }, { _x, _a, -_y }, { _b }, true); + Fq::mult_madd({ adjusted_x.sqr(), adjusted_x, adjusted_y }, + { adjusted_x, adjusted_a, -adjusted_y }, + { adjusted_b }, + true); } if ((!has_circuit_failed) && (get_context()->failed())) { @@ -166,8 +169,8 @@ template class element { **/ void convert_constant_to_fixed_witness(Builder* builder) { - this->x.convert_constant_to_fixed_witness(builder); - this->y.convert_constant_to_fixed_witness(builder); + this->_x.convert_constant_to_fixed_witness(builder); + this->_y.convert_constant_to_fixed_witness(builder); // Origin tags should be unset after fixing the witness unset_free_witness_tag(); } @@ -178,8 +181,8 @@ template class element { void fix_witness() { // Origin tags should be updated within - this->x.fix_witness(); - this->y.fix_witness(); + this->_x.fix_witness(); + this->_y.fix_witness(); // This is now effectively a constant unset_free_witness_tag(); @@ -219,8 +222,8 @@ template class element { byte_array to_byte_array() const { byte_array result(get_context()); - result.write(y.to_byte_array()); - result.write(x.to_byte_array()); + result.write(_y.to_byte_array()); + result.write(_x.to_byte_array()); return result; } @@ -232,7 +235,7 @@ template class element { element operator-() const { element result(*this); - result.y = -result.y; + result._y = -result._y; return result; } element operator+=(const element& other) @@ -252,7 +255,7 @@ template class element { element conditional_negate(const bool_ct& predicate) const { element result(*this); - result.y = result.y.conditional_negate(predicate); + result._y = result._y.conditional_negate(predicate); return result; } @@ -278,8 +281,8 @@ template class element { BB_ASSERT_NEQ(ctx, nullptr, "biggroup::conditional_select must have a context"); element result(*this); - result.x = result.x.conditional_select(other.x, predicate); - result.y = result.y.conditional_select(other.y, predicate); + result._x = result._x.conditional_select(other._x, predicate); + result._y = result._y.conditional_select(other._y, predicate); result._is_infinity = bool_ct::conditional_assign(predicate, other.is_point_at_infinity(), result.is_point_at_infinity()); return result; @@ -299,15 +302,15 @@ template class element { const std::string msg = "biggroup::incomplete_assert_equal") const { is_point_at_infinity().assert_equal(other.is_point_at_infinity(), msg + " (infinity flag)"); - x.assert_equal(other.x, msg + " (x coordinate)"); - y.assert_equal(other.y, msg + " (y coordinate)"); + _x.assert_equal(other._x, msg + " (x coordinate)"); + _y.assert_equal(other._y, msg + " (y coordinate)"); } element normalize() const { element result(*this); - result.x.reduce_mod_target_modulus(); - result.y.reduce_mod_target_modulus(); + result._x.reduce_mod_target_modulus(); + result._y.reduce_mod_target_modulus(); return result; } element scalar_mul(const Fr& scalar, const size_t max_num_bits = 0) const; @@ -315,11 +318,17 @@ template class element { element reduce() const { element result(*this); - result.x.self_reduce(); - result.y.self_reduce(); + result._x.self_reduce(); + result._y.self_reduce(); return result; } + void assert_coordinates_in_field(const std::string& msg = "biggroup::assert_coordinates_in_field") const + { + _x.assert_is_in_field(msg + " (x coordinate)"); + _y.assert_is_in_field(msg + " (y coordinate)"); + } + element dbl() const; // we use this data structure to add together a sequence of points. @@ -335,8 +344,8 @@ template class element { chain_add_accumulator() = default; explicit chain_add_accumulator(const element& input) - : x3_prev(input.x) - , y3_prev(input.y) + : x3_prev(input._x) + , y3_prev(input._y) , is_element(true) {} chain_add_accumulator(const chain_add_accumulator& other) = default; @@ -359,8 +368,8 @@ template class element { typename NativeGroup::affine_element get_value() const { - uint512_t x_val = x.get_value() % Fq::modulus_u512; - uint512_t y_val = y.get_value() % Fq::modulus_u512; + uint512_t x_val = _x.get_value() % Fq::modulus_u512; + uint512_t y_val = _y.get_value() % Fq::modulus_u512; auto result = typename NativeGroup::affine_element(x_val.lo, y_val.lo); if (is_point_at_infinity().get_value()) { result.self_set_infinity(); @@ -389,32 +398,39 @@ template class element { Builder* get_context() const { - if (x.context != nullptr) { - return x.context; + if (_x.context != nullptr) { + return _x.context; } - if (y.context != nullptr) { - return y.context; + if (_y.context != nullptr) { + return _y.context; } return nullptr; } Builder* get_context(const element& other) const { - if (x.context != nullptr) { - return x.context; + if (_x.context != nullptr) { + return _x.context; } - if (y.context != nullptr) { - return y.context; + if (_y.context != nullptr) { + return _y.context; } - if (other.x.context != nullptr) { - return other.x.context; + if (other._x.context != nullptr) { + return other._x.context; } - if (other.y.context != nullptr) { - return other.y.context; + if (other._y.context != nullptr) { + return other._y.context; } return nullptr; } + // Coordinate accessors (non-owning, const reference) + const Fq& x() const { return _x; } + const Fq& y() const { return _y; } + // BIGGROUP_AUDITTODO: Remove these non-const accessors by adding explicit methods for mutation where required. + Fq& x() { return _x; } + Fq& y() { return _y; } + bool_ct is_point_at_infinity() const { return _is_infinity; } void set_point_at_infinity(const bool_ct& is_infinity, const bool& add_to_used_witnesses = false) { @@ -427,14 +443,14 @@ template class element { void set_origin_tag(OriginTag tag) const { - x.set_origin_tag(tag); - y.set_origin_tag(tag); + _x.set_origin_tag(tag); + _y.set_origin_tag(tag); _is_infinity.set_origin_tag(tag); } OriginTag get_origin_tag() const { - return OriginTag(x.get_origin_tag(), y.get_origin_tag(), _is_infinity.get_origin_tag()); + return OriginTag(_x.get_origin_tag(), _y.get_origin_tag(), _is_infinity.get_origin_tag()); } /** @@ -442,8 +458,8 @@ template class element { */ void unset_free_witness_tag() { - x.unset_free_witness_tag(); - y.unset_free_witness_tag(); + _x.unset_free_witness_tag(); + _y.unset_free_witness_tag(); _is_infinity.unset_free_witness_tag(); } @@ -452,18 +468,17 @@ template class element { */ void set_free_witness_tag() { - x.set_free_witness_tag(); - y.set_free_witness_tag(); + _x.set_free_witness_tag(); + _y.set_free_witness_tag(); _is_infinity.set_free_witness_tag(); } - Fq x; - Fq y; - // For testing purposes only friend class element_test_accessor; private: + Fq _x; + Fq _y; bool_ct _is_infinity; /** @@ -969,7 +984,7 @@ class element_test_accessor { template inline std::ostream& operator<<(std::ostream& os, element const& v) { - return os << "{ " << v.x << " , " << v.y << " }"; + return os << "{ " << v._x << " , " << v._y << " }"; } } // namespace bb::stdlib::element_default diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp index b89bb943d713..f36c1e1d1bd6 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.test.cpp @@ -47,6 +47,7 @@ template class stdlib_biggroup : public testing::Test { using Builder = typename Curve::Builder; using witness_ct = stdlib::witness_t; using bool_ct = stdlib::bool_t; + using field_ct = stdlib::field_t; static constexpr auto EXPECT_CIRCUIT_CORRECTNESS = [](Builder& builder, bool expected_result = true) { info("num gates = ", builder.get_estimated_num_finalized_gates()); @@ -65,22 +66,105 @@ template class stdlib_biggroup : public testing::Test { EXPECT_EQ(a.get_origin_tag(), next_submitted_value_origin_tag); // Tags from members are merged - bool_ct pif = bool_ct(witness_ct(&builder, 0)); + // Create field elements with specific tags before constructing the biggroup element + affine_element input_c(element::random_element()); + auto x = element_ct::BaseField::from_witness(&builder, input_c.x); + auto y = element_ct::BaseField::from_witness(&builder, input_c.y); + auto pif = bool_ct(witness_ct(&builder, false)); + + // Set tags on the individual field elements + x.set_origin_tag(submitted_value_origin_tag); + y.set_origin_tag(challenge_origin_tag); pif.set_origin_tag(next_challenge_tag); - a.x.set_origin_tag(submitted_value_origin_tag); - a.y.set_origin_tag(challenge_origin_tag); - a.set_point_at_infinity(pif); - EXPECT_EQ(a.get_origin_tag(), first_second_third_merged_tag); + + // Construct biggroup element from pre-tagged field elements + element_ct c(x, y, pif); + + // The tag of the biggroup element should be the union of all 3 member tags + EXPECT_EQ(c.get_origin_tag(), first_second_third_merged_tag); #ifndef NDEBUG + // Test that instant_death_tag on x coordinate propagates correctly affine_element input_b(element::random_element()); - // Working with instant death tagged element causes an exception - element_ct b = element_ct::from_witness(&builder, input_b); - b.set_origin_tag(instant_death_tag); + auto x_death = element_ct::BaseField::from_witness(&builder, input_b.x); + auto y_normal = element_ct::BaseField::from_witness(&builder, input_b.y); + auto pif_normal = bool_ct(witness_ct(&builder, false)); + + x_death.set_origin_tag(instant_death_tag); + element_ct b(x_death, y_normal, pif_normal); + // Working with instant death tagged element causes an exception EXPECT_THROW(b + b, std::runtime_error); #endif } + + static void test_assert_coordinates_in_field() + { + // Only test for non-goblin builders (goblin elements don't have assert_coordinates_in_field + // because coordinate checks are done in the ECCVM circuit) + if constexpr (!HasGoblinBuilder) { + // Test 1: Valid coordinates should pass + { + Builder builder; + + // Test multiple random points to ensure assert_coordinates_in_field works correctly + for (size_t i = 0; i < 3; ++i) { + affine_element valid_point(element::random_element()); + element_ct point = element_ct::from_witness(&builder, valid_point); + + // This should not fail - coordinates are in field + point.assert_coordinates_in_field(); + } + + // Verify the circuit is correct + EXPECT_CIRCUIT_CORRECTNESS(builder); + } + + // Test 2: Invalid x coordinate should cause circuit to fail + { + Builder builder; + affine_element valid_point(element::random_element()); + + // Create a bigfield element with x coordinate that will be out of range + // We do this by creating a valid witness but then manipulating the limb values + // to make them represent a value >= the modulus + auto x_coord = element_ct::BaseField::from_witness(&builder, valid_point.x); + auto y_coord = element_ct::BaseField::from_witness(&builder, valid_point.y); + + // Manipulate the limbs to create an invalid value + // Set the highest limb to a very large value that would make the total >= modulus + x_coord.binary_basis_limbs[3].element = field_ct::from_witness(&builder, fr(uint256_t(1) << 68)); + x_coord.binary_basis_limbs[3].maximum_value = uint256_t(1) << 68; + + element_ct point(x_coord, y_coord, bool_ct(witness_ct(&builder, false))); + point.assert_coordinates_in_field(); + + // Circuit should fail because x coordinate is out of field + EXPECT_CIRCUIT_CORRECTNESS(builder, false); + } + + // Test 3: Invalid y coordinate should cause circuit to fail + { + Builder builder; + affine_element valid_point(element::random_element()); + + auto x_coord = element_ct::BaseField::from_witness(&builder, valid_point.x); + auto y_coord = element_ct::BaseField::from_witness(&builder, valid_point.y); + + // Manipulate the limbs to create an invalid value + // Set the highest limb to a very large value that would make the total >= modulus + y_coord.binary_basis_limbs[3].element = field_ct::from_witness(&builder, fr(uint256_t(1) << 68)); + y_coord.binary_basis_limbs[3].maximum_value = uint256_t(1) << 68; + + element_ct point(x_coord, y_coord, bool_ct(witness_ct(&builder, false))); + point.assert_coordinates_in_field(); + + // Circuit should fail because y coordinate is out of field + EXPECT_CIRCUIT_CORRECTNESS(builder, false); + } + } + } + static void test_add() { Builder builder; @@ -109,8 +193,8 @@ template class stdlib_biggroup : public testing::Test { affine_element c_expected(element(input_a) + element(input_b)); - uint256_t c_x_u256 = c.x.get_value().lo; - uint256_t c_y_u256 = c.y.get_value().lo; + uint256_t c_x_u256 = c.x().get_value().lo; + uint256_t c_y_u256 = c.y().get_value().lo; fq c_x_result(c_x_u256); fq c_y_result(c_y_u256); @@ -207,11 +291,11 @@ template class stdlib_biggroup : public testing::Test { EXPECT_EQ(standard_a.is_point_at_infinity().get_value(), true); EXPECT_EQ(standard_b.is_point_at_infinity().get_value(), true); - fq standard_a_x = standard_a.x.get_value().lo; - fq standard_a_y = standard_a.y.get_value().lo; + fq standard_a_x = standard_a.x().get_value().lo; + fq standard_a_y = standard_a.y().get_value().lo; - fq standard_b_x = standard_b.x.get_value().lo; - fq standard_b_y = standard_b.y.get_value().lo; + fq standard_b_x = standard_b.x().get_value().lo; + fq standard_b_y = standard_b.y().get_value().lo; EXPECT_EQ(standard_a_x, 0); EXPECT_EQ(standard_a_y, 0); @@ -243,8 +327,8 @@ template class stdlib_biggroup : public testing::Test { affine_element c_expected(element(input_a) - element(input_b)); - uint256_t c_x_u256 = c.x.get_value().lo; - uint256_t c_y_u256 = c.y.get_value().lo; + uint256_t c_x_u256 = c.x().get_value().lo; + uint256_t c_y_u256 = c.y().get_value().lo; fq c_x_result(c_x_u256); fq c_y_result(c_y_u256); @@ -330,8 +414,8 @@ template class stdlib_biggroup : public testing::Test { affine_element c_expected(element(input_a).dbl()); - uint256_t c_x_u256 = c.x.get_value().lo; - uint256_t c_y_u256 = c.y.get_value().lo; + uint256_t c_x_u256 = c.x().get_value().lo; + uint256_t c_y_u256 = c.y().get_value().lo; fq c_x_result(c_x_u256); fq c_y_result(c_y_u256); @@ -478,23 +562,27 @@ template class stdlib_biggroup : public testing::Test { { Builder builder; affine_element input_a(element::random_element()); - affine_element input_b(element::random_element()); - // Ensure inputs are different - while (input_a == input_b) { - input_b = element::random_element(); - } - element_ct a = element_ct::from_witness(&builder, input_a); - element_ct b = element_ct::from_witness(&builder, input_b); + + // Create a point with the same x coordinate but different y + // For an elliptic curve y^2 = x^3 + ax + b, if (x, y) is on the curve, then (x, -y) is also on the curve + affine_element input_b = input_a; + input_b.y = -input_a.y; // Negate y to get a different point with same x + + // Construct the circuit elements with same x but different y + auto x_coord = element_ct::BaseField::from_witness(&builder, input_a.x); + auto y_coord_a = element_ct::BaseField::from_witness(&builder, input_a.y); + auto y_coord_b = element_ct::BaseField::from_witness(&builder, input_b.y); + + element_ct a(x_coord, y_coord_a, bool_ct(witness_ct(&builder, false))); + element_ct b(x_coord, y_coord_b, bool_ct(witness_ct(&builder, false))); // Set different tags in a and b a.set_origin_tag(submitted_value_origin_tag); b.set_origin_tag(challenge_origin_tag); - // Make the x-coordinates equal, so we should get an error message about y-coordinates - b.x = a.x; a.incomplete_assert_equal(b, "elements don't match"); - // Circuit should fail + // Circuit should fail with y coordinate error EXPECT_EQ(builder.failed(), true); EXPECT_EQ(builder.err(), "elements don't match (y coordinate)"); } @@ -569,8 +657,8 @@ template class stdlib_biggroup : public testing::Test { affine_element c_expected(element(input_a).dbl() + element(input_b)); - uint256_t c_x_u256 = c.x.get_value().lo; - uint256_t c_y_u256 = c.y.get_value().lo; + uint256_t c_x_u256 = c.x().get_value().lo; + uint256_t c_y_u256 = c.y().get_value().lo; fq c_x_result(c_x_u256); fq c_y_result(c_y_u256); @@ -606,8 +694,8 @@ template class stdlib_biggroup : public testing::Test { // Check the result of the multiplication has a tag that's the union of inputs' tags EXPECT_EQ(c.get_origin_tag(), first_two_merged_tag); - fq c_x_result(c.x.get_value().lo); - fq c_y_result(c.y.get_value().lo); + fq c_x_result(c.x().get_value().lo); + fq c_y_result(c.y().get_value().lo); EXPECT_EQ(c_x_result, c_expected.x); EXPECT_EQ(c_y_result, c_expected.y); @@ -651,8 +739,8 @@ template class stdlib_biggroup : public testing::Test { // Check the result of the multiplication has a tag that's the union of inputs' tags EXPECT_EQ(c.get_origin_tag(), first_two_merged_tag); - fq c_x_result(c.x.get_value().lo); - fq c_y_result(c.y.get_value().lo); + fq c_x_result(c.x().get_value().lo); + fq c_y_result(c.y().get_value().lo); EXPECT_EQ(c_x_result, c_expected.x); @@ -691,8 +779,8 @@ template class stdlib_biggroup : public testing::Test { // Check the result of the multiplication has a tag that's the union of inputs' tags EXPECT_EQ(c.get_origin_tag(), first_two_merged_tag); - fq c_x_result(c.x.get_value().lo); - fq c_y_result(c.y.get_value().lo); + fq c_x_result(c.x().get_value().lo); + fq c_y_result(c.y().get_value().lo); EXPECT_EQ(c_x_result, c_expected.x); @@ -786,8 +874,8 @@ template class stdlib_biggroup : public testing::Test { element input_c = (element(input_a) * scalar_a); element input_d = (element(input_b) * scalar_b); affine_element expected(input_c + input_d); - fq c_x_result(c.x.get_value().lo); - fq c_y_result(c.y.get_value().lo); + fq c_x_result(c.x().get_value().lo); + fq c_y_result(c.y().get_value().lo); EXPECT_EQ(c_x_result, expected.x); EXPECT_EQ(c_y_result, expected.y); @@ -848,8 +936,8 @@ template class stdlib_biggroup : public testing::Test { element input_g = (element(input_c) * scalar_c); affine_element expected(input_e + input_f + input_g); - fq c_x_result(c.x.get_value().lo); - fq c_y_result(c.y.get_value().lo); + fq c_x_result(c.x().get_value().lo); + fq c_y_result(c.y().get_value().lo); EXPECT_EQ(c_x_result, expected.x); EXPECT_EQ(c_y_result, expected.y); @@ -924,8 +1012,8 @@ template class stdlib_biggroup : public testing::Test { element input_h = (element(input_d) * scalar_d); affine_element expected(input_e + input_f + input_g + input_h); - fq c_x_result(c.x.get_value().lo); - fq c_y_result(c.y.get_value().lo); + fq c_x_result(c.x().get_value().lo); + fq c_y_result(c.y().get_value().lo); EXPECT_EQ(c_x_result, expected.x); EXPECT_EQ(c_y_result, expected.y); @@ -956,8 +1044,8 @@ template class stdlib_biggroup : public testing::Test { // Check that the resulting tag is a union EXPECT_EQ(c.get_origin_tag(), first_two_merged_tag); affine_element expected(g1::one * scalar_a); - fq c_x_result(c.x.get_value().lo); - fq c_y_result(c.y.get_value().lo); + fq c_x_result(c.x().get_value().lo); + fq c_y_result(c.y().get_value().lo); EXPECT_EQ(c_x_result, expected.x); EXPECT_EQ(c_y_result, expected.y); @@ -1007,8 +1095,8 @@ template class stdlib_biggroup : public testing::Test { } expected_point = expected_point.normalize(); - fq result_x(result_point.x.get_value().lo); - fq result_y(result_point.y.get_value().lo); + fq result_x(result_point.x().get_value().lo); + fq result_y(result_point.y().get_value().lo); EXPECT_EQ(result_x, expected_point.x); EXPECT_EQ(result_y, expected_point.y); @@ -1057,8 +1145,8 @@ template class stdlib_biggroup : public testing::Test { expected_point = expected_point.normalize(); - fq result2_x(result_point2.x.get_value().lo); - fq result2_y(result_point2.y.get_value().lo); + fq result2_x(result_point2.x().get_value().lo); + fq result2_y(result_point2.y().get_value().lo); EXPECT_EQ(result2_x, expected_point.x); EXPECT_EQ(result2_y, expected_point.y); @@ -1111,8 +1199,8 @@ template class stdlib_biggroup : public testing::Test { } expected_point = expected_point.normalize(); - fq result_x(result_point.x.get_value().lo); - fq result_y(result_point.y.get_value().lo); + fq result_x(result_point.x().get_value().lo); + fq result_y(result_point.y().get_value().lo); EXPECT_EQ(result_x, expected_point.x); EXPECT_EQ(result_y, expected_point.y); @@ -1169,8 +1257,8 @@ template class stdlib_biggroup : public testing::Test { element expected_point = points[1]; expected_point = expected_point.normalize(); - fq result_x(result_point.x.get_value().lo); - fq result_y(result_point.y.get_value().lo); + fq result_x(result_point.x().get_value().lo); + fq result_y(result_point.y().get_value().lo); EXPECT_EQ(result_x, expected_point.x); EXPECT_EQ(result_y, expected_point.y); @@ -1217,8 +1305,8 @@ template class stdlib_biggroup : public testing::Test { element expected_point = points[1]; expected_point = expected_point.normalize(); - fq result_x(result_point.x.get_value().lo); - fq result_y(result_point.y.get_value().lo); + fq result_x(result_point.x().get_value().lo); + fq result_y(result_point.y().get_value().lo); EXPECT_EQ(result_x, expected_point.x); EXPECT_EQ(result_y, expected_point.y); @@ -1396,8 +1484,8 @@ template class stdlib_biggroup : public testing::Test { } expected_point = expected_point.normalize(); - fq result_x(result_point.x.get_value().lo); - fq result_y(result_point.y.get_value().lo); + fq result_x(result_point.x().get_value().lo); + fq result_y(result_point.y().get_value().lo); EXPECT_EQ(result_x, expected_point.x); EXPECT_EQ(result_y, expected_point.y); @@ -1477,8 +1565,8 @@ template class stdlib_biggroup : public testing::Test { out += (input4 * scalar4); affine_element c_expected(out); - fq c_x_result(c.x.get_value().lo); - fq c_y_result(c.y.get_value().lo); + fq c_x_result(c.x().get_value().lo); + fq c_y_result(c.y().get_value().lo); EXPECT_EQ(c_x_result, c_expected.x); EXPECT_EQ(c_y_result, c_expected.y); @@ -1522,8 +1610,8 @@ template class stdlib_biggroup : public testing::Test { } expected = expected.normalize(); - fq result_x(double_opening_result.x.get_value().lo); - fq result_y(double_opening_result.y.get_value().lo); + fq result_x(double_opening_result.x().get_value().lo); + fq result_y(double_opening_result.y().get_value().lo); EXPECT_EQ(result_x, expected.x); EXPECT_EQ(result_y, expected.y); @@ -1543,6 +1631,12 @@ TYPED_TEST(stdlib_biggroup, basic_tag_logic) { TestFixture::test_basic_tag_logic(); } + +TYPED_TEST(stdlib_biggroup, assert_coordinates_in_field) +{ + TestFixture::test_assert_coordinates_in_field(); +} + TYPED_TEST(stdlib_biggroup, add) { diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp index 652162845f1c..7c49fbd35d22 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp @@ -47,18 +47,18 @@ template class goblin_el goblin_element() = default; goblin_element(const typename NativeGroup::affine_element& input) - : x(input.x) - , y(input.y) + : _x(input.x) + , _y(input.y) , _is_infinity(input.is_point_at_infinity()) {} goblin_element(const Fq& x, const Fq& y) - : x(x) - , y(y) + : _x(x) + , _y(y) , _is_infinity(false) {} goblin_element(const Fq& x, const Fq& y, const bool_ct is_infinity) - : x(x) - , y(y) + : _x(x) + , _y(y) , _is_infinity(is_infinity) {} goblin_element(const goblin_element& other) = default; @@ -81,8 +81,8 @@ template class goblin_el const std::string msg = "goblin_element::incomplete_assert_equal") const { is_point_at_infinity().assert_equal(other.is_point_at_infinity(), msg + " (infinity flag)"); - x.assert_equal(other.x, msg + " (x coordinate)"); - y.assert_equal(other.y, msg + " (y coordinate)"); + _x.assert_equal(other._x, msg + " (x coordinate)"); + _y.assert_equal(other._y, msg + " (y coordinate)"); } static goblin_element from_witness(Builder* ctx, const typename NativeGroup::affine_element& input) @@ -92,13 +92,13 @@ template class goblin_el if (input.is_point_at_infinity()) { Fq x = Fq::from_witness(ctx, bb::fq(0)); Fq y = Fq::from_witness(ctx, bb::fq(0)); - out.x = x; - out.y = y; + out._x = x; + out._y = y; } else { Fq x = Fq::from_witness(ctx, input.x); Fq y = Fq::from_witness(ctx, input.y); - out.x = x; - out.y = y; + out._x = x; + out._y = y; } out.set_point_at_infinity(witness_t(ctx, input.is_point_at_infinity())); out.set_free_witness_tag(); @@ -110,8 +110,8 @@ template class goblin_el **/ void convert_constant_to_fixed_witness(Builder* builder) { - this->x.convert_constant_to_fixed_witness(builder); - this->y.convert_constant_to_fixed_witness(builder); + this->_x.convert_constant_to_fixed_witness(builder); + this->_y.convert_constant_to_fixed_witness(builder); this->unset_free_witness_tag(); } @@ -121,8 +121,8 @@ template class goblin_el void fix_witness() { // Origin tags should be updated within - this->x.fix_witness(); - this->y.fix_witness(); + this->_x.fix_witness(); + this->_y.fix_witness(); // This is now effectively a constant unset_free_witness_tag(); @@ -184,10 +184,10 @@ template class goblin_el auto x_hi = Fr::from_witness_index(builder, op_tuple.x_hi); auto y_lo = Fr::from_witness_index(builder, op_tuple.y_lo); auto y_hi = Fr::from_witness_index(builder, op_tuple.y_hi); - x_lo.assert_equal(other.x.limbs[0]); - x_hi.assert_equal(other.x.limbs[1]); - y_lo.assert_equal(other.y.limbs[0]); - y_hi.assert_equal(other.y.limbs[1]); + x_lo.assert_equal(other._x.limbs[0]); + x_hi.assert_equal(other._x.limbs[1]); + y_lo.assert_equal(other._y.limbs[0]); + y_hi.assert_equal(other._y.limbs[1]); } // if function queue_ecc_add_accum is used, op_tuple creates as a result of construct_and_populate_ultra_ops // function. In case of queue_ecc_add_accum, scalar is zero, (z_1, z_2) = (scalar, 0) = (0, 0) and they just put @@ -220,10 +220,10 @@ template class goblin_el auto y_lo = Fr::from_witness_index(builder, op_tuple3.y_lo); auto y_hi = Fr::from_witness_index(builder, op_tuple3.y_hi); - x_lo.assert_equal(x.limbs[0]); - x_hi.assert_equal(x.limbs[1]); - y_lo.assert_equal(y.limbs[0]); - y_hi.assert_equal(y.limbs[1]); + x_lo.assert_equal(_x.limbs[0]); + x_hi.assert_equal(_x.limbs[1]); + y_lo.assert_equal(_y.limbs[0]); + y_hi.assert_equal(_y.limbs[1]); } // Set the tag of the result to the union of the tags of inputs @@ -254,7 +254,7 @@ template class goblin_el { goblin_element negated = -(*this); goblin_element result(*this); - result.y = Fq::conditional_assign(predicate, negated.y, result.y); + result._y = Fq::conditional_assign(predicate, negated._y, result._y); return result; } @@ -268,8 +268,8 @@ template class goblin_el goblin_element conditional_select(const goblin_element& other, const bool_ct& predicate) const { goblin_element result(*this); - result.x = Fq::conditional_assign(predicate, other.x, result.x); - result.y = Fq::conditional_assign(predicate, other.y, result.y); + result._x = Fq::conditional_assign(predicate, other._x, result._x); + result._y = Fq::conditional_assign(predicate, other._y, result._y); result._is_infinity = bool_ct::conditional_assign(predicate, other.is_point_at_infinity(), result.is_point_at_infinity()); return result; @@ -302,8 +302,8 @@ template class goblin_el typename NativeGroup::affine_element get_value() const { - bb::fq x_val = x.get_value().lo; - bb::fq y_val = y.get_value().lo; + bb::fq x_val = _x.get_value().lo; + bb::fq y_val = _y.get_value().lo; auto result = typename NativeGroup::affine_element(x_val, y_val); if (is_point_at_infinity().get_value()) { result.self_set_infinity(); @@ -313,28 +313,28 @@ template class goblin_el Builder* get_context() const { - if (x.get_context() != nullptr) { - return x.get_context(); + if (_x.get_context() != nullptr) { + return _x.get_context(); } - if (y.get_context() != nullptr) { - return y.get_context(); + if (_y.get_context() != nullptr) { + return _y.get_context(); } return nullptr; } Builder* get_context(const goblin_element& other) const { - if (x.get_context() != nullptr) { - return x.get_context(); + if (_x.get_context() != nullptr) { + return _x.get_context(); } - if (y.get_context() != nullptr) { - return y.get_context(); + if (_y.get_context() != nullptr) { + return _y.get_context(); } - if (other.x.get_context() != nullptr) { - return other.x.get_context(); + if (other._x.get_context() != nullptr) { + return other._x.get_context(); } - if (other.y.get_context() != nullptr) { - return other.y.get_context(); + if (other._y.get_context() != nullptr) { + return other._y.get_context(); } return nullptr; } @@ -353,20 +353,20 @@ template class goblin_el const bool_ct is_infinity = is_point_at_infinity(); goblin_element result(*this); const Fq zero = Fq::zero(); - result.x = Fq::conditional_assign(is_infinity, zero, result.x); - result.y = Fq::conditional_assign(is_infinity, zero, result.y); + result._x = Fq::conditional_assign(is_infinity, zero, result._x); + result._y = Fq::conditional_assign(is_infinity, zero, result._y); return result; } OriginTag get_origin_tag() const { - return OriginTag(x.get_origin_tag(), y.get_origin_tag(), _is_infinity.get_origin_tag()); + return OriginTag(_x.get_origin_tag(), _y.get_origin_tag(), _is_infinity.get_origin_tag()); } void set_origin_tag(const OriginTag& tag) const { - x.set_origin_tag(tag); - y.set_origin_tag(tag); + _x.set_origin_tag(tag); + _y.set_origin_tag(tag); _is_infinity.set_origin_tag(tag); } @@ -375,8 +375,8 @@ template class goblin_el */ void set_free_witness_tag() { - x.set_free_witness_tag(); - y.set_free_witness_tag(); + _x.set_free_witness_tag(); + _y.set_free_witness_tag(); _is_infinity.set_free_witness_tag(); } @@ -385,8 +385,8 @@ template class goblin_el */ void unset_free_witness_tag() { - x.unset_free_witness_tag(); - y.unset_free_witness_tag(); + _x.unset_free_witness_tag(); + _y.unset_free_witness_tag(); _is_infinity.unset_free_witness_tag(); } /** @@ -400,12 +400,19 @@ template class goblin_el */ uint32_t set_public() const { - const uint32_t start_idx = x.set_public(); - y.set_public(); + const uint32_t start_idx = _x.set_public(); + _y.set_public(); return start_idx; } + // Coordinate accessors (non-owning, const reference) + const Fq& x() const { return _x; } + const Fq& y() const { return _y; } + // Non-const accessors for internal use (e.g., fix_witness in tests) + Fq& x() { return _x; } + Fq& y() { return _y; } + /** * @brief Reconstruct a goblin element from its representation as limbs stored in the public inputs * @details For consistency with biggroup, a goblin element is represented in the public inputs using eight field @@ -423,10 +430,9 @@ template class goblin_el return { Fq::reconstruct_from_public(x_limbs), Fq::reconstruct_from_public(y_limbs) }; } - Fq x; - Fq y; - private: + Fq _x; + Fq _y; bool_ct _is_infinity; }; @@ -438,7 +444,7 @@ using BiggroupGoblin = goblin_element inline std::ostream& operator<<(std::ostream& os, goblin_element const& v) { - return os << "{ " << v.x << " , " << v.y << " }"; + return os << "{ " << v._x << " , " << v._y << " }"; } } // namespace bb::stdlib::element_goblin diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.test.cpp index 6fc3ec6cbea3..865a90abd9ca 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.test.cpp @@ -74,8 +74,8 @@ template class stdlib_biggroup_goblin : public testing::Test { } expected_point = expected_point.normalize(); - fq result_x(result_point.x.get_value().lo); - fq result_y(result_point.y.get_value().lo); + fq result_x(result_point.x().get_value().lo); + fq result_y(result_point.y().get_value().lo); EXPECT_EQ(result_x, expected_point.x); EXPECT_EQ(result_y, expected_point.y); diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin_impl.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin_impl.hpp index 1fc78e13b292..30e245cc8d56 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin_impl.hpp @@ -75,12 +75,12 @@ goblin_element goblin_element::batch_mul(const std:: // Note: These constraints do not assume or enforce that the coordinates of the original point have been // asserted to be in the field, only that they are less than the smallest power of 2 greater than the field // modulus (a la the bigfield(lo, hi) constructor with can_overflow == false). - BB_ASSERT_LTE(uint1024_t(point.x.get_maximum_value()), Fq::DEFAULT_MAXIMUM_REMAINDER); - BB_ASSERT_LTE(uint1024_t(point.y.get_maximum_value()), Fq::DEFAULT_MAXIMUM_REMAINDER); - x_lo.assert_equal(point.x.limbs[0]); - x_hi.assert_equal(point.x.limbs[1]); - y_lo.assert_equal(point.y.limbs[0]); - y_hi.assert_equal(point.y.limbs[1]); + BB_ASSERT_LTE(uint1024_t(point._x.get_maximum_value()), Fq::DEFAULT_MAXIMUM_REMAINDER); + BB_ASSERT_LTE(uint1024_t(point._y.get_maximum_value()), Fq::DEFAULT_MAXIMUM_REMAINDER); + x_lo.assert_equal(point._x.limbs[0]); + x_hi.assert_equal(point._x.limbs[1]); + y_lo.assert_equal(point._y.limbs[0]); + y_hi.assert_equal(point._y.limbs[1]); // Add constraints demonstrating proper decomposition of scalar into endomorphism scalars if (!scalar_is_constant_equal_one) { diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp index 3ebe63734412..69ab2a0eedbc 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp @@ -17,43 +17,43 @@ namespace bb::stdlib::element_default { template element::element() - : x() - , y() + : _x() + , _y() , _is_infinity() {} template element::element(const typename G::affine_element& input) - : x(nullptr, input.x) - , y(nullptr, input.y) + : _x(nullptr, input.x) + , _y(nullptr, input.y) , _is_infinity(nullptr, input.is_point_at_infinity()) {} template element::element(const Fq& x_in, const Fq& y_in) - : x(x_in) - , y(y_in) - , _is_infinity(x.get_context() ? x.get_context() : y.get_context(), false) + : _x(x_in) + , _y(y_in) + , _is_infinity(_x.get_context() ? _x.get_context() : _y.get_context(), false) {} template element::element(const Fq& x_in, const Fq& y_in, const bool_ct& is_infinity) - : x(x_in) - , y(y_in) + : _x(x_in) + , _y(y_in) , _is_infinity(is_infinity) {} template element::element(const element& other) - : x(other.x) - , y(other.y) + : _x(other._x) + , _y(other._y) , _is_infinity(other.is_point_at_infinity()) {} template element::element(element&& other) noexcept - : x(other.x) - , y(other.y) + : _x(other._x) + , _y(other._y) , _is_infinity(other.is_point_at_infinity()) {} @@ -63,8 +63,8 @@ element& element::operator=(const element& other) if (&other == this) { return *this; } - x = other.x; - y = other.y; + _x = other._x; + _y = other._y; _is_infinity = other.is_point_at_infinity(); return *this; } @@ -75,8 +75,8 @@ element& element::operator=(element&& other) noexcep if (&other == this) { return *this; } - x = other.x; - y = other.y; + _x = other._x; + _y = other._y; _is_infinity = other.is_point_at_infinity(); return *this; } @@ -90,8 +90,8 @@ element element::operator+(const element& other) con // If (x_1, y_1), (x_2, y_2) have x_1 == x_2, and the generic formula for lambda has a division by 0. // Then y_1 == y_2 (i.e. we are doubling) or y_2 == y_1 (the sum is infinity). // The cases have a special addition formula. The following booleans allow us to handle these cases uniformly. - const bool_ct x_coordinates_match = other.x == x; - const bool_ct y_coordinates_match = (y == other.y); + const bool_ct x_coordinates_match = other._x == _x; + const bool_ct y_coordinates_match = (_y == other._y); const bool_ct infinity_predicate = (x_coordinates_match && !y_coordinates_match); const bool_ct double_predicate = (x_coordinates_match && y_coordinates_match); const bool_ct lhs_infinity = is_point_at_infinity(); @@ -99,13 +99,13 @@ element element::operator+(const element& other) con const bool_ct has_infinity_input = lhs_infinity || rhs_infinity; // Compute the gradient `lambda`. If we add, `lambda = (y2 - y1)/(x2 - x1)`, else `lambda = 3x1*x1/2y1 - const Fq add_lambda_numerator = other.y - y; - const Fq xx = x * x; + const Fq add_lambda_numerator = other._y - _y; + const Fq xx = _x * _x; const Fq dbl_lambda_numerator = xx + xx + xx; const Fq lambda_numerator = Fq::conditional_assign(double_predicate, dbl_lambda_numerator, add_lambda_numerator); - const Fq add_lambda_denominator = other.x - x; - const Fq dbl_lambda_denominator = y + y; + const Fq add_lambda_denominator = other._x - _x; + const Fq dbl_lambda_denominator = _y + _y; Fq lambda_denominator = Fq::conditional_assign(double_predicate, dbl_lambda_denominator, add_lambda_denominator); // If either inputs are points at infinity, we set lambda_denominator to be 1. This ensures we never trigger a // divide by zero error. @@ -115,16 +115,16 @@ element element::operator+(const element& other) con Fq::conditional_assign(has_infinity_input || infinity_predicate, safe_edgecase_denominator, lambda_denominator); const Fq lambda = Fq::div_without_denominator_check({ lambda_numerator }, lambda_denominator); - const Fq x3 = lambda.sqradd({ -other.x, -x }); - const Fq y3 = lambda.madd(x - x3, { -y }); + const Fq x3 = lambda.sqradd({ -other._x, -_x }); + const Fq y3 = lambda.madd(_x - x3, { -_y }); element result(x3, y3); // if lhs infinity, return rhs - result.x = Fq::conditional_assign(lhs_infinity, other.x, result.x); - result.y = Fq::conditional_assign(lhs_infinity, other.y, result.y); + result._x = Fq::conditional_assign(lhs_infinity, other._x, result._x); + result._y = Fq::conditional_assign(lhs_infinity, other._y, result._y); // if rhs infinity, return lhs - result.x = Fq::conditional_assign(rhs_infinity, x, result.x); - result.y = Fq::conditional_assign(rhs_infinity, y, result.y); + result._x = Fq::conditional_assign(rhs_infinity, _x, result._x); + result._y = Fq::conditional_assign(rhs_infinity, _y, result._y); // is result point at infinity? // yes = infinity_predicate && !lhs_infinity && !rhs_infinity @@ -151,8 +151,8 @@ element element::get_standard_form() const const bool_ct is_infinity = is_point_at_infinity(); element result(*this); const Fq zero = Fq::zero(); - result.x = Fq::conditional_assign(is_infinity, zero, this->x); - result.y = Fq::conditional_assign(is_infinity, zero, this->y); + result._x = Fq::conditional_assign(is_infinity, zero, this->_x); + result._y = Fq::conditional_assign(is_infinity, zero, this->_y); return result; } @@ -162,8 +162,8 @@ element element::operator-(const element& other) con // if x_coordinates match, lambda triggers a divide by zero error. // Adding in `x_coordinates_match` ensures that lambda will always be well-formed - const bool_ct x_coordinates_match = other.x == x; - const bool_ct y_coordinates_match = (y == other.y); + const bool_ct x_coordinates_match = other._x == _x; + const bool_ct y_coordinates_match = (_y == other._y); const bool_ct infinity_predicate = (x_coordinates_match && y_coordinates_match); const bool_ct double_predicate = (x_coordinates_match && !y_coordinates_match); const bool_ct lhs_infinity = is_point_at_infinity(); @@ -171,13 +171,13 @@ element element::operator-(const element& other) con const bool_ct has_infinity_input = lhs_infinity || rhs_infinity; // Compute the gradient `lambda`. If we add, `lambda = (y2 - y1)/(x2 - x1)`, else `lambda = 3x1*x1/2y1 - const Fq add_lambda_numerator = -other.y - y; - const Fq xx = x * x; + const Fq add_lambda_numerator = -other._y - _y; + const Fq xx = _x * _x; const Fq dbl_lambda_numerator = xx + xx + xx; const Fq lambda_numerator = Fq::conditional_assign(double_predicate, dbl_lambda_numerator, add_lambda_numerator); - const Fq add_lambda_denominator = other.x - x; - const Fq dbl_lambda_denominator = y + y; + const Fq add_lambda_denominator = other._x - _x; + const Fq dbl_lambda_denominator = _y + _y; Fq lambda_denominator = Fq::conditional_assign(double_predicate, dbl_lambda_denominator, add_lambda_denominator); // If either inputs are points at infinity, we set lambda_denominator to be 1. This ensures we never trigger // a divide by zero error. (if either inputs are points at infinity we will not use the result of this @@ -187,16 +187,16 @@ element element::operator-(const element& other) con Fq::conditional_assign(has_infinity_input || infinity_predicate, safe_edgecase_denominator, lambda_denominator); const Fq lambda = Fq::div_without_denominator_check({ lambda_numerator }, lambda_denominator); - const Fq x3 = lambda.sqradd({ -other.x, -x }); - const Fq y3 = lambda.madd(x - x3, { -y }); + const Fq x3 = lambda.sqradd({ -other._x, -_x }); + const Fq y3 = lambda.madd(_x - x3, { -_y }); element result(x3, y3); // if lhs infinity, return rhs - result.x = Fq::conditional_assign(lhs_infinity, other.x, result.x); - result.y = Fq::conditional_assign(lhs_infinity, -other.y, result.y); + result._x = Fq::conditional_assign(lhs_infinity, other._x, result._x); + result._y = Fq::conditional_assign(lhs_infinity, -other._y, result._y); // if rhs infinity, return lhs - result.x = Fq::conditional_assign(rhs_infinity, x, result.x); - result.y = Fq::conditional_assign(rhs_infinity, y, result.y); + result._x = Fq::conditional_assign(rhs_infinity, _x, result._x); + result._y = Fq::conditional_assign(rhs_infinity, _y, result._y); // is result point at infinity? // yes = infinity_predicate && !lhs_infinity && !rhs_infinity @@ -212,10 +212,10 @@ element element::operator-(const element& other) con template element element::checked_unconditional_add(const element& other) const { - other.x.assert_is_not_equal(x); - const Fq lambda = Fq::div_without_denominator_check({ other.y, -y }, (other.x - x)); - const Fq x3 = lambda.sqradd({ -other.x, -x }); - const Fq y3 = lambda.madd(x - x3, { -y }); + other._x.assert_is_not_equal(_x); + const Fq lambda = Fq::div_without_denominator_check({ other._y, -_y }, (other._x - _x)); + const Fq x3 = lambda.sqradd({ -other._x, -_x }); + const Fq y3 = lambda.madd(_x - x3, { -_y }); return element(x3, y3); } @@ -223,10 +223,10 @@ template element element::checked_unconditional_subtract(const element& other) const { - other.x.assert_is_not_equal(x); - const Fq lambda = Fq::div_without_denominator_check({ other.y, y }, (other.x - x)); - const Fq x_3 = lambda.sqradd({ -other.x, -x }); - const Fq y_3 = lambda.madd(x_3 - x, { -y }); + other._x.assert_is_not_equal(_x); + const Fq lambda = Fq::div_without_denominator_check({ other._y, _y }, (other._x - _x)); + const Fq x_3 = lambda.sqradd({ -other._x, -_x }); + const Fq y_3 = lambda.madd(x_3 - _x, { -_y }); return element(x_3, y_3); } @@ -252,17 +252,17 @@ std::array, 2> element::checked_unconditiona // TODO(https://github.com/AztecProtocol/barretenberg/issues/971): This will fail when the two elements are // the same even in the case of a valid circuit - other.x.assert_is_not_equal(x); + other._x.assert_is_not_equal(_x); - const Fq denominator = other.x - x; - const Fq x2x1 = -(other.x + x); + const Fq denominator = other._x - _x; + const Fq x2x1 = -(other._x + _x); - const Fq lambda1 = Fq::div_without_denominator_check({ other.y, -y }, denominator); + const Fq lambda1 = Fq::div_without_denominator_check({ other._y, -_y }, denominator); const Fq x_3 = lambda1.sqradd({ x2x1 }); - const Fq y_3 = lambda1.madd(x - x_3, { -y }); - const Fq lambda2 = Fq::div_without_denominator_check({ -other.y, -y }, denominator); + const Fq y_3 = lambda1.madd(_x - x_3, { -_y }); + const Fq lambda2 = Fq::div_without_denominator_check({ -other._y, -_y }, denominator); const Fq x_4 = lambda2.sqradd({ x2x1 }); - const Fq y_4 = lambda2.madd(x - x_4, { -y }); + const Fq y_4 = lambda2.madd(_x - x_4, { -_y }); return { element(x_3, y_3), element(x_4, y_4) }; } @@ -270,19 +270,19 @@ std::array, 2> element::checked_unconditiona template element element::dbl() const { - Fq two_x = x + x; + Fq two_x = _x + _x; if constexpr (G::has_a) { Fq a(get_context(), uint256_t(G::curve_a)); - Fq neg_lambda = Fq::msub_div({ x }, { (two_x + x) }, (y + y), { a }, /*enable_divisor_nz_check*/ false); + Fq neg_lambda = Fq::msub_div({ _x }, { (two_x + _x) }, (_y + _y), { a }, /*enable_divisor_nz_check*/ false); Fq x_3 = neg_lambda.sqradd({ -(two_x) }); - Fq y_3 = neg_lambda.madd(x_3 - x, { -y }); + Fq y_3 = neg_lambda.madd(x_3 - _x, { -_y }); // TODO(suyash): do we handle the point at infinity case here? return element(x_3, y_3); } // TODO(): handle y = 0 case. - Fq neg_lambda = Fq::msub_div({ x }, { (two_x + x) }, (y + y), {}, /*enable_divisor_nz_check*/ false); + Fq neg_lambda = Fq::msub_div({ _x }, { (two_x + _x) }, (_y + _y), {}, /*enable_divisor_nz_check*/ false); Fq x_3 = neg_lambda.sqradd({ -(two_x) }); - Fq y_3 = neg_lambda.madd(x_3 - x, { -y }); + Fq y_3 = neg_lambda.madd(x_3 - _x, { -_y }); element result = element(x_3, y_3); result.set_point_at_infinity(is_point_at_infinity()); return result; @@ -311,13 +311,13 @@ typename element::chain_add_accumulator element::cha const element& p2) { chain_add_accumulator output; - output.x1_prev = p1.x; - output.y1_prev = p1.y; + output.x1_prev = p1._x; + output.y1_prev = p1._y; - p1.x.assert_is_not_equal(p2.x); - const Fq lambda = Fq::div_without_denominator_check({ p2.y, -p1.y }, (p2.x - p1.x)); + p1._x.assert_is_not_equal(p2._x); + const Fq lambda = Fq::div_without_denominator_check({ p2._y, -p1._y }, (p2._x - p1._x)); - const Fq x3 = lambda.sqradd({ -p2.x, -p1.x }); + const Fq x3 = lambda.sqradd({ -p2._x, -p1._x }); output.x3_prev = x3; output.lambda_prev = lambda; return output; @@ -332,7 +332,7 @@ typename element::chain_add_accumulator element::cha return chain_add_start(p1, element(acc.x3_prev, acc.y3_prev)); } // validate we can use incomplete addition formulae - p1.x.assert_is_not_equal(acc.x3_prev); + p1._x.assert_is_not_equal(acc.x3_prev); // lambda = (y2 - y1) / (x2 - x1) // but we don't have y2! @@ -355,15 +355,15 @@ typename element::chain_add_accumulator element::cha const auto lambda = Fq::msub_div({ acc.lambda_prev }, { (x2 - acc.x1_prev) }, - (x2 - p1.x), - { acc.y1_prev, p1.y }, + (x2 - p1._x), + { acc.y1_prev, p1._y }, /*enable_divisor_nz_check*/ false); // divisor is non-zero as x2 != p1.x is enforced - const auto x3 = lambda.sqradd({ -x2, -p1.x }); + const auto x3 = lambda.sqradd({ -x2, -p1._x }); chain_add_accumulator output; output.x3_prev = x3; - output.x1_prev = p1.x; - output.y1_prev = p1.y; + output.x1_prev = p1._x; + output.y1_prev = p1._y; output.lambda_prev = lambda; return output; @@ -428,16 +428,16 @@ element element::chain_add_end(const chain_add_accum template element element::montgomery_ladder(const element& other) const { - other.x.assert_is_not_equal(x); - const Fq lambda_1 = Fq::div_without_denominator_check({ other.y - y }, (other.x - x)); + other._x.assert_is_not_equal(_x); + const Fq lambda_1 = Fq::div_without_denominator_check({ other._y - _y }, (other._x - _x)); - const Fq x_3 = lambda_1.sqradd({ -other.x, -x }); + const Fq x_3 = lambda_1.sqradd({ -other._x, -_x }); - const Fq minus_lambda_2 = lambda_1 + Fq::div_without_denominator_check({ y + y }, (x_3 - x)); + const Fq minus_lambda_2 = lambda_1 + Fq::div_without_denominator_check({ _y + _y }, (x_3 - _x)); - const Fq x_4 = minus_lambda_2.sqradd({ -x, -x_3 }); + const Fq x_4 = minus_lambda_2.sqradd({ -_x, -x_3 }); - const Fq y_4 = minus_lambda_2.madd(x_4 - x, { -y }); + const Fq y_4 = minus_lambda_2.madd(x_4 - _x, { -_y }); return element(x_4, y_4); } @@ -467,7 +467,7 @@ element element::montgomery_ladder(const chain_add_a if (to_add.is_element) { throw_or_abort("An accumulator expected"); } - x.assert_is_not_equal(to_add.x3_prev); + _x.assert_is_not_equal(to_add.x3_prev); // lambda = (y2 - y1) / (x2 - x1) // but we don't have y2! @@ -480,16 +480,16 @@ element element::montgomery_ladder(const chain_add_a auto& x2 = to_add.x3_prev; const auto lambda = Fq::msub_div({ to_add.lambda_prev }, { (x2 - to_add.x1_prev) }, - (x2 - x), - { to_add.y1_prev, y }, + (x2 - _x), + { to_add.y1_prev, _y }, /*enable_divisor_nz_check*/ false); // divisor is non-zero as x2 != x is enforced - const auto x3 = lambda.sqradd({ -x2, -x }); + const auto x3 = lambda.sqradd({ -x2, -_x }); - const Fq minus_lambda_2 = lambda + Fq::div_without_denominator_check({ y + y }, (x3 - x)); + const Fq minus_lambda_2 = lambda + Fq::div_without_denominator_check({ _y + _y }, (x3 - _x)); - const Fq x4 = minus_lambda_2.sqradd({ -x, -x3 }); + const Fq x4 = minus_lambda_2.sqradd({ -_x, -x3 }); - const Fq y4 = minus_lambda_2.madd(x4 - x, { -y }); + const Fq y4 = minus_lambda_2.madd(x4 - _x, { -_y }); return element(x4, y4); } @@ -523,7 +523,7 @@ element element::multiple_montgomery_ladder( bool is_negative = false; }; - Fq previous_x = x; + Fq previous_x = _x; composite_y previous_y{ std::vector(), std::vector(), std::vector(), false }; for (size_t i = 0; i < add.size(); ++i) { previous_x.assert_is_not_equal(add[i].x3_prev); @@ -535,7 +535,7 @@ element element::multiple_montgomery_ladder( std::vector lambda1_add; if (i == 0) { - lambda1_add.emplace_back(-y); + lambda1_add.emplace_back(-_y); } else { lambda1_left = previous_y.mul_left; lambda1_right = previous_y.mul_right; @@ -568,7 +568,7 @@ element element::multiple_montgomery_ladder( lambda1_add, /*enable_divisor_nz_check*/ false); // divisor is non-zero as previous_x != add[i].x3_prev is enforced } else { - lambda1 = Fq::div_without_denominator_check({ add[i].y3_prev - y }, (add[i].x3_prev - x)); + lambda1 = Fq::div_without_denominator_check({ add[i].y3_prev - _y }, (add[i].x3_prev - _x)); } Fq x_3 = lambda1.madd(lambda1, { -add[i].x3_prev, -previous_x }); @@ -580,7 +580,7 @@ element element::multiple_montgomery_ladder( // field multiplication Fq lambda2; if (i == 0) { - lambda2 = Fq::div_without_denominator_check({ y + y }, (previous_x - x_3)) - lambda1; + lambda2 = Fq::div_without_denominator_check({ _y + _y }, (previous_x - x_3)) - lambda1; } else { Fq l2_denominator = previous_y.is_negative ? previous_x - x_3 : x_3 - previous_x; // TODO(): analyse if l2_denominator can be zero. @@ -600,7 +600,7 @@ element element::multiple_montgomery_ladder( // Each iteration flips the sign of y_previous.is_negative. // i.e. whether we store y_4 or -y_4 depends on the number of points we have bool num_points_even = ((add.size() & 0x01UL) == 0); - y_4.add.emplace_back(num_points_even ? y : -y); + y_4.add.emplace_back(num_points_even ? _y : -_y); y_4.mul_left.emplace_back(lambda2); y_4.mul_right.emplace_back(num_points_even ? x_4 - previous_x : previous_x - x_4); y_4.is_negative = num_points_even; @@ -814,8 +814,8 @@ element element::scalar_mul(const Fr& scalar, const element result = element::batch_mul({ *this }, { scalar }, max_num_bits, /*with_edgecases=*/false); // Handle point at infinity - result.x = Fq::conditional_assign(is_point_at_infinity, x, result.x); - result.y = Fq::conditional_assign(is_point_at_infinity, y, result.y); + result._x = Fq::conditional_assign(is_point_at_infinity, _x, result._x); + result._y = Fq::conditional_assign(is_point_at_infinity, _y, result._y); result.set_point_at_infinity(is_point_at_infinity); diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.hpp index 381aaa3e7c66..8126100137c4 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.hpp @@ -126,7 +126,7 @@ element element::secp256k1_ecdsa_mul(const element& const bool_ct& positive_skew, const bool_ct& negative_skew) { auto to_add = base_point; - to_add.y = to_add.y.conditional_negate(negative_skew); + to_add._y = to_add._y.conditional_negate(negative_skew); element result = accumulator + to_add; // when computing the wNAF we have already validated that positive_skew and negative_skew cannot both be true diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.test.cpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.test.cpp index 9feef0809368..5fea2754c354 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.test.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_secp256k1.test.cpp @@ -250,8 +250,8 @@ template class stdlibBiggroupSecp256k1 : public testing::Test { auto output = element_ct::secp256k1_ecdsa_mul(P_a, u1, u2); auto expected = affine_element(g1::one * (scalar_c * scalar_b) + g1::one * scalar_a); - EXPECT_EQ(output.x.get_value().lo, uint256_t(expected.x)); - EXPECT_EQ(output.y.get_value().lo, uint256_t(expected.y)); + EXPECT_EQ(output.x().get_value().lo, uint256_t(expected.x)); + EXPECT_EQ(output.y().get_value().lo, uint256_t(expected.y)); } EXPECT_CIRCUIT_CORRECTNESS(builder); @@ -278,13 +278,13 @@ template class stdlibBiggroupSecp256k1 : public testing::Test { // After adding the u2_low skew (i.e., its base point), we get the point at infinity. Then we handle the // u2 high skew as follows: // result = acc ± u1_high_base_point - // result.x = u2_high_skew ? result.x : acc.x; - // result.y = u2_high_skew ? result.y : acc.y; + // result.x() = u2_high_skew ? result.x() : acc.x(); + // result.y() = u2_high_skew ? result.y() : acc.y(); // // However, we did not set the flag _is_point_at_infinity for result. We must copy the flag from the // accumulator in this case, i.e., we must do: - // result.x = u2_high_skew ? result.x : acc.x; - // result.y = u2_high_skew ? result.y : acc.y; + // result.x() = u2_high_skew ? result.x() : acc.x(); + // result.y() = u2_high_skew ? result.y() : acc.y(); // result._is_point_at_infinity = u2_high_skew ? result._is_point_at_infinity : // acc._is_point_at_infinity; // diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_tables.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_tables.hpp index 978d468a601a..2e427042c0f0 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_tables.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_tables.hpp @@ -40,25 +40,25 @@ std::array, Fq::NUM_LIMBS + 1> element::create_g std::vector, 2>> prime_limbs; for (size_t i = 0; i < num_elements; ++i) { - limb_max[0] = std::max(limb_max[0], rom_data[i].x.binary_basis_limbs[0].maximum_value); - limb_max[1] = std::max(limb_max[1], rom_data[i].x.binary_basis_limbs[1].maximum_value); - limb_max[2] = std::max(limb_max[2], rom_data[i].x.binary_basis_limbs[2].maximum_value); - limb_max[3] = std::max(limb_max[3], rom_data[i].x.binary_basis_limbs[3].maximum_value); - limb_max[4] = std::max(limb_max[4], rom_data[i].y.binary_basis_limbs[0].maximum_value); - limb_max[5] = std::max(limb_max[5], rom_data[i].y.binary_basis_limbs[1].maximum_value); - limb_max[6] = std::max(limb_max[6], rom_data[i].y.binary_basis_limbs[2].maximum_value); - limb_max[7] = std::max(limb_max[7], rom_data[i].y.binary_basis_limbs[3].maximum_value); - - x_lo_limbs.emplace_back(std::array, 2>{ rom_data[i].x.binary_basis_limbs[0].element, - rom_data[i].x.binary_basis_limbs[1].element }); - x_hi_limbs.emplace_back(std::array, 2>{ rom_data[i].x.binary_basis_limbs[2].element, - rom_data[i].x.binary_basis_limbs[3].element }); - y_lo_limbs.emplace_back(std::array, 2>{ rom_data[i].y.binary_basis_limbs[0].element, - rom_data[i].y.binary_basis_limbs[1].element }); - y_hi_limbs.emplace_back(std::array, 2>{ rom_data[i].y.binary_basis_limbs[2].element, - rom_data[i].y.binary_basis_limbs[3].element }); + limb_max[0] = std::max(limb_max[0], rom_data[i]._x.binary_basis_limbs[0].maximum_value); + limb_max[1] = std::max(limb_max[1], rom_data[i]._x.binary_basis_limbs[1].maximum_value); + limb_max[2] = std::max(limb_max[2], rom_data[i]._x.binary_basis_limbs[2].maximum_value); + limb_max[3] = std::max(limb_max[3], rom_data[i]._x.binary_basis_limbs[3].maximum_value); + limb_max[4] = std::max(limb_max[4], rom_data[i]._y.binary_basis_limbs[0].maximum_value); + limb_max[5] = std::max(limb_max[5], rom_data[i]._y.binary_basis_limbs[1].maximum_value); + limb_max[6] = std::max(limb_max[6], rom_data[i]._y.binary_basis_limbs[2].maximum_value); + limb_max[7] = std::max(limb_max[7], rom_data[i]._y.binary_basis_limbs[3].maximum_value); + + x_lo_limbs.emplace_back(std::array, 2>{ rom_data[i]._x.binary_basis_limbs[0].element, + rom_data[i]._x.binary_basis_limbs[1].element }); + x_hi_limbs.emplace_back(std::array, 2>{ rom_data[i]._x.binary_basis_limbs[2].element, + rom_data[i]._x.binary_basis_limbs[3].element }); + y_lo_limbs.emplace_back(std::array, 2>{ rom_data[i]._y.binary_basis_limbs[0].element, + rom_data[i]._y.binary_basis_limbs[1].element }); + y_hi_limbs.emplace_back(std::array, 2>{ rom_data[i]._y.binary_basis_limbs[2].element, + rom_data[i]._y.binary_basis_limbs[3].element }); prime_limbs.emplace_back( - std::array, 2>{ rom_data[i].x.prime_basis_limb, rom_data[i].y.prime_basis_limb }); + std::array, 2>{ rom_data[i]._x.prime_basis_limb, rom_data[i]._y.prime_basis_limb }); } std::array, Fq::NUM_LIMBS + 1> output_tables; output_tables[0] = twin_rom_table(x_lo_limbs); @@ -387,13 +387,13 @@ element::create_endo_pair_four_bit_table_plookup(const element& in P1.element_table[i] = (-P1.element_table[15 - i]); } for (size_t i = 0; i < 16; ++i) { - endoP1.element_table[i].y = P1.element_table[15 - i].y; + endoP1.element_table[i]._y = P1.element_table[15 - i]._y; } uint256_t beta_val = bb::field::cube_root_of_unity(); Fq beta(bb::fr(beta_val.slice(0, 136)), bb::fr(beta_val.slice(136, 256))); for (size_t i = 0; i < 8; ++i) { - endoP1.element_table[i].x = P1.element_table[i].x * beta; - endoP1.element_table[15 - i].x = endoP1.element_table[i].x; + endoP1.element_table[i]._x = P1.element_table[i]._x * beta; + endoP1.element_table[15 - i]._x = endoP1.element_table[i]._x; } P1.coordinates = create_group_element_rom_tables<16>(P1.element_table, P1.limb_max); endoP1.coordinates = create_group_element_rom_tables<16>(endoP1.element_table, endoP1.limb_max); diff --git a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp index d85843f8c873..0429bc7d1971 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.hpp @@ -253,7 +253,7 @@ template class StdlibCodec { return convert_grumpkin_fr_to_bn254_frs(val); } else if constexpr (IsAnyOf>) { return convert_goblin_fr_to_bn254_frs(val); - } else if constexpr (IsAnyOf) { + } else if constexpr (IsAnyOf) { using BaseField = typename T::BaseField; std::vector fr_vec_x = serialize_to_fields(val.x()); @@ -261,14 +261,6 @@ template class StdlibCodec { std::vector fr_vec(fr_vec_x.begin(), fr_vec_x.end()); fr_vec.insert(fr_vec.end(), fr_vec_y.begin(), fr_vec_y.end()); return fr_vec; - } else if constexpr (IsAnyOf) { - using BaseField = typename T::BaseField; - - std::vector fr_vec_x = serialize_to_fields(val.x); - std::vector fr_vec_y = serialize_to_fields(val.y); - std::vector fr_vec(fr_vec_x.begin(), fr_vec_x.end()); - fr_vec.insert(fr_vec.end(), fr_vec_y.begin(), fr_vec_y.end()); - return fr_vec; } else { // Array or Univariate std::vector fr_vec; diff --git a/barretenberg/cpp/src/barretenberg/stdlib/translator_vm_verifier/translator_recursive_verifier.cpp b/barretenberg/cpp/src/barretenberg/stdlib/translator_vm_verifier/translator_recursive_verifier.cpp index 9c21a26c275c..c80ee56ba5d2 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib/translator_vm_verifier/translator_recursive_verifier.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib/translator_vm_verifier/translator_recursive_verifier.cpp @@ -220,8 +220,8 @@ void TranslatorRecursiveVerifier::verify_consistency_with_final_merge( // These are witness commitments sent as part of the proof, so their coordinates are already in reduced form. // This approach is preferred over implementing assert_equal for biggroup, as it avoids the need to handle // constants within biggroup logic. - bool consistency_check_failed = (merge_commitment.y.get_value() != translator_commitment.y.get_value()) || - (merge_commitment.y.get_value() != translator_commitment.y.get_value()) || + bool consistency_check_failed = (merge_commitment.y().get_value() != translator_commitment.y().get_value()) || + (merge_commitment.y().get_value() != translator_commitment.y().get_value()) || (merge_commitment.is_point_at_infinity().get_value() != translator_commitment.is_point_at_infinity().get_value()); @@ -229,8 +229,8 @@ void TranslatorRecursiveVerifier::verify_consistency_with_final_merge( vinfo("translator commitments are inconsistent with the final merge commitments"); } - merge_commitment.x.assert_equal(translator_commitment.x); - merge_commitment.y.assert_equal(translator_commitment.y); + merge_commitment.x().assert_equal(translator_commitment.x()); + merge_commitment.y().assert_equal(translator_commitment.y()); merge_commitment.is_point_at_infinity().assert_equal(translator_commitment.is_point_at_infinity()); } } diff --git a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_circuit_builder.cpp b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_circuit_builder.cpp index 8fcaba950327..3f52eaccfe05 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_circuit_builder.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/ultra_circuit_builder.cpp @@ -251,7 +251,11 @@ void UltraCircuitBuilder_::create_big_mul_add_gate(const mul_qua { this->assert_valid_variables({ in.a, in.b, in.c, in.d }); blocks.arithmetic.populate_wires(in.a, in.b, in.c, in.d); - blocks.arithmetic.q_m().emplace_back(include_next_gate_w_4 ? in.mul_scaling * FF(2) : in.mul_scaling); + // If include_next_gate_w_4 is true then we set q_arith = 2. In this case, the linear term in the ArithmeticRelation + // is scaled by a factor of 2. We compensate here by scaling the quadratic term by 2 to achieve the constraint: + // 2 * [q_m * w_1 * w_2 + \sum_{i=1..4} q_i * w_i + q_c + w_4_shift] = 0 + const FF mul_scaling = include_next_gate_w_4 ? in.mul_scaling * FF(2) : in.mul_scaling; + blocks.arithmetic.q_m().emplace_back(mul_scaling); blocks.arithmetic.q_1().emplace_back(in.a_scaling); blocks.arithmetic.q_2().emplace_back(in.b_scaling); blocks.arithmetic.q_3().emplace_back(in.c_scaling); @@ -1661,13 +1665,16 @@ std::array UltraCircuitBuilder_::evaluate_non_nativ block.populate_wires(x_2, y_2, z_2, z_1); block.populate_wires(x_3, y_3, z_3, this->zero_idx()); + // When q_arith == 3, w_4_shift is scaled by 2 (see ArithmeticRelation for details). Therefore, for consistency we + // also scale each linear term by this factor of 2 so that the constraint is effectively: + // (q_l * w_1) + (q_r * w_2) + (q_o * w_3) + (q_4 * w_4) + q_c + w_4_shift = 0 + const FF linear_term_scale_factor = 2; block.q_m().emplace_back(addconstp); block.q_1().emplace_back(0); - block.q_2().emplace_back(-x_mulconst0 * - 2); // scale constants by 2. If q_arith = 3 then w_4_omega value (z0) gets scaled by 2x - block.q_3().emplace_back(-y_mulconst0 * 2); // z_0 - (x_0 * -xmulconst0) - (y_0 * ymulconst0) = 0 => z_0 = x_0 + y_0 + block.q_2().emplace_back(-x_mulconst0 * linear_term_scale_factor); + block.q_3().emplace_back(-y_mulconst0 * linear_term_scale_factor); block.q_4().emplace_back(0); - block.q_c().emplace_back(-addconst0 * 2); + block.q_c().emplace_back(-addconst0 * linear_term_scale_factor); block.set_gate_selector(3); block.q_m().emplace_back(0); @@ -1773,12 +1780,16 @@ std::array UltraCircuitBuilder_::evaluate_non_nativ block.populate_wires(x_2, y_2, z_2, z_1); block.populate_wires(x_3, y_3, z_3, this->zero_idx()); + // When q_arith == 3, w_4_shift is scaled by 2 (see ArithmeticRelation for details). Therefore, for consistency we + // also scale each linear term by this factor of 2 so that the constraint is effectively: + // (q_l * w_1) + (q_r * w_2) + (q_o * w_3) + (q_4 * w_4) + q_c + w_4_shift = 0 + const FF linear_term_scale_factor = 2; block.q_m().emplace_back(-addconstp); block.q_1().emplace_back(0); - block.q_2().emplace_back(-x_mulconst0 * 2); - block.q_3().emplace_back(y_mulconst0 * 2); // z_0 + (x_0 * -xmulconst0) + (y_0 * ymulconst0) = 0 => z_0 = x_0 - y_0 + block.q_2().emplace_back(-x_mulconst0 * linear_term_scale_factor); + block.q_3().emplace_back(y_mulconst0 * linear_term_scale_factor); block.q_4().emplace_back(0); - block.q_c().emplace_back(-addconst0 * 2); + block.q_c().emplace_back(-addconst0 * linear_term_scale_factor); block.set_gate_selector(3); block.q_m().emplace_back(0);