refactor!: Use circuit builders by codygunton · Pull Request #501 · AztecProtocol/barretenberg

codygunton · 2023-06-02T18:59:14Z

Description

Finally finish the splitting work, up to some renaming for clarity to be done in a follow-up.

Checklist:

kevaundray · 2023-06-12T19:01:37Z

 #include "barretenberg/common/net.hpp"

 const std::string protocol_name = "BARRETENBERG_GRUMPKIN_IPA_CRS";
+// WORKTODO: this is incorrect now right?


what is the difference between WORKTODO and a regular TODO?

WORKTODO is something I search for and then resolve before putting the PR up for review.

Tbh maybe I should use something shorter like HEY

Or perhaps HMM alla Zac

kevaundray

Still in draft so can ignore the nits.

Note: if check_circuit being false causes the prover to not create a proof, then this would be a breaking change since the observed behaviour from someone doing a recursive verification will be different when the circuit is invalid.

If only a flag is outputted or more generally, the observed behaviour does not change with or without check_circuit then it is non-breaking

kevaundray · 2023-06-12T19:02:23Z


            info("Verifying the ultra (inner) proof natively...");
-            auto native_result = native_verifier.verify_proof(recursive_proof);
+            auto native_result = native_verifier.verify_proof(recursive_proof); // WORKTODO


what is the TODO referring to here

recursive_proof is a bad name... misleading shorthand for proof_to_be_recursively_verified. Plan to rename to something easier to parse.

codygunton · 2023-06-14T20:41:03Z

Still in draft so can ignore the nits.

Note: if check_circuit being false causes the prover to not create a proof, then this would be a breaking change since the observed behaviour from someone doing a recursive verification will be different when the circuit is invalid.

If only a flag is outputted or more generally, the observed behaviour does not change with or without check_circuit then it is non-breaking

@kevaundray Did you see something that caused you to have a concern around this?

codygunton · 2023-06-14T20:46:22Z

^ space needed 🥇

codygunton

Leaving some explanatory notes.

codygunton · 2023-06-21T17:17:17Z

      - readability-function-cognitive-complexity
      # It is often nicer to not be explicit
      - google-explicit-constructor
-    CheckOptions:


I added this option before to avoid a warning we had, but now we don't need to avoid that warning and I added the option incorrectly anyway.

codygunton · 2023-06-21T17:19:07Z

 #include "ecc/curves/grumpkin/grumpkin.hpp"
 #include "numeric/random/engine.hpp"
 #include "numeric/uint256/uint256.hpp"
-#include "plonk/composer/turbo_plonk_composer.hpp"


The follow-up to this PR will include lots of renaming, eg "circuit constructor" becomes "circuit builder" in various places, "composer" becomes "circuit" or "builder" or "circuit_builder" or something in some places, and stays "composer" in other places.

That said, there were certain places where I found it helpful/clarifying to do some renaming in order to get this refactor done, and I leave some of that work in place.

codygunton · 2023-06-21T17:20:52Z

-    plonk::stdlib::field_t b(plonk::stdlib::witness_t(&composer, barretenberg::fr::random_element()));
-    plonk::stdlib::field_t c(&composer);
+    proof_system::plonk::stdlib::field_t a(
+        proof_system::plonk::stdlib::witness_t(&builder, barretenberg::fr::random_element()));


There was some using namespace pollution that let us get away with shortened namespace qualifiers.

codygunton · 2023-06-21T17:23:20Z

 * @param num_iterations
 */
-template <typename Composer> void generate_basic_arithmetic_circuit(Composer& composer, size_t num_gates)
+template <typename Builder> void generate_basic_arithmetic_circuit(Builder& builder, size_t num_gates)


Is it better to be more verbose and use CircuitBuilder, at least for the type name?

I think I prefer CircuitBuilder but I dont feel strongly. As long as we're not using B..

codygunton · 2023-06-21T17:23:59Z

-#include "barretenberg/ecc/curves/bn254/fr.hpp"
 #include <benchmark/benchmark.h>
 #include <cstddef>
-#include "barretenberg/honk/composer/standard_honk_composer.hpp"


Renaming and moving comes in the next PR.

codygunton · 2023-06-21T17:52:23Z

    }

