From 76c2de80a78537ae8d9ddabf71034a46397c1f45 Mon Sep 17 00:00:00 2001 From: Alistair Singh Date: Sat, 27 Jan 2024 01:25:58 +0200 Subject: [PATCH 1/2] reset nonces --- .../bridge-hub-rococo/src/xcm_config.rs | 12 +++ .../bridge-hub-rococo/tests/tests.rs | 74 ++++++++++++++++++- .../test-utils/src/test_cases/mod.rs | 4 +- .../runtimes/test-utils/src/test_cases.rs | 50 +++++++++++++ 4 files changed, 138 insertions(+), 2 deletions(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs index f641b428d4c6..7944d451e437 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs @@ -34,6 +34,7 @@ use bp_runtime::ChainId; use frame_support::{ parameter_types, traits::{ConstU32, Contains, Equals, Everything, Nothing}, + StoragePrefixedMap, }; use frame_system::EnsureRoot; use pallet_xcm::XcmPassthrough; @@ -168,6 +169,17 @@ impl Contains for SafeCallFilter { _ => (), }; + // Allow to removed dedicated storage items (called by governance-like) + if let RuntimeCall::System(frame_system::Call::kill_storage { keys }) = call { + return keys.iter().all(|k| { + // Allow resetting of Ethereum nonces in Rococo only. + k.starts_with(&snowbridge_pallet_inbound_queue::Nonce::::final_prefix()) || + k.starts_with( + &snowbridge_pallet_outbound_queue::Nonce::::final_prefix(), + ) + }); + } + matches!( call, RuntimeCall::PolkadotXcm( diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs index 9e46dc086548..9861d88afe56 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs @@ -26,11 +26,14 @@ use bridge_hub_rococo_runtime::{ }; use bridge_hub_test_utils::SlotDurations; use codec::{Decode, Encode}; -use frame_support::{dispatch::GetDispatchInfo, parameter_types, traits::ConstU8}; +use frame_support::{ + dispatch::GetDispatchInfo, parameter_types, traits::ConstU8, StoragePrefixedMap, +}; use parachains_common::{ rococo::{consensus::RELAY_CHAIN_SLOT_DURATION_MILLIS, fee::WeightToFee}, AccountId, AuraId, Balance, SLOT_DURATION, }; +use snowbridge_core::ChannelId; use sp_consensus_aura::SlotDuration; use sp_core::H160; use sp_keyring::AccountKeyring::Alice; @@ -222,6 +225,75 @@ mod bridge_hub_westend_tests { ) } + #[test] + fn kill_ethereum_nonces_by_governance_works() { + let channel_id_one: ChannelId = [1; 32].into(); + let channel_id_two: ChannelId = [2; 32].into(); + let nonce = 42; + + // Reset a single inbound channel + bridge_hub_test_utils::test_cases::kill_storage_keys_by_governance_works::( + collator_session_keys(), + bp_bridge_hub_rococo::BRIDGE_HUB_ROCOCO_PARACHAIN_ID, + Box::new(|call| RuntimeCall::System(call).encode()), + snowbridge_pallet_inbound_queue::Nonce::::hashed_key_for::( + channel_id_one, + ) + .to_vec(), + || { + snowbridge_pallet_inbound_queue::Nonce::::insert::( + channel_id_one, + nonce, + ); + snowbridge_pallet_inbound_queue::Nonce::::insert::( + channel_id_two, + nonce, + ); + }, + || { + assert_eq!( + snowbridge_pallet_inbound_queue::Nonce::::get(channel_id_one), + 0 + ); + assert_eq!( + snowbridge_pallet_inbound_queue::Nonce::::get(channel_id_two), + nonce + ); + }, + ); + + // Reset a single outbound channel + bridge_hub_test_utils::test_cases::kill_storage_keys_by_governance_works::( + collator_session_keys(), + bp_bridge_hub_rococo::BRIDGE_HUB_ROCOCO_PARACHAIN_ID, + Box::new(|call| RuntimeCall::System(call).encode()), + snowbridge_pallet_outbound_queue::Nonce::::hashed_key_for::( + channel_id_one, + ) + .to_vec(), + || { + snowbridge_pallet_outbound_queue::Nonce::::insert::( + channel_id_one, + nonce, + ); + snowbridge_pallet_outbound_queue::Nonce::::insert::( + channel_id_two, + nonce, + ); + }, + || { + assert_eq!( + snowbridge_pallet_outbound_queue::Nonce::::get(channel_id_one), + 0 + ); + assert_eq!( + snowbridge_pallet_outbound_queue::Nonce::::get(channel_id_two), + nonce + ); + }, + ); + } + #[test] fn change_delivery_reward_by_governance_works() { bridge_hub_test_utils::test_cases::change_storage_constant_by_governance_works::< diff --git a/cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs b/cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs index ce9396926449..edee39ef2116 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs @@ -72,7 +72,9 @@ pub type RuntimeHelper = parachains_runtimes_test_utils::RuntimeHelper; // Re-export test_case from `parachains-runtimes-test-utils` -pub use parachains_runtimes_test_utils::test_cases::change_storage_constant_by_governance_works; +pub use parachains_runtimes_test_utils::test_cases::{ + change_storage_constant_by_governance_works, kill_storage_keys_by_governance_works, +}; /// Prepare default runtime storage and run test within this context. pub fn run_test( diff --git a/cumulus/parachains/runtimes/test-utils/src/test_cases.rs b/cumulus/parachains/runtimes/test-utils/src/test_cases.rs index 950d0498130e..87c8d70468b1 100644 --- a/cumulus/parachains/runtimes/test-utils/src/test_cases.rs +++ b/cumulus/parachains/runtimes/test-utils/src/test_cases.rs @@ -91,3 +91,53 @@ pub fn change_storage_constant_by_governance_works( + collator_session_key: CollatorSessionKeys, + runtime_para_id: u32, + runtime_call_encode: Box) -> Vec>, + storage_constant_key: Vec, + initialize_storage: impl FnOnce() -> (), + assert_storage: impl FnOnce() -> (), +) where + Runtime: frame_system::Config + + pallet_balances::Config + + pallet_session::Config + + pallet_xcm::Config + + parachain_info::Config + + pallet_collator_selection::Config + + cumulus_pallet_parachain_system::Config, + ValidatorIdOf: From>, +{ + let mut runtime = ExtBuilder::::default() + .with_collators(collator_session_key.collators()) + .with_session_keys(collator_session_key.session_keys()) + .with_para_id(runtime_para_id.into()) + .with_tracing() + .build(); + runtime.execute_with(|| { + initialize_storage(); + }); + runtime.execute_with(|| { + // encode `kill_storage` call + let kill_storage_call = runtime_call_encode(frame_system::Call::::kill_storage { + keys: vec![storage_constant_key.clone()], + }); + + // estimate - storing just 1 value + use frame_system::WeightInfo; + let require_weight_at_most = + ::SystemWeightInfo::kill_storage(1); + + // execute XCM with Transact to `set_storage` as governance does + assert_ok!(RuntimeHelper::::execute_as_governance( + kill_storage_call, + require_weight_at_most + ) + .ensure_complete()); + }); + runtime.execute_with(|| { + assert_storage(); + }); +} From 963d84fa84fbe539e2060e92628ca001417deab0 Mon Sep 17 00:00:00 2001 From: Alistair Singh Date: Sat, 27 Jan 2024 21:26:01 +0200 Subject: [PATCH 2/2] use set storage instead of kill storage --- .../bridge-hub-rococo/src/xcm_config.rs | 20 ++---- .../bridge-hub-rococo/tests/tests.rs | 65 +++++++++---------- .../test-utils/src/test_cases/mod.rs | 2 +- .../runtimes/test-utils/src/test_cases.rs | 14 ++-- 4 files changed, 45 insertions(+), 56 deletions(-) diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs index 7944d451e437..237cfef258a5 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs @@ -161,25 +161,17 @@ impl Contains for SafeCallFilter { match call { RuntimeCall::System(frame_system::Call::set_storage { items }) if items.iter().all(|(k, _)| { - k.eq(&DeliveryRewardInBalance::key()) | - k.eq(&RequiredStakeForStakeAndSlash::key()) | - k.eq(&EthereumGatewayAddress::key()) + k.eq(&DeliveryRewardInBalance::key()) || + k.eq(&RequiredStakeForStakeAndSlash::key()) || + k.eq(&EthereumGatewayAddress::key()) || + // Allow resetting of Ethereum nonces in Rococo only. + k.starts_with(&snowbridge_pallet_inbound_queue::Nonce::::final_prefix()) || + k.starts_with(&snowbridge_pallet_outbound_queue::Nonce::::final_prefix()) }) => return true, _ => (), }; - // Allow to removed dedicated storage items (called by governance-like) - if let RuntimeCall::System(frame_system::Call::kill_storage { keys }) = call { - return keys.iter().all(|k| { - // Allow resetting of Ethereum nonces in Rococo only. - k.starts_with(&snowbridge_pallet_inbound_queue::Nonce::::final_prefix()) || - k.starts_with( - &snowbridge_pallet_outbound_queue::Nonce::::final_prefix(), - ) - }); - } - matches!( call, RuntimeCall::PolkadotXcm( diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs index 9861d88afe56..4131dfbc3deb 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs @@ -26,9 +26,7 @@ use bridge_hub_rococo_runtime::{ }; use bridge_hub_test_utils::SlotDurations; use codec::{Decode, Encode}; -use frame_support::{ - dispatch::GetDispatchInfo, parameter_types, traits::ConstU8, StoragePrefixedMap, -}; +use frame_support::{dispatch::GetDispatchInfo, parameter_types, traits::ConstU8}; use parachains_common::{ rococo::{consensus::RELAY_CHAIN_SLOT_DURATION_MILLIS, fee::WeightToFee}, AccountId, AuraId, Balance, SLOT_DURATION, @@ -226,62 +224,49 @@ mod bridge_hub_westend_tests { } #[test] - fn kill_ethereum_nonces_by_governance_works() { + fn change_ethereum_nonces_by_governance_works() { let channel_id_one: ChannelId = [1; 32].into(); let channel_id_two: ChannelId = [2; 32].into(); let nonce = 42; // Reset a single inbound channel - bridge_hub_test_utils::test_cases::kill_storage_keys_by_governance_works::( + bridge_hub_test_utils::test_cases::set_storage_keys_by_governance_works::( collator_session_keys(), bp_bridge_hub_rococo::BRIDGE_HUB_ROCOCO_PARACHAIN_ID, Box::new(|call| RuntimeCall::System(call).encode()), - snowbridge_pallet_inbound_queue::Nonce::::hashed_key_for::( - channel_id_one, - ) - .to_vec(), + vec![ + (snowbridge_pallet_outbound_queue::Nonce::::hashed_key_for::( + channel_id_one, + ) + .to_vec(), 0u64.encode()), + (snowbridge_pallet_inbound_queue::Nonce::::hashed_key_for::( + channel_id_one, + ) + .to_vec(), 0u64.encode()), + ], || { - snowbridge_pallet_inbound_queue::Nonce::::insert::( + // Outbound + snowbridge_pallet_outbound_queue::Nonce::::insert::( channel_id_one, nonce, ); - snowbridge_pallet_inbound_queue::Nonce::::insert::( + snowbridge_pallet_outbound_queue::Nonce::::insert::( channel_id_two, nonce, ); - }, - || { - assert_eq!( - snowbridge_pallet_inbound_queue::Nonce::::get(channel_id_one), - 0 - ); - assert_eq!( - snowbridge_pallet_inbound_queue::Nonce::::get(channel_id_two), - nonce - ); - }, - ); - // Reset a single outbound channel - bridge_hub_test_utils::test_cases::kill_storage_keys_by_governance_works::( - collator_session_keys(), - bp_bridge_hub_rococo::BRIDGE_HUB_ROCOCO_PARACHAIN_ID, - Box::new(|call| RuntimeCall::System(call).encode()), - snowbridge_pallet_outbound_queue::Nonce::::hashed_key_for::( - channel_id_one, - ) - .to_vec(), - || { - snowbridge_pallet_outbound_queue::Nonce::::insert::( + // Inbound + snowbridge_pallet_inbound_queue::Nonce::::insert::( channel_id_one, nonce, ); - snowbridge_pallet_outbound_queue::Nonce::::insert::( + snowbridge_pallet_inbound_queue::Nonce::::insert::( channel_id_two, nonce, ); }, || { + // Outbound assert_eq!( snowbridge_pallet_outbound_queue::Nonce::::get(channel_id_one), 0 @@ -290,6 +275,16 @@ mod bridge_hub_westend_tests { snowbridge_pallet_outbound_queue::Nonce::::get(channel_id_two), nonce ); + + // Inbound + assert_eq!( + snowbridge_pallet_inbound_queue::Nonce::::get(channel_id_one), + 0 + ); + assert_eq!( + snowbridge_pallet_inbound_queue::Nonce::::get(channel_id_two), + nonce + ); }, ); } diff --git a/cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs b/cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs index edee39ef2116..9b7d28932686 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs @@ -73,7 +73,7 @@ pub type RuntimeHelper = // Re-export test_case from `parachains-runtimes-test-utils` pub use parachains_runtimes_test_utils::test_cases::{ - change_storage_constant_by_governance_works, kill_storage_keys_by_governance_works, + change_storage_constant_by_governance_works, set_storage_keys_by_governance_works, }; /// Prepare default runtime storage and run test within this context. diff --git a/cumulus/parachains/runtimes/test-utils/src/test_cases.rs b/cumulus/parachains/runtimes/test-utils/src/test_cases.rs index 87c8d70468b1..f78bf9877ec2 100644 --- a/cumulus/parachains/runtimes/test-utils/src/test_cases.rs +++ b/cumulus/parachains/runtimes/test-utils/src/test_cases.rs @@ -92,12 +92,12 @@ pub fn change_storage_constant_by_governance_works( +/// Test-case makes sure that `Runtime` can change storage constant via governance-like call +pub fn set_storage_keys_by_governance_works( collator_session_key: CollatorSessionKeys, runtime_para_id: u32, runtime_call_encode: Box) -> Vec>, - storage_constant_key: Vec, + storage_items: Vec<(Vec, Vec)>, initialize_storage: impl FnOnce() -> (), assert_storage: impl FnOnce() -> (), ) where @@ -121,14 +121,16 @@ pub fn kill_storage_keys_by_governance_works( }); runtime.execute_with(|| { // encode `kill_storage` call - let kill_storage_call = runtime_call_encode(frame_system::Call::::kill_storage { - keys: vec![storage_constant_key.clone()], + let kill_storage_call = runtime_call_encode(frame_system::Call::::set_storage { + items: storage_items.clone(), }); // estimate - storing just 1 value use frame_system::WeightInfo; let require_weight_at_most = - ::SystemWeightInfo::kill_storage(1); + ::SystemWeightInfo::set_storage( + storage_items.len().try_into().unwrap(), + ); // execute XCM with Transact to `set_storage` as governance does assert_ok!(RuntimeHelper::::execute_as_governance(