Skip to content

chore: Merge is part of verification queue#12612

Merged
ledwards2225 merged 10 commits intomasterfrom
lde/incorporate_merge_in_queue
Mar 12, 2025
Merged

chore: Merge is part of verification queue#12612
ledwards2225 merged 10 commits intomasterfrom
lde/incorporate_merge_in_queue

Conversation

@ledwards2225
Copy link
Contributor

@ledwards2225 ledwards2225 commented Mar 10, 2025

Minor cleanup of ClientIvc/Goblin in prep for upcoming merge consistency work. Includes:

  • merge proof is part of ClientIvc::VerificationQueue, i.e. a queue entry is now {proof (oink/PG), merge_proof, VK, type} as opposed to having a separate merge_verification_queue. Due to the way data needs to be shared, its much more convenient for merge proofs to be recursively verified immediately following verification the corresponding oink/PG proof, not separately as before
  • Cleaning out some methods in Goblin that were hold-overs from the days when we thought our IVC scheme would be vanilla recursion with Goblin. (There's more to be done on this thread).

@ledwards2225 ledwards2225 self-assigned this Mar 10, 2025
@ledwards2225 ledwards2225 marked this pull request as ready for review March 10, 2025 19:57
echo SYS=ultra_honk FLOW=prove_then_verify RECURSIVE=true $run_test double_verify_honk_proof
echo SYS=ultra_honk FLOW=prove_then_verify HASH=keccak $run_test assert_statement
echo SYS=ultra_honk FLOW=prove_then_verify ROLLUP=true $run_test verify_rollup_honk_proof

Copy link
Contributor Author

@ledwards2225 ledwards2225 Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are going to be removed by this PR from Cody/Adam. They represent test-only functionality that we never had plans to support. (Creating a CIVC proof for a single circuit).

*/
void ClientIVC::accumulate(ClientCircuit& circuit,
const bool _one_circuit,
[[maybe_unused]] const bool _one_circuit,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last remnants of this mechanism will be cleaned out by the aforementioned PR from Cody/Adam

*
* @param circuit_builder
*/
GoblinAccumulationOutput accumulate(MegaBuilder& circuit_builder)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is left over from the pre-PG days when we thought we'd simply use vanilla recursion with Goblin as our IVC scheme. More cleanup is needed in Goblin to remove unneeded complexity from those days but I'm getting a start on it here.

{
// Append a recursive merge verification of the merge proof
if (merge_proof_exists) {
[[maybe_unused]] auto pairing_points = verify_merge(circuit_builder, merge_proof);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. There is no use case for Goblin appending recursive merge verifiers to circuits.

* @param circuit_builder
* @return PairingPoints
*/
static PairingPoints recursive_verify_merge(Builder& circuit_builder, const StdlibProof<Builder>& proof)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moving this logic from the prover to the verifier. Its debatable whether it should even live here or if CICV should just interact with the Merge classes directly. Keeping this model for now since it plausibly makes sense for Goblin to encapsulate the merge logic.

template <typename CircuitBuilder>
std::array<typename bn254<CircuitBuilder>::Element, 2> MergeRecursiveVerifier_<CircuitBuilder>::verify_proof(
const HonkProof& proof)
const StdlibProof<CircuitBuilder>& proof)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just bringing this in line with the other recursive verifiers which all receive a stdlib proof object

proving_key->proving_key.commitment_key = nullptr;
// #endif

return transcript->proof_data;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returned proof is used in CIVC but it doesn't need to be used elsewhere e.g. in UH where Oink is just the first stage

@ledwards2225 ledwards2225 requested a review from maramihali March 11, 2025 21:48
Copy link

@maramihali maramihali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup :)

@ledwards2225 ledwards2225 merged commit e324582 into master Mar 12, 2025
6 checks passed
@ledwards2225 ledwards2225 deleted the lde/incorporate_merge_in_queue branch March 12, 2025 15:20
sklppy88 pushed a commit that referenced this pull request Mar 12, 2025
🤖 I have created a new Aztec Packages release
---


##
[0.80.0](v0.79.0...v0.80.0)
(2025-03-12)


### ⚠ BREAKING CHANGES

* disable PXE concurrency
([#12637](#12637))

### Features

* add inspect command to bootstrap
([#12641](#12641))
([2f0c527](2f0c527))
* disable PXE concurrency
([#12637](#12637))
([2716898](2716898))
* remove public bytecode from ts ContractArtifact
([#12653](#12653))
([6345158](6345158))


### Bug Fixes

* **cli:** fix tag resolution for `aztec update --contract`
([#12657](#12657))
([b58c123](b58c123))
* **docs:** Fix link to source code after code snippets
([#12658](#12658))
([8bc5daf](8bc5daf))
* fix yarn-project typos in release
([#12652](#12652))
([93a6f4e](93a6f4e))
* handling undefined block number in sync data provider txe
([#12605](#12605))
([b764a9a](b764a9a))
* Inject blob sink client config to archiver start
([#12675](#12675))
([e2e857b](e2e857b))
* Load config from env on archiver start
([#12662](#12662))
([79579f3](79579f3))


### Miscellaneous

* aztec start should use pxe proving by default
([#12676](#12676))
([80cd840](80cd840)),
closes
[#12677](#12677)
* downgrade undici so its engine spec is compatible
([#12659](#12659))
([4f815ea](4f815ea)),
closes
[#12645](#12645)
* fix warning in aztec-nr
([#12647](#12647))
([3831bab](3831bab))
* Merge is part of verification queue
([#12612](#12612))
([e324582](e324582))
* Remove roots arg on __compute_fracs in blob lib
([#12663](#12663))
([8ec910b](8ec910b))
* replace relative paths to noir-protocol-circuits
([f684528](f684528))
* test cli upgrade
([#12506](#12506))
([fc728f3](fc728f3))


### Documentation

* **feat:** Aztec js intro page
([#11804](#11804))
([12d8f3f](12d8f3f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants