From 3d2ba05f2cff52a664644ffebca526c84e08ee5f Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 11:33:29 +0100 Subject: [PATCH 1/9] contracts: during execution -> contract trapped during execution This message confused many people so we are improving it to make clear what happened. --- frame/contracts/src/tests.rs | 6 +++--- frame/contracts/src/wasm/mod.rs | 4 ++-- frame/contracts/src/wasm/runtime.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 91679a9216a16..6602ae0ea3a56 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -650,7 +650,7 @@ fn dispatch_call_not_dispatched_after_top_level_transaction_failure() { 100_000, vec![], ), - "during execution" + "contract trapped during execution" ); assert_eq!(System::events(), vec![ EventRecord { @@ -1533,7 +1533,7 @@ fn storage_max_value_limit() { 100_000, Encode::encode(&(self::MaxValueSize::get() + 1)), ), - "during execution" + "contract trapped during execution" ); }); } @@ -2056,7 +2056,7 @@ fn cannot_self_destruct_while_live() { 100_000, vec![0], ), - "during execution" + "contract trapped during execution" ); // Check that BOB is still alive. diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index 8e02eb482bb8a..c0ce838e1ae57 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/frame/contracts/src/wasm/mod.rs @@ -1430,7 +1430,7 @@ mod tests { MockExt::default(), &mut gas_meter ), - Err(ExecError { reason: DispatchError::Other("during execution"), buffer: _ }) + Err(ExecError { reason: DispatchError::Other("contract trapped during execution"), buffer: _ }) ); } @@ -1472,7 +1472,7 @@ mod tests { MockExt::default(), &mut gas_meter ), - Err(ExecError { reason: DispatchError::Other("during execution"), buffer: _ }) + Err(ExecError { reason: DispatchError::Other("contract trapped during execution"), buffer: _ }) ); } diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index 81b0809f82b02..75751b6d359a3 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -109,7 +109,7 @@ pub(crate) fn to_execution_result( Err(ExecError { reason: "validation error".into(), buffer: runtime.scratch_buf }), // Any other kind of a trap should result in a failure. Err(sp_sandbox::Error::Execution) | Err(sp_sandbox::Error::OutOfBounds) => - Err(ExecError { reason: "during execution".into(), buffer: runtime.scratch_buf }), + Err(ExecError { reason: "contract trapped during execution".into(), buffer: runtime.scratch_buf }), } } From 437c9e0e7b4561cb4921a3a6275acd42fe1102da Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 11:53:56 +0100 Subject: [PATCH 2/9] contracts: rename Event::Contract -> Event::ContractExecution --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 2c77173135077..9d786c320b5a9 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -782,7 +782,7 @@ where fn deposit_event(&mut self, topics: Vec, data: Vec) { self.ctx.deferred.push(DeferredAction::DepositEvent { topics, - event: RawEvent::Contract(self.ctx.self_account.clone(), data), + event: RawEvent::ContractExecution(self.ctx.self_account.clone(), data), }); } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index df8da8866020d..28286364c45d8 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -906,8 +906,8 @@ decl_event! { /// successful execution or not. Dispatched(AccountId, bool), - /// An event from contract of account. - Contract(AccountId, Vec), + /// An event deposited upon execution of a contract from the account. + ContractExecution(AccountId, Vec), } } From 62c5b534e2b179ff2cb39ecde1ce3a958fa04f39 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 13:16:00 +0100 Subject: [PATCH 3/9] contracts: fix tests after ContractExecution renaming --- frame/contracts/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 6602ae0ea3a56..2507488b74e49 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -453,7 +453,7 @@ fn instantiate_and_call_and_deposit_event() { }, EventRecord { phase: Phase::ApplyExtrinsic(0), - event: MetaEvent::contract(RawEvent::Contract(BOB, vec![1, 2, 3, 4])), + event: MetaEvent::contract(RawEvent::ContractExecution(BOB, vec![1, 2, 3, 4])), topics: vec![], }, EventRecord { From 54c28be115f93533eb6794a1f09b98944b850c72 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 14:07:48 +0100 Subject: [PATCH 4/9] contracts: Add Evicted and Restored events --- frame/contracts/src/lib.rs | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 28286364c45d8..a11f1d7682ddd 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -671,6 +671,7 @@ decl_module! { // If poking the contract has lead to eviction of the contract, give out the rewards. if rent::try_evict::(&dest, handicap) == rent::RentOutcome::Evicted { T::Currency::deposit_into_existing(&rewarded, T::SurchargeReward::get())?; + Self::deposit_event(RawEvent::Evicted(dest)); } } @@ -786,7 +787,12 @@ impl Module { rent_allowance, delta, } => { - let _result = Self::restore_to(donor, dest, code_hash, rent_allowance, delta); + let result = Self::restore_to( + donor.clone(), dest.clone(), code_hash.clone(), rent_allowance.clone(), delta + ); + Self::deposit_event( + RawEvent::Restored(donor, dest, code_hash, rent_allowance, result.is_ok()) + ); } } }); @@ -896,6 +902,22 @@ decl_event! { /// Contract deployed by address at the specified address. Instantiated(AccountId, AccountId), + /// Contract has been evicted and is now in tombstone state. + Evicted(AccountId), + + /// Contract has successfully been restored from tombstone state to a + /// ctive. + /// + /// + /// # Params + /// + /// - `donor`: `AccountId`: Account ID of the restoring contract + /// - `dest`: `AccountId`: Account ID of the restored contract + /// - `code_hash`: `Hash`: Code hash of the restored contract + /// - `rent_allowance: `Balance`: Rent allowance of the restored contract + /// - `success`: `bool`: True if the restoration was successful + Restored(AccountId, AccountId, Hash, Balance, bool), + /// Code with the specified hash has been stored. CodeStored(Hash), From afb940f84626b2059f9c78f483b43af1ce14b68e Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 14:29:37 +0100 Subject: [PATCH 5/9] fix doc comment --- frame/contracts/src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a11f1d7682ddd..d33f3078fbbc2 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -905,9 +905,7 @@ decl_event! { /// Contract has been evicted and is now in tombstone state. Evicted(AccountId), - /// Contract has successfully been restored from tombstone state to a - /// ctive. - /// + /// Restoration for a contract has been initiated. /// /// # Params /// From f0b7770afc0a93b65fd532cca97c21ee4bb3d7ad Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 15:35:21 +0100 Subject: [PATCH 6/9] wrap to not go over (soft) 100 column line limit --- frame/contracts/src/wasm/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index c0ce838e1ae57..60402cf3a09cf 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/frame/contracts/src/wasm/mod.rs @@ -1430,7 +1430,9 @@ mod tests { MockExt::default(), &mut gas_meter ), - Err(ExecError { reason: DispatchError::Other("contract trapped during execution"), buffer: _ }) + Err(ExecError { + reason: DispatchError::Other("contract trapped during execution"), buffer: _ + }) ); } From 8576b0a292dee873a832808468b5d819cfacc156 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 15:46:08 +0100 Subject: [PATCH 7/9] add event deposit for eventual eviction upon pay_rent --- frame/contracts/src/exec.rs | 10 +++++++++- frame/contracts/src/rent.rs | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 9d786c320b5a9..a70729a3de50b 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -356,7 +356,15 @@ where // Assumption: pay_rent doesn't collide with overlay because // pay_rent will be done on first call and dest contract and balance // cannot be changed before the first call - let contract_info = rent::pay_rent::(&dest); + let (rent_outcome, contract_info) = rent::pay_rent::(&dest); + + // Deposit an event to indicate contract eviction. + if rent_outcome == rent::RentOutcome::Evicted { + self.deferred.push(DeferredAction::DepositEvent { + event: RawEvent::Evicted(dest.clone()), + topics: Vec::new(), + }); + } // Calls to dead contracts always fail. if let Some(ContractInfo::Tombstone(_)) = contract_info { diff --git a/frame/contracts/src/rent.rs b/frame/contracts/src/rent.rs index 59c8b02d199cf..3421cdd93265b 100644 --- a/frame/contracts/src/rent.rs +++ b/frame/contracts/src/rent.rs @@ -181,8 +181,8 @@ fn try_evict_or_and_pay_rent( /// Make account paying the rent for the current block number /// /// NOTE: This function acts eagerly. -pub fn pay_rent(account: &T::AccountId) -> Option> { - try_evict_or_and_pay_rent::(account, Zero::zero(), true).1 +pub fn pay_rent(account: &T::AccountId) -> (RentOutcome, Option>) { + try_evict_or_and_pay_rent::(account, Zero::zero(), true) } /// Evict the account if it should be evicted at the given block number. From 60dab593ffa2367637d14417d1776579d99c5f7f Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 17:28:56 +0100 Subject: [PATCH 8/9] contracts: adjust tests for the new events --- frame/contracts/src/tests.rs | 90 +++++++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 2507488b74e49..bac81b414d3a7 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -1139,8 +1139,16 @@ fn call_removed_contract() { Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()), "contract has been evicted" ); + // Calling a contract that is about to evict shall emit an event. + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Evicted(BOB)), + topics: vec![], + }, + ]); - // Subsequent contract calls should also fail. + // Subsequent contract calls should also fail. assert_err!( Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()), "contract has been evicted" @@ -1367,6 +1375,9 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: // Advance 4 blocks, to the 5th. initialize_block(5); + /// Preserve `BOB`'s code hash for later introspection. + let bob_code_hash = ContractInfoOf::::get(BOB).unwrap() + .get_alive().unwrap().code_hash; // Call `BOB`, which makes it pay rent. Since the rent allowance is set to 0 // we expect that it will get removed leaving tombstone. assert_err!( @@ -1374,6 +1385,15 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: "contract has been evicted" ); assert!(ContractInfoOf::::get(BOB).unwrap().get_tombstone().is_some()); + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract( + RawEvent::Evicted(BOB.clone()) + ), + topics: vec![], + }, + ]); /// Create another account with the address `DJANGO` with `CODE_RESTORATION`. /// @@ -1416,6 +1436,60 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: assert_eq!(django_contract.storage_size, 16); assert_eq!(django_contract.trie_id, django_trie_id); assert_eq!(django_contract.deduct_block, System::block_number()); + match (test_different_storage, test_restore_to_with_dirty_storage) { + (true, false) => { + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract( + RawEvent::Restored(DJANGO, BOB, bob_code_hash, 50, false) + ), + topics: vec![], + }, + ]); + } + (_, true) => { + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Evicted(BOB)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::balances(pallet_balances::RawEvent::NewAccount(CHARLIE, 1_000_000)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::balances(pallet_balances::RawEvent::NewAccount(DJANGO, 30_000)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Transfer(CHARLIE, DJANGO, 30_000)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Instantiated(CHARLIE, DJANGO)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract(RawEvent::Restored( + DJANGO, + BOB, + bob_code_hash, + 50, + false, + )), + topics: vec![], + }, + ]); + } + _ => unreachable!(), + } } else { // Here we expect that the restoration is succeeded. Check that the restoration // contract `DJANGO` ceased to exist and that `BOB` returned back. @@ -1427,6 +1501,20 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: assert_eq!(bob_contract.trie_id, django_trie_id); assert_eq!(bob_contract.deduct_block, System::block_number()); assert!(ContractInfoOf::::get(DJANGO).is_none()); + assert_eq!(System::events(), vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::balances(balances::RawEvent::ReapedAccount(DJANGO, 0)), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: MetaEvent::contract( + RawEvent::Restored(DJANGO, BOB, bob_contract.code_hash, 50, true) + ), + topics: vec![], + }, + ]); } }); } From 28ff8259b605c66e9f868cc53ca209e29c3d8d6d Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 20 Jan 2020 17:54:36 +0100 Subject: [PATCH 9/9] emit Evicted event immediately and add tombstone flag bool --- frame/contracts/src/exec.rs | 10 +--------- frame/contracts/src/lib.rs | 8 ++++++-- frame/contracts/src/rent.rs | 10 +++++++--- frame/contracts/src/tests.rs | 6 +++--- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index a70729a3de50b..9d786c320b5a9 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -356,15 +356,7 @@ where // Assumption: pay_rent doesn't collide with overlay because // pay_rent will be done on first call and dest contract and balance // cannot be changed before the first call - let (rent_outcome, contract_info) = rent::pay_rent::(&dest); - - // Deposit an event to indicate contract eviction. - if rent_outcome == rent::RentOutcome::Evicted { - self.deferred.push(DeferredAction::DepositEvent { - event: RawEvent::Evicted(dest.clone()), - topics: Vec::new(), - }); - } + let contract_info = rent::pay_rent::(&dest); // Calls to dead contracts always fail. if let Some(ContractInfo::Tombstone(_)) = contract_info { diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index d33f3078fbbc2..40ce86518a5e4 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -671,7 +671,6 @@ decl_module! { // If poking the contract has lead to eviction of the contract, give out the rewards. if rent::try_evict::(&dest, handicap) == rent::RentOutcome::Evicted { T::Currency::deposit_into_existing(&rewarded, T::SurchargeReward::get())?; - Self::deposit_event(RawEvent::Evicted(dest)); } } @@ -903,7 +902,12 @@ decl_event! { Instantiated(AccountId, AccountId), /// Contract has been evicted and is now in tombstone state. - Evicted(AccountId), + /// + /// # Params + /// + /// - `contract`: `AccountId`: The account ID of the evicted contract. + /// - `tombstone`: `bool`: True if the evicted contract left behind a tombstone. + Evicted(AccountId, bool), /// Restoration for a contract has been initiated. /// diff --git a/frame/contracts/src/rent.rs b/frame/contracts/src/rent.rs index 3421cdd93265b..508511da4cbe3 100644 --- a/frame/contracts/src/rent.rs +++ b/frame/contracts/src/rent.rs @@ -14,7 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo, Trait, AliveContractInfo}; +use crate::{Module, RawEvent, BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo, + Trait, AliveContractInfo}; use sp_runtime::traits::{Bounded, CheckedDiv, CheckedMul, Saturating, Zero, SaturatedConversion}; use frame_support::traits::{Currency, ExistenceRequirement, Get, WithdrawReason, OnUnbalanced}; @@ -101,6 +102,7 @@ fn try_evict_or_and_pay_rent( // The contract cannot afford to leave a tombstone, so remove the contract info altogether. >::remove(account); child::kill_storage(&contract.trie_id, contract.child_trie_unique_id()); + >::deposit_event(RawEvent::Evicted(account.clone(), false)); return (RentOutcome::Evicted, None); } @@ -160,6 +162,8 @@ fn try_evict_or_and_pay_rent( child::kill_storage(&contract.trie_id, contract.child_trie_unique_id()); + >::deposit_event(RawEvent::Evicted(account.clone(), true)); + return (RentOutcome::Evicted, Some(tombstone_info)); } @@ -181,8 +185,8 @@ fn try_evict_or_and_pay_rent( /// Make account paying the rent for the current block number /// /// NOTE: This function acts eagerly. -pub fn pay_rent(account: &T::AccountId) -> (RentOutcome, Option>) { - try_evict_or_and_pay_rent::(account, Zero::zero(), true) +pub fn pay_rent(account: &T::AccountId) -> Option> { + try_evict_or_and_pay_rent::(account, Zero::zero(), true).1 } /// Evict the account if it should be evicted at the given block number. diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index bac81b414d3a7..9a2ef36bb86f0 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -1143,7 +1143,7 @@ fn call_removed_contract() { assert_eq!(System::events(), vec![ EventRecord { phase: Phase::ApplyExtrinsic(0), - event: MetaEvent::contract(RawEvent::Evicted(BOB)), + event: MetaEvent::contract(RawEvent::Evicted(BOB, true)), topics: vec![], }, ]); @@ -1389,7 +1389,7 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: EventRecord { phase: Phase::ApplyExtrinsic(0), event: MetaEvent::contract( - RawEvent::Evicted(BOB.clone()) + RawEvent::Evicted(BOB.clone(), true) ), topics: vec![], }, @@ -1452,7 +1452,7 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: assert_eq!(System::events(), vec![ EventRecord { phase: Phase::ApplyExtrinsic(0), - event: MetaEvent::contract(RawEvent::Evicted(BOB)), + event: MetaEvent::contract(RawEvent::Evicted(BOB, true)), topics: vec![], }, EventRecord {