From 70d4907a32708e921de78e18fc9c7e3dbcfe4fe1 Mon Sep 17 00:00:00 2001 From: Sam Elamin Date: Thu, 12 Oct 2023 10:48:32 +0100 Subject: [PATCH 1/3] allow treasury to do reserve asset transfers (#1447) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This pr resolves https://github.com/paritytech/polkadot-sdk/issues/1428. *Added only to Kusama for now* I did raise it [here](https://github.com/polkadot-fellows/runtimes/pull/19) and we discussed creating a chopsticks test to run an end-to-end test however, to do that I will need a build agent/custom runner that is powerful enough to run the build I will be doing that separately as I still think having chopsticks test your runtime with each commit will be very powerful and extremely useful for the ecosystem For now I have used XCM simulator and replicated what the other reserve tests do --------- Co-authored-by: Gavin Wood Co-authored-by: Francisco Aguirre Co-authored-by: Branislav Kontur Co-authored-by: Bastian Köcher --- polkadot/xcm/xcm-builder/src/lib.rs | 6 +- .../xcm-builder/src/location_conversion.rs | 105 +++++++++++++++++- 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index 26f2aadadf3d..9fdd55d9f927 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -33,9 +33,9 @@ pub use location_conversion::{ Account32Hash, AccountId32Aliases, AccountKey20Aliases, AliasesIntoAccountId32, ChildParachainConvertsVia, DescribeAccountId32Terminal, DescribeAccountIdTerminal, DescribeAccountKey20Terminal, DescribeAllTerminal, DescribeBodyTerminal, DescribeFamily, - DescribeLocation, DescribePalletTerminal, DescribeTerminus, GlobalConsensusConvertsFor, - GlobalConsensusParachainConvertsFor, HashedDescription, ParentIsPreset, - SiblingParachainConvertsVia, + DescribeLocation, DescribePalletTerminal, DescribeTerminus, DescribeTreasuryVoiceTerminal, + GlobalConsensusConvertsFor, GlobalConsensusParachainConvertsFor, HashedDescription, + LocalTreasuryVoiceConvertsVia, ParentIsPreset, SiblingParachainConvertsVia, }; mod origin_conversion; diff --git a/polkadot/xcm/xcm-builder/src/location_conversion.rs b/polkadot/xcm/xcm-builder/src/location_conversion.rs index 4a5fbbb123be..25d16f7eb8cc 100644 --- a/polkadot/xcm/xcm-builder/src/location_conversion.rs +++ b/polkadot/xcm/xcm-builder/src/location_conversion.rs @@ -84,6 +84,20 @@ impl DescribeLocation for DescribeAccountKey20Terminal { } } +/// Create a description of the remote treasury `location` if possible. No two locations should have +/// the same descriptor. +pub struct DescribeTreasuryVoiceTerminal; + +impl DescribeLocation for DescribeTreasuryVoiceTerminal { + fn describe_location(l: &MultiLocation) -> Option> { + match (l.parents, &l.interior) { + (0, X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice })) => + Some((b"Treasury", b"Voice").encode()), + _ => None, + } + } +} + pub type DescribeAccountIdTerminal = (DescribeAccountId32Terminal, DescribeAccountKey20Terminal); pub struct DescribeBodyTerminal; @@ -101,6 +115,7 @@ pub type DescribeAllTerminal = ( DescribePalletTerminal, DescribeAccountId32Terminal, DescribeAccountKey20Terminal, + DescribeTreasuryVoiceTerminal, DescribeBodyTerminal, ); @@ -328,6 +343,25 @@ impl>, AccountId: From<[u8; 32]> + Into<[u8; 32]> } } +/// Returns specified `TreasuryAccount` as `AccountId32` if passed `location` matches Treasury +/// plurality. +pub struct LocalTreasuryVoiceConvertsVia( + PhantomData<(TreasuryAccount, AccountId)>, +); +impl, AccountId: From<[u8; 32]> + Into<[u8; 32]> + Clone> + ConvertLocation for LocalTreasuryVoiceConvertsVia +{ + fn convert_location(location: &MultiLocation) -> Option { + match *location { + MultiLocation { + parents: 0, + interior: X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice }), + } => Some((TreasuryAccount::get().into() as [u8; 32]).into()), + _ => None, + } + } +} + /// Conversion implementation which converts from a `[u8; 32]`-based `AccountId` into a /// `MultiLocation` consisting solely of a `AccountId32` junction with a fixed value for its /// network (provided by `Network`) and the `AccountId`'s `[u8; 32]` datum for the `id`. @@ -442,10 +476,13 @@ impl #[cfg(test)] mod tests { use super::*; - + use primitives::AccountId; pub type ForeignChainAliasAccount = HashedDescription; + pub type ForeignChainAliasTreasuryAccount = + HashedDescription>; + use frame_support::parameter_types; use xcm::latest::Junction; @@ -936,4 +973,70 @@ mod tests { }; assert!(ForeignChainAliasAccount::<[u8; 32]>::convert_location(&mul).is_none()); } + + #[test] + fn remote_account_convert_on_para_sending_from_remote_para_treasury() { + let relay_treasury_to_para_location = MultiLocation { + parents: 1, + interior: X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice }), + }; + let actual_description = ForeignChainAliasTreasuryAccount::<[u8; 32]>::convert_location( + &relay_treasury_to_para_location, + ) + .unwrap(); + + assert_eq!( + [ + 18, 84, 93, 74, 187, 212, 254, 71, 192, 127, 112, 51, 3, 42, 54, 24, 220, 185, 161, + 67, 205, 154, 108, 116, 108, 166, 226, 211, 29, 11, 244, 115 + ], + actual_description + ); + + let para_to_para_treasury_location = MultiLocation { + parents: 1, + interior: X2( + Parachain(1001), + Plurality { id: BodyId::Treasury, part: BodyPart::Voice }, + ), + }; + let actual_description = ForeignChainAliasTreasuryAccount::<[u8; 32]>::convert_location( + ¶_to_para_treasury_location, + ) + .unwrap(); + + assert_eq!( + [ + 202, 52, 249, 30, 7, 99, 135, 128, 153, 139, 176, 141, 138, 234, 163, 150, 7, 36, + 204, 92, 220, 137, 87, 57, 73, 91, 243, 189, 245, 200, 217, 204 + ], + actual_description + ); + } + + #[test] + fn local_account_convert_on_para_from_relay_treasury() { + let location = MultiLocation { + parents: 0, + interior: X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice }), + }; + + parameter_types! { + pub TreasuryAccountId: AccountId = AccountId::new([42u8; 32]); + } + + let actual_description = + LocalTreasuryVoiceConvertsVia::::convert_location( + &location, + ) + .unwrap(); + + assert_eq!( + [ + 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, + 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42 + ], + actual_description + ); + } } From f0e6d2ad52a8edb277f4144dee620eb1ce8a8a94 Mon Sep 17 00:00:00 2001 From: Kevin Krone Date: Thu, 12 Oct 2023 14:53:01 +0200 Subject: [PATCH 2/3] Adding `try_state` hook for `Treasury` pallet (#1820) This PR adds a `try_state` hook for the `Treasury` pallet. Part of #239. --- substrate/frame/treasury/src/benchmarking.rs | 6 +- substrate/frame/treasury/src/lib.rs | 90 ++++++- substrate/frame/treasury/src/tests.rs | 252 ++++++++++++++++--- 3 files changed, 308 insertions(+), 40 deletions(-) diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index f5f73ea8ddab..61fe29dafcae 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -336,5 +336,9 @@ mod benchmarks { Ok(()) } - impl_benchmark_test_suite!(Treasury, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!( + Treasury, + crate::tests::ExtBuilder::default().build(), + crate::tests::Test + ); } diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index b2b3a8801c15..5e429d3914bd 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -89,7 +89,8 @@ use sp_runtime::{ use sp_std::{collections::btree_map::BTreeMap, prelude::*}; use frame_support::{ - print, + dispatch::{DispatchResult, DispatchResultWithPostInfo}, + ensure, print, traits::{ tokens::Pay, Currency, ExistenceRequirement::KeepAlive, Get, Imbalance, OnUnbalanced, ReservableCurrency, WithdrawReasons, @@ -456,6 +457,14 @@ pub mod pallet { Weight::zero() } } + + #[cfg(feature = "try-runtime")] + fn try_state( + _: frame_system::pallet_prelude::BlockNumberFor, + ) -> Result<(), sp_runtime::TryRuntimeError> { + Self::do_try_state()?; + Ok(()) + } } #[derive(Default)] @@ -1020,6 +1029,85 @@ impl, I: 'static> Pallet { // Must never be less than 0 but better be safe. .saturating_sub(T::Currency::minimum_balance()) } + + /// Ensure the correctness of the state of this pallet. + #[cfg(any(feature = "try-runtime", test))] + fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { + Self::try_state_proposals()?; + Self::try_state_spends()?; + + Ok(()) + } + + /// ### Invariants of proposal storage items + /// + /// 1. [`ProposalCount`] >= Number of elements in [`Proposals`]. + /// 2. Each entry in [`Proposals`] should be saved under a key stricly less than current + /// [`ProposalCount`]. + /// 3. Each [`ProposalIndex`] contained in [`Approvals`] should exist in [`Proposals`]. + /// Note, that this automatically implies [`Approvals`].count() <= [`Proposals`].count(). + #[cfg(any(feature = "try-runtime", test))] + fn try_state_proposals() -> Result<(), sp_runtime::TryRuntimeError> { + let current_proposal_count = ProposalCount::::get(); + ensure!( + current_proposal_count as usize >= Proposals::::iter().count(), + "Actual number of proposals exceeds `ProposalCount`." + ); + + Proposals::::iter_keys().try_for_each(|proposal_index| -> DispatchResult { + ensure!( + current_proposal_count as u32 > proposal_index, + "`ProposalCount` should by strictly greater than any ProposalIndex used as a key for `Proposals`." + ); + Ok(()) + })?; + + Approvals::::get() + .iter() + .try_for_each(|proposal_index| -> DispatchResult { + ensure!( + Proposals::::contains_key(proposal_index), + "Proposal indices in `Approvals` must also be contained in `Proposals`." + ); + Ok(()) + })?; + + Ok(()) + } + + /// ## Invariants of spend storage items + /// + /// 1. [`SpendCount`] >= Number of elements in [`Spends`]. + /// 2. Each entry in [`Spends`] should be saved under a key stricly less than current + /// [`SpendCount`]. + /// 3. For each spend entry contained in [`Spends`] we should have spend.expire_at + /// > spend.valid_from. + #[cfg(any(feature = "try-runtime", test))] + fn try_state_spends() -> Result<(), sp_runtime::TryRuntimeError> { + let current_spend_count = SpendCount::::get(); + ensure!( + current_spend_count as usize >= Spends::::iter().count(), + "Actual number of spends exceeds `SpendCount`." + ); + + Spends::::iter_keys().try_for_each(|spend_index| -> DispatchResult { + ensure!( + current_spend_count > spend_index, + "`SpendCount` should by strictly greater than any SpendIndex used as a key for `Spends`." + ); + Ok(()) + })?; + + Spends::::iter().try_for_each(|(_index, spend)| -> DispatchResult { + ensure!( + spend.valid_from < spend.expire_at, + "Spend cannot expire before it becomes valid." + ); + Ok(()) + })?; + + Ok(()) + } } impl, I: 'static> OnUnbalanced> for Pallet { diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index 4bb00547d9f2..1748189723ed 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -217,16 +217,28 @@ impl Config for Test { type BenchmarkHelper = (); } -pub fn new_test_ext() -> sp_io::TestExternalities { - let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); - pallet_balances::GenesisConfig:: { - // Total issuance will be 200 with treasury account initialized at ED. - balances: vec![(0, 100), (1, 98), (2, 1)], +pub struct ExtBuilder {} + +impl Default for ExtBuilder { + fn default() -> Self { + Self {} + } +} + +impl ExtBuilder { + pub fn build(self) -> sp_io::TestExternalities { + let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); + pallet_balances::GenesisConfig:: { + // Total issuance will be 200 with treasury account initialized at ED. + balances: vec![(0, 100), (1, 98), (2, 1)], + } + .assimilate_storage(&mut t) + .unwrap(); + crate::GenesisConfig::::default().assimilate_storage(&mut t).unwrap(); + let mut ext = sp_io::TestExternalities::new(t); + ext.execute_with(|| System::set_block_number(1)); + ext } - .assimilate_storage(&mut t) - .unwrap(); - crate::GenesisConfig::::default().assimilate_storage(&mut t).unwrap(); - t.into() } fn get_payment_id(i: SpendIndex) -> Option { @@ -239,7 +251,7 @@ fn get_payment_id(i: SpendIndex) -> Option { #[test] fn genesis_config_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_eq!(Treasury::pot(), 0); assert_eq!(Treasury::proposal_count(), 0); }); @@ -247,7 +259,7 @@ fn genesis_config_works() { #[test] fn spend_local_origin_permissioning_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_noop!(Treasury::spend_local(RuntimeOrigin::signed(1), 1, 1), BadOrigin); assert_noop!( Treasury::spend_local(RuntimeOrigin::signed(10), 6, 1), @@ -271,7 +283,7 @@ fn spend_local_origin_permissioning_works() { #[docify::export] #[test] fn spend_local_origin_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { // Check that accumulate works when we have Some value in Dummy already. Balances::make_free_balance_be(&Treasury::account_id(), 101); // approve spend of some amount to beneficiary `6`. @@ -295,7 +307,7 @@ fn spend_local_origin_works() { #[test] fn minting_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { // Check that accumulate works when we have Some value in Dummy already. Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -304,7 +316,7 @@ fn minting_works() { #[test] fn spend_proposal_takes_min_deposit() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_ok!({ #[allow(deprecated)] Treasury::propose_spend(RuntimeOrigin::signed(0), 1, 3) @@ -316,7 +328,7 @@ fn spend_proposal_takes_min_deposit() { #[test] fn spend_proposal_takes_proportional_deposit() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_ok!({ #[allow(deprecated)] Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3) @@ -328,7 +340,7 @@ fn spend_proposal_takes_proportional_deposit() { #[test] fn spend_proposal_fails_when_proposer_poor() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_noop!( { #[allow(deprecated)] @@ -341,7 +353,7 @@ fn spend_proposal_fails_when_proposer_poor() { #[test] fn accepted_spend_proposal_ignored_outside_spend_period() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!({ @@ -361,7 +373,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { #[test] fn unused_pot_should_diminish() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { let init_total_issuance = Balances::total_issuance(); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::total_issuance(), init_total_issuance + 100); @@ -374,7 +386,7 @@ fn unused_pot_should_diminish() { #[test] fn rejected_spend_proposal_ignored_on_spend_period() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!({ @@ -394,7 +406,7 @@ fn rejected_spend_proposal_ignored_on_spend_period() { #[test] fn reject_already_rejected_spend_proposal_fails() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!({ @@ -417,7 +429,7 @@ fn reject_already_rejected_spend_proposal_fails() { #[test] fn reject_non_existent_spend_proposal_fails() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_noop!( { #[allow(deprecated)] @@ -430,7 +442,7 @@ fn reject_non_existent_spend_proposal_fails() { #[test] fn accept_non_existent_spend_proposal_fails() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_noop!( { #[allow(deprecated)] @@ -443,7 +455,7 @@ fn accept_non_existent_spend_proposal_fails() { #[test] fn accept_already_rejected_spend_proposal_fails() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!({ @@ -466,7 +478,7 @@ fn accept_already_rejected_spend_proposal_fails() { #[test] fn accepted_spend_proposal_enacted_on_spend_period() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -487,7 +499,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { #[test] fn pot_underflow_should_not_diminish() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -514,7 +526,7 @@ fn pot_underflow_should_not_diminish() { // i.e. pot should not include existential deposit needed for account survival. #[test] fn treasury_account_doesnt_get_deleted() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); let treasury_balance = Balances::free_balance(&Treasury::account_id()); @@ -613,7 +625,7 @@ fn genesis_funding_works() { #[test] fn max_approvals_limited() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), u64::MAX); Balances::make_free_balance_be(&0, u64::MAX); @@ -645,7 +657,7 @@ fn max_approvals_limited() { #[test] fn remove_already_removed_approval_fails() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!({ @@ -669,7 +681,7 @@ fn remove_already_removed_approval_fails() { #[test] fn spending_local_in_batch_respects_max_total() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { // Respect the `max_total` for the given origin. assert_ok!(RuntimeCall::from(UtilityCall::batch_all { calls: vec![ @@ -694,7 +706,7 @@ fn spending_local_in_batch_respects_max_total() { #[test] fn spending_in_batch_respects_max_total() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { // Respect the `max_total` for the given origin. assert_ok!(RuntimeCall::from(UtilityCall::batch_all { calls: vec![ @@ -739,7 +751,7 @@ fn spending_in_batch_respects_max_total() { #[test] fn spend_origin_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_ok!(Treasury::spend(RuntimeOrigin::signed(10), Box::new(1), 1, Box::new(6), None)); assert_ok!(Treasury::spend(RuntimeOrigin::signed(10), Box::new(1), 2, Box::new(6), None)); assert_noop!( @@ -764,7 +776,7 @@ fn spend_origin_works() { #[test] fn spend_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { System::set_block_number(1); assert_ok!(Treasury::spend(RuntimeOrigin::signed(10), Box::new(1), 2, Box::new(6), None)); @@ -796,7 +808,7 @@ fn spend_works() { #[test] fn spend_expires() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_eq!(::PayoutPeriod::get(), 5); // spend `0` expires in 5 blocks after the creating. @@ -816,7 +828,7 @@ fn spend_expires() { #[docify::export] #[test] fn spend_payout_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { System::set_block_number(1); // approve a `2` coins spend of asset `1` to beneficiary `6`, the spend valid from now. assert_ok!(Treasury::spend(RuntimeOrigin::signed(10), Box::new(1), 2, Box::new(6), None)); @@ -838,7 +850,7 @@ fn spend_payout_works() { #[test] fn payout_retry_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { System::set_block_number(1); assert_ok!(Treasury::spend(RuntimeOrigin::signed(10), Box::new(1), 2, Box::new(6), None)); assert_ok!(Treasury::payout(RuntimeOrigin::signed(1), 0)); @@ -863,7 +875,7 @@ fn payout_retry_works() { #[test] fn spend_valid_from_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_eq!(::PayoutPeriod::get(), 5); System::set_block_number(1); @@ -895,7 +907,7 @@ fn spend_valid_from_works() { #[test] fn void_spend_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { System::set_block_number(1); // spend cannot be voided if already attempted. assert_ok!(Treasury::spend( @@ -926,7 +938,7 @@ fn void_spend_works() { #[test] fn check_status_works() { - new_test_ext().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { assert_eq!(::PayoutPeriod::get(), 5); System::set_block_number(1); @@ -984,3 +996,167 @@ fn check_status_works() { System::assert_last_event(Event::::SpendProcessed { index: 4 }.into()); }); } + +#[test] +fn try_state_proposals_invariant_1_works() { + ExtBuilder::default().build().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + // Add a proposal using `propose_spend` + assert_ok!({ + #[allow(deprecated)] + Treasury::propose_spend(RuntimeOrigin::signed(0), 1, 3) + }); + assert_eq!(Proposals::::iter().count(), 1); + assert_eq!(ProposalCount::::get(), 1); + // Check invariant 1 holds + assert!(ProposalCount::::get() as usize >= Proposals::::iter().count()); + // Break invariant 1 by decreasing `ProposalCount` + ProposalCount::::put(0); + // Invariant 1 should be violated + assert_eq!( + Treasury::do_try_state(), + Err(Other("Actual number of proposals exceeds `ProposalCount`.")) + ); + }); +} + +#[test] +fn try_state_proposals_invariant_2_works() { + ExtBuilder::default().build().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + // Add a proposal using `propose_spend` + assert_ok!({ + #[allow(deprecated)] + Treasury::propose_spend(RuntimeOrigin::signed(0), 1, 3) + }); + assert_eq!(Proposals::::iter().count(), 1); + let current_proposal_count = ProposalCount::::get(); + assert_eq!(current_proposal_count, 1); + // Check invariant 2 holds + assert!( + Proposals::::iter_keys() + .all(|proposal_index| { + proposal_index < current_proposal_count + }) + ); + // Break invariant 2 by inserting the proposal under key = 1 + let proposal = Proposals::::take(0).unwrap(); + Proposals::::insert(1, proposal); + // Invariant 2 should be violated + assert_eq!( + Treasury::do_try_state(), + Err(Other("`ProposalCount` should by strictly greater than any ProposalIndex used as a key for `Proposals`.")) + ); + }); +} + +#[test] +fn try_state_proposals_invariant_3_works() { + ExtBuilder::default().build().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + // Add a proposal using `propose_spend` + assert_ok!({ + #[allow(deprecated)] + Treasury::propose_spend(RuntimeOrigin::signed(0), 10, 3) + }); + assert_eq!(Proposals::::iter().count(), 1); + // Approve the proposal + assert_ok!({ + #[allow(deprecated)] + Treasury::approve_proposal(RuntimeOrigin::root(), 0) + }); + assert_eq!(Approvals::::get().len(), 1); + // Check invariant 3 holds + assert!(Approvals::::get() + .iter() + .all(|proposal_index| { Proposals::::contains_key(proposal_index) })); + // Break invariant 3 by adding another key to `Approvals` + let mut approvals_modified = Approvals::::get(); + approvals_modified.try_push(2).unwrap(); + Approvals::::put(approvals_modified); + // Invariant 3 should be violated + assert_eq!( + Treasury::do_try_state(), + Err(Other("Proposal indices in `Approvals` must also be contained in `Proposals`.")) + ); + }); +} + +#[test] +fn try_state_spends_invariant_1_works() { + ExtBuilder::default().build().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + // Propose and approve a spend + assert_ok!({ + Treasury::spend(RuntimeOrigin::signed(10), Box::new(1), 1, Box::new(6), None) + }); + assert_eq!(Spends::::iter().count(), 1); + assert_eq!(SpendCount::::get(), 1); + // Check invariant 1 holds + assert!(SpendCount::::get() as usize >= Spends::::iter().count()); + // Break invariant 1 by decreasing `SpendCount` + SpendCount::::put(0); + // Invariant 1 should be violated + assert_eq!( + Treasury::do_try_state(), + Err(Other("Actual number of spends exceeds `SpendCount`.")) + ); + }); +} + +#[test] +fn try_state_spends_invariant_2_works() { + ExtBuilder::default().build().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + // Propose and approve a spend + assert_ok!({ + Treasury::spend(RuntimeOrigin::signed(10), Box::new(1), 1, Box::new(6), None) + }); + assert_eq!(Spends::::iter().count(), 1); + let current_spend_count = SpendCount::::get(); + assert_eq!(current_spend_count, 1); + // Check invariant 2 holds + assert!( + Spends::::iter_keys() + .all(|spend_index| { + spend_index < current_spend_count + }) + ); + // Break invariant 2 by inserting the spend under key = 1 + let spend = Spends::::take(0).unwrap(); + Spends::::insert(1, spend); + // Invariant 2 should be violated + assert_eq!( + Treasury::do_try_state(), + Err(Other("`SpendCount` should by strictly greater than any SpendIndex used as a key for `Spends`.")) + ); + }); +} + +#[test] +fn try_state_spends_invariant_3_works() { + ExtBuilder::default().build().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + // Propose and approve a spend + assert_ok!({ + Treasury::spend(RuntimeOrigin::signed(10), Box::new(1), 1, Box::new(6), None) + }); + assert_eq!(Spends::::iter().count(), 1); + let current_spend_count = SpendCount::::get(); + assert_eq!(current_spend_count, 1); + // Check invariant 3 holds + assert!(Spends::::iter_values() + .all(|SpendStatus { valid_from, expire_at, .. }| { valid_from < expire_at })); + // Break invariant 3 by reversing spend.expire_at and spend.valid_from + let spend = Spends::::take(0).unwrap(); + Spends::::insert( + 0, + SpendStatus { valid_from: spend.expire_at, expire_at: spend.valid_from, ..spend }, + ); + // Invariant 3 should be violated + assert_eq!( + Treasury::do_try_state(), + Err(Other("Spend cannot expire before it becomes valid.")) + ); + }); +} From 7aace06b3d653529ab992c8ff96571d54bf00a36 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 12 Oct 2023 16:01:07 +0300 Subject: [PATCH 3/3] Disabled validators runtime API (#1257) Exposes disabled validators list via a runtime API. --------- Co-authored-by: ordian Co-authored-by: ordian --- .../src/blockchain_rpc_client.rs | 7 +++++ .../src/rpc_client.rs | 8 ++++++ .../emulated/common/src/lib.rs | 6 ++--- polkadot/node/core/runtime-api/src/cache.rs | 18 +++++++++++++ polkadot/node/core/runtime-api/src/lib.rs | 10 +++++++ polkadot/node/core/runtime-api/src/tests.rs | 4 +++ polkadot/node/subsystem-types/src/messages.rs | 5 ++++ .../subsystem-types/src/runtime_client.rs | 9 ++++++- polkadot/node/subsystem-util/src/lib.rs | 1 + polkadot/primitives/src/runtime_api.rs | 7 +++++ .../src/runtime_api_impl/vstaging.rs | 27 +++++++++++++++++++ polkadot/runtime/rococo/src/lib.rs | 11 ++++++-- polkadot/runtime/westend/src/lib.rs | 10 +++++-- 13 files changed, 115 insertions(+), 8 deletions(-) diff --git a/cumulus/client/relay-chain-minimal-node/src/blockchain_rpc_client.rs b/cumulus/client/relay-chain-minimal-node/src/blockchain_rpc_client.rs index 3f4c08ecbb83..1e78df711543 100644 --- a/cumulus/client/relay-chain-minimal-node/src/blockchain_rpc_client.rs +++ b/cumulus/client/relay-chain-minimal-node/src/blockchain_rpc_client.rs @@ -346,6 +346,13 @@ impl RuntimeApiSubsystemClient for BlockChainRpcClient { Ok(self.rpc_client.parachain_host_minimum_backing_votes(at, session_index).await?) } + async fn disabled_validators( + &self, + at: Hash, + ) -> Result, ApiError> { + Ok(self.rpc_client.parachain_host_disabled_validators(at).await?) + } + async fn async_backing_params(&self, at: Hash) -> Result { Ok(self.rpc_client.parachain_host_async_backing_params(at).await?) } diff --git a/cumulus/client/relay-chain-rpc-interface/src/rpc_client.rs b/cumulus/client/relay-chain-rpc-interface/src/rpc_client.rs index b1fd7d1ab7d9..5924716adcb4 100644 --- a/cumulus/client/relay-chain-rpc-interface/src/rpc_client.rs +++ b/cumulus/client/relay-chain-rpc-interface/src/rpc_client.rs @@ -597,6 +597,14 @@ impl RelayChainRpcClient { .await } + pub async fn parachain_host_disabled_validators( + &self, + at: RelayHash, + ) -> Result, RelayChainError> { + self.call_remote_runtime_function("ParachainHost_disabled_validators", at, None::<()>) + .await + } + #[allow(missing_docs)] pub async fn parachain_host_async_backing_params( &self, diff --git a/cumulus/parachains/integration-tests/emulated/common/src/lib.rs b/cumulus/parachains/integration-tests/emulated/common/src/lib.rs index c2e065ccadcd..f8fe8831d3c7 100644 --- a/cumulus/parachains/integration-tests/emulated/common/src/lib.rs +++ b/cumulus/parachains/integration-tests/emulated/common/src/lib.rs @@ -33,7 +33,7 @@ use xcm_emulator::{ }; decl_test_relay_chains! { - #[api_version(7)] + #[api_version(8)] pub struct Westend { genesis = westend::genesis(), on_init = (), @@ -50,7 +50,7 @@ decl_test_relay_chains! { AssetRate: westend_runtime::AssetRate, } }, - #[api_version(7)] + #[api_version(8)] pub struct Rococo { genesis = rococo::genesis(), on_init = (), @@ -65,7 +65,7 @@ decl_test_relay_chains! { Balances: rococo_runtime::Balances, } }, - #[api_version(7)] + #[api_version(8)] pub struct Wococo { genesis = rococo::genesis(), on_init = (), diff --git a/polkadot/node/core/runtime-api/src/cache.rs b/polkadot/node/core/runtime-api/src/cache.rs index 6cf7fa744d3f..69eea22b23bd 100644 --- a/polkadot/node/core/runtime-api/src/cache.rs +++ b/polkadot/node/core/runtime-api/src/cache.rs @@ -64,6 +64,7 @@ pub(crate) struct RequestResultCache { unapplied_slashes: LruMap>, key_ownership_proof: LruMap<(Hash, ValidatorId), Option>, minimum_backing_votes: LruMap, + disabled_validators: LruMap>, para_backing_state: LruMap<(Hash, ParaId), Option>, async_backing_params: LruMap, } @@ -96,6 +97,7 @@ impl Default for RequestResultCache { unapplied_slashes: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)), key_ownership_proof: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)), minimum_backing_votes: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)), + disabled_validators: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)), para_backing_state: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)), async_backing_params: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)), } @@ -444,6 +446,21 @@ impl RequestResultCache { self.minimum_backing_votes.insert(session_index, minimum_backing_votes); } + pub(crate) fn disabled_validators( + &mut self, + relay_parent: &Hash, + ) -> Option<&Vec> { + self.disabled_validators.get(relay_parent).map(|v| &*v) + } + + pub(crate) fn cache_disabled_validators( + &mut self, + relay_parent: Hash, + disabled_validators: Vec, + ) { + self.disabled_validators.insert(relay_parent, disabled_validators); + } + pub(crate) fn para_backing_state( &mut self, key: (Hash, ParaId), @@ -520,6 +537,7 @@ pub(crate) enum RequestResult { slashing::OpaqueKeyOwnershipProof, Option<()>, ), + DisabledValidators(Hash, Vec), ParaBackingState(Hash, ParaId, Option), AsyncBackingParams(Hash, async_backing::AsyncBackingParams), } diff --git a/polkadot/node/core/runtime-api/src/lib.rs b/polkadot/node/core/runtime-api/src/lib.rs index 1b18941e5464..bdcca08b10dd 100644 --- a/polkadot/node/core/runtime-api/src/lib.rs +++ b/polkadot/node/core/runtime-api/src/lib.rs @@ -166,6 +166,8 @@ where .requests_cache .cache_key_ownership_proof((relay_parent, validator_id), key_ownership_proof), SubmitReportDisputeLost(_, _, _, _) => {}, + DisabledValidators(relay_parent, disabled_validators) => + self.requests_cache.cache_disabled_validators(relay_parent, disabled_validators), ParaBackingState(relay_parent, para_id, constraints) => self .requests_cache .cache_para_backing_state((relay_parent, para_id), constraints), @@ -296,6 +298,8 @@ where Request::SubmitReportDisputeLost(dispute_proof, key_ownership_proof, sender) }, ), + Request::DisabledValidators(sender) => query!(disabled_validators(), sender) + .map(|sender| Request::DisabledValidators(sender)), Request::ParaBackingState(para, sender) => query!(para_backing_state(para), sender) .map(|sender| Request::ParaBackingState(para, sender)), Request::AsyncBackingParams(sender) => query!(async_backing_params(), sender) @@ -565,6 +569,12 @@ where ver = Request::MINIMUM_BACKING_VOTES_RUNTIME_REQUIREMENT, sender ), + Request::DisabledValidators(sender) => query!( + DisabledValidators, + disabled_validators(), + ver = Request::DISABLED_VALIDATORS_RUNTIME_REQUIREMENT, + sender + ), Request::ParaBackingState(para, sender) => { query!( ParaBackingState, diff --git a/polkadot/node/core/runtime-api/src/tests.rs b/polkadot/node/core/runtime-api/src/tests.rs index fb97139a8028..979b3587d269 100644 --- a/polkadot/node/core/runtime-api/src/tests.rs +++ b/polkadot/node/core/runtime-api/src/tests.rs @@ -268,6 +268,10 @@ impl RuntimeApiSubsystemClient for MockSubsystemClient { async fn minimum_backing_votes(&self, _: Hash, _: SessionIndex) -> Result { todo!("Not required for tests") } + + async fn disabled_validators(&self, _: Hash) -> Result, ApiError> { + todo!("Not required for tests") + } } #[test] diff --git a/polkadot/node/subsystem-types/src/messages.rs b/polkadot/node/subsystem-types/src/messages.rs index 01ccee3add90..6bc874384594 100644 --- a/polkadot/node/subsystem-types/src/messages.rs +++ b/polkadot/node/subsystem-types/src/messages.rs @@ -695,6 +695,8 @@ pub enum RuntimeApiRequest { ), /// Get the minimum required backing votes. MinimumBackingVotes(SessionIndex, RuntimeApiSender), + /// Returns all disabled validators at a given block height. + DisabledValidators(RuntimeApiSender>), /// Get the backing state of the given para. ParaBackingState(ParaId, RuntimeApiSender>), /// Get candidate's acceptance limitations for asynchronous backing for a relay parent. @@ -726,6 +728,9 @@ impl RuntimeApiRequest { /// Minimum version to enable asynchronous backing: `AsyncBackingParams` and `ParaBackingState`. pub const ASYNC_BACKING_STATE_RUNTIME_REQUIREMENT: u32 = 7; + + /// `DisabledValidators` + pub const DISABLED_VALIDATORS_RUNTIME_REQUIREMENT: u32 = 8; } /// A message to the Runtime API subsystem. diff --git a/polkadot/node/subsystem-types/src/runtime_client.rs b/polkadot/node/subsystem-types/src/runtime_client.rs index 3007e985b4f7..f7adcf9862b5 100644 --- a/polkadot/node/subsystem-types/src/runtime_client.rs +++ b/polkadot/node/subsystem-types/src/runtime_client.rs @@ -255,6 +255,10 @@ pub trait RuntimeApiSubsystemClient { at: Hash, para_id: Id, ) -> Result, ApiError>; + + // === v8 === + /// Gets the disabled validators at a specific block height + async fn disabled_validators(&self, at: Hash) -> Result, ApiError>; } /// Default implementation of [`RuntimeApiSubsystemClient`] using the client. @@ -497,11 +501,14 @@ where self.client.runtime_api().para_backing_state(at, para_id) } - /// Returns candidate's acceptance limitations for asynchronous backing for a relay parent. async fn async_backing_params( &self, at: Hash, ) -> Result { self.client.runtime_api().async_backing_params(at) } + + async fn disabled_validators(&self, at: Hash) -> Result, ApiError> { + self.client.runtime_api().disabled_validators(at) + } } diff --git a/polkadot/node/subsystem-util/src/lib.rs b/polkadot/node/subsystem-util/src/lib.rs index 57e4f9cde09a..a5f3e9d4a0c0 100644 --- a/polkadot/node/subsystem-util/src/lib.rs +++ b/polkadot/node/subsystem-util/src/lib.rs @@ -226,6 +226,7 @@ specialize_requests! { fn request_unapplied_slashes() -> Vec<(SessionIndex, CandidateHash, slashing::PendingSlashes)>; UnappliedSlashes; fn request_key_ownership_proof(validator_id: ValidatorId) -> Option; KeyOwnershipProof; fn request_submit_report_dispute_lost(dp: slashing::DisputeProof, okop: slashing::OpaqueKeyOwnershipProof) -> Option<()>; SubmitReportDisputeLost; + fn request_disabled_validators() -> Vec; DisabledValidators; fn request_async_backing_params() -> AsyncBackingParams; AsyncBackingParams; } diff --git a/polkadot/primitives/src/runtime_api.rs b/polkadot/primitives/src/runtime_api.rs index 6cb66d40204d..5ec897c8cbb4 100644 --- a/polkadot/primitives/src/runtime_api.rs +++ b/polkadot/primitives/src/runtime_api.rs @@ -248,6 +248,7 @@ sp_api::decl_runtime_apis! { #[api_version(6)] fn minimum_backing_votes() -> u32; + /***** Added in v7: Asynchronous backing *****/ /// Returns the state of parachain backing for a given para. @@ -257,5 +258,11 @@ sp_api::decl_runtime_apis! { /// Returns candidate's acceptance limitations for asynchronous backing for a relay parent. #[api_version(7)] fn async_backing_params() -> AsyncBackingParams; + + /***** Added in v8 *****/ + + /// Returns a list of all disabled validators at the given block. + #[api_version(8)] + fn disabled_validators() -> Vec; } } diff --git a/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs b/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs index d01b543630c3..24a076f3a443 100644 --- a/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs +++ b/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs @@ -15,3 +15,30 @@ // along with Polkadot. If not, see . //! Put implementations of functions from staging APIs here. + +use crate::shared; +use primitives::ValidatorIndex; +use sp_std::{collections::btree_map::BTreeMap, prelude::Vec}; + +/// Implementation for `DisabledValidators` +// CAVEAT: this should only be called on the node side +// as it might produce incorrect results on session boundaries +pub fn disabled_validators() -> Vec +where + T: pallet_session::Config + shared::Config, +{ + let shuffled_indices = >::active_validator_indices(); + // mapping from raw validator index to `ValidatorIndex` + // this computation is the same within a session, but should be cheap + let reverse_index = shuffled_indices + .iter() + .enumerate() + .map(|(i, v)| (v.0, ValidatorIndex(i as u32))) + .collect::>(); + + // we might have disabled validators who are not parachain validators + >::disabled_validators() + .iter() + .filter_map(|v| reverse_index.get(v).cloned()) + .collect() +} diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index fd3e656f6957..9933f6442974 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -49,7 +49,9 @@ use runtime_parachains::{ inclusion::{AggregateMessageOrigin, UmpQueueId}, initializer as parachains_initializer, origin as parachains_origin, paras as parachains_paras, paras_inherent as parachains_paras_inherent, - runtime_api_impl::v7 as parachains_runtime_api_impl, + runtime_api_impl::{ + v7 as parachains_runtime_api_impl, vstaging as parachains_staging_runtime_api_impl, + }, scheduler as parachains_scheduler, session_info as parachains_session_info, shared as parachains_shared, }; @@ -1585,7 +1587,7 @@ sp_api::impl_runtime_apis! { } } - #[api_version(7)] + #[api_version(8)] impl primitives::runtime_api::ParachainHost for Runtime { fn validators() -> Vec { parachains_runtime_api_impl::validators::() @@ -1728,6 +1730,11 @@ sp_api::impl_runtime_apis! { fn async_backing_params() -> primitives::AsyncBackingParams { parachains_runtime_api_impl::async_backing_params::() } + + fn disabled_validators() -> Vec { + parachains_staging_runtime_api_impl::disabled_validators::() + } + } #[api_version(3)] diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index d61acf36b4cb..0e93b3449fec 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -70,7 +70,9 @@ use runtime_parachains::{ inclusion::{AggregateMessageOrigin, UmpQueueId}, initializer as parachains_initializer, origin as parachains_origin, paras as parachains_paras, paras_inherent as parachains_paras_inherent, reward_points as parachains_reward_points, - runtime_api_impl::v7 as parachains_runtime_api_impl, + runtime_api_impl::{ + v7 as parachains_runtime_api_impl, vstaging as parachains_staging_runtime_api_impl, + }, scheduler as parachains_scheduler, session_info as parachains_session_info, shared as parachains_shared, }; @@ -1695,7 +1697,7 @@ sp_api::impl_runtime_apis! { } } - #[api_version(7)] + #[api_version(8)] impl primitives::runtime_api::ParachainHost for Runtime { fn validators() -> Vec { parachains_runtime_api_impl::validators::() @@ -1838,6 +1840,10 @@ sp_api::impl_runtime_apis! { fn async_backing_params() -> primitives::AsyncBackingParams { parachains_runtime_api_impl::async_backing_params::() } + + fn disabled_validators() -> Vec { + parachains_staging_runtime_api_impl::disabled_validators::() + } } impl beefy_primitives::BeefyApi for Runtime {