Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

EIP-212 (bn128 curve pairing) #5307

Merged
merged 10 commits into from
Apr 21, 2017
Merged

EIP-212 (bn128 curve pairing) #5307

merged 10 commits into from
Apr 21, 2017

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Mar 27, 2017

goes after #4999

@NikVolf NikVolf added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Mar 27, 2017
@arkpar arkpar mentioned this pull request Mar 28, 2017
12 tasks
@NikVolf
Copy link
Contributor Author

NikVolf commented Mar 28, 2017

for reference ethereum/EIPs#212

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Apr 3, 2017
@arkpar
Copy link
Collaborator

arkpar commented Apr 5, 2017

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 5, 2017

@arkpar
it's just format fail test, no happy path
will add once eip specifies official test vectors

@@ -191,6 +192,9 @@ struct Bn128AddImpl;
#[derive(Debug)]
struct Bn128MulImpl;

#[derive(Debug)]
struct Bn128ParingImpl;
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "Paring"

fn execute(&self, input: &[u8], output: &mut BytesRef) -> Result<(), Error> {
use bn::{AffineG1, AffineG2, Fq, Fq2, pairing, G1, G2, Gt};

let elements = input.len() / 192; // (a, b_a, b_b - each 64-byte affine coordinates)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the input for builtins supposed to be infinitely zero-extended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this builtin, no.

@rphmeier
Copy link
Contributor

rphmeier commented Apr 7, 2017

Could you give a summary of changes to the "bn" library?

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 7, 2017

@rphmeier

no new logic, just some internal api exposed, which will allow

  • construct G1/G2 primitives from raw data
  • convert them between jacobian/affine representations
  • deconstruct primitives to the underlining data where applicable

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 18, 2017
@@ -713,7 +815,82 @@ mod tests {
assert!(res.is_err(), "There should be built-in error here");
}
}

fn builitin_pairing() -> Builtin {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: builitin

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Apr 19, 2017
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 21, 2017
@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 21, 2017
).expect("Generator P1(1, 2) is a valid curve point"));
}

lazy_static! {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these 3 lazy_static invocations could be combined. These are generators for alt_bn128, right? Eventually it would be nice to have these extracted to constants in a bn::alt_gen module, but not a blocker for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep sounds good

@rphmeier rphmeier merged commit 0768ce3 into master Apr 21, 2017
@rphmeier rphmeier deleted the eip-212 branch April 21, 2017 11:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants