diff --git a/maint/codeql/rust/lib/imports.qll b/maint/codeql/rust/lib/imports.qll index 0aa7365e..218c51d8 100644 --- a/maint/codeql/rust/lib/imports.qll +++ b/maint/codeql/rust/lib/imports.qll @@ -53,17 +53,20 @@ predicate isPublicMod(Module m) { not exists(m.getVisibility().getPath()) } +/** Holds if `u` sits directly inside the crate-root module `name`. */ +private predicate isInRootModule(Use u, string name) { + exists(Module m | + m.getName().getText() = name and + u.getParentNode() = m.getItemList() and + isRootModule(m) + ) +} + /** * Holds if `u` lives inside a crate-root `__private` module * (intentional crate-level re-exports for macro support). */ -predicate isMacroReexport(Use u) { - exists(Module priv | - priv.getName().getText() = "__private" and - u.getParentNode() = priv.getItemList() and - isRootModule(priv) - ) -} +predicate isMacroReexport(Use u) { isInRootModule(u, "__private") } /** Holds if `u` is an allowlisted re-export from a foreign crate. */ private predicate isAllowlistedReexport(Use u) { @@ -74,6 +77,29 @@ private predicate isAllowlistedReexport(Use u) { u.getUseTree().getPath().getSegment().getIdentifier().getText() = "Numeric" ) or + fileOf(u).getAbsolutePath().matches("%pkgs/pkc/%") and + // Crate emits types relying on types or traits defined by a dependency, part of public API + isInRootModule(u, "__deps") and + ( + usePrefix(u) = "blst" + or + usePrefix(u) = "dash_num" + or + usePrefix(u) = "dash_types" + or + usePrefix(u) = "ff" + or + usePrefix(u) = "group" + or + usePrefix(u) = "rand_core" + or + usePrefix(u) = "secp256k1" + or + usePrefix(u) = "subtle" + or + usePrefix(u) = "zeroize" + ) + or fileOf(u).getAbsolutePath().matches("%pkgs/script/%") and ( // Workaround for the orphan rule, not part of public API diff --git a/maint/codeql/rust/lib/pkc.qll b/maint/codeql/rust/lib/pkc.qll index 26dd3989..ba96bf1d 100644 --- a/maint/codeql/rust/lib/pkc.qll +++ b/maint/codeql/rust/lib/pkc.qll @@ -9,9 +9,11 @@ /** * Holds if `name` belongs to `arm` alone for `role`, exempting the other arms * from offering it. Rows live in `pkc.model.yml`. - * - * The rows name what an arm lacks rather than what the arms share, so a - * method added to one arm and forgotten in another is reported with no list - * to maintain. */ extensible predicate armOnly(string arm, string role, string name); + +/** + * Holds if `arm` cannot carry `trait` at `role`, though the other arms do. + * Rows live in `pkc.model.yml`. + */ +extensible predicate armLacksTrait(string arm, string role, string trait); diff --git a/maint/codeql/rust/lib/traits.qll b/maint/codeql/rust/lib/traits.qll index b2fd873d..b907def9 100644 --- a/maint/codeql/rust/lib/traits.qll +++ b/maint/codeql/rust/lib/traits.qll @@ -102,6 +102,32 @@ predicate implementsTrait(TypeItem t, string traitName) { exists(Impl i | i.getSelf() = t and implTraitName(i) = traitName) } +/** + * Holds if `i`'s trait reference carries a type or const argument, naming + * `From` rather than `Copy`. A lifetime does not count, since + * `Deserialize<'de>` is still a property of the type. + */ +private predicate hasTraitTypeArg(Impl i) { + exists(GenericArg a | + a = implTraitPath(i).getSegment().getGenericArgList().getAGenericArg() and + not a instanceof LifetimeArg + ) +} + +/** + * Holds if `t` implements `traitName` and the trait takes no type argument, + * through a derive, a hand-written impl, or a macro. Parameterised traits are + * excluded. + */ +predicate implementsPlainTrait(TypeItem t, string traitName) { + exists(Impl i | + i.getSelf() = t and + fileOf(i).fromSource() and + not hasTraitTypeArg(i) and + traitName = implTraitName(i) + ) +} + /** * Holds if `t` has a derived impl for `traitName` under `crate` * (i.e. the trait path is `::::`). diff --git a/maint/codeql/rust/pkc.model.yml b/maint/codeql/rust/pkc.model.yml index 38809e91..4c9342c0 100644 --- a/maint/codeql/rust/pkc.model.yml +++ b/maint/codeql/rust/pkc.model.yml @@ -64,3 +64,7 @@ extensions: # Recoverable signatures - ["Ecdsa", "SecretKey", "sign_recoverable"] # Sign - ["Ecdsa", "PublicKey", "recover"] # Get public key from a signature + - addsTo: + pack: base-sdk/codeql-rust + extensible: armLacksTrait + data: [] diff --git a/maint/codeql/rust/pkc.ql b/maint/codeql/rust/pkc.ql index 0740f954..fb91c227 100644 --- a/maint/codeql/rust/pkc.ql +++ b/maint/codeql/rust/pkc.ql @@ -5,7 +5,7 @@ * * @id base-sdk/pkc-rules * @name Rules for dash-pkc - * @description The arms must offer the same operations under the same names. + * @description The arms must offer the same operations and carry the same traits. * @kind problem * @precision high * @problem.severity warning @@ -77,6 +77,32 @@ predicate shapeGap(TypeItem lacks, string role, string name, string arm) { ) } -from TypeItem t, string role, string name, string arm -where shapeGap(t, role, name, arm) -select t, fmt("{0} offers {1}, {2} does not", arm + role, fmt("{0}()", name), t.getName().getText()) +/** + * Holds if `lacks` is missing `trait`, which `arm` carries for the same role + * inclusive of derives gated by `cfg_attr`. + */ +predicate traitGap(TypeItem lacks, string role, string trait, string arm) { + exists(TypeItem offers, string lacking | + armRole(offers, arm, role) and + armRole(lacks, lacking, role) and + lacking != arm and + implementsPlainTrait(offers, trait) and + not implementsPlainTrait(lacks, trait) and + not hasDerive(lacks, trait) and + not armLacksTrait(lacking, role, trait) + ) +} + +from TypeItem t, string message +where + exists(string role, string name, string arm | + shapeGap(t, role, name, arm) and + message = + fmt("{0} offers {1}, {2} does not", arm + role, fmt("{0}()", name), t.getName().getText()) + ) + or + exists(string role, string trait, string arm | + traitGap(t, role, trait, arm) and + message = fmt("{0} implements {1}, {2} does not", arm + role, trait, t.getName().getText()) + ) +select t, message diff --git a/pkgs/num/src/lib.rs b/pkgs/num/src/lib.rs index c319a6dc..ce7949e6 100644 --- a/pkgs/num/src/lib.rs +++ b/pkgs/num/src/lib.rs @@ -33,4 +33,5 @@ pub use crate::arith256::Arith256; pub use crate::compact::{CompactTarget, DecodedTarget}; pub use crate::hash::{Hash160, Hash256, HashBlob, ParseHexError}; +// TODO(kwvg): move to mod __deps with crate-level export pub use dash_types::Numeric; diff --git a/pkgs/pkc/bench/ecdsa.rs b/pkgs/pkc/bench/ecdsa.rs index f9372699..a3a0134c 100644 --- a/pkgs/pkc/bench/ecdsa.rs +++ b/pkgs/pkc/bench/ecdsa.rs @@ -30,7 +30,7 @@ fn verify(bencher: divan::Bencher) { let pk = sk.public_key(); bencher .counter(divan::counter::ItemsCount::new(1u32)) - .bench(|| pk.verify(&msg, &sig)); + .bench(|| pk.verify(&msg, sig)); } #[divan::bench] diff --git a/pkgs/pkc/src/bls/ies_ops.rs b/pkgs/pkc/src/bls/ies_ops.rs index cb5b3df1..1868e45c 100644 --- a/pkgs/pkc/src/bls/ies_ops.rs +++ b/pkgs/pkc/src/bls/ies_ops.rs @@ -141,7 +141,7 @@ impl BlsIesBlob { impl Clone for BlsIesBlob { fn clone(&self) -> Self { Self { - ephemeral_pk: self.ephemeral_pk.clone(), + ephemeral_pk: self.ephemeral_pk, iv_seed: self.iv_seed, data: self.data.clone(), } @@ -241,7 +241,7 @@ impl BlsIesMulti { /// recipient index. pub fn to_blob(&self, index: usize) -> Option> { Some(BlsIesBlob::new( - self.ephemeral_pk.clone(), + self.ephemeral_pk, self.iv_seed, self.blobs.get(index)?.clone(), )) @@ -261,7 +261,7 @@ impl BlsIesMulti { impl Clone for BlsIesMulti { fn clone(&self) -> Self { Self { - ephemeral_pk: self.ephemeral_pk.clone(), + ephemeral_pk: self.ephemeral_pk, iv_seed: self.iv_seed, blobs: self.blobs.clone(), } @@ -486,7 +486,7 @@ mod tests { /// The multi-recipient message the vectors record. fn message(&self) -> BlsIesMulti { BlsIesMulti::new( - self.eph_pk.clone(), + self.eph_pk, self.iv_seed, self.recipients.iter().map(|r| vec_from_hex(&r.ciphertext)).collect(), ) @@ -710,9 +710,9 @@ mod tests { let kat = load_kat::("ietf"); let misaligned = vec![0u8; 17]; - let blob = BlsIesBlob::new(kat.eph_pk.clone(), kat.iv_seed, misaligned.clone()); + let blob = BlsIesBlob::new(kat.eph_pk, kat.iv_seed, misaligned.clone()); assert_eq!(blob.data(), misaligned); - assert!(BlsIesMulti::new(kat.eph_pk.clone(), kat.iv_seed, vec![misaligned.clone()]).is_ok()); + assert!(BlsIesMulti::new(kat.eph_pk, kat.iv_seed, vec![misaligned.clone()]).is_ok()); let bag = BlsIesBlobBytes::new(BlsPkBytes::from(&kat.eph_pk), kat.iv_seed, misaligned.clone()); assert_eq!(BlsIesBlob::::try_from(&bag).unwrap().data(), misaligned); diff --git a/pkgs/pkc/src/bls/public_ops.rs b/pkgs/pkc/src/bls/public_ops.rs index 6b03e0a7..391a0f76 100644 --- a/pkgs/pkc/src/bls/public_ops.rs +++ b/pkgs/pkc/src/bls/public_ops.rs @@ -140,10 +140,12 @@ impl BlsPublicKey { impl Clone for BlsPublicKey { fn clone(&self) -> Self { - Self(self.0.clone()) + *self } } +impl Copy for BlsPublicKey {} + impl Debug for BlsPublicKey { fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { qtypestr(f, type_name::())?; diff --git a/pkgs/pkc/src/bls/scheme_ops.rs b/pkgs/pkc/src/bls/scheme_ops.rs index ebef4dda..bf892ab1 100644 --- a/pkgs/pkc/src/bls/scheme_ops.rs +++ b/pkgs/pkc/src/bls/scheme_ops.rs @@ -49,9 +49,9 @@ pub trait BlsScheme: BlsSchemeId + sealed::Sealed + Sized { /// Inner secret key representation. type InnerSk: Clone + Send + Sync; /// Inner public key representation. - type InnerPk: Clone + Debug + PartialEq + Eq + Send + Sync; + type InnerPk: Copy + Debug + PartialEq + Eq + Send + Sync; /// Inner signature representation. - type InnerSig: Clone + Debug + PartialEq + Eq + Send + Sync; + type InnerSig: Copy + Debug + PartialEq + Eq + Send + Sync; /// Message type accepted by signing and verification. type Msg: ?Sized; diff --git a/pkgs/pkc/src/bls/share_ops.rs b/pkgs/pkc/src/bls/share_ops.rs index 3f47c85d..692bff6b 100644 --- a/pkgs/pkc/src/bls/share_ops.rs +++ b/pkgs/pkc/src/bls/share_ops.rs @@ -100,7 +100,7 @@ impl Clone for BlsSigShare { fn clone(&self) -> Self { Self { id: self.id, - sig: self.sig.clone(), + sig: self.sig, } } } @@ -157,7 +157,7 @@ impl Clone for BlsPkShare { fn clone(&self) -> Self { Self { id: self.id, - pk: self.pk.clone(), + pk: self.pk, } } } diff --git a/pkgs/pkc/src/bls/sig_basic.rs b/pkgs/pkc/src/bls/sig_basic.rs index f8c26444..3197048c 100644 --- a/pkgs/pkc/src/bls/sig_basic.rs +++ b/pkgs/pkc/src/bls/sig_basic.rs @@ -74,10 +74,12 @@ impl BlsSignature { impl Clone for BlsSignature { fn clone(&self) -> Self { - Self(self.0.clone()) + *self } } +impl Copy for BlsSignature {} + impl Debug for BlsSignature { fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { qtypestr(f, core::any::type_name::())?; diff --git a/pkgs/pkc/src/ecdsa/public_ops.rs b/pkgs/pkc/src/ecdsa/public_ops.rs index b56ff8da..7d96b91f 100644 --- a/pkgs/pkc/src/ecdsa/public_ops.rs +++ b/pkgs/pkc/src/ecdsa/public_ops.rs @@ -44,7 +44,7 @@ pub(super) enum PkForm { } /// A secp256k1 public key. -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] #[cfg_attr(feature = "codec", derive(TypeId))] #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] #[cfg_attr(feature = "serde", serde(into = "EcdsaPkBytes", try_from = "EcdsaPkBytes"))] @@ -267,6 +267,14 @@ type_cvrt!(TryFrom for EcdsaPublicKey, EcdsaError, |bytes| { Self::from_bytes(bytes.as_bytes()) }); +type_cvrt!(From for PublicKey, |pk| { + pk.inner +}); + +type_cvrt!(From for EcdsaPublicKey, |inner| { + Self::from_inner(*inner, Compression::Compressed) +}); + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "test code")] mod tests { @@ -292,6 +300,22 @@ mod tests { pk: String, } + #[rstest] + fn backend_roundtrip_keeps_point_and_defaults_to_compressed(alice_pk: EcdsaPublicKey) { + let inner = secp256k1::PublicKey::from(&alice_pk); + assert_eq!(inner.serialize(), alice_pk.to_compressed()); + + let mut lifted = EcdsaPublicKey::from(inner); + assert!(lifted.is_compressed()); + assert_eq!(lifted, alice_pk); + lifted.decompress(); + assert_eq!( + secp256k1::PublicKey::from(&lifted), + inner, + "the form does not touch the point" + ); + } + #[rstest] fn compressed_roundtrip(alice_pk: EcdsaPublicKey) { let bytes = alice_pk.to_compressed(); @@ -413,6 +437,6 @@ mod tests { fn verify_rejects_wrong_message(alice_pk: EcdsaPublicKey, alice_sig: EcdsaSignature) { let mut bad = MSG; bad[0] ^= 0xff; - assert!(alice_pk.verify(&bad, &alice_sig).is_err()); + assert!(alice_pk.verify(&bad, alice_sig).is_err()); } } diff --git a/pkgs/pkc/src/ecdsa/secret_bytes.rs b/pkgs/pkc/src/ecdsa/secret_bytes.rs index f0595df9..4277a87b 100644 --- a/pkgs/pkc/src/ecdsa/secret_bytes.rs +++ b/pkgs/pkc/src/ecdsa/secret_bytes.rs @@ -51,26 +51,23 @@ impl EcdsaSkBytes { } #[cfg(feature = "codec")] - /// Decode a wallet import format-encoded private key. + /// Decode a wallet import format-encoded private key, returning the key + /// and the version prefix it was encoded under. /// - /// Returns `None` on a bad checksum, an unexpected version prefix, a length - /// outside 33 or 34 bytes, a malformed compression flag, or an all-zero - /// scalar. Scalars at or above the curve order still pass: range checking - /// belongs to [`EcdsaSecretKey`](crate::ecdsa::EcdsaSecretKey). - pub fn from_wif(s: &str, prefix: u8) -> Option { + /// Returns `None` on a bad checksum, a length outside 33 or 34 bytes, a + /// malformed compression flag, or an all-zero scalar. Scalars at or above the + /// curve order still pass: range checking belongs to + /// [`EcdsaSecretKey`](crate::ecdsa::EcdsaSecretKey). + pub fn from_wif(s: &str) -> Option<(Self, u8)> { let data = Zeroizing::new(decode_check(s).ok()?); - let result = match data.len() { - 33 if data[0] == prefix => { - let key: [u8; ECDSA_SK_LEN] = data[1..33].try_into().ok()?; - Some(Self::from_bytes(key, Compression::Uncompressed)) - } - 34 if data[0] == prefix && data[33] == 0x01 => { - let key: [u8; ECDSA_SK_LEN] = data[1..33].try_into().ok()?; - Some(Self::from_bytes(key, Compression::Compressed)) - } - _ => None, + let compressed = match data.len() { + 33 => Compression::Uncompressed, + 34 if data[33] == 0x01 => Compression::Compressed, + _ => return None, }; - result.filter(|sk| !sk.is_null()) + let key: [u8; ECDSA_SK_LEN] = data[1..33].try_into().ok()?; + let sk = Self::from_bytes(key, compressed); + (!sk.is_null()).then_some((sk, data[0])) } /// Copy out the raw inner bytes. @@ -149,13 +146,15 @@ mod tests { } #[rstest] - #[case::compressed(Compression::Compressed)] - #[case::uncompressed(Compression::Uncompressed)] - fn wif_roundtrip(#[case] compressed: Compression) { + #[case::compressed(Compression::Compressed, 0x80)] + #[case::uncompressed(Compression::Uncompressed, 0x80)] + #[case::other_prefix(Compression::Compressed, 0xcc)] + fn wif_roundtrip(#[case] compressed: Compression, #[case] prefix: u8) { let sk = EcdsaSkBytes::from_bytes([0x11u8; ECDSA_SK_LEN], compressed); - let wif = sk.to_wif(0x80).unwrap(); - let restored = EcdsaSkBytes::from_wif(&wif, 0x80).unwrap(); + let wif = sk.to_wif(prefix).unwrap(); + let (restored, found) = EcdsaSkBytes::from_wif(&wif).unwrap(); assert_eq!(restored, sk); + assert_eq!(found, prefix, "prefix preserved"); } /// The encoder must not emit a string the decoder refuses to read back. @@ -183,11 +182,6 @@ mod tests { base58ck::encode(&raw) } - fn wif_wrong_prefix() -> String { - let sk = EcdsaSkBytes::from_bytes([0x33u8; ECDSA_SK_LEN], Compression::Compressed); - (*sk.to_wif(0x80).unwrap()).clone() - } - fn wif_wrong_length() -> String { base58ck::encode_check(&[0x80u8; 32]) } @@ -201,12 +195,11 @@ mod tests { } #[rstest] - #[case::zero_key(wif_zero_key(), 0x80)] - #[case::bad_checksum(wif_bad_checksum(), 0x80)] - #[case::wrong_prefix(wif_wrong_prefix(), 0xef)] - #[case::wrong_length(wif_wrong_length(), 0x80)] - #[case::bad_compression_byte(wif_bad_compression_byte(), 0x80)] - fn wif_rejects(#[case] wif: String, #[case] prefix: u8) { - assert!(EcdsaSkBytes::from_wif(&wif, prefix).is_none()); + #[case::zero_key(wif_zero_key())] + #[case::bad_checksum(wif_bad_checksum())] + #[case::wrong_length(wif_wrong_length())] + #[case::bad_compression_byte(wif_bad_compression_byte())] + fn wif_rejects(#[case] wif: String) { + assert!(EcdsaSkBytes::from_wif(&wif).is_none()); } } diff --git a/pkgs/pkc/src/ecdsa/secret_ops.rs b/pkgs/pkc/src/ecdsa/secret_ops.rs index 248f44e5..7d4f11d1 100644 --- a/pkgs/pkc/src/ecdsa/secret_ops.rs +++ b/pkgs/pkc/src/ecdsa/secret_ops.rs @@ -333,6 +333,14 @@ type_cvrt!(TryFrom for EcdsaSecretKey, EcdsaError, |bytes| { Self::from_bytes(bytes.as_bytes(), Compression::from(bytes.is_compressed())) }); +type_cvrt!(From for SecretKey, |sk| { + sk.inner +}); + +type_cvrt!(From for EcdsaSecretKey, |inner| { + Self::from_inner(*inner, Compression::Compressed) +}); + #[cfg(test)] #[expect(clippy::ptr_arg, clippy::unwrap_used, reason = "test code")] mod tests { @@ -605,7 +613,7 @@ mod tests { #[rstest] fn sign_verify_roundtrip(alice_sk: EcdsaSecretKey) { let sig = alice_sk.sign(&MSG); - assert!(alice_sk.public_key().verify(&MSG, &sig).is_ok()); + assert!(alice_sk.public_key().verify(&MSG, sig).is_ok()); } #[rstest] @@ -624,6 +632,17 @@ mod tests { fn verify_rejects_wrong_key(alice_sk: EcdsaSecretKey, bob_sk: EcdsaSecretKey) { assert!(!alice_sk.verify_pubkey(&bob_sk.public_key())); let sig = alice_sk.sign(&MSG); - assert!(bob_sk.public_key().verify(&MSG, &sig).is_err()); + assert!(bob_sk.public_key().verify(&MSG, sig).is_err()); + } + + #[rstest] + fn backend_roundtrip_keeps_scalar_and_defaults_to_compressed(alice_sk: EcdsaSecretKey) { + let inner = secp256k1::SecretKey::from(&alice_sk); + assert_eq!(inner.to_secret_bytes(), *alice_sk.to_bytes()); + + let lifted = EcdsaSecretKey::from(inner); + assert!(lifted.is_compressed()); + assert_eq!(lifted, alice_sk); + assert!(lifted.verify_pubkey(&alice_sk.public_key())); } } diff --git a/pkgs/pkc/src/ecdsa/sig_ops.rs b/pkgs/pkc/src/ecdsa/sig_ops.rs index ad207f22..5d3497e1 100644 --- a/pkgs/pkc/src/ecdsa/sig_ops.rs +++ b/pkgs/pkc/src/ecdsa/sig_ops.rs @@ -22,7 +22,7 @@ use secp256k1::ecdsa::{SerializedSignature, Signature}; use core::hash::{Hash, Hasher}; /// An ECDSA signature (64-byte compact r||s). -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] #[cfg_attr(feature = "codec", derive(TypeId))] #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] #[cfg_attr( @@ -159,6 +159,14 @@ type_cvrt!(TryFrom for EcdsaSignature, EcdsaError, |bytes| { Self::from_bytes(bytes.as_bytes()) }); +type_cvrt!(From for Signature, |sig| { + sig.0 +}); + +type_cvrt!(From for EcdsaSignature, |inner| { + Self(*inner) +}); + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "test code")] mod tests { @@ -220,7 +228,7 @@ mod tests { let normalized = high_sig.normalize_s().unwrap(); assert!(normalized.is_low_s()); assert_eq!(normalized, alice_sig); - assert!(alice_pk.verify(&MSG, &normalized).is_ok()); + assert!(alice_pk.verify(&MSG, normalized).is_ok()); } #[cfg(feature = "serde")] @@ -228,4 +236,11 @@ mod tests { fn serde_sig_roundtrip(alice_sig: EcdsaSignature) { assert_json_rt(&alice_sig); } + + #[rstest] + fn backend_roundtrip(alice_sig: EcdsaSignature) { + let inner = secp256k1::ecdsa::Signature::from(&alice_sig); + assert_eq!(inner.serialize_compact(), alice_sig.to_bytes()); + assert_eq!(EcdsaSignature::from(inner), alice_sig); + } } diff --git a/pkgs/pkc/src/ecdsa/sig_rec_ops.rs b/pkgs/pkc/src/ecdsa/sig_rec_ops.rs index e7193b84..076f7b9e 100644 --- a/pkgs/pkc/src/ecdsa/sig_rec_ops.rs +++ b/pkgs/pkc/src/ecdsa/sig_rec_ops.rs @@ -17,10 +17,10 @@ use dash_num::Hash256; use dash_types::type_cvrt; #[cfg(feature = "codec")] use dash_types::{dlgt_codec, type_id::TypeId}; -use secp256k1::ecdsa::{RecoveryId, Signature}; +use secp256k1::ecdsa::{RecoverableSignature, RecoveryId, Signature}; /// An ECDSA signature with recovery id and compression metadata. -#[derive(Clone, Debug, Eq, Hash, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] #[cfg_attr(feature = "codec", derive(TypeId))] #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] #[cfg_attr(feature = "serde", serde(into = "EcdsaRecSigBytes", try_from = "EcdsaRecSigBytes"))] @@ -104,7 +104,7 @@ type_cvrt!(From for EcdsaRecSigBytes, |rec| { }); type_cvrt!(From for EcdsaSignature, |rec| { - rec.signature().clone() + *rec.signature() }); type_cvrt!(TryFrom for EcdsaRecSignature, EcdsaError, |bytes| { @@ -114,11 +114,26 @@ type_cvrt!(TryFrom for EcdsaRecSignature, EcdsaError, |bytes| }) }); +type_cvrt!(TryFrom for RecoverableSignature, EcdsaError, |rec| { + RecoverableSignature::from_compact(&rec.to_compact(), rec.backend_recovery_id()) + .map_err(|_| EcdsaError::InvalidSignature) +}); + +// The backend type holds only the scalars and the recovery id, the compression +// flag doesn't make it through and it is assumed to be compressed. To preserve +// the compression flag, use `from_parts`. +type_cvrt!(From for EcdsaRecSignature, |rec| { + let (rid, _) = rec.serialize_compact(); + Self::from_inner(rec.to_standard(), rid, Compression::Compressed) +}); + #[cfg(test)] #[expect(clippy::unwrap_used, reason = "test code")] mod tests { use crate::ecdsa::tests::*; - use crate::ecdsa::{Compression, EcdsaPublicKey, EcdsaRecSigBytes, EcdsaRecSignature, EcdsaSigBytes, EcdsaSignature}; + use crate::ecdsa::{ + Compression, EcdsaPublicKey, EcdsaRecSigBytes, EcdsaRecSignature, EcdsaSecretKey, EcdsaSigBytes, EcdsaSignature, + }; #[cfg(feature = "serde")] use dash_dev::assert_json_rt; @@ -147,14 +162,14 @@ mod tests { fn conversions_commute(alice_rec_sig: EcdsaRecSignature) { // Both paths to the plain bag must agree: drop metadata then serialize, or // serialize then strip the header. - let via_ops = EcdsaSigBytes::from(EcdsaSignature::from(alice_rec_sig.clone())); + let via_ops = EcdsaSigBytes::from(EcdsaSignature::from(alice_rec_sig)); let via_bag = EcdsaSigBytes::from(EcdsaRecSigBytes::from(&alice_rec_sig)); assert_eq!(via_ops, via_bag); } #[rstest] fn from_parts_rejects_out_of_range_id(alice_sig: EcdsaSignature) { - assert!(EcdsaRecSignature::from_parts(alice_sig.clone(), 4, Compression::Compressed).is_err()); + assert!(EcdsaRecSignature::from_parts(alice_sig, 4, Compression::Compressed).is_err()); assert!(EcdsaRecSignature::from_parts(alice_sig, 255, Compression::Compressed).is_err()); } @@ -201,7 +216,7 @@ mod tests { #[rstest] fn verifies_without_downcast(alice_pk: EcdsaPublicKey, alice_rec_sig: EcdsaRecSignature) { - assert!(alice_pk.verify(&MSG, &alice_rec_sig).is_ok()); + assert!(alice_pk.verify(&MSG, alice_rec_sig).is_ok()); assert!(alice_pk.verify(&MSG, alice_rec_sig.signature()).is_ok()); } @@ -210,4 +225,33 @@ mod tests { fn serde_roundtrip(alice_rec_sig: EcdsaRecSignature) { assert_json_rt(&alice_rec_sig); } + + #[rstest] + fn uncompressed_recsig_keeps_flag() { + let sk = EcdsaSecretKey::from_bytes(&ALICE_SK, Compression::Uncompressed).unwrap(); + let rec = sk.sign_recoverable(&MSG); + assert!(!rec.is_compressed()); + + // Recovery carries the flag onto the key it rebuilds. + let pk = EcdsaPublicKey::recover(&MSG, &rec).unwrap(); + assert!(!pk.is_compressed()); + assert_eq!(pk, sk.public_key()); + + // The backend type doesn't store the flag, so the round trip comes back + // compressed. + let inner = secp256k1::ecdsa::RecoverableSignature::try_from(&rec).unwrap(); + let back = EcdsaRecSignature::from(inner); + assert_eq!(back.signature(), rec.signature()); + assert_eq!(back.recovery_id(), rec.recovery_id()); + assert!(back.is_compressed()); + } + + #[rstest] + fn backend_roundtrip_keeps_recovery_id(alice_rec_sig: EcdsaRecSignature) { + let inner = secp256k1::ecdsa::RecoverableSignature::try_from(&alice_rec_sig).unwrap(); + let (rid, compact) = inner.serialize_compact(); + assert_eq!(compact, alice_rec_sig.to_compact()); + assert_eq!(rid.to_u8(), alice_rec_sig.recovery_id()); + assert_eq!(EcdsaRecSignature::from(inner), alice_rec_sig); + } } diff --git a/pkgs/pkc/src/lib.rs b/pkgs/pkc/src/lib.rs index 26938faf..eeae04c7 100644 --- a/pkgs/pkc/src/lib.rs +++ b/pkgs/pkc/src/lib.rs @@ -20,6 +20,25 @@ mod prelude; pub mod bls; pub mod ecdsa; +#[doc(hidden)] +pub mod __deps { + #[cfg(feature = "bls")] + pub use blst; + pub use dash_num; + pub use dash_types; + #[cfg(feature = "bls")] + pub use ff; + #[cfg(feature = "bls")] + pub use group; + #[cfg(any(feature = "bls", feature = "ecdsa"))] + pub use rand_core; + #[cfg(feature = "ecdsa")] + pub use secp256k1; + #[cfg(feature = "bls")] + pub use subtle; + pub use zeroize; +} + #[cfg(feature = "codec")] #[doc(hidden)] pub mod __private { diff --git a/pkgs/types/src/lib.rs b/pkgs/types/src/lib.rs index fb283584..d09a19a3 100644 --- a/pkgs/types/src/lib.rs +++ b/pkgs/types/src/lib.rs @@ -28,6 +28,7 @@ pub use crate::macros::qtypestr; pub use crate::numeric::Numeric; pub use crate::traits::{Checkable, Hashable}; +// TODO(kwvg): move to mod __deps pub use zeroize; cfg_if::cfg_if! {