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

Problems in mcl_field #177

Closed
maths644311798 opened this issue Dec 20, 2023 · 4 comments
Closed

Problems in mcl_field #177

maths644311798 opened this issue Dec 20, 2023 · 4 comments
Assignees

Comments

@maths644311798
Copy link
Contributor

The problems are mainly about mcl_field in yacl/crypto/base/field.
(1)
class Field is defined in crypto/base/field/ field_spi.h. class GaloisField is defined in /math/galois_field/gf_spi.h. Why not combine the two definitions (classes) into one?
(2) In crypto/base/field/mcl/mcl_field.cc,

template <typename T_, size_t degree_>
std::string MclField<T_, degree_>::GetFieldName() const {
  return fmt::format("<MclField F_p^{}>", degree_,
                     Mpz2Mp(T_::BaseFp::getOp().mp));
}

Should the fmt sentence be fmt::format("<MclField F_{}^{}>", Mpz2Mp(T_::BaseFp::getOp().mp),degree_);?
(3)

template <typename T_, size_t degree_>
class MclField : public Field
…

Why does this template need “degree_”? I doubt if this parameter is provided by some interface of T_.
(4) In mcl_field.cc,

template <typename T_, size_t degree_>
FElement MclField<T_, degree_>::Rand() const {
  using BaseFp = typename T_::BaseFp;
  const auto per_size = (BaseFp::getOp().mp.getBitSize() + 7) / 8;

  auto ret = MakeShared<T_>();
  Buffer buf(per_size * degree_);
  BaseFp p;
  for (uint64_t i = 0; i < degree_; i++) {
    p.setByCSPRNG();
    p.serialize(buf.data<uint8_t>() + i * per_size, per_size);
  }

  CastAny<T_>(ret)->deserialize(buf.data<uint8_t>(), buf.size());
  return ret;
}

Can we use T_:: setByCSPRNG(); instead of T_:: BaseFp:: setByCSPRNG();?

(5) In mcl_field.cc,

#include "yacl/crypto/base/field/mcl/mcl_field.h"

#include "mcl/fp_tower.hpp"
#include "mcl/op.hpp"
#include "mcl_field.h"
…

Are the two files "mcl_field.h" the same?

@usafchn
Copy link
Member

usafchn commented Dec 20, 2023

(1)有一些历史原因,crypto/base/field/ field_spi.h 内部其实已经删掉了,以后都在 yacl/math/galois_field

(2)是的,这是一个 bug。 这一类问题理论上升到 C++20 后会自动发现,因为 parse string 默认在编译期执行,括号-变量不匹配编译过不了。。

(3)这里的 T_ 是 mcl 库内部的类型,不方便自己添加字段。在 SPI 模式里面,instance 都通过 factory 创建,用户不会直接接触 MclField class,问题不大

(4) @xfap 会解答

(5)是的,一个编码规范问题,mcl 之前是实验性集成,正式代码会放在 yacl/math/galois_field 下面

1和5 需等下一次 repo sync (很遗憾没有赶上今天的 repo sync 班车)

感谢 @maths644311798

@xfap
Copy link

xfap commented Dec 25, 2023

(4) 是因为T_代表了几种有限域,Fp,Fp^2, Fp^6, Fp^12,其中仅有Fp才提供了Rand接口,其中各个有限域的BaseFp = Fp,因此需要通过BaseFp::Rand(),以及扩域degree去实现对应有限域的随机抽取。 @maths644311798

@maths644311798
Copy link
Contributor Author

在最近更新(#178)之后,mcl_field.cc中有段代码


template <typename T, size_t degree>
MclField<T, degree>::MclField(const MPInt& order, Type field_type) {
  switch (field_type) {
    case Type::Normal: {
      order_ = 0_mp;
      order_mul_ = 0_mp;
      order_add_ = order;
      break;
    }
    case Type::Mul: {
      order_ = 0_mp;
      order_mul_ = order;
      order_add_ = 0_mp;
      break;
    }
    default: {
      order_ = order;
      order_mul_ = order - 1_mp;
      order_add_ = order_;
    }
  }
}

我不确定枚举类enum Type {Normal, Add, Mul,};的确切含义,但是 MclField<T, degree>::MclField (…)中的Type::Normal情况是否应该改为 Type::Add

@xfap
Copy link

xfap commented Jan 2, 2024

@maths644311798 你好,enum Type {Normal, Add, Mul}涉及到有限域这个数学概念。
有限域概念:有限域本身是一个加法交换群,剔除zero element后的其余元素构成一个乘法交换群。上述类中的

  • Type::Normal表示将类视为普通的有限域;
  • Type::Mul表示实际使用的是有限域上的乘法子群;
  • Type::Add则对应于有限域上的加法子群。

构造函数中case Type::Normal...是应该修改为Type::Add,您方便的话,可以提个PR~

BTW, 目前MclField主要是为了mcl pairing(yacl/crypto/base/pairing/mcl/...)使用,尚不推荐单独直接使用。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants