feat(blockifier): add Secp256r1 cairo native syscalls

[PearsonWhite]

Currently waiting for other changes before proceeding with this.

This is currently a Draft. It requires some other Secp logic that will come in another issue. Then, this branch will be rebased on top of it and the overlapping logic will be removed.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.804 ms 33.845 ms 33.893 ms]
change: [-5.0449% -3.5452% -2.2315%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
4 (4.00%) high mild
5 (5.00%) high severe

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.89%. Comparing base (e3165c4) to head (3d40fd2).
Report is 543 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1675       +/-   ##
===========================================
+ Coverage   40.10%   68.89%   +28.79%     
===========================================
  Files          26      109       +83     
  Lines        1895    13899    +12004     
  Branches     1895    13899    +12004     
===========================================
+ Hits          760     9576     +8816     
- Misses       1100     3914     +2814     
- Partials       35      409      +374     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[avi-starkware]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

---

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

Previously, xrvdg (Xander van der Goot) wrote…

Based on a quick scan the conversion don't do any signed/unsigned conversion, just like Pearson said. Still I would be in favour of changing u256_to_biguint to u256_to_bigint, that should only require using bigint's constructor. There was no reason to use the biguint over the bigint, I overlooked this.

Okay, I understand. So the type for Secp256Point coordinates is supposed to be BigInt<4> according to the definition of the curve.
Two further comments:

1. I think we shouldn't go through BigUint. To get Secp256r1Point from Secp256Point, our current conversion is U256 -> BigUint -> BigInt<4>. We should do a direct conversion, as Xander suggested.
2. Is it good practice to use functions from starknet_stub.rs? It is a module containing a mock syscall handler for tests. It might not be the most well-maintained module. These helper functions are very short. Why not have a version of them in our repo? WDYT @Yoni-Starkware @meship-starkware ?

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 32 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Note to self: this is the only syscalls that is affected by the builtins gas cost, meaning that the fact that this gas cost is right means that the builtins costs are right :)

Note that they are not the same though. Native costs 10,000 more.
Even if they were the same, the fact that this gas cost is correct doesn't necessarily mean that the builtin costs are correct. It just means that our current tests don't catch any mistakes.

[github-actions]

Artifacts upload triggered. View details here

[PearsonWhite]

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

---

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

Previously, avi-starkware wrote…

Okay, I understand. So the type for Secp256Point coordinates is supposed to be BigInt<4> according to the definition of the curve.
Two further comments:

1. I think we shouldn't go through BigUint. To get Secp256r1Point from Secp256Point, our current conversion is U256 -> BigUint -> BigInt<4>. We should do a direct conversion, as Xander suggested.
2. Is it good practice to use functions from starknet_stub.rs? It is a module containing a mock syscall handler for tests. It might not be the most well-maintained module. These helper functions are very short. Why not have a version of them in our repo? WDYT @Yoni-Starkware @meship-starkware ?

Added u256_to_big4int and used that.

[Yoni-Starkware]

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

---

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

Previously, PearsonWhite wrote…

Added u256_to_big4int and used that.

As Avi said, can we please avoid using starknet_stub.rs?

[PearsonWhite]

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

---

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

Previously, Yoni-Starkware (Yoni) wrote…

As Avi said, can we please avoid using starknet_stub.rs?

I moved the helpers into crates/blockifier/src/execution/native/syscall_handler.rs now.

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

Reviewed 1 of 2 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @PearsonWhite)

---

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

    (hi << 128) + lo
}

Not needed anymore, right?

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

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

---

crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs line 32 at r3 (raw file):

Previously, avi-starkware wrote…

Note that they are not the same though. Native costs 10,000 more.
Even if they were the same, the fact that this gas cost is correct doesn't necessarily mean that the builtin costs are correct. It just means that our current tests don't catch any mistakes.

The 10K is a known issue. We double charge on entry point

[meship-starkware]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @PearsonWhite)

[PearsonWhite]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @Yoni-Starkware)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Not needed anymore, right?

ark_ff uses BigUint in places like PrimeField, so I think we're better off keeping it in. Otherwise we will have a Uint in the form of BigInt<4>, which would be confusing and only exist for the purposes of avoiding the conversion.

[Yoni-Starkware]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware)

[meship-starkware]

Dismissed @avi-starkware from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PearsonWhite)

[meship-starkware]

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

[meship-starkware]

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

---

a discussion (no related file):
Please rebase and resolve the conflicts.

[avi-starkware]

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

[avi-starkware]

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

[Yoni-Starkware]

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

[avi-starkware]

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

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @PearsonWhite)