-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: interaction for a mock first circuit handled inside the EccOpQueue
#4854
Changes from 6 commits
9d67a9a
82b6428
5371b60
8191d2d
1e3e447
2adaa14
b799a50
d96ef57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,6 @@ | |
|
||
namespace bb { | ||
|
||
ClientIVC::ClientIVC() | ||
{ | ||
// TODO(https://github.com/AztecProtocol/barretenberg/issues/723): | ||
GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(goblin.op_queue); | ||
} | ||
|
||
/** | ||
* @brief Initialize the IVC with a first circuit | ||
* @details Initializes the accumulator and performs the initial goblin merge | ||
|
@@ -125,10 +119,8 @@ void ClientIVC::precompute_folding_verification_keys() | |
|
||
vks.kernel_vk = std::make_shared<VerificationKey>(prover_instance->proving_key); | ||
|
||
// Clean the ivc state | ||
goblin.op_queue = std::make_shared<Goblin::OpQueue>(); | ||
goblin.merge_proof_exists = false; | ||
GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(goblin.op_queue); | ||
// Clean the Goblin state (reinitialise op_queue with mocking and clear merge proofs) | ||
goblin = Goblin(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a bit weird to me. I know it's not being introduced in this PR but is there some reason why we don't simply instantiate a new goblin instance for use within the scope of this function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added to do to aim to move vk precomputaion out of client ivc |
||
} | ||
|
||
} // namespace bb |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
#include "barretenberg/eccvm/eccvm_composer.hpp" | ||
#include "barretenberg/flavor/goblin_ultra.hpp" | ||
#include "barretenberg/goblin/mock_circuits.hpp" | ||
#include "barretenberg/proof_system/circuit_builder/eccvm/eccvm_circuit_builder.hpp" | ||
#include "barretenberg/proof_system/circuit_builder/goblin_translator_circuit_builder.hpp" | ||
#include "barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp" | ||
|
@@ -88,6 +89,7 @@ class Goblin { | |
AccumulationOutput accumulator; // Used only for ACIR methods for now | ||
|
||
public: | ||
Goblin() { GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add the appropriate TODO with issue here? |
||
/** | ||
* @brief Construct a GUH proof and a merge proof for the present circuit. | ||
* @details If there is a previous merge proof, recursively verify it. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,6 @@ | |
#include "barretenberg/crypto/merkle_tree/memory_store.hpp" | ||
#include "barretenberg/crypto/merkle_tree/merkle_tree.hpp" | ||
#include "barretenberg/flavor/goblin_ultra.hpp" | ||
#include "barretenberg/goblin/goblin.hpp" | ||
#include "barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp" | ||
#include "barretenberg/srs/global_crs.hpp" | ||
#include "barretenberg/stdlib/encryption/ecdsa/ecdsa.hpp" | ||
|
@@ -30,13 +29,17 @@ class GoblinMockCircuits { | |
using Flavor = bb::GoblinUltraFlavor; | ||
using RecursiveFlavor = bb::GoblinUltraRecursiveFlavor_<GoblinUltraBuilder>; | ||
using RecursiveVerifier = bb::stdlib::recursion::honk::UltraRecursiveVerifier_<RecursiveFlavor>; | ||
using KernelInput = Goblin::AccumulationOutput; | ||
using VerifierInstance = bb::VerifierInstance_<Flavor>; | ||
using RecursiveVerifierInstance = ::bb::stdlib::recursion::honk::RecursiveVerifierInstance_<RecursiveFlavor>; | ||
using RecursiveVerifierAccumulator = std::shared_ptr<RecursiveVerifierInstance>; | ||
using VerificationKey = Flavor::VerificationKey; | ||
static constexpr size_t NUM_OP_QUEUE_COLUMNS = Flavor::NUM_WIRES; | ||
|
||
struct KernelInput { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To mock inside Goblin I had to break the dependency between MockCircuits and Goblin (to avoid a circular dependency) |
||
HonkProof proof; | ||
std::shared_ptr<Flavor::VerificationKey> verification_key; | ||
}; | ||
|
||
/** | ||
* @brief Information required by the verifier to verify a folding round besides the previous accumulator. | ||
*/ | ||
|
@@ -148,6 +151,7 @@ class GoblinMockCircuits { | |
op_queue->set_size_data(); | ||
|
||
// Manually compute the op queue transcript commitments (which would normally be done by the merge prover) | ||
bb::srs::init_crs_factory("../srs_db/ignition"); | ||
auto commitment_key = CommitmentKey(op_queue->get_current_size()); | ||
std::array<Point, Flavor::NUM_WIRES> op_queue_commitments; | ||
size_t idx = 0; | ||
|
@@ -163,11 +167,8 @@ class GoblinMockCircuits { | |
* | ||
* @param builder | ||
*/ | ||
static void construct_simple_initial_circuit(GoblinUltraBuilder& builder) | ||
static void construct_simple_circuit(GoblinUltraBuilder& builder) | ||
{ | ||
// TODO(https://github.com/AztecProtocol/barretenberg/issues/800) Testing cleanup | ||
perform_op_queue_interactions_for_mock_first_circuit(builder.op_queue); | ||
|
||
// Add some arbitrary ecc op gates | ||
for (size_t i = 0; i < 3; ++i) { | ||
auto point = Point::random_element(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,30 +166,6 @@ class ECCOpQueue { | |
return result; | ||
} | ||
|
||
/** | ||
* @brief TESTING PURPOSES ONLY: Populate ECC op queue with mock data as stand in for "previous circuit" in tests | ||
* @details TODO(#723): We currently cannot support Goblin proofs (specifically, transcript aggregation) if there | ||
* is not existing data in the ECC op queue (since this leads to zero-commitment issues). This method populates the | ||
* op queue with mock data so that the prover of an arbitrary 'first' circuit can behave as if it were not the | ||
* prover over the first circuit in the stack. This method should be removed entirely once this is resolved. | ||
* | ||
* @param op_queue | ||
*/ | ||
void populate_with_mock_initital_data() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved this at the end of the file as a private function |
||
{ | ||
// Add a single row of data to the op queue and commit to each column as [1] * FF(data) | ||
std::array<Point, 4> mock_op_queue_commitments; | ||
for (size_t idx = 0; idx < 4; idx++) { | ||
auto mock_data = Fr::random_element(); | ||
this->ultra_ops[idx].emplace_back(mock_data); | ||
mock_op_queue_commitments[idx] = Point::one() * mock_data; | ||
} | ||
// Set some internal data based on the size of the op queue data | ||
this->set_size_data(); | ||
// Add the commitments to the op queue data for use by the next circuit | ||
this->set_commitment_data(mock_op_queue_commitments); | ||
} | ||
|
||
/** | ||
* @brief Write point addition op to queue and natively perform addition | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GoblinAcirComposer initialises Goblin which calls the
perform_op_queue_interactions_for_mock_first_circuit
function so the crs needs to be initalised prior to this. Not sure if there's a better way to achieve this