-
Notifications
You must be signed in to change notification settings - Fork 615
chore: field_conversion clean-up/int audit
#16898
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
Changes from 10 commits
9206325
7d1760a
abe1b42
0c8f5e6
d2e6bc8
c377bdd
acdaa51
a63559d
af8117b
5bfb6f6
0ca0024
f21bb97
3e792a5
74fcd3d
3d4efb4
883d484
f7e2154
3c37128
a0dfb28
95c68fd
6e9b48b
0ed5343
e0b240c
27c18bb
5436aca
a84c115
599e0fc
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 |
|---|---|---|
|
|
@@ -291,7 +291,7 @@ template <typename RecursiveFlavor> class RecursiveVerifierTest : public testing | |
| } | ||
| // Check the size of the recursive verifier | ||
| if constexpr (std::same_as<RecursiveFlavor, MegaZKRecursiveFlavor_<UltraCircuitBuilder>>) { | ||
| uint32_t NUM_GATES_EXPECTED = 796978; | ||
| uint32_t NUM_GATES_EXPECTED = 803143; | ||
|
Contributor
Author
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. More gates due to enabled on-curve checks |
||
| ASSERT_EQ(static_cast<uint32_t>(outer_circuit.get_num_finalized_gates()), NUM_GATES_EXPECTED) | ||
| << "MegaZKHonk Recursive verifier changed in Ultra gate count! Update this value if you " | ||
| "are sure this is expected."; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ template <class Builder_, class Fq, class Fr, class NativeGroup> class element { | |
| element(); | ||
| element(const typename NativeGroup::affine_element& input); | ||
| element(const Fq& x, const Fq& y); | ||
| element(const Fq& x, const Fq& y, const bool_ct& is_infinity); | ||
|
Contributor
Author
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. feels like a very natural constructor, here and in other groups |
||
|
|
||
| element(const element& other); | ||
| element(element&& other) noexcept; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,46 +5,44 @@ | |
| // ===================== | ||
|
|
||
| #include "barretenberg/stdlib/primitives/field/field_conversion.hpp" | ||
| #include "barretenberg/common/assert.hpp" | ||
|
|
||
| namespace bb::stdlib::field_conversion { | ||
|
|
||
| /** | ||
| * @brief Converts a challenge to a fq<Builder> | ||
| * @details We sometimes need challenges that are a bb::fq element, so we need to convert the bb::fr challenge to a | ||
| * bb::fq type. We do this by in a similar fashion to the convert_from_bn254_frs function that converts to a | ||
| * fq<Builder>. In fact, we do call that function that the end, but we first have to split the fr<Builder> into two | ||
| * pieces, one that is the 136 lower bits and one that is the 118 higher bits. Then, we can split these two pieces into | ||
| * their bigfield limbs through convert_from_bn254_frs, which is actually just a bigfield constructor that takes in two | ||
| * two-limb frs. | ||
| * @brief Converts an in-circuit `fr`element to an `fq`, i.e. `field_t` --> `bigfield`. | ||
| * | ||
| * TODO(https://github.com/AztecProtocol/barretenberg/issues/850): audit this function more carefully | ||
| * @details Our circuit builders are `fr`-native, which results in challenges being `field_t` elements. However, | ||
| * ECCVMRecursiveVerifier and IPA Recursive Verification need challenges that are `bigfield` elements. We do this in | ||
| * a similar fashion to the `convert_from_bn254_frs` function that converts to a `bigfield`. We split the `field_t` | ||
| * into two pieces, one that is the 136 lower bits and one that is the 118 higher bits, assert the correctness of the | ||
| * decomposition, and invoke the `bigfield` constructor. | ||
| * @tparam Builder | ||
| */ | ||
| template <typename Builder> fq<Builder> convert_to_grumpkin_fr(Builder& builder, const fr<Builder>& f) | ||
| template <typename Builder> fq<Builder> convert_to_grumpkin_fr(Builder& builder, const fr<Builder>& fr_element) | ||
| { | ||
| constexpr uint64_t NUM_BITS_IN_TWO_LIMBS = 2 * NUM_LIMB_BITS; // 136 | ||
| constexpr uint64_t UPPER_TWO_LIMB_BITS = TOTAL_BITS - NUM_BITS_IN_TWO_LIMBS; // 118 | ||
| static constexpr uint64_t NUM_LIMB_BITS = fq<Builder>::NUM_LIMB_BITS; | ||
|
|
||
| constexpr uint64_t NUM_BITS_IN_TWO_LIMBS = 2 * NUM_LIMB_BITS; // 136 | ||
|
|
||
| constexpr uint256_t shift = (uint256_t(1) << NUM_BITS_IN_TWO_LIMBS); | ||
| // split f into low_bits_in and high_bits_in | ||
| constexpr uint256_t LIMB_MASK = shift - 1; // mask for upper 128 bits | ||
| const uint256_t value = f.get_value(); | ||
| const uint256_t value = fr_element.get_value(); | ||
| const uint256_t low_val = static_cast<uint256_t>(value & LIMB_MASK); | ||
| const uint256_t hi_val = static_cast<uint256_t>(value >> NUM_BITS_IN_TWO_LIMBS); | ||
|
|
||
| fr<Builder> low{ witness_t<Builder>(&builder, low_val) }; | ||
| fr<Builder> hi{ witness_t<Builder>(&builder, hi_val) }; | ||
| // range constrain low to 136 bits and hi to 118 bits | ||
| builder.create_range_constraint(low.witness_index, NUM_BITS_IN_TWO_LIMBS, "create_range_constraint"); | ||
| builder.create_range_constraint(hi.witness_index, UPPER_TWO_LIMB_BITS, "create_range_constraint"); | ||
|
|
||
| BB_ASSERT_EQ(static_cast<uint256_t>(low_val) + (static_cast<uint256_t>(hi_val) << NUM_BITS_IN_TWO_LIMBS), value); | ||
| constrain_bigfield_limbs(low, hi); | ||
|
|
||
| BB_ASSERT_EQ(static_cast<uint256_t>(low_val) + (static_cast<uint256_t>(hi_val) << NUM_BITS_IN_TWO_LIMBS), | ||
| value, | ||
| "field_conversion: limb decomposition"); | ||
| // checks this decomposition low + hi * 2^64 = value with an assert_equal | ||
|
Contributor
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. nit: the comment needs to be corrected to:
Contributor
Author
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. oops, stale comment, fixed |
||
| auto sum = low + hi * shift; | ||
| builder.assert_equal(f.witness_index, sum.witness_index, "assert_equal"); | ||
| const fr<Builder> zero = fr<Builder>::from_witness_index(&builder, 0); | ||
| fr<Builder>::evaluate_linear_identity(hi * shift, low, -fr_element, zero); | ||
|
Contributor
Author
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. minus 1 gate thanks to combining the addition with constraining to be 0
Contributor
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. nice! |
||
|
|
||
| std::vector<fr<Builder>> fr_vec{ low, hi }; | ||
| return convert_from_bn254_frs<Builder, fq<Builder>>(builder, fr_vec); | ||
| return fq<Builder>(low, hi); | ||
|
Contributor
Author
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. not calling |
||
| } | ||
|
|
||
| template fq<UltraCircuitBuilder> convert_to_grumpkin_fr<UltraCircuitBuilder>(UltraCircuitBuilder& builder, | ||
|
|
||
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.
Here and in several places below,
builderis redundant asfield_conversioncan extract from the fields being passed while ensuring that those are in-circuit.