diff --git a/prdoc/pr_12711.prdoc b/prdoc/pr_12711.prdoc new file mode 100644 index 000000000000..91ab7eb3b242 --- /dev/null +++ b/prdoc/pr_12711.prdoc @@ -0,0 +1,10 @@ +title: 'pallet-multisig: add try_state invariant checks' + +doc: +- audience: Runtime Dev + description: |- + Adds a `try_state` hook to `pallet-multisig` (issue #239), run under the `try-runtime` feature and after every unit test. For each open operation it asserts that the approvals list is non empty, strictly sorted and duplicate free, and contains the depositor, and that every depositor's reserved balance covers the sum of its multisig deposits. The stored per-operation deposit is intentionally not asserted against the current deposit parameters, since it goes stale until poked when those parameters change. The change is additive with no migration or weight changes. + +crates: +- name: pallet-multisig + bump: minor diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index 08ceb5291d35..b96d4f9c10a4 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -60,6 +60,9 @@ pub use weights::WeightInfo; /// Re-export all pallet items. pub use pallet::*; +#[cfg(feature = "try-runtime")] +use frame::try_runtime::TryRuntimeError; + /// The log target of this pallet. pub const LOG_TARGET: &'static str = "runtime::multisig"; @@ -292,7 +295,14 @@ pub mod pallet { } #[pallet::hooks] - impl Hooks> for Pallet {} + impl Hooks> for Pallet { + #[cfg(feature = "try-runtime")] + fn try_state( + _n: frame_system::pallet_prelude::BlockNumberFor, + ) -> Result<(), TryRuntimeError> { + Self::do_try_state() + } + } #[pallet::call] impl Pallet { @@ -815,6 +825,42 @@ impl Pallet { } } +#[cfg(feature = "try-runtime")] +impl Pallet { + /// Invariants that must hold before and after every state transition of this pallet. + /// + /// Deposits are checked via the total reserved per depositor, not against `Deposit*` params, + /// which go stale until poked when changed. + pub fn do_try_state() -> Result<(), TryRuntimeError> { + use alloc::collections::btree_map::BTreeMap; + + let mut reserved_by_depositor: BTreeMap> = BTreeMap::new(); + for (_id, _call_hash, m) in Multisigs::::iter() { + ensure!(!m.approvals.is_empty(), "Multisig must have at least one approval"); + ensure!( + m.approvals.windows(2).all(|w| w[0] < w[1]), + "Multisig approvals must be strictly sorted and duplicate-free" + ); + ensure!( + m.approvals.contains(&m.depositor), + "Multisig depositor must be among the approvals" + ); + + let entry = reserved_by_depositor.entry(m.depositor).or_default(); + *entry = entry.saturating_add(m.deposit); + } + + for (depositor, reserved) in reserved_by_depositor { + ensure!( + T::Currency::reserved_balance(&depositor) >= reserved, + "Depositor's reserved balance must cover its multisig deposits" + ); + } + + Ok(()) + } +} + /// Return the weight of a dispatch call result as an `Option`. /// /// Will return the weight regardless of what the state of the result is. diff --git a/substrate/frame/multisig/src/tests.rs b/substrate/frame/multisig/src/tests.rs index c042189d1fd9..763ebaa7da71 100644 --- a/substrate/frame/multisig/src/tests.rs +++ b/substrate/frame/multisig/src/tests.rs @@ -90,6 +90,17 @@ pub fn new_test_ext() -> TestState { ext } +/// Run `test` in a fresh externality, then assert the pallet invariants hold. +/// +/// The `do_try_state` check only runs under the `try-runtime` feature. +fn new_test_ext_and_execute(test: impl FnOnce()) { + new_test_ext().execute_with(|| { + test(); + #[cfg(feature = "try-runtime")] + Multisig::do_try_state().expect("All invariants must hold after each test"); + }); +} + fn now() -> Timepoint { Multisig::timepoint() } @@ -100,7 +111,7 @@ fn call_transfer(dest: u64, value: u64) -> Box { #[test] fn multisig_deposit_is_taken_and_returned() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -134,7 +145,7 @@ fn multisig_deposit_is_taken_and_returned() { #[test] fn cancel_multisig_returns_deposit() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); assert_ok!(Multisig::approve_as_multi( @@ -163,7 +174,7 @@ fn cancel_multisig_returns_deposit() { #[test] fn timepoint_checking_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -221,7 +232,7 @@ fn timepoint_checking_works() { #[test] fn approve_as_multi_requires_timepoint_after_first_approval() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); @@ -262,7 +273,7 @@ fn approve_as_multi_requires_timepoint_after_first_approval() { #[test] fn multisig_2_of_3_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -307,7 +318,7 @@ fn multisig_2_of_3_works() { #[test] fn multisig_3_of_3_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 3); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -348,7 +359,7 @@ fn multisig_3_of_3_works() { #[test] fn cancel_multisig_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); assert_ok!(Multisig::approve_as_multi( @@ -377,7 +388,7 @@ fn cancel_multisig_works() { #[test] fn cancel_as_multi_not_found_with_wrong_signatories() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); // Create a multisig with signatories [1, 2, 3] @@ -414,7 +425,7 @@ fn cancel_as_multi_not_found_with_wrong_signatories() { #[test] fn cancel_as_multi_not_found_with_wrong_call_hash() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); // Create a multisig @@ -446,7 +457,7 @@ fn cancel_as_multi_not_found_with_wrong_call_hash() { #[test] fn approve_as_multi_unexpected_timepoint_with_wrong_signatories() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); // Create a multisig with signatories [1, 2, 3] @@ -487,7 +498,7 @@ fn approve_as_multi_unexpected_timepoint_with_wrong_signatories() { #[test] fn approve_as_multi_unexpected_timepoint_with_wrong_call_hash() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); // Create a multisig @@ -529,7 +540,7 @@ fn approve_as_multi_unexpected_timepoint_with_wrong_call_hash() { #[test] fn as_multi_unexpected_timepoint_with_wrong_signatories() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); // Create a multisig @@ -572,7 +583,7 @@ fn as_multi_unexpected_timepoint_with_wrong_signatories() { #[test] fn multisig_2_of_3_as_multi_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -604,7 +615,7 @@ fn multisig_2_of_3_as_multi_works() { #[test] fn multisig_2_of_3_as_multi_with_many_calls_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -655,7 +666,7 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() { #[test] fn multisig_2_of_3_cannot_reissue_same_call() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -714,7 +725,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() { #[test] fn minimum_threshold_check_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); assert_noop!( Multisig::as_multi( @@ -743,7 +754,7 @@ fn minimum_threshold_check_works() { #[test] fn too_many_signatories_fails() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); assert_noop!( Multisig::as_multi( @@ -761,7 +772,7 @@ fn too_many_signatories_fails() { #[test] fn as_multi_with_signatories_out_of_order_fails() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); assert_noop!( Multisig::as_multi( @@ -779,7 +790,7 @@ fn as_multi_with_signatories_out_of_order_fails() { #[test] fn approve_as_multi_with_signatories_out_of_order_fails() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); assert_noop!( @@ -798,7 +809,7 @@ fn approve_as_multi_with_signatories_out_of_order_fails() { #[test] fn cancel_as_multi_with_signatories_out_of_order_fails() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); @@ -837,7 +848,7 @@ fn cancel_as_multi_with_signatories_out_of_order_fails() { #[test] fn as_multi_with_sender_in_signatories_fails() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); assert_noop!( Multisig::as_multi( @@ -855,7 +866,7 @@ fn as_multi_with_sender_in_signatories_fails() { #[test] fn approve_as_multi_with_sender_in_signatories_fails() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); // First, create a valid multisig with account 1 @@ -884,7 +895,7 @@ fn approve_as_multi_with_sender_in_signatories_fails() { #[test] fn cancel_as_multi_with_sender_in_signatories_fails() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15).encode(); let hash = blake2_256(&call); // Create a multisig with account 1 @@ -912,7 +923,7 @@ fn cancel_as_multi_with_sender_in_signatories_fails() { #[test] fn duplicate_approvals_are_ignored() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); assert_ok!(Multisig::approve_as_multi( @@ -970,7 +981,7 @@ fn duplicate_approvals_are_ignored() { #[test] fn multisig_1_of_3_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 1); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -1022,7 +1033,7 @@ fn multisig_1_of_3_works() { #[test] fn multisig_filters() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = Box::new(RuntimeCall::System(frame_system::Call::set_code { code: vec![] })); assert_noop!( Multisig::as_multi_threshold_1(RuntimeOrigin::signed(1), vec![2], call.clone()), @@ -1033,7 +1044,7 @@ fn multisig_filters() { #[test] fn weight_check_works() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -1069,7 +1080,7 @@ fn multisig_handles_no_preimage_after_all_approve() { // This test checks the situation where everyone approves a multi-sig, but no-one provides the // call data. In the end, any of the multisig callers can approve again with the call data and // the call will go through. - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 3); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); @@ -1118,7 +1129,7 @@ fn multisig_handles_no_preimage_after_all_approve() { #[test] fn poke_deposit_fails_for_threshold_less_than_two() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); assert_noop!( @@ -1134,7 +1145,7 @@ fn poke_deposit_fails_for_threshold_less_than_two() { #[test] fn poke_deposit_fails_for_too_incorrect_number_of_signatories() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); assert_noop!( @@ -1150,7 +1161,7 @@ fn poke_deposit_fails_for_too_incorrect_number_of_signatories() { #[test] fn poke_deposit_fails_for_signatories_out_of_order() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); let threshold = 2; @@ -1178,7 +1189,7 @@ fn poke_deposit_fails_for_signatories_out_of_order() { #[test] fn poke_deposit_fails_for_non_existent_multisig() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { assert_noop!( Multisig::poke_deposit(RuntimeOrigin::signed(1), 2, vec![2, 3], [0; 32]), Error::::NotFound @@ -1188,7 +1199,7 @@ fn poke_deposit_fails_for_non_existent_multisig() { #[test] fn poke_deposit_fails_for_non_owner() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); let threshold = 2; @@ -1217,7 +1228,7 @@ fn poke_deposit_fails_for_non_owner() { #[test] fn poke_deposit_charges_fee_when_deposit_unchanged() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); let threshold = 2; @@ -1255,7 +1266,7 @@ fn poke_deposit_charges_fee_when_deposit_unchanged() { #[test] fn poke_deposit_works_when_deposit_increases() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); let threshold = 2; @@ -1325,7 +1336,7 @@ fn poke_deposit_works_when_deposit_increases() { #[test] fn poke_deposit_works_when_deposit_decreases() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { // Start with higher deposit requirements MultisigDepositBase::set(3); MultisigDepositFactor::set(2); @@ -1399,7 +1410,7 @@ fn poke_deposit_works_when_deposit_decreases() { #[test] fn poke_deposit_handles_insufficient_balance() { - new_test_ext().execute_with(|| { + new_test_ext_and_execute(|| { let call = call_transfer(6, 15); let hash = blake2_256(&call.encode()); let threshold = 2;