Skip to content
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

[5.0] Adapt to changes in bls12-381 lib and add tests #1882

Merged
merged 30 commits into from
Nov 30, 2023

Conversation

yarkinwho
Copy link
Contributor

@yarkinwho yarkinwho commented Nov 9, 2023

Removes host functions:

  • bls_g1_mul
  • bls_g2_mul
  • bls_g1_exp
  • bls_g2_exp

Adds host functions:

  • bls_g1_weighted_sum
  • bls_g2_weighted_sum
  • bls_fp_mul
  • bls_fp_exp

All bls host functions modified to accept and return affine non-montgomery little endian form.

Removes protocol feature: BLS_PRIMITIVES and adds protocol feature BLS_PRIMITIVES2.

Resolves #1701

@yarkinwho yarkinwho marked this pull request as draft November 9, 2023 07:33
@arhag arhag linked an issue Nov 13, 2023 that may be closed by this pull request
@arhag arhag self-requested a review November 14, 2023 17:56
@yarkinwho yarkinwho marked this pull request as ready for review November 15, 2023 01:40
benchmark/bls.cpp Outdated Show resolved Hide resolved
benchmark/bls.cpp Outdated Show resolved Hide resolved
libraries/chain/webassembly/crypto.cpp Outdated Show resolved Hide resolved
libraries/chain/webassembly/crypto.cpp Outdated Show resolved Hide resolved
libraries/chain/webassembly/crypto.cpp Outdated Show resolved Hide resolved
@greg7mdp
Copy link
Contributor

greg7mdp commented Nov 16, 2023

in fp.cpp, it looks like we check isValid() only if the check flag is set, even though the comment says otherwise:

optional<fp> fp::fromBytesBE(const span<const uint8_t, 48> in, const bool check, const bool raw)
{
    // We decided to always validate the input here. But we reserve the flag.
    fp e = fp(scalar::fromBytesBE<6>(in));
    if(check && !e.isValid()) return {};
    if(raw) return e;
    else    return e.toMont();
}

optional<fp> fp::fromBytesLE(const span<const uint8_t, 48> in, const bool check, const bool raw)
{
    // We decided to always validate the input here. But we reserve the flag.
    fp e = fp(scalar::fromBytesLE<6>(in));
    if(check && !e.isValid()) return {};
    if(raw) return e;
    else    return e.toMont();
}

@greg7mdp
Copy link
Contributor

I'm not quite understanding the logic of this function:

array<uint8_t, 48> fp::toBytesBE(const bool raw) const
{
    if(raw) return scalar::toBytesBE<6>(d);
    else    return scalar::toBytesBE<6>(fromMont().d);
}

In particular what does raw mean. Does it mean, as I understand, that *this is already in Montgomery form?

Is that is the case, when raw is false, and *this is not in Montgomery form, Why do we call fromMont()? Shouldn't it be toMont()?

@greg7mdp
Copy link
Contributor

greg7mdp commented Nov 16, 2023

