-
Notifications
You must be signed in to change notification settings - Fork 268
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
Conversation
* | ||
* @param op_queue | ||
*/ | ||
void populate_with_mock_initital_data() |
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.
moved this at the end of the file as a private function
ECCOPQueue
EccOpQueue
@@ -120,11 +129,7 @@ class ECCOpQueue { | |||
[[nodiscard]] size_t get_previous_size() const { return previous_ultra_ops_size; } | |||
[[nodiscard]] size_t get_current_size() const { return current_ultra_ops_size; } | |||
|
|||
void set_commitment_data(std::array<Point, 4>& commitments) | |||
{ | |||
previous_ultra_ops_commitments = ultra_ops_commitments; |
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.
nothing actually uses previous_ultra_ops_commitments (the merge prover considers C_T_prev to be ultra_ops_commitments) so removed the field
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.
sweet
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.
I may be missing something but I expected that the mocked ops would need to be valid in order for the ECCVM to fail. The fact that this isn't causing any problems is concerning. What am I missing?
@@ -120,11 +129,7 @@ class ECCOpQueue { | |||
[[nodiscard]] size_t get_previous_size() const { return previous_ultra_ops_size; } | |||
[[nodiscard]] size_t get_current_size() const { return current_ultra_ops_size; } | |||
|
|||
void set_commitment_data(std::array<Point, 4>& commitments) | |||
{ | |||
previous_ultra_ops_commitments = ultra_ops_commitments; |
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.
sweet
barretenberg/cpp/src/barretenberg/proof_system/op_queue/ecc_op_queue.hpp
Outdated
Show resolved
Hide resolved
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Values are compared against data from master at commit L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction processing duration by data writes.
|
…in benchmarks; make it unique function
2f52efa
to
82b6428
Compare
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 comment
The 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)
@@ -140,6 +140,13 @@ bool proveAndVerify(const std::string& bytecodePath, const std::string& witnessP | |||
*/ | |||
bool accumulateAndVerifyGoblin(const std::string& bytecodePath, const std::string& witnessPath) | |||
{ | |||
// TODO(https://github.com/AztecProtocol/barretenberg/issues/811): Don't hardcode dyadic circuit size. Currently set |
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
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.
LGTM. Just a couple of small comments/requests
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 comment
The 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 comment
The 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
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the appropriate TODO with issue here?
18a6090
to
d96ef57
Compare
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.27.2</summary> ## [0.27.2](aztec-package-v0.27.1...aztec-package-v0.27.2) (2024-03-13) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.27.2</summary> ## [0.27.2](barretenberg.js-v0.27.1...barretenberg.js-v0.27.2) (2024-03-13) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.27.2</summary> ## [0.27.2](aztec-cli-v0.27.1...aztec-cli-v0.27.2) (2024-03-13) ### Miscellaneous * **aztec-cli:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.27.2</summary> ## [0.27.2](aztec-packages-v0.27.1...aztec-packages-v0.27.2) (2024-03-13) ### Features * Check initialization arguments in constructors ([#5144](#5144)) ([d003bd6](d003bd6)) * Multithreaded prover folding ([#5147](#5147)) ([94922fc](94922fc)) * Run tests in parallel in `nargo test` (noir-lang/noir#4484) ([58e15ed](58e15ed)) * Track stack frames and their variables in the debugger (noir-lang/noir#4188) ([58e15ed](58e15ed)) ### Bug Fixes * **acir_gen:** More granular element sizes array check (noir-lang/noir#4528) ([58e15ed](58e15ed)) * Add `follow_bindings` to follow `Type::Alias` links (noir-lang/noir#4521) ([58e15ed](58e15ed)) * Allow type aliases in main (noir-lang/noir#4505) ([58e15ed](58e15ed)) * Constant gen ([#5172](#5172)) ([394a0e0](394a0e0)) * **docs:** Update quickstart.md ([#5021](#5021)) ([be9f8a1](be9f8a1)) * Dynamic assert messages in brillig (noir-lang/noir#4531) ([58e15ed](58e15ed)) * Fix brillig slowdown when assigning arrays in loops (noir-lang/noir#4472) ([58e15ed](58e15ed)) * Fix deployments ([#5183](#5183)) ([596253b](596253b)) * Force src impl for == on slices (noir-lang/noir#4507) ([58e15ed](58e15ed)) * Handling of gh deps in noir_wasm (noir-lang/noir#4499) ([58e15ed](58e15ed)) * Intermittent invert 0 in Goblin ([#5174](#5174)) ([3e68b49](3e68b49)) * Iterative flattening pass (noir-lang/noir#4492) ([58e15ed](58e15ed)) * Noir mirror merge strat ([#5166](#5166)) ([74fa8d6](74fa8d6)) * **ssa:** Handle mergers of slices returned from calls (noir-lang/noir#4496) ([58e15ed](58e15ed)) ### Miscellaneous * Add `ModuleDeclaration` struct (noir-lang/noir#4512) ([58e15ed](58e15ed)) * Add HashMap docs (noir-lang/noir#4457) ([58e15ed](58e15ed)) * Add regression test for issue 4449 (noir-lang/noir#4503) ([58e15ed](58e15ed)) * Better output in ci_deploy_contracts.sh ([#5171](#5171)) ([8d73f8a](8d73f8a)) * Bump bb to 0.26.3 (noir-lang/noir#4488) ([58e15ed](58e15ed)) * **ci:** Fix JS publishing workflow checking out inconsistent commits (noir-lang/noir#4493) ([58e15ed](58e15ed)) * Custom hash for eddsa (noir-lang/noir#4440) ([58e15ed](58e15ed)) * Deterministic mode ([#5155](#5155)) ([e68b56a](e68b56a)) * Document big integers (noir-lang/noir#4487) ([58e15ed](58e15ed)) * Generalise `FunctionVisibility` to `ItemVisibility` (noir-lang/noir#4495) ([58e15ed](58e15ed)) * Interaction for a mock first circuit handled inside the `EccOpQueue` ([#4854](#4854)) ([d9cbdc8](d9cbdc8)) * Move `check_method_signatures` to type checking phase (noir-lang/noir#4516) ([58e15ed](58e15ed)) * Move templated code for assert_message into the stdlib (noir-lang/noir#4475) ([58e15ed](58e15ed)) * Organize the `blackbox_solver` crate (noir-lang/noir#4519) ([58e15ed](58e15ed)) * Pass `import_directive` by reference (noir-lang/noir#4511) ([58e15ed](58e15ed)) * Pass macro processors by reference (noir-lang/noir#4501) ([58e15ed](58e15ed)) * Pull out separate function for compiling and running a test ([58e15ed](58e15ed)) * Release Noir(0.25.0) (noir-lang/noir#4352) ([58e15ed](58e15ed)) * Update cargo deny config (noir-lang/noir#4486) ([58e15ed](58e15ed)) * Update various dependencies (noir-lang/noir#4513) ([58e15ed](58e15ed)) </details> <details><summary>barretenberg: 0.27.2</summary> ## [0.27.2](barretenberg-v0.27.1...barretenberg-v0.27.2) (2024-03-13) ### Features * Multithreaded prover folding ([#5147](#5147)) ([94922fc](94922fc)) ### Bug Fixes * Intermittent invert 0 in Goblin ([#5174](#5174)) ([3e68b49](3e68b49)) ### Miscellaneous * Interaction for a mock first circuit handled inside the `EccOpQueue` ([#4854](#4854)) ([d9cbdc8](d9cbdc8)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.27.2</summary> ## [0.27.2](AztecProtocol/aztec-packages@aztec-package-v0.27.1...aztec-package-v0.27.2) (2024-03-13) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.27.2</summary> ## [0.27.2](AztecProtocol/aztec-packages@barretenberg.js-v0.27.1...barretenberg.js-v0.27.2) (2024-03-13) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.27.2</summary> ## [0.27.2](AztecProtocol/aztec-packages@aztec-cli-v0.27.1...aztec-cli-v0.27.2) (2024-03-13) ### Miscellaneous * **aztec-cli:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.27.2</summary> ## [0.27.2](AztecProtocol/aztec-packages@aztec-packages-v0.27.1...aztec-packages-v0.27.2) (2024-03-13) ### Features * Check initialization arguments in constructors ([#5144](AztecProtocol/aztec-packages#5144)) ([d003bd6](AztecProtocol/aztec-packages@d003bd6)) * Multithreaded prover folding ([#5147](AztecProtocol/aztec-packages#5147)) ([94922fc](AztecProtocol/aztec-packages@94922fc)) * Run tests in parallel in `nargo test` (noir-lang/noir#4484) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Track stack frames and their variables in the debugger (noir-lang/noir#4188) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) ### Bug Fixes * **acir_gen:** More granular element sizes array check (noir-lang/noir#4528) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Add `follow_bindings` to follow `Type::Alias` links (noir-lang/noir#4521) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Allow type aliases in main (noir-lang/noir#4505) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Constant gen ([#5172](AztecProtocol/aztec-packages#5172)) ([394a0e0](AztecProtocol/aztec-packages@394a0e0)) * **docs:** Update quickstart.md ([#5021](AztecProtocol/aztec-packages#5021)) ([be9f8a1](AztecProtocol/aztec-packages@be9f8a1)) * Dynamic assert messages in brillig (noir-lang/noir#4531) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Fix brillig slowdown when assigning arrays in loops (noir-lang/noir#4472) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Fix deployments ([#5183](AztecProtocol/aztec-packages#5183)) ([596253b](AztecProtocol/aztec-packages@596253b)) * Force src impl for == on slices (noir-lang/noir#4507) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Handling of gh deps in noir_wasm (noir-lang/noir#4499) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Intermittent invert 0 in Goblin ([#5174](AztecProtocol/aztec-packages#5174)) ([3e68b49](AztecProtocol/aztec-packages@3e68b49)) * Iterative flattening pass (noir-lang/noir#4492) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Noir mirror merge strat ([#5166](AztecProtocol/aztec-packages#5166)) ([74fa8d6](AztecProtocol/aztec-packages@74fa8d6)) * **ssa:** Handle mergers of slices returned from calls (noir-lang/noir#4496) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) ### Miscellaneous * Add `ModuleDeclaration` struct (noir-lang/noir#4512) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Add HashMap docs (noir-lang/noir#4457) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Add regression test for issue 4449 (noir-lang/noir#4503) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Better output in ci_deploy_contracts.sh ([#5171](AztecProtocol/aztec-packages#5171)) ([8d73f8a](AztecProtocol/aztec-packages@8d73f8a)) * Bump bb to 0.26.3 (noir-lang/noir#4488) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * **ci:** Fix JS publishing workflow checking out inconsistent commits (noir-lang/noir#4493) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Custom hash for eddsa (noir-lang/noir#4440) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Deterministic mode ([#5155](AztecProtocol/aztec-packages#5155)) ([e68b56a](AztecProtocol/aztec-packages@e68b56a)) * Document big integers (noir-lang/noir#4487) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Generalise `FunctionVisibility` to `ItemVisibility` (noir-lang/noir#4495) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Interaction for a mock first circuit handled inside the `EccOpQueue` ([#4854](AztecProtocol/aztec-packages#4854)) ([d9cbdc8](AztecProtocol/aztec-packages@d9cbdc8)) * Move `check_method_signatures` to type checking phase (noir-lang/noir#4516) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Move templated code for assert_message into the stdlib (noir-lang/noir#4475) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Organize the `blackbox_solver` crate (noir-lang/noir#4519) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Pass `import_directive` by reference (noir-lang/noir#4511) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Pass macro processors by reference (noir-lang/noir#4501) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Pull out separate function for compiling and running a test ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Release Noir(0.25.0) (noir-lang/noir#4352) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Update cargo deny config (noir-lang/noir#4486) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) * Update various dependencies (noir-lang/noir#4513) ([58e15ed](AztecProtocol/aztec-packages@58e15ed)) </details> <details><summary>barretenberg: 0.27.2</summary> ## [0.27.2](AztecProtocol/aztec-packages@barretenberg-v0.27.1...barretenberg-v0.27.2) (2024-03-13) ### Features * Multithreaded prover folding ([#5147](AztecProtocol/aztec-packages#5147)) ([94922fc](AztecProtocol/aztec-packages@94922fc)) ### Bug Fixes * Intermittent invert 0 in Goblin ([#5174](AztecProtocol/aztec-packages#5174)) ([3e68b49](AztecProtocol/aztec-packages@3e68b49)) ### Miscellaneous * Interaction for a mock first circuit handled inside the `EccOpQueue` ([#4854](AztecProtocol/aztec-packages#4854)) ([d9cbdc8](AztecProtocol/aztec-packages@d9cbdc8)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
PR #4854 removed the mocking function from the `EccOpQueue` `populate_with_mock_initital_data` which was stale and only populating the `ultra_ops` rather than both `raw_ops` and `ultra_ops` and ensured mocking (required to avoid commitments to 0) is done everywhere with a single function `GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);`. A mis-merge reintroduced `populate_with_mock_initital_data` so removing again.
Closes AztecProtocol/barretenberg#723.
Because of issues with handling zero-commitments, we need to populate the
EccOpQueue
with some mock ops from a first circuit in order for the first merge prover to work without problems. This was done client-side which required manually calling eitherGoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit
orop_queue->populate_with_mock_initital_data
(two functions for the same purpose) everywhere goblin and the merge protocol is used/tested. This PR ensures onlyGoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit
is used across the codebase since it's the one updating bothultra_ops
andraw_ops
, removing the other function, and isolates mocking inside the Goblin class so it doesn't have to be done in benchmarks.Note: tried to isolate it inside EccOpQueue, which is possible but then requires us to have an op_offset (from which non-mocked operations start) which complicates some code and tests unnecessarily.