diff --git a/bridges/bin/millau/node/src/chain_spec.rs b/bridges/bin/millau/node/src/chain_spec.rs index 9d29d8ca7b469..541d240b6a1ac 100644 --- a/bridges/bin/millau/node/src/chain_spec.rs +++ b/bridges/bin/millau/node/src/chain_spec.rs @@ -158,6 +158,7 @@ fn testnet_genesis( pallet_substrate_bridge: Some(BridgeRialtoConfig { // We'll initialize the pallet with a dispatchable instead. init_data: None, + owner: None, }), pallet_sudo: Some(SudoConfig { key: root_key }), pallet_session: Some(SessionConfig { diff --git a/bridges/bin/rialto/node/src/chain_spec.rs b/bridges/bin/rialto/node/src/chain_spec.rs index b55a758544334..b562e646a62e8 100644 --- a/bridges/bin/rialto/node/src/chain_spec.rs +++ b/bridges/bin/rialto/node/src/chain_spec.rs @@ -187,5 +187,6 @@ fn load_kovan_bridge_config() -> Option { fn load_millau_bridge_config() -> Option { Some(BridgeMillauConfig { init_data: Some(rialto_runtime::millau::init_data()), + owner: Some([0; 32].into()), }) } diff --git a/bridges/bin/rialto/runtime/src/millau.rs b/bridges/bin/rialto/runtime/src/millau.rs index d49ed9856df24..3549360b111b9 100644 --- a/bridges/bin/rialto/runtime/src/millau.rs +++ b/bridges/bin/rialto/runtime/src/millau.rs @@ -32,6 +32,7 @@ pub fn init_data() -> InitializationData
{ authority_list: authority_set.authorities, set_id: authority_set.set_id, scheduled_change: None, + is_halted: false, } } diff --git a/bridges/modules/message-lane/src/lib.rs b/bridges/modules/message-lane/src/lib.rs index 30f575ab6884e..936d590f64108 100644 --- a/bridges/modules/message-lane/src/lib.rs +++ b/bridges/modules/message-lane/src/lib.rs @@ -148,7 +148,7 @@ decl_storage! { /// `None`, then there are no direct ways to halt/resume pallet operations, but other /// runtime methods may still be used to do that (i.e. democracy::referendum to update halt /// flag directly or call the `halt_operations`). - pub ModuleOwner get(fn module_owner) config(): Option; + pub ModuleOwner get(fn module_owner): Option; /// If true, all pallet transactions are failed immediately. pub IsHalted get(fn is_halted) config(): bool; /// Map of lane id => inbound lane data. @@ -160,6 +160,12 @@ decl_storage! { } add_extra_genesis { config(phantom): sp_std::marker::PhantomData; + config(owner): Option; + build(|config| { + if let Some(ref owner) = config.owner { + >::put(owner); + } + }) } } @@ -188,8 +194,14 @@ decl_module! { pub fn set_owner(origin, new_owner: Option) { ensure_owner_or_root::(origin)?; match new_owner { - Some(new_owner) => ModuleOwner::::put(new_owner), - None => ModuleOwner::::kill(), + Some(new_owner) => { + ModuleOwner::::put(&new_owner); + frame_support::debug::info!("Setting pallet Owner to: {:?}", new_owner); + }, + None => { + ModuleOwner::::kill(); + frame_support::debug::info!("Removed Owner of pallet."); + }, } } @@ -200,6 +212,7 @@ decl_module! { pub fn halt_operations(origin) { ensure_owner_or_root::(origin)?; IsHalted::::put(true); + frame_support::debug::warn!("Stopping pallet operations."); } /// Resume all pallet operations. May be called even if pallet is halted. @@ -209,6 +222,7 @@ decl_module! { pub fn resume_operations(origin) { ensure_owner_or_root::(origin)?; IsHalted::::put(false); + frame_support::debug::info!("Resuming pallet operations."); } /// Send message over lane. @@ -225,7 +239,6 @@ decl_module! { // let's first check if message can be delivered to target chain T::TargetHeaderChain::verify_message(&payload).map_err(|err| { frame_support::debug::trace!( - target: "runtime", "Message to lane {:?} is rejected by target chain: {:?}", lane_id, err, @@ -242,7 +255,6 @@ decl_module! { &payload, ).map_err(|err| { frame_support::debug::trace!( - target: "runtime", "Message to lane {:?} is rejected by lane verifier: {:?}", lane_id, err, @@ -257,7 +269,6 @@ decl_module! { &delivery_and_dispatch_fee, ).map_err(|err| { frame_support::debug::trace!( - target: "runtime", "Message to lane {:?} is rejected because submitter {:?} is unable to pay fee {:?}: {:?}", lane_id, submitter, @@ -277,7 +288,6 @@ decl_module! { lane.prune_messages(T::MaxMessagesToPruneAtOnce::get()); frame_support::debug::trace!( - target: "runtime", "Accepted message {} to lane {:?}", nonce, lane_id, @@ -303,7 +313,6 @@ decl_module! { let messages = verify_and_decode_messages_proof::(proof) .map_err(|err| { frame_support::debug::trace!( - target: "runtime", "Rejecting invalid messages proof: {:?}", err, ); @@ -323,7 +332,6 @@ decl_module! { .sum(); if dispatch_weight < actual_dispatch_weight { frame_support::debug::trace!( - target: "runtime", "Rejecting messages proof because of dispatch weight mismatch: declared={}, expected={}", dispatch_weight, actual_dispatch_weight, @@ -342,7 +350,6 @@ decl_module! { let updated_latest_confirmed_nonce = lane.receive_state_update(lane_state); if let Some(updated_latest_confirmed_nonce) = updated_latest_confirmed_nonce { frame_support::debug::trace!( - target: "runtime", "Received lane {:?} state update: latest_confirmed_nonce={}", lane_id, updated_latest_confirmed_nonce, @@ -361,7 +368,6 @@ decl_module! { } frame_support::debug::trace!( - target: "runtime", "Received messages: total={}, valid={}", total_messages, valid_messages, @@ -378,7 +384,6 @@ decl_module! { let confirmation_relayer = ensure_signed(origin)?; let (lane_id, lane_data) = T::TargetHeaderChain::verify_messages_delivery_proof(proof).map_err(|err| { frame_support::debug::trace!( - target: "runtime", "Rejecting invalid messages delivery proof: {:?}", err, ); @@ -414,7 +419,6 @@ decl_module! { } frame_support::debug::trace!( - target: "runtime", "Received messages delivery proof up to (and including) {} at lane {:?}", lane_data.latest_received_nonce, lane_id, diff --git a/bridges/modules/substrate/src/lib.rs b/bridges/modules/substrate/src/lib.rs index cf7d4fbc6ffef..1515c46ca897a 100644 --- a/bridges/modules/substrate/src/lib.rs +++ b/bridges/modules/substrate/src/lib.rs @@ -33,10 +33,12 @@ use crate::storage::ImportedHeader; use bp_runtime::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderOf}; -use frame_support::{decl_error, decl_module, decl_storage, dispatch::DispatchResult, ensure}; -use frame_system::{ensure_root, ensure_signed}; +use frame_support::{ + decl_error, decl_module, decl_storage, dispatch::DispatchResult, ensure, traits::Get, weights::DispatchClass, +}; +use frame_system::{ensure_signed, RawOrigin}; use sp_runtime::traits::Header as HeaderT; -use sp_runtime::RuntimeDebug; +use sp_runtime::{traits::BadOrigin, RuntimeDebug}; use sp_std::{marker::PhantomData, prelude::*}; use sp_trie::StorageProof; @@ -105,17 +107,30 @@ decl_storage! { // Grandpa doesn't require there to always be a pending change. In fact, most of the time // there will be no pending change available. NextScheduledChange: map hasher(identity) BridgedBlockHash => Option>>; - /// Whether or not the bridge has been initialized. + /// Optional pallet owner. /// - /// This is important to know to ensure that we don't try and initialize the bridge twice - /// and create an inconsistent genesis state. - IsInitialized: bool; + /// Pallet owner has a right to halt all pallet operations and then resume it. If it is + /// `None`, then there are no direct ways to halt/resume pallet operations, but other + /// runtime methods may still be used to do that (i.e. democracy::referendum to update halt + /// flag directly or call the `halt_operations`). + ModuleOwner get(fn module_owner): Option; + /// If true, all pallet transactions are failed immediately. + IsHalted get(fn is_halted): bool; } add_extra_genesis { + config(owner): Option; config(init_data): Option>>; build(|config| { + if let Some(ref owner) = config.owner { + >::put(owner); + } + if let Some(init_data) = config.init_data.clone() { initialize_bridge::(init_data); + } else { + // Since the bridge hasn't been initialized we shouldn't allow anyone to perform + // transactions. + IsHalted::put(true); } }) } @@ -129,12 +144,14 @@ decl_error! { UnfinalizedHeader, /// The header is unknown. UnknownHeader, - /// The pallet has already been initialized. - AlreadyInitialized, /// The storage proof doesn't contains storage root. So it is invalid for given header. StorageRootMismatch, /// Error when trying to fetch storage value from the proof. StorageValueUnavailable, + /// All pallet operations are halted. + Halted, + /// The pallet has already been initialized. + AlreadyInitialized, } } @@ -153,8 +170,9 @@ decl_module! { origin, header: BridgedHeader, ) -> DispatchResult { + ensure_operational::()?; let _ = ensure_signed(origin)?; - frame_support::debug::trace!(target: "sub-bridge", "Got header {:?}", header); + frame_support::debug::trace!("Got header {:?}", header); let mut verifier = verifier::Verifier { storage: PalletStorage::::new(), @@ -179,8 +197,9 @@ decl_module! { hash: BridgedBlockHash, finality_proof: Vec, ) -> DispatchResult { + ensure_operational::()?; let _ = ensure_signed(origin)?; - frame_support::debug::trace!(target: "sub-bridge", "Got header hash {:?}", hash); + frame_support::debug::trace!("Got header hash {:?}", hash); let mut verifier = verifier::Verifier { storage: PalletStorage::::new(), @@ -208,10 +227,52 @@ decl_module! { origin, init_data: InitializationData>, ) { - let _ = ensure_root(origin)?; - let init_allowed = !IsInitialized::get(); + ensure_owner_or_root::(origin)?; + let init_allowed = !>::exists(); ensure!(init_allowed, >::AlreadyInitialized); - initialize_bridge::(init_data); + initialize_bridge::(init_data.clone()); + + frame_support::debug::info!( + "Pallet has been initialized with the following parameters: {:?}", init_data + ); + } + + /// Change `ModuleOwner`. + /// + /// May only be called either by root, or by `ModuleOwner`. + #[weight = (T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational)] + pub fn set_owner(origin, new_owner: Option) { + ensure_owner_or_root::(origin)?; + match new_owner { + Some(new_owner) => { + ModuleOwner::::put(&new_owner); + frame_support::debug::info!("Setting pallet Owner to: {:?}", new_owner); + }, + None => { + ModuleOwner::::kill(); + frame_support::debug::info!("Removed Owner of pallet."); + }, + } + } + + /// Halt all pallet operations. Operations may be resumed using `resume_operations` call. + /// + /// May only be called either by root, or by `ModuleOwner`. + #[weight = (T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational)] + pub fn halt_operations(origin) { + ensure_owner_or_root::(origin)?; + IsHalted::put(true); + frame_support::debug::warn!("Stopping pallet operations."); + } + + /// Resume all pallet operations. May be called even if pallet is halted. + /// + /// May only be called either by root, or by `ModuleOwner`. + #[weight = (T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational)] + pub fn resume_operations(origin) { + ensure_owner_or_root::(origin)?; + IsHalted::put(false); + frame_support::debug::info!("Resuming pallet operations."); } } } @@ -288,14 +349,33 @@ impl Module { } } -// Since this writes to storage with no real checks this should only be used in functions that were -// called by a trusted origin. +/// Ensure that the origin is either root, or `ModuleOwner`. +fn ensure_owner_or_root(origin: T::Origin) -> Result<(), BadOrigin> { + match origin.into() { + Ok(RawOrigin::Root) => Ok(()), + Ok(RawOrigin::Signed(ref signer)) if Some(signer) == >::module_owner().as_ref() => Ok(()), + _ => Err(BadOrigin), + } +} + +/// Ensure that the pallet is in operational mode (not halted). +fn ensure_operational() -> Result<(), Error> { + if IsHalted::get() { + Err(>::Halted) + } else { + Ok(()) + } +} + +/// Since this writes to storage with no real checks this should only be used in functions that were +/// called by a trusted origin. fn initialize_bridge(init_params: InitializationData>) { let InitializationData { header, authority_list, set_id, scheduled_change, + is_halted, } = init_params; let initial_hash = header.hash(); @@ -328,7 +408,7 @@ fn initialize_bridge(init_params: InitializationData> }, ); - IsInitialized::put(true); + IsHalted::put(is_halted); } /// Expected interface for interacting with bridge pallet storage. @@ -505,13 +585,14 @@ mod tests { use sp_runtime::DispatchError; #[test] - fn only_root_origin_can_initialize_pallet() { + fn init_root_or_owner_origin_can_initialize_pallet() { run_test(|| { let init_data = InitializationData { header: test_header(1), authority_list: authority_list(), set_id: 1, scheduled_change: None, + is_halted: false, }; assert_noop!( @@ -519,53 +600,134 @@ mod tests { DispatchError::BadOrigin, ); - assert_ok!(Module::::initialize(Origin::root(), init_data)); + assert_ok!(Module::::initialize(Origin::root(), init_data.clone())); + + // Reset storage so we can initialize the pallet again + BestFinalized::::kill(); + ModuleOwner::::put(2); + assert_ok!(Module::::initialize(Origin::signed(2), init_data)); }) } #[test] - fn can_only_initialize_pallet_once() { + fn init_storage_entries_are_correctly_initialized() { run_test(|| { let init_data = InitializationData { header: test_header(1), authority_list: authority_list(), set_id: 1, scheduled_change: None, + is_halted: false, }; assert_ok!(Module::::initialize(Origin::root(), init_data.clone())); - assert_noop!( - Module::::initialize(Origin::root(), init_data,), - >::AlreadyInitialized, + + let storage = PalletStorage::::new(); + assert!(storage.header_exists(init_data.header.hash())); + assert_eq!( + storage.best_headers()[0], + crate::HeaderId { + number: *init_data.header.number(), + hash: init_data.header.hash() + } ); + assert_eq!(storage.best_finalized_header().hash(), init_data.header.hash()); + assert_eq!(storage.current_authority_set().authorities, init_data.authority_list); + assert_eq!(IsHalted::get(), false); }) } #[test] - fn storage_entries_are_correctly_initialized() { + fn init_can_only_initialize_pallet_once() { run_test(|| { let init_data = InitializationData { header: test_header(1), authority_list: authority_list(), set_id: 1, scheduled_change: None, + is_halted: false, }; assert_ok!(Module::::initialize(Origin::root(), init_data.clone())); + assert_noop!( + Module::::initialize(Origin::root(), init_data), + >::AlreadyInitialized + ); + }) + } - let storage = PalletStorage::::new(); + #[test] + fn pallet_owner_may_change_owner() { + run_test(|| { + ModuleOwner::::put(2); - assert!(IsInitialized::get()); - assert!(storage.header_exists(init_data.header.hash())); - assert_eq!( - storage.best_headers()[0], - crate::HeaderId { - number: *init_data.header.number(), - hash: init_data.header.hash() - } + assert_ok!(Module::::set_owner(Origin::root(), Some(1))); + assert_noop!( + Module::::halt_operations(Origin::signed(2)), + DispatchError::BadOrigin, + ); + assert_ok!(Module::::halt_operations(Origin::root())); + + assert_ok!(Module::::set_owner(Origin::signed(1), None)); + assert_noop!( + Module::::resume_operations(Origin::signed(1)), + DispatchError::BadOrigin, + ); + assert_noop!( + Module::::resume_operations(Origin::signed(2)), + DispatchError::BadOrigin, + ); + assert_ok!(Module::::resume_operations(Origin::root())); + }); + } + + #[test] + fn pallet_may_be_halted_by_root() { + run_test(|| { + assert_ok!(Module::::halt_operations(Origin::root())); + assert_ok!(Module::::resume_operations(Origin::root())); + }); + } + + #[test] + fn pallet_may_be_halted_by_owner() { + run_test(|| { + ModuleOwner::::put(2); + + assert_ok!(Module::::halt_operations(Origin::signed(2))); + assert_ok!(Module::::resume_operations(Origin::signed(2))); + + assert_noop!( + Module::::halt_operations(Origin::signed(1)), + DispatchError::BadOrigin, + ); + assert_noop!( + Module::::resume_operations(Origin::signed(1)), + DispatchError::BadOrigin, + ); + + assert_ok!(Module::::halt_operations(Origin::signed(2))); + assert_noop!( + Module::::resume_operations(Origin::signed(1)), + DispatchError::BadOrigin, + ); + }); + } + + #[test] + fn pallet_rejects_transactions_if_halted() { + run_test(|| { + IsHalted::put(true); + + assert_noop!( + Module::::import_signed_header(Origin::signed(1), test_header(1)), + Error::::Halted, + ); + + assert_noop!( + Module::::finalize_header(Origin::signed(1), test_header(1).hash(), vec![]), + Error::::Halted, ); - assert_eq!(storage.best_finalized_header().hash(), init_data.header.hash()); - assert_eq!(storage.current_authority_set().authorities, init_data.authority_list); }) } diff --git a/bridges/modules/substrate/src/storage.rs b/bridges/modules/substrate/src/storage.rs index 24c42482955ce..86677f67c54f8 100644 --- a/bridges/modules/substrate/src/storage.rs +++ b/bridges/modules/substrate/src/storage.rs @@ -38,6 +38,8 @@ pub struct InitializationData { pub set_id: SetId, /// The first scheduled authority set change of the pallet. pub scheduled_change: Option>, + /// Should the pallet block transaction immediately after initialization. + pub is_halted: bool, } /// A Grandpa Authority List and ID.