refactor: protogalaxy relations #1897

codygunton · 2023-08-30T16:09:35Z

Will resolve AztecProtocol/barretenberg#682 and AztecProtocol/barretenberg#685

Add optimization to extend functions in BarycentricData.
Move barycentric evaluation, univariate, and relations classes for organizations and more efficient iteration.
Simplify relations classes.
Simplify relation consistency tests.

Cf also my comments below.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
Every change is related to the PR description.
I have linked this pull request to relevant issues (if any exist).

…actor

codygunton · 2023-09-05T19:13:39Z

circuits/cpp/barretenberg/cpp/src/barretenberg/polynomials/pow.test.cpp

    std::array<FF, d> variables{};
    for (auto& u_i : variables) {
        u_i = FF::random_element();
        pow_univariate.partially_evaluate(u_i);
    }
-
-    FF expected_eval = proof_system::honk::power_polynomial::evaluate<FF>(zeta, variables);
+


Legible, by-hand test is better AND it decouples this test from a very large module.

codygunton · 2023-09-05T19:20:27Z

circuits/cpp/barretenberg/cpp/src/barretenberg/proof_system/relations/arithmetic_relation.hpp

@@ -0,0 +1,57 @@
+#pragma once


The other git mvs display as such, but this one doesn't (?).

codygunton · 2023-09-05T19:33:15Z

circuits/cpp/barretenberg/cpp/src/barretenberg/proof_system/relations/auxiliary_relation.hpp

  public:
+    using FF = FF_;


Exposing this alias clarifies what's happening in relation_types.hpp

codygunton · 2023-09-05T19:34:18Z

circuits/cpp/barretenberg/cpp/src/barretenberg/proof_system/relations/auxiliary_relation.hpp

