feat: unify all acir recursion constraints based on RecursionConstraint and proof_type - #7993
Conversation
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. Proof generationEach column represents the number of threads used in proof generation.
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 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
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 size based on fee payment method | Metric | | |
| * @brief Test recursive honk recursive verification | ||
| * | ||
| */ | ||
| TEST_F(AcirIntegrationTest, DISABLED_HonkRecursion) |
There was a problem hiding this comment.
This is just a convenient way to debug honk recursion without getting bb involved
| std::vector<EcAdd> ec_add_constraints; | ||
| std::vector<RecursionConstraint> recursion_constraints; | ||
| std::vector<HonkRecursionConstraint> honk_recursion_constraints; | ||
| std::vector<RecursionConstraint> honk_recursion_constraints; |
There was a problem hiding this comment.
Eventually we can probably move to a model where there is only one vector of RecursionConstraints and they are handled accordingly based on proof_type but that's going to require a larger refactor and I've already strayed a bit from the original intent of this PR :)
| // final recursion output. | ||
| builder.set_recursive_proof(current_aggregation_object); | ||
| } | ||
| }; |
There was a problem hiding this comment.
I assume all of this was just moving around and not any actual changes?
| std::string test_name = "verify_honk_proof"; // arbitrary program with RAM gates | ||
| auto acir_program = get_program_data_from_test_file( | ||
| test_name, | ||
| /*honk_recursion=*/false); // WORKTODO: TODO(https://github.com/AztecProtocol/barretenberg/issues/1013): |
There was a problem hiding this comment.
Its false because with this new pattern, the honk_recursion flag is only used for the "this circuit will be recursively verified with honk" use case. The verifier is set to a honk verifier via the noir verify_proof_with_type call directly
There was a problem hiding this comment.
added a comment to this effect
|
|
||
| AggregationObjectIndices create_honk_recursion_constraints(Builder& builder, | ||
| const HonkRecursionConstraint& input, | ||
| const RecursionConstraint& input, |
There was a problem hiding this comment.
maybe we should add an assert in this function that it's actually taking in a honk recursion constraint
| ) { | ||
| std::verify_proof(verification_key, proof, public_inputs, key_hash); | ||
| // Proof type input specified as 1 to indicate honk | ||
| std::verify_proof_with_type(verification_key, proof, public_inputs, key_hash, 1); |
There was a problem hiding this comment.
nit: probably could make this a global like global HONK_IDENTIFIER, this is non-blocking though
| // the only means for setting the proof type. use of honk_recursion flag in this context can go away | ||
| // once all noir programs (e.g. protocol circuits) are updated to use the new pattern. | ||
| if (honk_recursion && proof_type_in != HONK_RECURSION) { | ||
| // info("WARNING: Recursion type is not being specified correctly via noir verify_proof()!"); |
There was a problem hiding this comment.
| // info("WARNING: Recursion type is not being specified correctly via noir verify_proof()!"); |
?
There was a problem hiding this comment.
Having prints here seemed to make the gate_counts tests on CI complain?
There was a problem hiding this comment.
I was suggesting to delete this as I was surprised to see it left here with no comment
| }; | ||
| af.recursion_constraints.push_back(c); | ||
| af.original_opcode_indices.recursion_constraints.push_back(opcode_index); | ||
| info("Invalid PROOF_TYPE in RecursionConstraint!"); |
There was a problem hiding this comment.
Nit: could probably print the valid proof types here vs. what was specified but this is a nit and can come later. In general, before moving to honk as the main public facing backend for noir we probably need to improve some of the error messages
There was a problem hiding this comment.
I'll leave this one for a follow on since I'll be touching this code a lot in coming weeks
…nstraint.hpp Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
…ges into lde/proof_type
5 participants
This PR moves us towards a model where the type of recursive verifier (plonk/honk, eventually IVC) can be specified from noir by setting a
proof_typeconstant input tostd::verify_proof(). The new mechanism has been integrated into theverify_honk_prooftest program but not yet into the protocol circuits. (The method defaults to Plonk recursion so this is not a breaking change).Other changes/updates:
RecursionConstraint(no moreHonkRecursionConstraint)process_plonk/honk_recursion_constraints()in acir_format for greater clarity. (More cleanup along these lines to come)GateCounter