Merge pull request #2267 from subspace/signing_key_index

Disallow operator registration with same operator signing key

crates/pallet-domains/src/lib.rs

@@ -27,6 +27,7 @@ mod tests;
 
 pub mod block_tree;
 pub mod domain_registry;
+pub mod migrations;
 pub mod runtime_registry;
 mod staking;
 mod staking_epoch;
@@ -110,7 +111,7 @@ pub type DomainHashingFor<T> = <<T as Config>::DomainHeader as Header>::Hashing;
 pub type ReceiptHashFor<T> = <<T as Config>::DomainHeader as Header>::Hash;
 
 /// The current storage version.
-const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);
 
 #[frame_support::pallet]
 mod pallet {
@@ -161,7 +162,7 @@ mod pallet {
     use sp_domains::bundle_producer_election::ProofOfElectionError;
     use sp_domains::{
         BundleDigest, DomainId, EpochIndex, GenesisDomain, OperatorAllowList, OperatorId,
-        RuntimeId, RuntimeType,
+        OperatorPublicKey, RuntimeId, RuntimeType,
     };
     use sp_domains_fraud_proof::fraud_proof::FraudProof;
     use sp_domains_fraud_proof::InvalidTransactionCode;
@@ -356,6 +357,13 @@ mod pallet {
     pub(super) type OperatorIdOwner<T: Config> =
         StorageMap<_, Identity, OperatorId, T::AccountId, OptionQuery>;
 
+    /// Indexes operator signing key against OperatorId.
+    // TODO: remove BTreeSet with single operatorId before next network
+    //  since there are multiple operators registered with same signing key in gemini-3g
+    #[pallet::storage]
+    pub(super) type OperatorSigningKey<T: Config> =
+        StorageMap<_, Identity, OperatorPublicKey, BTreeSet<OperatorId>, OptionQuery>;
+
     #[pallet::storage]
     pub(super) type DomainStakingSummary<T: Config> =
         StorageMap<_, Identity, DomainId, StakingSummary<OperatorId, BalanceOf<T>>, OptionQuery>;

crates/pallet-domains/src/migrations.rs

@@ -0,0 +1,92 @@
+//! Migration module for pallet-domains
+
+use crate::pallet::{OperatorSigningKey, Operators};
+use crate::Config;
+use frame_support::traits::OnRuntimeUpgrade;
+use frame_support::weights::Weight;
+use sp_core::Get;
+
+pub struct VersionUncheckedMigrateV1ToV2<T>(sp_std::marker::PhantomData<T>);
+impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateV1ToV2<T> {
+    fn on_runtime_upgrade() -> Weight {
+        index_operator_signing_keys::<T>()
+    }
+}
+
+/// Indexes the currently used operator's signing keys into
+/// newly introduced storage.
+fn index_operator_signing_keys<T: Config>() -> Weight {
+    let mut count = 0;
+    Operators::<T>::iter().for_each(|(operator_id, operator)| {
+        count += 1;
+        OperatorSigningKey::<T>::append(operator.signing_key, operator_id)
+    });
+
+    T::DbWeight::get().reads_writes(count, count)
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::migrations::index_operator_signing_keys;
+    use crate::pallet::{OperatorSigningKey, Operators};
+    use crate::staking::{Operator, OperatorStatus};
+    use crate::tests::{new_test_ext, Test};
+    use sp_core::{Pair, U256};
+    use sp_domains::OperatorPair;
+    use std::collections::BTreeSet;
+    use subspace_runtime_primitives::{Balance, SSC};
+
+    #[test]
+    fn test_index_operator_signing_keys() {
+        let mut ext = new_test_ext();
+        let create_operator = |signing_key| -> Operator<Balance, Balance> {
+            Operator {
+                signing_key,
+                current_domain_id: Default::default(),
+                next_domain_id: Default::default(),
+                minimum_nominator_stake: 100 * SSC,
+                nomination_tax: Default::default(),
+                current_total_stake: 100 * SSC,
+                current_epoch_rewards: Default::default(),
+                total_shares: Default::default(),
+                status: OperatorStatus::Registered,
+            }
+        };
+
+        let pair_1 = OperatorPair::from_seed(&U256::from(0u32).into());
+        let pair_2 = OperatorPair::from_seed(&U256::from(1u32).into());
+
+        ext.execute_with(|| {
+            // operator uses pair_1
+            Operators::<Test>::insert(1, create_operator(pair_1.public()));
+
+            // operator uses pair_2
+            Operators::<Test>::insert(2, create_operator(pair_2.public()));
+
+            // operator uses pair_2
+            Operators::<Test>::insert(3, create_operator(pair_2.public()));
+
+            assert!(!OperatorSigningKey::<Test>::contains_key(pair_1.public()));
+            assert!(!OperatorSigningKey::<Test>::contains_key(pair_2.public()));
+        });
+
+        ext.commit_all().unwrap();
+
+        ext.execute_with(|| {
+            let weights = index_operator_signing_keys::<Test>();
+            assert_eq!(
+                weights,
+                <Test as frame_system::Config>::DbWeight::get().reads_writes(3, 3),
+            );
+
+            assert_eq!(
+                OperatorSigningKey::<Test>::get(pair_1.public()),
+                Some(BTreeSet::from([1]))
+            );
+            assert_eq!(
+                OperatorSigningKey::<Test>::get(pair_2.public()),
+                Some(BTreeSet::from([2, 3]))
+            );
+        })
+    }
+}

crates/pallet-domains/src/staking.rs

@@ -2,7 +2,7 @@
 
 use crate::pallet::{
     DomainRegistry, DomainStakingSummary, NextOperatorId, NominatorCount, Nominators,
-    OperatorIdOwner, Operators, PendingDeposits, PendingNominatorUnlocks,
+    OperatorIdOwner, OperatorSigningKey, Operators, PendingDeposits, PendingNominatorUnlocks,
     PendingOperatorDeregistrations, PendingOperatorSwitches, PendingOperatorUnlocks,
     PendingSlashes, PendingStakingOperationCount, PendingWithdrawals,
 };
@@ -113,6 +113,7 @@ pub enum Error {
     OperatorNotAllowed,
     InvalidOperatorSigningKey,
     MaximumNominators,
+    DuplicateOperatorSigningKey,
 }
 
 // Increase `PendingStakingOperationCount` by one and check if the `MaxPendingStakingOperation`
@@ -144,6 +145,11 @@ pub(crate) fn do_register_operator<T: Config>(
             Error::InvalidOperatorSigningKey
         );
 
+        ensure!(
+            !OperatorSigningKey::<T>::contains_key(config.signing_key.clone()),
+            Error::DuplicateOperatorSigningKey
+        );
+
         let domain_obj = DomainRegistry::<T>::get(domain_id).ok_or(Error::DomainNotInitialized)?;
         ensure!(
             domain_obj
@@ -178,7 +184,7 @@ pub(crate) fn do_register_operator<T: Config>(
         } = config;
 
         let operator = Operator {
-            signing_key,
+            signing_key: signing_key.clone(),
             current_domain_id: domain_id,
             next_domain_id: domain_id,
             minimum_nominator_stake,
@@ -189,6 +195,7 @@ pub(crate) fn do_register_operator<T: Config>(
             status: OperatorStatus::Registered,
         };
         Operators::<T>::insert(operator_id, operator);
+        OperatorSigningKey::<T>::append(signing_key, operator_id);
         // update stake summary to include new operator for next epoch
         domain_stake_summary.next_operators.insert(operator_id);
         // update pending transfers
@@ -774,7 +781,7 @@ pub(crate) mod tests {
 
         let mut ext = new_test_ext();
         ext.execute_with(|| {
-            let (operator_id, operator_config) = register_operator(
+            let (operator_id, mut operator_config) = register_operator(
                 domain_id,
                 operator_account,
                 operator_free_balance,
@@ -813,7 +820,21 @@ pub(crate) mod tests {
                 operator_free_balance - operator_stake - ExistentialDeposit::get()
             );
 
+            // cannot register with same operator key
+            let res = Domains::register_operator(
+                RuntimeOrigin::signed(operator_account),
+                domain_id,
+                operator_stake,
+                operator_config.clone(),
+            );
+            assert_err!(
+                res,
+                Error::<Test>::Staking(crate::staking::Error::DuplicateOperatorSigningKey)
+            );
+
             // cannot use the locked funds to register a new operator
+            let new_pair = OperatorPair::from_seed(&U256::from(1u32).into());
+            operator_config.signing_key = new_pair.public();
             let res = Domains::register_operator(
                 RuntimeOrigin::signed(operator_account),
                 domain_id,

crates/pallet-domains/src/staking_epoch.rs

@@ -1,5 +1,7 @@
 //! Staking epoch transition for domain
 
+#[cfg(any(not(feature = "runtime-benchmarks"), test))]
+use crate::pallet::OperatorSigningKey;
 use crate::pallet::{
     DomainStakingSummary, LastEpochStakingDistribution, Nominators, OperatorIdOwner, Operators,
     PendingDeposits, PendingNominatorUnlocks, PendingOperatorDeregistrations,
@@ -270,6 +272,15 @@ fn unlock_operator<T: Config>(operator_id: OperatorId) -> Result<(), Error> {
         // remove OperatorOwner Details
         OperatorIdOwner::<T>::remove(operator_id);
 
+        // remove operator signing key
+        let maybe_operator_ids = OperatorSigningKey::<T>::take(operator.signing_key.clone());
+        if let Some(mut operator_ids) = maybe_operator_ids {
+            operator_ids.remove(&operator_id);
+            if !operator_ids.is_empty() {
+                OperatorSigningKey::<T>::insert(operator.signing_key, operator_ids)
+            }
+        }
+
         // remove nominator count for this operator.
         NominatorCount::<T>::remove(operator_id);
 
@@ -732,8 +743,8 @@ mod tests {
     use crate::domain_registry::{DomainConfig, DomainObject};
     use crate::pallet::{
         DomainRegistry, DomainStakingSummary, LastEpochStakingDistribution, NominatorCount,
-        Nominators, OperatorIdOwner, Operators, PendingDeposits, PendingOperatorSwitches,
-        PendingOperatorUnlocks, PendingUnlocks, PendingWithdrawals,
+        Nominators, OperatorIdOwner, OperatorSigningKey, Operators, PendingDeposits,
+        PendingOperatorSwitches, PendingOperatorUnlocks, PendingUnlocks, PendingWithdrawals,
     };
     use crate::staking::tests::register_operator;
     use crate::staking::{
@@ -889,6 +900,10 @@ mod tests {
             do_finalize_domain_current_epoch::<Test>(domain_id, domain_block_number).unwrap();
 
             // unlock operator
+            assert_eq!(
+                OperatorSigningKey::<Test>::get(pair.public()),
+                Some(BTreeSet::from([operator_id]))
+            );
             let unlock_at = 100 + crate::tests::StakeWithdrawalLockingPeriod::get();
             assert!(do_unlock_pending_withdrawals::<Test>(domain_id, unlock_at).is_ok());
 
@@ -913,6 +928,7 @@ mod tests {
 
             assert_eq!(Operators::<Test>::get(operator_id), None);
             assert_eq!(OperatorIdOwner::<Test>::get(operator_id), None);
+            assert_eq!(OperatorSigningKey::<Test>::get(pair.public()), None);
             assert!(PendingOperatorUnlocks::<Test>::get().is_empty());
             assert_eq!(NominatorCount::<Test>::get(operator_id), 0);
         });

crates/subspace-runtime/src/lib.rs

@@ -38,6 +38,7 @@ use domain_runtime_primitives::{
     BlockNumber as DomainNumber, Hash as DomainHash, MultiAccountId, TryConvertBack,
 };
 use frame_support::inherent::ProvideInherent;
+use frame_support::migrations::VersionedMigration;
 use frame_support::traits::{ConstU16, ConstU32, ConstU64, ConstU8, Currency, Everything, Get};
 use frame_support::weights::constants::{RocksDbWeight, WEIGHT_REF_TIME_PER_SECOND};
 use frame_support::weights::{ConstantMultiplier, IdentityFee, Weight};
@@ -102,7 +103,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_name: create_runtime_str!("subspace"),
     impl_name: create_runtime_str!("subspace"),
     authoring_version: 0,
-    spec_version: 3,
+    spec_version: 4,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
     transaction_version: 1,
@@ -722,13 +723,24 @@ pub type SignedExtra = (
 pub type UncheckedExtrinsic =
     generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;
 
+pub type VersionCheckedMigrateDomainsV1ToV2<T> = VersionedMigration<
+    1,
+    2,
+    pallet_domains::migrations::VersionUncheckedMigrateV1ToV2<T>,
+    pallet_domains::Pallet<T>,
+    <T as frame_system::Config>::DbWeight,
+>;
+
+pub type Migrations = VersionCheckedMigrateDomainsV1ToV2<Runtime>;
+
 /// Executive: handles dispatch to the various modules.
 pub type Executive = frame_executive::Executive<
     Runtime,
     Block,
     frame_system::ChainContext<Runtime>,
     Runtime,
     AllPalletsWithSystem,
+    Migrations,
 >;
 
 fn extract_segment_headers(ext: &UncheckedExtrinsic) -> Option<Vec<SegmentHeader>> {