Upgrade field_new macro to take in signed integers

[Pratyush]

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.

• Targeted PR against correct branch (master)
• Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
• Wrote unit tests
• Updated relevant documentation in the code
• Added a relevant changelog entry to the Pending section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

[ValarDragon]

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 ark-ff readme? This doesn't need to block this pr though

[Pratyush]

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)

[jon-chuang]

Is it possible to derive other parameters, like MODULUS_MINUS_ONE_DIV_TWO at compile time too?

I think this is just a bit shift?

[jon-chuang]

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 min_const_generics, which is pretty close to being stabilized? (rust-lang/rust#79135)?

Is there any real need for it? My guess is you want to deduplicate code across the const fn and non const fn uses?

In this case, I think one should convert all the original functions to const fn. As a const fn can ingest a non-constant result and output a non-const result. So it can play the dual purpose.

(this can be tested with the following snippet:

const fn f(x: u64) -> u64 {
    x + 5
}

let mut x = 5u64;
    
println!("{}", f(x));
    
x += 5;
    
println!("{}", f(x));

const Y: u64 = f(5u64);

println!("{}", Y);

)

One can also get rid of the duplication of code that is impl for FpParameters.

I'm not sure if const fn can be used to define const values generically in traits. (Unfortunately, I think not). If not, the dream of only needing to instantiate modulus without macros might be but a dream. But still, at least instantiating with a macro accepting only modulus would be possible where previously it was not.

---

Digression:
Possibly, it might even be possible to implement GLV purely from the parameters too. That would be pretty sweet.

I think most of the const fn functionality should reside in biginteger, we should add a div with remainder as well. This can possibly be used to compute R at compile-time too.

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

[Pratyush]

Is there any real need for it? My guess is you want to deduplicate code across the const fn and non const fn uses?

Yup!

In this case, I think one should convert all the original functions to const fn. As a const fn can ingest a non-constant result and output a non-const result. So it can play the dual purpose.

I did do this initially, but the problem right now is that const fns don't support mutable parameters, which means that I had to write the code as follows:

let (new_a, new_carry) = adc(a, b, carry);
a = new_a;
carry = new_carry;

This is much less readable than the original =/.

One can also get rid of the duplication of code that is impl for FpParameters.

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 ff_derive.

I'm not sure if const fn can be used to define const values generically in traits. (Unfortunately, I think not).

It is possible.

I think most of the const fn functionality should reside in biginteger, we should add a div with remainder as well. This can possibly be used to compute R at compile-time too.

That would be lovely =).

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.

two-adicity can also be found in const fn, just keep dividing by two until there are no more factors left.

Theoretically, one could also find square roots of unity and thus find the two-adic root of unity

I think this one is a bit more tricky. ff_derive does it by exponentiating by FpParameters::T, which might exhaust const_eval_limit. (I ran into this limit in a prior version of this PR).

[jon-chuang]

I did do this initially, but the problem right now is that const fns don't support mutable parameters...'

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 let (mut a, mut carry) = adc(a, b, carry);
LLVM will compile to same binary, I think.

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 min_const_generics impl.

[jon-chuang]

Unfortunately, impl const .. is still an incomplete feature (rust-lang/rfcs#2632, rust-lang/rust#67792, rust-lang/rust#71971), so there is no straightforward way to make neg() a const fn. Very sad.

[jon-chuang]

In the opposite direction then, I propose to rewrite the traits in terms of the const fn definitions. It's less than ideal but at least cleaner than having multiple definitions.

[Pratyush]

However, the situation is not as bad as you describe. One can simply write let (mut a, mut carry) = adc(a, b, carry);

This doesn't work when a is actually array[$i].

In the opposite direction then, I propose to rewrite the traits in terms of the const fn definitions. It's less than ideal but at least cleaner than having multiple definitions.

I would rather just wait until impl const Trait... is stable; it should hopefully be happening within the year. (Next steps are happening soon: rust-lang/rust#79287)

[jon-chuang]

Fair enough, let's consolidate into an issue

[jon-chuang]

@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.

[Pratyush]

This is the approach ff_derive takes, but it only does it for PrimeFields. We could certainly adapt that technique to expand it to other domains, but doing so is tricky because macros can't access values, which means they can't access things like the value of the modulus.

[jon-chuang]

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?

[Pratyush]

What I meant is that it's not possible to access the value when you're given just the identifier. For example, in cool_macro!(P::MODULUS), the only thing cool_macro receives are the tokens P::MODULUS; not the actual value of P::MODULUS. So cool_macro can only perform syntactic transformation on P::MODULUS.

[jon-chuang]

I don't think you're right about this, this is certainly true of ordinary macros, but in your to_sign_and_limbs macro, particularly in invoking BigInteger::from_str, one does get access to the values which can be used for computation.

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 to_u32_digits to get a vec).

Or, better still, one could work directly with the BigInt algebra to derive the params.

[jon-chuang]

In conclusion, I believe utilising const fn at this stage of the rustc stabilisation to be completely unecessary, and potentially introducing undesirable bloat.

[Pratyush]

I don't think you're right about this, this is certainly true of ordinary macros, but in your to_sign_and_limbs macro, particularly in invoking BigInteger::from_str, one does get access to the values which can be used for computation.

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".

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 to_u32_digits to get a vec).

Or, better still, one could work directly with the BigInt algebra to derive the params.

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 num_bigint::BigInt, just that it can output its internal representation into u64 limbs, which it can.

[jon-chuang]

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.

[Pratyush]

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.

[daira]

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.

[Pratyush]

Yeah sorry about that 😅

(Those frustrations were precisely what motivated this PR)

[ValarDragon]

Should all changes to field arithmetic now be duplicated in these macros?

[Pratyush]

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.

[ValarDragon]

Cool! Can you add a comment to that affect for both structs

[Pratyush]

Will do

(maybe I'll even do a partial deduplication now)

[Pratyush]

I did a partial deduplication, so that common logic is stored in a separate method that both the const and non-const versions invoke.

[ValarDragon]

This LGTM.