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

Added libfunc for u256_inv_mod_n. #3603

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Added libfunc for u256_inv_mod_n. #3603

merged 1 commit into from
Oct 4, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Jul 5, 2023

This change is Reviewable

@orizi orizi force-pushed the orizi/u256_div_mod_n branch 4 times, most recently from 2e95d3b to 6f30f2c Compare July 6, 2023 07:39
Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @orizi and @spapinistarkware)

a discussion (no related file):
Change PR title: div -> inv.


@orizi orizi changed the title Added libfunc for u256_div_mod_n. Added libfunc for u256_inv_mod_n. Jul 11, 2023
Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @liorgold2 and @spapinistarkware)

a discussion (no related file):

Previously, liorgold2 wrote…

Change PR title: div -> inv.

Done.


@orizi orizi force-pushed the orizi/u256_div_mod_n branch 2 times, most recently from 30a584a to 6a42e6f Compare July 23, 2023 07:36
Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @orizi and @spapinistarkware)

a discussion (no related file):
Shouldn't the target branch be sierra-minor-update?


Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @spapinistarkware)

a discussion (no related file):

Previously, liorgold2 wrote…

Shouldn't the target branch be sierra-minor-update?

no - since not in the allowed list.


Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/test/math_test.cairo line 98 at r2 (raw file):

        .unwrap();
    assert(q == 0, '0 / 3 != 0 (13)');
    assert(

Missing tests (e.g., random 256-bit numbers)

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @liorgold2 and @spapinistarkware)


corelib/src/test/math_test.cairo line 98 at r2 (raw file):

Previously, liorgold2 wrote…

Missing tests (e.g., random 256-bit numbers)

Done.

@orizi orizi force-pushed the orizi/u256_div_mod_n branch 2 times, most recently from 602957b to 1089cd1 Compare August 24, 2023 14:26
Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 10 files at r3.
Reviewable status: 2 of 10 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 10 files at r3, all commit messages.
Reviewable status: 4 of 10 files reviewed, 3 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/test/math_test.cairo line 98 at r2 (raw file):

Previously, orizi wrote…

Done.

Still missing.

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 10 files reviewed, 3 unresolved discussions (waiting on @liorgold2 and @spapinistarkware)


corelib/src/test/math_test.cairo line 98 at r2 (raw file):

Previously, liorgold2 wrote…

Still missing.

somehow got lost - readded.

@orizi orizi force-pushed the orizi/u256_div_mod_n branch 2 times, most recently from 1974acb to f281153 Compare October 1, 2023 14:06
Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r4, all commit messages.
Reviewable status: 3 of 10 files reviewed, 5 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/math.cairo line 81 at r4 (raw file):

/// Returns `1 / b (mod n)`, or None if `b` is not invertible modulo `n`.
/// Additionally returns several `U128MulGuarantee`s that are required for validating the calculation.

Line too long.


corelib/src/math.cairo line 100 at r4 (raw file):

/// Returns `a / b (mod n)`, or None if `b` is not invertible modulo `n`.
fn u256_div_mod_n(a: u256, b: NonZero<u256>, n: NonZero<u256>) -> Option<u256> {

Can you split u256_inv_mod_n out of this function?

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 10 files reviewed, 6 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/test/math_test.cairo line 2 at r4 (raw file):

/// Helper for making a non-zero value.
fn nz<N, impl TryIntoNonZeroN: TryInto<N, NonZero<N>>>(n: N) -> NonZero<N> {

Use + syntax.

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 10 files at r3, 1 of 6 files at r4.
Reviewable status: 6 of 10 files reviewed, 6 unresolved discussions (waiting on @orizi and @spapinistarkware)

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 10 files reviewed, 6 unresolved discussions (waiting on @liorgold2 and @spapinistarkware)


corelib/src/math.cairo line 81 at r4 (raw file):

Previously, liorgold2 wrote…

Line too long.

Done.


corelib/src/math.cairo line 100 at r4 (raw file):

Previously, liorgold2 wrote…

Can you split u256_inv_mod_n out of this function?

Done.


corelib/src/test/math_test.cairo line 2 at r4 (raw file):

Previously, liorgold2 wrote…

Use + syntax.

Done.

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 8 of 10 files reviewed, 9 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/math.cairo line 100 at r5 (raw file):

> implicits(RangeCheck) nopanic;

/// Returns the inverse of `a` modulo `n`, or None if `b` is not invertible modulo `n`.

.

Suggestion:

/// Returns the inverse of `a` modulo `n`, or None if `a` is not invertible modulo `n`.

corelib/src/test/math_test.cairo line 65 at r5 (raw file):

    assert(math::u256_div_mod_n(7, 2, nz(13)) == Option::Some(10), '7 / 2 != 10 (13)');
    assert(math::u256_div_mod_n(0, 3, nz(13)) == Option::Some(0), '0 / 3 != 0 (13)');
    assert(math::u256_div_mod_n(4, 3, nz(6)).is_none(), '4 / 3 == None (6)');

Add:

assert(math::u256_div_mod_n(5, 4, nz(6)).is_none(), '4 / 3 == None (6)');

corelib/src/test/math_test.cairo line 101 at r5 (raw file):

        'Random large values 2'
    );
}

