Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: IPA Accumulator in Builder #9846

Merged
merged 33 commits into from
Nov 19, 2024
Merged

Conversation

lucasxia01
Copy link
Contributor

@lucasxia01 lucasxia01 commented Nov 8, 2024

Adds IPA claim to builder, pk, vk, so that the verifier knows where to look to extract the IPA claim from. Modifies the UltraRecursiveVerifier to extract out the IPA claim from the public inputs and return it.

Also modifies native verifier to check the IPA claim and proof.

@lucasxia01 lucasxia01 self-assigned this Nov 8, 2024
@lucasxia01 lucasxia01 marked this pull request as ready for review November 14, 2024 00:53
@@ -749,20 +749,21 @@ template <typename Curve_> class IPA {
}

/**
* @brief Takes two IPA claims and accumulates them into 1 IPA claim.
* @details We create an IPA accumulator by running the IPA recursive verifier on each claim. Then, we generate challenges, and use these challenges to compute the new accumulator. We also create the accumulated polynomial.
* @brief Takes two IPA claims and accumulates them into 1 IPA claim. Also computes IPA proof for the claim.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified so that accumulate just returns the IPA proof directly

@@ -33,8 +33,8 @@ using namespace bb::join_split_example::proofs::notes::native;
using key_pair = join_split_example::fixtures::grumpkin_key_pair;

auto create_account_leaf_data(fr const& account_alias_hash,
grumpkin::g1::affine_element const& owner_key,
grumpkin::g1::affine_element const& signing_key)
bb::grumpkin::g1::affine_element const& owner_key,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to add namespace stuff to avoid ambiguous errors...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, surprised by that.

@@ -379,6 +379,9 @@ MegaZKRecursiveFlavor_<UltraCircuitBuilder>>;
template <typename T>
concept HasDataBus = IsMegaFlavor<T>;

template <typename T>
concept DoesRecursiveIPA = IsAnyOf<T, UltraFlavor, UltraFlavorWithZK>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not ultra keccak because that will only be called on the root rollup circuit which will just verify the IPA claim.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is a more precise description of this concept? Is it for flavors that do aggregation OR verification (and possibly both)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its a flavor that will attach an IPA proof and claim to its proof.

tube_builder->add_pairing_point_accumulator(
stdlib::recursion::init_default_agg_obj_indices<Builder>(*tube_builder));
// The tube only calls an IPA recursive verifier once, so we can just add this IPA claim and 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.

for the tube circuit, add the IPA claim

using byte_array_ct = byte_array<Builder>;
using field_ct = field_t<Builder>;
using witness_ct = witness_t<Builder>;
using bool_ct = stdlib::bool_t<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.

namespace changes for some reason.. maybe because of extra includes

bool contains_pairing_point_accumulator = false;

// Public input indices which contain the output IPA opening claim
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding the IPA claim public inputs to the builder

@lucasxia01 lucasxia01 removed the e2e-all CI: Enables this CI job. label Nov 18, 2024
@codygunton codygunton self-requested a review November 18, 2024 18:20
@@ -22,6 +23,45 @@ template <typename Flavor> bool UltraVerifier_<Flavor>::verify_proof(const HonkP
transcript->template get_challenge<FF>("Sumcheck:gate_challenge_" + std::to_string(idx)));
}

const auto recover_fq_from_public_inputs = [](std::array<FF, 4> limbs) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

parses out the IPA claim from the public inputs using the vkey and runs the. native verifier

};

// verify the ipa_proof with this claim
auto crs_factory = std::make_shared<srs::factories::FileCrsFactory<curve::Grumpkin>>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what I can do about this CRS stuff here

Copy link
Contributor

@codygunton codygunton left a comment

Choose a reason for hiding this comment

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

Good work, just asking you to strengthen it a bit

barretenberg/cpp/src/barretenberg/bb/main.cpp Show resolved Hide resolved
@@ -450,6 +450,15 @@ static bb::StdlibProof<Builder> convert_proof_to_witness(Builder* builder, const
return result;
}

template <typename Builder> static bb::HonkProof convert_stdlib_proof_to_native(const StdlibProof<Builder>& proof)
Copy link
Contributor

@codygunton codygunton Nov 18, 2024

Choose a reason for hiding this comment

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

By default we should use constructors to represent the construction of objects. Here is a pattern we can use to do that in this case: https://godbolt.org/z/MrcGooh69.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't follow a constructor pattern here because both HonkProof and StdlibProof are just vectors and not actually classes that we define. Could be reaching the point where we create classes for them

bb::fq(output_claim.opening_pair.evaluation.get_value()) };
Polynomial<fq> challenge_poly = create_challenge_poly(uint32_t(pair_1.log_poly_length.get_value()), native_u_challenges_inv_1, uint32_t(pair_2.log_poly_length.get_value()), native_u_challenges_inv_2, fq(alpha.get_value()));

ASSERT(challenge_poly.evaluate(opening_pair.challenge) == opening_pair.evaluation && "Opening claim does not hold for challenge polynomial.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked that the string evaluates to true... nice trick.

@@ -33,8 +33,8 @@ using namespace bb::join_split_example::proofs::notes::native;
using key_pair = join_split_example::fixtures::grumpkin_key_pair;

auto create_account_leaf_data(fr const& account_alias_hash,
grumpkin::g1::affine_element const& owner_key,
grumpkin::g1::affine_element const& signing_key)
bb::grumpkin::g1::affine_element const& owner_key,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, surprised by that.

@@ -379,6 +379,9 @@ MegaZKRecursiveFlavor_<UltraCircuitBuilder>>;
template <typename T>
concept HasDataBus = IsMegaFlavor<T>;

template <typename T>
concept DoesRecursiveIPA = IsAnyOf<T, UltraFlavor, UltraFlavorWithZK>;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a more precise description of this concept? Is it for flavors that do aggregation OR verification (and possibly both)?

// of the nested aggregation object.
using PairingPointAccumulatorPubInputIndices = std::array<uint32_t, PAIRING_POINT_ACCUMULATOR_SIZE>;

constexpr uint32_t IPA_CLAIM_SIZE = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI IIUC static constexpr will ensure that this is initialized at compilation time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't constexpr also initialize it at compilation time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but I mean static constexpr

const typename PCS::VerifierAccumulator batched_opening_accumulator =
PCS::reduce_verify(batch_opening_claim, ipa_transcript);
// // TODO(https://github.com/AztecProtocol/barretenberg/issues/1142): Handle this return value correctly.
// const typename PCS::VerifierAccumulator batched_opening_accumulator =
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, we no longer call the IPA recursive verifier in the eccvm recursive verifier

@@ -113,7 +113,7 @@ TEST_F(GoblinRecursiveVerifierTests, ECCVMFailure)

// Tamper with the ECCVM proof
for (auto& val : proof.eccvm_proof.pre_ipa_proof) {
if (val > 0) { // tamper by finding the tenth non-zero value and incrementing it by 1
if (val > 0) { // tamper by finding the first non-zero value and incrementing it by 1
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

auto ipa_transcript = std::make_shared<Transcript>(ipa_proof);
bool ipa_result =
IPA<curve::Grumpkin>::reduce_verify(grumpkin_verifier_commitment_key, ipa_claim, ipa_transcript);
if (!ipa_result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not return in the ipa_result case? Seems like it shouldn't compile...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, it continues on to the decider verifier. The function will always return

@lucasxia01 lucasxia01 enabled auto-merge (squash) November 19, 2024 04:25
@lucasxia01 lucasxia01 merged commit 8e74cd0 into master Nov 19, 2024
69 of 72 checks passed
@lucasxia01 lucasxia01 deleted the lx/ipa-accumulator-builder branch November 19, 2024 05:21
TomAFrench added a commit that referenced this pull request Nov 19, 2024
* master: (67 commits)
  chore: Fix bad merge on AztecLMDBStore initializer
  feat: add persisted database of proving jobs (#9942)
  chore: Clean up data configuration (#9973)
  chore: remove public kernels (#10027)
  chore: misc cleanup, docs and renaming (#9968)
  feat: IPA Accumulator in Builder (#9846)
  chore(docs): Updates to token contract (#9954)
  test(avm): minor benchmarking (#9869)
  chore(ci): run `l1-contracts` CI in parallel with `build` step (#10024)
  chore: build acir test programs in parallel to e2e build step (#9988)
  chore: pull out `array_set` pass changes (#9993)
  feat(avm): ephemeral avm tree (#9798)
  fix: don't take down runners with faulty runner check (#10019)
  feat(docs): add transaction profiler docs (#9932)
  chore: hotfix runner wait (#10018)
  refactor: remove EnqueuedCallSimulator (#10015)
  refactor: stop calling public kernels (#9971)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  ...
lucasxia01 added a commit that referenced this pull request Nov 19, 2024
lucasxia01 added a commit that referenced this pull request Nov 19, 2024
Reverts #9846 due to a failure in the
prover-full test
@lucasxia01 lucasxia01 restored the lx/ipa-accumulator-builder branch November 19, 2024 18:25
AztecBot pushed a commit to AztecProtocol/barretenberg that referenced this pull request Nov 20, 2024
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.

3 participants