core: replace secp256k with k256 in crypto::ecdsa #3525

michalkucharczyk · 2024-02-29T16:30:37Z

This PR replaces the usage of secp256k crate with k256 in core::crypto::ecdsa for non-std environments as outcome of discussion in #3448.

secp256k1 is used in std, meaning that we should not affect host performance with this PR.
k256 is enabled in runtimes (no-std), and is required to proceed with #2044.

If desirable, in future we can switch to k256 also for std. That would require some performance evaluation (e.g. for EVM chains as per #3448 (comment)).

Closes #3448

michalkucharczyk · 2024-02-29T16:33:47Z

cc: @burdges

michalkucharczyk · 2024-02-29T16:36:17Z

substrate/primitives/core/src/ecdsa.rs

 	}
 }

 #[cfg(feature = "full_crypto")]
-impl From<RecoverableSignature> for Signature {
-	fn from(recsig: RecoverableSignature) -> Signature {
+impl From<(k256::ecdsa::Signature, RecoveryId)> for Signature {


note: (k256::ecdsa::Signature, RecoveryId) is a result of sign_prehash_recoverable. Convenient From.

michalkucharczyk · 2024-02-29T16:37:48Z

substrate/primitives/core/src/ecdsa.rs

-		context.sign_ecdsa_recoverable(&message, &self.secret).into()
+		self.secret
+			.sign_prehash_recoverable(message)
+			.expect("signing may not fail (???). qed.")


In our API (e.g. TraitPair) we assume signing cannot fail (return type is not Option or Result).
Is expect fine here? What would be condition for potential failure?

substrate/primitives/core/src/ecdsa.rs

michalkucharczyk · 2024-02-29T16:40:01Z

substrate/primitives/core/src/ecdsa.rs

+		Self::unchecked_from(
+			pubkey.to_sec1_bytes()[..]
+				.try_into()
+				.expect("valid key is serializable to [u8,33]. qed."),


double-check needed here. Can we actually fail?

substrate/primitives/core/Cargo.toml

michalkucharczyk · 2024-02-29T17:21:50Z

bot fmt

command-bot · 2024-02-29T17:21:55Z

@michalkucharczyk https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5397860 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 6-05ac224d-82d5-4c02-8f45-8696b2e2ed40 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-02-29T17:23:50Z

@michalkucharczyk Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5397860 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5397860/artifacts/download.

davxy

I'll give it a try tomorrow. Maybe before merge we should try to sync from genesis (aka burn-in test)?

substrate/primitives/core/src/ecdsa.rs

Two features added: - backend_secp256k1 - backend_k256 This allows to choose a secp256k1 implementation backend for crypto/ecdsa module. backend_secp256k1 is by default enabled in std.

substrate/primitives/core/Cargo.toml

davxy · 2024-03-04T17:34:05Z

Maybe without leaving too much choice (choices sometimes makes the user confused 😆) we can just use secp256k1 on std and k256 for no-std. I mean, without introducing these two mutually exclusive features.

I'd personally find it easier if I have no idea what I'm doing and what is best

substrate/primitives/core/src/ecdsa.rs

…ypto-ecdsa

michalkucharczyk · 2024-03-06T11:55:58Z

I'll give it a try tomorrow. Maybe before merge we should try to sync from genesis (aka burn-in test)?

Sync from genesis test on kusama and polkadot was fine (with k256 on host side).

davxy

LGTM

substrate/primitives/core/src/ecdsa.rs

Co-authored-by: Davide Galassi <[email protected]>

substrate/primitives/core/Cargo.toml

Bring back k256/std. This reverts commit 38de881.

This PR replaces the usage of [secp256k](https://crates.io/crates/secp256k1) crate with [k256](https://crates.io/crates/k256) in `core::crypto::ecdsa` for `non-std` environments as outcome of discussion in paritytech#3448. `secp256k1` is used in `std`, meaning that we should not affect host performance with this PR. `k256` is enabled in runtimes (`no-std`), and is required to proceed with paritytech#2044. If desirable, in future we can switch to `k256` also for `std`. That would require some performance evaluation (e.g. for EVM chains as per paritytech#3448 (comment)). Closes paritytech#3448 --------- Co-authored-by: command-bot <> Co-authored-by: Davide Galassi <[email protected]>

michalkucharczyk added 2 commits February 29, 2024 17:25

core: crypto/ecdsa switch to k256

fa7589c

Cargo.lock

d6cf147

michalkucharczyk requested a review from davxy February 29, 2024 16:30

michalkucharczyk added T0-node This PR/Issue is related to the topic “node”. R0-silent Changes should not be mentioned in any release notes labels Feb 29, 2024

michalkucharczyk commented Feb 29, 2024

View reviewed changes

substrate/primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

michalkucharczyk commented Feb 29, 2024

View reviewed changes

fixes for no-std

682ddf0

michalkucharczyk commented Feb 29, 2024

View reviewed changes

substrate/primitives/core/Cargo.toml Show resolved Hide resolved

Cargo.lock

277068b

".git/.scripts/commands/fmt/fmt.sh"

80b133e

davxy reviewed Feb 29, 2024

View reviewed changes

substrate/primitives/core/src/ecdsa.rs Show resolved Hide resolved

substrate/primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

michalkucharczyk added 5 commits March 1, 2024 09:08

seed doc improved

35878b2

secp256k1 is used on host

e63fbac

Two features added: - backend_secp256k1 - backend_k256 This allows to choose a secp256k1 implementation backend for crypto/ecdsa module. backend_secp256k1 is by default enabled in std.

Cargo.lock

7db5674

features fix

93086ec

make clippy happy

42bb70c

davxy reviewed Mar 4, 2024

View reviewed changes

substrate/primitives/core/Cargo.toml Outdated Show resolved Hide resolved

michalkucharczyk added 2 commits March 4, 2024 19:08

backend features removed: std/no-std used

0fc7201

Merge branch 'master' into mku-k256-in-core-crypto-ecdsa

8b4c147

michalkucharczyk commented Mar 4, 2024

View reviewed changes

substrate/primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

michalkucharczyk commented Mar 4, 2024

View reviewed changes

substrate/primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

michalkucharczyk commented Mar 4, 2024

View reviewed changes

substrate/primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

fixes

6797ec4

michalkucharczyk requested a review from davxy March 4, 2024 20:55

michalkucharczyk added 3 commits March 5, 2024 08:38

minor fixes

461998e

Merge remote-tracking branch 'origin/master' into mku-k256-in-core-cr…

2d468dd

…ypto-ecdsa

Merge branch 'master' into mku-k256-in-core-crypto-ecdsa

237b155

michalkucharczyk requested review from a team and removed request for a team March 6, 2024 11:35

Merge branch 'master' into mku-k256-in-core-crypto-ecdsa

2349f61

davxy approved these changes Mar 8, 2024

View reviewed changes

davxy requested a review from a team March 8, 2024 16:08

davxy reviewed Mar 8, 2024

View reviewed changes

substrate/primitives/core/src/ecdsa.rs Outdated Show resolved Hide resolved

davxy reviewed Mar 8, 2024

View reviewed changes

substrate/primitives/core/src/ecdsa.rs Show resolved Hide resolved

davxy requested a review from a team March 8, 2024 16:13

Update substrate/primitives/core/src/ecdsa.rs

c6d3bf2

Co-authored-by: Davide Galassi <[email protected]>

michalkucharczyk requested a review from burdges March 8, 2024 16:51

bkchr approved these changes Mar 8, 2024

View reviewed changes

substrate/primitives/core/Cargo.toml Show resolved Hide resolved

michalkucharczyk added 3 commits March 8, 2024 21:38

Update substrate/primitives/core/Cargo.toml

38de881

Merge branch 'master' into mku-k256-in-core-crypto-ecdsa

b5ddea9

Revert "Update substrate/primitives/core/Cargo.toml"

b7b215a

Bring back k256/std. This reverts commit 38de881.

michalkucharczyk added this pull request to the merge queue Mar 9, 2024

Merged via the queue into master with commit 9f5d9fa Mar 9, 2024
129 of 130 checks passed

michalkucharczyk deleted the mku-k256-in-core-crypto-ecdsa branch March 9, 2024 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: replace secp256k with k256 in crypto::ecdsa #3525

core: replace secp256k with k256 in crypto::ecdsa #3525

michalkucharczyk commented Feb 29, 2024 •

edited by davxy

Loading

michalkucharczyk commented Feb 29, 2024

michalkucharczyk Feb 29, 2024 •

edited

Loading

michalkucharczyk Feb 29, 2024 •

edited

Loading

michalkucharczyk Feb 29, 2024

michalkucharczyk commented Feb 29, 2024

command-bot bot commented Feb 29, 2024 •

edited

Loading

command-bot bot commented Feb 29, 2024

davxy left a comment

davxy commented Mar 4, 2024 •

edited

Loading

michalkucharczyk commented Mar 6, 2024

davxy left a comment

core: replace secp256k with k256 in crypto::ecdsa #3525

core: replace secp256k with k256 in crypto::ecdsa #3525

Conversation

michalkucharczyk commented Feb 29, 2024 • edited by davxy Loading

michalkucharczyk commented Feb 29, 2024

michalkucharczyk Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

michalkucharczyk Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

michalkucharczyk Feb 29, 2024

Choose a reason for hiding this comment

michalkucharczyk commented Feb 29, 2024

command-bot bot commented Feb 29, 2024 • edited Loading

command-bot bot commented Feb 29, 2024

davxy left a comment

Choose a reason for hiding this comment

davxy commented Mar 4, 2024 • edited Loading

michalkucharczyk commented Mar 6, 2024

davxy left a comment

Choose a reason for hiding this comment

michalkucharczyk commented Feb 29, 2024 •

edited by davxy

Loading

michalkucharczyk Feb 29, 2024 •

edited

Loading

michalkucharczyk Feb 29, 2024 •

edited

Loading

command-bot bot commented Feb 29, 2024 •

edited

Loading

davxy commented Mar 4, 2024 •

edited

Loading