-
Notifications
You must be signed in to change notification settings - Fork 271
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: Protogalaxy folding of challenges #2935
Changes from 26 commits
a1c95c5
bee3ca2
6e77bce
98aec2a
d3bde61
b6df92e
12b25b0
ffaf07b
ddeaba3
bd8e812
ee83443
f087936
7092593
093255f
83de942
dc77501
0dac01d
dacacf9
2bbac89
4c207ee
03e31e7
f3caefa
a5c985c
567346d
24c8c75
3fe5d56
c0a6114
cd644d0
248d4e0
5e9c3b5
1ffdb98
22a6ee3
a327957
d0deb59
869b830
7f33f9f
b9bd1b4
e4097c3
02f1913
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 |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
#include "barretenberg/proof_system/relations/ecc_vm/ecc_set_relation.hpp" | ||
#include "barretenberg/proof_system/relations/ecc_vm/ecc_transcript_relation.hpp" | ||
#include "barretenberg/proof_system/relations/ecc_vm/ecc_wnaf_relation.hpp" | ||
#include "barretenberg/proof_system/relations/relation_parameters.hpp" | ||
#include "barretenberg/proof_system/relations/relation_types.hpp" | ||
#include <array> | ||
#include <concepts> | ||
|
@@ -65,15 +66,15 @@ template <typename CycleGroup_T, typename Curve_T, typename PCS_T> class ECCVMBa | |
sumcheck::ECCVMLookupRelation<FF>>; | ||
|
||
using LookupRelation = sumcheck::ECCVMLookupRelation<FF>; | ||
static constexpr size_t MAX_RELATION_LENGTH = get_max_relation_length<Relations>(); | ||
static constexpr size_t MAX_PARTIAL_RELATION_LENGTH = get_max_partial_relation_length<Relations>(); | ||
|
||
// MAX_RANDOM_RELATION_LENGTH = algebraic degree of sumcheck relation *after* multiplying by the `pow_zeta` random | ||
// polynomial e.g. For \sum(x) [A(x) * B(x) + C(x)] * PowZeta(X), relation length = 2 and random relation length = 3 | ||
static constexpr size_t MAX_RANDOM_RELATION_LENGTH = MAX_RELATION_LENGTH + 1; | ||
// BATCHED_RELATION_PARTIAL_LENGTH = algebraic degree of sumcheck relation *after* multiplying by the `pow_zeta` | ||
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. please explain what partial means here 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. also, to be matching with the line above i'd use 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. But it's the "partial length of the batched relation" we're computing here, not the "batching of the partial relation length", so I think what I wrote is more meaningful. 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. To avoid duplicating the same comments in every flavor file, I'll write descriptions of "total" versus "partial" in the comments that define the functions. That should be good enough, right? I think anybody who's curious would look there. 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. yes, that makes sense, also did not realise the nuance until now, self note to take ordering into consideration in a name |
||
// random polynomial e.g. For \sum(x) [A(x) * B(x) + C(x)] * PowZeta(X), relation length = 2 and random relation | ||
// length = 3 | ||
static constexpr size_t BATCHED_RELATION_PARTIAL_LENGTH = MAX_PARTIAL_RELATION_LENGTH + 1; | ||
static constexpr size_t NUM_RELATIONS = std::tuple_size<Relations>::value; | ||
|
||
// Instantiate the BarycentricData needed to extend each Relation Univariate | ||
// static_assert(instantiate_barycentric_utils<FF, MAX_RANDOM_RELATION_LENGTH>()); | ||
|
||
// define the containers for storing the contributions from each relation in Sumcheck | ||
using SumcheckTupleOfTuplesOfUnivariates = decltype(create_sumcheck_tuple_of_tuples_of_univariates<Relations>()); | ||
|
@@ -721,7 +722,7 @@ template <typename CycleGroup_T, typename Curve_T, typename PCS_T> class ECCVMBa | |
/** | ||
* @brief A container for univariates produced during the hot loop in sumcheck. | ||
*/ | ||
using ExtendedEdges = ProverUnivariates<MAX_RELATION_LENGTH>; | ||
using ExtendedEdges = ProverUnivariates<MAX_PARTIAL_RELATION_LENGTH>; | ||
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 find the usage of partial in the context of sumcheck a bit strange because in that case the partial degree is actually the total degree :-? |
||
|
||
/** | ||
* @brief A container for the prover polynomials handles; only stores spans. | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,14 +4,20 @@ | |||||
namespace proof_system::honk { | ||||||
|
||||||
template <typename Flavor_, size_t NUM_> struct ProverInstances_ { | ||||||
public: | ||||||
using Flavor = Flavor_; | ||||||
using FF = typename Flavor::FF; | ||||||
static constexpr size_t NUM = NUM_; | ||||||
using Instance = ProverInstance_<Flavor>; | ||||||
|
||||||
using ArrayType = std::array<std::shared_ptr<Instance>, NUM_>; | ||||||
// The extended length here is the length of a composition of polynomials. | ||||||
static constexpr size_t EXTENDED_LENGTH = (Flavor::MAX_TOTAL_RELATION_LENGTH - 1) * (NUM - 1) + 1; | ||||||
using RelationParameters = proof_system::RelationParameters<Univariate<FF, EXTENDED_LENGTH>>; | ||||||
|
||||||
public: | ||||||
static constexpr size_t NUM = NUM_; | ||||||
ArrayType _data; | ||||||
RelationParameters relation_parameters; | ||||||
|
||||||
std::shared_ptr<Instance> const& operator[](size_t idx) const { return _data[idx]; } | ||||||
typename ArrayType::iterator begin() { return _data.begin(); }; | ||||||
typename ArrayType::iterator end() { return _data.end(); }; | ||||||
|
@@ -24,6 +30,25 @@ template <typename Flavor_, size_t NUM_> struct ProverInstances_ { | |||||
} | ||||||
}; | ||||||
|
||||||
/** | ||||||
* @brief Create folded (univariate) relation parameters. | ||||||
* @details For a given relation parameter type, extract that parameter from each instance, place the values in a | ||||||
* univariate (i.e., sum them against an appropriate Lagrange basis) and then extended as needed during the | ||||||
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.
Suggested change
|
||||||
* constuction of the combiner. | ||||||
*/ | ||||||
void parameters_to_univariates() | ||||||
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 think I would have had this in the protogalaxy prover, called fold_relation_parameters 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. That's fair, I can move it. |
||||||
{ | ||||||
auto params_to_fold = relation_parameters.to_fold; | ||||||
for (size_t param_idx = 0; param_idx < params_to_fold.size(); param_idx++) { | ||||||
auto& univariate_param = *params_to_fold[param_idx]; | ||||||
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. folded_param maybe as this is the final result? 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 think folding is kind of a stupid name tbh and univariate is more descriptive here. 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. hm yeah, just suggested it to be in consistency with everything else that we fold |
||||||
Univariate<FF, NUM> tmp(0); | ||||||
for (size_t instance_idx = 0; instance_idx < NUM; instance_idx++) { | ||||||
tmp.value_at(instance_idx) = *((*_data[instance_idx]).relation_parameters.to_fold[param_idx]); | ||||||
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. how should I feel about double dereferencing?😅 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. Ah, thanks for raising this yes what I did here was no good and I thought I'd left a TODO note to help me catch it. Will rework. |
||||||
} | ||||||
univariate_param = tmp.template extend_to<EXTENDED_LENGTH>(); | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* @brief For a prover polynomial label and a fixed row index, construct a uninvariate from the corresponding value | ||||||
* from each instance. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
|
||
#include "instances.hpp" | ||
#include "barretenberg/ecc/curves/bn254/bn254.hpp" | ||
#include "barretenberg/honk/proof_system/grand_product_library.hpp" | ||
#include "barretenberg/polynomials/polynomial.hpp" | ||
#include "barretenberg/srs/factories/file_crs_factory.hpp" | ||
#include <gtest/gtest.h> | ||
|
||
namespace proof_system::honk::instance_tests { | ||
|
||
template <class Flavor> class InstancesTests : public testing::Test { | ||
using FF = typename Flavor::FF; | ||
using Builder = typename Flavor::CircuitBuilder; | ||
|
||
public: | ||
static void test_parameters_to_univariates() | ||
{ | ||
using Instances = ProverInstances_<Flavor, 2>; | ||
using Instance = typename Instances::Instance; | ||
|
||
Builder builder1; | ||
auto instance1 = std::make_shared<Instance>(builder1); | ||
instance1->relation_parameters.eta = 1; | ||
|
||
Builder builder2; | ||
builder2.add_variable(3); | ||
auto instance2 = std::make_shared<Instance>(builder2); | ||
instance2->relation_parameters.eta = 3; | ||
|
||
Instances instances{ { instance1, instance2 } }; | ||
instances.parameters_to_univariates(); | ||
|
||
Univariate<FF, 12> expected_eta{ { 1, 3, 5, 7, 9, 11, 13, 15, 17, 19, 21, 23 } }; | ||
EXPECT_EQ(instances.relation_parameters.eta, expected_eta); | ||
}; | ||
}; | ||
|
||
using FlavorTypes = testing::Types<flavor::Ultra>; | ||
TYPED_TEST_SUITE(InstancesTests, FlavorTypes); | ||
|
||
TYPED_TEST(InstancesTests, ParametersToUnivariates) | ||
{ | ||
TestFixture::test_parameters_to_univariates(); | ||
} | ||
|
||
} // namespace proof_system::honk::instance_tests |
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.
was this include necessary here, i see it's also in other bench files that actually use relation parameters
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.
This file does use them on Line 26.
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.
my bad