From 62df2814d59e500dd43ea39f902e30670cc24451 Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Fri, 17 May 2024 15:47:01 +0200 Subject: [PATCH] Contracts: Remove topics for internal events (#4510) --- prdoc/pr_4510.prdoc | 13 ++++ substrate/frame/contracts/src/exec.rs | 58 +++++++------- substrate/frame/contracts/src/lib.rs | 24 +++--- .../frame/contracts/src/storage/meter.rs | 30 +++---- substrate/frame/contracts/src/tests.rs | 78 ++++++++----------- substrate/frame/contracts/src/wasm/mod.rs | 22 +++--- 6 files changed, 107 insertions(+), 118 deletions(-) create mode 100644 prdoc/pr_4510.prdoc diff --git a/prdoc/pr_4510.prdoc b/prdoc/pr_4510.prdoc new file mode 100644 index 000000000000..fbd9bf961fe9 --- /dev/null +++ b/prdoc/pr_4510.prdoc @@ -0,0 +1,13 @@ +title: "[Contracts] Remove internal topic index" + +doc: + - audience: Runtime Dev + description: | + This PR removes topics from internal events emitted by pallet_contracts. It does not touch the `deposit_event` host function used by + smart contracts that can still include topics. + Event topics incurs significant Storage costs, and are only used by light clients to index events and avoid downloading the entire block. + They are not used by Dapp or Indexers that download the whole block anyway. + +crates: + - name: pallet-contracts + bump: patch diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs index 31cdadb4bb43..21ebb1e8c5f3 100644 --- a/substrate/frame/contracts/src/exec.rs +++ b/substrate/frame/contracts/src/exec.rs @@ -46,7 +46,7 @@ use sp_core::{ }; use sp_io::{crypto::secp256k1_ecdsa_recover_compressed, hashing::blake2_256}; use sp_runtime::{ - traits::{Convert, Dispatchable, Hash, Zero}, + traits::{Convert, Dispatchable, Zero}, DispatchError, }; use sp_std::{fmt::Debug, marker::PhantomData, mem, prelude::*, vec::Vec}; @@ -983,16 +983,16 @@ where let caller = self.caller().account_id()?.clone(); // Deposit an instantiation event. - Contracts::::deposit_event( - vec![T::Hashing::hash_of(&caller), T::Hashing::hash_of(account_id)], - Event::Instantiated { deployer: caller, contract: account_id.clone() }, - ); + Contracts::::deposit_event(Event::Instantiated { + deployer: caller, + contract: account_id.clone(), + }); }, (ExportedFunction::Call, Some(code_hash)) => { - Contracts::::deposit_event( - vec![T::Hashing::hash_of(account_id), T::Hashing::hash_of(&code_hash)], - Event::DelegateCalled { contract: account_id.clone(), code_hash }, - ); + Contracts::::deposit_event(Event::DelegateCalled { + contract: account_id.clone(), + code_hash, + }); }, (ExportedFunction::Call, None) => { // If a special limit was set for the sub-call, we enforce it here. @@ -1002,10 +1002,10 @@ where frame.nested_storage.enforce_subcall_limit(contract)?; let caller = self.caller(); - Contracts::::deposit_event( - vec![T::Hashing::hash_of(&caller), T::Hashing::hash_of(&account_id)], - Event::Called { caller: caller.clone(), contract: account_id.clone() }, - ); + Contracts::::deposit_event(Event::Called { + caller: caller.clone(), + contract: account_id.clone(), + }); }, } @@ -1324,13 +1324,10 @@ where .charge_deposit(frame.account_id.clone(), StorageDeposit::Refund(*deposit)); } - Contracts::::deposit_event( - vec![T::Hashing::hash_of(&frame.account_id), T::Hashing::hash_of(&beneficiary)], - Event::Terminated { - contract: frame.account_id.clone(), - beneficiary: beneficiary.clone(), - }, - ); + Contracts::::deposit_event(Event::Terminated { + contract: frame.account_id.clone(), + beneficiary: beneficiary.clone(), + }); Ok(()) } @@ -1422,7 +1419,7 @@ where } fn deposit_event(&mut self, topics: Vec, data: Vec) { - Contracts::::deposit_event( + Contracts::::deposit_indexed_event( topics, Event::ContractEmitted { contract: self.top_frame().account_id.clone(), data }, ); @@ -1527,14 +1524,11 @@ where Self::increment_refcount(hash)?; Self::decrement_refcount(prev_hash); - Contracts::::deposit_event( - vec![T::Hashing::hash_of(&frame.account_id), hash, prev_hash], - Event::ContractCodeUpdated { - contract: frame.account_id.clone(), - new_code_hash: hash, - old_code_hash: prev_hash, - }, - ); + Contracts::::deposit_event(Event::ContractCodeUpdated { + contract: frame.account_id.clone(), + new_code_hash: hash, + old_code_hash: prev_hash, + }); Ok(()) } @@ -1639,7 +1633,7 @@ mod tests { exec::ExportedFunction::*, gas::GasMeter, tests::{ - test_utils::{get_balance, hash, place_contract, set_balance}, + test_utils::{get_balance, place_contract, set_balance}, ExtBuilder, RuntimeCall, RuntimeEvent as MetaEvent, Test, TestFilter, ALICE, BOB, CHARLIE, GAS_LIMIT, }, @@ -3164,7 +3158,7 @@ mod tests { caller: Origin::from_account_id(ALICE), contract: BOB, }), - topics: vec![hash(&Origin::::from_account_id(ALICE)), hash(&BOB)], + topics: vec![], }, ] ); @@ -3264,7 +3258,7 @@ mod tests { caller: Origin::from_account_id(ALICE), contract: BOB, }), - topics: vec![hash(&Origin::::from_account_id(ALICE)), hash(&BOB)], + topics: vec![], }, ] ); diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 3e87eb9f37ea..d20f3c15fb56 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -135,7 +135,7 @@ use frame_system::{ use scale_info::TypeInfo; use smallvec::Array; use sp_runtime::{ - traits::{Convert, Dispatchable, Hash, Saturating, StaticLookup, Zero}, + traits::{Convert, Dispatchable, Saturating, StaticLookup, Zero}, DispatchError, RuntimeDebug, }; use sp_std::{fmt::Debug, prelude::*}; @@ -833,14 +833,11 @@ pub mod pallet { }; >>::increment_refcount(code_hash)?; >>::decrement_refcount(contract.code_hash); - Self::deposit_event( - vec![T::Hashing::hash_of(&dest), code_hash, contract.code_hash], - Event::ContractCodeUpdated { - contract: dest.clone(), - new_code_hash: code_hash, - old_code_hash: contract.code_hash, - }, - ); + Self::deposit_event(Event::ContractCodeUpdated { + contract: dest.clone(), + new_code_hash: code_hash, + old_code_hash: contract.code_hash, + }); contract.code_hash = code_hash; Ok(()) }) @@ -1827,8 +1824,13 @@ impl Pallet { Ok(()) } - /// Deposit a pallet contracts event. Handles the conversion to the overarching event type. - fn deposit_event(topics: Vec, event: Event) { + /// Deposit a pallet contracts event. + fn deposit_event(event: Event) { + >::deposit_event(::RuntimeEvent::from(event)) + } + + /// Deposit a pallet contracts indexed event. + fn deposit_indexed_event(topics: Vec, event: Event) { >::deposit_event_indexed( &topics, ::RuntimeEvent::from(event).into(), diff --git a/substrate/frame/contracts/src/storage/meter.rs b/substrate/frame/contracts/src/storage/meter.rs index 5db9a772ad82..7c55ce5d3f0c 100644 --- a/substrate/frame/contracts/src/storage/meter.rs +++ b/substrate/frame/contracts/src/storage/meter.rs @@ -34,10 +34,10 @@ use frame_support::{ DefaultNoBound, RuntimeDebugNoBound, }; use sp_runtime::{ - traits::{Hash as HashT, Saturating, Zero}, + traits::{Saturating, Zero}, DispatchError, FixedPointNumber, FixedU128, }; -use sp_std::{fmt::Debug, marker::PhantomData, vec, vec::Vec}; +use sp_std::{fmt::Debug, marker::PhantomData, vec::Vec}; /// Deposit that uses the native fungible's balance type. pub type DepositOf = Deposit>; @@ -551,14 +551,11 @@ impl Ext for ReservingExt { Fortitude::Polite, )?; - Pallet::::deposit_event( - vec![T::Hashing::hash_of(&origin), T::Hashing::hash_of(&contract)], - Event::StorageDepositTransferredAndHeld { - from: origin.clone(), - to: contract.clone(), - amount: *amount, - }, - ); + Pallet::::deposit_event(Event::StorageDepositTransferredAndHeld { + from: origin.clone(), + to: contract.clone(), + amount: *amount, + }); }, Deposit::Refund(amount) => { let transferred = T::Currency::transfer_on_hold( @@ -571,14 +568,11 @@ impl Ext for ReservingExt { Fortitude::Polite, )?; - Pallet::::deposit_event( - vec![T::Hashing::hash_of(&contract), T::Hashing::hash_of(&origin)], - Event::StorageDepositTransferredAndReleased { - from: contract.clone(), - to: origin.clone(), - amount: transferred, - }, - ); + Pallet::::deposit_event(Event::StorageDepositTransferredAndReleased { + from: contract.clone(), + to: origin.clone(), + amount: transferred, + }); if transferred < *amount { // This should never happen, if it does it means that there is a bug in the diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 8fe845fcf0f8..251c037d317d 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -20,13 +20,13 @@ mod test_debug; use self::{ test_debug::TestDebug, - test_utils::{ensure_stored, expected_deposit, hash}, + test_utils::{ensure_stored, expected_deposit}, }; use crate::{ self as pallet_contracts, chain_extension::{ ChainExtension, Environment, Ext, InitState, RegisteredChainExtension, - Result as ExtensionResult, RetVal, ReturnFlags, SysConfig, + Result as ExtensionResult, RetVal, ReturnFlags, }, exec::{Frame, Key}, migration::codegen::LATEST_MIGRATION_VERSION, @@ -63,7 +63,7 @@ use sp_io::hashing::blake2_256; use sp_keystore::{testing::MemoryKeystore, KeystoreExt}; use sp_runtime::{ testing::H256, - traits::{BlakeTwo256, Convert, Hash, IdentityLookup}, + traits::{BlakeTwo256, Convert, IdentityLookup}, AccountId32, BuildStorage, DispatchError, Perbill, TokenError, }; @@ -97,7 +97,7 @@ macro_rules! assert_refcount { } pub mod test_utils { - use super::{Contracts, DepositPerByte, DepositPerItem, Hash, SysConfig, Test}; + use super::{Contracts, DepositPerByte, DepositPerItem, Test}; use crate::{ exec::AccountIdOf, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, Nonce, PristineCode, @@ -145,9 +145,6 @@ pub mod test_utils { .saturating_mul(info_size) .saturating_add(DepositPerItem::get()) } - pub fn hash(s: &S) -> <::Hashing as Hash>::Output { - <::Hashing as Hash>::hash_of(s) - } pub fn expected_deposit(code_len: usize) -> u64 { // For code_info, the deposit for max_encoded_len is taken. let code_info_len = CodeInfo::::max_encoded_len() as u64; @@ -768,7 +765,7 @@ fn instantiate_and_call_and_deposit_event() { deployer: ALICE, contract: addr.clone() }), - topics: vec![hash(&ALICE), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -779,7 +776,7 @@ fn instantiate_and_call_and_deposit_event() { amount: test_utils::contract_info_storage_deposit(&addr), } ), - topics: vec![hash(&ALICE), hash(&addr)], + topics: vec![], }, ] ); @@ -1039,7 +1036,7 @@ fn deploy_and_call_other_contract() { deployer: caller_addr.clone(), contract: callee_addr.clone(), }), - topics: vec![hash(&caller_addr), hash(&callee_addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -1056,10 +1053,7 @@ fn deploy_and_call_other_contract() { caller: Origin::from_account_id(caller_addr.clone()), contract: callee_addr.clone(), }), - topics: vec![ - hash(&Origin::::from_account_id(caller_addr.clone())), - hash(&callee_addr) - ], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -1067,7 +1061,7 @@ fn deploy_and_call_other_contract() { caller: Origin::from_account_id(ALICE), contract: caller_addr.clone(), }), - topics: vec![hash(&Origin::::from_account_id(ALICE)), hash(&caller_addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -1078,7 +1072,7 @@ fn deploy_and_call_other_contract() { amount: test_utils::contract_info_storage_deposit(&callee_addr), } ), - topics: vec![hash(&ALICE), hash(&callee_addr)], + topics: vec![], }, ] ); @@ -1304,7 +1298,7 @@ fn self_destruct_works() { contract: addr.clone(), beneficiary: DJANGO }), - topics: vec![hash(&addr), hash(&DJANGO)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -1312,7 +1306,7 @@ fn self_destruct_works() { caller: Origin::from_account_id(ALICE), contract: addr.clone(), }), - topics: vec![hash(&Origin::::from_account_id(ALICE)), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -1323,7 +1317,7 @@ fn self_destruct_works() { amount: info_deposit, } ), - topics: vec![hash(&addr), hash(&ALICE)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2511,7 +2505,7 @@ fn upload_code_works() { deposit_held: deposit_expected, uploader: ALICE }), - topics: vec![code_hash], + topics: vec![], },] ); }); @@ -2599,7 +2593,7 @@ fn remove_code_works() { deposit_held: deposit_expected, uploader: ALICE }), - topics: vec![code_hash], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2608,7 +2602,7 @@ fn remove_code_works() { deposit_released: deposit_expected, remover: ALICE }), - topics: vec![code_hash], + topics: vec![], }, ] ); @@ -2648,7 +2642,7 @@ fn remove_code_wrong_origin() { deposit_held: deposit_expected, uploader: ALICE }), - topics: vec![code_hash], + topics: vec![], },] ); }); @@ -2727,7 +2721,7 @@ fn instantiate_with_zero_balance_works() { deposit_held: deposit_expected, uploader: ALICE }), - topics: vec![code_hash], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2759,7 +2753,7 @@ fn instantiate_with_zero_balance_works() { deployer: ALICE, contract: addr.clone(), }), - topics: vec![hash(&ALICE), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2770,7 +2764,7 @@ fn instantiate_with_zero_balance_works() { amount: test_utils::contract_info_storage_deposit(&addr), } ), - topics: vec![hash(&ALICE), hash(&addr)], + topics: vec![], }, ] ); @@ -2812,7 +2806,7 @@ fn instantiate_with_below_existential_deposit_works() { deposit_held: deposit_expected, uploader: ALICE }), - topics: vec![code_hash], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2853,7 +2847,7 @@ fn instantiate_with_below_existential_deposit_works() { deployer: ALICE, contract: addr.clone(), }), - topics: vec![hash(&ALICE), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2864,7 +2858,7 @@ fn instantiate_with_below_existential_deposit_works() { amount: test_utils::contract_info_storage_deposit(&addr), } ), - topics: vec![hash(&ALICE), hash(&addr)], + topics: vec![], }, ] ); @@ -2925,7 +2919,7 @@ fn storage_deposit_works() { caller: Origin::from_account_id(ALICE), contract: addr.clone(), }), - topics: vec![hash(&Origin::::from_account_id(ALICE)), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2936,7 +2930,7 @@ fn storage_deposit_works() { amount: charged0, } ), - topics: vec![hash(&ALICE), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2944,7 +2938,7 @@ fn storage_deposit_works() { caller: Origin::from_account_id(ALICE), contract: addr.clone(), }), - topics: vec![hash(&Origin::::from_account_id(ALICE)), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2955,7 +2949,7 @@ fn storage_deposit_works() { amount: charged1, } ), - topics: vec![hash(&ALICE), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2963,7 +2957,7 @@ fn storage_deposit_works() { caller: Origin::from_account_id(ALICE), contract: addr.clone(), }), - topics: vec![hash(&Origin::::from_account_id(ALICE)), hash(&addr)], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -2974,7 +2968,7 @@ fn storage_deposit_works() { amount: refunded0, } ), - topics: vec![hash(&addr.clone()), hash(&ALICE)], + topics: vec![], }, ] ); @@ -3078,7 +3072,7 @@ fn set_code_extrinsic() { new_code_hash, old_code_hash: code_hash, }), - topics: vec![hash(&addr), new_code_hash, code_hash], + topics: vec![], },] ); }); @@ -3230,7 +3224,7 @@ fn set_code_hash() { new_code_hash, old_code_hash: code_hash, }), - topics: vec![hash(&contract_addr), new_code_hash, code_hash], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -3238,10 +3232,7 @@ fn set_code_hash() { caller: Origin::from_account_id(ALICE), contract: contract_addr.clone(), }), - topics: vec![ - hash(&Origin::::from_account_id(ALICE)), - hash(&contract_addr) - ], + topics: vec![], }, EventRecord { phase: Phase::Initialization, @@ -3249,10 +3240,7 @@ fn set_code_hash() { caller: Origin::from_account_id(ALICE), contract: contract_addr.clone(), }), - topics: vec![ - hash(&Origin::::from_account_id(ALICE)), - hash(&contract_addr) - ], + topics: vec![], }, ], ); diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs index 8d7f928dba33..b40eb699db9e 100644 --- a/substrate/frame/contracts/src/wasm/mod.rs +++ b/substrate/frame/contracts/src/wasm/mod.rs @@ -184,10 +184,11 @@ impl WasmBlob { *existing = None; >::remove(&code_hash); - >::deposit_event( - vec![code_hash], - Event::CodeRemoved { code_hash, deposit_released, remover }, - ); + >::deposit_event(Event::CodeRemoved { + code_hash, + deposit_released, + remover, + }); Ok(()) } else { Err(>::CodeNotFound.into()) @@ -271,14 +272,11 @@ impl WasmBlob { self.code_info.refcount = 0; >::insert(code_hash, &self.code); *stored_code_info = Some(self.code_info.clone()); - >::deposit_event( - vec![code_hash], - Event::CodeStored { - code_hash, - deposit_held: deposit, - uploader: self.code_info.owner.clone(), - }, - ); + >::deposit_event(Event::CodeStored { + code_hash, + deposit_held: deposit, + uploader: self.code_info.owner.clone(), + }); Ok(deposit) }, }