refactor!: Use circuit builders #895

codygunton · 2023-06-20T22:12:41Z

Description

This PR is 'just' bulk cleanup to reflect changes in Barretenberg. These are the main changes.

Composer is renamed to Builder or CircuitBuilder
composer is renamed to builder. In some contexts circuit also makes sense and it might be preferable.
Circuit builders are not instantiated with a string specifying a crs, so these constructors (which were available because of a hack) are removed.
ComposerType was renamed to CircuitType and this is changed accordingly, along with renaming composer_type to circuit_type.
Rename DummyComposer and dummy_composer.hpp.
In order to restrict the barretenberg API, the Aggregator::aggregate function now calls a more encapsulated recursive verifier function.

Some unexpected compiler errors around unused variables came up. I'm not sure how to proceed. I made an issue (#914) recording this in the hopes that we can merge with todos referencing that issue.

I left some additional notes as comments on lines of code.

Checklist:

I have reviewed my diff in github, line by line.
Every change is related to the PR description.
I have linked this pull request to the issue(s) that it resolves.
There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
The branch has been merged or rebased against the head of its merge target.
I'm happy for the PR to be merged at the reviewer's next convenience.

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 AztecProtocol/aztec-packages#895.

ludamad · 2023-06-22T16:36:31Z

circuits/cpp/src/aztec3/circuits/kernel/public/.test.cpp

-                                                                    .l1_to_l2_messages_tree_root = 3000,
-                                                                    .private_kernel_vk_tree_root = 4000,
-                                                                } };
+    // CombinedHistoricTreeRoots<NT> const historic_tree_roots = { .private_historic_tree_roots = {


Should we remove instead?

Probably? I haven't looked into the history or discussed with core circuits team yet.

Yeah I didn't see it was a draft, my bad

I ended up making an issue to look at this and two other unused variables in the same test file, and I reference that issue in TODOs in the code.

ludamad

LGTM overall (although now I realize its a draft)

Weird stuff is happening

codygunton · 2023-06-27T00:21:22Z

