feat(blockifier): add secp logic

[xrvdg]

This PR adds the logic for secp in preparation of the native syscalls.

The code has been refactored such that the implementation is shared between the native and non-native syscalls.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

Attention: Patch coverage is 92.64706% with 5 lines in your changes missing coverage. Please review.

Project coverage is 69.35%. Comparing base (e3165c4) to head (c737b8b).
Report is 450 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/execution/secp.rs	93.22%	3 Missing and 1 partial ⚠️
crates/blockifier/src/execution/syscalls/secp.rs	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1813       +/-   ##
===========================================
+ Coverage   40.10%   69.35%   +29.24%     
===========================================
  Files          26      106       +80     
  Lines        1895    13798    +11903     
  Branches     1895    13798    +11903     
===========================================
+ Hits          760     9569     +8809     
- Misses       1100     3827     +2727     
- Partials       35      402      +367     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 1 of 1 files at r1, 1 of 4 files at r6, all commit messages.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @xrvdg)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 436 at r1 (raw file):

    use cairo_native::starknet::U256;

    use super::Secp256Point;

Please avoid using super

Code quote:

use super::Secp256Point;

---

crates/blockifier/src/execution/native/syscall_handler.rs line 450 at r1 (raw file):

        assert_eq!(p1, Secp256Point::add(p1, p2));
    }
}

Can we move these tests to the secp test? I am not sure this is the best solution, but I don't like the tests to be part of the syscall handler file.

Code quote:

#[cfg(test)]
mod test {
    use cairo_native::starknet::U256;

    use super::Secp256Point;

    #[test]
    fn infinity_test() {
        let p1 =
            Secp256Point::<ark_secp256k1::Config>::get_point_from_x(U256 { lo: 1, hi: 0 }, false)
                .unwrap()
                .unwrap();

        let p2 = Secp256Point::mul(p1, U256 { lo: 0, hi: 0 });
        assert!(p2.0.infinity);

        assert_eq!(p1, Secp256Point::add(p1, p2));
    }
}

[meship-starkware]

Reviewed 3 of 4 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @xrvdg)

[xrvdg]

crates/blockifier/src/execution/native/syscall_handler.rs line 450 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Can we move these tests to the secp test? I am not sure this is the best solution, but I don't like the tests to be part of the syscall handler file.

These can be moved to somewhere else, but that would require making Secp256Point public.

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @xrvdg)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 450 at r1 (raw file):

Previously, xrvdg (Xander van der Goot) wrote…

These can be moved to somewhere else, but that would require making Secp256Point public.

I see. Then I think it is better to keep it as is

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @xrvdg)

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 436 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Please avoid using super

Done

[avi-starkware]

crates/blockifier/src/execution/syscalls/secp.rs line 40 at r13 (raw file):

    pub fn secp_mul(&mut self, request: SecpMulRequest) -> SyscallResult<SecpMulResponse> {
        let ep_point = self.get_point_by_id(request.ec_point_id)?;
        let result = *ep_point * Curve::ScalarField::from(request.multiplier);

@ilyalesokhin-starkware
?

Suggestion:

        let ec_point = self.get_point_by_id(request.ec_point_id)?;
        let result = *ec_point * Curve::ScalarField::from(request.multiplier);

[avi-starkware]

Reviewed 2 of 4 files at r6, 1 of 1 files at r13.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1, @xrvdg, and @Yoni-Starkware)

[avi-starkware]

Reviewed 1 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1, @xrvdg, and @Yoni-Starkware)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 7 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @xrvdg)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 673 at r15 (raw file):

            Ok(None) => Ok(None),
            Ok(Some(point)) => Ok(Some(Secp256Point(point))),
            Err(error) => Err(error),

Please share this code

Code quote:

            Ok(None) => Ok(None),
            Ok(Some(point)) => Ok(Some(Secp256Point(point))),
            Err(error) => Err(error),

---

crates/blockifier/src/execution/native/syscall_handler.rs line 679 at r15 (raw file):

/// Data structure to tie together k1 and r1 points to it's corresponding
/// `Affine<Curve>`

Can you rephrase? It's not clear

Code quote:

/// Data structure to tie together k1 and r1 points to it's corresponding
/// `Affine<Curve>`

---

crates/blockifier/src/execution/native/syscall_handler.rs line 681 at r15 (raw file):

/// `Affine<Curve>`
#[derive(PartialEq, Clone, Copy)]
struct Secp256Point<Curve: SWCurveConfig>(Affine<Curve>);

Please place the struct above the impl blocks

Code quote:

struct Secp256Point<Curve: SWCurveConfig>(Affine<Curve>);

---

crates/blockifier/src/execution/syscalls/secp.rs line 63 at r15 (raw file):

    pub fn secp_new(&mut self, request: SecpNewRequest) -> SyscallResult<SecpNewResponse> {
        let affine = new_affine::<Curve>(request.x, request.y);

Why not using ??

Suggestion:

let affine = new_affine::<Curve>(request.x, request.y)?;

---

crates/blockifier/src/execution/syscalls/secp.rs line 94 at r15 (raw file):

        }
        let ec_point = if x.is_zero() && y.is_zero() {
            short_weierstrass::Affine::<Curve>::identity()

What about this case?

Code quote:

        let ec_point = if x.is_zero() && y.is_zero() {
            short_weierstrass::Affine::<Curve>::identity()

[varex83]

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, @xrvdg, and @Yoni-Starkware)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 673 at r15 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please share this code

Done

---

crates/blockifier/src/execution/native/syscall_handler.rs line 679 at r15 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Can you rephrase? It's not clear

Done

---

crates/blockifier/src/execution/native/syscall_handler.rs line 681 at r15 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please place the struct above the impl blocks

Done

---

crates/blockifier/src/execution/syscalls/secp.rs line 40 at r13 (raw file):

Previously, avi-starkware wrote…

@ilyalesokhin-starkware
?

Done

---

crates/blockifier/src/execution/syscalls/secp.rs line 63 at r15 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why not using ??

Since we return the same type, and for me, it's easier to just map instead of unwrapping and wrapping into Ok(..)

---

crates/blockifier/src/execution/syscalls/secp.rs line 94 at r15 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

What about this case?

It's moved to here:

Code snippet:

fn maybe_affine<Curve: SWCurveConfig>(
    x: Curve::BaseField,
    y: Curve::BaseField,
) -> Option<Affine<Curve>> {
    let ec_point = if x.is_zero() && y.is_zero() {
        Affine::<Curve>::identity()
    } else {
        Affine::<Curve>::new_unchecked(x, y)
    };

    if ec_point.is_on_curve() && ec_point.is_in_correct_subgroup_assuming_on_curve() {
        Some(ec_point)
    } else {
        None
    }
}

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/execution/syscalls/secp.rs line 63 at r15 (raw file):

Previously, varex83 (Bohdan Ohorodnii) wrote…

Since we return the same type, and for me, it's easier to just map instead of unwrapping and wrapping into Ok(..)

Done

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

Reviewed 1 of 4 files at r6, 1 of 3 files at r16, 1 of 1 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @xrvdg)

---

crates/blockifier/src/execution/secp.rs line 20 at r18 (raw file):

        .map(|(smaller, greater)| {
            // Return the correct y coordinate based on the parity.
            if ark_ff::BigInteger::is_odd(&smaller.into_bigint()) == y_parity {

Why not keep it as is?

Suggestion:

smaller.into_bigint().is_odd()

[meship-starkware]

Reviewed 1 of 3 files at r16, 1 of 1 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @xrvdg)

[meship-starkware]

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

[Yoni-Starkware]

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