-
Notifications
You must be signed in to change notification settings - Fork 265
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
Changes from 7 commits
3bae733
52c7bf2
90d28b3
6998193
0193003
82d5b3d
b0991e3
de9df0a
e9c0497
b125946
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -204,10 +204,11 @@ 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()); | ||
for (size_t idx = 0; idx < num_verifiers; idx++) { | ||
verifier.verify_folding_proof(stdlib_proof); | ||
} | ||
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 }); | ||
|
@@ -226,7 +227,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(); | ||
info("Folding Recursive Verifier: num gates finalized = ", folding_circuit.num_gates); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -405,6 +410,11 @@ TYPED_TEST(ProtogalaxyRecursiveTests, RecursiveFoldingTest) | |
TestFixture::test_recursive_folding(); | ||
} | ||
|
||
TYPED_TEST(ProtogalaxyRecursiveTests, RecursiveFoldingTwiceTest) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
{ | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -421,7 +421,7 @@ class UltraCircuitBuilder_ : public CircuitBuilderBase<typename Arithmetization_ | |
#endif // NDEBUG | ||
} | ||
|
||
void finalize_circuit(); | ||
void finalize_circuit(const bool ensure_nonzero = false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops