From d2e0bb8cd9f7a9f762eecc1854aeec0047206fa6 Mon Sep 17 00:00:00 2001 From: vedhavyas Date: Fri, 5 Apr 2024 11:17:15 +0530 Subject: [PATCH] remove usage of SudoKey and use PermissionedAction to do specific actions on pallet-domains --- crates/pallet-domains/src/benchmarking.rs | 4 +- crates/pallet-domains/src/lib.rs | 41 +++++++++++++++---- crates/pallet-domains/src/tests.rs | 7 +++- crates/sp-domains/src/lib.rs | 19 +++++++-- .../src/bin/subspace-malicious-operator.rs | 5 ++- .../src/chain_spec.rs | 11 ++++- crates/subspace-malicious-operator/src/lib.rs | 17 ++++++++ .../src/malicious_bundle_producer.rs | 14 +++---- .../src/malicious_domain_instance_starter.rs | 3 ++ crates/subspace-node/src/chain_spec.rs | 18 ++++++-- crates/subspace-runtime/src/lib.rs | 6 --- test/subspace-test-client/src/chain_spec.rs | 1 + test/subspace-test-runtime/src/lib.rs | 6 --- 13 files changed, 112 insertions(+), 40 deletions(-) diff --git a/crates/pallet-domains/src/benchmarking.rs b/crates/pallet-domains/src/benchmarking.rs index 7b19c6e78f..4eeca59d90 100644 --- a/crates/pallet-domains/src/benchmarking.rs +++ b/crates/pallet-domains/src/benchmarking.rs @@ -527,7 +527,7 @@ mod benchmarks { #[benchmark] fn instantiate_domain() { - let creator = T::SudoId::get(); + let creator = account("domain_creator", 1, SEED); T::Currency::set_balance( &creator, T::DomainInstantiationDeposit::get() + T::MinNominatorStake::get(), @@ -888,7 +888,7 @@ mod benchmarks { } fn register_domain() -> DomainId { - let creator = T::SudoId::get(); + let creator = account("domain_creator", 1, SEED); T::Currency::set_balance( &creator, T::DomainInstantiationDeposit::get() + T::MinNominatorStake::get(), diff --git a/crates/pallet-domains/src/lib.rs b/crates/pallet-domains/src/lib.rs index 1a75b9c776..20d3d30d7a 100644 --- a/crates/pallet-domains/src/lib.rs +++ b/crates/pallet-domains/src/lib.rs @@ -350,10 +350,6 @@ mod pallet { /// Randomness source. type Randomness: RandomnessT>; - /// The sudo account id - #[pallet::constant] - type SudoId: Get; - /// The pallet-domains's pallet id. #[pallet::constant] type PalletId: Get; @@ -634,6 +630,11 @@ mod pallet { pub(super) type LatestSubmittedER = StorageMap<_, Identity, (DomainId, OperatorId), DomainBlockNumberFor, ValueQuery>; + /// Storage for PermissionedActions for domain instantiation and other permissioned calls. + #[pallet::storage] + pub(super) type PermissionedActionAllowedBy = + StorageValue<_, sp_domains::PermissionedActionAllowedBy, OptionQuery>; + #[derive(TypeInfo, Encode, Decode, PalletError, Debug, PartialEq)] pub enum BundleError { /// Can not find the operator for given operator id. @@ -772,6 +773,8 @@ mod pallet { BlockTree(BlockTreeError), /// Bundle storage fund specific errors BundleStorageFund(BundleStorageFundError), + /// Permissioned action is not allowed by the caller. + PermissionedActionNotAllowed, } /// Reason for slashing an operator @@ -1246,9 +1249,13 @@ mod pallet { origin: OriginFor, domain_config: DomainConfig>, ) -> DispatchResult { - ensure_root(origin)?; - - let who = T::SudoId::get(); + let who = ensure_signed(origin)?; + ensure!( + PermissionedActionAllowedBy::::get() + .map(|allowed_by| allowed_by.is_allowed(&who)) + .unwrap_or_default(), + Error::::PermissionedActionNotAllowed + ); let created_at = frame_system::Pallet::::current_block_number(); @@ -1376,17 +1383,32 @@ mod pallet { Ok(Some(actual_weight).into()) } + + /// Update permissioned action allowed by storage by Sudo. + #[pallet::call_index(14)] + #[pallet::weight(::DbWeight::get().reads_writes(0, 1))] + pub fn set_permissioned_action_allowed_by( + origin: OriginFor, + permissioned_action_allowed_by: sp_domains::PermissionedActionAllowedBy, + ) -> DispatchResult { + ensure_root(origin)?; + PermissionedActionAllowedBy::::put(permissioned_action_allowed_by); + Ok(()) + } } #[pallet::genesis_config] pub struct GenesisConfig { pub genesis_domain: Option>>, + pub permissioned_action_allowed_by: + Option>, } impl Default for GenesisConfig { fn default() -> Self { GenesisConfig { genesis_domain: None, + permissioned_action_allowed_by: None, } } } @@ -1394,6 +1416,11 @@ mod pallet { #[pallet::genesis_build] impl BuildGenesisConfig for GenesisConfig { fn build(&self) { + if let Some(permissioned_action_allowed_by) = + self.permissioned_action_allowed_by.as_ref().cloned() + { + PermissionedActionAllowedBy::::put(permissioned_action_allowed_by) + } if let Some(genesis_domain) = self.genesis_domain.as_ref().cloned() { // Register the genesis domain runtime let runtime_id = register_runtime_at_genesis::( diff --git a/crates/pallet-domains/src/tests.rs b/crates/pallet-domains/src/tests.rs index dc78488ca6..2b480c775c 100644 --- a/crates/pallet-domains/src/tests.rs +++ b/crates/pallet-domains/src/tests.rs @@ -279,7 +279,6 @@ impl pallet_domains::Config for Test { type MaxPendingStakingOperation = MaxPendingStakingOperation; type MaxNominators = MaxNominators; type Randomness = MockRandomness; - type SudoId = (); type PalletId = DomainsPalletId; type StorageFee = DummyStorageFee; type BlockSlot = DummyBlockSlot; @@ -577,6 +576,10 @@ pub(crate) fn run_to_block(block_number: BlockNumberFor, parent_ha pub(crate) fn register_genesis_domain(creator: u128, operator_ids: Vec) -> DomainId { let raw_genesis_storage = RawGenesis::dummy(vec![1, 2, 3, 4]).encode(); + assert_ok!(crate::Pallet::::set_permissioned_action_allowed_by( + RawOrigin::Root.into(), + sp_domains::PermissionedActionAllowedBy::Anyone + )); assert_ok!(crate::Pallet::::register_domain_runtime( RawOrigin::Root.into(), "evm".to_owned(), @@ -591,7 +594,7 @@ pub(crate) fn register_genesis_domain(creator: u128, operator_ids: Vec::ExistentialDeposit::get(), ); crate::Pallet::::instantiate_domain( - RawOrigin::Root.into(), + RawOrigin::Signed(creator).into(), DomainConfig { domain_name: "evm-domain".to_owned(), runtime_id: 0, diff --git a/crates/sp-domains/src/lib.rs b/crates/sp-domains/src/lib.rs index 8d9409360d..469e8f067b 100644 --- a/crates/sp-domains/src/lib.rs +++ b/crates/sp-domains/src/lib.rs @@ -784,6 +784,22 @@ impl OperatorAllowList { } } +/// Permissioned actions allowed by either specific accounts or anyone. +#[derive(TypeInfo, Encode, Decode, Debug, PartialEq, Clone, Serialize, Deserialize)] +pub enum PermissionedActionAllowedBy { + Accounts(Vec), + Anyone, +} + +impl PermissionedActionAllowedBy { + pub fn is_allowed(&self, who: &AccountId) -> bool { + match self { + PermissionedActionAllowedBy::Accounts(accounts) => accounts.contains(who), + PermissionedActionAllowedBy::Anyone => true, + } + } +} + #[derive(TypeInfo, Debug, Encode, Decode, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct GenesisDomain { // Domain runtime items @@ -1310,9 +1326,6 @@ sp_api::decl_runtime_apis! { /// Get operator id by signing key fn operator_id_by_signing_key(signing_key: OperatorPublicKey) -> Option; - /// Get the consensus chain sudo account id, currently only used in the intentional malicious operator - fn sudo_account_id() -> subspace_runtime_primitives::AccountId; - /// Returns the execution receipt hash of the given domain and domain block number fn receipt_hash(domain_id: DomainId, domain_number: HeaderNumberFor) -> Option>; diff --git a/crates/subspace-malicious-operator/src/bin/subspace-malicious-operator.rs b/crates/subspace-malicious-operator/src/bin/subspace-malicious-operator.rs index c7aa659c4e..1d18236718 100644 --- a/crates/subspace-malicious-operator/src/bin/subspace-malicious-operator.rs +++ b/crates/subspace-malicious-operator/src/bin/subspace-malicious-operator.rs @@ -93,6 +93,7 @@ fn set_default_ss58_version>(chain_spec: C) { fn main() -> Result<(), Error> { let cli = Cli::from_args(); + let sudo_account = cli.sudo_account(); let runner = cli.create_runner(&cli.run)?; set_default_ss58_version(&runner.config().chain_spec); runner.run_node_until_exit(|mut consensus_chain_config| async move { @@ -354,7 +355,9 @@ fn main() -> Result<(), Error> { return; } }; - if let Err(error) = domain_starter.start(bootstrap_result).await { + if let Err(error) = + domain_starter.start(bootstrap_result, sudo_account).await + { log::error!("Domain starter exited with an error {error:?}"); } }), diff --git a/crates/subspace-malicious-operator/src/chain_spec.rs b/crates/subspace-malicious-operator/src/chain_spec.rs index 40bb8d17d2..119c18cf3f 100644 --- a/crates/subspace-malicious-operator/src/chain_spec.rs +++ b/crates/subspace-malicious-operator/src/chain_spec.rs @@ -7,7 +7,7 @@ use sc_service::{ChainSpec, ChainType, NoExtension}; use sp_core::crypto::AccountId32; use sp_core::{sr25519, Pair, Public}; use sp_domains::storage::RawGenesis; -use sp_domains::{OperatorAllowList, OperatorPublicKey, RuntimeType}; +use sp_domains::{OperatorAllowList, OperatorPublicKey, PermissionedActionAllowedBy, RuntimeType}; use sp_runtime::traits::{Convert, IdentifyAccount}; use sp_runtime::{BuildStorage, MultiSigner, Percent}; use std::marker::PhantomData; @@ -94,6 +94,10 @@ pub fn domain_dev_config() -> GenericChainSpec AccountId32 { + get_account_id_from_seed("Alice") +} + pub fn create_domain_spec( chain_id: &str, raw_genesis: RawGenesis, @@ -151,6 +155,7 @@ struct GenesisDomainParams { operator_signing_key: OperatorPublicKey, raw_genesis_storage: Vec, initial_balances: Vec<(MultiAccountId, Balance)>, + permissioned_action_allowed_by: PermissionedActionAllowedBy, } pub fn dev_config() -> Result, String> { @@ -207,6 +212,7 @@ pub fn dev_config() -> Result("Alice"), raw_genesis_storage: raw_genesis_storage.clone(), initial_balances: endowed_accounts(), + permissioned_action_allowed_by: PermissionedActionAllowedBy::Anyone, }, ) }, @@ -271,6 +277,9 @@ fn subspace_genesis_config( confirmation_depth_k, }, domains: DomainsConfig { + permissioned_action_allowed_by: Some( + genesis_domain_params.permissioned_action_allowed_by, + ), genesis_domain: Some(sp_domains::GenesisDomain { runtime_name: "evm".to_owned(), runtime_type: RuntimeType::Evm, diff --git a/crates/subspace-malicious-operator/src/lib.rs b/crates/subspace-malicious-operator/src/lib.rs index aa00772ba6..08942a7086 100644 --- a/crates/subspace-malicious-operator/src/lib.rs +++ b/crates/subspace-malicious-operator/src/lib.rs @@ -29,6 +29,7 @@ use sc_cli::{ }; use sc_service::config::{KeystoreConfig, NetworkConfiguration}; use sc_service::{BasePath, BlocksPruning, Configuration, DatabaseSource}; +use sp_core::crypto::{AccountId32, Ss58Codec}; use sp_domains::DomainId; /// Subspace Cli. @@ -43,6 +44,11 @@ pub struct Cli { #[clap(flatten)] pub run: RunCmd, + /// Sudo account to use for malicious operator + /// If not passed, dev sudo account is used instead. + #[arg(long)] + pub sudo_account: Option, + /// Domain arguments /// /// The command-line arguments provided first will be passed to the embedded consensus node, @@ -53,6 +59,17 @@ pub struct Cli { pub domain_args: Vec, } +impl Cli { + pub fn sudo_account(&self) -> AccountId32 { + self.sudo_account + .as_ref() + .map(|sudo_account| { + AccountId32::from_ss58check(sudo_account).expect("Invalid sudo account") + }) + .unwrap_or(crate::chain_spec::consensus_dev_sudo_account()) + } +} + impl SubstrateCli for Cli { fn impl_name() -> String { "Subspace".into() diff --git a/crates/subspace-malicious-operator/src/malicious_bundle_producer.rs b/crates/subspace-malicious-operator/src/malicious_bundle_producer.rs index 7565bc3d1b..b0f7a219dc 100644 --- a/crates/subspace-malicious-operator/src/malicious_bundle_producer.rs +++ b/crates/subspace-malicious-operator/src/malicious_bundle_producer.rs @@ -82,7 +82,7 @@ impl MaliciousOperatorStatus { pub struct MaliciousBundleProducer { domain_id: DomainId, - sudo_acccount: AccountId, + sudo_account: AccountId, consensus_keystore: KeystorePtr, operator_keystore: KeystorePtr, consensus_client: Arc, @@ -118,6 +118,7 @@ where consensus_keystore: KeystorePtr, consensus_offchain_tx_pool_factory: OffchainTransactionPoolFactory, domain_transaction_pool: Arc, + sudo_account: AccountId, ) -> Self { let operator_keystore = KeystoreContainer::new(&KeystoreConfig::InMemory) .expect("create in-memory keystore container must succeed") @@ -146,11 +147,6 @@ where let malicious_bundle_tamper = MaliciousBundleTamper::new(domain_client, operator_keystore.clone()); - let sudo_acccount = consensus_client - .runtime_api() - .sudo_account_id(consensus_client.info().best_hash) - .expect("Failed to get sudo account"); - Self { domain_id, consensus_client, @@ -159,7 +155,7 @@ where bundle_producer, malicious_bundle_tamper, malicious_operator_status: MaliciousOperatorStatus::NoStatus, - sudo_acccount, + sudo_account, consensus_offchain_tx_pool_factory, } } @@ -317,7 +313,7 @@ where fn sudo_acccount_nonce(&self) -> Result> { Ok(self.consensus_client.runtime_api().account_nonce( self.consensus_client.info().best_hash, - self.sudo_acccount.clone(), + self.sudo_account.clone(), )?) } @@ -368,7 +364,7 @@ where &self.consensus_keystore, self.consensus_client.info(), call.clone(), - self.sudo_acccount.clone(), + self.sudo_account.clone(), nonce, )?, None => UncheckedExtrinsic::new_unsigned(call.clone()), diff --git a/crates/subspace-malicious-operator/src/malicious_domain_instance_starter.rs b/crates/subspace-malicious-operator/src/malicious_domain_instance_starter.rs index 24f5d173f3..3cad2ce6bf 100644 --- a/crates/subspace-malicious-operator/src/malicious_domain_instance_starter.rs +++ b/crates/subspace-malicious-operator/src/malicious_domain_instance_starter.rs @@ -23,6 +23,7 @@ use std::path::PathBuf; use std::sync::Arc; use subspace_runtime::RuntimeApi as CRuntimeApi; use subspace_runtime_primitives::opaque::Block as CBlock; +use subspace_runtime_primitives::AccountId; use subspace_service::FullClient as CFullClient; /// `DomainInstanceStarter` used to start a domain instance node based on the given @@ -51,6 +52,7 @@ where pub async fn start( self, bootstrap_result: BootstrapResult, + sudo_account: AccountId, ) -> std::result::Result<(), Box> { let BootstrapResult { domain_instance_data, @@ -178,6 +180,7 @@ where consensus_keystore, consensus_offchain_tx_pool_factory, domain_node.transaction_pool.clone(), + sudo_account, ); domain_node diff --git a/crates/subspace-node/src/chain_spec.rs b/crates/subspace-node/src/chain_spec.rs index 003a250a6a..94f6b4956c 100644 --- a/crates/subspace-node/src/chain_spec.rs +++ b/crates/subspace-node/src/chain_spec.rs @@ -30,7 +30,7 @@ use sc_telemetry::TelemetryEndpoints; use sp_consensus_subspace::FarmerPublicKey; use sp_core::crypto::{Ss58Codec, UncheckedFrom}; use sp_domains::storage::RawGenesis; -use sp_domains::{OperatorAllowList, OperatorPublicKey, RuntimeType}; +use sp_domains::{OperatorAllowList, OperatorPublicKey, PermissionedActionAllowedBy, RuntimeType}; use sp_runtime::{BuildStorage, Percent}; use std::collections::btree_set::BTreeSet; use std::marker::PhantomData; @@ -110,6 +110,7 @@ struct GenesisDomainParams { operator_allow_list: OperatorAllowList, operator_signing_key: OperatorPublicKey, initial_balances: Vec<(MultiAccountId, Balance)>, + permissioned_action_allowed_by: PermissionedActionAllowedBy, } pub fn gemini_3h_compiled() -> Result, String> { @@ -194,7 +195,7 @@ pub fn gemini_3h_compiled() -> Result, St GenesisDomainParams { domain_name: "nova".to_owned(), operator_allow_list: OperatorAllowList::Operators(BTreeSet::from_iter(vec![ - sudo_account, + sudo_account.clone(), ])), operator_signing_key: OperatorPublicKey::unchecked_from(hex!( "aa3b05b4d649666723e099cf3bafc2f2c04160ebe0e16ddc82f72d6ed97c4b6b" @@ -202,6 +203,9 @@ pub fn gemini_3h_compiled() -> Result, St initial_balances: evm_chain_spec::get_testnet_endowed_accounts_by_spec_id( SpecId::Gemini, ), + permissioned_action_allowed_by: PermissionedActionAllowedBy::Accounts(vec![ + sudo_account, + ]), }, ) }, @@ -292,7 +296,7 @@ pub fn devnet_config_compiled() -> Result .collect::>(); subspace_genesis_config( SpecId::DevNet, - sudo_account, + sudo_account.clone(), balances, vesting_schedules, GenesisParams { @@ -321,6 +325,9 @@ pub fn devnet_config_compiled() -> Result initial_balances: evm_chain_spec::get_testnet_endowed_accounts_by_spec_id( SpecId::DevNet, ), + permissioned_action_allowed_by: PermissionedActionAllowedBy::Accounts(vec![ + sudo_account, + ]), }, ) }, @@ -396,6 +403,9 @@ pub fn dev_config() -> Result, String> { initial_balances: evm_chain_spec::get_testnet_endowed_accounts_by_spec_id( SpecId::Dev, ), + permissioned_action_allowed_by: PermissionedActionAllowedBy::Accounts(vec![ + get_account_id_from_seed("Alice"), + ]), }, ) }, @@ -487,6 +497,8 @@ fn subspace_genesis_config( confirmation_depth_k, }, domains: DomainsConfig { + permissioned_action_allowed_by: enable_domains + .then_some(genesis_domain_params.permissioned_action_allowed_by), genesis_domain: enable_domains.then_some(sp_domains::GenesisDomain { runtime_name: "evm".to_owned(), runtime_type: RuntimeType::Evm, diff --git a/crates/subspace-runtime/src/lib.rs b/crates/subspace-runtime/src/lib.rs index 31b3285a8c..ffab957aaa 100644 --- a/crates/subspace-runtime/src/lib.rs +++ b/crates/subspace-runtime/src/lib.rs @@ -612,7 +612,6 @@ parameter_types! { pub TreasuryAccount: AccountId = PalletId(*b"treasury").into_account_truncating(); pub const MaxPendingStakingOperation: u32 = 512; pub const MaxNominators: u32 = 256; - pub SudoId: AccountId = Sudo::key().expect("Sudo account must exist"); pub const DomainsPalletId: PalletId = PalletId(*b"domains_"); pub const MaxInitialDomainAccounts: u32 = 10; pub const MinInitialDomainAccountBalance: Balance = SSC; @@ -681,7 +680,6 @@ impl pallet_domains::Config for Runtime { type MaxPendingStakingOperation = MaxPendingStakingOperation; type MaxNominators = MaxNominators; type Randomness = Subspace; - type SudoId = SudoId; type PalletId = DomainsPalletId; type StorageFee = TransactionFees; type BlockSlot = BlockSlot; @@ -1224,10 +1222,6 @@ impl_runtime_apis! { Domains::operator_signing_key(signing_key) } - fn sudo_account_id() -> AccountId { - SudoId::get() - } - fn receipt_hash(domain_id: DomainId, domain_number: DomainNumber) -> Option { Domains::receipt_hash(domain_id, domain_number) } diff --git a/test/subspace-test-client/src/chain_spec.rs b/test/subspace-test-client/src/chain_spec.rs index e6746d1701..0382abb98c 100644 --- a/test/subspace-test-client/src/chain_spec.rs +++ b/test/subspace-test-client/src/chain_spec.rs @@ -133,6 +133,7 @@ fn create_genesis_config( }, vesting: VestingConfig { vesting }, domains: DomainsConfig { + permissioned_action_allowed_by: Some(sp_domains::PermissionedActionAllowedBy::Anyone), genesis_domain: Some(GenesisDomain { runtime_name: "evm".to_owned(), runtime_type: RuntimeType::Evm, diff --git a/test/subspace-test-runtime/src/lib.rs b/test/subspace-test-runtime/src/lib.rs index ae240dd66a..ae0f7491a6 100644 --- a/test/subspace-test-runtime/src/lib.rs +++ b/test/subspace-test-runtime/src/lib.rs @@ -647,7 +647,6 @@ parameter_types! { pub TreasuryAccount: AccountId = PalletId(*b"treasury").into_account_truncating(); pub const MaxPendingStakingOperation: u32 = 512; pub const MaxNominators: u32 = 100; - pub SudoId: AccountId = Sudo::key().expect("Sudo account must exist"); pub const DomainsPalletId: PalletId = PalletId(*b"domains_"); pub const MaxInitialDomainAccounts: u32 = 20; pub const MinInitialDomainAccountBalance: Balance = SSC; @@ -712,7 +711,6 @@ impl pallet_domains::Config for Runtime { type MaxPendingStakingOperation = MaxPendingStakingOperation; type MaxNominators = MaxNominators; type Randomness = Subspace; - type SudoId = SudoId; type MinNominatorStake = MinNominatorStake; type PalletId = DomainsPalletId; type StorageFee = TransactionFees; @@ -1332,10 +1330,6 @@ impl_runtime_apis! { Domains::operator_signing_key(signing_key) } - fn sudo_account_id() -> AccountId { - SudoId::get() - } - fn receipt_hash(domain_id: DomainId, domain_number: DomainNumber) -> Option { Domains::receipt_hash(domain_id, domain_number) }