diff --git a/bin/node/executor/tests/basic.rs b/bin/node/executor/tests/basic.rs index c18f81bdc07d5..279b6a776031a 100644 --- a/bin/node/executor/tests/basic.rs +++ b/bin/node/executor/tests/basic.rs @@ -26,7 +26,7 @@ use sp_runtime::{ traits::Hash as HashT, transaction_validity::InvalidTransaction, }; -use frame_system::{self, EventRecord, Phase}; +use frame_system::{self, EventRecord, Phase, AccountInfo}; use node_runtime::{ Header, Block, UncheckedExtrinsic, CheckedExtrinsic, Call, Runtime, Balances, @@ -227,11 +227,17 @@ 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, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + AccountInfo::<::Index, _> { + data: (111 * DOLLARS, 0u128, 0u128, 0u128), + .. Default::default() + }.encode(), ); t.insert( >::hashed_key_for(bob()), - (0u32, 0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() + AccountInfo::<::Index, _> { + data: (0 * DOLLARS, 0u128, 0u128, 0u128), + .. Default::default() + }.encode(), ); t.insert( >::hashed_key().to_vec(), @@ -270,11 +276,17 @@ 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, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + AccountInfo::<::Index, _> { + data: (111 * DOLLARS, 0u128, 0u128, 0u128), + .. Default::default() + }.encode(), ); t.insert( >::hashed_key_for(bob()), - (0u32, 0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() + AccountInfo::<::Index, _> { + data: (0 * DOLLARS, 0u128, 0u128, 0u128), + .. Default::default() + }.encode(), ); t.insert( >::hashed_key().to_vec(), @@ -704,7 +716,10 @@ fn panic_execution_gives_error() { let mut t = new_test_ext(bloaty_code_unwrap(), false); t.insert( >::hashed_key_for(alice()), - (0u32, 0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() + AccountInfo::<::Index, _> { + data: (0 * DOLLARS, 0u128, 0u128, 0u128), + .. Default::default() + }.encode(), ); t.insert(>::hashed_key().to_vec(), 0_u128.encode()); t.insert(>::hashed_key_for(0), vec![0u8; 32]); @@ -733,11 +748,17 @@ fn successful_execution_gives_ok() { let mut t = new_test_ext(compact_code_unwrap(), false); t.insert( >::hashed_key_for(alice()), - (0u32, 0u32, 0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + AccountInfo::<::Index, _> { + data: (111 * DOLLARS, 0u128, 0u128, 0u128), + .. Default::default() + }.encode(), ); t.insert( >::hashed_key_for(bob()), - (0u32, 0u32, 0u32, 0 * DOLLARS, 0u128, 0u128, 0u128).encode() + AccountInfo::<::Index, _> { + data: (0 * DOLLARS, 0u128, 0u128, 0u128), + .. Default::default() + }.encode(), ); t.insert( >::hashed_key().to_vec(), diff --git a/bin/node/executor/tests/fees.rs b/bin/node/executor/tests/fees.rs index 90b28539f7bcd..ad24db03f9832 100644 --- a/bin/node/executor/tests/fees.rs +++ b/bin/node/executor/tests/fees.rs @@ -129,6 +129,7 @@ fn new_account_info(free_dollars: u128) -> Vec { nonce: 0u32, consumers: 0, providers: 0, + sufficients: 0, data: (free_dollars * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS), }.encode() } diff --git a/bin/node/executor/tests/submit_transaction.rs b/bin/node/executor/tests/submit_transaction.rs index ff483d9ecd8cd..3de0758d81462 100644 --- a/bin/node/executor/tests/submit_transaction.rs +++ b/bin/node/executor/tests/submit_transaction.rs @@ -252,7 +252,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, consumers: 0, providers: 0, data }; + let account = frame_system::AccountInfo { data, .. Default::default() }; >::insert(&address, account); // check validity diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 53353c224a8b7..7170f85501aaf 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -775,7 +775,7 @@ mod tests { header: Header { parent_hash: [69u8; 32].into(), number: 1, - state_root: hex!("1599922f15b2d5cf75e83370e29e13b96fdf799d917a5b6319736af292f21665").into(), + state_root: hex!("2c01e6f33d595793119823478b45b36978a8f65a731b5ae3fdfb6330b4cd4b11").into(), extrinsics_root: hex!("03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314").into(), digest: Digest { logs: vec![], }, }, diff --git a/frame/system/src/extensions/check_nonce.rs b/frame/system/src/extensions/check_nonce.rs index bc48be925bc0d..3cb74a7ed918d 100644 --- a/frame/system/src/extensions/check_nonce.rs +++ b/frame/system/src/extensions/check_nonce.rs @@ -128,6 +128,7 @@ mod tests { nonce: 1, consumers: 0, providers: 0, + sufficients: 0, data: 0, }); let info = DispatchInfo::default(); diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index a99184650cf55..ce9ab0dddc105 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -262,9 +262,9 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { fn on_runtime_upgrade() -> frame_support::weights::Weight { - if !UpgradedToDualRefCount::::get() { - UpgradedToDualRefCount::::put(true); - migrations::migrate_to_dual_ref_count::() + if !UpgradedToTripleRefCount::::get() { + UpgradedToTripleRefCount::::put(true); + migrations::migrate_to_triple_ref_count::() } else { 0 } @@ -594,10 +594,10 @@ pub mod pallet { #[pallet::storage] pub(super) type UpgradedToU32RefCount = StorageValue<_, bool, ValueQuery>; - /// True if we have upgraded so that AccountInfo contains two types of `RefCount`. False + /// True if we have upgraded so that AccountInfo contains three types of `RefCount`. False /// (default) if not. #[pallet::storage] - pub(super) type UpgradedToDualRefCount = StorageValue<_, bool, ValueQuery>; + pub(super) type UpgradedToTripleRefCount = StorageValue<_, bool, ValueQuery>; /// The execution phase of the block. #[pallet::storage] @@ -627,7 +627,7 @@ pub mod pallet { >::put::(hash69()); >::put(LastRuntimeUpgradeInfo::from(T::Version::get())); >::put(true); - >::put(true); + >::put(true); sp_io::storage::set(well_known_keys::CODE, &self.code); sp_io::storage::set(well_known_keys::EXTRINSIC_INDEX, &0u32.encode()); @@ -642,16 +642,29 @@ mod migrations { use super::*; #[allow(dead_code)] + /// Migrate from unique `u8` reference counting to triple `u32` reference counting. 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 }) + Some(AccountInfo { nonce, consumers: rc as RefCount, providers: 1, sufficients: 0, data }) ); T::BlockWeights::get().max_block } + #[allow(dead_code)] + /// Migrate from unique `u32` reference counting to triple `u32` reference counting. 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 }) + Account::::translate::<(T::Index, RefCount, T::AccountData), _>(|_key, (nonce, consumers, data)| + Some(AccountInfo { nonce, consumers, providers: 1, sufficients: 0, data }) + ); + T::BlockWeights::get().max_block + } + + /// Migrate from dual `u32` reference counting to triple `u32` reference counting. + pub fn migrate_to_triple_ref_count() -> frame_support::weights::Weight { + Account::::translate::<(T::Index, RefCount, RefCount, T::AccountData), _>( + |_key, (nonce, consumers, providers, data)| { + Some(AccountInfo { nonce, consumers, providers, sufficients: 0, data }) + } ); T::BlockWeights::get().max_block } @@ -762,8 +775,11 @@ pub struct AccountInfo { /// cannot be reaped until this is zero. pub consumers: RefCount, /// The number of other modules that allow this account to exist. The account may not be reaped - /// until this is zero. + /// until this and `sufficients` are both zero. pub providers: RefCount, + /// The number of modules that allow this account to exist for their own purposes only. The + /// account may not be reaped until this and `providers` are both zero. + pub sufficients: RefCount, /// The additional data that belongs to this account. Used to store the balance(s) in a lot of /// chains. pub data: AccountData, @@ -974,8 +990,8 @@ pub enum RefStatus { Unreferenced, } -/// Some resultant status relevant to incrementing a provider reference. -#[derive(RuntimeDebug)] +/// Some resultant status relevant to incrementing a provider/self-sufficient reference. +#[derive(Eq, PartialEq, RuntimeDebug)] pub enum IncRefStatus { /// Account was created. Created, @@ -983,8 +999,8 @@ pub enum IncRefStatus { Existed, } -/// Some resultant status relevant to decrementing a provider reference. -#[derive(RuntimeDebug)] +/// Some resultant status relevant to decrementing a provider/self-sufficient reference. +#[derive(Eq, PartialEq, RuntimeDebug)] pub enum DecRefStatus { /// Account was destroyed. Reaped, @@ -993,14 +1009,14 @@ pub enum DecRefStatus { } /// Some resultant status relevant to decrementing a provider reference. -#[derive(RuntimeDebug)] +#[derive(Eq, PartialEq, 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)] +/// Some resultant status relevant to incrementing a consumer reference. +#[derive(Eq, PartialEq, RuntimeDebug)] pub enum IncRefError { /// Account cannot introduce a consumer while there are no providers. NoProviders, @@ -1036,11 +1052,9 @@ impl Module { !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. + /// Increment the provider reference counter on an account. pub fn inc_providers(who: &T::AccountId) -> IncRefStatus { - Account::::mutate(who, |a| if a.providers == 0 { + Account::::mutate(who, |a| if a.providers == 0 && a.sufficients == 0 { // Account is being created. a.providers = 1; Self::on_created_account(who.clone(), a); @@ -1051,30 +1065,34 @@ impl Module { }) } - /// Decrement the reference counter on an account. This *MUST* only be done once for every time - /// you called `inc_consumers` on `who`. + /// Decrement the provider reference counter on an account. + /// + /// This *MUST* only be done once for every time you called `inc_providers` 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. - log::error!( - target: "runtime::system", - "Logic error: Unexpected underflow in reducing provider", - ); - Ok(DecRefStatus::Reaped) - }, - (1, 0) => { + if account.providers == 0 { + // Logic error - cannot decrement beyond zero. + log::error!( + target: "runtime::system", + "Logic error: Unexpected underflow in reducing provider", + ); + account.providers = 1; + } + match (account.providers, account.consumers, account.sufficients) { + (1, 0, 0) => { + // No providers left (and no consumers) and no sufficients. Account dead. + Module::::on_killed_account(who.clone()); Ok(DecRefStatus::Reaped) } - (1, _) => { + (1, c, _) if c > 0 => { // Cannot remove last provider if there are consumers. Err(DecRefError::ConsumerRemaining) } - (x, _) => { + (x, _, _) => { + // Account will continue to exist as there is either > 1 provider or + // > 0 sufficients. account.providers = x - 1; *maybe_account = Some(account); Ok(DecRefStatus::Exists) @@ -1090,11 +1108,69 @@ impl Module { }) } - /// The number of outstanding references for the account `who`. + /// Increment the self-sufficient reference counter on an account. + pub fn inc_sufficients(who: &T::AccountId) -> IncRefStatus { + Account::::mutate(who, |a| if a.providers + a.sufficients == 0 { + // Account is being created. + a.sufficients = 1; + Self::on_created_account(who.clone(), a); + IncRefStatus::Created + } else { + a.sufficients = a.sufficients.saturating_add(1); + IncRefStatus::Existed + }) + } + + /// Decrement the sufficients reference counter on an account. + /// + /// This *MUST* only be done once for every time you called `inc_sufficients` on `who`. + pub fn dec_sufficients(who: &T::AccountId) -> DecRefStatus { + Account::::mutate_exists(who, |maybe_account| { + if let Some(mut account) = maybe_account.take() { + if account.sufficients == 0 { + // Logic error - cannot decrement beyond zero. + log::error!( + target: "runtime::system", + "Logic error: Unexpected underflow in reducing sufficients", + ); + } + match (account.sufficients, account.providers) { + (0, 0) | (1, 0) => { + Module::::on_killed_account(who.clone()); + DecRefStatus::Reaped + } + (x, _) => { + account.sufficients = x - 1; + *maybe_account = Some(account); + DecRefStatus::Exists + } + } + } else { + log::error!( + target: "runtime::system", + "Logic error: Account already dead when reducing provider", + ); + DecRefStatus::Reaped + } + }) + } + + /// The number of outstanding provider references for the account `who`. pub fn providers(who: &T::AccountId) -> RefCount { Account::::get(who).providers } + /// The number of outstanding sufficient references for the account `who`. + pub fn sufficients(who: &T::AccountId) -> RefCount { + Account::::get(who).sufficients + } + + /// The number of outstanding provider and sufficient references for the account `who`. + pub fn reference_count(who: &T::AccountId) -> RefCount { + let a = Account::::get(who); + a.providers + a.sufficients + } + /// Increment the reference counter on an account. /// /// The account `who`'s `providers` must be non-zero or this will return an error. @@ -1451,6 +1527,19 @@ impl HandleLifetime for Provider { } } +/// Event handler which registers a self-sufficient when created. +pub struct SelfSufficient(PhantomData); +impl HandleLifetime for SelfSufficient { + fn created(t: &T::AccountId) -> Result<(), StoredMapError> { + Module::::inc_sufficients(t); + Ok(()) + } + fn killed(t: &T::AccountId) -> Result<(), StoredMapError> { + Module::::dec_sufficients(t); + Ok(()) + } +} + /// Event handler which registers a consumer when created. pub struct Consumer(PhantomData); impl HandleLifetime for Consumer { diff --git a/frame/system/src/tests.rs b/frame/system/src/tests.rs index ca17edcf4b22a..9f500e5a3b050 100644 --- a/frame/system/src/tests.rs +++ b/frame/system/src/tests.rs @@ -19,10 +19,7 @@ use crate::*; use mock::{*, Origin}; use sp_core::H256; use sp_runtime::{DispatchError, DispatchErrorWithPostInfo, traits::{Header, BlakeTwo256}}; -use frame_support::{ - weights::WithPostDispatchInfo, - dispatch::PostDispatchInfo, -}; +use frame_support::{assert_noop, weights::WithPostDispatchInfo, dispatch::PostDispatchInfo}; #[test] fn origin_works() { @@ -37,7 +34,13 @@ fn stored_map_works() { assert!(System::insert(&0, 42).is_ok()); assert!(!System::is_provider_required(&0)); - assert_eq!(Account::::get(0), AccountInfo { nonce: 0, providers: 1, consumers: 0, data: 42 }); + assert_eq!(Account::::get(0), AccountInfo { + nonce: 0, + providers: 1, + consumers: 0, + sufficients: 0, + data: 42, + }); assert!(System::inc_consumers(&0).is_ok()); assert!(System::is_provider_required(&0)); @@ -54,6 +57,98 @@ fn stored_map_works() { }); } +#[test] +fn provider_ref_handover_to_self_sufficient_ref_works() { + new_test_ext().execute_with(|| { + assert_eq!(System::inc_providers(&0), IncRefStatus::Created); + System::inc_account_nonce(&0); + assert_eq!(System::account_nonce(&0), 1); + + // a second reference coming and going doesn't change anything. + assert_eq!(System::inc_sufficients(&0), IncRefStatus::Existed); + assert_eq!(System::dec_sufficients(&0), DecRefStatus::Exists); + assert_eq!(System::account_nonce(&0), 1); + + // a provider reference coming and going doesn't change anything. + assert_eq!(System::inc_providers(&0), IncRefStatus::Existed); + assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists); + assert_eq!(System::account_nonce(&0), 1); + + // decreasing the providers with a self-sufficient present should not delete the account + assert_eq!(System::inc_sufficients(&0), IncRefStatus::Existed); + assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists); + assert_eq!(System::account_nonce(&0), 1); + + // decreasing the sufficients should delete the account + assert_eq!(System::dec_sufficients(&0), DecRefStatus::Reaped); + assert_eq!(System::account_nonce(&0), 0); + }); +} + +#[test] +fn self_sufficient_ref_handover_to_provider_ref_works() { + new_test_ext().execute_with(|| { + assert_eq!(System::inc_sufficients(&0), IncRefStatus::Created); + System::inc_account_nonce(&0); + assert_eq!(System::account_nonce(&0), 1); + + // a second reference coming and going doesn't change anything. + assert_eq!(System::inc_providers(&0), IncRefStatus::Existed); + assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists); + assert_eq!(System::account_nonce(&0), 1); + + // a sufficient reference coming and going doesn't change anything. + assert_eq!(System::inc_sufficients(&0), IncRefStatus::Existed); + assert_eq!(System::dec_sufficients(&0), DecRefStatus::Exists); + assert_eq!(System::account_nonce(&0), 1); + + // decreasing the sufficients with a provider present should not delete the account + assert_eq!(System::inc_providers(&0), IncRefStatus::Existed); + assert_eq!(System::dec_sufficients(&0), DecRefStatus::Exists); + assert_eq!(System::account_nonce(&0), 1); + + // decreasing the providers should delete the account + assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Reaped); + assert_eq!(System::account_nonce(&0), 0); + }); +} + +#[test] +fn sufficient_cannot_support_consumer() { + new_test_ext().execute_with(|| { + assert_eq!(System::inc_sufficients(&0), IncRefStatus::Created); + System::inc_account_nonce(&0); + assert_eq!(System::account_nonce(&0), 1); + assert_noop!(System::inc_consumers(&0), IncRefError::NoProviders); + + assert_eq!(System::inc_providers(&0), IncRefStatus::Existed); + assert!(System::inc_consumers(&0).is_ok()); + assert_noop!(System::dec_providers(&0), DecRefError::ConsumerRemaining); + }); +} + +#[test] +fn provider_required_to_support_consumer() { + new_test_ext().execute_with(|| { + assert_noop!(System::inc_consumers(&0), IncRefError::NoProviders); + + assert_eq!(System::inc_providers(&0), IncRefStatus::Created); + System::inc_account_nonce(&0); + assert_eq!(System::account_nonce(&0), 1); + + assert_eq!(System::inc_providers(&0), IncRefStatus::Existed); + assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists); + assert_eq!(System::account_nonce(&0), 1); + + assert!(System::inc_consumers(&0).is_ok()); + assert_noop!(System::dec_providers(&0), DecRefError::ConsumerRemaining); + + System::dec_consumers(&0); + assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Reaped); + assert_eq!(System::account_nonce(&0), 0); + }); +} + #[test] fn deposit_event_should_work() { new_test_ext().execute_with(|| { @@ -403,7 +498,7 @@ fn events_not_emitted_during_genesis() { new_test_ext().execute_with(|| { // Block Number is zero at genesis assert!(System::block_number().is_zero()); - let mut account_data = AccountInfo { nonce: 0, consumers: 0, providers: 0, data: 0 }; + let mut account_data = AccountInfo::default(); System::on_created_account(Default::default(), &mut account_data); assert!(System::events().is_empty()); // Events will be emitted starting on block 1