feat: Add pairing points for all Honk circuits #13701

lucasxia01 · 2025-04-21T21:09:02Z

We add a couple of checks to enforce that honk circuits add exactly ONE pairing point object to its public outputs. The first check is in the DeciderProvingKey constructor, which all flows use to create the proving key from the circuit, and it checks that the pairing point object has been set in the builder. The second check happens in the aggregation_state itself, where if we try to call set_public() and the builder already has one set, it will throw an error.

These checks require us to add pairing point objects to a lot of different tests and also move around some of the logic to more proper positions (like from accumulate() to complete_kernel_circuit_logic()). Previously, we had varying amounts of pairing point objects for Mega circuits, but now that shouldn't be the case.

…points-client-ivc

lucasxia01 · 2025-04-23T21:43:56Z

barretenberg/cpp/src/barretenberg/api/api_client_ivc.cpp

    const size_t num_public_inputs_in_final_circuit = get_num_public_inputs_in_final_circuit(input_path);
    info("num_public_inputs_in_final_circuit: ", num_public_inputs_in_final_circuit);
-    // MAGIC_NUMBER is bb::PAIRING_POINT_ACCUMULATOR_SIZE or bb::PROPAGATED_DATABUS_COMMITMENTS_SIZE
-    static constexpr size_t MAGIC_NUMBER = 16;


this hack is not needed

lucasxia01 · 2025-04-23T21:45:37Z

barretenberg/cpp/src/barretenberg/benchmark/ultra_bench/mock_circuits.hpp

        a = b * b;
        b = c * c;
    }
+    stdlib::recursion::aggregation_state<Builder>::add_default_pairing_points_to_public_inputs(builder);


some circuit generation functions will have add_default_pairing_points_to_public_inputs like this one. Unfortunately, for ease, I didn't opt for always excluding them from these kind of functions, so there are also functions which don't have this

lucasxia01 · 2025-04-23T21:47:30Z

barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.cpp

        decider_vk->public_inputs,
        decider_vk->verification_key->databus_propagation_data);
+
+    // WORKTODO: extract agg obj from the decider_vk->public_inputs


will do in followup

lucasxia01 · 2025-04-23T21:47:55Z

barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.cpp

+        current_aggregation_object = perform_recursive_verification_and_databus_consistency_checks(
+            circuit, verifier_input, current_aggregation_object);
    }
+    current_aggregation_object.set_public();


one of the core changes was to set the agg obj public here

lucasxia01 · 2025-04-23T21:48:29Z

barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp

 using namespace bb;

-static constexpr size_t MAX_NUM_KERNELS = 17;
+static constexpr size_t MAX_NUM_KERNELS = 16;


had to decrease this unfortunately, not sure of its effects

Shouldn't be any immediate effect because we dont test anything pushing this limit

lucasxia01 · 2025-04-23T21:49:44Z

