From 7a79f54a5d92cecba1d9c1e4da71df1e8a6ed91b Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Sat, 16 Jan 2021 18:47:28 +0100 Subject: [PATCH] Introduces account existence providers reference counting (#7363) * Initial draft * Latest changes * Final bits. * Fixes * Fixes * Test fixes * Fix tests * Fix babe tests * Fix * Fix * Fix * Fix * Fix * fix warnings in assets * Fix UI tests * fix line width * Fix * Update frame/system/src/lib.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update frame/system/src/lib.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Fix * fix unused warnings * Fix * Update frame/system/src/lib.rs Co-authored-by: Guillaume Thiolliere * Update frame/system/src/lib.rs Co-authored-by: Guillaume Thiolliere * Fix * fix slash and comprehensive slash test * fix reserved slash and comprehensive tests * check slash on non-existent account * Revert "Fix UI tests" This reverts commit e818dc7c0556baefe39b9cf3e34ff8546e96c590. * Fix * Fix utility tests * keep dispatch error backwards compatible * Fix * Fix * fix ui test * Companion checker shouldn't be so anal. * Fix * Fix * Fix * Apply suggestions from code review Co-authored-by: Alexander Popiak * Update frame/balances/src/lib.rs Co-authored-by: Alexander Popiak * return correct slash info when failing gracefully * fix missing import * Update frame/system/src/lib.rs Co-authored-by: Guillaume Thiolliere * Fix * Update frame/balances/src/tests_local.rs Co-authored-by: Guillaume Thiolliere * Fixes Co-authored-by: Shawn Tabrizi Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Guillaume Thiolliere Co-authored-by: Alexander Popiak --- .../gitlab/check_polkadot_companion_build.sh | 4 +- .../gitlab/check_polkadot_companion_status.sh | 4 +- bin/node/executor/tests/basic.rs | 16 +- bin/node/executor/tests/fees.rs | 19 +- bin/node/executor/tests/submit_transaction.rs | 2 +- frame/assets/src/lib.rs | 10 +- frame/babe/src/mock.rs | 28 +- frame/balances/src/lib.rs | 198 ++++++---- frame/balances/src/tests.rs | 198 +++++++++- frame/balances/src/tests_local.rs | 16 +- frame/executive/src/lib.rs | 2 +- frame/grandpa/src/mock.rs | 28 +- frame/offences/benchmarking/src/mock.rs | 2 +- frame/proxy/src/tests.rs | 4 +- frame/recovery/src/lib.rs | 6 +- frame/session/benchmarking/src/mock.rs | 2 +- frame/session/src/historical/mod.rs | 15 +- frame/session/src/historical/offchain.rs | 25 +- frame/session/src/lib.rs | 12 +- frame/session/src/mock.rs | 19 +- frame/session/src/tests.rs | 4 +- frame/staking/fuzzer/src/mock.rs | 2 +- frame/staking/src/benchmarking.rs | 2 +- frame/staking/src/lib.rs | 15 +- frame/staking/src/mock.rs | 10 + frame/staking/src/tests.rs | 8 +- frame/support/src/traits.rs | 162 ++++---- frame/system/benchmarking/src/lib.rs | 21 +- frame/system/src/extensions/check_nonce.rs | 3 +- frame/system/src/lib.rs | 352 +++++++++++------- frame/system/src/tests.rs | 25 +- frame/utility/src/tests.rs | 7 +- primitives/runtime/src/lib.rs | 21 +- primitives/runtime/src/traits.rs | 19 + 34 files changed, 814 insertions(+), 447 deletions(-) diff --git a/.maintain/gitlab/check_polkadot_companion_build.sh b/.maintain/gitlab/check_polkadot_companion_build.sh index 9e412ce26a892..90354f809d4ac 100755 --- a/.maintain/gitlab/check_polkadot_companion_build.sh +++ b/.maintain/gitlab/check_polkadot_companion_build.sh @@ -67,8 +67,8 @@ then pr_body="$(sed -n -r 's/^[[:space:]]+"body": (".*")[^"]+$/\1/p' "${pr_data_file}")" pr_companion="$(echo "${pr_body}" | sed -n -r \ - -e 's;^.*polkadot companion: paritytech/polkadot#([0-9]+).*$;\1;p' \ - -e 's;^.*polkadot companion: https://github.com/paritytech/polkadot/pull/([0-9]+).*$;\1;p' \ + -e 's;^.*[Cc]ompanion.*paritytech/polkadot#([0-9]+).*$;\1;p' \ + -e 's;^.*[Cc]ompanion.*https://github.com/paritytech/polkadot/pull/([0-9]+).*$;\1;p' \ | tail -n 1)" if [ "${pr_companion}" ] diff --git a/.maintain/gitlab/check_polkadot_companion_status.sh b/.maintain/gitlab/check_polkadot_companion_status.sh index 35c2983886f46..4714baf54fb2f 100755 --- a/.maintain/gitlab/check_polkadot_companion_status.sh +++ b/.maintain/gitlab/check_polkadot_companion_status.sh @@ -43,8 +43,8 @@ pr_body="$(curl -H "${github_header}" -s ${github_api_substrate_pull_url}/${CI_C # get companion if explicitly specified pr_companion="$(echo "${pr_body}" | sed -n -r \ - -e 's;^.*polkadot companion: paritytech/polkadot#([0-9]+).*$;\1;p' \ - -e 's;^.*polkadot companion: https://github.com/paritytech/polkadot/pull/([0-9]+).*$;\1;p' \ + -e 's;^.*[Cc]ompanion.*paritytech/polkadot#([0-9]+).*$;\1;p' \ + -e 's;^.*[Cc]ompanion.*https://github.com/paritytech/polkadot/pull/([0-9]+).*$;\1;p' \ | tail -n 1)" if [ -z "${pr_companion}" ] diff --git a/bin/node/executor/tests/basic.rs b/bin/node/executor/tests/basic.rs index 2b644fad2915b..26c04efe49991 100644 --- a/bin/node/executor/tests/basic.rs +++ b/bin/node/executor/tests/basic.rs @@ -192,7 +192,7 @@ fn bad_extrinsic_with_native_equivalent_code_gives_error() { let mut t = new_test_ext(compact_code_unwrap(), false); t.insert( >::hashed_key_for(alice()), - (0u32, 0u32, 69u128, 0u128, 0u128, 0u128).encode() + (0u32, 0u32, 0u32, 69u128, 0u128, 0u128, 0u128).encode() ); t.insert(>::hashed_key().to_vec(), 69_u128.encode()); t.insert(>::hashed_key_for(0), vec![0u8; 32]); @@ -221,11 +221,11 @@ fn successful_execution_with_native_equivalent_code_gives_ok() { let mut t = new_test_ext(compact_code_unwrap(), false); t.insert( >::hashed_key_for(alice()), - (0u32, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u32, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() ); t.insert( >::hashed_key_for(bob()), - (0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() ); t.insert( >::hashed_key().to_vec(), @@ -264,11 +264,11 @@ fn successful_execution_with_foreign_code_gives_ok() { let mut t = new_test_ext(bloaty_code_unwrap(), false); t.insert( >::hashed_key_for(alice()), - (0u32, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u32, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() ); t.insert( >::hashed_key_for(bob()), - (0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() ); t.insert( >::hashed_key().to_vec(), @@ -702,7 +702,7 @@ fn panic_execution_gives_error() { let mut t = new_test_ext(bloaty_code_unwrap(), false); t.insert( >::hashed_key_for(alice()), - (0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() ); t.insert(>::hashed_key().to_vec(), 0_u128.encode()); t.insert(>::hashed_key_for(0), vec![0u8; 32]); @@ -731,11 +731,11 @@ fn successful_execution_gives_ok() { let mut t = new_test_ext(compact_code_unwrap(), false); t.insert( >::hashed_key_for(alice()), - (0u32, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u32, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() ); t.insert( >::hashed_key_for(bob()), - (0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() ); t.insert( >::hashed_key().to_vec(), diff --git a/bin/node/executor/tests/fees.rs b/bin/node/executor/tests/fees.rs index 07460e54680d9..ed354d553448a 100644 --- a/bin/node/executor/tests/fees.rs +++ b/bin/node/executor/tests/fees.rs @@ -121,6 +121,15 @@ fn fee_multiplier_increases_and_decreases_on_big_weight() { }); } +fn new_account_info(free_dollars: u128) -> Vec { + frame_system::AccountInfo { + nonce: 0u32, + consumers: 0, + providers: 0, + data: (free_dollars * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS), + }.encode() +} + #[test] fn transaction_fee_is_correct() { // This uses the exact values of substrate-node. @@ -131,14 +140,8 @@ fn transaction_fee_is_correct() { // - 1 milli-dot based on current polkadot runtime. // (this baed on assigning 0.1 CENT to the cheapest tx with `weight = 100`) let mut t = new_test_ext(compact_code_unwrap(), false); - t.insert( - >::hashed_key_for(alice()), - (0u32, 0u32, 100 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode() - ); - t.insert( - >::hashed_key_for(bob()), - (0u32, 0u32, 10 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode() - ); + t.insert(>::hashed_key_for(alice()), new_account_info(100)); + t.insert(>::hashed_key_for(bob()), new_account_info(10)); t.insert( >::hashed_key().to_vec(), (110 * DOLLARS).encode() diff --git a/bin/node/executor/tests/submit_transaction.rs b/bin/node/executor/tests/submit_transaction.rs index f3cb90cbecdd2..c628826c62bef 100644 --- a/bin/node/executor/tests/submit_transaction.rs +++ b/bin/node/executor/tests/submit_transaction.rs @@ -253,7 +253,7 @@ fn submitted_transaction_should_be_valid() { let author = extrinsic.signature.clone().unwrap().0; let address = Indices::lookup(author).unwrap(); let data = pallet_balances::AccountData { free: 5_000_000_000_000, ..Default::default() }; - let account = frame_system::AccountInfo { nonce: 0, refcount: 0, data }; + let account = frame_system::AccountInfo { nonce: 0, consumers: 0, providers: 0, data }; >::insert(&address, account); // check validity diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 0455f35e24557..dcb77cc6ebfd2 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -274,6 +274,8 @@ decl_error! { MinBalanceZero, /// A mint operation lead to an overflow. Overflow, + /// Some internal state is broken. + BadState, } } @@ -863,7 +865,7 @@ impl Module { ) -> Result { let accounts = d.accounts.checked_add(1).ok_or(Error::::Overflow)?; let r = Ok(if frame_system::Module::::account_exists(who) { - frame_system::Module::::inc_ref(who); + frame_system::Module::::inc_consumers(who).map_err(|_| Error::::BadState)?; false } else { ensure!(d.zombies < d.max_zombies, Error::::TooManyZombies); @@ -881,7 +883,9 @@ impl Module { is_zombie: &mut bool, ) { if *is_zombie && frame_system::Module::::account_exists(who) { - frame_system::Module::::inc_ref(who); + // If the account exists, then it should have at least one provider + // so this cannot fail... but being defensive anyway. + let _ = frame_system::Module::::inc_consumers(who); *is_zombie = false; d.zombies = d.zombies.saturating_sub(1); } @@ -895,7 +899,7 @@ impl Module { if is_zombie { d.zombies = d.zombies.saturating_sub(1); } else { - frame_system::Module::::dec_ref(who); + frame_system::Module::::dec_consumers(who); } d.accounts = d.accounts.saturating_sub(1); } diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 58e2af873fd91..0a7576aa0778a 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -379,6 +379,14 @@ pub fn new_test_ext_raw_authorities(authorities: Vec) -> sp_io::Tes .build_storage::() .unwrap(); + let balances: Vec<_> = (0..authorities.len()) + .map(|i| (i as u64, 10_000_000)) + .collect(); + + pallet_balances::GenesisConfig:: { balances } + .assimilate_storage(&mut t) + .unwrap(); + // stashes are the index. let session_keys: Vec<_> = authorities .iter() @@ -394,6 +402,12 @@ pub fn new_test_ext_raw_authorities(authorities: Vec) -> sp_io::Tes }) .collect(); + // NOTE: this will initialize the babe authorities + // through OneSessionHandler::on_genesis_session + pallet_session::GenesisConfig:: { keys: session_keys } + .assimilate_storage(&mut t) + .unwrap(); + // controllers are the index + 1000 let stakers: Vec<_> = (0..authorities.len()) .map(|i| { @@ -406,20 +420,6 @@ pub fn new_test_ext_raw_authorities(authorities: Vec) -> sp_io::Tes }) .collect(); - let balances: Vec<_> = (0..authorities.len()) - .map(|i| (i as u64, 10_000_000)) - .collect(); - - // NOTE: this will initialize the babe authorities - // through OneSessionHandler::on_genesis_session - pallet_session::GenesisConfig:: { keys: session_keys } - .assimilate_storage(&mut t) - .unwrap(); - - pallet_balances::GenesisConfig:: { balances } - .assimilate_storage(&mut t) - .unwrap(); - let staking_config = pallet_staking::GenesisConfig:: { stakers, validator_count: 8, diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 10451aca15b1a..ef069455bbabe 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -157,22 +157,22 @@ mod benchmarking; pub mod weights; use sp_std::prelude::*; -use sp_std::{cmp, result, mem, fmt::Debug, ops::BitOr, convert::Infallible}; +use sp_std::{cmp, result, mem, fmt::Debug, ops::BitOr}; use codec::{Codec, Encode, Decode}; use frame_support::{ StorageValue, Parameter, decl_event, decl_storage, decl_module, decl_error, ensure, traits::{ - Currency, OnKilledAccount, OnUnbalanced, TryDrop, StoredMap, + Currency, OnUnbalanced, TryDrop, StoredMap, WithdrawReasons, LockIdentifier, LockableCurrency, ExistenceRequirement, Imbalance, SignedImbalance, ReservableCurrency, Get, ExistenceRequirement::KeepAlive, - ExistenceRequirement::AllowDeath, IsDeadAccount, BalanceStatus as Status, + ExistenceRequirement::AllowDeath, BalanceStatus as Status, } }; use sp_runtime::{ RuntimeDebug, DispatchResult, DispatchError, traits::{ Zero, AtLeast32BitUnsigned, StaticLookup, Member, CheckedAdd, CheckedSub, - MaybeSerializeDeserialize, Saturating, Bounded, + MaybeSerializeDeserialize, Saturating, Bounded, StoredMapError, }, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -419,7 +419,7 @@ decl_storage! { assert!(endowed_accounts.len() == config.balances.len(), "duplicate balances in genesis."); for &(ref who, free) in config.balances.iter() { - T::AccountStore::insert(who, AccountData { free, .. Default::default() }); + assert!(T::AccountStore::insert(who, AccountData { free, .. Default::default() }).is_ok()); } }); } @@ -524,7 +524,7 @@ decl_module! { account.reserved = new_reserved; (account.free, account.reserved) - }); + })?; Self::deposit_event(RawEvent::BalanceSet(who, free, reserved)); } @@ -634,9 +634,8 @@ impl, I: Instance> Module { pub fn mutate_account( who: &T::AccountId, f: impl FnOnce(&mut AccountData) -> R - ) -> R { - Self::try_mutate_account(who, |a, _| -> Result { Ok(f(a)) }) - .expect("Error is infallible; qed") + ) -> Result { + Self::try_mutate_account(who, |a, _| -> Result { Ok(f(a)) }) } /// Mutate an account to some new value, or delete it entirely with `None`. Will enforce @@ -648,7 +647,7 @@ impl, I: Instance> Module { /// /// NOTE: LOW-LEVEL: This will not attempt to maintain total issuance. It is expected that /// the caller will do this. - fn try_mutate_account( + fn try_mutate_account>( who: &T::AccountId, f: impl FnOnce(&mut AccountData, bool) -> Result ) -> Result { @@ -676,7 +675,8 @@ impl, I: Instance> Module { A runtime configuration adjustment may be needed." ); } - Self::mutate_account(who, |b| { + // No way this can fail since we do not alter the existential balances. + let _ = Self::mutate_account(who, |b| { b.misc_frozen = Zero::zero(); b.fee_frozen = Zero::zero(); for l in locks.iter() { @@ -695,12 +695,20 @@ impl, I: Instance> Module { if existed { // TODO: use Locks::::hashed_key // https://github.com/paritytech/substrate/issues/4969 - system::Module::::dec_ref(who); + system::Module::::dec_consumers(who); } } else { Locks::::insert(who, locks); if !existed { - system::Module::::inc_ref(who); + if system::Module::::inc_consumers(who).is_err() { + // No providers for the locks. This is impossible under normal circumstances + // since the funds that are under the lock will themselves be stored in the + // account and therefore will need a reference. + frame_support::debug::warn!( + "Warning: Attempt to introduce lock consumer reference, yet no providers. \ + This is unexpected but should be safe." + ); + } } } } @@ -711,13 +719,14 @@ impl, I: Instance> Module { mod imbalances { use super::{ result, DefaultInstance, Imbalance, Config, Zero, Instance, Saturating, - StorageValue, TryDrop, + StorageValue, TryDrop, RuntimeDebug, }; use sp_std::mem; /// Opaque, move-only struct with private fields that serves as a token denoting that /// funds have been created without any equal and opposite accounting. #[must_use] + #[derive(RuntimeDebug, PartialEq, Eq)] pub struct PositiveImbalance, I: Instance=DefaultInstance>(T::Balance); impl, I: Instance> PositiveImbalance { @@ -730,6 +739,7 @@ mod imbalances { /// Opaque, move-only struct with private fields that serves as a token denoting that /// funds have been destroyed without any equal and opposite accounting. #[must_use] + #[derive(RuntimeDebug, PartialEq, Eq)] pub struct NegativeImbalance, I: Instance=DefaultInstance>(T::Balance); impl, I: Instance> NegativeImbalance { @@ -963,10 +973,12 @@ impl, I: Instance> Currency for Module where value, WithdrawReasons::TRANSFER, from_account.free, - )?; + ).map_err(|_| Error::::LiquidityRestrictions)?; + // TODO: This is over-conservative. There may now be other providers, and this module + // may not even be a provider. let allow_death = existence_requirement == ExistenceRequirement::AllowDeath; - let allow_death = allow_death && system::Module::::allow_death(transactor); + let allow_death = allow_death && !system::Module::::is_provider_required(transactor); ensure!(allow_death || from_account.free >= ed, Error::::KeepAlive); Ok(()) @@ -993,21 +1005,48 @@ impl, I: Instance> Currency for Module where value: Self::Balance ) -> (Self::NegativeImbalance, Self::Balance) { if value.is_zero() { return (NegativeImbalance::zero(), Zero::zero()) } - if Self::is_dead_account(&who) { return (NegativeImbalance::zero(), value) } - - Self::mutate_account(who, |account| { - let free_slash = cmp::min(account.free, value); - account.free -= free_slash; - - let remaining_slash = value - free_slash; - if !remaining_slash.is_zero() { - let reserved_slash = cmp::min(account.reserved, remaining_slash); - account.reserved -= reserved_slash; - (NegativeImbalance::new(free_slash + reserved_slash), remaining_slash - reserved_slash) - } else { - (NegativeImbalance::new(value), Zero::zero()) + if Self::total_balance(&who).is_zero() { return (NegativeImbalance::zero(), value) } + + for attempt in 0..2 { + match Self::try_mutate_account(who, + |account, _is_new| -> Result<(Self::NegativeImbalance, Self::Balance), StoredMapError> { + // Best value is the most amount we can slash following liveness rules. + let best_value = match attempt { + // First attempt we try to slash the full amount, and see if liveness issues happen. + 0 => value, + // If acting as a critical provider (i.e. first attempt failed), then slash + // as much as possible while leaving at least at ED. + _ => value.min((account.free + account.reserved).saturating_sub(T::ExistentialDeposit::get())), + }; + + let free_slash = cmp::min(account.free, best_value); + account.free -= free_slash; // Safe because of above check + let remaining_slash = best_value - free_slash; // Safe because of above check + + if !remaining_slash.is_zero() { + // If we have remaining slash, take it from reserved balance. + let reserved_slash = cmp::min(account.reserved, remaining_slash); + account.reserved -= reserved_slash; // Safe because of above check + Ok(( + NegativeImbalance::new(free_slash + reserved_slash), + value - free_slash - reserved_slash, // Safe because value is gt or eq total slashed + )) + } else { + // Else we are done! + Ok(( + NegativeImbalance::new(free_slash), + value - free_slash, // Safe because value is gt or eq to total slashed + )) + } + } + ) { + Ok(r) => return r, + Err(_) => (), } - }) + } + + // Should never get here. But we'll be defensive anyway. + (Self::NegativeImbalance::zero(), value) } /// Deposit some `value` into the free balance of an existing target account `who`. @@ -1030,7 +1069,8 @@ impl, I: Instance> Currency for Module where /// /// This function is a no-op if: /// - the `value` to be deposited is zero; or - /// - if the `value` to be deposited is less than the ED and the account does not yet exist; or + /// - the `value` to be deposited is less than the required ED and the account does not yet exist; or + /// - the deposit would necessitate the account to exist and there are no provider references; or /// - `value` is so large it would cause the balance of `who` to overflow. fn deposit_creating( who: &T::AccountId, @@ -1038,17 +1078,22 @@ impl, I: Instance> Currency for Module where ) -> Self::PositiveImbalance { if value.is_zero() { return Self::PositiveImbalance::zero() } - Self::try_mutate_account(who, |account, is_new| -> Result { - // bail if not yet created and this operation wouldn't be enough to create it. + let r = Self::try_mutate_account(who, |account, is_new| -> Result { + let ed = T::ExistentialDeposit::get(); - ensure!(value >= ed || !is_new, Self::PositiveImbalance::zero()); + ensure!(value >= ed || !is_new, Error::::ExistentialDeposit); // defensive only: overflow should never happen, however in case it does, then this // operation is a no-op. - account.free = account.free.checked_add(&value).ok_or_else(|| Self::PositiveImbalance::zero())?; + account.free = match account.free.checked_add(&value) { + Some(x) => x, + None => return Ok(Self::PositiveImbalance::zero()), + }; Ok(PositiveImbalance::new(value)) - }).unwrap_or_else(|x| x) + }).unwrap_or_else(|_| Self::PositiveImbalance::zero()); + + r } /// Withdraw some free balance from an account, respecting existence requirements. @@ -1087,9 +1132,10 @@ impl, I: Instance> Currency for Module where -> SignedImbalance { Self::try_mutate_account(who, |account, is_new| - -> Result, ()> + -> Result, DispatchError> { let ed = T::ExistentialDeposit::get(); + let total = value.saturating_add(account.reserved); // If we're attempting to set an existing account to less than ED, then // bypass the entire operation. It's a no-op if you follow it through, but // since this is an instance where we might account for a negative imbalance @@ -1097,7 +1143,7 @@ impl, I: Instance> Currency for Module where // equal and opposite cause (returned as an Imbalance), then in the // instance that there's no other accounts on the system at all, we might // underflow the issuance and our arithmetic will be off. - ensure!(value.saturating_add(account.reserved) >= ed || !is_new, ()); + ensure!(total >= ed || !is_new, Error::::ExistentialDeposit); let imbalance = if account.free <= value { SignedImbalance::Positive(PositiveImbalance::new(value - account.free)) @@ -1150,16 +1196,24 @@ impl, I: Instance> ReservableCurrency for Module Self::Balance { if value.is_zero() { return Zero::zero() } - if Self::is_dead_account(&who) { return value } + if Self::total_balance(&who).is_zero() { return value } - let actual = Self::mutate_account(who, |account| { + let actual = match Self::mutate_account(who, |account| { let actual = cmp::min(account.reserved, value); account.reserved -= actual; // defensive only: this can never fail since total issuance which is at least free+reserved // fits into the same data type. account.free = account.free.saturating_add(actual); actual - }); + }) { + Ok(x) => x, + Err(_) => { + // This should never happen since we don't alter the total amount in the account. + // If it ever does, then we should fail gracefully though, indicating that nothing + // could be done. + return value + } + }; Self::deposit_event(RawEvent::Unreserved(who.clone(), actual.clone())); value - actual @@ -1174,14 +1228,33 @@ impl, I: Instance> ReservableCurrency for Module (Self::NegativeImbalance, Self::Balance) { if value.is_zero() { return (NegativeImbalance::zero(), Zero::zero()) } - if Self::is_dead_account(&who) { return (NegativeImbalance::zero(), value) } - - Self::mutate_account(who, |account| { - // underflow should never happen, but it if does, there's nothing to be done here. - let actual = cmp::min(account.reserved, value); - account.reserved -= actual; - (NegativeImbalance::new(actual), value - actual) - }) + if Self::total_balance(&who).is_zero() { return (NegativeImbalance::zero(), value) } + + // NOTE: `mutate_account` may fail if it attempts to reduce the balance to the point that an + // account is attempted to be illegally destroyed. + + for attempt in 0..2 { + match Self::mutate_account(who, |account| { + let best_value = match attempt { + 0 => value, + // If acting as a critical provider (i.e. first attempt failed), then ensure + // slash leaves at least the ED. + _ => value.min((account.free + account.reserved).saturating_sub(T::ExistentialDeposit::get())), + }; + + let actual = cmp::min(account.reserved, best_value); + account.reserved -= actual; + + // underflow should never happen, but it if does, there's nothing to be done here. + (NegativeImbalance::new(actual), value - actual) + }) { + Ok(r) => return r, + Err(_) => (), + } + } + // Should never get here as we ensure that ED is left in the second attempt. + // In case we do, though, then we fail gracefully. + (Self::NegativeImbalance::zero(), value) } /// Move the reserved balance of one account into the balance of another, according to `status`. @@ -1222,24 +1295,6 @@ impl, I: Instance> ReservableCurrency for Module, I: Instance> OnKilledAccount for Module { - fn on_killed_account(who: &T::AccountId) { - Account::::mutate_exists(who, |account| { - let total = account.as_ref().map(|acc| acc.total()).unwrap_or_default(); - if !total.is_zero() { - T::DustRemoval::on_unbalanced(NegativeImbalance::new(total)); - Self::deposit_event(RawEvent::DustLost(who.clone(), total)); - } - *account = None; - }); - } -} - impl, I: Instance> LockableCurrency for Module where T::Balance: MaybeSerializeDeserialize + Debug @@ -1304,12 +1359,3 @@ where Self::update_locks(who, &locks[..]); } } - -impl, I: Instance> IsDeadAccount for Module where - T::Balance: MaybeSerializeDeserialize + Debug -{ - fn is_dead_account(who: &T::AccountId) -> bool { - // this should always be exactly equivalent to `Self::account(who).total().is_zero()` if ExistentialDeposit > 0 - !T::AccountStore::is_explicit(who) - } -} diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 1c120272dd0b6..90e8e0d7cbdcf 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -43,7 +43,7 @@ macro_rules! decl_tests { assert_noop, assert_storage_noop, assert_ok, assert_err, traits::{ LockableCurrency, LockIdentifier, WithdrawReasons, - Currency, ReservableCurrency, ExistenceRequirement::AllowDeath, StoredMap + Currency, ReservableCurrency, ExistenceRequirement::AllowDeath } }; use pallet_transaction_payment::{ChargeTransactionPayment, Multiplier}; @@ -91,7 +91,8 @@ macro_rules! decl_tests { <$ext_builder>::default().existential_deposit(1).monied(true).build().execute_with(|| { assert_eq!(Balances::free_balance(1), 10); assert_ok!(>::transfer(&1, &2, 10, AllowDeath)); - assert!(!<::AccountStore as StoredMap>>::is_explicit(&1)); + // Check that the account is dead. + assert!(!frame_system::Account::::contains_key(&1)); }); } @@ -262,14 +263,12 @@ macro_rules! decl_tests { .monied(true) .build() .execute_with(|| { - assert_eq!(Balances::is_dead_account(&5), true); // account 5 should not exist // ext_deposit is 10, value is 9, not satisfies for ext_deposit assert_noop!( Balances::transfer(Some(1).into(), 5, 9), Error::<$test, _>::ExistentialDeposit, ); - assert_eq!(Balances::is_dead_account(&5), true); // account 5 should not exist assert_eq!(Balances::free_balance(1), 100); }); } @@ -282,31 +281,25 @@ macro_rules! decl_tests { .build() .execute_with(|| { System::inc_account_nonce(&2); - assert_eq!(Balances::is_dead_account(&2), false); - assert_eq!(Balances::is_dead_account(&5), true); assert_eq!(Balances::total_balance(&2), 256 * 20); assert_ok!(Balances::reserve(&2, 256 * 19 + 1)); // account 2 becomes mostly reserved assert_eq!(Balances::free_balance(2), 255); // "free" account deleted." assert_eq!(Balances::total_balance(&2), 256 * 20); // reserve still exists. - assert_eq!(Balances::is_dead_account(&2), false); assert_eq!(System::account_nonce(&2), 1); // account 4 tries to take index 1 for account 5. assert_ok!(Balances::transfer(Some(4).into(), 5, 256 * 1 + 0x69)); assert_eq!(Balances::total_balance(&5), 256 * 1 + 0x69); - assert_eq!(Balances::is_dead_account(&5), false); assert!(Balances::slash(&2, 256 * 19 + 2).1.is_zero()); // account 2 gets slashed // "reserve" account reduced to 255 (below ED) so account deleted assert_eq!(Balances::total_balance(&2), 0); assert_eq!(System::account_nonce(&2), 0); // nonce zero - assert_eq!(Balances::is_dead_account(&2), true); // account 4 tries to take index 1 again for account 6. assert_ok!(Balances::transfer(Some(4).into(), 6, 256 * 1 + 0x69)); assert_eq!(Balances::total_balance(&6), 256 * 1 + 0x69); - assert_eq!(Balances::is_dead_account(&6), false); }); } @@ -417,7 +410,7 @@ macro_rules! decl_tests { fn refunding_balance_should_work() { <$ext_builder>::default().build().execute_with(|| { let _ = Balances::deposit_creating(&1, 42); - Balances::mutate_account(&1, |a| a.reserved = 69); + assert!(Balances::mutate_account(&1, |a| a.reserved = 69).is_ok()); Balances::unreserve(&1, 69); assert_eq!(Balances::free_balance(1), 111); assert_eq!(Balances::reserved_balance(1), 0); @@ -623,7 +616,6 @@ macro_rules! decl_tests { Balances::transfer_keep_alive(Some(1).into(), 2, 100), Error::<$test, _>::KeepAlive ); - assert_eq!(Balances::is_dead_account(&1), false); assert_eq!(Balances::total_balance(&1), 100); assert_eq!(Balances::total_balance(&2), 0); }); @@ -695,7 +687,6 @@ macro_rules! decl_tests { // Reserve some free balance let _ = Balances::slash(&1, 1); // The account should be dead. - assert!(Balances::is_dead_account(&1)); assert_eq!(Balances::free_balance(1), 0); assert_eq!(Balances::reserved_balance(1), 0); }); @@ -767,7 +758,7 @@ macro_rules! decl_tests { #[test] fn emit_events_with_no_existential_deposit_suicide() { <$ext_builder>::default() - .existential_deposit(0) + .existential_deposit(1) .build() .execute_with(|| { assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 100, 0)); @@ -783,11 +774,6 @@ macro_rules! decl_tests { let _ = Balances::slash(&1, 100); - // no events - assert_eq!(events(), []); - - assert_ok!(System::suicide(Origin::signed(1))); - assert_eq!( events(), [ @@ -797,6 +783,178 @@ macro_rules! decl_tests { }); } + #[test] + fn slash_loop_works() { + <$ext_builder>::default() + .existential_deposit(100) + .build() + .execute_with(|| { + /* User has no reference counter, so they can die in these scenarios */ + + // SCENARIO: Slash would not kill account. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 0)); + // Slashed completed in full + assert_eq!(Balances::slash(&1, 900), (NegativeImbalance::new(900), 0)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Slash will kill account because not enough balance left. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 0)); + // Slashed completed in full + assert_eq!(Balances::slash(&1, 950), (NegativeImbalance::new(950), 0)); + // Account is killed + assert!(!System::account_exists(&1)); + + // SCENARIO: Over-slash will kill account, and report missing slash amount. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 0)); + // Slashed full free_balance, and reports 300 not slashed + assert_eq!(Balances::slash(&1, 1_300), (NegativeImbalance::new(1000), 300)); + // Account is dead + assert!(!System::account_exists(&1)); + + // SCENARIO: Over-slash can take from reserved, but keep alive. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 400)); + // Slashed full free_balance and 300 of reserved balance + assert_eq!(Balances::slash(&1, 1_300), (NegativeImbalance::new(1300), 0)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Over-slash can take from reserved, and kill. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 350)); + // Slashed full free_balance and 300 of reserved balance + assert_eq!(Balances::slash(&1, 1_300), (NegativeImbalance::new(1300), 0)); + // Account is dead because 50 reserved balance is not enough to keep alive + assert!(!System::account_exists(&1)); + + // SCENARIO: Over-slash can take as much as possible from reserved, kill, and report missing amount. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 250)); + // Slashed full free_balance and 300 of reserved balance + assert_eq!(Balances::slash(&1, 1_300), (NegativeImbalance::new(1250), 50)); + // Account is super dead + assert!(!System::account_exists(&1)); + + /* User will now have a reference counter on them, keeping them alive in these scenarios */ + + // SCENARIO: Slash would not kill account. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 0)); + assert_ok!(System::inc_consumers(&1)); // <-- Reference counter added here is enough for all tests + // Slashed completed in full + assert_eq!(Balances::slash(&1, 900), (NegativeImbalance::new(900), 0)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Slash will take as much as possible without killing account. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 0)); + // Slashed completed in full + assert_eq!(Balances::slash(&1, 950), (NegativeImbalance::new(900), 50)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Over-slash will not kill account, and report missing slash amount. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 0)); + // Slashed full free_balance minus ED, and reports 400 not slashed + assert_eq!(Balances::slash(&1, 1_300), (NegativeImbalance::new(900), 400)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Over-slash can take from reserved, but keep alive. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 400)); + // Slashed full free_balance and 300 of reserved balance + assert_eq!(Balances::slash(&1, 1_300), (NegativeImbalance::new(1300), 0)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Over-slash can take from reserved, but keep alive. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 350)); + // Slashed full free_balance and 250 of reserved balance to leave ED + assert_eq!(Balances::slash(&1, 1_300), (NegativeImbalance::new(1250), 50)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Over-slash can take as much as possible from reserved and report missing amount. + assert_ok!(Balances::set_balance(Origin::root(), 1, 1_000, 250)); + // Slashed full free_balance and 300 of reserved balance + assert_eq!(Balances::slash(&1, 1_300), (NegativeImbalance::new(1150), 150)); + // Account is still alive + assert!(System::account_exists(&1)); + + // Slash on non-existent account is okay. + assert_eq!(Balances::slash(&12345, 1_300), (NegativeImbalance::new(0), 1300)); + }); + } + + #[test] + fn slash_reserved_loop_works() { + <$ext_builder>::default() + .existential_deposit(100) + .build() + .execute_with(|| { + /* User has no reference counter, so they can die in these scenarios */ + + // SCENARIO: Slash would not kill account. + assert_ok!(Balances::set_balance(Origin::root(), 1, 50, 1_000)); + // Slashed completed in full + assert_eq!(Balances::slash_reserved(&1, 900), (NegativeImbalance::new(900), 0)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Slash would kill account. + assert_ok!(Balances::set_balance(Origin::root(), 1, 50, 1_000)); + // Slashed completed in full + assert_eq!(Balances::slash_reserved(&1, 1_000), (NegativeImbalance::new(1_000), 0)); + // Account is dead + assert!(!System::account_exists(&1)); + + // SCENARIO: Over-slash would kill account, and reports left over slash. + assert_ok!(Balances::set_balance(Origin::root(), 1, 50, 1_000)); + // Slashed completed in full + assert_eq!(Balances::slash_reserved(&1, 1_300), (NegativeImbalance::new(1_000), 300)); + // Account is dead + assert!(!System::account_exists(&1)); + + // SCENARIO: Over-slash does not take from free balance. + assert_ok!(Balances::set_balance(Origin::root(), 1, 300, 1_000)); + // Slashed completed in full + assert_eq!(Balances::slash_reserved(&1, 1_300), (NegativeImbalance::new(1_000), 300)); + // Account is alive because of free balance + assert!(System::account_exists(&1)); + + /* User has a reference counter, so they cannot die */ + + // SCENARIO: Slash would not kill account. + assert_ok!(Balances::set_balance(Origin::root(), 1, 50, 1_000)); + assert_ok!(System::inc_consumers(&1)); // <-- Reference counter added here is enough for all tests + // Slashed completed in full + assert_eq!(Balances::slash_reserved(&1, 900), (NegativeImbalance::new(900), 0)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Slash as much as possible without killing. + assert_ok!(Balances::set_balance(Origin::root(), 1, 50, 1_000)); + // Slashed as much as possible + assert_eq!(Balances::slash_reserved(&1, 1_000), (NegativeImbalance::new(950), 50)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Over-slash reports correctly, where reserved is needed to keep alive. + assert_ok!(Balances::set_balance(Origin::root(), 1, 50, 1_000)); + // Slashed as much as possible + assert_eq!(Balances::slash_reserved(&1, 1_300), (NegativeImbalance::new(950), 350)); + // Account is still alive + assert!(System::account_exists(&1)); + + // SCENARIO: Over-slash reports correctly, where full reserved is removed. + assert_ok!(Balances::set_balance(Origin::root(), 1, 200, 1_000)); + // Slashed as much as possible + assert_eq!(Balances::slash_reserved(&1, 1_300), (NegativeImbalance::new(1_000), 300)); + // Account is still alive + assert!(System::account_exists(&1)); + + // Slash on non-existent account is okay. + assert_eq!(Balances::slash_reserved(&12345, 1_300), (NegativeImbalance::new(0), 1300)); + }); + } + #[test] fn operations_on_dead_account_should_not_change_state() { // These functions all use `mutate_account` which may introduce a storage change when @@ -805,7 +963,7 @@ macro_rules! decl_tests { .existential_deposit(0) .build() .execute_with(|| { - assert!(!::AccountStore::is_explicit(&1337)); + assert!(!frame_system::Account::::contains_key(&1337)); // Unreserve assert_storage_noop!(assert_eq!(Balances::unreserve(&1337, 42), 42)); diff --git a/frame/balances/src/tests_local.rs b/frame/balances/src/tests_local.rs index 887b280945f1a..2cbf46709275a 100644 --- a/frame/balances/src/tests_local.rs +++ b/frame/balances/src/tests_local.rs @@ -74,9 +74,9 @@ impl frame_system::Config for Test { type BlockHashCount = BlockHashCount; type Version = (); type PalletInfo = (); - type AccountData = super::AccountData; + type AccountData = (); type OnNewAccount = (); - type OnKilledAccount = Module; + type OnKilledAccount = (); type SystemWeightInfo = (); type SS58Prefix = (); } @@ -99,9 +99,9 @@ impl Config for Test { type ExistentialDeposit = ExistentialDeposit; type AccountStore = StorageMapShim< super::Account, - system::CallOnCreatedAccount, - system::CallKillAccount, - u64, super::AccountData + system::Provider, + u64, + super::AccountData, >; type MaxLocks = MaxLocks; type WeightInfo = (); @@ -162,7 +162,7 @@ decl_tests!{ Test, ExtBuilder, EXISTENTIAL_DEPOSIT } #[test] fn emit_events_with_no_existential_deposit_suicide_with_dust() { ::default() - .existential_deposit(0) + .existential_deposit(2) .build() .execute_with(|| { assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 100, 0)); @@ -176,12 +176,12 @@ fn emit_events_with_no_existential_deposit_suicide_with_dust() { ] ); - let _ = Balances::slash(&1, 99); + let _ = Balances::slash(&1, 98); // no events assert_eq!(events(), []); - assert_ok!(System::suicide(Origin::signed(1))); + let _ = Balances::slash(&1, 1); assert_eq!( events(), diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index fdde914b07e04..05b4b0f982a0b 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -749,7 +749,7 @@ mod tests { header: Header { parent_hash: [69u8; 32].into(), number: 1, - state_root: hex!("ba1a82a264b8007e0c04c9ea35e541593daad08b6e2bf7c0a6780a67d1c55018").into(), + state_root: hex!("1599922f15b2d5cf75e83370e29e13b96fdf799d917a5b6319736af292f21665").into(), extrinsics_root: hex!("03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314").into(), digest: Digest { logs: vec![], }, }, diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index bf4ce5a519e7c..fd7230fd9ceb0 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -287,6 +287,14 @@ pub fn new_test_ext_raw_authorities(authorities: AuthorityList) -> sp_io::TestEx .build_storage::() .unwrap(); + let balances: Vec<_> = (0..authorities.len()) + .map(|i| (i as u64, 10_000_000)) + .collect(); + + pallet_balances::GenesisConfig:: { balances } + .assimilate_storage(&mut t) + .unwrap(); + // stashes are the index. let session_keys: Vec<_> = authorities .iter() @@ -302,6 +310,12 @@ pub fn new_test_ext_raw_authorities(authorities: AuthorityList) -> sp_io::TestEx }) .collect(); + // NOTE: this will initialize the grandpa authorities + // through OneSessionHandler::on_genesis_session + pallet_session::GenesisConfig:: { keys: session_keys } + .assimilate_storage(&mut t) + .unwrap(); + // controllers are the index + 1000 let stakers: Vec<_> = (0..authorities.len()) .map(|i| { @@ -314,20 +328,6 @@ pub fn new_test_ext_raw_authorities(authorities: AuthorityList) -> sp_io::TestEx }) .collect(); - let balances: Vec<_> = (0..authorities.len()) - .map(|i| (i as u64, 10_000_000)) - .collect(); - - // NOTE: this will initialize the grandpa authorities - // through OneSessionHandler::on_genesis_session - pallet_session::GenesisConfig:: { keys: session_keys } - .assimilate_storage(&mut t) - .unwrap(); - - pallet_balances::GenesisConfig:: { balances } - .assimilate_storage(&mut t) - .unwrap(); - let staking_config = pallet_staking::GenesisConfig:: { stakers, validator_count: 8, diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index 8e0bb361e15ce..20fd3ba9b0670 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -61,7 +61,7 @@ impl frame_system::Config for Test { type PalletInfo = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnKilledAccount = (Balances,); + type OnKilledAccount = (); type SystemWeightInfo = (); type SS58Prefix = (); } diff --git a/frame/proxy/src/tests.rs b/frame/proxy/src/tests.rs index 6867dd510dd9e..3c417647c2cd1 100644 --- a/frame/proxy/src/tests.rs +++ b/frame/proxy/src/tests.rs @@ -317,7 +317,7 @@ fn proxy_announced_removes_announcement_and_returns_deposit() { #[test] fn filtering_works() { new_test_ext().execute_with(|| { - Balances::mutate_account(&1, |a| a.free = 1000); + assert!(Balances::mutate_account(&1, |a| a.free = 1000).is_ok()); assert_ok!(Proxy::add_proxy(Origin::signed(1), 2, ProxyType::Any, 0)); assert_ok!(Proxy::add_proxy(Origin::signed(1), 3, ProxyType::JustTransfer, 0)); assert_ok!(Proxy::add_proxy(Origin::signed(1), 4, ProxyType::JustUtility, 0)); @@ -331,7 +331,7 @@ fn filtering_works() { expect_event(RawEvent::ProxyExecuted(Err(DispatchError::BadOrigin))); let derivative_id = Utility::derivative_account_id(1, 0); - Balances::mutate_account(&derivative_id, |a| a.free = 1000); + assert!(Balances::mutate_account(&derivative_id, |a| a.free = 1000).is_ok()); let inner = Box::new(Call::Balances(BalancesCall::transfer(6, 1))); let call = Box::new(Call::Utility(UtilityCall::as_derivative(0, inner.clone()))); diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 606cb82250773..00cd6ff2a7f79 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -317,6 +317,8 @@ decl_error! { Overflow, /// This account is already set up for recovery AlreadyProxy, + /// Some internal state is broken. + BadState, } } @@ -586,9 +588,9 @@ decl_module! { recovery_config.threshold as usize <= active_recovery.friends.len(), Error::::Threshold ); + system::Module::::inc_consumers(&who).map_err(|_| Error::::BadState)?; // Create the recovery storage item Proxy::::insert(&who, &account); - system::Module::::inc_ref(&who); Self::deposit_event(RawEvent::AccountRecovered(account, who)); } @@ -675,7 +677,7 @@ decl_module! { // Check `who` is allowed to make a call on behalf of `account` ensure!(Self::proxy(&who) == Some(account), Error::::NotAllowed); Proxy::::remove(&who); - system::Module::::dec_ref(&who); + system::Module::::dec_consumers(&who); } } } diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index 31593b3da54b3..711cde8e8ecf5 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -65,7 +65,7 @@ impl frame_system::Config for Test { type PalletInfo = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnKilledAccount = Balances; + type OnKilledAccount = (); type SystemWeightInfo = (); type SS58Prefix = (); } diff --git a/frame/session/src/historical/mod.rs b/frame/session/src/historical/mod.rs index 85d7c3f3f349e..48f32af7b474c 100644 --- a/frame/session/src/historical/mod.rs +++ b/frame/session/src/historical/mod.rs @@ -327,16 +327,21 @@ pub(crate) mod tests { set_next_validators, Test, System, Session, }; use frame_support::traits::{KeyOwnerProofSystem, OnInitialize}; + use frame_support::BasicExternalities; type Historical = Module; pub(crate) fn new_test_ext() -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); - crate::GenesisConfig:: { - keys: NEXT_VALIDATORS.with(|l| - l.borrow().iter().cloned().map(|i| (i, i, UintAuthorityId(i).into())).collect() - ), - }.assimilate_storage(&mut t).unwrap(); + let keys: Vec<_> = NEXT_VALIDATORS.with(|l| + l.borrow().iter().cloned().map(|i| (i, i, UintAuthorityId(i).into())).collect() + ); + BasicExternalities::execute_with_storage(&mut t, || { + for (ref k, ..) in &keys { + frame_system::Module::::inc_providers(k); + } + }); + crate::GenesisConfig:: { keys }.assimilate_storage(&mut t).unwrap(); sp_io::TestExternalities::new(t) } diff --git a/frame/session/src/historical/offchain.rs b/frame/session/src/historical/offchain.rs index 7a636c6e14c84..38cf09124ccf6 100644 --- a/frame/session/src/historical/offchain.rs +++ b/frame/session/src/historical/offchain.rs @@ -152,28 +152,27 @@ mod tests { }; use sp_runtime::testing::UintAuthorityId; + use frame_support::BasicExternalities; type Historical = Module; pub fn new_test_ext() -> sp_io::TestExternalities { - let mut ext = frame_system::GenesisConfig::default() + let mut t = frame_system::GenesisConfig::default() .build_storage::() .expect("Failed to create test externalities."); - crate::GenesisConfig:: { - keys: NEXT_VALIDATORS.with(|l| { - l.borrow() - .iter() - .cloned() - .map(|i| (i, i, UintAuthorityId(i).into())) - .collect() - }), - } - .assimilate_storage(&mut ext) - .unwrap(); + let keys: Vec<_> = NEXT_VALIDATORS.with(|l| + l.borrow().iter().cloned().map(|i| (i, i, UintAuthorityId(i).into())).collect() + ); + BasicExternalities::execute_with_storage(&mut t, || { + for (ref k, ..) in &keys { + frame_system::Module::::inc_providers(k); + } + }); + crate::GenesisConfig::{ keys }.assimilate_storage(&mut t).unwrap(); - let mut ext = sp_io::TestExternalities::new(ext); + let mut ext = sp_io::TestExternalities::new(t); let (offchain, offchain_state) = TestOffchainExt::with_offchain_db(ext.offchain_db()); diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 90eba3815a7a5..74105ade15f17 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -444,7 +444,7 @@ decl_storage! { for (account, val, keys) in config.keys.iter().cloned() { >::inner_set_keys(&val, keys) .expect("genesis config must not contain duplicates; qed"); - frame_system::Module::::inc_ref(&account); + assert!(frame_system::Module::::inc_consumers(&account).is_ok()); } let initial_validators_0 = T::SessionManager::new_session(0) @@ -498,6 +498,8 @@ decl_error! { DuplicatedKey, /// No keys are associated with this account. NoKeys, + /// Key setting account is not live, so it's impossible to associate keys. + NoAccount, } } @@ -746,9 +748,11 @@ impl Module { let who = T::ValidatorIdOf::convert(account.clone()) .ok_or(Error::::NoAssociatedValidatorId)?; + frame_system::Module::::inc_consumers(&account).map_err(|_| Error::::NoAccount)?; let old_keys = Self::inner_set_keys(&who, keys)?; - if old_keys.is_none() { - frame_system::Module::::inc_ref(&account); + if old_keys.is_some() { + let _ = frame_system::Module::::dec_consumers(&account); + // ^^^ Defensive only; Consumers were incremented just before, so should never fail. } Ok(()) @@ -796,7 +800,7 @@ impl Module { let key_data = old_keys.get_raw(*id); Self::clear_key_owner(*id, key_data); } - frame_system::Module::::dec_ref(&account); + frame_system::Module::::dec_consumers(&account); Ok(()) } diff --git a/frame/session/src/mock.rs b/frame/session/src/mock.rs index 3201500ee640a..2923530daf418 100644 --- a/frame/session/src/mock.rs +++ b/frame/session/src/mock.rs @@ -19,7 +19,7 @@ use super::*; use std::cell::RefCell; -use frame_support::{impl_outer_origin, parameter_types}; +use frame_support::{impl_outer_origin, parameter_types, BasicExternalities}; use sp_core::{crypto::key_types::DUMMY, H256}; use sp_runtime::{ Perbill, impl_opaque_keys, @@ -178,11 +178,18 @@ pub fn reset_before_session_end_called() { pub fn new_test_ext() -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); - GenesisConfig:: { - keys: NEXT_VALIDATORS.with(|l| - l.borrow().iter().cloned().map(|i| (i, i, UintAuthorityId(i).into())).collect() - ), - }.assimilate_storage(&mut t).unwrap(); + let keys: Vec<_> = NEXT_VALIDATORS.with(|l| + l.borrow().iter().cloned().map(|i| (i, i, UintAuthorityId(i).into())).collect() + ); + BasicExternalities::execute_with_storage(&mut t, || { + for (ref k, ..) in &keys { + frame_system::Module::::inc_providers(k); + } + frame_system::Module::::inc_providers(&4); + // An additional identity that we use. + frame_system::Module::::inc_providers(&69); + }); + GenesisConfig:: { keys }.assimilate_storage(&mut t).unwrap(); sp_io::TestExternalities::new(t) } diff --git a/frame/session/src/tests.rs b/frame/session/src/tests.rs index 4ef3bb9f98025..7c1d3c9dcdd24 100644 --- a/frame/session/src/tests.rs +++ b/frame/session/src/tests.rs @@ -60,9 +60,9 @@ fn keys_cleared_on_kill() { let id = DUMMY; assert_eq!(Session::key_owner(id, UintAuthorityId(1).get_raw(id)), Some(1)); - assert!(!System::allow_death(&1)); + assert!(System::is_provider_required(&1)); assert_ok!(Session::purge_keys(Origin::signed(1))); - assert!(System::allow_death(&1)); + assert!(!System::is_provider_required(&1)); assert_eq!(Session::load_keys(&1), None); assert_eq!(Session::key_owner(id, UintAuthorityId(1).get_raw(id)), None); diff --git a/frame/staking/fuzzer/src/mock.rs b/frame/staking/fuzzer/src/mock.rs index b3c9dd9f57b60..75e67fa36518a 100644 --- a/frame/staking/fuzzer/src/mock.rs +++ b/frame/staking/fuzzer/src/mock.rs @@ -63,7 +63,7 @@ impl frame_system::Config for Test { type PalletInfo = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnKilledAccount = (Balances,); + type OnKilledAccount = (); type SystemWeightInfo = (); type SS58Prefix = (); } diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 29e71f9539864..df2095e858809 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -397,7 +397,7 @@ benchmarks! { let s in 1 .. MAX_SPANS; let (stash, controller) = create_stash_controller::(0, 100, Default::default())?; add_slashing_spans::(&stash, s); - T::Currency::make_free_balance_be(&stash, 0u32.into()); + T::Currency::make_free_balance_be(&stash, T::Currency::minimum_balance()); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), stash.clone(), s) verify { diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 795f222158e05..99c1fe842a238 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1222,6 +1222,8 @@ decl_error! { IncorrectHistoryDepth, /// Incorrect number of slashing spans provided. IncorrectSlashingSpans, + /// Internal state has become somehow corrupted and the operation cannot continue. + BadState, } } @@ -1423,13 +1425,13 @@ decl_module! { Err(Error::::InsufficientValue)? } + system::Module::::inc_consumers(&stash).map_err(|_| Error::::BadState)?; + // You're auto-bonded forever, here. We might improve this by only bonding when // you actually validate/nominate and remove once you unbond __everything__. >::insert(&stash, &controller); >::insert(&stash, payee); - system::Module::::inc_ref(&stash); - let current_era = CurrentEra::get().unwrap_or(0); let history_depth = Self::history_depth(); let last_reward_era = current_era.saturating_sub(history_depth); @@ -2028,9 +2030,9 @@ decl_module! { } } - /// Remove all data structure concerning a staker/stash once its balance is zero. + /// Remove all data structure concerning a staker/stash once its balance is at the minimum. /// This is essentially equivalent to `withdraw_unbonded` except it can be called by anyone - /// and the target `stash` must have no funds left. + /// and the target `stash` must have no funds left beyond the ED. /// /// This can be called from any origin. /// @@ -2045,7 +2047,8 @@ decl_module! { /// # #[weight = T::WeightInfo::reap_stash(*num_slashing_spans)] fn reap_stash(_origin, stash: T::AccountId, num_slashing_spans: u32) { - ensure!(T::Currency::total_balance(&stash).is_zero(), Error::::FundedTarget); + let at_minimum = T::Currency::total_balance(&stash) == T::Currency::minimum_balance(); + ensure!(at_minimum, Error::::FundedTarget); Self::kill_stash(&stash, num_slashing_spans)?; T::Currency::remove_lock(STAKING_ID, &stash); } @@ -3007,7 +3010,7 @@ impl Module { >::remove(stash); >::remove(stash); - system::Module::::dec_ref(stash); + system::Module::::dec_consumers(stash); Ok(()) } diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 048806b062395..b8e756a494071 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -395,6 +395,8 @@ impl ExtBuilder { }; let num_validators = self.num_validators.unwrap_or(self.validator_count); + // Check that the number of validators is sensible. + assert!(num_validators <= 8); let validators = (0..num_validators) .map(|x| ((x + 1) * 10 + 1) as AccountId) .collect::>(); @@ -413,6 +415,14 @@ impl ExtBuilder { (31, balance_factor * 2000), (40, balance_factor), (41, balance_factor * 2000), + (50, balance_factor), + (51, balance_factor * 2000), + (60, balance_factor), + (61, balance_factor * 2000), + (70, balance_factor), + (71, balance_factor * 2000), + (80, balance_factor), + (81, balance_factor * 2000), (100, 2000 * balance_factor), (101, 2000 * balance_factor), // This allows us to have a total_payout different from 0. diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index bf0b2bf0da484..914aff9c45242 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -1540,7 +1540,7 @@ fn on_free_balance_zero_stash_removes_validator() { // Reduce free_balance of stash to 0 let _ = Balances::slash(&11, Balance::max_value()); // Check total balance of stash - assert_eq!(Balances::total_balance(&11), 0); + assert_eq!(Balances::total_balance(&11), 10); // Reap the stash assert_ok!(Staking::reap_stash(Origin::none(), 11, 0)); @@ -1596,7 +1596,7 @@ fn on_free_balance_zero_stash_removes_nominator() { // Reduce free_balance of stash to 0 let _ = Balances::slash(&11, Balance::max_value()); // Check total balance of stash - assert_eq!(Balances::total_balance(&11), 0); + assert_eq!(Balances::total_balance(&11), 10); // Reap the stash assert_ok!(Staking::reap_stash(Origin::none(), 11, 0)); @@ -2454,8 +2454,8 @@ fn garbage_collection_after_slashing() { // validator and nominator slash in era are garbage-collected by era change, // so we don't test those here. - assert_eq!(Balances::free_balance(11), 0); - assert_eq!(Balances::total_balance(&11), 0); + assert_eq!(Balances::free_balance(11), 2); + assert_eq!(Balances::total_balance(&11), 2); let slashing_spans = ::SlashingSpans::get(&11).unwrap(); assert_eq!(slashing_spans.iter().count(), 2); diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 0b2d3bceea5ec..1d3048cc9c0b6 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -27,7 +27,7 @@ use sp_runtime::{ traits::{ MaybeSerializeDeserialize, AtLeast32Bit, Saturating, TrailingZeroInput, Bounded, Zero, BadOrigin, AtLeast32BitUnsigned, UniqueSaturatedFrom, UniqueSaturatedInto, - SaturatedConversion, + SaturatedConversion, StoredMapError, }, }; use crate::dispatch::Parameter; @@ -300,42 +300,61 @@ mod test_impl_filter_stack { /// An abstraction of a value stored within storage, but possibly as part of a larger composite /// item. -pub trait StoredMap { +pub trait StoredMap { /// Get the item, or its default if it doesn't yet exist; we make no distinction between the /// two. fn get(k: &K) -> T; - /// Get whether the item takes up any storage. If this is `false`, then `get` will certainly - /// return the `T::default()`. If `true`, then there is no implication for `get` (i.e. it - /// may return any value, including the default). - /// - /// NOTE: This may still be `true`, even after `remove` is called. This is the case where - /// a single storage entry is shared between multiple `StoredMap` items single, without - /// additional logic to enforce it, deletion of any one them doesn't automatically imply - /// deletion of them all. - fn is_explicit(k: &K) -> bool; - /// Mutate the item. - fn mutate(k: &K, f: impl FnOnce(&mut T) -> R) -> R; - /// Mutate the item, removing or resetting to default value if it has been mutated to `None`. - fn mutate_exists(k: &K, f: impl FnOnce(&mut Option) -> R) -> R; + /// Maybe mutate the item only if an `Ok` value is returned from `f`. Do nothing if an `Err` is /// returned. It is removed or reset to default value if it has been mutated to `None` - fn try_mutate_exists(k: &K, f: impl FnOnce(&mut Option) -> Result) -> Result; + fn try_mutate_exists>( + k: &K, + f: impl FnOnce(&mut Option) -> Result, + ) -> Result; + + // Everything past here has a default implementation. + + /// Mutate the item. + fn mutate(k: &K, f: impl FnOnce(&mut T) -> R) -> Result { + Self::mutate_exists(k, |maybe_account| match maybe_account { + Some(ref mut account) => f(account), + x @ None => { + let mut account = Default::default(); + let r = f(&mut account); + *x = Some(account); + r + } + }) + } + + /// Mutate the item, removing or resetting to default value if it has been mutated to `None`. + /// + /// This is infallible as long as the value does not get destroyed. + fn mutate_exists( + k: &K, + f: impl FnOnce(&mut Option) -> R, + ) -> Result { + Self::try_mutate_exists(k, |x| -> Result { Ok(f(x)) }) + } + /// Set the item to something new. - fn insert(k: &K, t: T) { Self::mutate(k, |i| *i = t); } + fn insert(k: &K, t: T) -> Result<(), StoredMapError> { Self::mutate(k, |i| *i = t) } + /// Remove the item or otherwise replace it with its default value; we don't care which. - fn remove(k: &K); + fn remove(k: &K) -> Result<(), StoredMapError> { Self::mutate_exists(k, |x| *x = None) } } /// A simple, generic one-parameter event notifier/handler. -pub trait Happened { - /// The thing happened. - fn happened(t: &T); -} +pub trait HandleLifetime { + /// An account was created. + fn created(_t: &T) -> Result<(), StoredMapError> { Ok(()) } -impl Happened for () { - fn happened(_: &T) {} + /// An account was killed. + fn killed(_t: &T) -> Result<(), StoredMapError> { Ok(()) } } +impl HandleLifetime for () {} + /// A shim for placing around a storage item in order to use it as a `StoredValue`. Ideally this /// wouldn't be needed as `StorageValue`s should blanket implement `StoredValue`s, however this /// would break the ability to have custom impls of `StoredValue`. The other workaround is to @@ -347,68 +366,63 @@ impl Happened for () { /// be the default value), or where the account is being removed or reset back to the default value /// where previously it did exist (though may have been in a default state). This works well with /// system module's `CallOnCreatedAccount` and `CallKillAccount`. -pub struct StorageMapShim< - S, - Created, - Removed, - K, - T ->(sp_std::marker::PhantomData<(S, Created, Removed, K, T)>); +pub struct StorageMapShim(sp_std::marker::PhantomData<(S, L, K, T)>); impl< S: StorageMap, - Created: Happened, - Removed: Happened, + L: HandleLifetime, K: FullCodec, - T: FullCodec, -> StoredMap for StorageMapShim { + T: FullCodec + Default, +> StoredMap for StorageMapShim { fn get(k: &K) -> T { S::get(k) } - fn is_explicit(k: &K) -> bool { S::contains_key(k) } - fn insert(k: &K, t: T) { - let existed = S::contains_key(&k); - S::insert(k, t); - if !existed { - Created::happened(k); + fn insert(k: &K, t: T) -> Result<(), StoredMapError> { + if !S::contains_key(&k) { + L::created(k)?; } + S::insert(k, t); + Ok(()) } - fn remove(k: &K) { - let existed = S::contains_key(&k); - S::remove(k); - if existed { - Removed::happened(&k); + fn remove(k: &K) -> Result<(), StoredMapError> { + if S::contains_key(&k) { + L::killed(&k)?; + S::remove(k); } + Ok(()) } - fn mutate(k: &K, f: impl FnOnce(&mut T) -> R) -> R { - let existed = S::contains_key(&k); - let r = S::mutate(k, f); - if !existed { - Created::happened(k); + fn mutate(k: &K, f: impl FnOnce(&mut T) -> R) -> Result { + if !S::contains_key(&k) { + L::created(k)?; } - r + Ok(S::mutate(k, f)) } - fn mutate_exists(k: &K, f: impl FnOnce(&mut Option) -> R) -> R { - let (existed, exists, r) = S::mutate_exists(k, |maybe_value| { + fn mutate_exists(k: &K, f: impl FnOnce(&mut Option) -> R) -> Result { + S::try_mutate_exists(k, |maybe_value| { let existed = maybe_value.is_some(); let r = f(maybe_value); - (existed, maybe_value.is_some(), r) - }); - if !existed && exists { - Created::happened(k); - } else if existed && !exists { - Removed::happened(k); - } - r + let exists = maybe_value.is_some(); + + if !existed && exists { + L::created(k)?; + } else if existed && !exists { + L::killed(k)?; + } + Ok(r) + }) } - fn try_mutate_exists(k: &K, f: impl FnOnce(&mut Option) -> Result) -> Result { + fn try_mutate_exists>( + k: &K, + f: impl FnOnce(&mut Option) -> Result, + ) -> Result { S::try_mutate_exists(k, |maybe_value| { let existed = maybe_value.is_some(); - f(maybe_value).map(|v| (existed, maybe_value.is_some(), v)) - }).map(|(existed, exists, v)| { + let r = f(maybe_value)?; + let exists = maybe_value.is_some(); + if !existed && exists { - Created::happened(k); + L::created(k).map_err(E::from)?; } else if existed && !exists { - Removed::happened(k); + L::killed(k).map_err(E::from)?; } - v + Ok(r) }) } } @@ -507,18 +521,6 @@ pub trait ContainsLengthBound { fn max_len() -> usize; } -/// Determiner to say whether a given account is unused. -pub trait IsDeadAccount { - /// Is the given account dead? - fn is_dead_account(who: &AccountId) -> bool; -} - -impl IsDeadAccount for () { - fn is_dead_account(_who: &AccountId) -> bool { - true - } -} - /// Handler for when a new account has been created. #[impl_for_tuples(30)] pub trait OnNewAccount { diff --git a/frame/system/benchmarking/src/lib.rs b/frame/system/benchmarking/src/lib.rs index 57ae998862959..9ff749950ab5e 100644 --- a/frame/system/benchmarking/src/lib.rs +++ b/frame/system/benchmarking/src/lib.rs @@ -26,11 +26,11 @@ use sp_core::{ChangesTrieConfiguration, storage::well_known_keys}; use sp_runtime::traits::Hash; use frame_benchmarking::{benchmarks, whitelisted_caller}; use frame_support::{ - storage::{self, StorageMap}, + storage, traits::Get, weights::DispatchClass, }; -use frame_system::{Module as System, Call, RawOrigin, DigestItemOf, AccountInfo}; +use frame_system::{Module as System, Call, RawOrigin, DigestItemOf}; mod mock; @@ -136,22 +136,6 @@ benchmarks! { verify { assert_eq!(storage::unhashed::get_raw(&last_key), None); } - - suicide { - let caller: T::AccountId = whitelisted_caller(); - let account_info = AccountInfo:: { - nonce: 1337u32.into(), - refcount: 0, - data: T::AccountData::default() - }; - frame_system::Account::::insert(&caller, account_info); - let new_account_info = System::::account(caller.clone()); - assert_eq!(new_account_info.nonce, 1337u32.into()); - }: _(RawOrigin::Signed(caller.clone())) - verify { - let account_info = System::::account(&caller); - assert_eq!(account_info.nonce, 0u32.into()); - } } #[cfg(test)] @@ -170,7 +154,6 @@ mod tests { assert_ok!(test_benchmark_set_storage::()); assert_ok!(test_benchmark_kill_storage::()); assert_ok!(test_benchmark_kill_prefix::()); - assert_ok!(test_benchmark_suicide::()); }); } } diff --git a/frame/system/src/extensions/check_nonce.rs b/frame/system/src/extensions/check_nonce.rs index 0c610506d6616..c5d0e5242b484 100644 --- a/frame/system/src/extensions/check_nonce.rs +++ b/frame/system/src/extensions/check_nonce.rs @@ -129,7 +129,8 @@ mod tests { new_test_ext().execute_with(|| { crate::Account::::insert(1, crate::AccountInfo { nonce: 1, - refcount: 0, + consumers: 0, + providers: 0, data: 0, }); let info = DispatchInfo::default(); diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 463712ba68df5..0efa511e99b5e 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -97,7 +97,6 @@ use serde::Serialize; use sp_std::prelude::*; #[cfg(any(feature = "std", test))] use sp_std::map; -use sp_std::convert::Infallible; use sp_std::marker::PhantomData; use sp_std::fmt::Debug; use sp_version::RuntimeVersion; @@ -107,17 +106,16 @@ use sp_runtime::{ self, CheckEqual, AtLeast32Bit, Zero, Lookup, LookupError, SimpleBitOps, Hash, Member, MaybeDisplay, BadOrigin, MaybeSerialize, MaybeSerializeDeserialize, MaybeMallocSizeOf, StaticLookup, One, Bounded, - Dispatchable, AtLeast32BitUnsigned, Saturating, + Dispatchable, AtLeast32BitUnsigned, Saturating, StoredMapError, }, offchain::storage_lock::BlockNumberProvider, }; use sp_core::{ChangesTrieConfiguration, storage::well_known_keys}; use frame_support::{ - decl_module, decl_event, decl_storage, decl_error, Parameter, ensure, debug, - storage, + decl_module, decl_event, decl_storage, decl_error, Parameter, debug, storage, traits::{ - Contains, Get, PalletInfo, OnNewAccount, OnKilledAccount, IsDeadAccount, Happened, + Contains, Get, PalletInfo, OnNewAccount, OnKilledAccount, HandleLifetime, StoredMap, EnsureOrigin, OriginTrait, Filter, }, weights::{ @@ -352,7 +350,10 @@ pub struct AccountInfo { pub nonce: Index, /// The number of other modules that currently depend on this account's existence. The account /// cannot be reaped until this is zero. - pub refcount: RefCount, + pub consumers: RefCount, + /// The number of other modules that allow this account to exist. The account may not be reaped + /// until this is zero. + pub providers: RefCount, /// The additional data that belongs to this account. Used to store the balance(s) in a lot of /// chains. pub data: AccountData, @@ -445,6 +446,10 @@ decl_storage! { /// True if we have upgraded so that `type RefCount` is `u32`. False (default) if not. UpgradedToU32RefCount build(|_| true): bool; + /// True if we have upgraded so that AccountInfo contains two types of `RefCount`. False + /// (default) if not. + UpgradedToDualRefCount build(|_| true): bool; + /// The execution phase of the block. ExecutionPhase: Option; } @@ -505,6 +510,25 @@ decl_error! { } } +mod migrations { + use super::*; + + #[allow(dead_code)] + pub fn migrate_all() -> frame_support::weights::Weight { + Account::::translate::<(T::Index, u8, T::AccountData), _>(|_key, (nonce, rc, data)| + Some(AccountInfo { nonce, consumers: rc as RefCount, providers: 1, data }) + ); + T::BlockWeights::get().max_block + } + + pub fn migrate_to_dual_ref_count() -> frame_support::weights::Weight { + Account::::translate::<(T::Index, RefCount, T::AccountData), _>(|_key, (nonce, rc, data)| + Some(AccountInfo { nonce, consumers: rc as RefCount, providers: 1, data }) + ); + T::BlockWeights::get().max_block + } +} + /// Pallet struct placeholder on which is implemented the pallet logic. /// /// It is currently an alias for `Module` as old macros still generate/use old name. @@ -531,12 +555,9 @@ decl_module! { const SS58Prefix: u8 = T::SS58Prefix::get(); fn on_runtime_upgrade() -> frame_support::weights::Weight { - if !UpgradedToU32RefCount::get() { - Account::::translate::<(T::Index, u8, T::AccountData), _>(|_key, (nonce, rc, data)| - Some(AccountInfo { nonce, refcount: rc as RefCount, data }) - ); - UpgradedToU32RefCount::put(true); - T::BlockWeights::get().max_block + if !UpgradedToDualRefCount::get() { + UpgradedToDualRefCount::put(true); + migrations::migrate_to_dual_ref_count::() } else { 0 } @@ -700,25 +721,6 @@ decl_module! { ensure_root(origin)?; storage::unhashed::kill_prefix(&prefix); } - - /// Kill the sending account, assuming there are no references outstanding and the composite - /// data is equal to its default value. - /// - /// # - /// - `O(1)` - /// - 1 storage read and deletion. - /// -------------------- - /// Base Weight: 8.626 µs - /// No DB Read or Write operations because caller is already in overlay - /// # - #[weight = (T::SystemWeightInfo::suicide(), DispatchClass::Operational)] - pub fn suicide(origin) { - let who = ensure_signed(origin)?; - let account = Account::::get(&who); - ensure!(account.refcount == 0, Error::::NonZeroRefCount); - ensure!(account.data == T::AccountData::default(), Error::::NonDefaultComposite); - Self::kill_account(&who); - } } } @@ -894,40 +896,162 @@ impl Default for InitKind { } /// Reference status; can be either referenced or unreferenced. +#[derive(RuntimeDebug)] pub enum RefStatus { Referenced, Unreferenced, } -impl Module { - /// Deposits an event into this block's event record. - pub fn deposit_event(event: impl Into) { - Self::deposit_event_indexed(&[], event.into()); - } +/// Some resultant status relevant to incrementing a provider reference. +#[derive(RuntimeDebug)] +pub enum IncRefStatus { + /// Account was created. + Created, + /// Account already existed. + Existed, +} + +/// Some resultant status relevant to decrementing a provider reference. +#[derive(RuntimeDebug)] +pub enum DecRefStatus { + /// Account was destroyed. + Reaped, + /// Account still exists. + Exists, +} + +/// Some resultant status relevant to decrementing a provider reference. +#[derive(RuntimeDebug)] +pub enum DecRefError { + /// Account cannot have the last provider reference removed while there is a consumer. + ConsumerRemaining, +} +/// Some resultant status relevant to incrementing a provider reference. +#[derive(RuntimeDebug)] +pub enum IncRefError { + /// Account cannot introduce a consumer while there are no providers. + NoProviders, +} + +impl Module { pub fn account_exists(who: &T::AccountId) -> bool { Account::::contains_key(who) } /// Increment the reference counter on an account. + #[deprecated = "Use `inc_consumers` instead"] pub fn inc_ref(who: &T::AccountId) { - Account::::mutate(who, |a| a.refcount = a.refcount.saturating_add(1)); + let _ = Self::inc_consumers(who); } /// Decrement the reference counter on an account. This *MUST* only be done once for every time - /// you called `inc_ref` on `who`. + /// you called `inc_consumers` on `who`. + #[deprecated = "Use `dec_consumers` instead"] pub fn dec_ref(who: &T::AccountId) { - Account::::mutate(who, |a| a.refcount = a.refcount.saturating_sub(1)); + let _ = Self::dec_consumers(who); } /// The number of outstanding references for the account `who`. + #[deprecated = "Use `consumers` instead"] pub fn refs(who: &T::AccountId) -> RefCount { - Account::::get(who).refcount + Self::consumers(who) } /// True if the account has no outstanding references. + #[deprecated = "Use `!is_provider_required` instead"] pub fn allow_death(who: &T::AccountId) -> bool { - Account::::get(who).refcount == 0 + !Self::is_provider_required(who) + } + + /// Increment the reference counter on an account. + /// + /// The account `who`'s `providers` must be non-zero or this will return an error. + pub fn inc_providers(who: &T::AccountId) -> IncRefStatus { + Account::::mutate(who, |a| if a.providers == 0 { + // Account is being created. + a.providers = 1; + Self::on_created_account(who.clone(), a); + IncRefStatus::Created + } else { + a.providers = a.providers.saturating_add(1); + IncRefStatus::Existed + }) + } + + /// Decrement the reference counter on an account. This *MUST* only be done once for every time + /// you called `inc_consumers` on `who`. + pub fn dec_providers(who: &T::AccountId) -> Result { + Account::::try_mutate_exists(who, |maybe_account| { + if let Some(mut account) = maybe_account.take() { + match (account.providers, account.consumers) { + (0, _) => { + // Logic error - cannot decrement beyond zero and no item should + // exist with zero providers. + debug::print!("Logic error: Unexpected underflow in reducing provider"); + Ok(DecRefStatus::Reaped) + }, + (1, 0) => { + Module::::on_killed_account(who.clone()); + Ok(DecRefStatus::Reaped) + } + (1, _) => { + // Cannot remove last provider if there are consumers. + Err(DecRefError::ConsumerRemaining) + } + (x, _) => { + account.providers = x - 1; + *maybe_account = Some(account); + Ok(DecRefStatus::Exists) + } + } + } else { + debug::print!("Logic error: Account already dead when reducing provider"); + Ok(DecRefStatus::Reaped) + } + }) + } + + /// The number of outstanding references for the account `who`. + pub fn providers(who: &T::AccountId) -> RefCount { + Account::::get(who).providers + } + + /// Increment the reference counter on an account. + /// + /// The account `who`'s `providers` must be non-zero or this will return an error. + pub fn inc_consumers(who: &T::AccountId) -> Result<(), IncRefError> { + Account::::try_mutate(who, |a| if a.providers > 0 { + a.consumers = a.consumers.saturating_add(1); + Ok(()) + } else { + Err(IncRefError::NoProviders) + }) + } + + /// Decrement the reference counter on an account. This *MUST* only be done once for every time + /// you called `inc_consumers` on `who`. + pub fn dec_consumers(who: &T::AccountId) { + Account::::mutate(who, |a| if a.consumers > 0 { + a.consumers -= 1; + } else { + debug::print!("Logic error: Unexpected underflow in reducing consumer"); + }) + } + + /// The number of outstanding references for the account `who`. + pub fn consumers(who: &T::AccountId) -> RefCount { + Account::::get(who).consumers + } + + /// True if the account has some outstanding references. + pub fn is_provider_required(who: &T::AccountId) -> bool { + Account::::get(who).consumers != 0 + } + + /// Deposits an event into this block's event record. + pub fn deposit_event(event: impl Into) { + Self::deposit_event_indexed(&[], event.into()); } /// Deposits an event into this block's event record adding this event @@ -1196,7 +1320,7 @@ impl Module { } /// An account is being created. - pub fn on_created_account(who: T::AccountId) { + pub fn on_created_account(who: T::AccountId, _a: &mut AccountInfo) { T::OnNewAccount::on_new_account(&who); Self::deposit_event(RawEvent::NewAccount(who)); } @@ -1207,24 +1331,6 @@ impl Module { Self::deposit_event(RawEvent::KilledAccount(who)); } - /// Remove an account from storage. This should only be done when its refs are zero or you'll - /// get storage leaks in other modules. Nonetheless we assume that the calling logic knows best. - /// - /// This is a no-op if the account doesn't already exist. If it does then it will ensure - /// cleanups (those in `on_killed_account`) take place. - fn kill_account(who: &T::AccountId) { - if Account::::contains_key(who) { - let account = Account::::take(who); - if account.refcount > 0 { - debug::debug!( - target: "system", - "WARNING: Referenced account deleted. This is probably a bug." - ); - } - } - Module::::on_killed_account(who.clone()); - } - /// Determine whether or not it is possible to update the code. /// /// Checks the given code if it is a valid runtime wasm blob by instantianting @@ -1248,19 +1354,34 @@ impl Module { } } -/// Event handler which calls on_created_account when it happens. -pub struct CallOnCreatedAccount(PhantomData); -impl Happened for CallOnCreatedAccount { - fn happened(who: &T::AccountId) { - Module::::on_created_account(who.clone()); +/// Event handler which registers a provider when created. +pub struct Provider(PhantomData); +impl HandleLifetime for Provider { + fn created(t: &T::AccountId) -> Result<(), StoredMapError> { + Module::::inc_providers(t); + Ok(()) + } + fn killed(t: &T::AccountId) -> Result<(), StoredMapError> { + Module::::dec_providers(t) + .map(|_| ()) + .or_else(|e| match e { + DecRefError::ConsumerRemaining => Err(StoredMapError::ConsumerRemaining), + }) } } -/// Event handler which calls kill_account when it happens. -pub struct CallKillAccount(PhantomData); -impl Happened for CallKillAccount { - fn happened(who: &T::AccountId) { - Module::::kill_account(who) +/// Event handler which registers a consumer when created. +pub struct Consumer(PhantomData); +impl HandleLifetime for Consumer { + fn created(t: &T::AccountId) -> Result<(), StoredMapError> { + Module::::inc_consumers(t) + .map_err(|e| match e { + IncRefError::NoProviders => StoredMapError::NoProviders + }) + } + fn killed(t: &T::AccountId) -> Result<(), StoredMapError> { + Module::::dec_consumers(t); + Ok(()) } } @@ -1273,59 +1394,44 @@ impl BlockNumberProvider for Module } } -// Implement StoredMap for a simple single-item, kill-account-on-remove system. This works fine for -// storing a single item which is required to not be empty/default for the account to exist. -// Anything more complex will need more sophisticated logic. +fn is_providing(d: &T) -> bool { + d != &T::default() +} + +/// Implement StoredMap for a simple single-item, provide-when-not-default system. This works fine +/// for storing a single item which allows the account to continue existing as long as it's not +/// empty/default. +/// +/// Anything more complex will need more sophisticated logic. impl StoredMap for Module { fn get(k: &T::AccountId) -> T::AccountData { Account::::get(k).data } - fn is_explicit(k: &T::AccountId) -> bool { - Account::::contains_key(k) - } - fn insert(k: &T::AccountId, data: T::AccountData) { - let existed = Account::::contains_key(k); - Account::::mutate(k, |a| a.data = data); - if !existed { - Self::on_created_account(k.clone()); - } - } - fn remove(k: &T::AccountId) { - Self::kill_account(k) - } - fn mutate(k: &T::AccountId, f: impl FnOnce(&mut T::AccountData) -> R) -> R { - let existed = Account::::contains_key(k); - let r = Account::::mutate(k, |a| f(&mut a.data)); - if !existed { - Self::on_created_account(k.clone()); - } - r - } - fn mutate_exists(k: &T::AccountId, f: impl FnOnce(&mut Option) -> R) -> R { - Self::try_mutate_exists(k, |x| -> Result { Ok(f(x)) }).expect("Infallible; qed") - } - fn try_mutate_exists(k: &T::AccountId, f: impl FnOnce(&mut Option) -> Result) -> Result { - Account::::try_mutate_exists(k, |maybe_value| { - let existed = maybe_value.is_some(); - let (maybe_prefix, mut maybe_data) = split_inner( - maybe_value.take(), - |account| ((account.nonce, account.refcount), account.data) - ); - f(&mut maybe_data).map(|result| { - *maybe_value = maybe_data.map(|data| { - let (nonce, refcount) = maybe_prefix.unwrap_or_default(); - AccountInfo { nonce, refcount, data } - }); - (existed, maybe_value.is_some(), result) - }) - }).map(|(existed, exists, v)| { - if !existed && exists { - Self::on_created_account(k.clone()); - } else if existed && !exists { - Self::on_killed_account(k.clone()); + + fn try_mutate_exists>( + k: &T::AccountId, + f: impl FnOnce(&mut Option) -> Result, + ) -> Result { + let account = Account::::get(k); + let was_providing = is_providing(&account.data); + let mut some_data = if was_providing { Some(account.data) } else { None }; + let result = f(&mut some_data)?; + let is_providing = some_data.is_some(); + if !was_providing && is_providing { + Self::inc_providers(k); + } else if was_providing && !is_providing { + match Self::dec_providers(k) { + Err(DecRefError::ConsumerRemaining) => Err(StoredMapError::ConsumerRemaining)?, + Ok(DecRefStatus::Reaped) => return Ok(result), + Ok(DecRefStatus::Exists) => { + // Update value as normal... + } } - v - }) + } else if !was_providing && !is_providing { + return Ok(result) + } + Account::::mutate(k, |a| a.data = some_data.unwrap_or_default()); + Ok(result) } } @@ -1342,16 +1448,10 @@ pub fn split_inner(option: Option, splitter: impl FnOnce(T) -> (R, S } } -impl IsDeadAccount for Module { - fn is_dead_account(who: &T::AccountId) -> bool { - !Account::::contains_key(who) - } -} - -pub struct ChainContext(sp_std::marker::PhantomData); +pub struct ChainContext(PhantomData); impl Default for ChainContext { fn default() -> Self { - ChainContext(sp_std::marker::PhantomData) + ChainContext(PhantomData) } } diff --git a/frame/system/src/tests.rs b/frame/system/src/tests.rs index ca91630110366..89b84a2cc7ed5 100644 --- a/frame/system/src/tests.rs +++ b/frame/system/src/tests.rs @@ -31,20 +31,22 @@ fn origin_works() { #[test] fn stored_map_works() { new_test_ext().execute_with(|| { - System::insert(&0, 42); - assert!(System::allow_death(&0)); + assert!(System::insert(&0, 42).is_ok()); + assert!(!System::is_provider_required(&0)); - System::inc_ref(&0); - assert!(!System::allow_death(&0)); + assert_eq!(Account::::get(0), AccountInfo { nonce: 0, providers: 1, consumers: 0, data: 42 }); - System::insert(&0, 69); - assert!(!System::allow_death(&0)); + assert!(System::inc_consumers(&0).is_ok()); + assert!(System::is_provider_required(&0)); - System::dec_ref(&0); - assert!(System::allow_death(&0)); + assert!(System::insert(&0, 69).is_ok()); + assert!(System::is_provider_required(&0)); + + System::dec_consumers(&0); + assert!(!System::is_provider_required(&0)); assert!(KILLED.with(|r| r.borrow().is_empty())); - System::kill_account(&0); + assert!(System::remove(&0).is_ok()); assert_eq!(KILLED.with(|r| r.borrow().clone()), vec![0u64]); }); } @@ -398,11 +400,12 @@ fn events_not_emitted_during_genesis() { new_test_ext().execute_with(|| { // Block Number is zero at genesis assert!(System::block_number().is_zero()); - System::on_created_account(Default::default()); + let mut account_data = AccountInfo { nonce: 0, consumers: 0, providers: 0, data: 0 }; + System::on_created_account(Default::default(), &mut account_data); assert!(System::events().is_empty()); // Events will be emitted starting on block 1 System::set_block_number(1); - System::on_created_account(Default::default()); + System::on_created_account(Default::default(), &mut account_data); assert!(System::events().len() == 1); }); } diff --git a/frame/utility/src/tests.rs b/frame/utility/src/tests.rs index 9d03ead0eb122..556107529e1a2 100644 --- a/frame/utility/src/tests.rs +++ b/frame/utility/src/tests.rs @@ -144,7 +144,8 @@ pub struct TestBaseCallFilter; impl Filter for TestBaseCallFilter { fn filter(c: &Call) -> bool { match *c { - Call::Balances(_) => true, + // Transfer works. Use `transfer_keep_alive` for a call that doesn't pass the filter. + Call::Balances(pallet_balances::Call::transfer(..)) => true, Call::Utility(_) => true, // For benchmarking, this acts as a noop call Call::System(frame_system::Call::remark(..)) => true, @@ -275,7 +276,7 @@ fn as_derivative_filters() { assert_err_ignore_postinfo!(Utility::as_derivative( Origin::signed(1), 1, - Box::new(Call::System(frame_system::Call::suicide())), + Box::new(Call::Balances(pallet_balances::Call::transfer_keep_alive(2, 1))), ), DispatchError::BadOrigin); }); } @@ -320,7 +321,7 @@ fn batch_with_signed_filters() { new_test_ext().execute_with(|| { assert_ok!( Utility::batch(Origin::signed(1), vec![ - Call::System(frame_system::Call::suicide()) + Call::Balances(pallet_balances::Call::transfer_keep_alive(2, 1)) ]), ); expect_event(Event::BatchInterrupted(0, DispatchError::BadOrigin)); diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 563e0965d83aa..2fb4f7546d233 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -411,6 +411,10 @@ pub enum DispatchError { #[cfg_attr(feature = "std", serde(skip_deserializing))] message: Option<&'static str>, }, + /// At least one consumer is remaining so the account cannot be destroyed. + ConsumerRemaining, + /// There are no providers so the account cannot be created. + NoProviders, } /// Result of a `Dispatchable` which contains the `DispatchResult` and additional information about @@ -460,6 +464,15 @@ impl From for DispatchError { } } +impl From for DispatchError { + fn from(e: crate::traits::StoredMapError) -> Self { + match e { + crate::traits::StoredMapError::ConsumerRemaining => Self::ConsumerRemaining, + crate::traits::StoredMapError::NoProviders => Self::NoProviders, + } + } +} + impl From<&'static str> for DispatchError { fn from(err: &'static str) -> DispatchError { DispatchError::Other(err) @@ -470,9 +483,11 @@ impl From for &'static str { fn from(err: DispatchError) -> &'static str { match err { DispatchError::Other(msg) => msg, - DispatchError::CannotLookup => "Can not lookup", + DispatchError::CannotLookup => "Cannot lookup", DispatchError::BadOrigin => "Bad origin", DispatchError::Module { message, .. } => message.unwrap_or("Unknown module error"), + DispatchError::ConsumerRemaining => "Consumer remaining", + DispatchError::NoProviders => "No providers", } } } @@ -490,7 +505,7 @@ impl traits::Printable for DispatchError { "DispatchError".print(); match self { Self::Other(err) => err.print(), - Self::CannotLookup => "Can not lookup".print(), + Self::CannotLookup => "Cannot lookup".print(), Self::BadOrigin => "Bad origin".print(), Self::Module { index, error, message } => { index.print(); @@ -499,6 +514,8 @@ impl traits::Printable for DispatchError { msg.print(); } } + Self::ConsumerRemaining => "Consumer remaining".print(), + Self::NoProviders => "No providers".print(), } } } diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index b0567b7ae0d05..128c9a6eed0a0 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -153,6 +153,25 @@ impl From for &'static str { } } +/// Error that can be returned by our impl of `StoredMap`. +#[derive(Encode, Decode, RuntimeDebug)] +pub enum StoredMapError { + /// Attempt to create map value when it is a consumer and there are no providers in place. + NoProviders, + /// Attempt to anull/remove value when it is the last provider and there is still at + /// least one consumer left. + ConsumerRemaining, +} + +impl From for &'static str { + fn from(e: StoredMapError) -> &'static str { + match e { + StoredMapError::NoProviders => "No providers", + StoredMapError::ConsumerRemaining => "Consumer remaining", + } + } +} + /// An error that indicates that a lookup failed. #[derive(Encode, Decode, RuntimeDebug)] pub struct LookupError;