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

Updated README for BigNum lib #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ewynx
Copy link
Contributor

@ewynx ewynx commented Dec 21, 2024

Description

Replacement of previously added documentation here as requested by @jzaki.

Problem*

Resolves

Summary*

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link

@jzaki jzaki left a comment

Choose a reason for hiding this comment

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

Just some minor nits for clarity, otherwise looks good

pub limbs: [Field; N],
}
```
- `N` is the number of limbs needed to represent the number. Each limb is a `Field`, which contains max 120 bits (a field has 254 bits)
Copy link

Choose a reason for hiding this comment

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

I found it confusing when reading "contains max 120 bits" and "has 254 bits".

Suggested change
- `N` is the number of limbs needed to represent the number. Each limb is a `Field`, which contains max 120 bits (a field has 254 bits)
- `N` is the number of limbs needed to represent the number. Each limb is at most 120 bits, stored in a `Field` (254 bits)

And if useful could add reasoning "enables limb multiplication without overflow" (if that is indeed the case)


## `RuntimeBigNum`
- `redc_param` is equal to `(1 << (2 * Params::modulus_bits())) / modulus` . This must be computed outside of the circuit and provided either as a private witness or hardcoded constant. (computing it via an unconstrained function would be very expensive until noir witness computation times improve)
Copy link

Choose a reason for hiding this comment

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

Did you mean "constrained"?

Suggested change
- `redc_param` is equal to `(1 << (2 * Params::modulus_bits())) / modulus` . This must be computed outside of the circuit and provided either as a private witness or hardcoded constant. (computing it via an unconstrained function would be very expensive until noir witness computation times improve)
- `redc_param` is equal to `(1 << (2 * Params::modulus_bits())) / modulus` . This must be computed outside of the circuit and provided either as a private witness or hardcoded constant. (computing it via a constrained function would be very expensive until noir witness computation times improve)

let one: My_RBN = RuntimeBigNum::from_array(params, [1, 0, 0]);
let two: My_RBN = RuntimeBigNum::from_array(params, [2, 0, 0]);
let three: My_RBN = RuntimeBigNum::from_array(params, [3, 0, 0]);
The methods that are available on the types `BigNum` and `RuntimeBigNum` are almost the same. Thsi section discusses the methods and talks about "bignum" to indicate both types.
Copy link

Choose a reason for hiding this comment

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

Suggested change
The methods that are available on the types `BigNum` and `RuntimeBigNum` are almost the same. Thsi section discusses the methods and talks about "bignum" to indicate both types.
The methods that are available on the types `BigNum` and `RuntimeBigNum` are almost the same. This section discusses these methods and uses "bignum" to indicate both types.


`modulus` represents the BigNum modulus, encoded as an array of `Field` elements that each encode 120 bits of the modulus. The first array element represents the least significant 120 bits.
## Custom or predefined parameter set
Copy link

Choose a reason for hiding this comment

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

Previously I've noticed "parameterset" as one word, and didn't know if it was intentional?


TODO: the paramgen tool is not up to date for `BigNum` version >= `0.4`, an issue has been created for this [here](https://github.com/noir-lang/noir-bignum-paramgen/issues/4). This section should be adjusted after this fix.

The easiest way to generate everything you need for a parameter set is to use [this tool](https://github.com/noir-lang/noir-bignum-paramgen).
Copy link

Choose a reason for hiding this comment

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

In most places I've seen "parameterset" as one word, is that intentional? In a couple of places it is "parameter set".

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