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

Finite base field and elements #12

Merged
merged 14 commits into from
Jul 22, 2024
Merged

Finite base field and elements #12

merged 14 commits into from
Jul 22, 2024

Conversation

trbritt
Copy link
Collaborator

@trbritt trbritt commented Jul 16, 2024

This makes a few upgrades, namely not rolling our own multiprecision arithmetic. It now uses crypto_bigint and its build in functionality for these things.

This required a change to the structure of FinitePrimeField because of the internal layout of crypto_bigint. The biggest issue that I've come across is that is defines a modulus struct on the fly via a macro:

macro_rules! impl_modulus {
    ($name:ident, $uint_type:ty, $value:expr) => {
        #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
        pub struct $name;
        impl<const DLIMBS: usize> $crate::modular::ConstMontyParams<{ <$uint_type>::LIMBS }>
            for $name
        where
            $uint_type: $crate::ConcatMixed<MixedOutput = $crate::Uint<DLIMBS>>,

so in order to use it for our case, I build the necessary structs in house. However, this requires the trait bound pub trait ModulusTrait: std::cmp::Eq + std::default::Default + std::fmt::Debug + std::marker::Copy + Send + Sync + 'static to be flying around, which is verbose, and not ideal.

The field struct now incorporates the modulus information as a generic T, which allows it to port with it all of the associated constants (R, R2, INV, etc) determined by the Montgomery backend.

Unclear how to improve right now. Too zonked.

All operations are constant time.

Further, there are only two tests that currently fail, both of which have to do with the multiplication wrapping around the modulus. I don't know if this is an issue with the reference values in the tests, or the definition of the modulus, or what. Everything else passes no problem, so this is a todo.

One more todo, is to fix the Deref implementation for the FinitePrimeField, as it currently uses unsafe code.

@trbritt trbritt requested a review from 0xAlcibiades July 16, 2024 23:37
@trbritt
Copy link
Collaborator Author

trbritt commented Jul 17, 2024

The updated version has a few big changes from the previous iteration. Namely, crypto_bigint generates the struct that contains the modular information via macro, so in order to use this, you either create a local wrap and then assign all of the needed traits which was very verbose, or do what I did and roll our field generation into a macro itself, which was one of our goals anyhow.

This now defines the DefineFinitePrimeField macro, which generates the struct for a given integer type, and big endian hex string. The usage of crypto_bigint ensures that all of the functionality should work for sensible choices of either of these quantities (ie U32 .. U32k).

The struct created by the macro is designed in the build ladder format, where new() returns a new element of the field based on the input value. All standard binary operators and the euclidean operators are implemented, and pass tests.

There are, however, some issues created now by the usage of crypto_bigint.

  • There are a few operations which return subtle::CtOption or Option, such as element inversion, division and/or remainder
    • The results of the binary operations cannot be changed to return an Option<Self> since this is incompatible with the signatures of the binops. Therefore, all questionable values are currently unwrapped. However, this is not great since the unwrapping panics if it fails. The built in const-time options of crytpto_bigint also don't have any public methods to get around this manually.
  • Talking about panicking, the code will also panic if the modulus is not odd, so we should be able to check this ourselves with Choice::from(U256::from_be_hex("...").as_limbs().0 as u8 & 1) but that is left as a point for discussion.

@trbritt trbritt marked this pull request as ready for review July 17, 2024 19:36
@trbritt trbritt requested a review from merolish July 17, 2024 19:36
Copy link
Member

@0xAlcibiades 0xAlcibiades left a comment

Choose a reason for hiding this comment

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

  • Constant time equality in partial eq
  • Documentation and commentary

@trbritt trbritt requested a review from 0xAlcibiades July 18, 2024 18:23
Copy link
Member

@0xAlcibiades 0xAlcibiades left a comment

Choose a reason for hiding this comment

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

@trbritt please fix clippy, fmt, docs and satisfy the CI pipeline before merge incl test cases passing or marked out not expecting to pass yet. Otherwise looks good.

@0xAlcibiades 0xAlcibiades changed the title BigInt Finite base field and elements Jul 20, 2024
@trbritt trbritt merged commit 0946df3 into main Jul 22, 2024
14 checks passed
@trbritt trbritt deleted the tristan/bigint branch July 22, 2024 15:47
Copy link
Member

@0xAlcibiades 0xAlcibiades left a comment

Choose a reason for hiding this comment

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

Nice.

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