Add one with a large odd gcd (>2**128)


crates/cairo-lang-casm/src/hints/mod.rs line 240 at r5 (raw file):

    /// `g * t = n`
    ///
    /// In all cases - `name`0 is the least significant limb.

Suggestion:

    /// Provides the inverse of b (represented by 2 128-bit limbs) modulo n (represented by 2 128-bit
    /// limbs), or a proof that b has no inverse.
    ///
    /// In case b has an inverse: Returns `r` and `k` such that:
    ///   * `r = 1 / b (mod n)`
    ///   * `k = (result * b - 1) / n`
    ///   * `g0_or_no_inv = 0`
    ///
    /// In case b has no inverse: Returns `g`, `s`, and `t`, such that:
    /// `g > 1`
    /// `g == 2 || g % 2 == 1` (in particular, `g0_or_no_inv = g0 != 0`)
    /// `g * s = b`
    /// `g * t = n`
    ///
    /// In all cases - `name`0 is the least significant limb.

crates/cairo-lang-casm/src/hints/mod.rs line 728 at r5 (raw file):

                        from starkware.python.math_utils import igcdex
    

Remove spaces.


crates/cairo-lang-casm/src/hints/mod.rs line 730 at r5 (raw file):

    
                        b = {b0} + {b1} * 2**128
                        n = {n0} + {n1} * 2**128

Suggestion:

                        b = {b0} + ({b1} << 128)
                        n = {n0} + ({n1} << 128)

crates/cairo-lang-casm/src/hints/mod.rs line 746 at r5 (raw file):

                        else:
                            r %= n
                            if r < 0:

This can't happen after the %, right?

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r4.
Reviewable status: 9 of 10 files reviewed, 10 unresolved discussions (waiting on @orizi and @spapinistarkware)


crates/cairo-lang-runner/src/casm_run/mod.rs line 1952 at r5 (raw file):

            } else {
                r %= &n;
                if r.is_negative() {

I guess this one (unlike python) is necessary?

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @orizi and @spapinistarkware)

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 10 files reviewed, 8 unresolved discussions (waiting on @liorgold2 and @spapinistarkware)


corelib/src/math.cairo line 100 at r5 (raw file):

Previously, liorgold2 wrote…

.

Done.


corelib/src/test/math_test.cairo line 65 at r5 (raw file):

Previously, liorgold2 wrote…

Add:

assert(math::u256_div_mod_n(5, 4, nz(6)).is_none(), '4 / 3 == None (6)');

Done.


corelib/src/test/math_test.cairo line 101 at r5 (raw file):

Previously, liorgold2 wrote…

Add one with a large odd gcd (>2**128)

Done.


crates/cairo-lang-casm/src/hints/mod.rs line 240 at r5 (raw file):

    /// `g * t = n`
    ///
    /// In all cases - `name`0 is the least significant limb.

Done.


crates/cairo-lang-casm/src/hints/mod.rs line 728 at r5 (raw file):

Previously, liorgold2 wrote…

Remove spaces.

Done.


crates/cairo-lang-casm/src/hints/mod.rs line 730 at r5 (raw file):

    
                        b = {b0} + {b1} * 2**128
                        n = {n0} + {n1} * 2**128

Done.


crates/cairo-lang-casm/src/hints/mod.rs line 746 at r5 (raw file):

Previously, liorgold2 wrote…

This can't happen after the %, right?

correct - fixed as this is python.


crates/cairo-lang-runner/src/casm_run/mod.rs line 1952 at r5 (raw file):

Previously, liorgold2 wrote…

I guess this one (unlike python) is necessary?

we have no promise from extended gcd to get a negative r - so this is required here (as rust is like c++ AFAIK)

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


corelib/src/test/math_test.cairo line 101 at r5 (raw file):

Previously, orizi wrote…

Done.

Add another test with b==n==256-bit odd number. This will test the branch where g1 is large.

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi and @spapinistarkware)

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @liorgold2 and @spapinistarkware)


corelib/src/test/math_test.cairo line 101 at r5 (raw file):

Previously, liorgold2 wrote…

Add another test with b==n==256-bit odd number. This will test the branch where g1 is large.

Done.

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

@orizi orizi added this pull request to the merge queue Oct 4, 2023
Merged via the queue into main with commit e9ce31e Oct 4, 2023
38 checks passed
@orizi orizi deleted the orizi/u256_div_mod_n branch October 10, 2023 11:10
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