Added recover_public_key to secp256k1

[yuvalsw]

This change is 

[yuvalsw]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

a discussion (no related file):
change target branch to main

---

[yoga-braavos]

hi @yuvalsw , @bbrandtom and @liorgold2 , for secure enclave signing we cannot use the ECRECOVER algorithm for ECDSA as we only get (r,s) back from the secure enclave with no further info.

I suppose that one of the main motivations (if not the only one) to support the r1 curve is to allow signing with secure modules on Android, iOS and webauthn - so support is needed for the standard ECDSA verification algorithm.

[yuvalsw]

Hi
IIUC, you don't have v or y_parity.
In that case, one option is to call the function twice, once with false and once with true, and verify that one of them succeeded.
It'll be great if you send a PR that exposes this functionality (this is Cairo1 code). This might be possible to do even more efficiently than calling the function twice.

Please share if you have anything else in mind.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

[orizi]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @yuvalsw)

---

corelib/src/starknet/secp256k1.cairo line 22 at r2 (raw file):

fn get_N() -> u256 {
    u256 { high: 0xfffffffffffffffffffffffffffffffe, low: 0xbaaedce6af48a03bbfd25e8cd0364141 }
}

Suggestion:

// TODO(yuval): change to constant once u256 constants are supported.
fn get_N() -> u256 {
    0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141
}

---

corelib/src/starknet/secp256k1.cairo line 58 at r2 (raw file):

    msg_hash: u256, r: u256, s: u256, y_parity: bool
) -> Option<Secp256K1EcPoint> {
    let r_point = secp256k1_ec_get_point_from_x_syscall(x: r, :y_parity).expect('SYSCALL_FAILED')?;

i think this is probably preferred.

(and everywhere)

Suggestion:

.unwrap_syscall()?;

[Lukasz2891]

hi ;) What's the status of this PR? Is someone working on it? When do you expect to have it finished?

[yuvalsw]

good timing :)
Just finished working on some preliminary work needed for this and other secp functionality. Should be submitted very soon.

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

---

corelib/src/starknet/secp256k1.cairo line 22 at r2 (raw file):

fn get_N() -> u256 {
    u256 { high: 0xfffffffffffffffffffffffffffffffe, low: 0xbaaedce6af48a03bbfd25e8cd0364141 }
}

Done.

---

corelib/src/starknet/secp256k1.cairo line 58 at r2 (raw file):

Previously, orizi wrote…

i think this is probably preferred.

(and everywhere)

Done.

[orizi]

Reviewed 326 of 326 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yuvalsw)

---

corelib/src/starknet/secp256k1.cairo line 18 at r4 (raw file):

// TODO(yuval): change to constant once u256 constants are supported.
/// N = 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141

remove

Code quote:

/// N = 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141

[yuvalsw]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi)

---

corelib/src/starknet/secp256k1.cairo line 18 at r4 (raw file):

Previously, orizi wrote…

remove

Done.

[orizi]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yuvalsw)

---

corelib/src/starknet/secp256k1.cairo line 66 at r5 (raw file):

    let u1 = u256_div_mod_n(msg_hash, r, get_N());
    let minus_u1 = secp256k1_ec_negate_scalar(u1);

Consider inlining and removing the function

[yuvalsw]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi)

---

corelib/src/starknet/secp256k1.cairo line 66 at r5 (raw file):

Previously, orizi wrote…

Consider inlining and removing the function

I think this is a bit easier to follow (documents a bit more), and might be useful for more functions (but not sure). I submit it this way. LMK if you want a followup to change, I have no strong objection...

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)

a discussion (no related file):
please add a recovery example test.

---

[yuvalsw]

change target branch

[orizi]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yuvalsw)

[orizi]

Reviewed 324 of 326 files at r4, 1 of 4 files at r9, 6 of 10 files at r10, all commit messages.
Reviewable status: 331 of 335 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)

---

examples/secp256k1.cairo line 44 at r10 (raw file):

/// Returns a valid message hash and its signature for testing.
fn get_message_and_signature() -> (u256, u256, u256, bool, u256, u256) {
    // TODO(yg): change golden values to hex.

any reason to not already do that?

Code quote:

    // TODO(yg): change golden values to hex.

[yuvalsw]

Reviewable status: 330 of 336 files reviewed, 2 unresolved discussions (waiting on @orizi)

a discussion (no related file):

Previously, orizi wrote…

please add a recovery example test.

Done.

---

---

examples/secp256k1.cairo line 44 at r10 (raw file):

Previously, orizi wrote…

any reason to not already do that?

Done.

[orizi]

Reviewed 1 of 6 files at r11, all commit messages.
Reviewable status: 331 of 336 files reviewed, 3 unresolved discussions (waiting on @yuvalsw)

---

scripts/cairo_test.sh line 5 at r11 (raw file):

cargo run --bin cairo-test -- corelib/ && \
cargo run --bin cairo-test -- tests/bug_samples/ --starknet && \
cargo run --bin cairo-test -- examples/ --starknet

just move the example into cairo_level_tests/lib.rs and revert this.

Code quote:

cargo run --bin cairo-test -- examples/ --starknet

[yuvalsw]

Reviewable status: 331 of 336 files reviewed, 1 unresolved discussion (waiting on @orizi)

---

scripts/cairo_test.sh line 5 at r11 (raw file):

Previously, orizi wrote…

just move the example into cairo_level_tests/lib.rs and revert this.

Moved to corelib/src/tests as discussed f2f.

[orizi]

Reviewed 4 of 6 files at r11, 3 of 3 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)