There are inconsistencies between the types used in the for various APIs.

  • Sometimes we use uint8_t (which translates to unsigned char, as in void g1::toAffineBytesBE(const span<uint8_t, 96> out, const bool raw) const.
  • Sometimes we use char, as in interface::bls_g1_add(span<const char> op1, span<const char> op2, span<char> result) const.

Can we use the same type throughout?

Also use std::array instead of std::vector when appropriate, as it converts implicitely to eosio::chain::span.

This would help making the code simpler and avoid unnecessary repetition of sizes, which can be error-prone.
For example, instead of:

void benchmark_bls_g1_add() {
   // prepare g1 operand in Jacobian LE format
   g1 p = random_g1();
   std::vector<char> buf(96);
   p.toAffineBytesLE(std::span<uint8_t, 96>((uint8_t*)buf.data(), 96), true);
   eosio::chain::span<const char> op1(buf.data(), buf.size());

   // prepare result operand
   std::vector<char> result_buf(96);
   eosio::chain::span<char> result(result_buf.data(), result_buf.size());

   // set up bls_g1_add to be benchmarked
   interface_in_benchmark interface;
   auto g1_add_func = [&]() {
      interface.interface->bls_g1_add(op1, op1, result);
   };

   benchmarking("bls_g1_add", g1_add_func);
}

we could have:

void benchmark_bls_g1_add() {
   // prepare g1 operand in Jacobian LE format
   g1 p = random_g1();
   std::array<unsigned char, 96> op1;
   p.toAffineBytesLE(op1, true);

   // prepare result operand
   std::array<char, 96> result;

   // set up bls_g1_add to be benchmarked
   interface_in_benchmark interface;
   auto g1_add_func = [&]() {
      interface.interface->bls_g1_add(op1, op1, result);
   };

   benchmarking("bls_g1_add", g1_add_func);
}

@yarkinwho
Copy link
Contributor Author

I'm not quite understanding the logic of this function:

array<uint8_t, 48> fp::toBytesBE(const bool raw) const
{
    if(raw) return scalar::toBytesBE<6>(d);
    else    return scalar::toBytesBE<6>(fromMont().d);
}

In particular what does raw mean. Does it mean, as I understand, that *this is already in Montgomery form?

Is that is the case, when raw is false, and *this is not in Montgomery form, Why do we call fromMont()? Shouldn't it be toMont()?

raw here means whether the the output should be the "raw" form used for internal calculation, aka. Montgomery form.
*this is supposed to always be in Montgomery form. That's why we need extra caution when constructing fp.

.

@yarkinwho
Copy link
Contributor Author

in fp.cpp, it looks like we check isValid() only if the check flag is set, even though the comment says otherwise:

optional<fp> fp::fromBytesBE(const span<const uint8_t, 48> in, const bool check, const bool raw)
{
    // We decided to always validate the input here. But we reserve the flag.
    fp e = fp(scalar::fromBytesBE<6>(in));
    if(check && !e.isValid()) return {};
    if(raw) return e;
    else    return e.toMont();
}

optional<fp> fp::fromBytesLE(const span<const uint8_t, 48> in, const bool check, const bool raw)
{
    // We decided to always validate the input here. But we reserve the flag.
    fp e = fp(scalar::fromBytesLE<6>(in));
    if(check && !e.isValid()) return {};
    if(raw) return e;
    else    return e.toMont();
}

Oh, that's the comment I forgot to delete...

@yarkinwho
Copy link
Contributor Author

There are inconsistencies between the types used in the for various APIs.

  • Sometimes we use uint8_t (which translates to unsigned char, as in void g1::toAffineBytesBE(const span<uint8_t, 96> out, const bool raw) const.
  • Sometimes we use char, as in interface::bls_g1_add(span<const char> op1, span<const char> op2, span<char> result) const.

Can we use the same type throughout?

Also use std::array instead of std::vector when appropriate, as it converts implicitely to eosio::chain::span.

This would help making the code simpler and avoid unnecessary repetition of sizes, which can be error-prone. For example, instead of:

void benchmark_bls_g1_add() {
   // prepare g1 operand in Jacobian LE format
   g1 p = random_g1();
   std::vector<char> buf(96);
   p.toAffineBytesLE(std::span<uint8_t, 96>((uint8_t*)buf.data(), 96), true);
   eosio::chain::span<const char> op1(buf.data(), buf.size());

   // prepare result operand
   std::vector<char> result_buf(96);
   eosio::chain::span<char> result(result_buf.data(), result_buf.size());

   // set up bls_g1_add to be benchmarked
   interface_in_benchmark interface;
   auto g1_add_func = [&]() {
      interface.interface->bls_g1_add(op1, op1, result);
   };

   benchmarking("bls_g1_add", g1_add_func);
}

we could have:

void benchmark_bls_g1_add() {
   // prepare g1 operand in Jacobian LE format
   g1 p = random_g1();
   std::array<unsigned char, 96> op1;
   p.toAffineBytesLE(op1, true);

   // prepare result operand
   std::array<char, 96> result;

   // set up bls_g1_add to be benchmarked
   interface_in_benchmark interface;
   auto g1_add_func = [&]() {
      interface.interface->bls_g1_add(op1, op1, result);
   };

   benchmarking("bls_g1_add", g1_add_func);
}

good points. guess that's because that's code originally written by different people :)
Let me try if i can unify them

@greg7mdp
Copy link
Contributor

I'm not quite understanding the logic of this function:

array<uint8_t, 48> fp::toBytesBE(const bool raw) const
{
    if(raw) return scalar::toBytesBE<6>(d);
    else    return scalar::toBytesBE<6>(fromMont().d);

raw here means whether the the output should be the "raw" form used for internal calculation, aka. Montgomery form. *this is supposed to always be in Montgomery form. That's why we need extra caution when constructing fp.

So when we call scalar::toBytesBE<6>(), would it not work if we used the Montgomery form as input?

@yarkinwho
Copy link
Contributor Author

yarkinwho commented Nov 16, 2023

I'm not quite understanding the logic of this function:

array<uint8_t, 48> fp::toBytesBE(const bool raw) const
{
    if(raw) return scalar::toBytesBE<6>(d);
    else    return scalar::toBytesBE<6>(fromMont().d);

raw here means whether the the output should be the "raw" form used for internal calculation, aka. Montgomery form. *this is supposed to always be in Montgomery form. That's why we need extra caution when constructing fp.

So when we call scalar::toBytesBE<6>(), would it not work if we used the Montgomery form as input?

scalar is defined as numbers that are not in GF(p) so there's no Montgomery form, it always views bytes as plain number.

And Montgomery form is pretty much just map the number to another value in the group, so no matter the fp is in Montgomery form or not, its bytes, viewed by scalar, are always valid number.

So:
if(raw) return scalar::toBytesBE<6>(d); <-we want to export the "raw" number, so treat it as scalar and export.
else return scalar::toBytesBE<6>(fromMont().d); <- we want to export the "not raw" number so it will need to be converted back "fromMont"

@greg7mdp
Copy link
Contributor

greg7mdp commented Nov 16, 2023

A suggestion, do you think it might be better to define the sizes as members instead of having the hardcoded sizes everywhere. We could also define buffer types:

class g1
{

public:
    fp x;
    fp y;
    fp z;

    static constexpr size_t jacobian_sz   = 144;
    static constexpr size_t affine_sz     = 96;
    static constexpr size_t compressed_sz = 48;

    using jacobian_buf   = std::array<uint8_t, jacobian_sz>;
    using affine_buf     = std::array<uint8_t, affine_sz>;
    using compressed_buf = std::array<uint8_t, compressed_sz>;
    
    g1();
    explicit g1(const std::array<fp, 3>& e3);
    g1(const g1& e);
    static std::optional<g1> fromJacobianBytesBE(const std::span<const uint8_t, jacobian_sz> in, const bool check = false, const bool raw = false);
...

@yarkinwho
Copy link
Contributor Author

A suggestion, do you think it might be better to define the sizes as members instead of having the hardcoded sizes everywhere. We could also define buffer types:

class g1
{

public:
    fp x;
    fp y;
    fp z;

    static constexpr size_t jacobian_sz   = 144;
    static constexpr size_t affine_sz     = 96;
    static constexpr size_t compressed_sz = 48;

    using jacobian_buf   = std::array<uint8_t, jacobian_sz>;
    using affine_buf     = std::array<uint8_t, affine_sz>;
    using compressed_buf = std::array<uint8_t, compressed_sz>;
    
    g1();
    explicit g1(const std::array<fp, 3>& e3);
    g1(const g1& e);
    static std::optional<g1> fromJacobianBytesBE(const std::span<const uint8_t, jacobian_sz> in, const bool check = false, const bool raw = false);
...

Not sure, usually it's not good to have magic numbers around, but in this case, maybe it's more clear to have those numbers in function declaration so that people don't need to click around to find the correct size.

@greg7mdp
Copy link
Contributor

greg7mdp commented Nov 16, 2023

Not sure, usually it's not good to have magic numbers around, but in this case, maybe it's more clear to have those numbers in function declaration so that people don't need to click around to find the correct size.

Hum, I don't know, maybe @arhag can give his opinion.

I think I'd even define span types, so we'd have something like this:

class g1
{

public:
    fp x;
    fp y;
    fp z;

    static constexpr size_t jacobian_sz   = 144;
    static constexpr size_t affine_sz     = 96;
    static constexpr size_t compressed_sz = 48;

    using jacobian_buf   = std::array<uint8_t, jacobian_sz>;
    using affine_buf     = std::array<uint8_t, affine_sz>;
    using compressed_buf = std::array<uint8_t, compressed_sz>;

    using jacobian_in    = std::span<const uint8_t, jacobian_sz>;
    using jacobian_out   = std::span<uint8_t, jacobian_sz>;
    using affine_in      = std::span<const uint8_t, affine_sz>;
    using affine_out     = std::span<uint8_t, affine_sz>;
    using compressed_in  = std::span<const uint8_t, compressed_sz>;
    using compressed_out = std::span<uint8_t, compressed_sz>;
    
    
    g1();
    explicit g1(const std::array<fp, 3>& e3);
    g1(const g1& e);
    static std::optional<g1> fromJacobianBytesBE(const jacobian_in in, const bool check = false, const bool raw = false);
    static std::optional<g1> fromJacobianBytesLE(const jacobian_in in, const bool check = false, const bool raw = false);
    static std::optional<g1> fromAffineBytesBE(const affine_in in, const bool check = false, const bool raw = false);
    static std::optional<g1> fromAffineBytesLE(const affine_in in, const bool check = false, const bool raw = false);
    static std::optional<g1> fromCompressedBytesBE(const compressed_in in);
    void toJacobianBytesBE(const jacobian_out out, const bool raw = false) const;
    void toJacobianBytesLE(const jacobian_out out, const bool raw = false) const;
    void toAffineBytesBE(const affine_out out, const bool raw = false) const;
    void toAffineBytesLE(const affine_out out, const bool raw = false) const;
    void toCompressedBytesBE(const compressed_out out) const;
    jacobian_buf toJacobianBytesBE(const bool raw = false) const;
    jacobian_buf toJacobianBytesLE(const bool raw = false) const;
    affine_buf toAffineBytesBE(const bool raw = false) const;
    affine_buf toAffineBytesLE(const bool raw = false) const;
    compressed_buf toCompressedBytesBE() const;

In user code, I think this:

   g2::affine_buf buf;
   random_g2().toAffineBytesLE(buf, true);

is better than this:

   std::vector<char> buf(192);
   random_g2().toAffineBytesLE(std::span<uint8_t, 192>((uint8_t*)buf.data(), 192), true);

@yarkinwho
Copy link
Contributor Author

Not sure, usually it's not good to have magic numbers around, but in this case, maybe it's more clear to have those numbers in function declaration so that people don't need to click around to find the correct size.

Hum, I don't know, maybe @arhag can give his opinion.

I think I'd even define span types, so we'd have something like this:

class g1
{

public:
    fp x;
    fp y;
    fp z;

    static constexpr size_t jacobian_sz   = 144;
    static constexpr size_t affine_sz     = 96;
    static constexpr size_t compressed_sz = 48;

    using jacobian_buf   = std::array<uint8_t, jacobian_sz>;
    using affine_buf     = std::array<uint8_t, affine_sz>;
    using compressed_buf = std::array<uint8_t, compressed_sz>;

    using jacobian_in    = std::span<const uint8_t, jacobian_sz>;
    using jacobian_out   = std::span<uint8_t, jacobian_sz>;
    using affine_in      = std::span<const uint8_t, affine_sz>;
    using affine_out     = std::span<uint8_t, affine_sz>;
    using compressed_in  = std::span<const uint8_t, compressed_sz>;
    using compressed_out = std::span<uint8_t, compressed_sz>;
    
    
    g1();
    explicit g1(const std::array<fp, 3>& e3);
    g1(const g1& e);
    static std::optional<g1> fromJacobianBytesBE(const jacobian_in in, const bool check = false, const bool raw = false);
    static std::optional<g1> fromJacobianBytesLE(const jacobian_in in, const bool check = false, const bool raw = false);
    static std::optional<g1> fromAffineBytesBE(const affine_in in, const bool check = false, const bool raw = false);
    static std::optional<g1> fromAffineBytesLE(const affine_in in, const bool check = false, const bool raw = false);
    static std::optional<g1> fromCompressedBytesBE(const compressed_in in);
    void toJacobianBytesBE(const jacobian_out out, const bool raw = false) const;
    void toJacobianBytesLE(const jacobian_out out, const bool raw = false) const;
    void toAffineBytesBE(const affine_out out, const bool raw = false) const;
    void toAffineBytesLE(const affine_out out, const bool raw = false) const;
    void toCompressedBytesBE(const compressed_out out) const;
    jacobian_buf toJacobianBytesBE(const bool raw = false) const;
    jacobian_buf toJacobianBytesLE(const bool raw = false) const;
    affine_buf toAffineBytesBE(const bool raw = false) const;
    affine_buf toAffineBytesLE(const bool raw = false) const;
    compressed_buf toCompressedBytesBE() const;

In user code, I think this:

   g2::affine_buf buf;
   random_g2().toAffineBytesLE(buf, true);

is better than this:

   std::vector<char> buf(192);
   random_g2().toAffineBytesLE(std::span<uint8_t, 192>((uint8_t*)buf.data(), 192), true);

I guess if we are going to pick similar approaches, we will need some better design for hexToBytes and bytesToHex methods. With the current design, I think we will still need to put some hardcoded number to copy data to buf in some steps.

linh2931 and others added 2 commits November 16, 2023 19:09
Adds new cryptographic host functions
- Add, multiply, multi-exponentiation and pairing functions for the bls12-381 elliptic curve.
- Add, weighted-sum, map, and pairing functions for the bls12-381 elliptic curve.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://t.me/jungletestnet/87773

Test restarting a node that had the previous digest for BLS_PRIMITIVES. Consider renaming if it avoid the restart issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Member

@arhag arhag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also update bls12-381 library submodule to the commit https://github.com/AntelopeIO/bls12-381/commits/13888087ac9225b1f266fee8b149c4effe10fabb.

Otherwise, looks good to me.

* @param[out] result - the result op1 + op2.
* @return -1 if there was an error 0 otherwise
* @param op1 - a span containing the affine coordingates of the first operand G1 point - 96 bytes little-endian.
* @param op2 - a span containing the affine coordingates of the second operand G1 point - 96 bytes little-endian.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param op2 - a span containing the affine coordingates of the second operand G1 point - 96 bytes little-endian.
* @param op2 - a span containing the affine coordinates of the second operand G1 point - 96 bytes little-endian.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #1938

* @param[out] result - the result op1 + op2.
* @return -1 if there was an error 0 otherwise
* @param op1 - a span containing the affine coordingates of the first operand G2 point - 192 bytes little-endian.
* @param op2 - a span containing the affine coordingates of the second operand G2 point - 192 bytes little-endian.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param op2 - a span containing the affine coordingates of the second operand G2 point - 192 bytes little-endian.
* @param op2 - a span containing the affine coordinates of the second operand G2 point - 192 bytes little-endian.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #1938

@heifner heifner merged commit c584cdb into release/5.0 Nov 30, 2023
29 checks passed
@heifner heifner deleted the yarkin/update_bls branch November 30, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify BLS host functions to fix failing tests on ARM builds
5 participants