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 32 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
59 changes: 58 additions & 1 deletion barretenberg/cpp/src/barretenberg/client_ivc/client_ivc.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
#include "barretenberg/client_ivc/test_bench_shared.hpp"
#include "barretenberg/goblin/goblin.hpp"
#include "barretenberg/goblin/mock_circuits.hpp"
#include "barretenberg/protogalaxy/folding_test_utils.hpp"
#include "barretenberg/stdlib_circuit_builders/mega_circuit_builder.hpp"
#include "barretenberg/stdlib_circuit_builders/ultra_circuit_builder.hpp"

#include <gtest/gtest.h>

using namespace bb;
Expand Down Expand Up @@ -403,5 +403,62 @@ 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;

const size_t NUM_CIRCUITS = 2;

// define parameters for two circuits; the first fits within the structured trace, the second overflows
const 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);
}

EXPECT_EQ(check_accumulator_target_sum_manual(ivc.fold_output.accumulator), true);
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;

const size_t NUM_CIRCUITS = 2;

// define parameters for two circuits; the first fits within the structured trace, the second overflows
const std::vector<size_t> log2_num_arith_gates = { 14, 18 };
// 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);
}

EXPECT_EQ(check_accumulator_target_sum_manual(ivc.fold_output.accumulator), true);
EXPECT_TRUE(ivc.prove_and_verify());
};
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#pragma once

#include "../claim.hpp"
#include "barretenberg/commitment_schemes/claim.hpp"
#include "barretenberg/commitment_schemes/commitment_key.hpp"
#include "barretenberg/commitment_schemes/utils/batch_mul_native.hpp"
#include "barretenberg/commitment_schemes/verification_key.hpp"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,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.
const size_t dyadic_circuit_size = circuit.blocks.get_structured_dyadic_size();

// 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 +114,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 +133,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 @@ -220,10 +227,10 @@ class MegaExecutionTraceBlocks : public MegaTraceBlockData<MegaTraceBlock> {
info("");
}

