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: Checking finalized sizes + a test of two folding verifiers #8503

Merged
merged 10 commits into from
Sep 12, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ MemBn254CrsFactory::MemBn254CrsFactory(std::vector<g1::affine_element> const& po
std::shared_ptr<bb::srs::factories::ProverCrs<curve::BN254>> MemBn254CrsFactory::get_prover_crs(size_t degree)
{
if (prover_crs_->get_monomial_size() < degree) {
throw_or_abort(format("prover trying to get too many points in MemGrumpkinCrsFactory! ",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo in error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops

throw_or_abort(format("prover trying to get too many points in MemBn254CrsFactory! ",
prover_crs_->get_monomial_size(),
" vs ",
degree));
Expand All @@ -72,7 +72,7 @@ std::shared_ptr<bb::srs::factories::VerifierCrs<curve::BN254>> MemBn254CrsFactor
{

if (prover_crs_->get_monomial_size() < degree) {
throw_or_abort(format("verifier trying to get too many points in MemGrumpkinCrsFactory! ",
throw_or_abort(format("verifier trying to get too many points in MemBn254CrsFactory! ",
prover_crs_->get_monomial_size(),
" vs ",
degree));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ template <typename RecursiveFlavor> class ProtogalaxyRecursiveTests : public tes
* @brief Tests that a valid recursive fold works as expected.
*
*/
static void test_recursive_folding()
static void test_recursive_folding(const size_t num_verifiers = 1)
{
// Create two arbitrary circuits for the first round of folding
InnerBuilder builder1;
Expand Down Expand Up @@ -204,14 +204,30 @@ template <typename RecursiveFlavor> class ProtogalaxyRecursiveTests : public tes

auto verifier =
FoldingRecursiveVerifier{ &folding_circuit, recursive_decider_vk_1, { recursive_decider_vk_2 } };
verifier.verify_folding_proof(stdlib_proof);
info("Folding Recursive Verifier: num gates = ", folding_circuit.get_num_gates());
std::shared_ptr<RecursiveDeciderVerificationKey> accumulator;
for (size_t idx = 0; idx < num_verifiers; idx++) {
accumulator = verifier.verify_folding_proof(stdlib_proof);
if (idx < num_verifiers - 1) { // else the transcript is null in the test below
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, how was there no error from the transcript being null before (when I first reviewed)?

verifier = FoldingRecursiveVerifier{ &folding_circuit,
accumulator,
{ std::make_shared<RecursiveVerificationKey>(
&folding_circuit, decider_vk_1->verification_key) } };
}
}
info("Folding Recursive Verifier: num gates unfinalized = ", folding_circuit.num_gates);
EXPECT_EQ(folding_circuit.failed(), false) << folding_circuit.err();

// Perform native folding verification and ensure it returns the same result (either true or false) as
// calling check_circuit on the recursive folding verifier
InnerFoldingVerifier native_folding_verifier({ decider_vk_1, decider_vk_2 });
std::shared_ptr<InnerDeciderVerificationKey> native_accumulator;
native_folding_verifier.verify_folding_proof(folding_proof.proof);
for (size_t idx = 0; idx < num_verifiers; idx++) {
native_accumulator = native_folding_verifier.verify_folding_proof(folding_proof.proof);
if (idx < num_verifiers - 1) { // else the transcript is null in the test below
native_folding_verifier = InnerFoldingVerifier{ { native_accumulator, decider_vk_1 } };
}
}

// Ensure that the underlying native and recursive folding verification algorithms agree by ensuring the
// manifestsproduced by each agree.
Expand All @@ -226,7 +242,11 @@ template <typename RecursiveFlavor> class ProtogalaxyRecursiveTests : public tes
// Check for a failure flag in the recursive verifier circuit

if constexpr (!IsSimulator<OuterBuilder>) {
// inefficiently check finalized size
folding_circuit.finalize_circuit(/* ensure_nonzero= */ true);
info("Folding Recursive Verifier: num gates finalized = ", folding_circuit.num_gates);
Copy link
Contributor

Choose a reason for hiding this comment

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

so this is now printing out finalized gates without the ensure nonzero gates? But in practice, our circuit is actually going to include these ensure_nonzero gates so I don't understand why we wouldn't want them.

Copy link
Contributor Author

@codygunton codygunton Sep 12, 2024

Choose a reason for hiding this comment

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

Indeed--typo! I agree this is confusing but note that it's only adding better functionality that we should adopt and then using it in one case. The default behavior almost everywhere is to finalize without ensuring Whether it should be that behavior... I'll make an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

could link this to code but also no need if you just want to merge

auto decider_pk = std::make_shared<OuterDeciderProvingKey>(folding_circuit);
info("Dyadic size of verifier circuit: ", decider_pk->proving_key.circuit_size);
OuterProver prover(decider_pk);
auto honk_vk = std::make_shared<typename OuterFlavor::VerificationKey>(decider_pk->proving_key);
OuterVerifier verifier(honk_vk);
Expand Down Expand Up @@ -405,6 +425,11 @@ TYPED_TEST(ProtogalaxyRecursiveTests, RecursiveFoldingTest)
TestFixture::test_recursive_folding();
}

TYPED_TEST(ProtogalaxyRecursiveTests, RecursiveFoldingTwiceTest)
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 new test from refactoring the other test.

{
TestFixture::test_recursive_folding(/* num_verifiers= */ 2);
}

TYPED_TEST(ProtogalaxyRecursiveTests, FullProtogalaxyRecursiveTest)
{

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,15 @@ using namespace bb::crypto;

namespace bb {

template <typename FF> void MegaCircuitBuilder_<FF>::finalize_circuit()
template <typename FF> void MegaCircuitBuilder_<FF>::finalize_circuit(const bool ensure_nonzero)
{
if (ensure_nonzero && !this->circuit_finalized) {
// do the mega part of ensuring all polynomials are nonzero; ultra part will be done inside of
// Ultra::finalize_circuit
add_mega_gates_to_ensure_all_polys_are_non_zero();
}
// All of the gates involved in finalization are part of the Ultra arithmetization
UltraCircuitBuilder_<MegaArith<FF>>::finalize_circuit();
UltraCircuitBuilder_<MegaArith<FF>>::finalize_circuit(ensure_nonzero);
}

/**
Expand All @@ -26,11 +31,8 @@ template <typename FF> void MegaCircuitBuilder_<FF>::finalize_circuit()
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1066): This function adds valid (but arbitrary) gates to
// ensure that the circuit which includes them will not result in any zero-polynomials. It also ensures that the first
// coefficient of the wire polynomials is zero, which is required for them to be shiftable.
template <typename FF> void MegaCircuitBuilder_<FF>::add_gates_to_ensure_all_polys_are_non_zero()
template <typename FF> void MegaCircuitBuilder_<FF>::add_mega_gates_to_ensure_all_polys_are_non_zero()
{
// Most polynomials are handled via the conventional Ultra method
UltraCircuitBuilder_<MegaArith<FF>>::add_gates_to_ensure_all_polys_are_non_zero();

// All that remains is to handle databus related and poseidon2 related polynomials. In what follows we populate the
// calldata with some mock data then constuct a single calldata read gate

Expand Down Expand Up @@ -62,6 +64,23 @@ template <typename FF> void MegaCircuitBuilder_<FF>::add_gates_to_ensure_all_pol
this->queue_ecc_eq();
}

/**
* @brief Ensure all polynomials have at least one non-zero coefficient to avoid commiting to the zero-polynomial.
* This only adds gates for the Goblin polynomials. Most polynomials are handled via the Ultra method,
* which should be done by a separate call to the Ultra builder's non zero polynomial gates method.
*
* @param in Structure containing variables and witness selectors
*/
// TODO(https://github.com/AztecProtocol/barretenberg/issues/1066): This function adds valid (but arbitrary) gates to
// ensure that the circuit which includes them will not result in any zero-polynomials. It also ensures that the first
// coefficient of the wire polynomials is zero, which is required for them to be shiftable.
template <typename FF> void MegaCircuitBuilder_<FF>::add_ultra_and_mega_gates_to_ensure_all_polys_are_non_zero()
{
// Most polynomials are handled via the conventional Ultra method
UltraCircuitBuilder_<MegaArith<FF>>::add_gates_to_ensure_all_polys_are_non_zero();
add_mega_gates_to_ensure_all_polys_are_non_zero();
}

/**
* @brief Add simple point addition operation to the op queue and add corresponding gates
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,9 @@ template <typename FF> class MegaCircuitBuilder_ : public UltraCircuitBuilder_<M
return null_op_idx;
}

void finalize_circuit();
void add_gates_to_ensure_all_polys_are_non_zero();
void finalize_circuit(const bool ensure_nonzero = false);
void add_ultra_and_mega_gates_to_ensure_all_polys_are_non_zero();
void add_mega_gates_to_ensure_all_polys_are_non_zero();

size_t get_num_constant_gates() const override { return 0; }

Expand Down Expand Up @@ -141,7 +142,7 @@ template <typename FF> class MegaCircuitBuilder_ : public UltraCircuitBuilder_<M
MegaCircuitBuilder_<FF> builder; // instantiate new builder

size_t num_gates_prior = builder.get_num_gates();
builder.add_gates_to_ensure_all_polys_are_non_zero();
builder.add_ultra_and_mega_gates_to_ensure_all_polys_are_non_zero();
size_t num_gates_post = builder.get_num_gates(); // accounts for finalization gates

return num_gates_post - num_gates_prior;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@

namespace bb {

template <typename Arithmetization> void UltraCircuitBuilder_<Arithmetization>::finalize_circuit()
template <typename Arithmetization>
void UltraCircuitBuilder_<Arithmetization>::finalize_circuit(const bool ensure_nonzero)
{
/**
* First of all, add the gates related to ROM arrays and range lists.
Expand Down Expand Up @@ -41,6 +42,9 @@ template <typename Arithmetization> void UltraCircuitBuilder_<Arithmetization>::
* our circuit is finalized, and we must not to execute these functions again.
*/
if (!circuit_finalized) {
if (ensure_nonzero) {
add_gates_to_ensure_all_polys_are_non_zero();
Copy link
Contributor

Choose a reason for hiding this comment

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

confusing that this wasn't renamed to add_ultra_gates_to_ensure_all_polys_are_non_zero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a member function of UltraCircuitBuilder so it feels redundant.

}
process_non_native_field_multiplications();
process_ROM_arrays();
process_RAM_arrays();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ class UltraCircuitBuilder_ : public CircuitBuilderBase<typename Arithmetization_
#endif // NDEBUG
}

void finalize_circuit();
void finalize_circuit(const bool ensure_nonzero = false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to preserve the ability to finalize without ensuring nonzero because this is how the function is used in most places (e.g. in the SMT solver). I should raise this issue internally to make sure people know this is happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this makes things more confusing. Like we're going to have unfinalized gate count, sorta finalized gate count without ensure_nonzero gates, and also a finalized gate count...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'd rather not add this but I also don't want to change underlying assumptions--literally only one case DOES ensure gates are nonzero...


void add_gates_to_ensure_all_polys_are_non_zero();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ template <class Flavor> class DeciderProvingKey_ {
std::shared_ptr<typename Flavor::CommitmentKey> commitment_key = nullptr)
{
BB_OP_COUNT_TIME_NAME("DeciderProvingKey(Circuit&)");
circuit.add_gates_to_ensure_all_polys_are_non_zero();
circuit.finalize_circuit();
circuit.finalize_circuit(/* ensure_nonzero = */ true);

// Set flag indicating whether the polynomials will be constructed with fixed block sizes for each gate type
const bool is_structured = (trace_structure != TraceStructure::NONE);
Expand Down
Loading