Skip to content
2 changes: 1 addition & 1 deletion barretenberg/cpp/src/barretenberg/goblin/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
barretenberg_module(goblin ultra_honk eccvm translator_vm stdlib_honk_verifier stdlib_pedersen_commitment stdlib_sha256 )
barretenberg_module(goblin ultra_honk eccvm translator_vm stdlib_honk_verifier stdlib_pedersen_commitment stdlib_sha256 )
8 changes: 2 additions & 6 deletions barretenberg/cpp/src/barretenberg/goblin/goblin.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,8 @@ class GoblinProver {

public:
GoblinProver(const std::shared_ptr<CommitmentKey<curve::BN254>>& bn254_commitment_key = nullptr)
{ // Mocks the interaction of a first circuit with the op queue due to the inability to currently handle zero
// commitments (https://github.com/AztecProtocol/barretenberg/issues/871) which would otherwise appear in the
// first round of the merge protocol. To be removed once the issue has been resolved.
commitment_key = bn254_commitment_key ? bn254_commitment_key : nullptr;
GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);
}
: commitment_key(bn254_commitment_key)
{}

/**
* @brief Construct a merge proof for the goblin ECC ops in the provided circuit
Expand Down
29 changes: 0 additions & 29 deletions barretenberg/cpp/src/barretenberg/goblin/mock_circuits.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,35 +117,6 @@ class GoblinMockCircuits {
stdlib::generate_ecdsa_verification_test_circuit(builder, 1);
stdlib::generate_merkle_membership_test_circuit(builder, 10);
}

// TODO(https://github.com/AztecProtocol/barretenberg/issues/911): We require goblin ops to be added to the
// function circuit because we cannot support zero commtiments. While the builder handles this at
// DeciderProvingKey creation stage via the add_gates_to_ensure_all_polys_are_non_zero function for other
// MegaHonk circuits (where we don't explicitly need to add goblin ops), in ClientIVC merge proving happens
// prior to folding where the absense of goblin ecc ops will result in zero commitments.
MockCircuits::construct_goblin_ecc_op_circuit(builder);
}

/**
* @brief Mock the interactions of a simple curcuit with the op_queue
* @todo The transcript aggregation protocol in the Goblin proof system can not yet support an empty "previous
* transcript" (see issue #723) because the corresponding commitments are zero / the point at infinity. This
* function mocks the interactions with the op queue of a fictional "first" circuit. This way, when we go to
* generate a proof over our first "real" circuit, the transcript aggregation protocol can proceed nominally.
* The mock data is valid in the sense that it can be processed by all stages of Goblin as if it came from a
* genuine circuit.
*
*
* @param op_queue
*/
static void perform_op_queue_interactions_for_mock_first_circuit(std::shared_ptr<bb::ECCOpQueue>& op_queue)
{
PROFILE_THIS();

bb::MegaCircuitBuilder builder{ op_queue };

// Add some goblinized ecc ops
MockCircuits::construct_goblin_ecc_op_circuit(builder);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ template <typename FF_> class TranslatorDeltaRangeConstraintRelationImpl {

// 1 + polynomial degree of this relation
static constexpr size_t RELATION_LENGTH =
7; // degree((lagrange_real_last - 1)(lagrange_masking - 1) * D(D - 1)(D - 2)(D - 3)) = 5
7; // degree((lagrange_real_last - 1)(lagrange_masking - 1) * D(D - 1)(D - 2)(D - 3)) = 6

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to commit this in my previous PR

static constexpr std::array<size_t, 10> SUBRELATION_PARTIAL_LENGTHS{
7, // ordered_range_constraints_0 step in {0,1,2,3} subrelation
Expand Down
2 changes: 0 additions & 2 deletions barretenberg/cpp/src/barretenberg/srs/global_crs.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

namespace bb::srs {

// The call comes from GoblinProver ctor and call to perform_op_queue_interactions_for_mock_first_circuit.
// Delete the ifdef block in fn above once resolved.
inline std::string get_ignition_crs_path()
{
const char* env_var = std::getenv("IGNITION_CRS_PATH");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ barretenberg_module(
stdlib_honk_verifier
stdlib_eccvm_verifier
stdlib_translator_vm_verifier
)
)
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ template <class RecursiveBuilder> class RecursiveMergeVerifierTest : public test
static void test_recursive_merge_verification()
{
auto op_queue = std::make_shared<ECCOpQueue>();
// TODO(https://github.com/AztecProtocol/barretenberg/issues/800) Testing cleanup
GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);

InnerBuilder sample_circuit{ op_queue };
GoblinMockCircuits::construct_simple_circuit(sample_circuit);
Expand Down
24 changes: 4 additions & 20 deletions barretenberg/cpp/src/barretenberg/ultra_honk/mega_honk.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,11 @@ TYPED_TEST(MegaHonkTests, MergeProofSizeCheck)
using Flavor = TypeParam;
using MergeProver = MergeProver_<Flavor>;

// Constuct an op queue and populate it with some arbitrary ops
auto op_queue = std::make_shared<bb::ECCOpQueue>();
GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);

auto builder = typename Flavor::CircuitBuilder{ op_queue };
auto builder = typename Flavor::CircuitBuilder{};
GoblinMockCircuits::construct_simple_circuit(builder);

// Construct a merge proof and ensure its size matches expectation; if not, the constant may need to be updated
MergeProver merge_prover{ op_queue };
MergeProver merge_prover{ builder.op_queue };
auto merge_proof = merge_prover.construct_proof();

EXPECT_EQ(merge_proof.size(), MERGE_PROOF_SIZE);
Expand Down Expand Up @@ -235,10 +231,7 @@ TYPED_TEST(MegaHonkTests, DynamicVirtualSizeIncrease)
TYPED_TEST(MegaHonkTests, SingleCircuit)
{
using Flavor = TypeParam;
auto op_queue = std::make_shared<bb::ECCOpQueue>();

GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);
auto builder = typename Flavor::CircuitBuilder{ op_queue };
auto builder = typename Flavor::CircuitBuilder{};

GoblinMockCircuits::construct_simple_circuit(builder);

Expand All @@ -247,7 +240,7 @@ TYPED_TEST(MegaHonkTests, SingleCircuit)
EXPECT_TRUE(honk_verified);

// Construct and verify Goblin ECC op queue Merge proof
auto merge_verified = this->construct_and_verify_merge_proof(op_queue);
auto merge_verified = this->construct_and_verify_merge_proof(builder.op_queue);
EXPECT_TRUE(merge_verified);
}

Expand All @@ -261,9 +254,6 @@ TYPED_TEST(MegaHonkTests, MultipleCircuitsMergeOnly)
using Flavor = TypeParam;
// Instantiate EccOpQueue. This will be shared across all circuits in the series
auto op_queue = std::make_shared<bb::ECCOpQueue>();

GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);

// Construct multiple test circuits that share an ECC op queue. Generate and verify a proof for each.
size_t NUM_CIRCUITS = 3;
for (size_t i = 0; i < NUM_CIRCUITS; ++i) {
Expand All @@ -288,9 +278,6 @@ TYPED_TEST(MegaHonkTests, MultipleCircuitsHonkOnly)

// Instantiate EccOpQueue. This will be shared across all circuits in the series
auto op_queue = std::make_shared<bb::ECCOpQueue>();

GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);

// Construct multiple test circuits that share an ECC op queue. Generate and verify a proof for each.
size_t NUM_CIRCUITS = 3;
for (size_t i = 0; i < NUM_CIRCUITS; ++i) {
Expand All @@ -315,9 +302,6 @@ TYPED_TEST(MegaHonkTests, MultipleCircuitsHonkAndMerge)

// Instantiate EccOpQueue. This will be shared across all circuits in the series
auto op_queue = std::make_shared<bb::ECCOpQueue>();

GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);

// Construct multiple test circuits that share an ECC op queue. Generate and verify a proof for each.
size_t NUM_CIRCUITS = 3;
for (size_t i = 0; i < NUM_CIRCUITS; ++i) {
Expand Down
12 changes: 3 additions & 9 deletions barretenberg/cpp/src/barretenberg/ultra_honk/merge_prover.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ template <class Flavor>
MergeProver_<Flavor>::MergeProver_(const std::shared_ptr<ECCOpQueue>& op_queue,
std::shared_ptr<CommitmentKey> commitment_key)
: op_queue(op_queue)
{
// Update internal size data in the op queue that allows for extraction of e.g. previous aggregate transcript
pcs_commitment_key =
commitment_key ? commitment_key : std::make_shared<CommitmentKey>(op_queue->get_ultra_ops_table_num_rows());
}
, pcs_commitment_key(commitment_key ? commitment_key
: std::make_shared<CommitmentKey>(op_queue->get_ultra_ops_table_num_rows()))
{}

/**
* @brief Prove proper construction of the aggregate Goblin ECC op queue polynomials T_j, j = 1,2,3,4.
Expand All @@ -41,10 +39,6 @@ template <typename Flavor> MergeProver_<Flavor>::MergeProof MergeProver_<Flavor>
std::array<Polynomial, NUM_WIRES> T_prev = op_queue->construct_previous_ultra_ops_table_columns();
std::array<Polynomial, NUM_WIRES> t_current = op_queue->construct_current_ultra_ops_subtable_columns();

// TODO(#723): Cannot currently support an empty T_prev. Need to be able to properly handle zero commitment.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

723 is a closed issue, we can handle zero commitments in some of the scenarios, the issues remain around this AztecProtocol/barretenberg#993
at least this my understanding for a preliminary investigation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue you mention can also be closed now. We're always going to run the merge regardless of whether a circuit adds ops or not. Issue AztecProtocol/barretenberg#905 is still relevant though

ASSERT(T_prev[0].size() > 0);
ASSERT(T_current[0].size() > T_prev[0].size()); // Must have some new ops to accumulate otherwise [t_j] = 0

const size_t current_table_size = T_current[0].size();
const size_t current_subtable_size = t_current[0].size();

Expand Down