-    if constexpr (C::type == ComposerType::PLOOKUP && C::commitment_type == pedersen::CommitmentType::LOOKUP_PEDERSEN) {
+    if constexpr (C::type == proof_system::ComposerType::PLOOKUP &&


IOU: get rid of ComposerType and introduce a concept like UsePlookup or something like this.

codygunton · 2023-06-21T17:55:21Z

    return output;
 }

-TEST(stdlib_sha256, test_duplicate_proving_key)


After rewriting this (stdlib) test to not use a composer, it was clear that it's not a test of circuit logic, but instead of composer/proof system logic, so I deleted it.

codygunton · 2023-06-21T17:57:41Z

@@ -1,73 +0,0 @@
-/**


(moved and pared down to one of the above files, similarly with composers_fwd.hpp)

codygunton · 2023-06-21T18:07:30Z

    return result;
 }

+template <typename Flavor>


I added this so that I could avoid needing to construct a manifest (a proof-system detail) in the the ACL. @maramihali in future recursion work, we should expose a nice and simple interface.

Automatically nicer for Honk since no manifest..

codygunton · 2023-06-21T18:09:07Z

@@ -1,70 +1,101 @@
 #include "verifier.hpp"


It took a while to properly refactor this. Since the tests were already hard to read IMO, I went ahead and did a deeper refactor, aiming to save the next person some time.

ledwards2225

Looks great. Big step forward. No major comments

ledwards2225 · 2023-06-21T19:45:06Z

 * @param num_iterations
 */
-template <typename Composer> void generate_basic_arithmetic_circuit(Composer& composer, size_t num_gates)
+template <typename Builder> void generate_basic_arithmetic_circuit(Builder& builder, size_t num_gates)


I think I prefer CircuitBuilder but I dont feel strongly. As long as we're not using B..

ledwards2225 · 2023-06-21T19:55:57Z


        bool first_result =
            crypto::ecdsa::verify_signature<Sha256Hasher, fq, fr, g1>(message_string, account.public_key, signature);
+        static_cast<void>(first_result); // TODO(Cody): This is not used anywhere.


I've started using [[ maybe_unused ]] first_result = ... instead, but I think in this case the entire crypto::ecdsa::verify_signature can be deleted. I think this was debugging code that I forgot to delete.

ledwards2225 · 2023-06-21T20:01:14Z

 template <typename T> void benchmark_format_chain(std::ostream& os, T const& first)
 {
-    // We will be saving these values to a CSV file, so we can't tolerate commas
+    // We will be saving these values to


Strange. Line breaks way too short

ledwards2225 · 2023-06-21T20:01:30Z

 #define BENCHMARK_INFO_PREFIX "##BENCHMARK_INFO_PREFIX##"
 #define BENCHMARK_INFO_SEPARATOR "#"
 #define BENCHMARK_INFO_SUFFIX "##BENCHMARK_INFO_SUFFIX##"
-#define GET_COMPOSER_NAME_STRING(composer)                                                                             \


ledwards2225 · 2023-06-21T20:02:37Z


-Composer create_circuit_with_witness(acir_format const& constraint_system,
-                                     WitnessVector const& witness,
-                                     std::shared_ptr<barretenberg::srs::factories::CrsFactory> const& crs_factory,


ledwards2225 · 2023-06-21T20:38:06Z

    {
-        InnerComposer inner_composer = InnerComposer(srs_path);
-        OuterComposer outer_composer = OuterComposer(srs_path);
+        InnerBuilder inner_circuit;


I suppose the difficulty is that the CircuitBuilder contains both the methods that do the building and the data that defines the circuit. This isnt very appealing and I'm not suggesting it but logically just Circuit actually makes the most sense to me. Presumably this is what led you to wanting to call the instance inner_circuit. So perhaps your blend of InnerBuilder inner_circuit is just the thing..

ledwards2225 · 2023-06-21T20:59:07Z

    return result;
 }

+template <typename Flavor>


Automatically nicer for Honk since no manifest..

ledwards2225 · 2023-06-21T21:01:40Z

+    //  * use this additional function to check that the recursive proof points work.
+    //  *
+    //  * @return boolean result
+    //  */


uncommment this description?

ledwards2225 · 2023-06-21T21:04:29Z

-    // verifies a proof of a circuit that verifies one of two proofs. Test 'a' uses a proof over the first of the two
-    // variable circuits
+    // verifies a proof of a circuit that verifies one of two proofs. Test 'a' uses a proof over the first of the
+    // two variable circuits


I find that this happens comment code is further commented out then formatted

Ah, good call.

ledwards2225 · 2023-06-21T21:08:11Z

+    if constexpr (std::same_as<TypeParam, TurboPlonkComposerHelper>) {
+        TestFixture::test_double_verification();
+    } else {
+        // Test doesn't compile.


Is this expected? Could be worth elaborating here

Yeah, the comment I rewrote, which said that the test was too heavy to run in CI, was misleading. It doesn't run because in the else branch there's a type clash due to how the test setup functions are written.

Gets rid of the composers class that wrapped a circuit constructor and a composer helper. This entailed various improvements such as a refactor of the stdlib verifier tests. Cleanup such a renaming of classes and moving of files will come in another PR. Corresponding PR in the core circuits library is #895.

codygunton added 2 commits June 2, 2023 18:58

Initial commit so tests run

2b681d2

Illustrate aux gate issue

4fe7ac2

codygunton self-assigned this Jun 5, 2023

codygunton added 4 commits June 5, 2023 18:27

Add notes, note last point where circuit checks

ae2f094

Merge branch 'master' into cg/big-split

56ebb56

composer~>circuit_constructor

a779895

StandardHonk test use CCtor

bfd910f

kevaundray reviewed Jun 12, 2023

View reviewed changes

codygunton added 13 commits June 12, 2023 20:56

UltraHonk tests use CCtor

7b4ed77

Remove honk composer include from UH tests.

bc4bcbe

Merge branch 'master' into cg/big-split

33f31ab

Honk doesn't depend on Plonk anymore

83412a7

Fix honk tests with Grumpkin.

83cf7d9

Honk is split.

08e2741

Plonk tests split.

dc05a2c

Fix recursive verifier

40e4d69

Move ensure_... to finalize_circuit.

ecea160

Move _ensure_ to composer helper.

4e7006d

Cleanup

5b10604

Re-add untested code used by circuits.

e3e1d45

Split honk benchmarks.

063250b

codygunton changed the title ~~Big split~~ refactor!:Big split Jun 14, 2023

codygunton changed the title ~~refactor!:Big split~~ feat!:Big split Jun 14, 2023

codygunton changed the title ~~feat!:Big split~~ feat:Big split Jun 14, 2023

codygunton changed the title ~~feat:Big split~~ refactor!: Big split Jun 14, 2023

codygunton added 2 commits June 14, 2023 21:28

Split ultra honk bench.

624b4fa

Fix include.

c32e542

codygunton added 12 commits June 20, 2023 22:31

Need to use commit hash(?)

0493ef0

Undo hack to stop exposing manifest in circuits.

efb4119

Make cast more explicit.

daec1bc

Need Montgomery conversion.

b053b79

Merge remote-tracking branch 'origin/master' into cg/big-split

02f9bd4

WORKTODO: remove unused function

86ed54f

Removed hack function.

4a642c6

WORKTODO: remove non-circuit-focused test.

dc25ece

WORKTODOs: misc

a33cb5b

WORKTODO: mv stdlib composers files.

a6ab841

Update packages commit.

22d2ff6

Run acir_format_tests in CI

63a814d

codygunton commented Jun 21, 2023

View reviewed changes

codygunton linked an issue Jun 21, 2023 that may be closed by this pull request

Reinstate ACIR format tests. #551

Closed

codygunton changed the title ~~refactor!: Big split~~ refactor!: Use circuit builders Jun 21, 2023

codygunton added 3 commits June 21, 2023 19:15

Cleanup

36a8a24

Try to use automatic workflow

e3954de

Whoops, I misunderstood--set packages commit.

74124ba

codygunton marked this pull request as ready for review June 21, 2023 19:24

ledwards2225 approved these changes Jun 21, 2023

View reviewed changes

Reformat some comments.

77742d6

codygunton merged commit 709a29c into master Jun 21, 2023

codygunton deleted the cg/big-split branch June 21, 2023 21:55

github-actions Bot mentioned this pull request Jun 21, 2023

chore(barretenberg): Release 0.2.0 #537

Merged

codygunton mentioned this pull request Jun 22, 2023

chore: Remove usage of ComposerHelper and use CircuitBuilder from the API #555

Merged

10 tasks

github-actions Bot mentioned this pull request Jul 11, 2023

chore(barretenberg): Release 0.3.0 #595

Merged

This was referenced Jul 25, 2023

chore(aztec-sandbox): Release 0.2.0-alpha27 AztecProtocol/aztec-packages#1180

Closed

chore(aztec-sandbox): Release 0.2.0 AztecProtocol/aztec-packages#1181

Closed

Conversation

codygunton commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevaundray left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codygunton commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codygunton commented Jun 14, 2023

Uh oh!

codygunton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ledwards2225 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codygunton commented Jun 2, 2023 •

edited

Loading

codygunton commented Jun 14, 2023 •

edited

Loading

codygunton Jun 21, 2023 •

edited

Loading