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: swap polys to facilitate dynamic trace overflow #9976

Merged
merged 36 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
560373c
swapping in folding appears to work
ledwards2225 Nov 14, 2024
981074c
WiP; dyadic size bump tst failing without real clues
ledwards2225 Nov 15, 2024
090e720
Merge branch 'master' into lde/swap_polys
ledwards2225 Nov 18, 2024
2eded0b
Merge branch 'master' into lde/swap_polys
ledwards2225 Nov 18, 2024
d06c1e8
changes from other PR; test passes if overflow is set in advance
ledwards2225 Nov 18, 2024
b4cb9de
WiP debug with check circuit shows decider sumcheck failure
ledwards2225 Nov 18, 2024
a7231ef
add test that proves out dynamic virtual size increase
ledwards2225 Nov 18, 2024
0cf3757
move relation checker to pk inspector
ledwards2225 Nov 18, 2024
f7e8fc7
add proper relation checker class
ledwards2225 Nov 19, 2024
35c15a0
debug
ledwards2225 Nov 19, 2024
515fe8b
WiP virtual size increase pg test
ledwards2225 Nov 19, 2024
e07eb4d
basic pg test passes after replacing cir size with vir size
ledwards2225 Nov 19, 2024
7d4ba63
correctly set circuit size in pg verifier; pg decide test passes
ledwards2225 Nov 19, 2024
ba7bfd5
Merge branch 'master' into lde/swap_polys
ledwards2225 Nov 20, 2024
559bb2e
turn off debugging check circuit calls
ledwards2225 Nov 20, 2024
916fc98
comments and fix build error
ledwards2225 Nov 20, 2024
b23cd91
corrctly set circuit size in pg rec verifier
ledwards2225 Nov 20, 2024
864ee3a
comments and cleanup
ledwards2225 Nov 20, 2024
bfb2a70
add some debug utility in ivc tests
ledwards2225 Nov 20, 2024
2494af1
debug stuff
ledwards2225 Nov 20, 2024
531c0d1
comments
ledwards2225 Nov 20, 2024
0b582eb
loads of cleanup and execution trace tracker needs fixing
maramihali Nov 26, 2024
d781974
fixed
maramihali Nov 26, 2024
0b714d2
cleanup and enable parallelisation
maramihali Nov 26, 2024
c9e6af0
create todos
maramihali Nov 26, 2024
90277ab
make a folding test utils
maramihali Nov 26, 2024
0891578
Merge remote-tracking branch 'origin/master' into lde/swap_polys
maramihali Nov 26, 2024
9f05f0b
undo comment
maramihali Nov 26, 2024
c13923b
revert submodule changes
maramihali Nov 26, 2024
48052d0
some constifying
maramihali Nov 26, 2024
681c1e6
unused variable error when building everything
maramihali Nov 26, 2024
1f79b46
now both builds work
maramihali Nov 27, 2024
e7d8499
wopsie, I removed the commitment key from DeciderProvingKey construct…
maramihali Nov 27, 2024
1f4df0f
fix commitment ket caveat
maramihali Nov 27, 2024
cd92b66
update PR based on review
maramihali Nov 27, 2024
067cbb0
remove extra boolean in compute_row_evaluations
maramihali Nov 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,10 @@ void ClientIVC::accumulate(ClientCircuit& circuit, const std::shared_ptr<Verific

initialized = true;
} else { // Otherwise, fold the new key into the accumulator
vinfo("in folding prover");
FoldingProver folding_prover({ fold_output.accumulator, proving_key }, trace_usage_tracker);
vinfo("constructed folding prover");
fold_output = folding_prover.prove();
vinfo("constructed folding proof");
info("constructed folding proof");

// Add fold proof and corresponding verification key to the verification queue
verification_queue.push_back(bb::ClientIVC::VerifierInputs{ fold_output.proof, honk_vk, QUEUE_TYPE::PG });
Expand Down Expand Up @@ -279,6 +279,8 @@ HonkProof ClientIVC::construct_and_prove_hiding_circuit()
merge_verification_queue.emplace_back(merge_proof);

auto decider_pk = std::make_shared<DeciderProvingKey>(builder);
// WORKTODO: This fails in the dynamic accum expansion case
ASSERT(CircuitChecker::check(builder));
honk_vk = std::make_shared<VerificationKey>(decider_pk->proving_key);
MegaProver prover(decider_pk);

Expand Down
106 changes: 106 additions & 0 deletions barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,45 @@ class ClientIVCTests : public ::testing::Test {
using DeciderVerificationKeys = DeciderVerificationKeys_<Flavor>;
using FoldingVerifier = ProtogalaxyVerifier_<DeciderVerificationKeys>;

// WORKTODO: duplicate of similar logic in pg test suite; put in some more central location for use by all modules
static void check_accumulator_target_sum_consistency(const std::shared_ptr<DeciderProvingKey>& accumulator)

{
info("in check accumulator target sum consistency, size: ", accumulator->proving_key.circuit_size);
ProtogalaxyProverInternal<DeciderProvingKeys> pg_internal;
auto expected_honk_evals = pg_internal.compute_row_evaluations(
accumulator->proving_key.polynomials, accumulator->alphas, accumulator->relation_parameters);
// Construct pow(\vec{betas*}) as in the paper
GateSeparatorPolynomial expected_gate_separators(accumulator->gate_challenges,
accumulator->gate_challenges.size());

// Compute the corresponding target sum and create a dummy accumulator
FF expected_target_sum{ 0 };
for (size_t idx = 0; idx < accumulator->proving_key.circuit_size; idx++) {
expected_target_sum += expected_honk_evals[idx] * expected_gate_separators[idx];
}
info("expected target sum: ", expected_target_sum);
info("acc target sum: ", accumulator->target_sum);
EXPECT_TRUE(accumulator->target_sum == expected_target_sum);
}

// WORKTODO: similar to above, this doesnt really belong here
static void fold_verify_then_decider_prove_and_verify(const ClientIVC& ivc)
{
const auto& queue_entry = ivc.verification_queue.back();
const auto& fold_proof = queue_entry.proof;
auto key_to_fold = std::make_shared<ClientIVC::DeciderVerificationKey>(queue_entry.honk_verification_key);
const auto& prover_accumulator = ivc.fold_output.accumulator;

ClientIVC::FoldingVerifier folding_verifier({ ivc.verifier_accumulator, key_to_fold });
auto verifier_accumulator = folding_verifier.verify_folding_proof(fold_proof);

DeciderProver decider_prover(prover_accumulator);
DeciderVerifier decider_verifier(verifier_accumulator);
HonkProof decider_proof = decider_prover.construct_proof();
EXPECT_TRUE(decider_verifier.verify_proof(decider_proof));
}

/**
* @brief Construct mock circuit with arithmetic gates and goblin ops
* @details Defaulted to add 2^16 gates (which will bump to next power of two with the addition of dummy gates).
Expand Down Expand Up @@ -403,5 +442,72 @@ TEST_F(ClientIVCTests, StructuredTraceOverflow)
log2_num_gates += 1;
}

EXPECT_TRUE(ivc.prove_and_verify());
};

/**
* @brief Test dynamic structured trace overflow block mechanism
* @details Tests the case where the required overflow capacity is not known until runtime. Accumulates two circuits,
* the second of which overflows the trace but not enough to change the dyadic circuit size and thus there is no need
* for a virtual size increase of the first key.
*
*/
TEST_F(ClientIVCTests, DynamicOverflow)
{
// Define trace settings with zero overflow capacity
ClientIVC ivc{ { SMALL_TEST_STRUCTURE, /*overflow_capacity=*/0 } };
Copy link
Contributor

Choose a reason for hiding this comment

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

If small tests structure changes behind the scenes, this test could silently break (i.e., it may continue to pass but cease to test what it's supposed to). Will you please define a new trace structure in this file for use in these two tests only?


MockCircuitProducer circuit_producer;

size_t NUM_CIRCUITS = 2;

// define parameters for two circuits; the first fits within the structured trace, the second overflows
std::vector<size_t> log2_num_arith_gates = { 14, 16 };
// Accumulate
for (size_t idx = 0; idx < NUM_CIRCUITS; ++idx) {
auto circuit = circuit_producer.create_next_circuit(ivc, log2_num_arith_gates[idx]);
ivc.accumulate(circuit);
}

// DEBUG: check consistency of the target sum computed internal to ivc
check_accumulator_target_sum_consistency(ivc.fold_output.accumulator);

// DEBUG: run native pg verifier then native decider prover/verifier
fold_verify_then_decider_prove_and_verify(ivc);

EXPECT_TRUE(ivc.prove_and_verify());
};

/**
* @brief Test dynamic trace overflow where the dyadic circuit size also increases
* @details Accumulates two circuits, the second of which overflows the trace structure and leads to an increased dyadic
* circuit size. This requires the virtual size of the polynomials in the first key to be increased accordingly which
* should be handled automatically in PG/ClientIvc.
*
*/
TEST_F(ClientIVCTests, DynamicOverflowCircuitSizeChange)
{
uint32_t overflow_capacity = 0;
// uint32_t overflow_capacity = 1 << 1;
ClientIVC ivc{ { SMALL_TEST_STRUCTURE, overflow_capacity } };

MockCircuitProducer circuit_producer;

size_t NUM_CIRCUITS = 2;

// define parameters for two circuits; the first fits within the structured trace, the second overflows
std::vector<size_t> log2_num_arith_gates = { 14, 18, 16 };
// Accumulate
for (size_t idx = 0; idx < NUM_CIRCUITS; ++idx) {
auto circuit = circuit_producer.create_next_circuit(ivc, log2_num_arith_gates[idx]);
ivc.accumulate(circuit);
}

check_accumulator_target_sum_consistency(ivc.fold_output.accumulator);
// DEBUG: check consistency of the target sum computed internal to ivc

// DEBUG: run native pg verifier then native decider prover/verifier
fold_verify_then_decider_prove_and_verify(ivc);

EXPECT_TRUE(ivc.prove_and_verify());
};
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ TEST_F(IvcRecursionConstraintTest, GenerateVK)
// Construct kernel consisting only of the kernel completion logic
AcirProgram program = construct_mock_kernel_program(ivc.verification_queue, { num_app_public_inputs });
Builder kernel = acir_format::create_kernel_circuit(program.constraints, ivc);
// WORKTODO: this would normally happen in accumulate()
// Mimic what would normally happen in accumulate()
kernel.add_pairing_point_accumulator(stdlib::recursion::init_default_agg_obj_indices<Builder>(kernel));

auto proving_key = std::make_shared<DeciderProvingKey_<MegaFlavor>>(kernel, trace_settings);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ void compute_logderivative_inverse(Polynomials& polynomials, auto& relation_para
using Accumulator = typename Relation::ValueAccumulator0;
constexpr size_t READ_TERMS = Relation::READ_TERMS;
constexpr size_t WRITE_TERMS = Relation::WRITE_TERMS;

static_cast<void>(circuit_size);
auto& inverse_polynomial = Relation::template get_inverse_polynomial(polynomials);
for (size_t i = 0; i < circuit_size; ++i) {
// TODO(https://github.com/AztecProtocol/barretenberg/issues/940): avoid get_row if possible.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ struct ExecutionTraceUsageTracker {
using MegaTraceActiveRanges = MegaTraceBlockData<Range>;
using MegaTraceFixedBlockSizes = MegaExecutionTraceBlocks;

TraceStructure max_sizes; // max utilization of each block
MegaTraceFixedBlockSizes fixed_sizes; // fixed size of each block prescribed by structuring
TraceStructure max_sizes; // max utilization of each block
// Fixed size of each block prescribed by strucuring and the highest size of an overflow block encountered
MegaTraceFixedBlockSizes fixed_sizes;
// Store active ranges based on the most current accumulator and those based on all but the most recently
// accumulated circuit. The former is needed for the combiner calculation and the latter for the perturbator.
std::vector<Range> active_ranges;
Expand Down Expand Up @@ -48,7 +49,7 @@ struct ExecutionTraceUsageTracker {
}

// Update the max block utilization and active trace ranges based on the data from a provided circuit
void update(const Builder& circuit)
void update(Builder& circuit)
{
// Update the max utilization of each gate block
for (auto [block, max_size] : zip_view(circuit.blocks.get(), max_sizes.get())) {
Expand All @@ -72,8 +73,11 @@ struct ExecutionTraceUsageTracker {
}

// The active ranges must also include the rows where the actual databus and lookup table data are stored.
// (Note: lookup tables are constructed at the end of the trace; databus data is constructed at the start).
size_t dyadic_circuit_size = fixed_sizes.get_structured_dyadic_size();
// (Note: lookup tables are constructed at the end of the trace; databus data is constructed at the start) so we
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment will break if the ordering in the trace structure ever changes--there's little chance that the person changing the structure remembers this comment exists, if they ever knew.

Copy link
Contributor

Choose a reason for hiding this comment

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

a way to make this more robust is to update the active ranges as we are constructing the DeciderProvingKey so if the position of the rows changes it is obvious that the tracker's active ranges also need to be updated, will add an issue.

// need to determine the dyadic size for this. We call the size function on the current circuit which will have
// the same fixed block sizes but might also have an overflow block potentially influencing the dyadic circuit
// size.
size_t dyadic_circuit_size = circuit.blocks.get_structured_dyadic_size();
Copy link
Contributor

@maramihali maramihali Nov 26, 2024

Choose a reason for hiding this comment

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

This was the major issue: The fixed sizes are set using the trace settings so the overflow block was 0 and hence the dyadic_circuit_size was not computed correctly. This was leading us to not take into account the lookup contributions at the end of the current circuit trace in PG. Modifying the fixed_sizes to include the overflow block is not easily possible in the current design and it makes sense to compute the dyadic_circuit_size using the current circuit. It will have the same TraceSetting (hence the same sizes also contained in the fixed_sizes structure ) but also the correct size of the overflow block. I expect we might be able to get rid of the fixed_sizes structure as a follow up or do something a bit cleaner in a follow up


// TODO(https://github.com/AztecProtocol/barretenberg/issues/1152): should be able to use simply Range{ 0,
// max_databus_size } but this breaks for certain choices of num_threads.
Expand Down Expand Up @@ -111,6 +115,13 @@ struct ExecutionTraceUsageTracker {
"lookup",
"overflow" };

std::vector<std::string> active_ranges_labels = [this] {
maramihali marked this conversation as resolved.
Show resolved Hide resolved
std::vector<std::string> result = block_labels;
result.push_back("databus table data");
result.push_back("lookup table data");
return result;
}();

void print()
{
info("Minimum required block sizes for structured trace: ");
Expand All @@ -123,7 +134,17 @@ struct ExecutionTraceUsageTracker {
void print_active_ranges()
{
info("Active regions of accumulator: ");
for (auto [label, range] : zip_view(block_labels, active_ranges)) {
for (auto [label, range] : zip_view(active_ranges_labels, active_ranges)) {
std::cout << std::left << std::setw(20) << (label + ":") << "(" << range.first << ", " << range.second
<< ")" << std::endl;
}
info("");
}

void print_previous_active_ranges()
Copy link
Contributor

Choose a reason for hiding this comment

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

when i was printing the active ranges it wasn't reflecting everything due to the lookup and databus caveat so I think it made sense to refine the labels

{
info("Active regions of previous accumulator: ");
for (auto [label, range] : zip_view(active_ranges_labels, previous_active_ranges)) {
std::cout << std::left << std::setw(20) << (label + ":") << "(" << range.first << ", " << range.second
<< ")" << std::endl;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,17 @@ template <typename T> struct MegaTraceBlockData {

std::vector<std::string_view> get_labels() const
{
return { "ecc_op", "pub_inputs", "busread",
"arithmetic", "delta_range", "elliptic",
"aux", "poseidon2_external", "poseidon2_internal",
"lookup" };
return { "ecc_op",
"pub_inputs",
"busread",
"arithmetic",
"delta_range",
"elliptic",
"aux",
"poseidon2_external",
"poseidon2_internal",
"lookup",
"overflow" };
}

auto get()
Expand Down Expand Up @@ -174,7 +181,6 @@ class MegaExecutionTraceBlocks : public MegaTraceBlockData<MegaTraceBlock> {
this->overflow.fixed_size = settings.overflow_capacity;
}

// WORKTODO: use or remove
MegaExecutionTraceBlocks(const TraceSettings& settings)
: MegaExecutionTraceBlocks()
{
Expand Down Expand Up @@ -217,6 +223,7 @@ class MegaExecutionTraceBlocks : public MegaTraceBlockData<MegaTraceBlock> {
for (auto block : this->get()) {
total_size += block.get_fixed_size();
}
info("total size: ");

auto log2_n = static_cast<size_t>(numeric::get_msb(total_size));
if ((1UL << log2_n) != (total_size)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ typename Flavor::FF compute_public_input_delta(std::span<const typename Flavor::
// Note: The public inputs may be offset from the 0th index of the wires, for example due to the inclusion of an
// initial zero row or Goblin-stlye ECC op gates. Accordingly, the indices i in the above formulas are given by i =
// [0, m-1] + offset, i.e. i = offset, 1 + offset, …, m - 1 + offset.

// TODO(https://github.com/AztecProtocol/barretenberg/issues/1158): Ensure correct construction of public input
// delta in the face of increases to virtual size caused by execution trace overflow
Field numerator_acc = gamma + (beta * Field(domain_size + offset));
Field denominator_acc = gamma - beta * Field(1 + offset);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include "barretenberg/common/assert.hpp"
#include "barretenberg/common/log.hpp"

namespace bb::proving_key_inspector {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#include "relation_checker.hpp"

// Hack to make the module compile.
Loading