feat: add support for secp256r1 and replace libsecp256k1 #6581

obycode · 2025-10-09T15:44:54Z

Implementing #6554.

jcnelson · 2025-10-10T14:24:30Z

Why are we dropping libsecp256k1 in favor of the one from RustCrypto? The former has been extensively audited and is used by Bitcoin.

Add tests for `secp256k1-verify` and `secp256k1-recover?` while adding tests for this new function.

obycode · 2025-10-10T21:43:14Z

Why are we dropping libsecp256k1 in favor of the one from RustCrypto? The former has been extensively audited and is used by Bitcoin.

The thought was that since we're pulling in the RustCrypto crate for secp256r1, we may as well also replace the k256 crate as well with this Rust-native solution that seems to be well-vetted and audited as well. libsecp256k1 is a wrapped C library.

This removes the need for trying the recovery bytes to find the right one.

…eserialize` The signature will be checked after deserialization. Checking it here will cause a deserialization failure when what we actually want is a signature validation failure.

obycode · 2025-10-14T14:41:32Z

stackslib/src/chainstate/stacks/block.rs

-        // signature must be well-formed
-        let _ = signature
-            .to_secp256k1_recoverable()
-            .ok_or(codec_error::DeserializeError(
-                "Failed to parse signature".to_string(),
-            ))?;


Note to reviewers: this check here causes the deserialization of fake signatures used in tests to fail early. The signature itself will always be checked later.

(it fails when one of the components is all 0s)

Can you leave this behavior here, and simply turn it off for testing? Wouldn't want to break anything by accident.

Sure, I can leave it to be safe, but I don't think it is necessary. Added back in bec654c.

jcnelson · 2025-10-14T20:24:05Z

clarity/src/vm/docs/mod.rs

+    snippet: "secp256r1-verify ${1:message-hash} ${2:signature} ${3:public-key})",
+    output_type: "bool",
+    signature: "(secp256r1-verify message-hash signature public-key)",
+    description: "The `secp256r1-verify` function verifies that the provided signature of the message-hash


Might want to clarify specifically that this is not the Bitcoin signature scheme, despite having a nearly-identical spelling. This is NIST P-256.

jcnelson · 2025-10-14T20:26:03Z

clarity/src/vm/docs/mod.rs

+    signature: "(secp256r1-verify message-hash signature public-key)",
+    description: "The `secp256r1-verify` function verifies that the provided signature of the message-hash
+was signed with the private key that generated the public key.
+`message-hash` is the `sha256` of the message and `signature` is the raw 64-byte signature.",


To be clear, it doesn't actually matter what hash function is used from the perspective of ECDSA. In this particular implementation, we only require that the message m is 32 bytes long (which in practice is a 256-bit cryptographic hash of a larger message).

Right. I copied that language from secp256k1-verify. We can remove it from both if that is more clear.

Yeah I think it could be as simple as changing both to be "... is typically the sha256..."

Updated in 1b5c605.

jcnelson · 2025-10-14T20:26:49Z

clarity/src/vm/functions/crypto.rs

+                )
+                .into());
+            }
+            if data.len() != 64 {


Shouldn't this be a TypeValueError like it is above?

~~Also, aren't recoverable P-256 signatures 65 bytes long? You need 32 bytes for r, 32 bytes for s, and 1 byte for v to indicate which of a set of recovered public keys is the real one.~~

EDIT: nevermind, recoverable signatures aren't called for in the SIP. But my question still stands -- shouldn't we be treating anything other than a (buff 64) as a type value error?

The one above is a TypeValueError that should never be reachable, because the type-checker should have ensured that this was a (buff 64). For this case, if the signature is < 64 bytes, the type-checker wouldn't have caught that, so it just returns false, so that we can make this function infallible (it only returns true or false in practice). secp256k1-verify behaves the same way.

After discussion based on real-world usage of secp256r1, I decided to drop the recovery byte and the -recover? function, so these signatures are just the 64-bytes.

jcnelson · 2025-10-14T20:35:31Z

clarity/src/vm/tests/crypto.rs

+}
+
+#[test]
+fn test_secp256r1_verify_signature_too_short_returns_false() {


I'm still confused by this choice. How would a caller be led to believe that a message signature could be anything other than 65 bytes? A NIST P-256 signature has r and s values that fit into 32 bytes. We don't use DER encoding (or any sort of self-describing variable-length field encoding) for the signature values, so a signature buffer must allocate fixed-sized fields for r and s. This means that r and s must fit into a 64 byte buffer. ~~Because we want these signatures to be recoverable, we include a 65th byte to be a parity byte v.~~ Given that we already offer no other way to represent a signature in anything other than 65 64 bytes, I think that a signature with anything other than 65 64 bytes should just be treated as a runtime error.

EDIT: crossed out language regarding recoverable signatures, since the SIP does not call for this.

My preference is to leave it returning false in this case, rather than an un-catchable runtime error. The alternative is to make secp256r1-verify return a response and return an err in this case, but that would be different behavior than what secp256k1-verify has, and it is kind of nice to just get a bool here, with no need to deal with a response.

Yeah, I think my preference would also be to return false instead of a runtime error

clarity/src/vm/tests/crypto.rs

jcnelson · 2025-10-14T20:37:23Z

clarity/src/vm/tests/crypto.rs

+}
+
+#[test]
+fn test_secp256k1_verify_signature_too_short_returns_false() {


I think we should at least plan to fix this for secp256k1 as well. If the signature isn't 65 bytes, it's a runtime error.

clarity/src/vm/tests/crypto.rs

jcnelson · 2025-10-14T20:41:00Z

stacks-common/src/util/secp256k1.rs

+        let recovery_id = K256RecoveryId::from_byte(self.0[0])?;
+        let mut sig_bytes = [0u8; 64];
+        sig_bytes[..64].copy_from_slice(&self.0[1..=64]);
+        let signature = K256Signature::from_slice(&sig_bytes).ok()?;


Can't this just be the following?

let signature = K256Signature::from_slice(&self.0[1..=64]).ok()?;

Then we can cut out the need for sig_bytes

Good catch! 58d4dd3

jcnelson · 2025-10-14T20:42:32Z

stacks-common/src/util/secp256k1.rs

+
+    /// Recovers message and signature to public key (will be compressed).
+    pub fn recover_to_pubkey(
+        _msg: &[u8],


Why are these arguments prefixed with _?

Oops, artifact of something I did at some point. Fixed in 31f25e5.

jcnelson · 2025-10-14T20:44:56Z

stacks-common/src/util/secp256k1.rs

+}
+
+#[cfg(test)]
+mod tests {


Can you include all of the tests from the previous (now deleted) version of secp256k1.rs so we can be certain that no behavioral changes crept in?

Re-added in a2fb57f

stacks-common/src/util/secp256r1.rs

This check is unnecessary and it can't run in tests, since we sometimes provide fake signatures which will fail. Leaving it in place outside of tests to make sure we don't accidentally change some behavior.

Consensus in epoch 3.3 is broken because costs-4 changed.

hstove

Everything looks great! I tried out some fixtures by using @noble/curves and it all worked as expected. My comments are pretty much all about code "style" / DX and risk bike-shedding.

It would be nice to not add any new clippy::indexing_slicing violations, so some of my suggestions are around that, even though I know you're just copy/pasting from existing code.

hstove · 2025-10-20T21:53:02Z

stacks-common/src/util/secp256r1.rs

+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum Secp256r1Error {
+    InvalidKey,
+    InvalidSignature,
+    InvalidMessage,
+    InvalidRecoveryId,
+    SigningFailed,
+    RecoveryFailed,
+}
+
+impl fmt::Display for Secp256r1Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Secp256r1Error::InvalidKey => write!(f, "Invalid key"),
+            Secp256r1Error::InvalidSignature => write!(f, "Invalid signature"),
+            Secp256r1Error::InvalidMessage => write!(f, "Invalid message"),
+            Secp256r1Error::InvalidRecoveryId => write!(f, "Invalid recovery ID"),
+            Secp256r1Error::SigningFailed => write!(f, "Signing failed"),
+            Secp256r1Error::RecoveryFailed => write!(f, "Recovery failed"),
+        }
+    }
+}


My preference would be for use to use thiserror here

it's annoying that a lot of these RustCrypto functions return Result<Self>, otherwise using thiserror would also provide some nice DX improvements. Oh well!

Thanks. I haven't gotten used to thiserror yet, but it is nice. Updated in 0ca9f79.

hstove · 2025-10-20T22:05:54Z

stacks-common/src/util/secp256k1.rs

+    RecoveryFailed,
+}
+
+impl fmt::Display for Secp256k1Error {


If you do go with thiserror, also here

👍 Updated in 0ca9f79.

hstove · 2025-10-20T22:21:59Z

stacks-common/src/util/secp256k1.rs

+        if sig.len() < 65 {
+            buf[..sig.len()].copy_from_slice(sig);
+        } else {
+            buf.copy_from_slice(&sig[..65]);
+        }


Suggested change

if sig.len() < 65 {

buf[..sig.len()].copy_from_slice(sig);

} else {

buf.copy_from_slice(&sig[..65]);

}

for (dst, src) in buf.iter_mut().zip(sig.iter().copied()) {

*dst = src;

}

How about this instead?

/// Generates place-holder data (for testing purposes only). #[cfg(any(test, feature = "testing"))] pub fn from_raw(sig: &[u8]) -> MessageSignature { const LEN: usize = 65; let mut buf = [0u8; LEN]; let n = sig.len().min(LEN); buf[..n].copy_from_slice(&sig[..n]); MessageSignature(buf) }

Oh., never mind, I see you were trying to get rid of the array accesses. Will fix.

hstove · 2025-10-20T22:23:18Z

stacks-common/src/util/secp256k1.rs

+
+    /// Converts to a secp256k1::ecdsa::RecoverableSignature.
+    pub fn to_secp256k1_recoverable(&self) -> Option<RecoverableSignature> {
+        let recovery_id = K256RecoveryId::from_byte(self.0[0])?;


Suggested change

let recovery_id = K256RecoveryId::from_byte(self.0[0])?;

let (recid_byte, sig_bytes) = self.0.split_first()?;

let recovery_id = K256RecoveryId::from_byte(*recid_byte)?;

let signature = K256Signature::from_slice(sig_bytes).ok()?;

hstove · 2025-10-20T22:27:26Z

stacks-common/src/util/secp256k1.rs

+        let recovery_id_byte = recid.to_byte();
+        ret_bytes[0] = recovery_id_byte;
+        ret_bytes[1..=64].copy_from_slice(&bytes[..64]);


Suggested change

let recovery_id_byte = recid.to_byte();

ret_bytes[0] = recovery_id_byte;

ret_bytes[1..=64].copy_from_slice(&bytes[..64]);

if let Some((first, rest)) = ret_bytes.split_first_mut() {

*first = recid.to_byte();

rest.copy_from_slice(&bytes);

}

hstove · 2025-10-20T22:39:19Z

stacks-common/src/util/secp256r1.rs

+}
+
+/// A Secp256r1 public key
+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)]


This is sorta nit, but what do you think about just serializing the whole struct with ::to_hex and ::from_hex? And also for Secp256r1PrivateKey? It would probably also help with logging - you'd just see the hex instead of { key: string, compressed: bool }

You can actually just do:

#[derive(Debug, PartialEq, Eq, Clone)] pub struct Secp256r1PublicKey { key: P256VerifyingKey, compressed: bool, } impl_byte_array_serde!(Secp256r1PublicKey);

Oh, nice. Looks like I can do the same for the private key as well.

hstove · 2025-10-20T22:42:22Z

stacks-common/src/util/secp256r1.rs

+#[cfg(any(test, feature = "testing"))]
+impl Default for Secp256r1PublicKey {
+    fn default() -> Self {
+        Self::new()


Since Secp256r1PublicKey::new() isn't used elsewhere, can we just remove it and add the implementation here? Or, maybe better, rename ::new to random()?

hstove · 2025-10-20T22:43:40Z

stacks-common/src/util/secp256r1.rs

+                // set this to true: LocalPeer will be doing this anyways,
+                //  and that's currently the only way this method is used


Copypasta - you can just remove these lines

hstove · 2025-10-20T22:46:32Z

stacks-common/src/util/secp256r1.rs

+    /// Converts from a p256::ecdsa::Signature to our MessageSignature
+    pub fn from_p256_signature(sig: &P256Signature) -> MessageSignature {
+        let sig_bytes = sig.to_bytes();
+        let mut ret_bytes = [0u8; 64];
+        ret_bytes.copy_from_slice(&sig_bytes);
+        MessageSignature(ret_bytes)
+    }
+
+    /// Converts to a p256::ecdsa::Signature
+    pub fn to_p256_signature(&self) -> Result<P256Signature, Secp256r1Error> {
+        P256Signature::from_slice(&self.0).map_err(|_| Secp256r1Error::InvalidSignature)
+    }


This is definitely nit, but what do you think about instead implementing MessageSignature::from<P256Signature> and P256Signature::try_from<MessageSignature>? I honestly see both sides of which is better (from_p256_signature is more explicit, but From gives into() etc). Feel free to resolve this if you prefer the current code!

The problem is that we can't implement P256Signature::try_from<MessageSignature> since that type is defined in the p256 crate. That's why I decided to just leave both like this for consistency.

hstove · 2025-10-20T23:03:43Z

stacks-common/src/util/secp256r1.rs

+    let encoded_point =
+        EncodedPoint::from_bytes(pubkey_arr).map_err(|_| Secp256r1Error::InvalidKey)?;
+
+    let public_key =
+        Option::<P256PublicKey>::from(P256PublicKey::from_encoded_point(&encoded_point))
+            .ok_or(Secp256r1Error::InvalidKey)?;
+    let verifying_key = P256VerifyingKey::from(public_key);
+
+    let signature =
+        P256Signature::from_slice(signature_arr).map_err(|_| Secp256r1Error::InvalidSignature)?;
+
+    verifying_key
+        .verify(message_arr, &signature)
+        .map_err(|_| Secp256r1Error::InvalidSignature)


IMO it would be better to re-use our "library wrapper" code to prevent us from having differing applications

Suggested change

let encoded_point =

EncodedPoint::from_bytes(pubkey_arr).map_err(|_| Secp256r1Error::InvalidKey)?;

let public_key =

Option::<P256PublicKey>::from(P256PublicKey::from_encoded_point(&encoded_point))

.ok_or(Secp256r1Error::InvalidKey)?;

let verifying_key = P256VerifyingKey::from(public_key);

let signature =

P256Signature::from_slice(signature_arr).map_err(|_| Secp256r1Error::InvalidSignature)?;

verifying_key

.verify(message_arr, &signature)

.map_err(|_| Secp256r1Error::InvalidSignature)

let pk = Secp256r1PublicKey::from_slice(pubkey_arr).map_err(|_| Secp256r1Error::InvalidKey)?;

let sig =

MessageSignature::from_bytes(signature_arr).ok_or(Secp256r1Error::InvalidSignature)?;

pk.verify_digest(message_arr, &sig)

.map_err(|_| Secp256r1Error::InvalidSignature)?;

Ok(())

Good call. 58ca134

obycode added 2 commits October 9, 2025 11:42

feat: add support for secp256r1 and replace libsecp256k1

9c945ed

Merge branch 'develop' into feat/secp256r1

e2d2957

feat: implement secp256r1-verify

7d688dd

Add tests for `secp256k1-verify` and `secp256k1-recover?` while adding tests for this new function.

obycode marked this pull request as ready for review October 10, 2025 21:41

obycode requested review from a team as code owners October 10, 2025 21:41

obycode linked an issue Oct 10, 2025 that may be closed by this pull request

[Clarity-4] secp256r1 #6554

Open

obycode requested review from jcnelson and kantai October 10, 2025 21:55

obycode added 6 commits October 13, 2025 11:52

fix: missing case in test

b3e53c5

Merge branch 'develop' into feat/secp256r1

0439c6e

test: update test to allow high-s signatures for secp256r1

99e8c54

chore: fix typo

64b7ceb

feat: use sign_prehash_recoverable

3127583

This removes the need for trying the recovery bytes to find the right one.

fix: delete unnecessary check in `StacksMicroblockHeader::consensus_d…

a63baea

…eserialize` The signature will be checked after deserialization. Checking it here will cause a deserialization failure when what we actually want is a signature validation failure.

obycode commented Oct 14, 2025

View reviewed changes

obycode added 4 commits October 14, 2025 11:35

test: update consensus tests

a89d69d

fix: restrict Default implementation to rand feature

cf513c7

chore: update Cargo.lock

3749d7f

chore: update other fuzz/Cargo.lock

2db0108

jcnelson reviewed Oct 14, 2025

View reviewed changes

clarity/src/vm/tests/crypto.rs Show resolved Hide resolved

jcnelson reviewed Oct 14, 2025

View reviewed changes

clarity/src/vm/tests/crypto.rs Show resolved Hide resolved

jcnelson reviewed Oct 14, 2025

View reviewed changes

clarity/src/vm/tests/crypto.rs Show resolved Hide resolved

jcnelson reviewed Oct 14, 2025

View reviewed changes

stacks-common/src/util/secp256r1.rs Show resolved Hide resolved

obycode added 5 commits October 14, 2025 17:38

chore: leave signature check in consensus_deserialize

bec654c

This check is unnecessary and it can't run in tests, since we sometimes provide fake signatures which will fail. Leaving it in place outside of tests to make sure we don't accidentally change some behavior.

docs: clarify secp256r1-verify description

11ce346

refactor: avoid clone

58d4dd3

chore: remove _ in used param names

31f25e5

test: re-add tests from old implementation

a2fb57f

obycode requested review from hstove and removed request for kantai October 15, 2025 14:34

brice-stacks requested a review from jcnelson October 16, 2025 18:42

obycode added 2 commits October 20, 2025 11:45

Merge branch 'develop' into feat/secp256r1

f218a11

fix: consensus tests affected by these changes

f5fdbfc

Consensus in epoch 3.3 is broken because costs-4 changed.

hstove reviewed Oct 20, 2025

View reviewed changes

obycode added 7 commits October 20, 2025 20:28

docs: clarify message hash of secp functions

1b5c605

feat: use thiserror for secp errors

0ca9f79

refactor: various PR suggestions

6977195

refactor: minor changes to avoid array indexing

b4c3dd6

refactor: use impl_byte_array_serde!

6a639e6

refactor: use library wrapper code in secp256r1_verify

58ca134

Merge branch 'develop' into feat/secp256r1

d4a3dff

-        let recovery_id = K256RecoveryId::from_byte(self.0[0])?;
+        let (recid_byte, sig_bytes) = self.0.split_first()?;
+        let recovery_id = K256RecoveryId::from_byte(*recid_byte)?;
+        let signature = K256Signature::from_slice(sig_bytes).ok()?;

		// set this to true: LocalPeer will be doing this anyways,
		// and that's currently the only way this method is used

feat: add support for secp256r1 and replace libsecp256k1 #6581

Are you sure you want to change the base?

feat: add support for secp256r1 and replace libsecp256k1 #6581

Conversation

obycode commented Oct 9, 2025

Uh oh!

jcnelson commented Oct 10, 2025

Uh oh!

obycode commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcnelson Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obycode Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcnelson Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hstove left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jcnelson Oct 14, 2025 •

edited

Loading

obycode Oct 14, 2025 •

edited

Loading

jcnelson Oct 14, 2025 •

edited

Loading