fix: Patches to cycle_group and cycle_group fuzzer by Sarkoxed · Pull Request #12385 · AztecProtocol/aztec-packages

Sarkoxed · 2025-02-28T20:10:52Z

This pr adds some features and fixes some bugs

Cycle Group

features:

new class member bool _is_standard that detects whether the point is in it's standard form. i.e.
- when it's not infinity it is (x, y)
- when it's infinity it should be (0, 0)
  this constant tracks the of the state of the point. So we won't create unnecessary gates repeatedly.

The reason we need it to be (0, 0) is because the ordinary representation in AffineElement is not compatible with our arithmetic design choices.

added the necessary _is_standard throughout the whole file
redesigned the get_standard_form method accordingly
added the method standardize
added some comments to the use cases of unconditional operations
cycle_group::conditional_assign now keeps track of the resulting standard'ness
redesigned the cycle_group::set_point_at_infinity to match the standard form. Also setting the resulting value to be witness in some cases.

Bugs:

cycle_group::from_witness now doesn't throw an error during the point at infinity initialization
cycle_group::from_constant_witness now doesn't throw an error during the point at infinity initialization
cycle_group::dbl now doesn't throw an error when we pass a point with _is_infinity being const and true
cycle_group::checked_unconditional_add/subtract now doesn't crash on circuit_builder level when we pass two constant points with colliding x coordinates
cycle_group::operator+\- now handles the cases when we pass allegedly witness point but with constant _is_infinity part
cycle_group::operator+\-. Previously when we added any two points the result point was marked as constant. Not the case anymore.
cycle_group::operator-(). Previously the result value was not normalized. Which resulted in wrong witness.
cycle_group::operator==. Now both values should be standardized first.
cycle_group::assert_equal same as ==
cycle_group::conditional_assign now results into both coordinates being witness or constant at the same time.

Also added the regression test on each bug that I've fixed

bool_t

made conditional_assign behave like the field_t::conditoinal_assign. Which doesn't create a gate when we choose between two equal witnesses

UltraCircuitBuilder

added a assert_valid_variables inside the create_ecc_dbl_gate

CycleGroupFuzzer

`fuzzer.hpp`

added the BATCH_MUL and COND_ASSIGN opcodes

cycle_group.fuzzer.hpp

Added SHOW_PRETTY_INFORMATION define. It just spits out a code that I can then debug separately. Really helped to shorten the amount of time spent per crash
Added DISABLE_MULTIPLICATION define. For now there're still too much bugs related to the ordinary logic.
Added witness predicate possibility in construct_predicate.
Renamed ExecutionHandler::operator+- to operator_add and operator_sub respectively. Needed to pass builder as one of the variables due to point_at_infinity possibility
Added smth_inf flag during addition/subtraction, because unconditional operations can't work with points at infinity
Added standardized check during postProcess to check whether I handled all the cases of initialization correctly

Rumata888

Please look at the comments

Rumata888 · 2025-03-13T13:04:49Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.fuzzer.hpp

@@ -6,13 +6,17 @@
 #pragma clang diagnostic push
 // TODO(luke/kesha): Add a comment explaining why we need this ignore and what the solution is.
 // TODO(alex): resolve this todo in current pr


Maybe it's time?

Rumata888 · 2025-03-13T13:05:07Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.fuzzer.hpp

 #define HAVOC_TESTING

 #include "barretenberg/common/fuzzer.hpp"
+// TODO: figure out how to detect w + w = c


Could you elaborate so someone else might understand?

I think of just deleting this comment for now, since I'll add this check in the following pr

Rumata888 · 2025-03-13T13:11:45Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.hpp

+    // Most of the time it is true, so we won't need to do extra conditional_assign
+    // during `get_standard_form` or `assert_equal` call
+    // However sometimes it won't be the case, so we can handle these cases using this flag
+    bool _is_standard;


Could you write what "standard" means?

Rumata888 · 2025-03-13T13:15:40Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp

+    }
+    this->_is_standard = true;
+
+    // TODO(alex): there can be a possible bug. Again due to montgomery arithmetic.


If you're talking about the infinity representation in native affine elements, I don't think it should affect anything here

Rumata888 · 2025-03-13T13:24:19Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp

+    // This ensures that at least one of the switches was performed
+    this->_is_constant = this->x.is_constant() && this->y.is_constant();
+    if (this->_is_constant) {
+        this->_is_infinity = this->_is_infinity.get_value();


Could you explain to me what this does?

After describing this, I realized that it was not necessary, after all the stuff that set_point_at_infinity does.

I can bet that I saw a case when we got a non-standard point after some operations. But it seems like not the case anymore. I will delete most of this method for now.

Rumata888 · 2025-03-13T13:33:36Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp

-    return equal_and_not_infinity || both_infinity;
+    this->standardize();
+    other.standardize();
+    const auto equal = (x == other.x) && (y == other.y);


I'm not sure if we should add the Point at infinity check, because right now if you compare a broken 0,0 point that is not set as point at infinity to a point at infinity, you'll get true

I guess we can throw extra (lhs._is_infinity == rhs._is_infinity) into this bool. Same goes to assert_equal

Rumata888 · 2025-03-13T13:39:07Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/bool/bool.cpp

    if (is_constant() || !witness_inverted) {
        return *this;
    }
+    // TODO(alex): shouldn't normalize return this->witness_value^witness_inverted in const case?


You should probably assert that it is not inverted (since what would be the point of inverting it separately, when you can just change the value)

Rumata888 · 2025-03-13T13:39:29Z

barretenberg/cpp/src/barretenberg/stdlib/primitives/bool/bool.cpp

+    bool same = (lhs.witness_index == rhs.witness_index) && (lhs.witness_inverted == rhs.witness_inverted);
+    bool witness_same = same && lhs.witness_index != IS_CONSTANT;
+    bool const_same = same && (lhs.witness_index == IS_CONSTANT) && (lhs.witness_bool == rhs.witness_bool);
+    // TODO(alex): reference to the above todo just to not forget to change the lhs.witness_inverted ==


The previous comment about constants possibly having witness_inverted true. Fixed that

Rumata888 · 2025-03-13T14:00:27Z

barretenberg/cpp/src/barretenberg/examples/join_split/join_split_circuit.cpp

-        allow_chain_2.must_imply(output_note_2.owner == self, "inter-user chaining disallowed");
+
+        group_ct output_note_1_owner = output_note_1.owner;
+        group_ct output_note_2_owner = output_note_2.owner;


standardize method changes the class state, so we can't mark assert_equal and == to const anymore. Hence we can't call them on consts either

barretenberg/cpp/src/barretenberg/commitment_schemes/ipa/ipa.hpp

Rumata888

Because after that, the coordinates are not normalized anywhere. Hence after calling create_ecc_dbl_gate, create_ecc_add_gate with this point as an argument, the witness will be wrong

I think you should add that comment then, otherwise someone reading this might remove it

Rumata888

PLease add the comment explaining why you're calling normalize after negation and then merge

* master: fix: filter for empty attestations when pulling from block (#12740) feat: one-way noir sync (#12592) feat(avm): Address derivation gadget (#12721) chore(ci): add workflow dispatch to masternet (#12739) feat: add default accounts (#12734) feat: gas reports and snapshots (#12724) fix(avm): fix vm1 fake proof size (#12733) feat(bb): extend_edges optimization for zero values past end_index (#12703) fix: remove hard coding of constructor for account manager (#12678) git subrepo push --branch=master noir-projects/aztec-nr git_subrepo.sh: Fix parent in .gitrepo file. [skip ci] chore: replace relative paths to noir-protocol-circuits git subrepo push --branch=master barretenberg fix: verify_honk_proof inputs generation in bootstrap (#12457) fix: Patches to cycle_group and cycle_group fuzzer (#12385)

Sarkoxed added 2 commits February 28, 2025 20:09

Several patches to cycle_group and cycle_group fuzzer

9f962a8

standard

d5a6c55

Sarkoxed added the T-bug Type: Bug. Something is broken. label Mar 3, 2025

Sarkoxed added 20 commits March 6, 2025 14:55

few updates

01e2367

not a bug

e52d99d

Standard form related fixes

a1b8d75

Fixed subtraction + new assert_equal

552214f

negative operator fix

03479d4

Non constant predicates

9112e0f

regression

81e911a

Handling const set inf + ==

1aa2081

new ==

96c0656

New features

4ed0510

Better handling of const case

b50c80e

Mixup bug

6816467

Added pretty prints and defines to disable mul

9e3c662

Added keywords for BATCH_MUL and COND_ASSIGN

f260f60

new handlers for bool

1022444

should be here I guess

c0f16c6

Added few features

3284a8f

is_standard()

afbcab1

Two more tests and one still fails

af74a20

fuzzer update

0a3e45d

Sarkoxed marked this pull request as ready for review March 12, 2025 00:50

Sarkoxed self-assigned this Mar 12, 2025

Sarkoxed added 5 commits March 12, 2025 13:34

less gates on average

47053a0

Coverage track

55bad89

newline

ea4d9ea

Merge branch 'master' into as/cycle-group-patches

c8c8be2

refactoring due to methods growth

7653b8d

Sarkoxed added 6 commits March 12, 2025 15:36

Leave it like that for now

380ec4e

Resolve the constantness issues

3348ddb

clang-format again

2c11ad0

fixes for ci 1

bed01b2

fixes for ci 2:

ccb3f79

Remember origin tag

4e0ce11

Sarkoxed requested a review from Rumata888 March 12, 2025 19:51

Rumata888 reviewed Mar 13, 2025

View reviewed changes

Sarkoxed added 3 commits March 13, 2025 15:44

Addressing bool_t comments

90bf51d

Addressing CycleGroup comments

507e0ab

Addressing fuzzer comments

10790af

Rumata888 reviewed Mar 13, 2025

View reviewed changes

Rumata888 approved these changes Mar 13, 2025

View reviewed changes

Sarkoxed added 2 commits March 13, 2025 21:40

Comment on normalizations

815ddbd

Merge branch 'master' into as/cycle-group-patches

9450eb5

Sarkoxed merged commit de9f0d8 into master Mar 13, 2025
6 checks passed

Sarkoxed deleted the as/cycle-group-patches branch March 13, 2025 22:27

AztecBot mentioned this pull request Mar 13, 2025

chore(master): release 0.81.0 #12689

Merged

Conversation

Sarkoxed commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cycle Group

bool_t

UltraCircuitBuilder

CycleGroupFuzzer

fuzzer.hpp

cycle_group.fuzzer.hpp

Uh oh!

Rumata888 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rumata888 left a comment

Choose a reason for hiding this comment

Uh oh!

Rumata888 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sarkoxed commented Feb 28, 2025 •

edited

Loading

`fuzzer.hpp`