feat: Big Integer implementation #495

qalisander · 2025-01-15T11:14:56Z

Resolves #481

PR Checklist

Tests
Documentation
Changelog

…/biginteger

qalisander · 2025-01-29T09:11:32Z

lib/crypto/src/arithmetic/uint.rs

+        !self.ct_is_odd()
+    }
+
+    const fn ct_geq(&self, rhs: &Self) -> bool {


The whole point of ct_* prefix is to distinguish constant function from runtime function.
E.g. we can have geq for runtime and ct_geq for compile time.

Also same convention applied to ct_for! macro, that adds convenience of for but in const context.
p.s. for cycle is not available in const context

bidzyyys

Great progress, left some comments.

benches/src/report.rs

examples/oz-crypto/src/lib.rs

lib/crypto/src/arithmetic/limb.rs

lib/crypto/src/arithmetic/mod.rs

bidzyyys · 2025-01-29T09:04:23Z

lib/crypto/src/field/fp.rs

+            let (_, mut carry) = arithmetic::limb::mac(lo.limbs[i], tmp, P::MODULUS.limbs[0]);
+
+            ct_for_unroll6!((j in 1..N) {
+                let k = i + j;


Nit: I do not like k, i, l for loop iterations, prefer more self-descriptive names to not mess them.

In this (complex) case I have to agree

lib/crypto/src/arithmetic/uint.rs

bidzyyys · 2025-01-29T09:10:12Z

lib/crypto/src/arithmetic/uint.rs

+        ct_for!((i in 0..N) {
+            let a = self.limbs[N - i - 1];
+            let b = rhs.limbs[N - i - 1];
+            if a < b {


if a != b { return a > b }
I believe it reduces some condition calculation in if

I'm not sure this is more optimal 🤔 It requires 2 comparisons every time, whereas the current impl can sometimes be only 1 comparison, so it should be more performant in general.
We'd have to measure to know for sure.

bidzyyys · 2025-01-29T09:11:19Z

lib/crypto/src/arithmetic/uint.rs

+    #[must_use]
+    pub const fn ct_ge(&self, rhs: &Self) -> bool {
+        ct_for!((i in 0..N) {
+            if self.limbs[i] < rhs.limbs[i] {


if a != b { return a > b }
I believe it reduces some condition calculation in if

Actually I like your way:) Thnx!

bidzyyys · 2025-01-29T09:12:20Z

lib/crypto/src/arithmetic/uint.rs

+                break;
+            }
+
+            if index == 0 {


Nit: I would merge this condition with the previous if.

qalisander · 2025-01-29T10:06:11Z

lib/crypto/src/field/fp.rs

+    /// reduction for efficient implementation.
+    #[inline(always)]
+    fn mul_assign(a: &mut Fp<Self, N>, b: &Fp<Self, N>) {
+        // Implements CIOS.


Attach link

qalisander · 2025-01-29T10:38:55Z

lib/crypto/src/arithmetic/uint.rs

+    #[inline]
+    fn cmp(&self, rhs: &Self) -> Ordering {
+        ct_for_unroll6!((i in 0..N) {
+            let a = &self.limbs[N - i - 1];


Add reversed const for cycle

bidzyyys

Only few comments left.

Please:

update CHANGELOG.md
add GH Issue describing how we should add prop tests and fuzzing to this Big Integer implementation

bidzyyys · 2025-01-29T17:13:09Z

benches/src/PoseidonT3_asm.sol

+/// SPDX-License-Identifier: MIT
+pragma solidity >=0.7.0;
+
+library PoseidonT3 {


I believe you should mention the author/repository of this Solidity implementation.

bidzyyys · 2025-01-29T17:15:20Z

benches/src/PoseidonT3_asm.sol

Do we need this file as it is the same as in poseidon_asm_sol.rs bench?

benches/src/report.rs

lib/crypto/src/arithmetic/limb.rs

bidzyyys · 2025-01-29T17:25:18Z

lib/crypto/src/arithmetic/uint.rs

+    }
+}
+
+// TODO#q: Document ShrAssign and ShlAssign implementations


0xNeshi

My one big critique is terse/missing doc comments.

I think for the sake of our future selves, and for the sake of our audit team, we should have clear doc comments for each functions.

This is especially important if we plan to make this bigint implementation into a separate crate that we want to share with the world

0xNeshi · 2025-01-29T18:43:58Z

lib/crypto/src/arithmetic/uint.rs

+        true
+    }
+
+    /// Compute a right shift of `self`


Suggested change

/// Compute a right shift of `self`

/// Compute a right shift of `self`.

0xNeshi · 2025-01-29T18:45:50Z

lib/crypto/src/arithmetic/uint.rs

+            let leading = self.limbs[index].leading_zeros() as usize;
+            num_bits -= leading;
+
+            // If the limb is not empty, stop processing other limbs,


Suggested change

// If the limb is not empty, stop processing other limbs,

// If the limb is not empty, stop processing other limbs.

other comments use a dot (comma makes little sense anyway).

0xNeshi · 2025-01-29T19:20:15Z

lib/crypto/src/arithmetic/uint.rs

+        (self.limbs[limb] & mask) != 0
+    }
+
+    /// Multiply `self` by `2`, assign the result to `self` and return


Suggested change

/// Multiply `self` by `2`, assign the result to `self` and return

/// Multiplies `self` by `2` in-place, returning whether overflow occurred.

///

/// Returns `true` if the operation overflowed, `false` otherwise.

Wdyt about this comment style for all functions? Seems a bit clearer to me

0xNeshi · 2025-01-29T19:21:54Z

lib/crypto/src/arithmetic/uint.rs

+    }
+
+    #[inline]
+    pub(crate) const fn ct_checked_sub(mut self, rhs: &Self) -> (Self, bool) {


Missing docs. Even though this is internal to the crate, we as maintainers will need to know what each function does, especially months/years from now

0xNeshi · 2025-01-29T19:22:22Z

lib/crypto/src/arithmetic/uint.rs

+    #[inline]
+    #[allow(dead_code)]
+    pub(crate) const fn ct_checked_add(mut self, rhs: &Self) -> (Self, bool) {


Why do we need this if it isn't used?

0xNeshi · 2025-01-29T20:01:23Z

lib/crypto/src/field/fp.rs

    /// Compute `a^{-1}` if `a` is not zero.
+    ///
+    /// Guajardo, Kumar, Paar, Pelzl.
+    /// Efficient Software-Implementation of Finite Fields with Applications to
+    /// Cryptography [reference].
+    /// Algorithm 16 (BEA for Inversion in Fp).
+    ///
+    /// [reference]: https://www.sandeep.de/my/papers/2006_ActaApplMath_EfficientSoftFiniteF.pdf


The reference looks weird as-is (like it's part of text).
This is what Claude spit out, and it looks really clean:

Suggested change

/// Compute `a^{-1}` if `a` is not zero.

///

/// Guajardo, Kumar, Paar, Pelzl.

/// Efficient Software-Implementation of Finite Fields with Applications to

/// Cryptography [reference].

/// Algorithm 16 (BEA for Inversion in Fp).

///

/// [reference]: https://www.sandeep.de/my/papers/2006_ActaApplMath_EfficientSoftFiniteF.pdf

/// Computes the multiplicative inverse `a^{-1}` if `a` is non-zero.

///

/// Uses the Binary Extended Algorithm (BEA) for inversion in Fp as

/// described in [Guajardo et al., 2006][paper].

///

/// [paper]: https://www.sandeep.de/my/papers/2006_ActaApplMath_EfficientSoftFiniteF.pdf "Efficient Software-Implementation of Finite Fields with Applications to Cryptography"

It added paper title as hover text in the link definition

We could really utilize LLMs for comments too, they generally create very clean & clear docs

0xNeshi · 2025-01-29T20:09:02Z

lib/crypto/src/field/fp.rs

    }
 }

+/// Compute `-M^{-1} mod 2^64`.


This is a very terse doc comment.

See LLM's suggestion

Suggested change

/// Compute `-M^{-1} mod 2^64`.

/// Computes the Montgomery modular inverse `-MODULUS^{-1} mod 2^64` for the field modulus.

///

/// This is a constant function that computes the modular multiplicative inverse of

/// the negative field modulus, modulo 2^64. This value is crucial for Montgomery arithmetic

/// operations in the field.

///

/// The calculation uses the fact that for a 64-bit value:

/// - We only need the lowest limb of the modulus

/// - The Euler totient φ(2^64) = 2^63

/// - Therefore we can compute the inverse by raising to power (2^63 - 1)

I think the audit team will thank us

0xNeshi · 2025-01-29T20:12:24Z

lib/crypto/src/field/fp.rs

+            let (_, mut carry) = arithmetic::limb::mac(lo.limbs[i], tmp, P::MODULUS.limbs[0]);
+
+            ct_for_unroll6!((j in 1..N) {
+                let k = i + j;


In this (complex) case I have to agree

0xNeshi · 2025-01-29T20:12:56Z

lib/crypto/src/field/fp.rs

+    /// Algorithm 14.32 in Handbook of Applied Cryptography [reference].
+    ///
+    /// [reference]: https://cacr.uwaterloo.ca/hac/about/chap14.pdf


See previous comment on how to link to a paper in a cleaner way

0xNeshi · 2025-01-29T20:17:08Z

lib/crypto/src/poseidon2/mod.rs

@@ -42,6 +42,7 @@ impl<P: PoseidonParams<F>, F: PrimeField> Default for Poseidon2<P, F> {
 impl<P: PoseidonParams<F>, F: PrimeField> Poseidon2<P, F> {
    /// Create a new Poseidon sponge.
    #[must_use]
+    #[inline]


Inlining everything is the way to reducing gas cost in all cases? 🤔
I just noticed that many lib/crypto functions are inlined, but not all. Shouldn't we then be inlining everything?

Did you measure the impact of not using #[inline]?
Shouldn't link-time optimizations perform the same optimization? Stylus projects have this option ON by default, and so do we.

qalisander added 30 commits January 9, 2025 19:27

++

2974aad

++

7b8a8ba

++

9cc5f49

++

69d753d

++

63011f9

++

9b82dd5

++

8df14c0

++

b5dff65

++

5c988ca

++

f68e8e1

++

0d9e3be

++

286b3c2

++

474d82d

++

306025e

++

26af3ac

++

3f86a87

poseidon bn256 works

d44383f

++

8d29263

add loops unrolls

f8ca316

++

815553b

++

e5885bf

++

c64a245

++

d95cbdd

++

9794a28

++

90cc32a

++

1929652

biginteger ff multiplication benchmark

bbaa102

biginteger ff multiplication benchmark

79cebd1

++

3a65c47

++

6f99e1b

qalisander added 7 commits January 29, 2025 11:18

++

3173def

++

9b1ff17

++

530c126

++

e4f936c

++

1d9d571

Merge remote-tracking branch 'origin/main' into benchmark-experiments…

27b8e6d

…/biginteger

++

dc3a44d

bidzyyys self-requested a review January 29, 2025 08:33

qalisander added 2 commits January 29, 2025 12:42

soothe clippy

d5f0db5

drop oz_crypto example

a135ab4

qalisander requested a review from 0xNeshi January 29, 2025 08:57

++

e1bcf53

qalisander commented Jan 29, 2025

View reviewed changes

bidzyyys reviewed Jan 29, 2025

View reviewed changes

qalisander added 6 commits January 29, 2025 13:18

++

e94dda8

++

86ae9d2

rename Example -> Examples

d2e4212

review fixes

5601466

clippy fixes

207a8e2

don't use !ct_is_odd for possible optimization reasons

19a031d

qalisander commented Jan 29, 2025

View reviewed changes

qalisander added 3 commits January 29, 2025 14:09

remove #[allow(unused_qualifications)]

528f667

++

300ae09

++

ddc792c

qalisander commented Jan 29, 2025

View reviewed changes

add docs

fb3c935

qalisander marked this pull request as ready for review January 29, 2025 13:08

bidzyyys reviewed Jan 29, 2025

View reviewed changes

bidzyyys changed the title ~~feat: biginteger implementation~~ feat: Big Integer implementation Jan 29, 2025

0xNeshi reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Big Integer implementation #495

feat: Big Integer implementation #495

qalisander commented Jan 15, 2025 •

edited

Loading

qalisander Jan 29, 2025

bidzyyys left a comment

bidzyyys Jan 29, 2025

0xNeshi Jan 29, 2025

bidzyyys Jan 29, 2025

0xNeshi Jan 29, 2025

bidzyyys Jan 29, 2025

qalisander Jan 29, 2025

bidzyyys Jan 29, 2025 •

edited

Loading

qalisander Jan 29, 2025

qalisander Jan 29, 2025 •

edited

Loading

bidzyyys left a comment

bidzyyys Jan 29, 2025

bidzyyys Jan 29, 2025

bidzyyys Jan 29, 2025

0xNeshi left a comment •

edited

Loading

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025

0xNeshi Jan 29, 2025 •

edited

Loading

0xNeshi Jan 29, 2025 •

edited

Loading

	/// Compute a right shift of `self`
	/// Compute a right shift of `self`.

	// If the limb is not empty, stop processing other limbs,
	// If the limb is not empty, stop processing other limbs.

-    /// Multiply `self` by `2`, assign the result to `self` and return
+    /// Multiplies `self` by `2` in-place, returning whether overflow occurred.
+    ///
+    /// Returns `true` if the operation overflowed, `false` otherwise.

-/// Compute `-M^{-1} mod 2^64`.
+/// Computes the Montgomery modular inverse `-MODULUS^{-1} mod 2^64` for the field modulus.
+///
+/// This is a constant function that computes the modular multiplicative inverse of
+/// the negative field modulus, modulo 2^64. This value is crucial for Montgomery arithmetic
+/// operations in the field.
+///
+/// The calculation uses the fact that for a 64-bit value:
+/// - We only need the lowest limb of the modulus
+/// - The Euler totient φ(2^64) = 2^63
+/// - Therefore we can compute the inverse by raising to power (2^63 - 1)

feat: Big Integer implementation #495

Are you sure you want to change the base?

feat: Big Integer implementation #495

Conversation

qalisander commented Jan 15, 2025 • edited Loading

PR Checklist

Choose a reason for hiding this comment

bidzyyys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bidzyyys Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qalisander Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

bidzyyys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xNeshi Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

0xNeshi Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

qalisander commented Jan 15, 2025 •

edited

Loading

bidzyyys Jan 29, 2025 •

edited

Loading

qalisander Jan 29, 2025 •

edited

Loading

0xNeshi left a comment •

edited

Loading

0xNeshi Jan 29, 2025 •

edited

Loading

0xNeshi Jan 29, 2025 •

edited

Loading