From 5afd5b8a4ee7bfb7d60d114b0968910f12748160 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 10 Mar 2022 12:28:00 +0100 Subject: [PATCH] contracts: Add test to verify unique trie ids (#10914) * Add test to verify unique trie ids * Rename trie_seed to nonce * Rename AccountCounter -> Nonce * fmt --- frame/contracts/src/exec.rs | 67 +++++++++++++-------------- frame/contracts/src/lib.rs | 27 +++++++++-- frame/contracts/src/migration.rs | 24 ++++++++++ frame/contracts/src/storage.rs | 7 ++- frame/contracts/src/tests.rs | 77 ++++++++++++++++++++++++++++---- frame/contracts/src/weights.rs | 16 +++---- 6 files changed, 162 insertions(+), 56 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index eb7a68d81ad50..455665687d973 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -18,7 +18,7 @@ use crate::{ gas::GasMeter, storage::{self, Storage, WriteOutcome}, - AccountCounter, BalanceOf, CodeHash, Config, ContractInfo, ContractInfoOf, Error, Event, + BalanceOf, CodeHash, Config, ContractInfo, ContractInfoOf, Error, Event, Nonce, Pallet as Contracts, Schedule, }; use frame_support::{ @@ -315,9 +315,10 @@ pub struct Stack<'a, T: Config, E> { timestamp: MomentOf, /// The block number at the time of call stack instantiation. block_number: T::BlockNumber, - /// The account counter is cached here when accessed. It is written back when the call stack - /// finishes executing. - account_counter: Option, + /// The nonce is cached here when accessed. It is written back when the call stack + /// finishes executing. Please refer to [`Nonce`] to a description of + /// the nonce itself. + nonce: Option, /// The actual call stack. One entry per nested contract called/instantiated. /// This does **not** include the [`Self::first_frame`]. frames: SmallVec, @@ -385,8 +386,8 @@ enum FrameArgs<'a, T: Config, E> { Instantiate { /// The contract or signed origin which instantiates the new contract. sender: T::AccountId, - /// The seed that should be used to derive a new trie id for the contract. - trie_seed: u64, + /// The nonce that should be used to derive a new trie id for the contract. + nonce: u64, /// The executable whose `deploy` function is run. executable: E, /// A salt used in the contract address deriviation of the new contract. @@ -571,7 +572,7 @@ where let (mut stack, executable) = Self::new( FrameArgs::Instantiate { sender: origin.clone(), - trie_seed: Self::initial_trie_seed(), + nonce: Self::initial_nonce(), executable, salt, }, @@ -596,7 +597,7 @@ where value: BalanceOf, debug_message: Option<&'a mut Vec>, ) -> Result<(Self, E), ExecError> { - let (first_frame, executable, account_counter) = + let (first_frame, executable, nonce) = Self::new_frame(args, value, gas_meter, storage_meter, 0, &schedule)?; let stack = Self { origin, @@ -605,7 +606,7 @@ where storage_meter, timestamp: T::Time::now(), block_number: >::block_number(), - account_counter, + nonce, first_frame, frames: Default::default(), debug_message, @@ -627,7 +628,7 @@ where gas_limit: Weight, schedule: &Schedule, ) -> Result<(Frame, E, Option), ExecError> { - let (account_id, contract_info, executable, delegate_caller, entry_point, account_counter) = + let (account_id, contract_info, executable, delegate_caller, entry_point, nonce) = match frame_args { FrameArgs::Call { dest, cached_info, delegated_call } => { let contract = if let Some(contract) = cached_info { @@ -645,10 +646,10 @@ where (dest, contract, executable, delegate_caller, ExportedFunction::Call, None) }, - FrameArgs::Instantiate { sender, trie_seed, executable, salt } => { + FrameArgs::Instantiate { sender, nonce, executable, salt } => { let account_id = >::contract_address(&sender, executable.code_hash(), &salt); - let trie_id = Storage::::generate_trie_id(&account_id, trie_seed); + let trie_id = Storage::::generate_trie_id(&account_id, nonce); let contract = Storage::::new_contract( &account_id, trie_id, @@ -660,7 +661,7 @@ where executable, None, ExportedFunction::Constructor, - Some(trie_seed), + Some(nonce), ) }, }; @@ -676,7 +677,7 @@ where allows_reentry: true, }; - Ok((frame, executable, account_counter)) + Ok((frame, executable, nonce)) } /// Create a subsequent nested frame. @@ -782,9 +783,9 @@ where /// This is called after running the current frame. It commits cached values to storage /// and invalidates all stale references to it that might exist further down the call stack. fn pop_frame(&mut self, persist: bool) { - // Revert the account counter in case of a failed instantiation. + // Revert changes to the nonce in case of a failed instantiation. if !persist && self.top_frame().entry_point == ExportedFunction::Constructor { - self.account_counter.as_mut().map(|c| *c = c.wrapping_sub(1)); + self.nonce.as_mut().map(|c| *c = c.wrapping_sub(1)); } // Pop the current frame from the stack and return it in case it needs to interact @@ -861,8 +862,8 @@ where if let Some(contract) = contract { >::insert(&self.first_frame.account_id, contract); } - if let Some(counter) = self.account_counter { - >::set(counter); + if let Some(nonce) = self.nonce { + >::set(nonce); } } } @@ -920,20 +921,20 @@ where !self.frames().any(|f| &f.account_id == id && !f.allows_reentry) } - /// Increments the cached account id and returns the value to be used for the trie_id. - fn next_trie_seed(&mut self) -> u64 { - let next = if let Some(current) = self.account_counter { + /// Increments and returns the next nonce. Pulls it from storage if it isn't in cache. + fn next_nonce(&mut self) -> u64 { + let next = if let Some(current) = self.nonce { current.wrapping_add(1) } else { - Self::initial_trie_seed() + Self::initial_nonce() }; - self.account_counter = Some(next); + self.nonce = Some(next); next } - /// The account seed to be used to instantiate the account counter cache. - fn initial_trie_seed() -> u64 { - >::get().wrapping_add(1) + /// Pull the current nonce from storage. + fn initial_nonce() -> u64 { + >::get().wrapping_add(1) } } @@ -1020,11 +1021,11 @@ where salt: &[u8], ) -> Result<(AccountIdOf, ExecReturnValue), ExecError> { let executable = E::from_storage(code_hash, &self.schedule, self.gas_meter())?; - let trie_seed = self.next_trie_seed(); + let nonce = self.next_nonce(); let executable = self.push_frame( FrameArgs::Instantiate { sender: self.top_frame().account_id.clone(), - trie_seed, + nonce, executable, salt, }, @@ -2445,7 +2446,7 @@ mod tests { } #[test] - fn account_counter() { + fn nonce() { let fail_code = MockLoader::insert(Constructor, |_, _| exec_trapped()); let success_code = MockLoader::insert(Constructor, |_, _| exec_success()); let succ_fail_code = MockLoader::insert(Constructor, move |ctx, _| { @@ -2495,7 +2496,7 @@ mod tests { None, ) .ok(); - assert_eq!(>::get(), 0); + assert_eq!(>::get(), 0); assert_ok!(MockStack::run_instantiate( ALICE, @@ -2508,7 +2509,7 @@ mod tests { &[], None, )); - assert_eq!(>::get(), 1); + assert_eq!(>::get(), 1); assert_ok!(MockStack::run_instantiate( ALICE, @@ -2521,7 +2522,7 @@ mod tests { &[], None, )); - assert_eq!(>::get(), 2); + assert_eq!(>::get(), 2); assert_ok!(MockStack::run_instantiate( ALICE, @@ -2534,7 +2535,7 @@ mod tests { &[], None, )); - assert_eq!(>::get(), 4); + assert_eq!(>::get(), 4); }); } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 037e3f1d33ae3..4edf43a672152 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -134,7 +134,7 @@ type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; /// The current storage version. -const STORAGE_VERSION: StorageVersion = StorageVersion::new(6); +const STORAGE_VERSION: StorageVersion = StorageVersion::new(7); /// Used as a sentinel value when reading and writing contract memory. /// @@ -665,9 +665,30 @@ pub mod pallet { #[pallet::storage] pub(crate) type OwnerInfoOf = StorageMap<_, Identity, CodeHash, OwnerInfo>; - /// The subtrie counter. + /// This is a **monotonic** counter incremented on contract instantiation. + /// + /// This is used in order to generate unique trie ids for contracts. + /// The trie id of a new contract is calculated from hash(account_id, nonce). + /// The nonce is required because otherwise the following sequence would lead to + /// a possible collision of storage: + /// + /// 1. Create a new contract. + /// 2. Terminate the contract. + /// 3. Immediately recreate the contract with the same account_id. + /// + /// This is bad because the contents of a trie are deleted lazily and there might be + /// storage of the old instantiation still in it when the new contract is created. Please + /// note that we can't replace the counter by the block number because the sequence above + /// can happen in the same block. We also can't keep the account counter in memory only + /// because storage is the only way to communicate across different extrinsics in the + /// same block. + /// + /// # Note + /// + /// Do not use it to determine the number of contracts. It won't be decremented if + /// a contract is destroyed. #[pallet::storage] - pub(crate) type AccountCounter = StorageValue<_, u64, ValueQuery>; + pub(crate) type Nonce = StorageValue<_, u64, ValueQuery>; /// The code associated with a given account. /// diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 59d3721ab13cf..035e3b4409cf9 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -19,6 +19,7 @@ use crate::{BalanceOf, CodeHash, Config, Pallet, TrieId, Weight}; use codec::{Decode, Encode}; use frame_support::{ codec, generate_storage_alias, + pallet_prelude::*, storage::migration, traits::{Get, PalletInfoAccess}, Identity, Twox64Concat, @@ -47,6 +48,11 @@ pub fn migrate() -> Weight { StorageVersion::new(6).put::>(); } + if version < 7 { + weight = weight.saturating_add(v7::migrate::()); + StorageVersion::new(7).put::>(); + } + weight } @@ -249,3 +255,21 @@ mod v6 { weight } } + +/// Rename `AccountCounter` to `Nonce`. +mod v7 { + use super::*; + + pub fn migrate() -> Weight { + generate_storage_alias!( + Contracts, + AccountCounter => Value + ); + generate_storage_alias!( + Contracts, + Nonce => Value + ); + Nonce::set(AccountCounter::take()); + T::DbWeight::get().reads_writes(1, 2) + } +} diff --git a/frame/contracts/src/storage.rs b/frame/contracts/src/storage.rs index 51a43a1782425..17022e9427664 100644 --- a/frame/contracts/src/storage.rs +++ b/frame/contracts/src/storage.rs @@ -290,10 +290,9 @@ where weight_limit.saturating_sub(weight_per_key.saturating_mul(remaining_key_budget as Weight)) } - /// This generator uses inner counter for account id and applies the hash over `AccountId + - /// accountid_counter`. - pub fn generate_trie_id(account_id: &AccountIdOf, seed: u64) -> TrieId { - let buf: Vec<_> = account_id.as_ref().iter().chain(&seed.to_le_bytes()).cloned().collect(); + /// Generates a unique trie id by returning `hash(account_id ++ nonce)`. + pub fn generate_trie_id(account_id: &AccountIdOf, nonce: u64) -> TrieId { + let buf: Vec<_> = account_id.as_ref().iter().chain(&nonce.to_le_bytes()).cloned().collect(); T::Hashing::hash(&buf).as_ref().into() } diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 8add424db0892..4c8c42c77da56 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -74,17 +74,15 @@ frame_support::construct_runtime!( #[macro_use] pub mod test_utils { use super::{Balances, Test}; - use crate::{ - exec::AccountIdOf, storage::Storage, AccountCounter, CodeHash, Config, ContractInfoOf, - }; + use crate::{exec::AccountIdOf, storage::Storage, CodeHash, Config, ContractInfoOf, Nonce}; use frame_support::traits::Currency; pub fn place_contract(address: &AccountIdOf, code_hash: CodeHash) { - let seed = >::mutate(|counter| { + let nonce = >::mutate(|counter| { *counter += 1; *counter }); - let trie_id = Storage::::generate_trie_id(address, seed); + let trie_id = Storage::::generate_trie_id(address, nonce); set_balance(address, ::Currency::minimum_balance() * 10); let contract = Storage::::new_contract(&address, trie_id, code_hash).unwrap(); >::insert(address, contract); @@ -349,6 +347,11 @@ where Ok((wasm_binary, code_hash)) } +fn initialize_block(number: u64) { + System::reset_events(); + System::initialize(&number, &[0u8; 32].into(), &Default::default()); +} + // Perform a call to a plain account. // The actual transfer fails because we can only call contracts. // Then we check that at least the base costs where charged (no runtime gas costs.) @@ -540,9 +543,67 @@ fn run_out_of_gas() { }); } -fn initialize_block(number: u64) { - System::reset_events(); - System::initialize(&number, &[0u8; 32].into(), &Default::default()); +/// Check that contracts with the same account id have different trie ids. +/// Check the `Nonce` storage item for more information. +#[test] +fn instantiate_unique_trie_id() { + let (wasm, code_hash) = compile_module::("self_destruct").unwrap(); + + ExtBuilder::default().existential_deposit(500).build().execute_with(|| { + let _ = Balances::deposit_creating(&ALICE, 1_000_000); + Contracts::upload_code(Origin::signed(ALICE), wasm, None).unwrap(); + let addr = Contracts::contract_address(&ALICE, &code_hash, &[]); + + // Instantiate the contract and store its trie id for later comparison. + assert_ok!(Contracts::instantiate( + Origin::signed(ALICE), + 0, + GAS_LIMIT, + None, + code_hash, + vec![], + vec![], + )); + let trie_id = ContractInfoOf::::get(&addr).unwrap().trie_id; + + // Try to instantiate it again without termination should yield an error. + assert_err_ignore_postinfo!( + Contracts::instantiate( + Origin::signed(ALICE), + 0, + GAS_LIMIT, + None, + code_hash, + vec![], + vec![], + ), + >::DuplicateContract, + ); + + // Terminate the contract. + assert_ok!(Contracts::call( + Origin::signed(ALICE), + addr.clone(), + 0, + GAS_LIMIT, + None, + vec![] + )); + + // Re-Instantiate after termination. + assert_ok!(Contracts::instantiate( + Origin::signed(ALICE), + 0, + GAS_LIMIT, + None, + code_hash, + vec![], + vec![], + )); + + // Trie ids shouldn't match or we might have a collision + assert_ne!(trie_id, ContractInfoOf::::get(&addr).unwrap().trie_id); + }); } #[test] diff --git a/frame/contracts/src/weights.rs b/frame/contracts/src/weights.rs index 7d73a3a1cecc7..b438ad51cbfca 100644 --- a/frame/contracts/src/weights.rs +++ b/frame/contracts/src/weights.rs @@ -201,7 +201,7 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().writes(2 as Weight)) } // Storage: Contracts CodeStorage (r:1 w:1) - // Storage: Contracts AccountCounter (r:1 w:1) + // Storage: Contracts Nonce (r:1 w:1) // Storage: Contracts ContractInfoOf (r:1 w:1) // Storage: Timestamp Now (r:1 w:0) // Storage: System Account (r:1 w:1) @@ -217,7 +217,7 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().writes(6 as Weight)) } // Storage: Contracts CodeStorage (r:1 w:1) - // Storage: Contracts AccountCounter (r:1 w:1) + // Storage: Contracts Nonce (r:1 w:1) // Storage: Contracts ContractInfoOf (r:1 w:1) // Storage: Timestamp Now (r:1 w:0) // Storage: System Account (r:1 w:1) @@ -651,7 +651,7 @@ impl WeightInfo for SubstrateWeight { // Storage: Contracts ContractInfoOf (r:1 w:1) // Storage: Contracts CodeStorage (r:1 w:0) // Storage: Timestamp Now (r:1 w:0) - // Storage: Contracts AccountCounter (r:1 w:1) + // Storage: Contracts Nonce (r:1 w:1) // Storage: Contracts OwnerInfoOf (r:100 w:100) fn seal_instantiate(r: u32, ) -> Weight { (0 as Weight) @@ -666,7 +666,7 @@ impl WeightInfo for SubstrateWeight { // Storage: Contracts ContractInfoOf (r:101 w:101) // Storage: Contracts CodeStorage (r:2 w:1) // Storage: Timestamp Now (r:1 w:0) - // Storage: Contracts AccountCounter (r:1 w:1) + // Storage: Contracts Nonce (r:1 w:1) // Storage: Contracts OwnerInfoOf (r:1 w:1) fn seal_instantiate_per_transfer_salt_kb(t: u32, s: u32, ) -> Weight { (14_790_752_000 as Weight) @@ -1092,7 +1092,7 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().writes(2 as Weight)) } // Storage: Contracts CodeStorage (r:1 w:1) - // Storage: Contracts AccountCounter (r:1 w:1) + // Storage: Contracts Nonce (r:1 w:1) // Storage: Contracts ContractInfoOf (r:1 w:1) // Storage: Timestamp Now (r:1 w:0) // Storage: System Account (r:1 w:1) @@ -1108,7 +1108,7 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().writes(6 as Weight)) } // Storage: Contracts CodeStorage (r:1 w:1) - // Storage: Contracts AccountCounter (r:1 w:1) + // Storage: Contracts Nonce (r:1 w:1) // Storage: Contracts ContractInfoOf (r:1 w:1) // Storage: Timestamp Now (r:1 w:0) // Storage: System Account (r:1 w:1) @@ -1542,7 +1542,7 @@ impl WeightInfo for () { // Storage: Contracts ContractInfoOf (r:1 w:1) // Storage: Contracts CodeStorage (r:1 w:0) // Storage: Timestamp Now (r:1 w:0) - // Storage: Contracts AccountCounter (r:1 w:1) + // Storage: Contracts Nonce (r:1 w:1) // Storage: Contracts OwnerInfoOf (r:100 w:100) fn seal_instantiate(r: u32, ) -> Weight { (0 as Weight) @@ -1557,7 +1557,7 @@ impl WeightInfo for () { // Storage: Contracts ContractInfoOf (r:101 w:101) // Storage: Contracts CodeStorage (r:2 w:1) // Storage: Timestamp Now (r:1 w:0) - // Storage: Contracts AccountCounter (r:1 w:1) + // Storage: Contracts Nonce (r:1 w:1) // Storage: Contracts OwnerInfoOf (r:1 w:1) fn seal_instantiate_per_transfer_salt_kb(t: u32, s: u32, ) -> Weight { (14_790_752_000 as Weight)