feat(identity): switch from libsecp256k1 to k256

[drHuangMHT]

Description

libsecp256k1 is considered unmaintained, see #159.
Brief discussion here: #5888.
k256 is also a pure Rust implementation just like libsecp256k1.

Notes & open questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[drHuangMHT]

This is very interesting. The test vector for secp256k1(aka k256) contains a p256(aka ecdsa) public key, so the test failed. But the test should fail before switching to k256 because it still fails after I revert the changes.
Public key hex:0458F7E9581748FF9BDD933B655CC0E5552A1248F840658CC221DEC2186B5A2FE4641B86AB7590A3422CDBB1000CF97662F27E5910D7569F22FEED8829C8B52E0F(520 bit)

[Prabhat1308]

This is very interesting. The test vector for secp256k1(aka k256) contains a p256(aka ecdsa) public key, so the test failed. But the test should fail before switching to k256 because it still fails after I revert the changes. Public key hex:0458F7E9581748FF9BDD933B655CC0E5552A1248F840658CC221DEC2186B5A2FE4641B86AB7590A3422CDBB1000CF97662F27E5910D7569F22FEED8829C8B52E0F(520 bit)

Are you sure that this is the case for the test failing ?
Adding some print statements just after

let user_owns_sk = self
            .extension
            .public_key
            .verify(&msg, &self.extension.signature);

in P2pCertificate verify function , I get this result .

          public_key: PublicKey { publickey: Secp256k1(PublicKey(compressed): 26b53094d1112bce799dc826040ae6d4eb574157929f159817261f753d9b1b) }
          signature: [48, 68, 2, 32, 64, 113, 39, 7, 233, 119, 148, 196, 120, 217, 57, 137, 170, 162, 138, 225, 247, 28, 3, 175, 82, 74, 138, 75, 210, 217, 132, 36, 148, 138, 120, 35, 2, 32, 123, 97, 183, 240, 116, 182, 150, 162, 95, 185, 224, 5, 145, 65, 168, 17, 204, 204, 76, 194, 128, 66, 217, 48, 27, 155, 42, 64, 21, 232, 116, 112]
          msg: [108, 105, 98, 112, 50, 112, 45, 116, 108, 115, 45, 104, 97, 110, 100, 115, 104, 97, 107, 101, 58, 48, 89, 48, 19, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 8, 42, 134, 72, 206, 61, 3, 1, 7, 3, 66, 0, 4, 88, 247, 233, 88, 23, 72, 255, 155, 221, 147, 59, 101, 92, 192, 229, 85, 42, 18, 72, 248, 64, 101, 140, 194, 33, 222, 194, 24, 107, 90, 47, 228, 100, 27, 134, 171, 117, 144, 163, 66, 44, 219, 177, 0, 12, 249, 118, 98, 242, 126, 89, 16, 215, 86, 159, 34, 254, 237, 136, 41, 200, 181, 46, 15]

This is consistent in both the PR branch and the master branch. After this , this just calls the Secp256k1's verify function if I am correct ?

Also the public key you mentioned is derived from certificate's subject public key info while the public key used in this is from certificate's libp2pextension which might be the cause of the difference.

[drHuangMHT]

This is consistent in both the PR branch and the master branch. After this , this just calls the Secp256k1's verify function if I am correct ?

verify calls verify_hash, which is currently not correctly implemented. I can fix verify but not verify_hash because it probably involves changing the signature due to the generics.

Also the public key you mentioned is derived from certificate's subject public key info while the public key used in this is from certificate's libp2pextension which might be the cause of the difference.

That makes sense. Thanks for the explanation!

[Prabhat1308]

LGTM 7b69d77

[drHuangMHT]

cc @jxs

[drHuangMHT]

secp256k1::PublicKey can be Copy. Should I derive Copy for it in this PR?

[drHuangMHT]

CI reported Version of 'libp2p-identity' has been bumped more than once since last release v0.2.8. but I've checked on crates.io that the latest version of libp2p-identity is 0.2.10.

