From bbdb80b07d01043db7d3af0f58b1472cbe5a2864 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 17 Sep 2026 18:20:19 +0530 Subject: [PATCH 1/7] pkc%lint(codeql): track traits alongside method parity in `pkc` APIs --- maint/codeql/rust/lib/traits.qll | 26 +++++++++++++++++++++++++ maint/codeql/rust/pkc.ql | 33 ++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 4 deletions(-) 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.ql b/maint/codeql/rust/pkc.ql index 0740f954..57746e4b 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,31 @@ 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) + ) +} + +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 From 639a7e064d57954b4779aed44c84e311455af9c7 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 17 Sep 2026 18:20:56 +0530 Subject: [PATCH 2/7] pkc%lint(codeql): introduce suppression list for unmatched traits --- maint/codeql/rust/lib/pkc.qll | 10 ++++++---- maint/codeql/rust/pkc.model.yml | 4 ++++ maint/codeql/rust/pkc.ql | 3 ++- 3 files changed, 12 insertions(+), 5 deletions(-) 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/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 57746e4b..fb91c227 100644 --- a/maint/codeql/rust/pkc.ql +++ b/maint/codeql/rust/pkc.ql @@ -88,7 +88,8 @@ predicate traitGap(TypeItem lacks, string role, string trait, string arm) { lacking != arm and implementsPlainTrait(offers, trait) and not implementsPlainTrait(lacks, trait) and - not hasDerive(lacks, trait) + not hasDerive(lacks, trait) and + not armLacksTrait(lacking, role, trait) ) } From cf4c1283c914879555a7040608fbae37bc314abd Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:50:07 +0530 Subject: [PATCH 3/7] pkc%feat(ecdsa): derive `Copy` for `Ecdsa{PublicKey,Signature}` --- maint/codeql/rust/pkc.model.yml | 4 +++- pkgs/pkc/bench/ecdsa.rs | 2 +- pkgs/pkc/src/ecdsa/public_ops.rs | 4 ++-- pkgs/pkc/src/ecdsa/secret_ops.rs | 4 ++-- pkgs/pkc/src/ecdsa/sig_ops.rs | 4 ++-- pkgs/pkc/src/ecdsa/sig_rec_ops.rs | 10 +++++----- 6 files changed, 15 insertions(+), 13 deletions(-) diff --git a/maint/codeql/rust/pkc.model.yml b/maint/codeql/rust/pkc.model.yml index 4c9342c0..75af2e34 100644 --- a/maint/codeql/rust/pkc.model.yml +++ b/maint/codeql/rust/pkc.model.yml @@ -67,4 +67,6 @@ extensions: - addsTo: pack: base-sdk/codeql-rust extensible: armLacksTrait - data: [] + data: + - ["Bls", "PublicKey", "Copy"] + - ["Bls", "Signature", "Copy"] 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/ecdsa/public_ops.rs b/pkgs/pkc/src/ecdsa/public_ops.rs index b56ff8da..50247a7c 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"))] @@ -413,6 +413,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_ops.rs b/pkgs/pkc/src/ecdsa/secret_ops.rs index 248f44e5..6416f186 100644 --- a/pkgs/pkc/src/ecdsa/secret_ops.rs +++ b/pkgs/pkc/src/ecdsa/secret_ops.rs @@ -605,7 +605,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 +624,6 @@ 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()); } } diff --git a/pkgs/pkc/src/ecdsa/sig_ops.rs b/pkgs/pkc/src/ecdsa/sig_ops.rs index ad207f22..46e76b24 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( @@ -220,7 +220,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")] diff --git a/pkgs/pkc/src/ecdsa/sig_rec_ops.rs b/pkgs/pkc/src/ecdsa/sig_rec_ops.rs index e7193b84..2648484b 100644 --- a/pkgs/pkc/src/ecdsa/sig_rec_ops.rs +++ b/pkgs/pkc/src/ecdsa/sig_rec_ops.rs @@ -20,7 +20,7 @@ use dash_types::{dlgt_codec, type_id::TypeId}; use secp256k1::ecdsa::{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| { @@ -147,14 +147,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 +201,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()); } From 2d4ab0f285ae018ac9065d1b192a6bc5aac28362 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:53:15 +0530 Subject: [PATCH 4/7] pkc%feat(bls): derive `Copy` for `Bls{PublicKey,Signature}` --- maint/codeql/rust/pkc.model.yml | 4 +--- pkgs/pkc/src/bls/ies_ops.rs | 12 ++++++------ pkgs/pkc/src/bls/public_ops.rs | 4 +++- pkgs/pkc/src/bls/scheme_ops.rs | 4 ++-- pkgs/pkc/src/bls/share_ops.rs | 4 ++-- pkgs/pkc/src/bls/sig_basic.rs | 4 +++- 6 files changed, 17 insertions(+), 15 deletions(-) diff --git a/maint/codeql/rust/pkc.model.yml b/maint/codeql/rust/pkc.model.yml index 75af2e34..4c9342c0 100644 --- a/maint/codeql/rust/pkc.model.yml +++ b/maint/codeql/rust/pkc.model.yml @@ -67,6 +67,4 @@ extensions: - addsTo: pack: base-sdk/codeql-rust extensible: armLacksTrait - data: - - ["Bls", "PublicKey", "Copy"] - - ["Bls", "Signature", "Copy"] + data: [] 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::())?; From 358d7a881c7b6a1b7563e866a12bc5c94f0f359b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:55:27 +0530 Subject: [PATCH 5/7] pkc%chore: re-export crates surfaced by the public API --- maint/codeql/rust/lib/imports.qll | 20 ++++++++++++++++++++ pkgs/num/src/lib.rs | 1 + pkgs/pkc/src/lib.rs | 17 +++++++++++++++++ pkgs/types/src/lib.rs | 1 + 4 files changed, 39 insertions(+) diff --git a/maint/codeql/rust/lib/imports.qll b/maint/codeql/rust/lib/imports.qll index 0aa7365e..cf8c747b 100644 --- a/maint/codeql/rust/lib/imports.qll +++ b/maint/codeql/rust/lib/imports.qll @@ -74,6 +74,26 @@ 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 + 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) = "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/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/src/lib.rs b/pkgs/pkc/src/lib.rs index 26938faf..9d982e14 100644 --- a/pkgs/pkc/src/lib.rs +++ b/pkgs/pkc/src/lib.rs @@ -20,6 +20,23 @@ 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 = "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! { From 8b73c47e1ca7de0024e7d2b36b3faa0a043d8b18 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:57:13 +0530 Subject: [PATCH 6/7] pkc%feat(ecdsa): define conversions to/from underlying types --- maint/codeql/rust/lib/imports.qll | 22 ++++++++------ pkgs/pkc/src/ecdsa/public_ops.rs | 24 ++++++++++++++++ pkgs/pkc/src/ecdsa/secret_ops.rs | 19 ++++++++++++ pkgs/pkc/src/ecdsa/sig_ops.rs | 15 ++++++++++ pkgs/pkc/src/ecdsa/sig_rec_ops.rs | 48 +++++++++++++++++++++++++++++-- pkgs/pkc/src/lib.rs | 2 ++ 6 files changed, 120 insertions(+), 10 deletions(-) diff --git a/maint/codeql/rust/lib/imports.qll b/maint/codeql/rust/lib/imports.qll index cf8c747b..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) { @@ -75,8 +78,9 @@ private predicate isAllowlistedReexport(Use u) { ) 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 ( - // Crate emits types relying on types or traits defined by a dependency, part of public API usePrefix(u) = "blst" or usePrefix(u) = "dash_num" @@ -89,6 +93,8 @@ private predicate isAllowlistedReexport(Use u) { or usePrefix(u) = "rand_core" or + usePrefix(u) = "secp256k1" + or usePrefix(u) = "subtle" or usePrefix(u) = "zeroize" diff --git a/pkgs/pkc/src/ecdsa/public_ops.rs b/pkgs/pkc/src/ecdsa/public_ops.rs index 50247a7c..7d96b91f 100644 --- a/pkgs/pkc/src/ecdsa/public_ops.rs +++ b/pkgs/pkc/src/ecdsa/public_ops.rs @@ -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(); diff --git a/pkgs/pkc/src/ecdsa/secret_ops.rs b/pkgs/pkc/src/ecdsa/secret_ops.rs index 6416f186..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 { @@ -626,4 +634,15 @@ mod tests { let sig = alice_sk.sign(&MSG); 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 46e76b24..5d3497e1 100644 --- a/pkgs/pkc/src/ecdsa/sig_ops.rs +++ b/pkgs/pkc/src/ecdsa/sig_ops.rs @@ -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 { @@ -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 2648484b..076f7b9e 100644 --- a/pkgs/pkc/src/ecdsa/sig_rec_ops.rs +++ b/pkgs/pkc/src/ecdsa/sig_rec_ops.rs @@ -17,7 +17,7 @@ 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, Copy, Debug, Eq, Hash, PartialEq)] @@ -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; @@ -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 9d982e14..eeae04c7 100644 --- a/pkgs/pkc/src/lib.rs +++ b/pkgs/pkc/src/lib.rs @@ -32,6 +32,8 @@ pub mod __deps { 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; From 98e0c14585900e582535f311a7949718358e75d4 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 17 Sep 2026 18:00:12 +0530 Subject: [PATCH 7/7] pkc%refac(ecdsa): return version prefix with `from_wif` --- pkgs/pkc/src/ecdsa/secret_bytes.rs | 61 +++++++++++++----------------- 1 file changed, 27 insertions(+), 34 deletions(-) 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()); } }