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

Change Scalar::new input from u128 to BigUint #4

Merged
merged 4 commits into from
Aug 30, 2024

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Aug 29, 2024

This PR does these things:

  • Update pyo3 to the latest version.
  • Add the num-bigint feature which allows us to use bigints.
  • Update the Scalar::new method to take BigUint instead of u128.
  • Add a quick test to examples/scalar.py to ensure it works.
  • Override __truediv__ to allow scalar division.
  • Override __int__ so we can convert back to an integer in Python.

Comment on lines +231 to +240
let bls_modulus = BigUint::from_str(BLS_MODULUS).unwrap();
let base_bigint = BigUint::from_bytes_le(self.to_le_bytes()?.as_slice());
let exp_bigint = BigUint::from_bytes_le(exp.to_le_bytes()?.as_slice());
let result = base_bigint.modpow(&exp_bigint, &bls_modulus);
Ok(Scalar(
ark_bls12_381::Fr::from_str(&*result.to_string()).map_err(|_| {
exceptions::PyValueError::new_err("Failed to convert result to scalar")
})?,
))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not blocking as we can speed this up after -- Fr should have a pow method that one can use too, which should allow us to remove the BLS_MODULUS import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it's exponent parameter is a u32, so it won't work for us 😢

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its more than a u32, last time I checked or that would be almost unusable!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might have been &[u32] or something of that nature

Copy link
Contributor Author

@jtraglia jtraglia Aug 30, 2024

Choose a reason for hiding this comment

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

Ah you're right. I can fix this! Here's a screenshot of the method:

image

I must have misinterpreted it. Good catch!

Copy link
Collaborator

@kevaundray kevaundray Aug 30, 2024

Choose a reason for hiding this comment

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

No worries! Feel free to push another PR when ready :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +226 to +227
// Bug, Fr::to_string will print nothing if the value is zero
BigUint::from_str(&*self.0.to_string()).unwrap_or(BigUint::ZERO)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also not blocking: I've tried to avoid the str method in arkworks in place of using hex since the .to_string method had weird behaviours last time I checked and the way it was doing the conversion seemed non-optimal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevaundray kevaundray merged commit 7825c7a into crate-crypto:master Aug 30, 2024
50 checks passed
@jtraglia jtraglia deleted the allow-big-ints branch August 30, 2024 17:10
@kevaundray
Copy link
Collaborator

Merging as comments were non-blocking

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

Successfully merging this pull request may close these issues.

2 participants