feat(avm): Port field gt to vm2 by sirasistant · Pull Request #12883 · AztecProtocol/aztec-packages

sirasistant · 2025-03-19T16:58:32Z

Ports field greater than to vm2, removing non-ff and eq functionality, which could be trivally inlined in other gadgets.

sirasistant · 2025-03-19T17:36:03Z

barretenberg/cpp/pil/vm2/ff_gt.pil

+    sel * (128 - constant_128) = 0;
+
+    pol POW_128 = 2 ** 128;
+    pol P_LO = 53438638232309528389504892708671455233; // Lower 128 bits of p


This was p-1 in vm1, which was very counter intuitive

sirasistant · 2025-03-19T17:36:28Z

barretenberg/cpp/pil/vm2/ff_gt.pil

+    pol B_SUB_A_HI = b_hi - a_hi - borrow;
+
+    pol IS_GT = sel_gt * result;
+    // When IS_GT = 1, we enforce the condition that a > b and thus a - b - 1 does not underflow.


Explainer comment taken from vm1

IlyasRidhuan · 2025-03-20T08:58:44Z

barretenberg/cpp/pil/vm2/ff_gt.pil

+        a_lo, constant_128
+    } in range_check.sel {
+        range_check.value, range_check.rng_chk_bits
+    };


nit: formatting should probably match what we typically do

IlyasRidhuan · 2025-03-20T09:12:26Z

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/field_gt.test.cpp

+    // GT
+    TestParams{ 27, 0, true },
+    TestParams{ TWO_POW_128, 0, true },
+    TestParams{ FF::neg_one(), FF::zero(), true },


FYI FF::zero() is being deprecated, #1270. We should probably get into the habit of not using them

fcarreiro

