fix: make vk metadata actual witnesses by lucasxia01 · Pull Request #12459 · AztecProtocol/aztec-packages

lucasxia01 · 2025-03-04T21:48:39Z

Makes the vk metadata witnesses instead of native types. I think only circuit_size actually needs to be a witness, but its fine to make them all witnesses. Still a lot of security concerns around this area since there's missing constraints with these witnesses.

lucasxia01 · 2025-03-05T19:36:26Z

barretenberg/cpp/src/barretenberg/flavor/flavor.hpp

-template <typename PrecomputedCommitments, typename VerifierCommitmentKey>
-class VerificationKey_ : public PrecomputedCommitments {
+template <typename FF_, typename PrecomputedCommitments, typename VerifierCommitmentKey>
+class VerificationKey_ : public PrecomputedCommitments, public VerificationKeyBase<FF_> {


core change: inherit from VerificationKeyBase as well

lucasxia01 · 2025-03-05T19:36:49Z

barretenberg/cpp/src/barretenberg/flavor/flavor.hpp

 *
 */
-class PrecomputedEntitiesBase {
+template <typename FF_> class VerificationKeyBase {


rename this since it is only useful for the VerificationKey. Could honestly be deleted

Im all for deleting needless inheritance structures. If you dont think its adding anything, lets ditch it

lucasxia01 · 2025-03-05T19:37:25Z

barretenberg/cpp/src/barretenberg/stdlib/eccvm_verifier/eccvm_recursive_flavor.hpp

-            this->pub_inputs_offset = native_key->pub_inputs_offset;
+            this->circuit_size = FF::from_witness(builder, native_key->circuit_size);
+            // TODO(https://github.com/AztecProtocol/barretenberg/issues/1283): Use stdlib get_msb.
+            this->log_circuit_size = FF::from_witness(builder, numeric::get_msb(native_key->circuit_size));


probably need constraints between circuit_size and log_circuit_size, or maybe we can just get rid of storing log_circuit_size at all

lucasxia01 · 2025-03-05T19:37:41Z

barretenberg/cpp/src/barretenberg/stdlib/eccvm_verifier/eccvm_recursive_verifier.cpp

    commitments.z_perm = transcript->template receive_from_prover<Commitment>(commitment_labels.z_perm);

    // Execute Sumcheck Verifier
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/1283): Suspicious get_value().


added these TODOs in places which I thought were suspicious.

lucasxia01 · 2025-03-05T19:48:49Z

barretenberg/cpp/src/barretenberg/flavor/flavor.hpp

    using FF = typename VerifierCommitmentKey::Curve::ScalarField;
    using Commitment = typename VerifierCommitmentKey::Commitment;
    std::shared_ptr<VerifierCommitmentKey> pcs_verification_key;
-    bool contains_pairing_point_accumulator = false;


I have no idea if this change is necessary. In the plonk recursive VK, this is just a bool...

also unsure for the PairingPointAccumulatorPubInputIndices and some of the other stuff too.

lucasxia01 · 2025-03-05T19:49:29Z

barretenberg/cpp/src/barretenberg/stdlib/eccvm_verifier/eccvm_recursive_flavor.hpp

     */
    class VerificationKey
-        : public VerificationKey_<ECCVMFlavor::PrecomputedEntities<Commitment>, VerifierCommitmentKey> {
+        : public VerificationKey_<FF, ECCVMFlavor::PrecomputedEntities<Commitment>, VerifierCommitmentKey> {


for recursive flavors, we use FF, which is the stdlib scalar field of the group.

lucasxia01 · 2025-03-05T19:49:47Z

barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/mega_flavor.hpp

     * @todo TODO(https://github.com/AztecProtocol/barretenberg/issues/876)
     */
-    class VerificationKey : public VerificationKey_<PrecomputedEntities<Commitment>, VerifierCommitmentKey> {
+    class VerificationKey : public VerificationKey_<uint64_t, PrecomputedEntities<Commitment>, VerifierCommitmentKey> {


for native flavors, we use uint64_t

lucasxia01 · 2025-03-05T20:02:59Z

barretenberg/cpp/src/barretenberg/stdlib/honk_verifier/oink_recursive_verifier.cpp


    FF circuit_size = transcript->template receive_from_prover<FF>(domain_separator + "circuit_size");
    FF public_input_size = transcript->template receive_from_prover<FF>(domain_separator + "public_input_size");
-    FF pub_inputs_offset = transcript->template receive_from_prover<FF>(domain_separator + "pub_inputs_offset");


this part is irrelevant since it will be rewritten by my other PR

nvm, it causes BadProofFailure to fail

lucasxia01 · 2025-03-05T20:03:37Z

...erg/cpp/src/barretenberg/stdlib/protogalaxy_verifier/recursive_decider_verification_keys.hpp

-                std::max(max_log_circuit_size, static_cast<size_t>(key->verification_key->log_circuit_size));
+            max_log_circuit_size = std::max(
+                max_log_circuit_size,
+                static_cast<size_t>(static_cast<uint32_t>(key->verification_key->log_circuit_size.get_value())));


should've added the TODO here too

lucasxia01 · 2025-03-05T20:04:21Z

...tenberg/cpp/src/barretenberg/stdlib/translator_vm_verifier/translator_recursive_verifier.cpp

@@ -76,7 +76,7 @@ std::array<typename Flavor::GroupElement, 2> TranslatorRecursiveVerifier_<Flavor
    CommitmentLabels commitment_labels;

    const FF circuit_size = transcript->template receive_from_prover<FF>("circuit_size");


maybe circuit_size in the eccvm and translator flavors can also be removed from the proof at some point

ledwards2225

LGTM - one step closer to getting these recursive verifiers in order

ledwards2225 · 2025-03-05T19:52:53Z

barretenberg/cpp/src/barretenberg/flavor/flavor.hpp

 *
 */
-class PrecomputedEntitiesBase {
+template <typename FF_> class VerificationKeyBase {


Im all for deleting needless inheritance structures. If you dont think its adding anything, lets ditch it

…_accumulator back to a bool instead of a stdlib field

This reverts commit dada06f.

🤖 I have created a new Aztec Packages release --- ## [0.78.0](v0.77.1...v0.78.0) (2025-03-07) ### ⚠ BREAKING CHANGES * convert `TraitMethodNotInScope` to error (noir-lang/noir#7427) * bump bb version to v0.77.0 (noir-lang/noir#7599) * remove merkle module from stdlib (noir-lang/noir#7582) * remove deprecated hash functions from stdlib (noir-lang/noir#7477) * **frontend:** Restrict capturing mutable variable in lambdas (noir-lang/noir#7488) * remove U128 struct from stdlib (noir-lang/noir#7529) ### Features * **barretenberg:** Graph methods for circuit analysis (part 2) ([#12130](#12130)) ([ec4c0c4](ec4c0c4)) * **cli:** Log and replay oracle transcript (noir-lang/noir#7417) ([f13b729](f13b729)) * Compare bincode to CBOR, FlexBuffers and Protobuf - implement best (noir-lang/noir#7513) ([8eb727c](8eb727c)) * **experimental:** Enable ownership syntax (noir-lang/noir#7603) ([1a3c112](1a3c112)) * **experimental:** Issue errors for unreachable match branches (noir-lang/noir#7556) ([f13b729](f13b729)) * nullify just-added notes ([#12552](#12552)) ([dcba7a4](dcba7a4)) * perform constant sha256 compressions at compile-time (noir-lang/noir#7566) ([f13b729](f13b729)) * relate errors to macro built-ins errors (noir-lang/noir#7609) ([fbaa634](fbaa634)) * simplify simple conditionals for brillig (noir-lang/noir#7205) ([f13b729](f13b729)) * Support `<Type as Trait>::method` in expressions (noir-lang/noir#7551) ([f13b729](f13b729)) * Sync from aztec-packages (noir-lang/noir#7606) ([8eb727c](8eb727c)) * teardown in call interface ([#12499](#12499)) ([062df02](062df02)) * translation evaluations with zk ([#12222](#12222)) ([568982d](568982d)) ### Bug Fixes * **avm:** use the correct number of rows in check_interaction ([#12519](#12519)) ([b1284ef](b1284ef)) * aztec-up ([#12509](#12509)) ([3ddb6de](3ddb6de)) * bbup ([#12555](#12555)) ([e7b5353](e7b5353)) * Bitwise lookup ([#12471](#12471)) ([a38f353](a38f353)) * **ci:** remove regex - transfer explicitly ([#12525](#12525)) ([352bb1d](352bb1d)) * Cl/fix arm anvil ([#12565](#12565)) ([e4bfbd1](e4bfbd1)) * compare Quoted by expanding interned values (noir-lang/noir#7602) ([1a3c112](1a3c112)) * Display causes but not stack trace in CLI error report (noir-lang/noir#7584) ([f13b729](f13b729)) * **experimental:** Fix execution of match expressions with multiple branches (noir-lang/noir#7570) ([1a3c112](1a3c112)) * fix a few cases where safety comment wasn't correctly identified (noir-lang/noir#7548) ([f13b729](f13b729)) * fix bbup and add CI ([#12541](#12541)) ([1b2604c](1b2604c)) * Fix the config ([#12513](#12513)) ([fb9fac6](fb9fac6)) * **frontend:** Restrict capturing mutable variable in lambdas (noir-lang/noir#7488) ([f13b729](f13b729)) * FunctionDefinition::as_typed_expr didn't work well for trait imp… (noir-lang/noir#7611) ([1a3c112](1a3c112)) * Log to `stderr` (noir-lang/noir#7585) ([f13b729](f13b729)) * **LSP:** references/rename only when underlying span has the correct… (noir-lang/noir#7598) ([8eb727c](8eb727c)) * make vk metadata actual witnesses ([#12459](#12459)) ([dada06f](dada06f)) * no fast deployments when the boot node needs to restart. ([#12557](#12557)) ([866582e](866582e)) * **node:** drop log level of handler not registered ([#12523](#12523)) ([cb7e42d](cb7e42d)) * override bb path in cli-wallet PXE config ([#12511](#12511)) ([0c3024e](0c3024e)) * publish-bb-mac.yml version replace ([#12554](#12554)) ([7e89dfb](7e89dfb)) * release bb-mac ([fac5fb5](fac5fb5)) * Revert "make vk metadata actual witnesses" ([#12534](#12534)) ([ed46a3c](ed46a3c)) * shift right overflow in ACIR with unknown var now returns zero (noir-lang/noir#7509) ([f13b729](f13b729)) * TokensPrettyPrinter was missing some spaces between tokens (noir-lang/noir#7607) ([1a3c112](1a3c112)) * yarn-project e2e bench ([#12547](#12547)) ([b40b904](b40b904)) ### Miscellaneous * add some extra tests (noir-lang/noir#7544) ([f13b729](f13b729)) * add underscore parameter documentation (noir-lang/noir#7562) ([1a3c112](1a3c112)) * add yaml aliases in .test_patterns.yml ([#12516](#12516)) ([3ee8d51](3ee8d51)) * address some frontend tests TODOs (noir-lang/noir#7554) ([f13b729](f13b729)) * addressing remaining feedback in PR 12182 ([#12494](#12494)) ([f733879](f733879)), closes [#12193](#12193) * bump `light-poseidon` (noir-lang/noir#7568) ([f13b729](f13b729)) * bump bb version to v0.77.0 (noir-lang/noir#7599) ([f13b729](f13b729)) * bump external pinned commits (noir-lang/noir#7561) ([f13b729](f13b729)) * bump external pinned commits (noir-lang/noir#7565) ([f13b729](f13b729)) * bump external pinned commits (noir-lang/noir#7581) ([f13b729](f13b729)) * bump external pinned commits (noir-lang/noir#7601) ([f13b729](f13b729)) * bump external pinned commits (noir-lang/noir#7618) ([fbaa634](fbaa634)) * bump ring to address advisory (noir-lang/noir#7619) ([fbaa634](fbaa634)) * Cleaner PXE ([#12515](#12515)) ([a69f416](a69f416)) * cleanup committing and masking utility ([#12514](#12514)) ([9f57048](9f57048)) * **cli:** exclude kind smoke test from flake list ([#12518](#12518)) ([778bfa6](778bfa6)) * **cli:** Forward `nargo execute` to `noir_artifact_cli` (noir-lang/noir#7406) ([f13b729](f13b729)) * convert `TraitMethodNotInScope` to error (noir-lang/noir#7427) ([fbaa634](fbaa634)) * explode aliases when looking up owners in `.test_patterns.yml` ([#12526](#12526)) ([2e0d791](2e0d791)) * fix trait import issues ([#12500](#12500)) ([fd9f145](fd9f145)) * Fix yarn install immutable issues ([#12539](#12539)) ([fb9ada3](fb9ada3)), closes [#12538](#12538) * More config defaults and forward p2p ports ([#12529](#12529)) ([2c45fb9](2c45fb9)) * **node:** return correct node version ([#12520](#12520)) ([5502901](5502901)) * **profiler:** Add option to only get the total sample count for the `execution-opcodes` command (noir-lang/noir#7578) ([f13b729](f13b729)) * put RcTracker as part of the DIE context (noir-lang/noir#7309) ([f13b729](f13b729)) * remove deprecated hash functions from stdlib (noir-lang/noir#7477) ([f13b729](f13b729)) * remove FileDiagnostic (noir-lang/noir#7546) ([f13b729](f13b729)) * remove merkle module from stdlib (noir-lang/noir#7582) ([f13b729](f13b729)) * Remove scope interpolation from env vars ([#12522](#12522)) ([70942e9](70942e9)) * remove U128 struct from stdlib (noir-lang/noir#7529) ([f13b729](f13b729)) * replace relative paths to noir-protocol-circuits ([f20c0dd](f20c0dd)) * replace relative paths to noir-protocol-circuits ([4365064](4365064)) * restore bb --version ([#12542](#12542)) ([ab13d43](ab13d43)) * restore method syntax on `get_storage_slot` calls ([#12532](#12532)) ([8e9f594](8e9f594)) * rm unused methods ([#12544](#12544)) ([ed1dbdc](ed1dbdc)) * some SSA improvements (noir-lang/noir#7588) ([f13b729](f13b729)) * **spartan:** kind test speedup ([#12478](#12478)) ([8ede7b1](8ede7b1)) * **ssa:** Turn the Brillig constraints check back on by default (noir-lang/noir#7404) ([f13b729](f13b729)) * track more critical libraries (noir-lang/noir#7604) ([f13b729](f13b729)) * update and lock AVM's lockfile ([#12533](#12533)) ([2babc50](2babc50)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

the precomputed entities base is only used for vk, so make it so

caa01aa

lucasxia01 self-assigned this Mar 5, 2025

lucasxia01 added 3 commits March 5, 2025 16:31

try to change the field type of VerificationKeyBase

b76a659

update some avm stuff

4850c58

fix build

6aa925b

lucasxia01 marked this pull request as ready for review March 5, 2025 17:56

lucasxia01 requested review from IlyasRidhuan, Maddiaa0, fcarreiro and jeanmon as code owners March 5, 2025 17:56

lucasxia01 added 3 commits March 5, 2025 18:15

fix avm build

ae76566

add TODOs and fix wasm build

6691011

fix this time hopefully

36f35e3

lucasxia01 commented Mar 5, 2025

View reviewed changes

ledwards2225 approved these changes Mar 5, 2025

View reviewed changes

lucasxia01 added 3 commits March 5, 2025 21:04

delete VerificationKeyBase entirely and revert contains_pairing_point…

e80a744

…_accumulator back to a bool instead of a stdlib field

missing TODO

d781a07

fix test

2e9ec7f

lucasxia01 enabled auto-merge (squash) March 6, 2025 00:01

lucasxia01 merged commit dada06f into master Mar 6, 2025
7 checks passed

lucasxia01 deleted the lx/make-vk-metadata-into-witnesses branch March 6, 2025 00:46

AztecBot mentioned this pull request Mar 6, 2025

chore(master): release 0.78.0 #12508

Merged

ludamad restored the lx/make-vk-metadata-into-witnesses branch March 6, 2025 17:08

ludamad deleted the lx/make-vk-metadata-into-witnesses branch March 6, 2025 17:08

ludamad added a commit that referenced this pull request Mar 6, 2025

Revert "fix: make vk metadata actual witnesses (#12459)"

9d2067d

This reverts commit dada06f.

		@@ -76,7 +76,7 @@ std::array<typename Flavor::GroupElement, 2> TranslatorRecursiveVerifier_<Flavor
		CommitmentLabels commitment_labels;

		const FF circuit_size = transcript->template receive_from_prover<FF>("circuit_size");

Conversation

lucasxia01 commented Mar 4, 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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lucasxia01 commented Mar 4, 2025 •

edited

Loading