From 3e68b49f717aa643eb616976f6cc7ed0ac07686d Mon Sep 17 00:00:00 2001 From: Cody Gunton Date: Wed, 13 Mar 2024 07:48:35 -0400 Subject: [PATCH] fix: Intermittent invert 0 in Goblin (#5174) We systematize the work of https://github.com/AztecProtocol/aztec-packages/pull/4751 to avoid scalar multiplication by 0, an issue that has arisen in various ways. In our case, the acir test `6_array` was reliably failing (failing the with `Trying to invert zero in the field`) about 1/256 of the time. The error was thrown from https://github.com/AztecProtocol/aztec-packages/blob/394a0e06928946c1c9eea1bdfec39269cb2d601a/barretenberg/cpp/src/barretenberg/ecc/groups/element_impl.hpp#L68 coming from this line in Zerormorph verification https://github.com/AztecProtocol/aztec-packages/blob/394a0e06928946c1c9eea1bdfec39269cb2d601a/barretenberg/cpp/src/barretenberg/commitment_schemes/zeromorph/zeromorph.hpp#L622 Spawning an issue to handle this situation more systmatically: https://github.com/AztecProtocol/barretenberg/issues/905 The PR allowed the acir test to be run more than 512 times without any issue. --- .../proof_system/op_queue/ecc_op_queue.hpp | 13 +++++++++++++ .../sumcheck/instance/prover_instance.hpp | 3 +++ .../goblin_translator_composer.test.cpp | 12 ------------ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp b/barretenberg/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp index 7afee28f38f..0697e46fedc 100644 --- a/barretenberg/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp +++ b/barretenberg/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp @@ -46,6 +46,19 @@ class ECCOpQueue { std::array ultra_ops_commitments; + // TODO(https://github.com/AztecProtocol/barretenberg/issues/905): Can remove this with better handling of scalar + // mul against 0 + void append_nonzero_ops() + { + // Add an element and scalar the accumulation of which leaves no Point-at-Infinity commitments + const auto x = uint256_t(0xd3c208c16d87cfd3, 0xd97816a916871ca8, 0x9b85045b68181585, 0x30644e72e131a02); + const auto y = uint256_t(0x3ce1cc9c7e645a83, 0x2edac647851e3ac5, 0xd0cbe61fced2bc53, 0x1a76dae6d3272396); + auto padding_element = Point(x, y); + auto padding_scalar = -Fr::one(); + mul_accumulate(padding_element, padding_scalar); + eq(); + } + Point get_accumulator() { return accumulator; } /** diff --git a/barretenberg/cpp/src/barretenberg/sumcheck/instance/prover_instance.hpp b/barretenberg/cpp/src/barretenberg/sumcheck/instance/prover_instance.hpp index 4c4d62cb177..62f9cca22af 100644 --- a/barretenberg/cpp/src/barretenberg/sumcheck/instance/prover_instance.hpp +++ b/barretenberg/cpp/src/barretenberg/sumcheck/instance/prover_instance.hpp @@ -53,6 +53,9 @@ template class ProverInstance_ { BB_OP_COUNT_TIME_NAME("ProverInstance(Circuit&)"); circuit.add_gates_to_ensure_all_polys_are_non_zero(); circuit.finalize_circuit(); + if constexpr (IsGoblinFlavor) { + circuit.op_queue->append_nonzero_ops(); + } dyadic_circuit_size = compute_dyadic_size(circuit); diff --git a/barretenberg/cpp/src/barretenberg/translator_vm/goblin_translator_composer.test.cpp b/barretenberg/cpp/src/barretenberg/translator_vm/goblin_translator_composer.test.cpp index fb1c9216794..0bd890ccd34 100644 --- a/barretenberg/cpp/src/barretenberg/translator_vm/goblin_translator_composer.test.cpp +++ b/barretenberg/cpp/src/barretenberg/translator_vm/goblin_translator_composer.test.cpp @@ -49,24 +49,12 @@ TEST_F(GoblinTranslatorComposerTests, Basic) using Fr = fr; using Fq = fq; - // Add an element and scalar the accumulation of which leaves no Point-at-Infinity commitments - const auto x = uint256_t(0xd3c208c16d87cfd3, 0xd97816a916871ca8, 0x9b85045b68181585, 0x30644e72e131a02); - const auto y = uint256_t(0x3ce1cc9c7e645a83, 0x2edac647851e3ac5, 0xd0cbe61fced2bc53, 0x1a76dae6d3272396); - auto padding_element = G1(x, y); - auto padding_scalar = -Fr::one(); - auto P1 = G1::random_element(); auto P2 = G1::random_element(); auto z = Fr::random_element(); // Add the same operations to the ECC op queue; the native computation is performed under the hood. auto op_queue = std::make_shared(); - - // Accumulate padding so that we don't produce Point-at-Infinity commitments. Currently our transcript can't handle - // them - op_queue->mul_accumulate(padding_element, padding_scalar); - - // Push everything else for (size_t i = 0; i < 500; i++) { op_queue->add_accumulate(P1); op_queue->mul_accumulate(P2, z);