secp256r1 utils and is_valid_p256_signature #988

0xknwn · 2024-05-09T20:10:34Z

Fixes #987

PR Checklist

Tests
Documentation <- From what we see there is currently no documentation associated with the secp256k1 version of those utilities and we do not know where or how to add those. We would suggest leave it that way. Feel free to comment this PR.
Added entry to CHANGELOG.md
Tried the feature on a public network

0xknwn · 2024-05-09T23:11:04Z

We've deployed an account on sepolia that can validate P256 signature. If you check the Scarb.toml from 0xknwn/starknet-modular-account, you can see it is based on this branch for now and that is has both the secp256k1 and secp256r1 utils/store from you project

We have run a transaction on sepolia with that account:

the transaction hash on sepolia is 0x419d921d19fbb356ebe8f57be613b8d4d1880c5ad18768ba4e12baa566ec06
the transaction has succeeded from the receipt
the signature is

[
    "0xa1913d80b4b735ea849919d5636cb5e9",
    "0x98fd57ced24ecea3e19b87540589788d",
    "0x6a05a126fc09205b0a3a509ba0db9243",
    "0x3e75ae4643235e4dad268f0a480ef3f5",
    "0x0"
  ]

Now if you check this typescript program, you can see that,:

using the following private key: 0x1efecf7ee1e25bb87098baf2aaab0406167aae0d5ea9ba0d31404bf01886bd0e
on that message (i.e. the tx hash): 0x419d921d19fbb356ebe8f57be613b8d4d1880c5ad18768ba4e12baa566ec06
generates the following signature:
- r: 0x98fd57ced24ecea3e19b87540589788da1913d80b4b735ea849919d5636cb5e9
- s: 0x3e75ae4643235e4dad268f0a480ef3f56a05a126fc09205b0a3a509ba0db9243
which are 2 uint256 that you can format: [r.low, r.high, s.low, s.high] to get:

[
  "0xa1913d80b4b735ea849919d5636cb5e9",
  "0x98fd57ced24ecea3e19b87540589788d",
  "0x6a05a126fc09205b0a3a509ba0db9243",
  "0x3e75ae4643235e4dad268f0a480ef3f5"
]

So with OpenZeppelin/cairo-contracts with the store/utils for secp256r1 have allowed us to develop an account module that can validate a P256 signature. For the implementation, you can check the p256validator.cairo file in the project.

andrew-fleming · 2024-05-11T23:19:15Z

Thanks for opening this PR, @0xknwn! I think this will be an awesome addition to the lib. We'll take a look as soon as we can

andrew-fleming

Thanks for the PR, @0xknwn! This looks very good

I left a few comments and suggestions—the most pressing being that test_secp256r1 tests aren't running

src/account/interface.cairo

src/tests/account.cairo

src/tests/account/test_p256_account.cairo

src/tests/account/test_secp256r1.cairo

0xknwn · 2024-05-22T20:37:11Z

I will work on it tomorrow @andrew-fleming ! Thank you for the feedback and sorry if I did not include the tests.

FYI, this is the transaction that uses the code on Sepolia https://sepolia.starkscan.co/tx/0x419d921d19fbb356ebe8f57be613b8d4d1880c5ad18768ba4e12baa566ec06

andrew-fleming · 2024-05-22T20:53:31Z

@0xknwn it's my pleasure and thank you for opening the PR!

sorry if I did not include the tests.

No worries, this was easy to miss

0xknwn · 2024-05-23T10:47:25Z

@andrew-fleming I have done the following to follow up with your feedback:

