Skip to content

feat: switch from libsecp256k1 to k256 #1672

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

Merged
merged 1 commit into from
Apr 15, 2025
Merged

Conversation

Stebalien
Copy link
Member

The former is deprecated in favor of the latter.

@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz Apr 15, 2025
@github-project-automation github-project-automation bot moved this from 📌 Triage to ✔️ Approved by reviewer in FilOz Apr 15, 2025
Base automatically changed from steb/update-fvm to master April 15, 2025 01:36
@Stebalien Stebalien force-pushed the steb/replace-libsecp256k1 branch 2 times, most recently from 21f805d to e9e7f6d Compare April 15, 2025 03:23
The former is deprecated in favor of the latter.
@Stebalien Stebalien force-pushed the steb/replace-libsecp256k1 branch from e9e7f6d to b5e9409 Compare April 15, 2025 03:27
@Stebalien
Copy link
Member Author

We should consider using the signature verification logic from shared in our tests. I didn't want to do that because I wanted to eventually remove it from shared but we'll get better test coverage...

Alternatively, we should port the other 3 ecrecover test vectors to the EVM. TBH, that's probably the right thing to do.

@rvagg
Copy link
Member

rvagg commented Apr 15, 2025

Alternatively, we should port the other 3 ecrecover test vectors to the EVM

can you spell this out a bit more, I've lost the list of failing tests now that you've fixed it but I imagine it would be obvious if I could see what caused you to discover the usage bug

@Stebalien
Copy link
Member Author

  1. The EC recover test cases from . Test vectors in https://github.com/filecoin-project/builtin-actors/blob/b5e940909192f41e70d179e0c9627f2858e62617/actors/evm/precompile-testdata/ecRecover.json.
  2. Possibly also a good idea to port
    fn bn_recover() {
    let rt = MockRuntime::default();
    rt.in_call.replace(true);
    let mut system = System::create(&rt).unwrap();
    let input = &hex!(
    "456e9aea5e197a1f1af7a3e85a3212fa4049a3ba34c2289b4c860fc0b0c64ef3" // h(ash)
    "000000000000000000000000000000000000000000000000000000000000001c" // v (recovery byte)
    // signature
    "9242685bf161793cc25603c231bc2f568eb630ea16aa137d2664ac8038825608" // r
    "4f8ae3bd7535248d0bd448298cc2e2071e56992d0774dc340c368ae950852ada" // s
    );
    let expected = hex!("0000000000000000000000007156526fbd7a3c72969b54f64e42c10fbb768c8a");
    let res = ec_recover(&mut system, input, PrecompileContext::default()).unwrap();
    assert_eq!(&res, &expected);
    let input = &hex!(
    "456e9aea5e197a1f1af7a3e85a3212fa4049a3ba34c2289b4c860fc0b0c64ef3" // h(ash)
    "000000000000000000000000000000000000000000000000000000000000001c" // v (recovery byte)
    // signature
    "0000005bf161793cc25603c231bc2f568eb630ea16aa137d2664ac8038825608" // r
    "4f8ae3bd7535248d0bd448298cc2e2071e56992d0774dc340c368ae950852ada" // s
    );
    // wrong signature
    let res = ec_recover(&mut system, input, PrecompileContext::default()).unwrap();
    assert!(res.is_empty());
    let input = &hex!(
    "456e9aea5e197a1f1af7a3e85a3212fa4049a3ba34c2289b4c860fc0b0c64ef3" // h(ash)
    "000000000000000000000000000000000000000000000000000000000000000a" // v (recovery byte)
    // signature
    "0000005bf161793cc25603c231bc2f568eb630ea16aa137d2664ac8038825608" // r
    "4f8ae3bd7535248d0bd448298cc2e2071e56992d0774dc340c368ae950852ada" // s
    );
    // invalid recovery byte
    let res = ec_recover(&mut system, input, PrecompileContext::default()).unwrap();
    assert!(res.is_empty());
    }
    .

@Stebalien
Copy link
Member Author

I attempted to switch to shared in #1676 but we'd need to add a separate "signature" feature because including "all of crypto" is too much.

@Stebalien Stebalien added this pull request to the merge queue Apr 15, 2025
Merged via the queue into master with commit 3660627 Apr 15, 2025
17 checks passed
@Stebalien Stebalien deleted the steb/replace-libsecp256k1 branch April 15, 2025 15:36
@github-project-automation github-project-automation bot moved this from ✔️ Approved by reviewer to 🎉 Done in FilOz Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

2 participants