Merge pull request #4994 from namada-net/grarco/pubkeys-for-mock-sigs

Public keys when mocking signatures

Author: mergify[bot]

.changelog/unreleased/SDK/4994-pubkeys-for-mock-sigs.md

@@ -0,0 +1,3 @@
+- Changed the SDK signing functions to accept a public key instead of a
+  private one when mocking a signature. ([\#4994](https://github.com/namada-
+  net/namada/pull/4994))

crates/account/src/auth.rs

@@ -6,6 +6,7 @@ use borsh::{BorshDeserialize, BorshSerialize};
 use namada_core::collections::HashMap;
 use namada_core::hints;
 use namada_core::key::common;
+use namada_core::key::common::SigOrPubKey;
 use namada_macros::BorshDeserializer;
 #[cfg(feature = "migrations")]
 use namada_migrations::*;
@@ -77,16 +78,15 @@ impl AccountPublicKeysMap {
         self.pk_to_idx.get(public_key).cloned()
     }
 
-    /// Index the given set of secret keys
-    pub fn index_secret_keys(
-        &self,
-        secret_keys: Vec<common::SecretKey>,
-    ) -> BTreeMap<u8, common::SecretKey> {
-        secret_keys
-            .into_iter()
-            .filter_map(|secret_key: common::SecretKey| {
-                self.get_index_from_public_key(&secret_key.to_public())
-                    .map(|index| (index, secret_key))
+    /// Index the given set of keys
+    pub fn index_keys<KEY>(&self, keys: Vec<KEY>) -> BTreeMap<u8, KEY>
+    where
+        KEY: SigOrPubKey,
+    {
+        keys.into_iter()
+            .filter_map(|key| {
+                self.get_index_from_public_key(&key.pubkey())
+                    .map(|index| (index, key))
             })
             .collect()
     }

crates/benches/host_env.rs

@@ -41,7 +41,7 @@ fn tx_section_signature_validation(c: &mut Criterion) {
 
     let multisig = Authorization::new(
         vec![section_hash],
-        pkim.index_secret_keys(vec![defaults::albert_keypair()]),
+        pkim.index_keys(vec![defaults::albert_keypair()]),
         None,
     );
 

crates/core/src/key/common.rs

@@ -517,18 +517,68 @@ impl super::SigScheme for SigScheme {
         }
     }
 
-    fn mock(keypair: &SecretKey) -> Self::Signature {
-        match keypair {
-            SecretKey::Ed25519(kp) => {
-                Signature::Ed25519(ed25519::SigScheme::mock(kp))
+    fn mock(pubkey: &Self::PublicKey) -> Self::Signature {
+        match pubkey {
+            PublicKey::Ed25519(pk) => {
+                Signature::Ed25519(ed25519::SigScheme::mock(pk))
             }
-            SecretKey::Secp256k1(kp) => {
-                Signature::Secp256k1(secp256k1::SigScheme::mock(kp))
+            PublicKey::Secp256k1(pk) => {
+                Signature::Secp256k1(secp256k1::SigScheme::mock(pk))
             }
         }
     }
 }
 
+/// Share behavior for both private and public keys. Useful to dispatch function
+/// calls when mocking signatures
+pub trait SigOrPubKey {
+    /// Get the public key
+    fn pubkey(&self) -> PublicKey;
+
+    /// Get the private key if possible ([`None`] if the underlying type is a
+    /// public key)
+    fn sigkey(&self) -> Option<SecretKey>;
+
+    /// Produce a signature (either a valid or a mock one)
+    fn sign(
+        &self,
+        data: impl SignableBytes,
+    ) -> <SigScheme as super::SigScheme>::Signature;
+}
+
+impl SigOrPubKey for SecretKey {
+    fn pubkey(&self) -> PublicKey {
+        self.ref_to()
+    }
+
+    fn sigkey(&self) -> Option<SecretKey> {
+        Some(self.to_owned())
+    }
+
+    fn sign(
+        &self,
+        data: impl SignableBytes,
+    ) -> <SigScheme as super::SigScheme>::Signature {
+        SigScheme::sign(self, data)
+    }
+}
+impl SigOrPubKey for PublicKey {
+    fn pubkey(&self) -> PublicKey {
+        self.to_owned()
+    }
+
+    fn sigkey(&self) -> Option<SecretKey> {
+        None
+    }
+
+    fn sign(
+        &self,
+        _: impl SignableBytes,
+    ) -> <SigScheme as super::SigScheme>::Signature {
+        SigScheme::mock(self)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/core/src/key/ed25519.rs

@@ -403,7 +403,7 @@ impl super::SigScheme for SigScheme {
         }
     }
 
-    fn mock(_: &Self::SecretKey) -> Self::Signature {
+    fn mock(_: &Self::PublicKey) -> Self::Signature {
         Signature(ed25519_consensus::Signature::from([0; 64]))
     }
 }

crates/core/src/key/mod.rs

@@ -270,7 +270,7 @@ pub trait SigScheme: Eq + Ord + Debug + Serialize + Default {
     }
 
     /// Provide a dummy signature
-    fn mock(keypair: &Self::SecretKey) -> Self::Signature;
+    fn mock(pubkey: &Self::PublicKey) -> Self::Signature;
 }
 
 /// Public key hash derived from `common::Key` borsh encoded bytes (hex string

crates/core/src/key/secp256k1.rs

@@ -603,7 +603,7 @@ impl super::SigScheme for SigScheme {
         }
     }
 
-    fn mock(_: &Self::SecretKey) -> Self::Signature {
+    fn mock(_: &Self::PublicKey) -> Self::Signature {
         Signature(
             k256::ecdsa::Signature::from_scalars(
                 k256::Scalar::ONE,

crates/sdk/src/signing.rs

@@ -17,7 +17,6 @@ use namada_core::address::{Address, ImplicitAddress, MASP};
 use namada_core::arith::checked;
 use namada_core::collections::{HashMap, HashSet};
 use namada_core::ibc::primitives::IntoHostTime;
-use namada_core::key::common::{CommonPublicKey, CommonSignature};
 use namada_core::key::*;
 use namada_core::masp::{AssetData, MaspTxId, PaymentAddress};
 use namada_core::tendermint::Time as TmTime;
@@ -38,9 +37,7 @@ use namada_token::storage_key::balance_key;
 use namada_tx::data::pgf::UpdateStewardCommission;
 use namada_tx::data::pos;
 use namada_tx::data::pos::BecomeValidator;
-use namada_tx::{
-    Authorization, MaspBuilder, Section, SignatureIndex, Signer, Tx,
-};
+use namada_tx::{Authorization, MaspBuilder, Section, SignatureIndex, Tx};
 use rand::rngs::OsRng;
 use serde::{Deserialize, Serialize};
 use tokio::sync::RwLock;
@@ -277,42 +274,6 @@ pub async fn default_sign(
     )))
 }
 
-/// When mocking signatures, if a key is in a hardware wallet,
-/// use this function instead to mock the signature
-fn mock_hw_sig(tx: &mut Tx, pk: common::PublicKey, signable: Signable) {
-    let targets = match signable {
-        Signable::FeeRawHeader => {
-            vec![tx.sechashes(), vec![tx.raw_header_hash()]]
-        }
-        Signable::RawHeader => vec![vec![tx.raw_header_hash()]],
-    };
-    tx.protocol_filter();
-    let sig = match pk {
-        CommonPublicKey::Ed25519(_) => {
-            // this key has no effect other than to satisfy api constraints
-            let kp = ed25519::SigScheme::from_bytes([0; 32]);
-            CommonSignature::Ed25519(ed25519::SigScheme::mock(&kp))
-        }
-        CommonPublicKey::Secp256k1(_) => {
-            // this key has no effect other than to satisfy api constraints
-            const SECRET_KEY_HEX: &str = "c2c72dfbff11dfb4e9d5b0a20c620c58b15bb7552753601f043db91331b0db15";
-            let sk_bytes = HEXLOWER.decode(SECRET_KEY_HEX.as_bytes()).unwrap();
-            let kp =
-                secp256k1::SecretKey::try_from_slice(&sk_bytes[..]).unwrap();
-            CommonSignature::Secp256k1(secp256k1::SigScheme::mock(&kp))
-        }
-    };
-    for t in targets {
-        let mut signatures = BTreeMap::new();
-        signatures.insert(0, sig.clone());
-        tx.add_section(Section::Authorization(Authorization {
-            targets: t,
-            signer: Signer::PubKeys(vec![pk.clone()]),
-            signatures,
-        }));
-    }
-}
-
 /// Sign a transaction with a given signing key or public key of a given signer.
 /// If no explicit signer given, use the `default`. If no `default` is given,
 /// Error.
@@ -361,40 +322,55 @@ where
             signing_tx_data.account_public_keys_map
         {
             let mut wallet = wallet.write().await;
-            let mut signing_tx_keypairs = vec![];
-
-            for public_key in &signing_tx_data.public_keys {
-                if !used_pubkeys.contains(public_key) {
-                    let Ok(secret_key) =
-                        find_key_by_pk(&mut wallet, args, public_key)
-                    else {
-                        // If the secret key is not found, continue because the
-                        // hardware wallet may still be able to sign this
-                        continue;
-                    };
-                    used_pubkeys.insert(public_key.clone());
-                    signing_tx_keypairs.push(secret_key);
+
+            if args.dry_run.is_some() {
+                // For dry-runs produce mock signatures
+                let mut signing_tx_pubkeys = vec![];
+
+                for public_key in &signing_tx_data.public_keys {
+                    if !used_pubkeys.contains(public_key) {
+                        used_pubkeys.insert(public_key.clone());
+                        signing_tx_pubkeys.push(public_key.to_owned());
+                    }
                 }
-            }
 
-            if !signing_tx_keypairs.is_empty() {
-                if args.dry_run.is_some() {
+                if !signing_tx_pubkeys.is_empty() {
                     tx.mock(
-                        signing_tx_keypairs,
+                        signing_tx_pubkeys,
                         account_public_keys_map,
                         signing_tx_data.owner,
-                    )
-                } else {
+                    );
+                }
+            } else {
+                let mut signing_tx_keypairs = vec![];
+
+                for public_key in &signing_tx_data.public_keys {
+                    if !used_pubkeys.contains(public_key) {
+                        let Ok(secret_key) =
+                            find_key_by_pk(&mut wallet, args, public_key)
+                        else {
+                            // If the secret key is not found, continue because
+                            // the hardware wallet
+                            // may still be able to sign this
+                            continue;
+                        };
+                        used_pubkeys.insert(public_key.clone());
+                        signing_tx_keypairs.push(secret_key);
+                    }
+                }
+
+                if !signing_tx_keypairs.is_empty() {
                     tx.sign_raw(
                         signing_tx_keypairs,
                         account_public_keys_map,
                         signing_tx_data.owner,
-                    )
-                };
+                    );
+                }
             }
         }
 
-        // Then try to sign the raw header using the hardware wallet
+        // Then try to sign the raw header using the hardware wallet (only if
+        // this is not a dry-run requiring mock signatures)
         for pubkey in &signing_tx_data.public_keys {
             if !used_pubkeys.contains(pubkey) {
                 match &fee_auth {
@@ -407,14 +383,10 @@ where
                         used_pubkeys.insert(pubkey.clone());
                     }
                     _ => {
-                        if args.dry_run.is_some() {
-                            mock_hw_sig(
-                                tx,
-                                pubkey.clone(),
-                                Signable::RawHeader,
-                            );
-                            used_pubkeys.insert(pubkey.clone());
-                        } else if let Ok(ntx) = sign(
+                        // No need to account for mock signatures when dealing
+                        // with the hardware wallet as those are produced fully
+                        // in software here above
+                        if let Ok(ntx) = sign(
                             tx.clone(),
                             pubkey.clone(),
                             Signable::RawHeader,
@@ -456,25 +428,22 @@ where
             pubkey,
             disposable_fee_payer: _,
         }) => {
-            let key = {
-                // Lock the wallet just long enough to extract a key from it
-                // without interfering with the sign closure
-                // call
-                let mut wallet = wallet.write().await;
-                find_key_by_pk(&mut *wallet, args, pubkey)
-            };
-            match key {
-                Ok(fee_payer_keypair) => {
-                    if args.dry_run.is_some() {
-                        tx.mock_sign_wrapper(fee_payer_keypair);
-                    } else {
+            if args.dry_run.is_some() {
+                tx.mock_sign_wrapper(pubkey.to_owned());
+            } else {
+                let key = {
+                    // Lock the wallet just long enough to extract a key from it
+                    // without interfering with the sign closure
+                    // call
+                    let mut wallet = wallet.write().await;
+                    find_key_by_pk(&mut *wallet, args, pubkey)
+                };
+
+                match key {
+                    Ok(fee_payer_keypair) => {
                         tx.sign_wrapper(fee_payer_keypair);
                     }
-                }
-                Err(_) => {
-                    if args.dry_run.is_some() {
-                        mock_hw_sig(tx, pubkey.clone(), Signable::FeeRawHeader);
-                    } else {
+                    Err(_) => {
                         *tx = sign(
                             tx.clone(),
                             pubkey.clone(),