Skip to content

Commit

Permalink
fix: Intermittent invert 0 in Goblin (#5174)
Browse files Browse the repository at this point in the history
We systematize the work of
#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:
AztecProtocol/barretenberg#905

The PR allowed the acir test to be run more than 512 times without any
issue.
  • Loading branch information
codygunton authored Mar 13, 2024
1 parent f518d34 commit 3e68b49
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,19 @@ class ECCOpQueue {

std::array<Point, 4> 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; }

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ template <class Flavor> 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<Flavor>) {
circuit.op_queue->append_nonzero_ops();
}

dyadic_circuit_size = compute_dyadic_size(circuit);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<bb::ECCOpQueue>();

// 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);
Expand Down

0 comments on commit 3e68b49

Please sign in to comment.