From c6cbe39eed23dc845aef898e937e99de43f71675 Mon Sep 17 00:00:00 2001 From: ledwards2225 <98505400+ledwards2225@users.noreply.github.com> Date: Fri, 7 Jun 2024 12:01:46 -0700 Subject: [PATCH] feat: affine_element read/write with proper handling of point at infinity (#6963) Please read [contributing guidelines](CONTRIBUTING.md) and remove this line. --------- Co-authored-by: codygunton Co-authored-by: ludamad Co-authored-by: ludamad --- .github/workflows/ci.yml | 2 +- .../ultra_circuit_builder.test.cpp | 2 +- .../src/barretenberg/crypto/ecdsa/c_bind.cpp | 4 +- .../crypto/pedersen_commitment/c_bind.cpp | 2 +- .../barretenberg/crypto/schnorr/c_bind.cpp | 8 ++-- .../barretenberg/crypto/schnorr/multisig.hpp | 8 ++-- .../crypto/schnorr/proof_of_possession.hpp | 8 ++-- .../ecc/groups/affine_element.hpp | 43 +++++++++++++---- .../ecc/groups/affine_element.test.cpp | 47 ++++++++++++------- .../examples/join_split/join_split_tx.cpp | 2 +- .../notes/native/value/value_note.hpp | 2 +- .../plonk/composer/ultra_composer.test.cpp | 2 +- .../barretenberg/polynomials/univariate.hpp | 2 +- .../plookup_tables/fixed_base/fixed_base.cpp | 4 +- .../ultra_honk/ultra_composer.test.cpp | 2 +- 15 files changed, 87 insertions(+), 51 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e4c22fa56a0..671ce7a2acb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,7 +35,7 @@ jobs: changes: runs-on: ubuntu-20.04 - # Required permissions + # Required permissions. permissions: pull-requests: read # Set job outputs to values from filter step diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp index 6f814a95844..005a50ea156 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp @@ -65,7 +65,7 @@ TEST(ultra_circuit_constructor, create_gates_from_plookup_accumulators) grumpkin::g1::affine_element base_point = plookup::fixed_base::table::LHS_GENERATOR_POINT; std::vector input_buf; - serialize::write(input_buf, base_point); + write(input_buf, base_point); const auto offset_generators = grumpkin::g1::derive_generators(input_buf, plookup::fixed_base::table::NUM_TABLES_PER_LO_MULTITABLE); diff --git a/barretenberg/cpp/src/barretenberg/crypto/ecdsa/c_bind.cpp b/barretenberg/cpp/src/barretenberg/crypto/ecdsa/c_bind.cpp index 8c0c8295c23..b7ffe3102b5 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/ecdsa/c_bind.cpp +++ b/barretenberg/cpp/src/barretenberg/crypto/ecdsa/c_bind.cpp @@ -8,7 +8,7 @@ WASM_EXPORT void ecdsa__compute_public_key(uint8_t const* private_key, uint8_t* { auto priv_key = from_buffer(private_key); secp256k1::g1::affine_element pub_key = secp256k1::g1::one * priv_key; - serialize::write(public_key_buf, pub_key); + write(public_key_buf, pub_key); } WASM_EXPORT void ecdsa__construct_signature(uint8_t const* message, @@ -45,7 +45,7 @@ WASM_EXPORT void ecdsa__recover_public_key_from_signature(uint8_t const* message ecdsa_signature sig = { r, s, v }; auto recovered_pub_key = ecdsa_recover_public_key( std::string((char*)message, msg_len), sig); - serialize::write(output_pub_key, recovered_pub_key); + write(output_pub_key, recovered_pub_key); } WASM_EXPORT bool ecdsa__verify_signature(uint8_t const* message, diff --git a/barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/c_bind.cpp b/barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/c_bind.cpp index cf0f544090c..c6b16e96b51 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/c_bind.cpp +++ b/barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/c_bind.cpp @@ -11,5 +11,5 @@ WASM_EXPORT void pedersen_commit(fr::vec_in_buf inputs_buffer, grumpkin::g1::aff read(inputs_buffer, to_commit); grumpkin::g1::affine_element pedersen_commitment = crypto::pedersen_commitment::commit_native(to_commit); - serialize::write(output, pedersen_commitment); + write(output, pedersen_commitment); } \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/crypto/schnorr/c_bind.cpp b/barretenberg/cpp/src/barretenberg/crypto/schnorr/c_bind.cpp index b5fc5570e86..8507b5e0f9a 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/schnorr/c_bind.cpp +++ b/barretenberg/cpp/src/barretenberg/crypto/schnorr/c_bind.cpp @@ -13,7 +13,7 @@ WASM_EXPORT void schnorr_compute_public_key(uint8_t const* private_key, uint8_t* { auto priv_key = from_buffer(private_key); grumpkin::g1::affine_element pub_key = grumpkin::g1::one * priv_key; - serialize::write(public_key_buf, pub_key); + write(public_key_buf, pub_key); } WASM_EXPORT void schnorr_negate_public_key(uint8_t const* public_key_buffer, uint8_t* output) @@ -21,7 +21,7 @@ WASM_EXPORT void schnorr_negate_public_key(uint8_t const* public_key_buffer, uin // Negate the public key (effectively negating the y-coordinate of the public key) and return the resulting public // key. auto account_public_key = from_buffer(public_key_buffer); - serialize::write(output, -account_public_key); + write(output, -account_public_key); } WASM_EXPORT void schnorr_construct_signature(uint8_t const* message_buf, @@ -75,10 +75,10 @@ WASM_EXPORT void schnorr_multisig_validate_and_combine_signer_pubkeys(uint8_t co auto combined_key = multisig::validate_and_combine_signer_pubkeys(pubkeys); if (combined_key) { - serialize::write(combined_key_buf, *combined_key); + write(combined_key_buf, *combined_key); *success = true; } else { - serialize::write(combined_key_buf, affine_element::one()); + write(combined_key_buf, affine_element::one()); *success = false; } } diff --git a/barretenberg/cpp/src/barretenberg/crypto/schnorr/multisig.hpp b/barretenberg/cpp/src/barretenberg/crypto/schnorr/multisig.hpp index 4d48bfaec94..c46a2886dc0 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/schnorr/multisig.hpp +++ b/barretenberg/cpp/src/barretenberg/crypto/schnorr/multisig.hpp @@ -166,10 +166,10 @@ template cl domain_separator_nonce.begin(), domain_separator_nonce.end(), std::back_inserter(nonce_challenge_buffer)); // write the group generator - serialize::write(nonce_challenge_buffer, G1::affine_one); + write(nonce_challenge_buffer, G1::affine_one); // write X - serialize::write(nonce_challenge_buffer, aggregate_pubkey); + write(nonce_challenge_buffer, aggregate_pubkey); // we slightly deviate from the protocol when including 'm', since the length of 'm' is variable // by writing a prefix and a suffix, we prevent the message from being interpreted as coming from a different @@ -188,8 +188,8 @@ template cl // write {(R1, S1), ..., (Rn, Sn)} for (const auto& nonce : round_1_nonces) { - serialize::write(nonce_challenge_buffer, nonce.R); - serialize::write(nonce_challenge_buffer, nonce.S); + write(nonce_challenge_buffer, nonce.R); + write(nonce_challenge_buffer, nonce.S); } // uses the different hash function for proper domain separation diff --git a/barretenberg/cpp/src/barretenberg/crypto/schnorr/proof_of_possession.hpp b/barretenberg/cpp/src/barretenberg/crypto/schnorr/proof_of_possession.hpp index f99878900c5..3faf3889ded 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/schnorr/proof_of_possession.hpp +++ b/barretenberg/cpp/src/barretenberg/crypto/schnorr/proof_of_possession.hpp @@ -106,14 +106,14 @@ template struct SchnorrProofOfPossession { std::copy(domain_separator_pop.begin(), domain_separator_pop.end(), std::back_inserter(challenge_buf)); // write the group generator - serialize::write(challenge_buf, G1::affine_one); + write(challenge_buf, G1::affine_one); // write X twice as per the spec - serialize::write(challenge_buf, public_key); - serialize::write(challenge_buf, public_key); + write(challenge_buf, public_key); + write(challenge_buf, public_key); // write R - serialize::write(challenge_buf, R); + write(challenge_buf, R); // generate the raw bits of H_reg(X,X,R) return Hash::hash(challenge_buf); diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.hpp b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.hpp index e50677834e0..7f3754c7e17 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.hpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.hpp @@ -1,4 +1,5 @@ #pragma once +#include "barretenberg/common/serialize.hpp" #include "barretenberg/ecc/curves/bn254/fq2.hpp" #include "barretenberg/numeric/uint256/uint256.hpp" #include "barretenberg/serialize/msgpack.hpp" @@ -101,15 +102,18 @@ template class alignas(64) affine_ * @warning This will need to be updated if we serialize points over composite-order fields other than fq2! * */ - static void serialize_to_buffer(const affine_element& value, uint8_t* buffer) + static void serialize_to_buffer(const affine_element& value, uint8_t* buffer, bool write_x_first = false) { + using namespace serialize; if (value.is_point_at_infinity()) { // if we are infinity, just set all buffer bits to 1 // we only need this case because the below gets mangled converting from montgomery for infinity points memset(buffer, 255, sizeof(Fq) * 2); } else { - Fq::serialize_to_buffer(value.y, buffer); - Fq::serialize_to_buffer(value.x, buffer + sizeof(Fq)); + // Note: for historic reasons we will need to redo downstream hashes if we want this to always be written in + // the same order in our various serialization flows + write(buffer, write_x_first ? value.x : value.y); + write(buffer, write_x_first ? value.y : value.x); } } @@ -125,8 +129,9 @@ template class alignas(64) affine_ * * @warning This will need to be updated if we serialize points over composite-order fields other than fq2! */ - static affine_element serialize_from_buffer(uint8_t* buffer) + static affine_element serialize_from_buffer(const uint8_t* buffer, bool write_x_first = false) { + using namespace serialize; // Does the buffer consist entirely of set bits? If so, we have a point at infinity // Note that if it isn't, this loop should end early. // We only need this case because the below gets mangled converting to montgomery for infinity points @@ -136,8 +141,10 @@ template class alignas(64) affine_ return affine_element::infinity(); } affine_element result; - result.y = Fq::serialize_from_buffer(buffer); - result.x = Fq::serialize_from_buffer(buffer + sizeof(Fq)); + // Note: for historic reasons we will need to redo downstream hashes if we want this to always be read in the + // same order in our various serialization flows + read(buffer, write_x_first ? result.x : result.y); + read(buffer, write_x_first ? result.y : result.x); return result; } @@ -160,11 +167,27 @@ template class alignas(64) affine_ } Fq x; Fq y; - - // for serialization: update with new fields - // TODO(https://github.com/AztecProtocol/barretenberg/issues/908) point at inifinty isn't handled - MSGPACK_FIELDS(x, y); }; + +template +inline void read(B& it, group_elements::affine_element& element) +{ + using namespace serialize; + std::array buffer; + read(it, buffer); + element = group_elements::affine_element::serialize_from_buffer( + buffer.data(), /* use legacy field order */ true); +} + +template +inline void write(B& it, group_elements::affine_element const& element) +{ + using namespace serialize; + std::array buffer; + group_elements::affine_element::serialize_to_buffer( + element, buffer.data(), /* use legacy field order */ true); + write(it, buffer); +} } // namespace bb::group_elements #include "./affine_element_impl.hpp" diff --git a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp index 94b24d95872..f1c29250aaf 100644 --- a/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp +++ b/barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp @@ -66,6 +66,29 @@ template class TestAffineElement : public testing::Test { } } + static void test_read_and_write() + { + // a generic point + { + affine_element P = affine_element(element::random_element()); + [[maybe_unused]] affine_element R; + + std::vector v(sizeof(R)); + uint8_t* ptr = v.data(); + write(ptr, P); + ASSERT_TRUE(P.on_curve()); + + // // Reset to start? + // ptr = v.data(); + + const uint8_t* read_ptr = v.data(); + // good read + read(read_ptr, R); + ASSERT_TRUE(R.on_curve()); + ASSERT_TRUE(P == R); + } + } + static void test_point_compression() { for (size_t i = 0; i < 10; i++) { @@ -110,17 +133,6 @@ template class TestAffineElement : public testing::Test { EXPECT_NE(P < Q, Q < P); } - /** - * @brief Check that msgpack encoding is consistent with decoding - * - */ - static void test_msgpack_roundtrip() - { - // TODO(https://github.com/AztecProtocol/barretenberg/issues/908) point at inifinty isn't handled - auto [actual, expected] = msgpack_roundtrip(affine_element{ 1, 1 }); - EXPECT_EQ(actual, expected); - } - /** * @brief A regression test to make sure the -1 case is covered * @@ -139,11 +151,17 @@ template class TestAffineElement : public testing::Test { } }; -using TestTypes = testing::Types; +using TestTypes = testing::Types; +// using TestTypes = testing::Types; } // namespace TYPED_TEST_SUITE(TestAffineElement, TestTypes); +TYPED_TEST(TestAffineElement, ReadWrite) +{ + TestFixture::test_read_and_write(); +} + TYPED_TEST(TestAffineElement, ReadWriteBuffer) { TestFixture::test_read_write_buffer(); @@ -172,11 +190,6 @@ TYPED_TEST(TestAffineElement, InfinityOrderingRegression) TestFixture::test_infinity_ordering_regression(); } -TYPED_TEST(TestAffineElement, Msgpack) -{ - TestFixture::test_msgpack_roundtrip(); -} - namespace bb::group_elements { // mul_with_endomorphism and mul_without_endomorphism are private in affine_element. // We could make those public to test or create other public utilities, but to keep the API intact we diff --git a/barretenberg/cpp/src/barretenberg/examples/join_split/join_split_tx.cpp b/barretenberg/cpp/src/barretenberg/examples/join_split/join_split_tx.cpp index 9f7adae976d..b9339d6235f 100644 --- a/barretenberg/cpp/src/barretenberg/examples/join_split/join_split_tx.cpp +++ b/barretenberg/cpp/src/barretenberg/examples/join_split/join_split_tx.cpp @@ -25,7 +25,7 @@ void write(std::vector& buf, join_split_tx const& tx) write(buf, tx.account_required); write(buf, tx.account_note_index); write(buf, tx.account_note_path); - serialize::write(buf, tx.signing_pub_key); + write(buf, tx.signing_pub_key); write(buf, tx.backward_link); write(buf, tx.allow_chain); diff --git a/barretenberg/cpp/src/barretenberg/examples/join_split/notes/native/value/value_note.hpp b/barretenberg/cpp/src/barretenberg/examples/join_split/notes/native/value/value_note.hpp index 4912fa22cab..627c498f014 100644 --- a/barretenberg/cpp/src/barretenberg/examples/join_split/notes/native/value/value_note.hpp +++ b/barretenberg/cpp/src/barretenberg/examples/join_split/notes/native/value/value_note.hpp @@ -53,7 +53,7 @@ inline void write(std::vector& buf, value_note const& note) write(buf, note.value); write(buf, note.asset_id); write(buf, note.account_required); - serialize::write(buf, note.owner); + write(buf, note.owner); write(buf, note.secret); write(buf, note.creator_pubkey); write(buf, note.input_nullifier); diff --git a/barretenberg/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp b/barretenberg/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp index ef5f67cd1e7..e46237a4306 100644 --- a/barretenberg/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp +++ b/barretenberg/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp @@ -85,7 +85,7 @@ TYPED_TEST(ultra_plonk_composer, create_gates_from_plookup_accumulators) grumpkin::g1::affine_element base_point = plookup::fixed_base::table::LHS_GENERATOR_POINT; std::vector input_buf; - serialize::write(input_buf, base_point); + write(input_buf, base_point); const auto offset_generators = grumpkin::g1::derive_generators(input_buf, plookup::fixed_base::table::NUM_TABLES_PER_LO_MULTITABLE); diff --git a/barretenberg/cpp/src/barretenberg/polynomials/univariate.hpp b/barretenberg/cpp/src/barretenberg/polynomials/univariate.hpp index 05107c33b5b..2a64416f21e 100644 --- a/barretenberg/cpp/src/barretenberg/polynomials/univariate.hpp +++ b/barretenberg/cpp/src/barretenberg/polynomials/univariate.hpp @@ -686,4 +686,4 @@ template std::array array_to_array namespace std { template struct tuple_size> : std::integral_constant {}; -} // namespace std \ No newline at end of file +} // namespace std diff --git a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/plookup_tables/fixed_base/fixed_base.cpp b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/plookup_tables/fixed_base/fixed_base.cpp index 6a470eb835c..0feae681d45 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/plookup_tables/fixed_base/fixed_base.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/plookup_tables/fixed_base/fixed_base.cpp @@ -56,7 +56,7 @@ template table::fixed_base_scalar_mul_tables table::generate_t result.reserve(NUM_TABLES); std::vector input_buf; - serialize::write(input_buf, input); + write(input_buf, input); const auto offset_generators = grumpkin::g1::derive_generators(input_buf, NUM_TABLES); grumpkin::g1::element accumulator = input; @@ -87,7 +87,7 @@ grumpkin::g1::affine_element table::generate_generator_offset(const grumpkin::g1 constexpr size_t NUM_TABLES = get_num_tables_per_multi_table(); std::vector input_buf; - serialize::write(input_buf, input); + write(input_buf, input); const auto offset_generators = grumpkin::g1::derive_generators(input_buf, NUM_TABLES); grumpkin::g1::element acc = grumpkin::g1::point_at_infinity; for (const auto& gen : offset_generators) { diff --git a/barretenberg/cpp/src/barretenberg/ultra_honk/ultra_composer.test.cpp b/barretenberg/cpp/src/barretenberg/ultra_honk/ultra_composer.test.cpp index 330a2ab8c14..b04bd0b7fd1 100644 --- a/barretenberg/cpp/src/barretenberg/ultra_honk/ultra_composer.test.cpp +++ b/barretenberg/cpp/src/barretenberg/ultra_honk/ultra_composer.test.cpp @@ -169,7 +169,7 @@ TEST_F(UltraHonkComposerTests, create_gates_from_plookup_accumulators) grumpkin::g1::affine_element base_point = plookup::fixed_base::table::LHS_GENERATOR_POINT; std::vector input_buf; - serialize::write(input_buf, base_point); + write(input_buf, base_point); const auto offset_generators = grumpkin::g1::derive_generators(input_buf, plookup::fixed_base::table::NUM_TABLES_PER_LO_MULTITABLE);