rebase on main as it is right now (i.e. 0.13.0)
nitpick: remove the empty line
added test_secp256r1 in src/tests/account.cairo - obviously tests are failing, now! I am working on this...
remove the traits that are useless from src/tests/account/test_p256_account.cairo
removed the _u256 from src/tests/account/test_p256_account.cairo as you are right, it is not needed
I am working on the tests now: you are right, I have 5 failing and they are all failing with get_curve_size (as you've reproduced) on the following error:

Panicked with 0x4f7074696f6e3a3a756e77726170206661696c65642e ('Option::unwrap failed.').

~~I've also figured out that SIGNED_TX_DATA() is actually not referenced in the tests so is just here to document~~. I will let you know if I can get deeper into the get_curve_size issue.

0xknwn · 2024-05-23T15:16:12Z

@andrew-fleming ,

I did some additional investigations and I've now fixed the remaining issues, afaik:

the p256 curve size is returned correctly: I have added test_curve_size to show it is the case. As a matter of fact, it is hardcoded in the corelib.
secp256_ec_get_point_from_x_syscall returns None when used with curve_size as a private key. You would have to ask someone to understand why. TL;DR: (1) I have added test_get_point_from_x_syscall_on_curve_size_is_none to document that case and (2) if that is not the expected behaviour, the issue is in the corelib and not in your code anyway
I have added and used a sample private key to test the code remaining code. The fn P256_PRIVATEKEY_SAMPLE() helper function has been added into the code for that purpose. All the tests from test_secp256r1.cairo are now passing.

Hopefully I did not miss anything else. Thank you for your patience.

andrew-fleming

I left a final suggestion. Otherwise, LGTM!

andrew-fleming · 2024-05-28T04:16:57Z

src/tests/account/test_secp256r1.cairo

+#[test]
+fn test_curve_size() {
+ let curve_size = Secp256r1Impl::get_curve_size();
+ assert_eq!(curve_size, 0xffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551);
+ assert_eq!(curve_size.low, 0xbce6faada7179e84f3b9cac2fc632551);
+ assert_eq!(curve_size.high, 0xffffffff00000000ffffffffffffffff);
+}
+
+#[test]
+fn test_get_point_from_x_syscall_on_curve_size_is_none() {
+ let curve_size = Secp256r1Impl::get_curve_size();
+ match Secp256r1Impl::secp256_ec_get_point_from_x_syscall(curve_size, true).unwrap_syscall() {
+ Option::Some(_data) => { assert_eq!(true, false); },
+ Option::None => { assert_eq!(true, true); },
+ }
+
+ match Secp256r1Impl::secp256_ec_get_point_from_x_syscall(curve_size, false).unwrap_syscall() {
+ Option::Some(_data) => { assert_eq!(true, false); },
+ Option::None => { assert_eq!(true, true); },
+ }
+}


Using the curve size as x in secp256k1 appears to have been a coincidence. It's also outside the scope of this PR, so we don't need to include these two tests. I appreciate the effort in adding them though!

I have removed these 2 tests as requested.

andrew-fleming

All good on my end!

ericnordelo

Hey @0xknwn! The PR is looking great, thanks again for taking the time. I left some small suggestions. Also, we are in the middle of a migration to the latest edition, which would conflict with this PR if we merge it now. It would be great if you wait for that one to be merged, and then update this PR. If not we will be happy to update it and merge it later.

ericnordelo · 2024-05-29T12:14:57Z

_typos.toml

@@ -0,0 +1,2 @@
+[files]
+extend-exclude = ["test_p256_account.cairo"]


Why are we excluding this file?

Because your tool that checks for syntax consider I do spell some hexa value wrong and I do not know any other ways to pass the associated test. see https:/OpenZeppelin/cairo-contracts/actions/runs/9023095283/job/24794108410 for details. What is a better way to fix it?

ericnordelo · 2024-05-29T12:17:01Z

src/tests/account/test_secp256r1.cairo

+//
+
+/// This signature was computed using @noble/curves.
+fn P256_PRIVATEKEY_SAMPLE() -> u256 {


Since we are not verifying signatures for this test, Is this function needed?

see comment my next comment. I will remove it, once we agree on what to name it.

ericnordelo · 2024-05-29T12:17:43Z

src/tests/account/test_secp256r1.cairo

+}
+
+fn get_points() -> (Secp256r1Point, Secp256r1Point) {
+ let private_key = P256_PRIVATEKEY_SAMPLE();


Do we need x to be a private key? The name seems a bit misleading.

I can replace the x value on the curve by a const and remove the function. Whatever you want but tell me what you prefer otherwise we may spin around:

do you want a const or simply the value?

if you prefer a const or a function, how would you like I name it?

In this case, I think the value is okay.

ericnordelo · 2024-05-29T12:19:50Z

src/tests/account/test_secp256r1.cairo

+
+#[test]
+fn test_pack_big_secp256r1_points() {
+ let (big_point_1, big_point_2) = get_points();


Are these points big points? The idea of this test is to check that the packing is right even for big values, maybe it makes sense to use just big values even if they are not valid points.

Can you define big points or point me in the right direction so that I can build values that would make sense for that test?

A point with x big enough for the high part of the u256 to use as many bits as possible would be the best case, but making sure that at least the high part is not 0 should suffice.

ericnordelo · 2024-05-29T12:20:32Z

src/tests/account/test_p256_account.cairo

@@ -0,0 +1,29 @@
+use openzeppelin::account::interface::P256PublicKey;


Since we don't have this account in the library yet, I would move this into the test_secp256r1 test file.

You do not but we do, see https:/0xknwn/starknet-modular-account/blob/749a0485b0f50b9350f8f41eb1898e88f2f3a195/src/modules/p256validator/p256validator.cairo#L17 . The reason behind this PR is because we use your project as a library to develop a modular account. It makes sense that we use the same tool for secp256k1 and secp256r1 and that you provide the technology. For that purpose, it really makes sense types are kept in files that are not test and Eth and P256 public keys are in a parallel location

Got it. I'm not recommending to move the type though, but the contents of this file (to move test_p256_account.cairo contents into test_secp256r1.cairo). Sorry if it was a bit confusing.

Okay. I will do!

0xknwn · 2024-05-29T14:00:09Z

Hey @0xknwn! The PR is looking great, thanks again for taking the time. I left some small suggestions. Also, we are in the middle of a migration to the latest edition, which would conflict with this PR if we merge it now. It would be great if you wait for that one to be merged, and then update this PR. If not we will be happy to update it and merge it later.

@ericnordelo, I guess it was a rhetorical question 🤣. jk! I am not in a rush: I have forked your repository and our project is using the fork. So let's proceed that way, I will rebase on 0.14 once out and will rework this PR.

Can you please review my answers to your comments and let me know what you think?

ericnordelo · 2024-09-11T12:05:06Z

Hey @0xknwn! We've already finished migrating to foundry and workspaces, and we are more than happy to prioritize getting this merged since it's been stalled for a while. Would you happen to have the time to continue working on it with our feedback, or would you prefer us to take the lead on finishing it?

0xknwn mentioned this pull request May 9, 2024

Add support for secp256r1 #987

Open

0xknwn marked this pull request as ready for review May 9, 2024 23:14

0xknwn force-pushed the feature/add-secp256r1-support branch from 2dbb797 to d9a2744 Compare May 16, 2024 07:43

andrew-fleming reviewed May 21, 2024

View reviewed changes

0xknwn added 7 commits May 23, 2024 11:22

add support for secp256r1

237546a

expose is_valid_p256_signature

fa87a04

add tests for p256 keys and signatures

f2c175b

add tests for p256

ad74c63

added in unreleased section of CHANGELOG.md

fd49eed

attempt to exclude file from typos check

77a2260

integrate the feedback

e6754f1

0xknwn force-pushed the feature/add-secp256r1-support branch from d9a2744 to e6754f1 Compare May 23, 2024 10:44

0xknwn added 3 commits May 23, 2024 16:21

shows secp256_ec_get_point_from_x_syscall returns None

fa2485d

scarb fmt

9e6d203

use a sample private key

95e709b

0xknwn requested a review from andrew-fleming May 23, 2024 15:19

andrew-fleming reviewed May 28, 2024

View reviewed changes

andrew-fleming requested a review from ericnordelo May 28, 2024 14:03

remove tests as requested in latest feedback

150375a

0xknwn requested a review from andrew-fleming May 28, 2024 14:31

andrew-fleming approved these changes May 28, 2024

View reviewed changes

ericnordelo reviewed May 29, 2024

View reviewed changes

0xknwn requested a review from ericnordelo May 29, 2024 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

secp256r1 utils and is_valid_p256_signature #988

secp256r1 utils and is_valid_p256_signature #988

0xknwn commented May 9, 2024 •

edited

Loading

0xknwn commented May 9, 2024 •

edited

Loading

andrew-fleming commented May 11, 2024

andrew-fleming left a comment

0xknwn commented May 22, 2024

andrew-fleming commented May 22, 2024

0xknwn commented May 23, 2024 •

edited

Loading

0xknwn commented May 23, 2024

andrew-fleming left a comment

andrew-fleming May 28, 2024

0xknwn May 28, 2024

andrew-fleming left a comment

ericnordelo left a comment

ericnordelo May 29, 2024

0xknwn May 29, 2024 •

edited

Loading

ericnordelo May 29, 2024

0xknwn May 29, 2024

ericnordelo May 29, 2024

0xknwn May 29, 2024 •

edited

Loading

ericnordelo May 29, 2024

ericnordelo May 29, 2024

0xknwn May 29, 2024

ericnordelo May 29, 2024

ericnordelo May 29, 2024

0xknwn May 29, 2024 •

edited

Loading

ericnordelo May 29, 2024 •

edited

Loading

0xknwn May 29, 2024

0xknwn commented May 29, 2024

ericnordelo commented Sep 11, 2024

		@@ -0,0 +1,2 @@
		[files]
		extend-exclude = ["test_p256_account.cairo"]

		@@ -0,0 +1,29 @@
		use openzeppelin::account::interface::P256PublicKey;

secp256r1 utils and is_valid_p256_signature #988

Are you sure you want to change the base?

secp256r1 utils and is_valid_p256_signature #988

Conversation

0xknwn commented May 9, 2024 • edited Loading

PR Checklist

0xknwn commented May 9, 2024 • edited Loading

andrew-fleming commented May 11, 2024

andrew-fleming left a comment

Choose a reason for hiding this comment

0xknwn commented May 22, 2024

andrew-fleming commented May 22, 2024

0xknwn commented May 23, 2024 • edited Loading

0xknwn commented May 23, 2024

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xknwn May 29, 2024 • 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

0xknwn May 29, 2024 • 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

0xknwn May 29, 2024 • edited Loading

Choose a reason for hiding this comment

ericnordelo May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xknwn commented May 29, 2024

ericnordelo commented Sep 11, 2024

0xknwn commented May 9, 2024 •

edited

Loading

0xknwn commented May 9, 2024 •

edited

Loading

0xknwn commented May 23, 2024 •

edited

Loading

0xknwn May 29, 2024 •

edited

Loading

0xknwn May 29, 2024 •

edited

Loading

0xknwn May 29, 2024 •

edited

Loading

ericnordelo May 29, 2024 •

edited

Loading