feat(std_lib)!: modulus bits and modulus bytes methods

[vezenovm]

Related issue(s)

Resolves #549

Description

Adds two builtin functions, modulus_bits and modulus_be_byte_array. modulus_bits returns the number of bits in the modulus while modulus_be_byte_array returns the modulus as a byte array.

Summary of changes

We can evaluate the modulus at callsite so I simply added some logic to try_evaluate_call (where we evaluate the arraylen builtin).

I am considering to instead change modulus_bits to return a bit array of the modulus. Then if a user wants the number of bits in the modulus they can just take the array len of this bit array.

Dependency additions / changes

Test additions / changes

I added a test that calls both methods and tests their outputs.

Checklist

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt with default settings.
• I have linked this PR to the issue(s) that it resolves.
• I have reviewed the changes on GitHub, line by line.
• I have ensured all changes are covered in the description.

Additional context

Reference issue for some further discussion about the best array types for returning the modulus

[guipublic]

I think this could be simplified, once we have big integers, into a simple fn field_characteristic() ->BigInt method in the std::lib. Then one would use to_bits on the result if he needs the bits.
In order to minimise backward compatibility, should we wait for the big int first?

[vezenovm]

I think this could be simplified, once we have big integers

I had thought of something similar and mentioned it in the issue #549. Are you worried that users might use these modulus methods and when we introduce BigInt and change the methods they would have to update their circuits?

modulus_bits right now returns the exact bit size of the modulus. I was considering to change this to returning a bit array and then having a separate methods modulus_num_bits that called array len on the bit array. We could probably make modulus_bits and modulus_be_byte_array backwards compatible in this case still. Once we have big ints and add a method like fn field_characteristic() ->BigInt, field_characteristic would be our only builtin method. modulus_bits and modulus_be_byte_array would then use BigInt::to_bits and BigInt::to_bytes respectively on the result of calling field_characteristic(). It would be a similar update to how we switched to the to_radix builtin without having to change the to_bits and to_bytes methods.

[jfecher]

What is the reasoning behind the byte array being big endian? Since to_le_bytes exists I think it'd be easier for users if everything was little endian (or the reverse).

[guipublic]

Are you worried that users might use these modulus methods and when we introduce BigInt and change the methods they would have to update their circuits?

Yes, but I agree it will be either a small change for them or we can make it transparent easily.

[vezenovm]

What is the reasoning behind the byte array being big endian? Since to_le_bytes exists I think it'd be easier for users if everything was little endian (or the reverse).

I was mainly trying to follow the discussion in the issue. We also currently have an issue up in acvm about updating to both to_be_bytes and to_le_bytes. Perhaps it would be good to just do the same here and have modulus_be_byte_array and modulus_le_byte_array both exposed.

[vezenovm]

@guipublic @jfecher @kevaundray What do we think about also moving these methods to a std::field module? We are starting to have more methods to interact with the native field and as we add more it would probably be good to move it out of the main lib.nr file.

[jfecher]

I'd be for moving it to std::field::*. I'd like to also enable methods for builtin types so we can have array.len() and these methods could be called as methods as well.

[vezenovm]

So once we have a native BigInt we can make a new builtin simply for fn modulus() -> BigInt. The native BigInt type should then enable to_le_bits, to_be_bits, to_be_bytes, to_be_bytes which can be called inside the respective modulus methods added in this PR.

[jfecher]

Is std::array::len still failing on the returned arrays of modulus_array_literal?

[vezenovm]

Is std::array::len still failing on the returned arrays of modulus_array_literal?

modulus_array_literal is the method I added when evaluating the modulus builtin funcs in monomorphisation. Do you mean is std::array::len failing when calling these builtin funcs in the std lib? If so, then yes it is failing. It panics at this line let len = typ.array_length().unwrap(); when evaluating the arraylen opcode as the Type of the array returned from those funcs is an unbound NamedGeneric.

[vezenovm]

I renamed all the functions to have the endian specification trailing the function name (ex. to_bytes_le instead of to_le_bytes). I think this makes more sense intuitively and this follows the syntax of existing big integer libraries (the num_bigint crate and the arkworks BigInteger class both follow this naming). You can see this in this PR by an external contributor as well: noir-lang/acvm#58

I held off on implementing to_bits_be just yet, as we probably need to adapt to_radix into to_radix_be and to_radix_le for this to be accurate. I thought I could reverse the outputs of the current to_radix function when evaluating the opcode in acir_gen, however, for to_bits we let a user decompose a specified number of bits of a field but we return a u1 array of the max num bits of the field (254 for bn254). For little-endian this is fine as the trailing zeros don't have any value. But for big endian this would give an incorrect result. I think this would be best scoped in a separate PR.

[Ethan-000]

mentioned naming these to @kevaundray the other day. It seems rust is using to_le_bytes for example. may be a discussion on this. cc @kevaundray

[vezenovm]

It seems rust is using to_le_bytes for example.

I switched back to use to_le_bytes to match Rust. I made the modulus methods consistent with this naming as well