size_t get_structured_dyadic_size()
size_t get_structured_dyadic_size() const
{
size_t total_size = 1; // start at 1 because the 0th row is unused for selectors for Honk
for (auto block : this->get()) {
for (const auto& block : this->get()) {
total_size += block.get_fixed_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.
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
#pragma once

#include "barretenberg/common/assert.hpp"
#include "barretenberg/common/log.hpp"
#include "barretenberg/stdlib_circuit_builders/mega_flavor.hpp"
#include "barretenberg/stdlib_circuit_builders/ultra_flavor.hpp"

namespace bb {

/**
* @brief A debugging utility for checking whether a set of polynomials satisfies the relations for a given Flavor
*
* @tparam Flavor
*/
template <typename Flavor> class RelationChecker {
public:
/**
* @brief Check that the provided polynomials satisfy all relations for a given Flavor
*/
static void check_all([[maybe_unused]] const auto& polynomials, [[maybe_unused]] const auto& params)
{
// default; do nothing
}

/**
* @brief Check that a single specified relation is satisfied for a set of polynomials
*
* @tparam Relation a linearly independent Relation to be checked
* @param polynomials prover polynomials
* @param params a RelationParameters instance
*/
template <typename Relation>
static void check(const auto& polynomials,
const auto& params,
bool is_linearly_independent,
std::string label = "Relation")
{
// Define the appropriate accumulator type for the relation and initialize to zero
typename Relation::SumcheckArrayOfValuesOverSubrelations result;
for (auto& element : result) {
element = 0;
}

// for (size_t i = 0; i < polynomials.get_polynomial_size(); i++) {
maramihali marked this conversation as resolved.
Show resolved Hide resolved
for (size_t i = 0; i < polynomials.w_l.virtual_size(); i++) {
if (is_linearly_independent) {
// Evaluate each constraint in the relation and check that each is satisfied
Relation::accumulate(result, polynomials.get_row(i), params, 1);
size_t subrelation_idx = 0;
for (auto& element : result) {
if (element != 0) {
info("RelationChecker: ",
label,
" relation (subrelation idx: ",
subrelation_idx,
") failed at row idx: ",
i,
".");
ASSERT(false);
}
subrelation_idx++;
}
}
}

if (!is_linearly_independent) {
// Result accumulated across entire execution trace should be zero
for (auto& element : result) {
if (element != 0) {
info("RelationChecker: ", label, " relation (linearly indep.) failed.");
ASSERT(false);
}
}
}
}
};

// Specialization for Ultra
template <> class RelationChecker<bb::UltraFlavor> : public RelationChecker<void> {
using Base = RelationChecker<void>;

public:
static void check_all(const auto& polynomials, const auto& params)
{
using FF = UltraFlavor::FF;

// Linearly independent relations (must be satisfied at each row)
Base::check<UltraArithmeticRelation<FF>>(polynomials, params, true, "UltraArithmetic");
Base::check<UltraPermutationRelation<FF>>(polynomials, params, true, "UltraPermutation");
Base::check<DeltaRangeConstraintRelation<FF>>(polynomials, params, true, "DeltaRangeConstraint");
Base::check<EllipticRelation<FF>>(polynomials, params, true, "Elliptic");
Base::check<AuxiliaryRelation<FF>>(polynomials, params, true, "Auxiliary");
Base::check<Poseidon2ExternalRelation<FF>>(polynomials, params, true, "Poseidon2External");
Base::check<Poseidon2InternalRelation<FF>>(polynomials, params, true, "Poseidon2Internal");

// Linearly dependent relations (must be satisfied as a sum across all rows)
Base::check<LogDerivLookupRelation<FF>>(polynomials, params, false, "LogDerivLookup");
}
};

// Specialization for Mega
template <> class RelationChecker<MegaFlavor> : public RelationChecker<void> {
using Base = RelationChecker<void>;

public:
static void check_all(const auto& polynomials, const auto& params)
{
// Check relations that are shared with Ultra
RelationChecker<UltraFlavor>::check_all(polynomials, params);

using FF = MegaFlavor::FF;

// Linearly independent relations (must be satisfied at each row)
Base::check<EccOpQueueRelation<FF>>(polynomials, params, true, "EccOpQueue");

// Linearly dependent relations (must be satisfied as a sum across all rows)
Base::check<DatabusLookupRelation<FF>>(polynomials, params, false, "DatabusLookup");
}
};

} // namespace bb
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ template <typename Fr> class Polynomial {

std::size_t size() const { return coefficients_.size(); }
std::size_t virtual_size() const { return coefficients_.virtual_size(); }
void increase_virtual_size(const size_t size_in) { coefficients_.increase_virtual_size(size_in); };

Fr* data() { return coefficients_.data(); }
const Fr* data() const { return coefficients_.data(); }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "barretenberg/common/assert.hpp"
#include "barretenberg/common/log.hpp"
#include <cstddef>
#include <memory>

Expand Down Expand Up @@ -47,6 +48,14 @@ template <typename T> struct SharedShiftedVirtualZeroesArray {
const T& get(size_t index, size_t virtual_padding = 0) const
{
static const T zero{};
if (index >= virtual_size_ + virtual_padding) {
info("BAD GET(): index = ",
index,
", virtual_size_ = ",
virtual_size_,
", virtual_padding = ",
virtual_padding);
}
ASSERT(index < virtual_size_ + virtual_padding);
if (index >= start_ && index < end_) {
return data()[index - start_];
Expand All @@ -68,6 +77,12 @@ template <typename T> struct SharedShiftedVirtualZeroesArray {
// Getter for consistency with size();
size_t virtual_size() const { return virtual_size_; }

void increase_virtual_size(const size_t new_virtual_size)
{
ASSERT(new_virtual_size >= virtual_size_); // shrinking is not allowed
virtual_size_ = new_virtual_size;
}

T& operator[](size_t index)
{
ASSERT(index >= start_ && index < end_);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#include "barretenberg/polynomials/gate_separator.hpp"
#include "barretenberg/protogalaxy/protogalaxy_prover.hpp"
#include "barretenberg/protogalaxy/protogalaxy_prover_internal.hpp"
#include "barretenberg/protogalaxy/protogalaxy_verifier.hpp"
#include "barretenberg/ultra_honk/decider_prover.hpp"
#include "barretenberg/ultra_honk/decider_verifier.hpp"

namespace bb {
/**
* @brief Utility to manually compute the target sum of an accumulator and compare it to the one produced in Protogalxy
* to attest correctness.
*/
template <typename Flavor>
static bool check_accumulator_target_sum_manual(const std::shared_ptr<DeciderProvingKey_<Flavor>>& accumulator)
{
using DeciderProvingKeys = DeciderProvingKeys_<Flavor, 2>;
using PGInternal = ProtogalaxyProverInternal<DeciderProvingKeys>;

const size_t accumulator_size = accumulator->proving_key.circuit_size;
PGInternal pg_internal;
const 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
typename Flavor::FF expected_target_sum{ 0 };
for (size_t idx = 0; idx < accumulator_size; idx++) {
expected_target_sum += expected_honk_evals[idx] * expected_gate_separators[idx];
}
return accumulator->target_sum == expected_target_sum;
}
} // namespace bb
Loading
Loading