circuits/cpp/src/aztec3/circuits/abis/function_leaf_preimage.hpp

    {
        static_assert((std::is_same<NativeTypes, NCT>::value));

-        // Capture the composer:
-        auto to_ct = [&](auto& e) { return aztec3::utils::types::to_ct(composer, e); };
+        // Capture the circuit builder:


FYI, I think this this comment and this lambda are repeated 30 times!

codygunton · 2023-06-27T00:27:29Z

circuits/cpp/src/aztec3/circuits/kernel/private/utils.cpp

@@ -91,7 +91,7 @@ std::shared_ptr<NT::VK> fake_vk()
 {
    std::map<std::string, NT::bn254_point> commitments;
    commitments["FAKE"] = *new NT::bn254_point(NT::fq(0), NT::fq(0));
-    NT::VKData vk_data = { .composer_type = proof_system::ComposerType::TURBO,
+    NT::VKData vk_data = { .circuit_type = static_cast<uint32_t>(proof_system::CircuitType::TURBO),


enum ComposerType became enum class CircuitType : uint32_t and so we need to do explicit casting sometimes.

codygunton · 2023-06-27T00:36:23Z

circuits/cpp/src/aztec3/circuits/recursion/aggregator.hpp

-        CT::AggregationObject result = verify_proof<CT::bn254, CT::recursive_inner_verifier_settings>(
-            composer, vk, recursive_manifest, proof, previous_aggregation_output);
+        CT::AggregationObject result =
+            verify_proof<plonk::flavor::Ultra>(builder, vk, proof, previous_aggregation_output);


This function was added to Barretenberg to avoid the circuits library from having to know about a manifest. In general, we should insulate the circuits library from details of proof construction (such as a manifest). I'm not sure that the flavor is the "right" thing, but it's an improvement, and doing it this way let me expose MUCH less in barretenberg.hpp.

codygunton · 2023-06-27T00:42:37Z

yarn-project/circuits.js/src/structs/shared.ts

  STANDARD = 0,
  TURBO = 1,
-  PLOOKUP = 2,
-  STANDARD_HONK = 3,


There is no difference between a standard Honk circuit and a standard Plonk circuit! This was a hack we introduced, iirc, in order to make the stdlib tests run with Honk.

yarn-project/circuits.js/src/structs/verification_key.ts

suyash67 · 2023-06-27T08:37:25Z

circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.hpp

    PreviousKernelData<NT> const& previous_kernel,
    std::array<NT::fr, READ_REQUESTS_LENGTH> const& read_requests,
    std::array<MembershipWitness<NT, PRIVATE_DATA_TREE_HEIGHT>, READ_REQUESTS_LENGTH> const&
        read_request_membership_witnesses);

-CircuitResult<KernelCircuitPublicInputs<NT>> native_private_kernel_circuit_ordering_rr_dummy(
+CircuitResult<KernelCircuitPublicInputs<NT> > native_private_kernel_circuit_ordering_rr_dummy(


nit but is this an unwanted space?

suyash67 · 2023-06-27T08:38:50Z

circuits/cpp/src/aztec3/circuits/kernel/private/private_kernel_circuit.cpp

 {
    // compute P0, P1 for private function proof
-    CT::AggregationObject aggregation_object = Aggregator::aggregate(
-        &composer, private_inputs.private_call.vk, private_inputs.private_call.proof, num_private_call_public_inputs);
+    CT::AggregationObject aggregation_object =


This is nice, aggregate shouldn't have to know what the number of public inputs of the private call is.

suyash67 · 2023-06-27T08:54:05Z

circuits/cpp/src/aztec3/utils/types/circuit_types.hpp

-        std::conditional_t<std::same_as<Composer, plonk::TurboPlonkComposer>,
-                           stdlib::recursion::recursive_turbo_verifier_settings<bn254>,
-                           stdlib::recursion::recursive_ultra_verifier_settings<bn254>>;
+    using recursive_inner_verifier_settings = stdlib::recursion::recursive_ultra_verifier_settings<bn254>;


Is his because we don't expect turbo to be used as inner circuit builder anymore? If so, makes sense.

Well, Luke's previous PR fixed the inner circuit type to Ultra (indeed, Aggregate::aggregate now uses a fixed flavor), so it wouldn't even work to toggle the inner type. In general this recursion stuff needs a rework, so I thought I'd pare it down to make it simpler for when we do that work. In fact, I see now this alias is not in use anywhere, so I think I'll remove it.

suyash67

Looks good, just left a couple of minor comments. Thanks @codygunton.

This reverts commit bef4f57.

Changing test description to reference Schnorr not Ecdsa leads to weird build error.

codygunton · 2023-06-28T02:19:02Z

yarn-project/circuits.js/src/structs/kernel/__snapshots__/index.test.ts.snap

@@ -113,7 +113,7 @@ value: 0xd05
 "
 `;

-exports[`structs/kernel serializes and prints EcdsaSignature 1`] = `"{ 0101010101010101010101010101010101010101010101010101010101010101, 0101010101010101010101010101010101010101010101010101010101010101, 0 }"`;


As discussed with @PhilWindle, this final value in the snapshot here should not exist because the signature tested is not an ECDSA signature as the naming suggests--it was switched in some places to a Schnorr signature. I complete this switch by changing the definition of abis__test_roundtrip_serialize_signature to use a Schnorr signature and updated the snapshot accordingly.

BTW, I tried to rename the test and ran into a weird error:

FAIL structs/kernel/index.test.ts ● Test suite failed to run ReferenceError: exports is not defined 1 | import { expectSerializeToMatchSnapshot } from '../../tests/expectSerialize.js'; > 2 | import { makeSchnorrSignature } from '../../tests/factories.js'; | ^ 3 | import { 4 | makePreviousKernelData, 5 | makePrivateKernelInputsInner, at structs/kernel/index.test.ts:2:23

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 5 commits June 20, 2023 22:10

Use circuit constructor.

2dc4d77

Undo hack to stop exposing manifest.

0d186af

Update Bb

350ab82

Update Bb

ea69fc5

Bump Bb (need Mont conversion)

64c8ba7

codygunton self-assigned this Jun 21, 2023

ludamad reviewed Jun 22, 2023

View reviewed changes

ludamad approved these changes Jun 22, 2023

View reviewed changes

codygunton added 9 commits June 26, 2023 04:11

Using builder and CircuitType

d488fc4

Fix some things break others.

ded2a64

Satisfy prettier?

2224d0f

Composer ~> Builder

aa6f850

composer ~> builder

f91ece9

mv dummy_composer.hpp and rename class.

86fa2cd

Clean change to settingss.

b3097e2

Bump Bb.

e3eed2f

Merge branch 'master' into cg/circuit-builders

019c759

Weird stuff is happening

codygunton mentioned this pull request Jun 27, 2023

Unused variables in circuits tests #914

Open

codygunton added 3 commits June 27, 2023 00:49

Note on compilation errors in tests.

f888db5

Try to satisfy clang-tidy.

4958f0a

Fix test that didn't compile before.

2b8e215

codygunton commented Jun 27, 2023

View reviewed changes

codygunton marked this pull request as ready for review June 27, 2023 01:08

suyash67 reviewed Jun 27, 2023

View reviewed changes

suyash67 approved these changes Jun 27, 2023

View reviewed changes

codygunton added 2 commits June 27, 2023 13:42

Merge branch 'master' into cg/circuit-builders

d19e5ee

Respond to comments and clean up

80d7d22

codygunton added 10 commits June 27, 2023 13:59

Fix typo and formatting.

ee71367

Merge branch 'master' into cg/circuit-builders

27b9a0a

Fix compilation post-merge

46b831f

It should be a Schnorr signature?

c10ae38

Merge branch 'master' into cg/circuit-builders

d5fe0d3

Revert to Ecdsa

ee3d9e0

Proposed fix.

bef4f57

Revert "Proposed fix."

aaeb225

This reverts commit bef4f57.

Test Schnorr; update snapshot.

ec3fbf4

Changing test description to reference Schnorr not Ecdsa leads to weird build error.

Rebuild WASM before making snapshot.

5d20a8d

codygunton commented Jun 28, 2023

View reviewed changes

codygunton merged commit 1df243e into master Jun 28, 2023

codygunton deleted the cg/circuit-builders branch June 28, 2023 02:23

codygunton restored the cg/circuit-builders branch June 28, 2023 02:29

codygunton mentioned this pull request Jun 28, 2023

Revert "refactor!: Use circuit builders" #921

Closed

suyash67 deleted the cg/circuit-builders branch June 28, 2023 03:20

codygunton mentioned this pull request Jun 28, 2023

Cg/circuit builders #922

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Use circuit builders #895

refactor!: Use circuit builders #895

codygunton commented Jun 20, 2023 •

edited

Loading

ludamad Jun 22, 2023

codygunton Jun 22, 2023

ludamad Jun 23, 2023

codygunton Jun 27, 2023

ludamad left a comment •

edited

Loading

codygunton Jun 27, 2023 •

edited

Loading

codygunton Jun 27, 2023 •

edited

Loading

codygunton Jun 27, 2023

codygunton Jun 27, 2023

suyash67 Jun 27, 2023

codygunton Jun 27, 2023

suyash67 Jun 27, 2023

suyash67 Jun 27, 2023

codygunton Jun 27, 2023

suyash67 left a comment

codygunton Jun 28, 2023

codygunton Jun 28, 2023

refactor!: Use circuit builders #895

refactor!: Use circuit builders #895

Conversation

codygunton commented Jun 20, 2023 • edited Loading

Description

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludamad left a comment • edited Loading

Choose a reason for hiding this comment

codygunton Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

codygunton Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suyash67 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codygunton commented Jun 20, 2023 •

edited

Loading

ludamad left a comment •

edited

Loading

codygunton Jun 27, 2023 •

edited

Loading

codygunton Jun 27, 2023 •

edited

Loading