barretenberg/cpp/src/barretenberg/client_ivc/mock_circuit_producer.hpp

    {
        ClientCircuit circuit{ ivc.goblin.op_queue };
        circuit = create_mock_circuit(ivc, log2_num_gates); // construct mock base logic
-        if (is_kernel) {


this should come after the inner public inputs get added. not sure how things were passing before, maybe the databus stuff is broken?

or maybe we only use this in circuits where the databus is ignored

the databus shouldnt ever be ignored so its hard to say

barretenberg/cpp/src/barretenberg/dsl/acir_format/block_constraint.test.cpp

lucasxia01 · 2025-04-23T21:55:38Z

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp

        // default one if the circuit is recursive and honk_recursion is true.
        if (!constraint_system.honk_recursion_constraints.empty() ||
            !constraint_system.avm_recursion_constraints.empty()) {
-            // TODO(https://github.com/AztecProtocol/barretenberg/issues/1336): Delete this if statement


this part of the code is only reached if its UltraBuilder, so the if is useless

lucasxia01 · 2025-04-23T21:55:54Z

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp

        }

        // Accumulate the IPA claims and set it to be public inputs
-        if constexpr (IsUltraBuilder<Builder>) {


same here, the if is useless

lucasxia01 · 2025-04-23T21:56:56Z

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.cpp

            process_ivc_recursion_constraints(
                builder, constraint_system, metadata.ivc, has_valid_witness_assignments, gate_counter);
        }
+


this code checks that we don't have both honk_recursion_constraints and ivc_recursion_constraints.

It also will add a default pairing point object for any circuit that doesn't set one through the recursion constraints.

lucasxia01 · 2025-04-23T21:57:37Z

barretenberg/cpp/src/barretenberg/plonk_honk_shared/execution_trace/mega_execution_trace.hpp

                                                     .busread = 3,
                                                     .lookup = 2,
-                                                     .pub_inputs = 1,
+                                                     .pub_inputs = 20,


needed to increase this to accommodate for agg obj

lucasxia01 · 2025-04-23T21:58:03Z

barretenberg/cpp/src/barretenberg/stdlib/goblin_verifier/goblin_recursive_verifier.cpp

-    [[maybe_unused]] auto merge_pairing_points = merge_verifier.verify_proof(stdlib_merge_proof);
-    return { opening_claim, ipa_transcript };
+    auto merge_pairing_points = merge_verifier.verify_proof(stdlib_merge_proof);
+    return { merge_pairing_points, opening_claim, ipa_transcript };


getting ahead of myself

lucasxia01 · 2025-04-23T21:59:20Z

...etenberg/cpp/src/barretenberg/stdlib/plonk_recursion/aggregation_state/aggregation_state.hpp

     * @param recursion_separator
     */
-    void aggregate(aggregation_state const& other, typename Curve::ScalarField recursion_separator)
+    void aggregate(aggregation_state const& other)


moved the recursion_separator generation into the aggregate call itself since I think we'll just generate a new one every time. The cost shouldn't be too bad since its just 70 for a hash and we do very few aggregates

actually, maybe it would be cheaper to batch the aggregation over several points but idk if it matters too much with goblin

does this mean we eventually have to pass the transcript into this method once we're generating the challenge correctly though?

I'm not sure that's the right separation of concerns if so

no, there shouldn't be a need to pass in a transcript. To properly generate this challenge, we won't be using the same Transcript as the recursive verifier. In fact, we don't need to use a Transcript at all since this isn't even a verifier challenge - it's a "random" separator to linearly combine the points with. We really only need to call some hash function to generate it, and the Transcript is a way to easily do that.

lucasxia01 · 2025-04-23T22:01:53Z

barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/databus.hpp

        // Kernel return data is the first public input, followed by app return data
-        data.kernel_return_data_commitment_pub_input_key.start_idx = 0;
-        data.app_return_data_commitment_pub_input_key.start_idx = 8;
+        data.kernel_return_data_commitment_pub_input_key.start_idx = PAIRING_POINT_ACCUMULATOR_SIZE;


had to update this to fix a vk comparison test, but we definitely some better fix than randomly updating constants

Yeah this is bad, would you mind spinning up an issue for this?

lucasxia01 · 2025-04-23T22:02:33Z

barretenberg/cpp/src/barretenberg/ultra_honk/decider_proving_key.hpp

            proving_key.ipa_proof = circuit.ipa_proof;
        }
        // Set the pairing point accumulator indices
+        ASSERT(circuit.contains_pairing_point_accumulator && "Honk circuit must output a pairing point accumulator");


core change: check that we set a pairing point accumulator

lucasxia01 · 2025-04-23T22:03:15Z

...etenberg/cpp/src/barretenberg/stdlib/plonk_recursion/aggregation_state/aggregation_state.hpp

    uint32_t set_public()
    {
+        Builder* ctx = P0.get_context();
+        if (ctx->contains_pairing_point_accumulator) {


core change: check that we don't set 2 pairing point objects to be public inputs.

wait, this is now wrong as this is a plonk only thing oops

…ecProtocol/aztec-packages into lx/turn-on-pairing-points-client-ivc

…points-client-ivc

lucasxia01 · 2025-04-24T16:39:04Z

...etenberg/cpp/src/barretenberg/stdlib/plonk_recursion/aggregation_state/aggregation_state.hpp

    uint32_t set_public()
    {
+        Builder* ctx = P0.get_context();
+        if (ctx->contains_pairing_point_accumulator || ctx->pairing_inputs_public_input_key.is_set()) {


core change: check that we don't set this before

I think you can remove the contains_pairing_point_accumulator check from this conditional, its not used for Honk. Unless this is giving us something useful for plonk but I didnt think set_public was used there

ledwards2225

LG, just a few comments/things to consider

ledwards2225 · 2025-04-24T18:20:50Z

barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp

 using namespace bb;

-static constexpr size_t MAX_NUM_KERNELS = 17;
+static constexpr size_t MAX_NUM_KERNELS = 16;


Shouldn't be any immediate effect because we dont test anything pushing this limit

ledwards2225 · 2025-04-24T18:22:54Z

barretenberg/cpp/src/barretenberg/client_ivc/mock_circuit_producer.hpp

    {
        ClientCircuit circuit{ ivc.goblin.op_queue };
        circuit = create_mock_circuit(ivc, log2_num_gates); // construct mock base logic
-        if (is_kernel) {


the databus shouldnt ever be ignored so its hard to say

barretenberg/cpp/src/barretenberg/dsl/acir_format/block_constraint.test.cpp

ledwards2225 · 2025-04-24T18:26:59Z

barretenberg/cpp/src/barretenberg/dsl/acir_format/ivc_recursion_constraint.test.cpp

        GoblinMockCircuits::add_some_ecc_op_gates(circuit);
        MockCircuits::add_arithmetic_gates(circuit);
-
+        AggregationObject::add_default_pairing_points_to_public_inputs(circuit);


Is it possible to just do this defaulting in circuit finalization instead of in all these individual places? Would that cause issues somehow?

Discussed on slack - it seems possible but would require some ugly code to do so (manual variable instantiation with a lot of random-looking constants). Maybe its still a better pattern than adding this line everywhere.

Another minor issue with this idea is that it would allow for more circuits to silently forget the pairing points, whereas you MUST address them in this current format, either through a set_public() call or a add_default_pairing_points_to_public_inputs() call.

ledwards2225 · 2025-04-24T18:33:49Z

...etenberg/cpp/src/barretenberg/stdlib/plonk_recursion/aggregation_state/aggregation_state.hpp

    uint32_t set_public()
    {
+        Builder* ctx = P0.get_context();
+        if (ctx->contains_pairing_point_accumulator || ctx->pairing_inputs_public_input_key.is_set()) {


I think you can remove the contains_pairing_point_accumulator check from this conditional, its not used for Honk. Unless this is giving us something useful for plonk but I didnt think set_public was used there

ledwards2225 · 2025-04-24T18:34:56Z

...etenberg/cpp/src/barretenberg/stdlib/plonk_recursion/aggregation_state/aggregation_state.hpp

     * @param recursion_separator
     */
-    void aggregate(aggregation_state const& other, typename Curve::ScalarField recursion_separator)
+    void aggregate(aggregation_state const& other)


does this mean we eventually have to pass the transcript into this method once we're generating the challenge correctly though?

ledwards2225 · 2025-04-24T18:35:23Z

...etenberg/cpp/src/barretenberg/stdlib/plonk_recursion/aggregation_state/aggregation_state.hpp

     * @param recursion_separator
     */
-    void aggregate(aggregation_state const& other, typename Curve::ScalarField recursion_separator)
+    void aggregate(aggregation_state const& other)


I'm not sure that's the right separation of concerns if so

ledwards2225 · 2025-04-24T18:39:27Z

barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/databus.hpp

        // Kernel return data is the first public input, followed by app return data
-        data.kernel_return_data_commitment_pub_input_key.start_idx = 0;
-        data.app_return_data_commitment_pub_input_key.start_idx = 8;
+        data.kernel_return_data_commitment_pub_input_key.start_idx = PAIRING_POINT_ACCUMULATOR_SIZE;


Yeah this is bad, would you mind spinning up an issue for this?

ledwards2225 · 2025-04-24T18:41:46Z

yarn-project/bb-prover/src/bb/execute.ts


  try {
-    const args = ['--scheme', 'client_ivc', '-p', proofPath, '-k', keyPath];
+    const args = ['--scheme', 'client_ivc', '-p', proofPath, '-k', keyPath, '-v'];


is this intended to stay?

Yeah, it shouldn't hurt. We already have -v in the arguments for prove

lucasxia01 added 3 commits April 16, 2025 20:39

add this assert because it should catch most cases

cae7dab

Merge remote-tracking branch 'origin/master' into lx/turn-on-pairing-…

566fff6

…points-client-ivc

everything should work except for the pesky ivc rec con test

252ecd0

lucasxia01 requested review from IlyasRidhuan, fcarreiro and jeanmon as code owners April 21, 2025 21:09

lucasxia01 self-assigned this Apr 21, 2025

lucasxia01 added 11 commits April 22, 2025 16:30

fix mock circuits write_vk

af30fb6

update to max kernels as a result of pairing point aggregation

50184b2

remove random magic number

1c2ace0

fix location of databus propagation data for mock vks

ddccf67

Merge remote-tracking branch 'origin/master' into lx/turn-on-pairing-…

484761d

…points-client-ivc

undo change

38a1898

undo change

ce8a3e7

git is bugging out

4c1780f

Update private_base_rollup.nr

6953438

fix test

6ec4215

added verbose flag

d77dc46

lucasxia01 commented Apr 23, 2025

View reviewed changes

barretenberg/cpp/src/barretenberg/dsl/acir_format/block_constraint.test.cpp Outdated Show resolved Hide resolved

lucasxia01 commented Apr 23, 2025

View reviewed changes

lucasxia01 added 5 commits April 24, 2025 15:34

try to fix ultra honk bench

2e63638

Merge branch 'lx/turn-on-pairing-points-client-ivc' of github.com:Azt…

6b9ddd0

…ecProtocol/aztec-packages into lx/turn-on-pairing-points-client-ivc

Merge remote-tracking branch 'origin/master' into lx/turn-on-pairing-…

cbd6710

…points-client-ivc

contains_pairing_point_accumulator is only for plonk now

3131049

fix ultra honk bench

7e83ab7

lucasxia01 commented Apr 24, 2025

View reviewed changes

ledwards2225 approved these changes Apr 24, 2025

View reviewed changes

address comments

15c2b4b

lucasxia01 enabled auto-merge April 24, 2025 19:20

lucasxia01 added this pull request to the merge queue Apr 24, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 24, 2025

lucasxia01 added this pull request to the merge queue Apr 24, 2025

Merged via the queue into master with commit eadb87d Apr 24, 2025
7 checks passed

lucasxia01 deleted the lx/turn-on-pairing-points-client-ivc branch April 24, 2025 21:31

AztecBot mentioned this pull request Apr 24, 2025

chore(master): release 0.87.0 #13800

Merged

lucasxia01 mentioned this pull request Apr 30, 2025

Enable adding the pairing point object to the public inputs for MegaBuilder (CIVC) AztecProtocol/barretenberg#1336

Closed

feat: Add pairing points for all Honk circuits #13701

feat: Add pairing points for all Honk circuits #13701

Uh oh!

Conversation

lucasxia01 commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

lucasxia01 Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasxia01 Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

lucasxia01 commented Apr 21, 2025 •

edited

Loading

lucasxia01 Apr 23, 2025 •

edited

Loading

lucasxia01 Apr 23, 2025 •

edited

Loading