-
Notifications
You must be signed in to change notification settings - Fork 259
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
Upgrade field_new
macro to take in signed integers
#96
Conversation
243e875
to
82a93b3
Compare
Awesome change! We should also add a short description somewhere for finding where the actual arithmetic of a field is implemented. I'm worried that this adds another layer of separation between specifying field parameters and being able to find / understand the field arithmetic's implementation Perhaps this is what should go in an |
Yeah, there's a couple of levels of indirection that need to be traced through to find the actual impl. We should indeed document the code to explain the structure. (I don't think this adds any more levels compared to the previous impl though) |
Is it possible to derive other parameters, like I think this is just a bit shift? |
Also, wouldn't you say that utilising the macros for adc/sbb/mac_with_carry etc. is a step backwards in terms of relying on macros instead of functions? Especially given our desire in the future to rid of macros altogether and rely on Is there any real need for it? My guess is you want to deduplicate code across the In this case, I think one should convert all the original functions to (this can be tested with the following snippet:
) One can also get rid of the duplication of code that is impl for I'm not sure if Digression: I think most of the Generators are by default only 2 for prime fields right, with no variation? In which case the only params left to impl concretely are modulus and two_adicity. Theoretically, one could also find square roots of unity and thus find the two-adic root of unity |
Yup!
I did do this initially, but the problem right now is that let (new_a, new_carry) = adc(a, b, carry);
a = new_a;
carry = new_carry; This is much less readable than the original =/.
Yes, I believe so, at least partially. And I think we can do it in a way that's more auditable than the way it's done in
It is possible.
That would be lovely =).
two-adicity can also be found in const fn, just keep dividing by two until there are no more factors left.
I think this one is a bit more tricky. |
51f2fc0
to
0ec1bde
Compare
I see, I think there is some work to allow mutable parameters. However, the situation is not as bad as you describe. One can simply write Although it's not so important atm, my personal take going forward is that we should reduce reliance on macros, so it could be future work once we move to a |
Unfortunately, |
In the opposite direction then, I propose to rewrite the traits in terms of the |
This doesn't work when
I would rather just wait until |
Fair enough, let's consolidate into an issue |
@Pratyush how do you feel about using proc macros to perform all constant evaluation instead? I feel much error-prone copy pasta can be avoided as desired with little downside, while also circumventing the need to wait on const generics or const trait impl, which no one knows when will actually land. Since the macros would be invoked in curves, and most methods concern biginteger, it does seem that the dependencies are not an issue. |
This is the approach |
Hmm, can't one do as per your proc macro in this PR and parse a &[u64] etc? How is this not accessing the value? |
What I meant is that it's not possible to access the value when you're given just the identifier. For example, in |
I don't think you're right about this, this is certainly true of ordinary macros, but in your Since recreating this functionality is complicated, it seems more straightforward to create functionality to translate between num_bigint BigInts and our BigIntegers, perhaps strictly for the proc macro crate internal use. I think that as long as we can tweak num_bigint to build with BigDigit = u64 all the time, this would be perfectly fine to build on any target (otherwise this translation would be hairy for 32-bit targets.) If not, one can simply check if BigDigit is u32 or u64 and conditionally make the conversion. (one can also just use Or, better still, one could work directly with the BigInt algebra to derive the params. |
In conclusion, I believe utilising |
Right, in this case I explicitly provide the token "1234555555" (for example) to the macro, so the macro can parse it and can manipulate a string "123455555".
I'm not sure I understand what you mean here; for use inside the proc macro implementation, we don't care about the internal representation of |
Well sure, but what I'm saying is that one can access the u64 values and generate other parameters from them entirely within the proc macros, without relying on const fn. However, one does have to redefine some functionality, such as Fp mul_assign etc. Given that one has to do so for either path, now I'm unsure which approach is better. |
To clarify, I agree that this approach works when generating parameters for prime fields. It does not work when generating parameters for other things (elliptic curves, extension fields, etc) unless you explicitly pass the value of the underlying prime modulus to the macro. |
It would certainly have been easier to add the Pasta curves (#21) with this improvement. In particular it wasn't entirely clear which constants needed to be in Montgomery form, since there's no naming convention that distinguishes Montgomery vs plain representations. |
Yeah sorry about that 😅 (Those frustrations were precisely what motivated this PR) |
@@ -115,6 +116,117 @@ macro_rules! impl_Fp { | |||
pub const fn new(element: $BigIntegerType) -> Self { | |||
Self(element, PhantomData) | |||
} | |||
|
|||
#[ark_ff_asm::unroll_for_loops] | |||
const fn const_is_zero(&self) -> bool { |
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.
Should all changes to field arithmetic now be duplicated in these macros?
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.
Hmm I don't anticipate these methods will change much, but if you're asking whether we should have a comment on the structs indicating that both copies should be updated if either one is, then yes. Longer term, the goal would be to deduplicate the code, and just use one impl in both const and non-const contexts.
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.
Cool! Can you add a comment to that affect for both structs
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.
Will do
(maybe I'll even do a partial deduplication now)
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 did a partial deduplication, so that common logic is stored in a separate method that both the const
and non-const
versions invoke.
54006d7
to
1b54f42
Compare
Compile time Montgomery multiplication!
1b54f42
to
b652024
Compare
field_new
macro to take in signed integers
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.
This LGTM.
So far, the
field_new!
macro is quite unintuitive: it requires users to write prime field elements in Montgomery form already, which usually requires a detour through a sage script. This PR adds a feature for converting to Montgomery form at compile time, which makes it possible to specify constants in normal form, and have these be automatically converted.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
Files changed
in the Github PR explorer