@@ -20,8 +18,8 @@ template <typename FF> class AuxiliaryRelationBase {
    static constexpr size_t LEN_4 = 6; // RAM consistency sub-relation 1


I'd rather expose an array but there's no easy way to unpack an array to a parameter pack.

codygunton · 2023-09-05T19:34:59Z

circuits/cpp/barretenberg/cpp/src/barretenberg/proof_system/relations/auxiliary_relation.hpp

-                                                  const auto& extended_edges,
-                                                  const RelationParameters<FF>& relation_parameters,
-                                                  const FF& scaling_factor)
+    inline static void accumulate(typename AccumulatorTypes::Accumulators& accumulators,


The name didn't make sense anymore once the function was reused for more than just univariates.

codygunton · 2023-09-05T19:36:31Z

circuits/cpp/barretenberg/cpp/src/barretenberg/proof_system/relations/relation_types.hpp

@@ -0,0 +1,118 @@
+#pragma once


I have mostly tried to make this more legible by giving things more accurate names. I think a deeper rework would be nice but there's not time now.

codygunton · 2023-09-05T19:39:14Z

...retenberg/cpp/src/barretenberg/proof_system/relations/standard_relation_consistency.test.cpp

+ * For this purpose, we simply feed (the same) random inputs into each of the two implementations and confirm that
+ * the outputs match. This does not confirm the correctness of the identity arithmetic (the identities will not be
+ * satisfied in general by random inputs) only that the two implementations are equivalent.
+ *


I've significantly reworked these relation consistency tests to simplify them, as they now don't need to test separate code paths (each relation implementation now contains a single function called accumulate).

maramihali

looks good to me, as discussed it would be nice to add an issue about the remaining refactorings that need to be done

maramihali · 2023-09-06T09:59:22Z

circuits/cpp/barretenberg/cpp/src/barretenberg/benchmark/relations_bench/barycentric.bench.cpp

+    BarycentricData<FF, 2, 6> barycentric_2_to_6;
+    for (auto _ : state)
+    {
+        DoNotOptimize(barycentric_2_to_6.extend(univariate));


what's the purpose of this DoNotOptimize here?

From https://github.com/google/benchmark/blob/main/docs/user_guide.md#preventing-optimization

DoNotOptimize() forces the result of to be stored in either memory or a register.

Since the result of extend is not used, the compiler would most likely optimize away the function call, meaning at the benchmark would measure the cost of doing nothing.

maramihali · 2023-09-06T14:02:22Z

circuits/cpp/barretenberg/cpp/src/barretenberg/polynomials/univariate.hpp


 template <class Fr, size_t view_length> class UnivariateView;

+// IMPROVEMENT(Cody) this is not used anywhere? Move to memeber function of U/snivariate?


Suggested change

// IMPROVEMENT(Cody) this is not used anywhere? Move to memeber function of U/snivariate?

// IMPROVEMENT(Cody) this is not used anywhere? Move to memeber function of Univariate?

also I think this is a stale comment from get_random which has been moved from the test file

Thanks, fixed.

maramihali · 2023-09-06T14:08:23Z

circuits/cpp/barretenberg/cpp/src/barretenberg/honk/sumcheck/sumcheck_round.hpp

@@ -91,13 +85,13 @@ template <typename Flavor> class SumcheckProverRound {
     *
     * @tparam T : In practice, this is a Univariate<FF, MAX_NUM_RELATIONS>.
     */
-    Univariate<FF, MAX_RANDOM_RELATION_LENGTH> batch_over_relations(FF challenge,
-                                                                    const PowUnivariate<FF>& pow_univariate)
+    barretenberg::Univariate<FF, MAX_RANDOM_RELATION_LENGTH> batch_over_relations(


Not sure what the best practices are here, but isn't it a bit too verbose to have barretenberg::* everywhere?

I noticed that in the test file you included the various data structures with using declarations at the beginning of the file and maybe it's worth doing here as well.

Indeed, but this is in a header so we shouldn't use using outside of a function or class scope :/

maramihali · 2023-09-06T14:35:25Z

circuits/cpp/barretenberg/cpp/src/barretenberg/proof_system/relations/arithmetic_relation.hpp

+     * @param accumulator the term being calculated by a sequence of calls to this function
+     * @param new_term the term to be accumulated
+     * @param parameters inputs not varying between successive executions of this function
+     * @param scaling_factor term to scale the new accumulator contribution by before incorporating it
+     */


Suggested change

* @param accumulator the term being calculated by a sequence of calls to this function

* @param new_term the term to be accumulated

* @param parameters inputs not varying between successive executions of this function

* @param scaling_factor term to scale the new accumulator contribution by before incorporating it

*/

* @param accumulator the term being calculated by a sequence of calls to this function

* @param new_term the term added to the accumulator in this iteration of the function

* @param parameters inputs not varying between successive executions of this function

* @param scaling_factor scales the new_term before incorporating it into the accumulator

*/

maramihali · 2023-09-06T14:53:19Z

circuits/cpp/barretenberg/cpp/src/barretenberg/polynomials/univariate.hpp

 #include <span>

-namespace proof_system::honk::sumcheck {
+namespace barretenberg {


I think there should be a comment somewhere that explains the difference between Univariate and UnivariateView

Added comments

maramihali · 2023-09-07T09:55:03Z

circuits/cpp/barretenberg/cpp/src/barretenberg/proof_system/relations/relation_types.hpp

+    };
+
+  public:
+    using UnivariateAccumulatorsAndViews = typename RelationImpl::template GetAccumulatorTypes<UnivariateAccumulatorsAndViewsTemplate>;


I think it would be good to have some comments in places like line 68-69 which take a while to understand unless very experienced with templates. Would be useful for future refactoring as well

Made an issue AztecProtocol/barretenberg#720 and referenced it in the head block comment, added a comment above Line 68.

maramihali · 2023-09-07T10:04:41Z

...barretenberg/cpp/src/barretenberg/proof_system/relations/ultra_relation_consistency.test.cpp

+        return result;
+    }
+
+    static InputElements get_special()


can you add a comment explaining why this is special?

maramihali · 2023-09-07T10:08:08Z

...retenberg/cpp/src/barretenberg/proof_system/relations/standard_relation_consistency.test.cpp

+        return result;
+    }
+
+    static InputElements get_special()


a comment here as well would be useful

maramihali

looks good to me, as discussed it would be nice to add an issue about the remaining refactorings that need to be done

codygunton · 2023-09-07T14:47:44Z

looks good to me, as discussed it would be nice to add an issue about the remaining refactorings that need to be done

Good idea, here it is: AztecProtocol/barretenberg#720

🤖 I have created a new Aztec Packages release --- ## [0.1.0-alpha63](v0.1.0-alpha62...v0.1.0-alpha63) (2023-09-08) ### Features * `GrumpkinScalar` type ([#1919](#1919)) ([3a9238a](3a9238a)) ### Bug Fixes * add retry to tag and docker actions ([#2099](#2099)) ([9f741f4](9f741f4)) * **breaking change:** change embedded curve scalar mul to use two limbs to properly encode the scalar field ([#2105](#2105)) ([070cc4c](070cc4c)) * broken bootstrap.sh after renaming `aztec-cli` dir as `cli` ([#2097](#2097)) ([2386781](2386781)) * browser test in canary flow ([#2102](#2102)) ([d52af6c](d52af6c)), closes [#2086](#2086) * check a note is read before nullifying it. ([#2076](#2076)) ([aabfb13](aabfb13)), closes [#1899](#1899) * circuits issues when building with gcc ([#2107](#2107)) ([4f5c4fe](4f5c4fe)) * COMMIT_TAG arg value in canary Dockerfile ([#2118](#2118)) ([a3d6459](a3d6459)) * dont assume safety of nvm ([#2079](#2079)) ([a4167e7](a4167e7)) * end-to-end aztec cli dependency issue ([#2092](#2092)) ([16ee3e5](16ee3e5)) * minor annoyances ([#2115](#2115)) ([a147582](a147582)) * padded printing for e2e-cli ([#2106](#2106)) ([5988014](5988014)) * remaining refs to clang15 ([#2077](#2077)) ([2c16547](2c16547)) * run e2e tests without spot ([#2081](#2081)) ([f0aa3ca](f0aa3ca)) * updated CLI readme ([#2098](#2098)) ([2226091](2226091)), closes [#1784](#1784) ### Miscellaneous * **circuits:** - remove dead code from cbind of private kernel circuit ([#2088](#2088)) ([43dc9d7](43dc9d7)) * **circuits:** remove dead code in cbind.cpp for public kernel ([#2094](#2094)) ([861f960](861f960)) * Conservatively raise the minimum supported clang version in CMakeList ([#2023](#2023)) ([f49c416](f49c416)) * **constants:** bump number of private reads and writes ([#2062](#2062)) ([ab6c6b1](ab6c6b1)) * **contracts:** Use autogenerated Noir interfaces where possible ([#2073](#2073)) ([bd6368b](bd6368b)), closes [#1604](#1604) * merge bb release-please ([#2080](#2080)) ([e89b043](e89b043)) * move storage into main.nr. ([#2068](#2068)) ([2c2d72b](2c2d72b)) * protogalaxy relations ([#1897](#1897)) ([35407e2](35407e2)) ### Documentation * **limitations:** limitations on ordering and logs of chopped notes ([#2085](#2085)) ([315ad3d](315ad3d)), closes [#1652](#1652) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Pw/dbp to testnet

codygunton added 4 commits August 24, 2023 01:19

Initial work Lagrange poly def.

021adfc

Refactor to all compile time.

868227b

Refactor to subclass.

06a796b

Light renaming and add comments.

9e44ce5

codygunton self-assigned this Aug 30, 2023

codygunton added 25 commits August 30, 2023 16:16

Cleanup

e027cb4

Efficient 2_to_N with benchmarks.

cfad8ad

consts, std, etc.

f707120

Add relaiton benchmarks.

2f2def3

Clean up includes.

0ee5f81

Remove more includes

e28045c

Rename test suite.

6fc8b26

Rename tests--even better.

6688556

Move multivariates.test.cpp

0b545df

Clean up includes.

ce06bee

...clean up includes.

b148b49

Move up: univariate, barycentric, pow.

9daf7d7

Move univariate, pow, barycentric to bb namespace.

8ef4655

Link only polynomaials.

4e6be56

Merge remote-tracking branch 'origin/master' into cg/pg-relations-ref…

8e9118f

…actor

Prune includes

25c4440

Refactor to be indep of flavor, hence Honk.

3a9307f

Rename file

72cad4c

Update comment

defab37

Refactor ultra relation consistency tests.

7a803aa

Change relations namespace.

69ab878

Move relations to proof_system.

b4d11c2

Fix test.

9929cbc

Resolve some WORKTODO.

77bf2ad

Formatting

784e2c5

Prune includes.

a37913b

codygunton commented Sep 5, 2023

View reviewed changes

codygunton added 4 commits September 5, 2023 19:15

Remove WORKTODOs

ce3a6c3

Remove worktodo (explained above).

45f726b

dirty move.

733ae6a

clean move

3d73f64

codygunton commented Sep 5, 2023

View reviewed changes

Bring improved extend to RunTime class

cc33504

codygunton marked this pull request as ready for review September 5, 2023 19:53

Merge branch 'master' into cg/pg-relations-refactor

70dec35

codygunton requested a review from maramihali September 5, 2023 20:22

iAmMichaelConnor added the crypto cryptography label Sep 6, 2023

maramihali reviewed Sep 7, 2023

View reviewed changes

maramihali approved these changes Sep 7, 2023

View reviewed changes

codygunton added 2 commits September 7, 2023 15:57

Respond to review.

ec2a4c8

Merge branch 'master' into cg/pg-relations-refactor

7ddecb5

codygunton merged commit 35407e2 into master Sep 7, 2023

codygunton deleted the cg/pg-relations-refactor branch September 7, 2023 18:41

github-actions bot mentioned this pull request Sep 7, 2023

chore(master): release 0.1.0-alpha63 #2078

Merged

This was referenced Sep 7, 2023

Test out new uses of Barycentric classes AztecProtocol/barretenberg#682

Closed

Refactor relations for use in computation of the perturbation and combiner polynomials AztecProtocol/barretenberg#685

Closed

github-actions bot mentioned this pull request Sep 8, 2023

chore(master): Release 0.5.2 #2141

Merged

codygunton pushed a commit that referenced this pull request Jan 23, 2024

Merge pull request #1897 from AztecProtocol/pw/dbp-to-testnet

6f6376e

Pw/dbp to testnet

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: protogalaxy relations #1897

refactor: protogalaxy relations #1897

codygunton commented Aug 30, 2023 •

edited

Loading

codygunton Sep 5, 2023

codygunton Sep 5, 2023

codygunton Sep 5, 2023

codygunton Sep 5, 2023

codygunton Sep 5, 2023

codygunton Sep 5, 2023

codygunton Sep 5, 2023

maramihali left a comment

maramihali Sep 6, 2023

codygunton Sep 7, 2023

maramihali Sep 6, 2023

maramihali Sep 6, 2023

codygunton Sep 7, 2023

maramihali Sep 6, 2023

codygunton Sep 7, 2023

maramihali Sep 6, 2023

maramihali Sep 6, 2023

codygunton Sep 7, 2023

maramihali Sep 7, 2023

codygunton Sep 7, 2023 •

edited

Loading

maramihali Sep 7, 2023

codygunton Sep 7, 2023

maramihali Sep 7, 2023

codygunton Sep 7, 2023

maramihali left a comment

codygunton commented Sep 7, 2023

		@@ -20,8 +18,8 @@ template <typename FF> class AuxiliaryRelationBase {
		static constexpr size_t LEN_4 = 6; // RAM consistency sub-relation 1


		template <class Fr, size_t view_length> class UnivariateView;

		// IMPROVEMENT(Cody) this is not used anywhere? Move to memeber function of U/snivariate?

refactor: protogalaxy relations #1897

refactor: protogalaxy relations #1897

Conversation

codygunton commented Aug 30, 2023 • edited Loading

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maramihali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codygunton Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maramihali left a comment

Choose a reason for hiding this comment

codygunton commented Sep 7, 2023

codygunton commented Aug 30, 2023 •

edited

Loading

codygunton Sep 7, 2023 •

edited

Loading