Very sad about simulation :(. Will think a bit but IDK if there's a better way

fcarreiro · 2025-03-20T20:26:48Z

barretenberg/cpp/src/barretenberg/vm2/simulation/lib/field_comparison.hpp

+    bool operator==(const FFDecomposition& other) const = default;
+};
+
+struct LimbsComparisonWitness {


Simulation should not have a concept of witness. I'd say find a proper rephrasing (also for the function) or we can discuss it.

It's a witness in the general sense, it's a value used to prove a limb comparison. I know it's weird for simulation to have it, but right now simulation needs it to range check it ):

fcarreiro · 2025-03-20T20:27:17Z

barretenberg/cpp/src/barretenberg/vm2/simulation/lib/field_comparison.hpp

This whole file feels very strange under simulation.

Do you expect the functions to be used somewhere else or can they be put in field_gt?

I think we can leave them under field gt yep. Do you think they should be simulator class static members? they are used in tests

fcarreiro · 2025-03-20T20:29:56Z

barretenberg/cpp/src/barretenberg/vm2/simulation/field_gt.cpp

+    uint256_t a_integer = static_cast<uint256_t>(a);
+    uint256_t b_integer = static_cast<uint256_t>(b);


Do you need the cast? If uint256_t has a constructor for FF it should be ok.

ah let me see

fcarreiro · 2025-03-20T20:30:45Z

barretenberg/cpp/src/barretenberg/vm2/simulation/field_gt.cpp

@@ -0,0 +1,49 @@
+#include "barretenberg/vm2/simulation/field_gt.hpp"


"Include what you use"
in particular uint256_t, and the lib/field_comparison file.

fcarreiro · 2025-03-20T20:32:27Z

barretenberg/cpp/src/barretenberg/vm2/simulation/field_gt.cpp

+
+namespace bb::avm2::simulation {
+
+bool FieldGreaterThan::ff_gt(const FF& a, const FF& b)


This whole method feels so so bad from a simulation PoV... but since we have to call the range check gadget I'm not sure there's a far better way...

fcarreiro · 2025-03-20T20:35:06Z

barretenberg/cpp/src/barretenberg/vm2/tracegen/field_gt_trace.cpp

+        auto a_limbs = event.a_limbs;
+        auto p_sub_a_witness = event.p_sub_a_witness;
+        auto b_limbs = event.b_limbs;
+        auto p_sub_b_witness = event.p_sub_b_witness;
+        auto res_witness = event.res_witness;


just fyi these are all making copies most likely. use auto& or better cost auto& if you want to avoid it.

Also in general this use of auto is not very kosher. The rule of thumb is: "if the type can be known from context, you can use auto" but if it's not, ideally not, unless you do it for very specific reasons. E.g., you reaaaaaly don't need it, you want to avoid an include, type is too verbose, you need to destructure, etc.

I do want the copy, because I'm going to mutate them to rotate them around (and the event comes as const&)
Regarding auto, will fix

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/field_gt.test.cpp

barretenberg/cpp/pil/vm2/ff_gt.pil

IlyasRidhuan · 2025-03-21T04:32:23Z

Very sad about simulation :(. Will think a bit but IDK if there's a better way

This seems to be a common issue we keep running into when it comes to inter-trace communication (we just accept it for "higher level" stuff like merkle -> poseidon2 or poseidon2 -> perm). This feels more egregious because it would otherwise be a 1-liner. Thinking out loud with some crazy ideas:

We internalise the range check, this sucks because it will be ~16/17 extra columns to this trace. Although this trace should be pretty sparse and (hopefully) long term goblin work will make this not so bad.
Allow some event generation within the various process functions in tracegen. So AvmTraceGenHelper would need to carry an emitter/s. The downside of this is that we lose some parallelisation - we essentially need to process the trace in the implicit hierarchy we have.

I dont mind (2) for range checks just because we will never directly call the range check - so we know that any range checks events is a result of another event.

jeanmon · 2025-03-21T07:55:22Z

Allow some event generation within the various process functions in tracegen. So AvmTraceGenHelper would need to carry an emitter/s. The downside of this is that we lose some parallelisation - we essentially need to process the trace in the implicit hierarchy we have.

@IlyasRidhuan @fcarreiro I was thinking the same the other day when calling a range check from bytecode_manager.cpp felt artificial. I think that what we cannot reconcile is directly emitting per each subtrace in simulation and not leaking circuit stuff into simulation. This will happen for each inter-circuit calls which is meant to be internal. We can still maintain parallelism in the tracegen if we can trigger these "internal inter-subtrace events" at event generation. For instance, through some "kind of event listeners" mechanism (e.g., rangecheck emitter listens to instr_fetching_event and emits whenever this happens). I did not study the code though to see how this could be done concretely (observer pattern?). @fcarreiro is the master on this.

IlyasRidhuan · 2025-03-21T08:43:14Z

hmm maybe, but i think that would just be moving the logic to the range check (thereby overloading it) when it should really just be in tracegen.

The problem is that some of the internal range check calls involve work that the simulation shouldnt care about. If you want to trigger them at event generation, you will still need to do the "internal computation" while we are simulating, which we want to avoid. E.g. in FF_GT we range check some value a_lo, IMO a_lo should only exist in tracegen time, it's not necessary for a pure simulator to know about a_lo. But even with the listener approach, we need to compute a_lo somewhere during simulation even though we might never use it.

jeanmon · 2025-03-21T09:04:10Z

@IlyasRidhuan Good point about values to be generated during simulation. We should be able to abstract the computation of such "circuit relevant values" based on the "parent event" and the "observer" could derive such values before emitting its own event. If you run the simulator with the "no-event" mode, then the work made by the observer is gone and you really simulate only what you need. What I am trying to do is move logic which needs to be done at simulation but for circuit only purposes into this "observer layer". This is done during simulation (in emit events mode) time but in a separate place.

fcarreiro · 2025-03-21T09:18:23Z

Thank you for the ideas guys. I have to think about them (maybe after hints?) since all of them have ramifications that cannot be taken lightly. I think for now I'm ok with polluting the simulator like this, and at least keeping the properties we already know about. If I come up with something else and a new set of properties we can probably refactor the (hopefully few) gadgets that do things like this.

sirasistant · 2025-03-21T09:58:05Z

It's funny because this took me longer since the first implementation I did it with a one liner in simulation. Then I went to constraining and i was like OH WAIT how will the range check gadget know about my range checks, and I had to go back and pollute simulation and the event with internal information.

fcarreiro

LRTM (looks reasonable to me) :)

Will let Ilyas/@jeanmon approve the PIL

fcarreiro · 2025-03-21T11:24:28Z

barretenberg/cpp/src/barretenberg/vm2/generated/relations/merkle_check.hpp

            std::get<6>(evals) += typename Accumulator::View(tmp);
        }
-        {
+        { // INITIALIZE_CURRENT_NODE


I thought these were already on master?

Maybe they were generated with an old bb-pilcom? Will regen mine with latest build just in case

fcarreiro · 2025-03-21T11:28:46Z

barretenberg/cpp/src/barretenberg/vm2/simulation/field_gt.test.cpp

+    uint128_t res_witness_hi = a_hi - b_hi - 0;
+
+    EXPECT_THAT(range_checks,
+                ElementsAreArray({ a_lo,


ElementsAreArray is useful when you already have an array variable. Otherwise ElementsAre(a,b,c,d...) should just work without needing to construct an array/vector.

Same in other places.

fcarreiro · 2025-03-21T11:30:01Z

barretenberg/cpp/src/barretenberg/vm2/simulation/field_gt.test.cpp

+    assert(field_gt.ff_gt(1, 0));
+    assert(field_gt.ff_gt(-1, 0));
+
+    assert(!field_gt.ff_gt(0, 0));
+    assert(!field_gt.ff_gt(-1, -1));
+    assert(!field_gt.ff_gt(0, 1));
+    assert(!field_gt.ff_gt(0, -1));


I think you might want these to be EXPECT_EQ()s?

yup it'd be a bit better

fcarreiro · 2025-03-21T11:32:27Z

barretenberg/cpp/src/barretenberg/vm2/simulation/lib/u256_decomposition.cpp

+
+namespace bb::avm2::simulation {
+
+const uint256_t TWO_POW_128 = uint256_t{ 1 } << 128;


nit: prefer uint256_t(1). Brace initialization is a can of worms. I like to follow this: https://abseil.io/tips/88

Can also be a constexpr most likely.

(also check if it's not already in the constants gen)

🤖 I have created a new Aztec Packages release --- ## [0.82.1](v0.82.0...v0.82.1) (2025-03-24) ### Features * **avm:** Port field gt to vm2 ([#12883](#12883)) ([0ae6891](0ae6891)) * use msgpack for ClientIvc::Proof in API ([#12911](#12911)) ([1a01602](1a01602)) ### Bug Fixes * disable proving on vite box ([#12971](#12971)) ([69a0fb6](69a0fb6)) * no hardcoded versions in bbup ([#12944](#12944)) ([397144f](397144f)) * pull CRS data ahead of time ([#12945](#12945)) ([43155d6](43155d6)) * Remove workaround ([#12952](#12952)) ([c3337af](c3337af)) * set the correct env var ([#12959](#12959)) ([bd0f4b2](bd0f4b2)) * yolo add bunch of test flakes ([13c19da](13c19da)) * yolo e2e_p2p tests now fully skipped due to huge speed regression ([9141410](9141410)) * yolo txe binds just to localhost by default. ([3933b35](3933b35)) ### Miscellaneous * Change `/bin/bash` shebang to be env based ([#12834](#12834)) ([7843a67](7843a67)) * clean up avm codeowners ([#12860](#12860)) ([35a8f46](35a8f46)) * deflake the kind smoke test ([#12955](#12955)) ([1a37d6d](1a37d6d)), closes [#11177](#11177) * fee cleanup ([#12941](#12941)) ([fdf1da4](fdf1da4)) * Increase bot count ([#12963](#12963)) ([16edd06](16edd06)) * L2 chain config for alpha testnet ([#12962](#12962)) ([e13edb8](e13edb8)) * Reduce bots ([#12953](#12953)) ([4bbc5da](4bbc5da)) * remove selector from public call request ([#12828](#12828)) ([18bcc1b](18bcc1b)) * replace relative paths to noir-protocol-circuits ([61cf4b6](61cf4b6)) * replace relative paths to noir-protocol-circuits ([4356c17](4356c17)) * replace relative paths to noir-protocol-circuits ([f73f47d](f73f47d)) * Set default proving config to true ([#12964](#12964)) ([75c1549](75c1549)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Ports field greater than to vm2, removing non-ff and eq functionality, which could be trivally inlined in other gadgets.

🤖 I have created a new Aztec Packages release --- ## [0.82.1](v0.82.0...v0.82.1) (2025-03-24) ### Features * **avm:** Port field gt to vm2 ([#12883](#12883)) ([0ae6891](0ae6891)) * use msgpack for ClientIvc::Proof in API ([#12911](#12911)) ([1a01602](1a01602)) ### Bug Fixes * disable proving on vite box ([#12971](#12971)) ([69a0fb6](69a0fb6)) * no hardcoded versions in bbup ([#12944](#12944)) ([397144f](397144f)) * pull CRS data ahead of time ([#12945](#12945)) ([43155d6](43155d6)) * Remove workaround ([#12952](#12952)) ([c3337af](c3337af)) * set the correct env var ([#12959](#12959)) ([bd0f4b2](bd0f4b2)) * yolo add bunch of test flakes ([13c19da](13c19da)) * yolo e2e_p2p tests now fully skipped due to huge speed regression ([9141410](9141410)) * yolo txe binds just to localhost by default. ([3933b35](3933b35)) ### Miscellaneous * Change `/bin/bash` shebang to be env based ([#12834](#12834)) ([7843a67](7843a67)) * clean up avm codeowners ([#12860](#12860)) ([35a8f46](35a8f46)) * deflake the kind smoke test ([#12955](#12955)) ([1a37d6d](1a37d6d)), closes [#11177](#11177) * fee cleanup ([#12941](#12941)) ([fdf1da4](fdf1da4)) * Increase bot count ([#12963](#12963)) ([16edd06](16edd06)) * L2 chain config for alpha testnet ([#12962](#12962)) ([e13edb8](e13edb8)) * Reduce bots ([#12953](#12953)) ([4bbc5da](4bbc5da)) * remove selector from public call request ([#12828](#12828)) ([18bcc1b](18bcc1b)) * replace relative paths to noir-protocol-circuits ([61cf4b6](61cf4b6)) * replace relative paths to noir-protocol-circuits ([4356c17](4356c17)) * replace relative paths to noir-protocol-circuits ([f73f47d](f73f47d)) * Set default proving config to true ([#12964](#12964)) ([75c1549](75c1549)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Ports field greater than to vm2, removing non-ff and eq functionality, which could be trivally inlined in other gadgets.

🤖 I have created a new Aztec Packages release --- ## [0.82.1](v0.82.0...v0.82.1) (2025-03-24) ### Features * **avm:** Port field gt to vm2 ([#12883](#12883)) ([0ae6891](0ae6891)) * use msgpack for ClientIvc::Proof in API ([#12911](#12911)) ([1a01602](1a01602)) ### Bug Fixes * disable proving on vite box ([#12971](#12971)) ([69a0fb6](69a0fb6)) * no hardcoded versions in bbup ([#12944](#12944)) ([397144f](397144f)) * pull CRS data ahead of time ([#12945](#12945)) ([43155d6](43155d6)) * Remove workaround ([#12952](#12952)) ([c3337af](c3337af)) * set the correct env var ([#12959](#12959)) ([bd0f4b2](bd0f4b2)) * yolo add bunch of test flakes ([13c19da](13c19da)) * yolo e2e_p2p tests now fully skipped due to huge speed regression ([9141410](9141410)) * yolo txe binds just to localhost by default. ([3933b35](3933b35)) ### Miscellaneous * Change `/bin/bash` shebang to be env based ([#12834](#12834)) ([7843a67](7843a67)) * clean up avm codeowners ([#12860](#12860)) ([35a8f46](35a8f46)) * deflake the kind smoke test ([#12955](#12955)) ([1a37d6d](1a37d6d)), closes [#11177](#11177) * fee cleanup ([#12941](#12941)) ([fdf1da4](fdf1da4)) * Increase bot count ([#12963](#12963)) ([16edd06](16edd06)) * L2 chain config for alpha testnet ([#12962](#12962)) ([e13edb8](e13edb8)) * Reduce bots ([#12953](#12953)) ([4bbc5da](4bbc5da)) * remove selector from public call request ([#12828](#12828)) ([18bcc1b](18bcc1b)) * replace relative paths to noir-protocol-circuits ([61cf4b6](61cf4b6)) * replace relative paths to noir-protocol-circuits ([4356c17](4356c17)) * replace relative paths to noir-protocol-circuits ([f73f47d](f73f47d)) * Set default proving config to true ([#12964](#12964)) ([75c1549](75c1549)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

sirasistant added 6 commits March 18, 2025 17:06

wip field gt gadget

5d81305

fix cpp impl, tests

0af269c

fix

1483b40

add negative test to erase shifts

cbfc1b3

Merge branch 'master' into arv/cmp_gadget_port

d84b030

Add missing processing

9e66121

sirasistant marked this pull request as ready for review March 19, 2025 17:34

sirasistant requested review from IlyasRidhuan, Maddiaa0, fcarreiro and jeanmon as code owners March 19, 2025 17:34

sirasistant commented Mar 19, 2025

View reviewed changes

IlyasRidhuan reviewed Mar 20, 2025

View reviewed changes

sirasistant added 2 commits March 20, 2025 10:13

Merge branch 'master' into arv/cmp_gadget_port

ec936ee

address PR comments

c6cbcf0

fcarreiro reviewed Mar 20, 2025

View reviewed changes

sirasistant added 2 commits March 21, 2025 11:06

Addressed PR comments

5ad9fc0

Merge branch 'master' into arv/cmp_gadget_port

38f10c3

fcarreiro requested review from IlyasRidhuan and fcarreiro March 21, 2025 11:22

fcarreiro reviewed Mar 21, 2025

View reviewed changes

style

eb12895

IlyasRidhuan approved these changes Mar 21, 2025

View reviewed changes

sirasistant merged commit 0ae6891 into master Mar 21, 2025
7 checks passed

sirasistant deleted the arv/cmp_gadget_port branch March 21, 2025 17:36

AztecBot mentioned this pull request Mar 21, 2025

chore(master): release 0.82.1 #12943

Merged

DanielKotov pushed a commit that referenced this pull request Mar 27, 2025

feat(avm): Port field gt to vm2 (#12883)

0c1f5b7

Ports field greater than to vm2, removing non-ff and eq functionality, which could be trivally inlined in other gadgets.

DanielKotov pushed a commit that referenced this pull request Mar 27, 2025

feat(avm): Port field gt to vm2 (#12883)

6924870

Ports field greater than to vm2, removing non-ff and eq functionality, which could be trivally inlined in other gadgets.

		uint256_t a_integer = static_cast<uint256_t>(a);
		uint256_t b_integer = static_cast<uint256_t>(b);

		@@ -0,0 +1,49 @@
		#include "barretenberg/vm2/simulation/field_gt.hpp"


		namespace bb::avm2::simulation {

		bool FieldGreaterThan::ff_gt(const FF& a, const FF& b)


		namespace bb::avm2::simulation {

		const uint256_t TWO_POW_128 = uint256_t{ 1 } << 128;

Conversation

sirasistant commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

fcarreiro left a comment

Choose a reason for hiding this comment

Uh oh!

fcarreiro Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sirasistant Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IlyasRidhuan commented Mar 21, 2025

Uh oh!

jeanmon commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IlyasRidhuan commented Mar 21, 2025

Uh oh!

jeanmon commented Mar 21, 2025

Uh oh!

fcarreiro commented Mar 21, 2025

Uh oh!

sirasistant commented Mar 21, 2025

Uh oh!

fcarreiro 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!

Uh oh!

Reviewers

Assignees

Labels

sirasistant commented Mar 19, 2025 •

edited

Loading

fcarreiro Mar 20, 2025 •

edited

Loading

sirasistant Mar 21, 2025 •

edited

Loading

jeanmon commented Mar 21, 2025 •

edited

Loading