-
Notifications
You must be signed in to change notification settings - Fork 32
Conversation
df01c1b
to
5b889a5
Compare
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.
lgtm,
pending @Rjected, not too familiar with the point math -.-
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.
I have one small nit about using the builtin multiplication methods, although the current impl looks fine, especially since tests pass. The move to blst will also change all the API usage
for byte in scalar0.into_iter().rev() { | ||
for bit_index in 0..8 { | ||
let bit = (byte >> bit_index) & 1; | ||
if bit == 0x01 { | ||
q = q.add(n); | ||
} | ||
n = n.double(); | ||
} | ||
} | ||
q |
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.
It looks like G1Projective
implements multiply
(and Mul<Scalar>
), so we probably don't need to reimplement double-and-add
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.
indeed, I started using the methods you mentioned but the results didn't match the test vectors. Finally ended up writing a direct translation of the method used in geth https://github.com/ethereum/go-ethereum/blob/master/crypto/bls12381/g1.go#L345-L356, with this everything is ok.
As you said, with the move to blst everything will change
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.
indeed, I started using the methods you mentioned but the results didn't match the test vectors. Finally ended up writing a direct translation of the method used in geth https://github.com/ethereum/go-ethereum/blob/master/crypto/bls12381/g1.go#L345-L356, with this everything is ok.
As you said, with the move to blst everything will change
hmm, interesting, let's keep this as-is then, and see what happens after blst
Towards #11
Implements the
BLS12_G1MUL
precompile following the geth reference implementation. Includes tests based on test vectors from https://github.com/ethereum/go-ethereum/tree/ab49f228ad6f37ba78be66b34aa5fee740245f57/core/vm/testdata/precompiles