[elenaf9]

CI reported Version of 'libp2p-identity' has been bumped more than once since last release v0.2.8. but I've checked on crates.io that the latest version of libp2p-identity is 0.2.10.

The tag for libp2p-identity-v0.2.10 was missing. Pushed it now and re-ran the CI check.

[drHuangMHT]

The tag for libp2p-identity-v0.2.10 was missing. Pushed it now and re-ran the CI check.

semver check is still failing because of this:

rust-libp2p/Cargo.toml

Line 87 in 3b72e75

libp2p-identity = { version = "0.2.10" }

which is not linked to the workspace, but patched by:

rust-libp2p/Cargo.toml

Lines 148 to 156 in 3b72e75

	[patch.crates-io]
	# Patch away `libp2p-identity` in our dependency tree with the workspace version.
	# `libp2p-identity` is a leaf dependency and used within `rust-multiaddr` which is **not** part of the workspace.
	# As a result, we cannot just reference the workspace version in our crates because the types would mismatch with what
	# we import via `rust-multiaddr`.
	# This is expected to stay here until we move `libp2p-identity` to a separate repository which makes the dependency relationship more obvious.
	libp2p-identity = { path = "identity" }

I'm not sure how do deal with this.

[elenaf9]

The tag for libp2p-identity-v0.2.10 was missing. Pushed it now and re-ran the CI check.

semver check is still failing because of this:

rust-libp2p/Cargo.toml

Line 87 in 3b72e75

libp2p-identity = { version = "0.2.10" }

which is not linked to the workspace, but patched by:

rust-libp2p/Cargo.toml

Lines 148 to 156 in 3b72e75

	[patch.crates-io]
	# Patch away `libp2p-identity` in our dependency tree with the workspace version.
	# `libp2p-identity` is a leaf dependency and used within `rust-multiaddr` which is **not** part of the workspace.
	# As a result, we cannot just reference the workspace version in our crates because the types would mismatch with what
	# we import via `rust-multiaddr`.
	# This is expected to stay here until we move `libp2p-identity` to a separate repository which makes the dependency relationship more obvious.
	libp2p-identity = { path = "identity" }

I'm not sure how do deal with this.

@jxs can you help here?

[jxs]

The tag for libp2p-identity-v0.2.10 was missing. Pushed it now and re-ran the CI check.

semver check is still failing because of this:

rust-libp2p/Cargo.toml

Line 87 in 3b72e75

libp2p-identity = { version = "0.2.10" }

which is not linked to the workspace, but patched by:

rust-libp2p/Cargo.toml

Lines 148 to 156 in 3b72e75

	[patch.crates-io]
	# Patch away `libp2p-identity` in our dependency tree with the workspace version.
	# `libp2p-identity` is a leaf dependency and used within `rust-multiaddr` which is **not** part of the workspace.
	# As a result, we cannot just reference the workspace version in our crates because the types would mismatch with what
	# we import via `rust-multiaddr`.
	# This is expected to stay here until we move `libp2p-identity` to a separate repository which makes the dependency relationship more obvious.
	libp2p-identity = { path = "identity" }

I'm not sure how do deal with this.

it's expected, we need to release it for cargo-semver-checks to stop complaining

[drHuangMHT]

it's expected, we need to release it for cargo-semver-checks to stop complaining

That means we will merge with the failed check, right? I know it is not a required check though.

[jxs]

it's expected, we need to release it for cargo-semver-checks to stop complaining

That means we will merge with the failed check, right? I know it is not a required check though.

yeah exactly

[jxs]

Thanks DHuang, looks almost ready. Left some final comments

[jxs]

Thanks DrHuang LGTM! @elenaf9 want to give a final look before merging?
Feel free to add the send-it label if you also approve.

[jxs]

Thanks DrHuang LGTM! @elenaf9 want to give a final look before merging?
Feel free to add the send-it label if you also approve.