From 77c78e1561bbe5ee0ecf414312bae82396ae6d11 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Wed, 15 Jan 2025 18:50:42 +0200 Subject: [PATCH 1/6] litep2p: Provide partial results to speedup GetRecord queries (#7099) This PR provides the partial results of the `GetRecord` kademlia query. This significantly improves the authority discovery records, from ~37 minutes to ~2/3 minutes. In contrast, libp2p discovers authority records in around ~10 minutes. The authority discovery was slow because litep2p provided the records only after the Kademlia query was completed. A normal Kademlia query completes in around 40 seconds to a few minutes. In this PR, partial records are provided as soon as they are discovered from the network. ### Testing Done Started a node in Kusama with `--validator` and litep2p backend. The node discovered 996/1000 authority records in ~ 1 minute 45 seconds. ![Screenshot 2025-01-09 at 12 26 08](https://github.com/user-attachments/assets/b618bf7c-2bba-43a0-a021-4047e854c075) ### Before & After In this image, on the left side is libp2p, in the middle litep2p without this PR, on the right litep2p with this PR ![Screenshot 2025-01-07 at 17 57 56](https://github.com/user-attachments/assets/a8d467f7-8dc7-461c-bcff-163b94d01ae8) Closes: https://github.com/paritytech/polkadot-sdk/issues/7077 cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile --- Cargo.lock | 4 +- Cargo.toml | 2 +- prdoc/pr_7099.prdoc | 16 ++++ .../client/network/src/litep2p/discovery.rs | 33 +++++-- substrate/client/network/src/litep2p/mod.rs | 87 ++++++++----------- 5 files changed, 79 insertions(+), 63 deletions(-) create mode 100644 prdoc/pr_7099.prdoc diff --git a/Cargo.lock b/Cargo.lock index 7725db743c41..0d71a770d38b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10446,9 +10446,9 @@ dependencies = [ [[package]] name = "litep2p" -version = "0.8.4" +version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2b0fef34af8847e816003bf7fdeac5ea50b9a7a88441ac927a6166b5e812ab79" +checksum = "6ca6ee50a125dc4fc4e9a3ae3640010796d1d07bc517a0ac715fdf0b24a0b6ac" dependencies = [ "async-trait", "bs58", diff --git a/Cargo.toml b/Cargo.toml index c30a9949e85e..eb99b80e16fa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -850,7 +850,7 @@ linked-hash-map = { version = "0.5.4" } linked_hash_set = { version = "0.1.4" } linregress = { version = "0.5.1" } lite-json = { version = "0.2.0", default-features = false } -litep2p = { version = "0.8.4", features = ["websocket"] } +litep2p = { version = "0.9.0", features = ["websocket"] } log = { version = "0.4.22", default-features = false } macro_magic = { version = "0.5.1" } maplit = { version = "1.0.2" } diff --git a/prdoc/pr_7099.prdoc b/prdoc/pr_7099.prdoc new file mode 100644 index 000000000000..58d809f3c090 --- /dev/null +++ b/prdoc/pr_7099.prdoc @@ -0,0 +1,16 @@ +title: Provide partial results to speedup GetRecord queries + +doc: + - audience: Node Dev + description: | + This PR provides the partial results of the GetRecord kademlia query. + + This significantly improves the authority discovery records, from ~37 minutes to ~2/3 minutes. + In contrast, libp2p discovers authority records in around ~10 minutes. + + The authority discovery was slow because litep2p provided the records only after the Kademlia query was completed. A normal Kademlia query completes in around 40 seconds to a few minutes. + In this PR, partial records are provided as soon as they are discovered from the network. + +crates: + - name: sc-network + bump: patch diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index b55df374f60e..eb571804f30e 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -33,8 +33,8 @@ use litep2p::{ identify::{Config as IdentifyConfig, IdentifyEvent}, kademlia::{ Config as KademliaConfig, ConfigBuilder as KademliaConfigBuilder, ContentProvider, - IncomingRecordValidationMode, KademliaEvent, KademliaHandle, QueryId, Quorum, - Record, RecordKey, RecordsType, + IncomingRecordValidationMode, KademliaEvent, KademliaHandle, PeerRecord, QueryId, + Quorum, Record, RecordKey, }, ping::{Config as PingConfig, PingEvent}, }, @@ -129,13 +129,19 @@ pub enum DiscoveryEvent { address: Multiaddr, }, - /// Record was found from the DHT. + /// `GetRecord` query succeeded. GetRecordSuccess { /// Query ID. query_id: QueryId, + }, - /// Records. - records: RecordsType, + /// Record was found from the DHT. + GetRecordPartialResult { + /// Query ID. + query_id: QueryId, + + /// Record. + record: PeerRecord, }, /// Record was successfully stored on the DHT. @@ -573,13 +579,24 @@ impl Stream for Discovery { peers: peers.into_iter().collect(), })) }, - Poll::Ready(Some(KademliaEvent::GetRecordSuccess { query_id, records })) => { + Poll::Ready(Some(KademliaEvent::GetRecordSuccess { query_id })) => { log::trace!( target: LOG_TARGET, - "`GET_RECORD` succeeded for {query_id:?}: {records:?}", + "`GET_RECORD` succeeded for {query_id:?}", ); - return Poll::Ready(Some(DiscoveryEvent::GetRecordSuccess { query_id, records })); + return Poll::Ready(Some(DiscoveryEvent::GetRecordSuccess { query_id })); + }, + Poll::Ready(Some(KademliaEvent::GetRecordPartialResult { query_id, record })) => { + log::trace!( + target: LOG_TARGET, + "`GET_RECORD` intermediary succeeded for {query_id:?}: {record:?}", + ); + + return Poll::Ready(Some(DiscoveryEvent::GetRecordPartialResult { + query_id, + record, + })); }, Poll::Ready(Some(KademliaEvent::PutRecordSuccess { query_id, key: _ })) => return Poll::Ready(Some(DiscoveryEvent::PutRecordSuccess { query_id })), diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 52b2970525df..fc4cce476283 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -58,7 +58,7 @@ use litep2p::{ protocol::{ libp2p::{ bitswap::Config as BitswapConfig, - kademlia::{QueryId, Record, RecordsType}, + kademlia::{QueryId, Record}, }, request_response::ConfigBuilder as RequestResponseConfigBuilder, }, @@ -836,23 +836,45 @@ impl NetworkBackend for Litep2pNetworkBac self.peerstore_handle.add_known_peer(peer.into()); } } - Some(DiscoveryEvent::GetRecordSuccess { query_id, records }) => { + Some(DiscoveryEvent::GetRecordPartialResult { query_id, record }) => { + if !self.pending_queries.contains_key(&query_id) { + log::error!( + target: LOG_TARGET, + "Missing/invalid pending query for `GET_VALUE` partial result: {query_id:?}" + ); + + continue + } + + let peer_id: sc_network_types::PeerId = record.peer.into(); + let record = PeerRecord { + record: P2PRecord { + key: record.record.key.to_vec().into(), + value: record.record.value, + publisher: record.record.publisher.map(|peer_id| { + let peer_id: sc_network_types::PeerId = peer_id.into(); + peer_id.into() + }), + expires: record.record.expires, + }, + peer: Some(peer_id.into()), + }; + + self.event_streams.send( + Event::Dht( + DhtEvent::ValueFound( + record.into() + ) + ) + ); + } + Some(DiscoveryEvent::GetRecordSuccess { query_id }) => { match self.pending_queries.remove(&query_id) { Some(KadQuery::GetValue(key, started)) => { log::trace!( target: LOG_TARGET, - "`GET_VALUE` for {:?} ({query_id:?}) succeeded", - key, + "`GET_VALUE` for {key:?} ({query_id:?}) succeeded", ); - for record in litep2p_to_libp2p_peer_record(records) { - self.event_streams.send( - Event::Dht( - DhtEvent::ValueFound( - record.into() - ) - ) - ); - } if let Some(ref metrics) = self.metrics { metrics @@ -1165,42 +1187,3 @@ impl NetworkBackend for Litep2pNetworkBac } } } - -// Glue code to convert from a litep2p records type to a libp2p2 PeerRecord. -fn litep2p_to_libp2p_peer_record(records: RecordsType) -> Vec { - match records { - litep2p::protocol::libp2p::kademlia::RecordsType::LocalStore(record) => { - vec![PeerRecord { - record: P2PRecord { - key: record.key.to_vec().into(), - value: record.value, - publisher: record.publisher.map(|peer_id| { - let peer_id: sc_network_types::PeerId = peer_id.into(); - peer_id.into() - }), - expires: record.expires, - }, - peer: None, - }] - }, - litep2p::protocol::libp2p::kademlia::RecordsType::Network(records) => records - .into_iter() - .map(|record| { - let peer_id: sc_network_types::PeerId = record.peer.into(); - - PeerRecord { - record: P2PRecord { - key: record.record.key.to_vec().into(), - value: record.record.value, - publisher: record.record.publisher.map(|peer_id| { - let peer_id: sc_network_types::PeerId = peer_id.into(); - peer_id.into() - }), - expires: record.record.expires, - }, - peer: Some(peer_id.into()), - } - }) - .collect::>(), - } -} From ece32e38a1a37aa354d51b16c07a42c66f23976e Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Wed, 15 Jan 2025 18:37:59 +0100 Subject: [PATCH 2/6] [pallet-revive] Remove debug buffer (#7163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the `debug_buffer` feature --------- Co-authored-by: command-bot <> Co-authored-by: Cyrill Leutwiler Co-authored-by: Alexander Theißen --- .../assets/asset-hub-westend/src/lib.rs | 15 +- prdoc/pr_7163.prdoc | 13 + substrate/bin/node/runtime/src/lib.rs | 10 +- substrate/frame/revive/README.md | 23 -- .../contracts/debug_message_invalid_utf8.rs | 33 --- .../debug_message_logging_disabled.rs | 33 --- .../fixtures/contracts/debug_message_works.rs | 33 --- substrate/frame/revive/proc-macro/src/lib.rs | 7 +- .../revive/src/benchmarking/call_builder.rs | 15 +- .../frame/revive/src/benchmarking/mod.rs | 28 --- substrate/frame/revive/src/exec.rs | 228 +----------------- substrate/frame/revive/src/lib.rs | 67 +---- substrate/frame/revive/src/limits.rs | 5 - substrate/frame/revive/src/primitives.rs | 53 +--- .../frame/revive/src/test_utils/builder.rs | 17 +- substrate/frame/revive/src/tests.rs | 146 +---------- .../frame/revive/src/tests/test_debug.rs | 4 - substrate/frame/revive/src/wasm/mod.rs | 2 +- substrate/frame/revive/src/wasm/runtime.rs | 34 +-- substrate/frame/revive/src/weights.rs | 21 -- substrate/frame/revive/uapi/src/host.rs | 20 -- .../frame/revive/uapi/src/host/riscv64.rs | 7 - substrate/frame/revive/uapi/src/lib.rs | 7 +- 23 files changed, 54 insertions(+), 767 deletions(-) create mode 100644 prdoc/pr_7163.prdoc delete mode 100644 substrate/frame/revive/fixtures/contracts/debug_message_invalid_utf8.rs delete mode 100644 substrate/frame/revive/fixtures/contracts/debug_message_logging_disabled.rs delete mode 100644 substrate/frame/revive/fixtures/contracts/debug_message_works.rs diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs index 7844b0d885ec..5966dd01f18f 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs @@ -952,11 +952,6 @@ parameter_types! { pub CodeHashLockupDepositPercent: Perbill = Perbill::from_percent(30); } -type EventRecord = frame_system::EventRecord< - ::RuntimeEvent, - ::Hash, ->; - impl pallet_revive::Config for Runtime { type Time = Timestamp; type Currency = Balances; @@ -2073,7 +2068,7 @@ impl_runtime_apis! { } } - impl pallet_revive::ReviveApi for Runtime + impl pallet_revive::ReviveApi for Runtime { fn balance(address: H160) -> U256 { Revive::evm_balance(&address) @@ -2108,7 +2103,7 @@ impl_runtime_apis! { gas_limit: Option, storage_deposit_limit: Option, input_data: Vec, - ) -> pallet_revive::ContractResult { + ) -> pallet_revive::ContractResult { let blockweights= ::BlockWeights::get(); Revive::bare_call( RuntimeOrigin::signed(origin), @@ -2117,8 +2112,6 @@ impl_runtime_apis! { gas_limit.unwrap_or(blockweights.max_block), pallet_revive::DepositLimit::Balance(storage_deposit_limit.unwrap_or(u128::MAX)), input_data, - pallet_revive::DebugInfo::UnsafeDebug, - pallet_revive::CollectEvents::UnsafeCollect, ) } @@ -2130,7 +2123,7 @@ impl_runtime_apis! { code: pallet_revive::Code, data: Vec, salt: Option<[u8; 32]>, - ) -> pallet_revive::ContractResult + ) -> pallet_revive::ContractResult { let blockweights= ::BlockWeights::get(); Revive::bare_instantiate( @@ -2141,8 +2134,6 @@ impl_runtime_apis! { code, data, salt, - pallet_revive::DebugInfo::UnsafeDebug, - pallet_revive::CollectEvents::UnsafeCollect, ) } diff --git a/prdoc/pr_7163.prdoc b/prdoc/pr_7163.prdoc new file mode 100644 index 000000000000..669c480b835b --- /dev/null +++ b/prdoc/pr_7163.prdoc @@ -0,0 +1,13 @@ +title: '[pallet-revive] Remove debug buffer' +doc: +- audience: Runtime Dev + description: Remove the `debug_buffer` feature +crates: +- name: asset-hub-westend-runtime + bump: minor +- name: pallet-revive + bump: major +- name: pallet-revive-proc-macro + bump: minor +- name: pallet-revive-uapi + bump: minor diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index e11a009c1c3f..97728f12f5f9 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -3212,7 +3212,7 @@ impl_runtime_apis! { } } - impl pallet_revive::ReviveApi for Runtime + impl pallet_revive::ReviveApi for Runtime { fn balance(address: H160) -> U256 { Revive::evm_balance(&address) @@ -3247,7 +3247,7 @@ impl_runtime_apis! { gas_limit: Option, storage_deposit_limit: Option, input_data: Vec, - ) -> pallet_revive::ContractResult { + ) -> pallet_revive::ContractResult { Revive::bare_call( RuntimeOrigin::signed(origin), dest, @@ -3255,8 +3255,6 @@ impl_runtime_apis! { gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block), pallet_revive::DepositLimit::Balance(storage_deposit_limit.unwrap_or(u128::MAX)), input_data, - pallet_revive::DebugInfo::UnsafeDebug, - pallet_revive::CollectEvents::UnsafeCollect, ) } @@ -3268,7 +3266,7 @@ impl_runtime_apis! { code: pallet_revive::Code, data: Vec, salt: Option<[u8; 32]>, - ) -> pallet_revive::ContractResult + ) -> pallet_revive::ContractResult { Revive::bare_instantiate( RuntimeOrigin::signed(origin), @@ -3278,8 +3276,6 @@ impl_runtime_apis! { code, data, salt, - pallet_revive::DebugInfo::UnsafeDebug, - pallet_revive::CollectEvents::UnsafeCollect, ) } diff --git a/substrate/frame/revive/README.md b/substrate/frame/revive/README.md index 575920dfaac7..7538f77d10bc 100644 --- a/substrate/frame/revive/README.md +++ b/substrate/frame/revive/README.md @@ -49,29 +49,6 @@ This module executes PolkaVM smart contracts. These can potentially be written i RISC-V. For now, the only officially supported languages are Solidity (via [`revive`](https://github.com/xermicus/revive)) and Rust (check the `fixtures` directory for Rust examples). -## Debugging - -Contracts can emit messages to the client when called as RPC through the -[`debug_message`](https://paritytech.github.io/substrate/master/pallet_revive/trait.SyscallDocs.html#tymethod.debug_message) -API. - -Those messages are gathered into an internal buffer and sent to the RPC client. It is up to the individual client if -and how those messages are presented to the user. - -This buffer is also printed as a debug message. In order to see these messages on the node console the log level for the -`runtime::revive` target needs to be raised to at least the `debug` level. However, those messages are easy to -overlook because of the noise generated by block production. A good starting point for observing them on the console is -using this command line in the root directory of the Substrate repository: - -```bash -cargo run --release -- --dev -lerror,runtime::revive=debug -``` - -This raises the log level of `runtime::revive` to `debug` and all other targets to `error` in order to prevent them -from spamming the console. - -`--dev`: Use a dev chain spec `--tmp`: Use temporary storage for chain data (the chain state is deleted on exit) - ## Host function tracing For contract authors, it can be a helpful debugging tool to see which host functions are called, with which arguments, diff --git a/substrate/frame/revive/fixtures/contracts/debug_message_invalid_utf8.rs b/substrate/frame/revive/fixtures/contracts/debug_message_invalid_utf8.rs deleted file mode 100644 index 6c850a9ec663..000000000000 --- a/substrate/frame/revive/fixtures/contracts/debug_message_invalid_utf8.rs +++ /dev/null @@ -1,33 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Emit a debug message with an invalid utf-8 code. -#![no_std] -#![no_main] - -extern crate common; -use uapi::{HostFn, HostFnImpl as api}; - -#[no_mangle] -#[polkavm_derive::polkavm_export] -pub extern "C" fn deploy() {} - -#[no_mangle] -#[polkavm_derive::polkavm_export] -pub extern "C" fn call() { - api::debug_message(b"\xFC").unwrap(); -} diff --git a/substrate/frame/revive/fixtures/contracts/debug_message_logging_disabled.rs b/substrate/frame/revive/fixtures/contracts/debug_message_logging_disabled.rs deleted file mode 100644 index 0ce2b6b5628d..000000000000 --- a/substrate/frame/revive/fixtures/contracts/debug_message_logging_disabled.rs +++ /dev/null @@ -1,33 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Emit a "Hello World!" debug message but assume that logging is disabled. -#![no_std] -#![no_main] - -extern crate common; -use uapi::{HostFn, HostFnImpl as api, ReturnErrorCode}; - -#[no_mangle] -#[polkavm_derive::polkavm_export] -pub extern "C" fn deploy() {} - -#[no_mangle] -#[polkavm_derive::polkavm_export] -pub extern "C" fn call() { - assert_eq!(api::debug_message(b"Hello World!"), Err(ReturnErrorCode::LoggingDisabled)); -} diff --git a/substrate/frame/revive/fixtures/contracts/debug_message_works.rs b/substrate/frame/revive/fixtures/contracts/debug_message_works.rs deleted file mode 100644 index 3a2509509d8f..000000000000 --- a/substrate/frame/revive/fixtures/contracts/debug_message_works.rs +++ /dev/null @@ -1,33 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Emit a "Hello World!" debug message. -#![no_std] -#![no_main] - -extern crate common; -use uapi::{HostFn, HostFnImpl as api}; - -#[no_mangle] -#[polkavm_derive::polkavm_export] -pub extern "C" fn deploy() {} - -#[no_mangle] -#[polkavm_derive::polkavm_export] -pub extern "C" fn call() { - api::debug_message(b"Hello World!").unwrap(); -} diff --git a/substrate/frame/revive/proc-macro/src/lib.rs b/substrate/frame/revive/proc-macro/src/lib.rs index b09bdef14632..6e38063d20a6 100644 --- a/substrate/frame/revive/proc-macro/src/lib.rs +++ b/substrate/frame/revive/proc-macro/src/lib.rs @@ -510,12 +510,7 @@ fn expand_functions(def: &EnvDef) -> TokenStream2 { quote! { // wrap body in closure to make sure the tracing is always executed let result = (|| #body)(); - if ::log::log_enabled!(target: "runtime::revive::strace", ::log::Level::Trace) { - use core::fmt::Write; - let mut msg = alloc::string::String::default(); - let _ = core::write!(&mut msg, #trace_fmt_str, #( #trace_fmt_args, )* result); - self.ext().append_debug_buffer(&msg); - } + ::log::trace!(target: "runtime::revive::strace", #trace_fmt_str, #( #trace_fmt_args, )* result); result } }; diff --git a/substrate/frame/revive/src/benchmarking/call_builder.rs b/substrate/frame/revive/src/benchmarking/call_builder.rs index 1177d47aadc3..077e18ff5f0b 100644 --- a/substrate/frame/revive/src/benchmarking/call_builder.rs +++ b/substrate/frame/revive/src/benchmarking/call_builder.rs @@ -22,7 +22,7 @@ use crate::{ storage::meter::Meter, transient_storage::MeterEntry, wasm::{PreparedCall, Runtime}, - BalanceOf, Config, DebugBuffer, Error, GasMeter, MomentOf, Origin, WasmBlob, Weight, + BalanceOf, Config, Error, GasMeter, MomentOf, Origin, WasmBlob, Weight, }; use alloc::{vec, vec::Vec}; use frame_benchmarking::benchmarking; @@ -38,7 +38,6 @@ pub struct CallSetup { gas_meter: GasMeter, storage_meter: Meter, value: BalanceOf, - debug_message: Option, data: Vec, transient_storage_size: u32, } @@ -91,7 +90,6 @@ where gas_meter: GasMeter::new(Weight::MAX), storage_meter, value: 0u32.into(), - debug_message: None, data: vec![], transient_storage_size: 0, } @@ -122,16 +120,6 @@ where self.transient_storage_size = size; } - /// Set the debug message. - pub fn enable_debug_message(&mut self) { - self.debug_message = Some(Default::default()); - } - - /// Get the debug message. - pub fn debug_message(&self) -> Option { - self.debug_message.clone() - } - /// Get the call's input data. pub fn data(&self) -> Vec { self.data.clone() @@ -150,7 +138,6 @@ where &mut self.gas_meter, &mut self.storage_meter, self.value, - self.debug_message.as_mut(), ); if self.transient_storage_size > 0 { Self::with_transient_storage(&mut ext.0, self.transient_storage_size).unwrap(); diff --git a/substrate/frame/revive/src/benchmarking/mod.rs b/substrate/frame/revive/src/benchmarking/mod.rs index 1796348ff321..e23554f21ba8 100644 --- a/substrate/frame/revive/src/benchmarking/mod.rs +++ b/substrate/frame/revive/src/benchmarking/mod.rs @@ -107,8 +107,6 @@ where Code::Upload(module.code), data, salt, - DebugInfo::Skip, - CollectEvents::Skip, ); let address = outcome.result?.addr; @@ -1047,32 +1045,6 @@ mod benchmarks { ); } - // Benchmark debug_message call - // Whereas this function is used in RPC mode only, it still should be secured - // against an excessive use. - // - // i: size of input in bytes up to maximum allowed contract memory or maximum allowed debug - // buffer size, whichever is less. - #[benchmark] - fn seal_debug_message( - i: Linear<0, { (limits::code::BLOB_BYTES).min(limits::DEBUG_BUFFER_BYTES) }>, - ) { - let mut setup = CallSetup::::default(); - setup.enable_debug_message(); - let (mut ext, _) = setup.ext(); - let mut runtime = crate::wasm::Runtime::<_, [u8]>::new(&mut ext, vec![]); - // Fill memory with printable ASCII bytes. - let mut memory = (0..i).zip((32..127).cycle()).map(|i| i.1).collect::>(); - - let result; - #[block] - { - result = runtime.bench_debug_message(memory.as_mut_slice(), 0, i); - } - assert_ok!(result); - assert_eq!(setup.debug_message().unwrap().len() as u32, i); - } - #[benchmark(skip_meta, pov_mode = Measured)] fn get_storage_empty() -> Result<(), BenchmarkError> { let max_key_len = limits::STORAGE_KEY_BYTES; diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index c069216d6cc7..e20c5dd7786e 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -24,8 +24,8 @@ use crate::{ runtime_decl_for_revive_api::{Decode, Encode, RuntimeDebugNoBound, TypeInfo}, storage::{self, meter::Diff, WriteOutcome}, transient_storage::TransientStorage, - BalanceOf, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, DebugBuffer, Error, - Event, ImmutableData, ImmutableDataOf, Pallet as Contracts, LOG_TARGET, + BalanceOf, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, Error, Event, + ImmutableData, ImmutableDataOf, Pallet as Contracts, }; use alloc::vec::Vec; use core::{fmt::Debug, marker::PhantomData, mem}; @@ -378,19 +378,6 @@ pub trait Ext: sealing::Sealed { /// Charges `diff` from the meter. fn charge_storage(&mut self, diff: &Diff); - /// Append a string to the debug buffer. - /// - /// It is added as-is without any additional new line. - /// - /// This is a no-op if debug message recording is disabled which is always the case - /// when the code is executing on-chain. - /// - /// Returns `true` if debug message recording is enabled. Otherwise `false` is returned. - fn append_debug_buffer(&mut self, msg: &str) -> bool; - - /// Returns `true` if debug message recording is enabled. Otherwise `false` is returned. - fn debug_buffer_enabled(&self) -> bool; - /// Call some dispatchable and return the result. fn call_runtime(&self, call: ::RuntimeCall) -> DispatchResultWithPostInfo; @@ -555,11 +542,6 @@ pub struct Stack<'a, T: Config, E> { frames: BoundedVec, ConstU32<{ limits::CALL_STACK_DEPTH }>>, /// Statically guarantee that each call stack has at least one frame. first_frame: Frame, - /// A text buffer used to output human readable information. - /// - /// All the bytes added to this field should be valid UTF-8. The buffer has no defined - /// structure and is intended to be shown to users as-is for debugging purposes. - debug_message: Option<&'a mut DebugBuffer>, /// Transient storage used to store data, which is kept for the duration of a transaction. transient_storage: TransientStorage, /// Whether or not actual transfer of funds should be performed. @@ -765,11 +747,6 @@ where { /// Create and run a new call stack by calling into `dest`. /// - /// # Note - /// - /// `debug_message` should only ever be set to `Some` when executing as an RPC because - /// it adds allocations and could be abused to drive the runtime into an OOM panic. - /// /// # Return Value /// /// Result<(ExecReturnValue, CodeSize), (ExecError, CodeSize)> @@ -781,7 +758,6 @@ where value: U256, input_data: Vec, skip_transfer: bool, - debug_message: Option<&'a mut DebugBuffer>, ) -> ExecResult { let dest = T::AddressMapper::to_account_id(&dest); if let Some((mut stack, executable)) = Self::new( @@ -791,7 +767,6 @@ where storage_meter, value, skip_transfer, - debug_message, )? { stack.run(executable, input_data).map(|_| stack.first_frame.last_frame_output) } else { @@ -801,11 +776,6 @@ where /// Create and run a new call stack by instantiating a new contract. /// - /// # Note - /// - /// `debug_message` should only ever be set to `Some` when executing as an RPC because - /// it adds allocations and could be abused to drive the runtime into an OOM panic. - /// /// # Return Value /// /// Result<(NewContractAccountId, ExecReturnValue), ExecError)> @@ -818,7 +788,6 @@ where input_data: Vec, salt: Option<&[u8; 32]>, skip_transfer: bool, - debug_message: Option<&'a mut DebugBuffer>, ) -> Result<(H160, ExecReturnValue), ExecError> { let (mut stack, executable) = Self::new( FrameArgs::Instantiate { @@ -832,7 +801,6 @@ where storage_meter, value, skip_transfer, - debug_message, )? .expect(FRAME_ALWAYS_EXISTS_ON_INSTANTIATE); let address = T::AddressMapper::to_address(&stack.top_frame().account_id); @@ -848,7 +816,6 @@ where gas_meter: &'a mut GasMeter, storage_meter: &'a mut storage::meter::Meter, value: BalanceOf, - debug_message: Option<&'a mut DebugBuffer>, ) -> (Self, E) { Self::new( FrameArgs::Call { @@ -861,7 +828,6 @@ where storage_meter, value.into(), false, - debug_message, ) .unwrap() .unwrap() @@ -878,7 +844,6 @@ where storage_meter: &'a mut storage::meter::Meter, value: U256, skip_transfer: bool, - debug_message: Option<&'a mut DebugBuffer>, ) -> Result, ExecError> { origin.ensure_mapped()?; let Some((first_frame, executable)) = Self::new_frame( @@ -903,7 +868,6 @@ where block_number: >::block_number(), first_frame, frames: Default::default(), - debug_message, transient_storage: TransientStorage::new(limits::TRANSIENT_STORAGE_BYTES), skip_transfer, _phantom: Default::default(), @@ -1250,13 +1214,6 @@ where } } } else { - if let Some((msg, false)) = self.debug_message.as_ref().map(|m| (m, m.is_empty())) { - log::debug!( - target: LOG_TARGET, - "Execution finished with debug buffer: {}", - core::str::from_utf8(msg).unwrap_or(""), - ); - } self.gas_meter.absorb_nested(mem::take(&mut self.first_frame.nested_gas)); if !persist { return; @@ -1759,28 +1716,6 @@ where self.top_frame_mut().nested_storage.charge(diff) } - fn debug_buffer_enabled(&self) -> bool { - self.debug_message.is_some() - } - - fn append_debug_buffer(&mut self, msg: &str) -> bool { - if let Some(buffer) = &mut self.debug_message { - buffer - .try_extend(&mut msg.bytes()) - .map_err(|_| { - log::debug!( - target: LOG_TARGET, - "Debug buffer (of {} bytes) exhausted!", - limits::DEBUG_BUFFER_BYTES, - ) - }) - .ok(); - true - } else { - false - } - } - fn call_runtime(&self, call: ::RuntimeCall) -> DispatchResultWithPostInfo { let mut origin: T::RuntimeOrigin = RawOrigin::Signed(self.account_id().clone()).into(); origin.add_filter(T::CallFilter::contains); @@ -2103,7 +2038,6 @@ mod tests { value.into(), vec![], false, - None, ), Ok(_) ); @@ -2196,7 +2130,6 @@ mod tests { value.into(), vec![], false, - None, ) .unwrap(); @@ -2237,7 +2170,6 @@ mod tests { value.into(), vec![], false, - None, )); assert_eq!(get_balance(&ALICE), 100 - value); @@ -2274,7 +2206,6 @@ mod tests { U256::zero(), vec![], false, - None, ), ExecError { error: Error::::CodeNotFound.into(), @@ -2292,7 +2223,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -2321,7 +2251,6 @@ mod tests { 55u64.into(), vec![], false, - None, ) .unwrap(); @@ -2371,7 +2300,6 @@ mod tests { U256::zero(), vec![], false, - None, ); let output = result.unwrap(); @@ -2401,7 +2329,6 @@ mod tests { U256::zero(), vec![], false, - None, ); let output = result.unwrap(); @@ -2431,7 +2358,6 @@ mod tests { U256::zero(), vec![1, 2, 3, 4], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -2468,7 +2394,6 @@ mod tests { vec![1, 2, 3, 4], Some(&[0; 32]), false, - None, ); assert_matches!(result, Ok(_)); }); @@ -2523,7 +2448,6 @@ mod tests { value.into(), vec![], false, - None, ); assert_matches!(result, Ok(_)); @@ -2588,7 +2512,6 @@ mod tests { U256::zero(), vec![], false, - None, ); assert_matches!(result, Ok(_)); @@ -2654,7 +2577,6 @@ mod tests { U256::zero(), vec![], false, - None, ); assert_matches!(result, Ok(_)); @@ -2687,7 +2609,6 @@ mod tests { U256::zero(), vec![], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -2725,7 +2646,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -2752,7 +2672,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -2797,7 +2716,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -2824,7 +2742,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -2851,7 +2768,6 @@ mod tests { 1u64.into(), vec![0], false, - None, ); assert_matches!(result, Err(_)); }); @@ -2896,7 +2812,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -2942,7 +2857,6 @@ mod tests { U256::zero(), vec![], false, - None, ); assert_matches!(result, Ok(_)); @@ -2969,7 +2883,6 @@ mod tests { vec![], Some(&[0; 32]), false, - None, ), Err(_) ); @@ -3005,7 +2918,6 @@ mod tests { vec![], Some(&[0 ;32]), false, - None, ), Ok((address, ref output)) if output.data == vec![80, 65, 83, 83] => address ); @@ -3060,7 +2972,6 @@ mod tests { vec![], Some(&[0; 32]), false, - None, ), Ok((address, ref output)) if output.data == vec![70, 65, 73, 76] => address ); @@ -3125,7 +3036,6 @@ mod tests { (min_balance * 10).into(), vec![], false, - None, ), Ok(_) ); @@ -3206,7 +3116,6 @@ mod tests { U256::zero(), vec![], false, - None, ), Ok(_) ); @@ -3250,7 +3159,6 @@ mod tests { vec![], Some(&[0; 32]), false, - None, ), Err(Error::::TerminatedInConstructor.into()) ); @@ -3315,7 +3223,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -3378,7 +3285,6 @@ mod tests { vec![], Some(&[0; 32]), false, - None, ); assert_matches!(result, Ok(_)); }); @@ -3425,113 +3331,11 @@ mod tests { U256::zero(), vec![], false, - None, ) .unwrap(); }); } - #[test] - fn printing_works() { - let code_hash = MockLoader::insert(Call, |ctx, _| { - ctx.ext.append_debug_buffer("This is a test"); - ctx.ext.append_debug_buffer("More text"); - exec_success() - }); - - let mut debug_buffer = DebugBuffer::try_from(Vec::new()).unwrap(); - - ExtBuilder::default().build().execute_with(|| { - let min_balance = ::Currency::minimum_balance(); - - let mut gas_meter = GasMeter::::new(GAS_LIMIT); - set_balance(&ALICE, min_balance * 10); - place_contract(&BOB, code_hash); - let origin = Origin::from_account_id(ALICE); - let mut storage_meter = storage::meter::Meter::new(&origin, 0, 0).unwrap(); - MockStack::run_call( - origin, - BOB_ADDR, - &mut gas_meter, - &mut storage_meter, - U256::zero(), - vec![], - false, - Some(&mut debug_buffer), - ) - .unwrap(); - }); - - assert_eq!(&String::from_utf8(debug_buffer.to_vec()).unwrap(), "This is a testMore text"); - } - - #[test] - fn printing_works_on_fail() { - let code_hash = MockLoader::insert(Call, |ctx, _| { - ctx.ext.append_debug_buffer("This is a test"); - ctx.ext.append_debug_buffer("More text"); - exec_trapped() - }); - - let mut debug_buffer = DebugBuffer::try_from(Vec::new()).unwrap(); - - ExtBuilder::default().build().execute_with(|| { - let min_balance = ::Currency::minimum_balance(); - - let mut gas_meter = GasMeter::::new(GAS_LIMIT); - set_balance(&ALICE, min_balance * 10); - place_contract(&BOB, code_hash); - let origin = Origin::from_account_id(ALICE); - let mut storage_meter = storage::meter::Meter::new(&origin, 0, 0).unwrap(); - let result = MockStack::run_call( - origin, - BOB_ADDR, - &mut gas_meter, - &mut storage_meter, - U256::zero(), - vec![], - false, - Some(&mut debug_buffer), - ); - assert!(result.is_err()); - }); - - assert_eq!(&String::from_utf8(debug_buffer.to_vec()).unwrap(), "This is a testMore text"); - } - - #[test] - fn debug_buffer_is_limited() { - let code_hash = MockLoader::insert(Call, move |ctx, _| { - ctx.ext.append_debug_buffer("overflowing bytes"); - exec_success() - }); - - // Pre-fill the buffer almost up to its limit, leaving not enough space to the message - let debug_buf_before = DebugBuffer::try_from(vec![0u8; DebugBuffer::bound() - 5]).unwrap(); - let mut debug_buf_after = debug_buf_before.clone(); - - ExtBuilder::default().build().execute_with(|| { - let min_balance = ::Currency::minimum_balance(); - let mut gas_meter = GasMeter::::new(GAS_LIMIT); - set_balance(&ALICE, min_balance * 10); - place_contract(&BOB, code_hash); - let origin = Origin::from_account_id(ALICE); - let mut storage_meter = storage::meter::Meter::new(&origin, 0, 0).unwrap(); - MockStack::run_call( - origin, - BOB_ADDR, - &mut gas_meter, - &mut storage_meter, - U256::zero(), - vec![], - false, - Some(&mut debug_buf_after), - ) - .unwrap(); - assert_eq!(debug_buf_before, debug_buf_after); - }); - } - #[test] fn call_reentry_direct_recursion() { // call the contract passed as input with disabled reentry @@ -3559,7 +3363,6 @@ mod tests { U256::zero(), CHARLIE_ADDR.as_bytes().to_vec(), false, - None, )); // Calling into oneself fails @@ -3572,7 +3375,6 @@ mod tests { U256::zero(), BOB_ADDR.as_bytes().to_vec(), false, - None, ) .map_err(|e| e.error), >::ReentranceDenied, @@ -3623,7 +3425,6 @@ mod tests { U256::zero(), vec![0], false, - None, ) .map_err(|e| e.error), >::ReentranceDenied, @@ -3658,7 +3459,6 @@ mod tests { U256::zero(), vec![], false, - None, ) .unwrap(); @@ -3743,7 +3543,6 @@ mod tests { U256::zero(), vec![], false, - None, ) .unwrap(); @@ -3870,7 +3669,6 @@ mod tests { vec![], Some(&[0; 32]), false, - None, ) .ok(); assert_eq!(System::account_nonce(&ALICE), 0); @@ -3884,7 +3682,6 @@ mod tests { vec![], Some(&[0; 32]), false, - None, )); assert_eq!(System::account_nonce(&ALICE), 1); @@ -3897,7 +3694,6 @@ mod tests { vec![], Some(&[0; 32]), false, - None, )); assert_eq!(System::account_nonce(&ALICE), 2); @@ -3910,7 +3706,6 @@ mod tests { vec![], Some(&[0; 32]), false, - None, )); assert_eq!(System::account_nonce(&ALICE), 3); }); @@ -3979,7 +3774,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -4091,7 +3885,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -4131,7 +3924,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -4171,7 +3963,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -4225,7 +4016,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -4282,7 +4072,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -4358,7 +4147,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -4429,7 +4217,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -4468,7 +4255,6 @@ mod tests { U256::zero(), vec![], false, - None, )); }); } @@ -4531,7 +4317,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -4565,7 +4350,6 @@ mod tests { U256::zero(), vec![], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -4641,7 +4425,6 @@ mod tests { U256::zero(), vec![], false, - None, ) .unwrap() }); @@ -4710,7 +4493,6 @@ mod tests { U256::zero(), vec![0], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -4782,7 +4564,6 @@ mod tests { U256::zero(), vec![], false, - None, ); assert_matches!(result, Ok(_)); }); @@ -4834,7 +4615,6 @@ mod tests { U256::zero(), vec![], false, - None, ) .unwrap() }); @@ -4904,7 +4684,6 @@ mod tests { U256::zero(), vec![], false, - None, ) .unwrap() }); @@ -4951,7 +4730,6 @@ mod tests { U256::zero(), vec![], false, - None, ) .unwrap() }); @@ -4996,7 +4774,6 @@ mod tests { U256::zero(), vec![], false, - None, ) .unwrap() }); @@ -5052,7 +4829,6 @@ mod tests { U256::zero(), vec![0], false, - None, ), Ok(_) ); diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index bdb4b92edd9e..403598ae136e 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -71,7 +71,7 @@ use frame_support::{ use frame_system::{ ensure_signed, pallet_prelude::{BlockNumberFor, OriginFor}, - EventRecord, Pallet as System, + Pallet as System, }; use pallet_transaction_payment::OnChargeTransaction; use scale_info::TypeInfo; @@ -98,9 +98,6 @@ type BalanceOf = <::Currency as Inspect<::AccountId>>::Balance; type OnChargeTransactionBalanceOf = <::OnChargeTransaction as OnChargeTransaction>::Balance; type CodeVec = BoundedVec>; -type EventRecordOf = - EventRecord<::RuntimeEvent, ::Hash>; -type DebugBuffer = BoundedVec>; type ImmutableData = BoundedVec>; /// Used as a sentinel value when reading and writing contract memory. @@ -258,9 +255,9 @@ pub mod pallet { #[pallet::no_default_bounds] type InstantiateOrigin: EnsureOrigin; - /// For most production chains, it's recommended to use the `()` implementation of this - /// trait. This implementation offers additional logging when the log target - /// "runtime::revive" is set to trace. + /// Debugging utilities for contracts. + /// For production chains, it's recommended to use the `()` implementation of this + /// trait. #[pallet::no_default_bounds] type Debug: Debugger; @@ -810,9 +807,8 @@ pub mod pallet { gas_limit, DepositLimit::Balance(storage_deposit_limit), data, - DebugInfo::Skip, - CollectEvents::Skip, ); + if let Ok(return_value) = &output.result { if return_value.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -848,8 +844,6 @@ pub mod pallet { Code::Existing(code_hash), data, salt, - DebugInfo::Skip, - CollectEvents::Skip, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -914,8 +908,6 @@ pub mod pallet { Code::Upload(code), data, salt, - DebugInfo::Skip, - CollectEvents::Skip, ); if let Ok(retval) = &output.result { if retval.result.did_revert() { @@ -1085,16 +1077,10 @@ where gas_limit: Weight, storage_deposit_limit: DepositLimit>, data: Vec, - debug: DebugInfo, - collect_events: CollectEvents, - ) -> ContractResult, EventRecordOf> { + ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); - let mut debug_message = if matches!(debug, DebugInfo::UnsafeDebug) { - Some(DebugBuffer::default()) - } else { - None - }; + let try_call = || { let origin = Origin::from_runtime_origin(origin)?; let mut storage_meter = match storage_deposit_limit { @@ -1109,7 +1095,6 @@ where Self::convert_native_to_evm(value), data, storage_deposit_limit.is_unchecked(), - debug_message.as_mut(), )?; storage_deposit = storage_meter .try_into_deposit(&origin, storage_deposit_limit.is_unchecked()) @@ -1119,18 +1104,11 @@ where Ok(result) }; let result = Self::run_guarded(try_call); - let events = if matches!(collect_events, CollectEvents::UnsafeCollect) { - Some(System::::read_events_no_consensus().map(|e| *e).collect()) - } else { - None - }; ContractResult { result: result.map_err(|r| r.error), gas_consumed: gas_meter.gas_consumed(), gas_required: gas_meter.gas_required(), storage_deposit, - debug_message: debug_message.unwrap_or_default().to_vec(), - events, } } @@ -1138,8 +1116,7 @@ where /// /// Identical to [`Self::instantiate`] or [`Self::instantiate_with_code`] but tailored towards /// being called by other code within the runtime as opposed to from an extrinsic. It returns - /// more information and allows the enablement of features that are not suitable for an - /// extrinsic (debugging, event collection). + /// more information to the caller useful to estimate the cost of the operation. pub fn bare_instantiate( origin: OriginFor, value: BalanceOf, @@ -1148,14 +1125,9 @@ where code: Code, data: Vec, salt: Option<[u8; 32]>, - debug: DebugInfo, - collect_events: CollectEvents, - ) -> ContractResult, EventRecordOf> { + ) -> ContractResult> { let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); - let mut debug_message = - if debug == DebugInfo::UnsafeDebug { Some(DebugBuffer::default()) } else { None }; - let unchecked_deposit_limit = storage_deposit_limit.is_unchecked(); let mut storage_deposit_limit = match storage_deposit_limit { DepositLimit::Balance(limit) => limit, @@ -1195,7 +1167,6 @@ where data, salt.as_ref(), unchecked_deposit_limit, - debug_message.as_mut(), ); storage_deposit = storage_meter .try_into_deposit(&instantiate_origin, unchecked_deposit_limit)? @@ -1203,11 +1174,6 @@ where result }; let output = Self::run_guarded(try_instantiate); - let events = if matches!(collect_events, CollectEvents::UnsafeCollect) { - Some(System::::read_events_no_consensus().map(|e| *e).collect()) - } else { - None - }; ContractResult { result: output .map(|(addr, result)| InstantiateReturnValue { result, addr }) @@ -1215,8 +1181,6 @@ where gas_consumed: gas_meter.gas_consumed(), gas_required: gas_meter.gas_required(), storage_deposit, - debug_message: debug_message.unwrap_or_default().to_vec(), - events, } } @@ -1273,8 +1237,6 @@ where }; let input = tx.input.clone().unwrap_or_default().0; - let debug = DebugInfo::Skip; - let collect_events = CollectEvents::Skip; let extract_error = |err| { if err == Error::::TransferFailed.into() || @@ -1305,8 +1267,6 @@ where gas_limit, storage_deposit_limit, input.clone(), - debug, - collect_events, ); let data = match result.result { @@ -1363,8 +1323,6 @@ where Code::Upload(code.to_vec()), data.to_vec(), None, - debug, - collect_events, ); let returned_data = match result.result { @@ -1535,12 +1493,11 @@ environmental!(executing_contract: bool); sp_api::decl_runtime_apis! { /// The API used to dry-run contract interactions. #[api_version(1)] - pub trait ReviveApi where + pub trait ReviveApi where AccountId: Codec, Balance: Codec, Nonce: Codec, BlockNumber: Codec, - EventRecord: Codec, { /// Returns the free balance of the given `[H160]` address, using EVM decimals. fn balance(address: H160) -> U256; @@ -1558,7 +1515,7 @@ sp_api::decl_runtime_apis! { gas_limit: Option, storage_deposit_limit: Option, input_data: Vec, - ) -> ContractResult; + ) -> ContractResult; /// Instantiate a new contract. /// @@ -1571,7 +1528,7 @@ sp_api::decl_runtime_apis! { code: Code, data: Vec, salt: Option<[u8; 32]>, - ) -> ContractResult; + ) -> ContractResult; /// Perform an Ethereum call. diff --git a/substrate/frame/revive/src/limits.rs b/substrate/frame/revive/src/limits.rs index 3b55106c67d8..f101abf0ea7e 100644 --- a/substrate/frame/revive/src/limits.rs +++ b/substrate/frame/revive/src/limits.rs @@ -57,11 +57,6 @@ pub const TRANSIENT_STORAGE_BYTES: u32 = 4 * 1024; /// The maximum allowable length in bytes for (transient) storage keys. pub const STORAGE_KEY_BYTES: u32 = 128; -/// The maximum size of the debug buffer contracts can write messages to. -/// -/// The buffer will always be disabled for on-chain execution. -pub const DEBUG_BUFFER_BYTES: u32 = 2 * 1024 * 1024; - /// The page size in which PolkaVM should allocate memory chunks. pub const PAGE_SIZE: u32 = 4 * 1024; diff --git a/substrate/frame/revive/src/primitives.rs b/substrate/frame/revive/src/primitives.rs index 452d2c8a3067..9c149c7cc389 100644 --- a/substrate/frame/revive/src/primitives.rs +++ b/substrate/frame/revive/src/primitives.rs @@ -63,7 +63,7 @@ impl From for DepositLimit { /// `ContractsApi` version. Therefore when SCALE decoding a `ContractResult` its trailing data /// should be ignored to avoid any potential compatibility issues. #[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug, TypeInfo)] -pub struct ContractResult { +pub struct ContractResult { /// How much weight was consumed during execution. pub gas_consumed: Weight, /// How much weight is required as gas limit in order to execute this call. @@ -84,26 +84,8 @@ pub struct ContractResult { /// is `Err`. This is because on error all storage changes are rolled back including the /// payment of the deposit. pub storage_deposit: StorageDeposit, - /// An optional debug message. This message is only filled when explicitly requested - /// by the code that calls into the contract. Otherwise it is empty. - /// - /// The contained bytes are valid UTF-8. This is not declared as `String` because - /// this type is not allowed within the runtime. - /// - /// Clients should not make any assumptions about the format of the buffer. - /// They should just display it as-is. It is **not** only a collection of log lines - /// provided by a contract but a formatted buffer with different sections. - /// - /// # Note - /// - /// The debug message is never generated during on-chain execution. It is reserved for - /// RPC calls. - pub debug_message: Vec, /// The execution result of the wasm code. pub result: Result, - /// The events that were emitted during execution. It is an option as event collection is - /// optional. - pub events: Option>, } /// The result of the execution of a `eth_transact` call. @@ -284,36 +266,3 @@ where } } } - -/// Determines whether events should be collected during execution. -#[derive( - Copy, Clone, PartialEq, Eq, RuntimeDebug, Decode, Encode, MaxEncodedLen, scale_info::TypeInfo, -)] -pub enum CollectEvents { - /// Collect events. - /// - /// # Note - /// - /// Events should only be collected when called off-chain, as this would otherwise - /// collect all the Events emitted in the block so far and put them into the PoV. - /// - /// **Never** use this mode for on-chain execution. - UnsafeCollect, - /// Skip event collection. - Skip, -} - -/// Determines whether debug messages will be collected. -#[derive( - Copy, Clone, PartialEq, Eq, RuntimeDebug, Decode, Encode, MaxEncodedLen, scale_info::TypeInfo, -)] -pub enum DebugInfo { - /// Collect debug messages. - /// # Note - /// - /// This should only ever be set to `UnsafeDebug` when executing as an RPC because - /// it adds allocations and could be abused to drive the runtime into an OOM panic. - UnsafeDebug, - /// Skip collection of debug messages. - Skip, -} diff --git a/substrate/frame/revive/src/test_utils/builder.rs b/substrate/frame/revive/src/test_utils/builder.rs index 8ba5e7384070..7fbb5b676439 100644 --- a/substrate/frame/revive/src/test_utils/builder.rs +++ b/substrate/frame/revive/src/test_utils/builder.rs @@ -17,9 +17,8 @@ use super::{deposit_limit, GAS_LIMIT}; use crate::{ - address::AddressMapper, AccountIdOf, BalanceOf, Code, CollectEvents, Config, ContractResult, - DebugInfo, DepositLimit, EventRecordOf, ExecReturnValue, InstantiateReturnValue, OriginFor, - Pallet, Weight, + address::AddressMapper, AccountIdOf, BalanceOf, Code, Config, ContractResult, DepositLimit, + ExecReturnValue, InstantiateReturnValue, OriginFor, Pallet, Weight, }; use frame_support::pallet_prelude::DispatchResultWithPostInfo; use paste::paste; @@ -138,9 +137,7 @@ builder!( code: Code, data: Vec, salt: Option<[u8; 32]>, - debug: DebugInfo, - collect_events: CollectEvents, - ) -> ContractResult, EventRecordOf>; + ) -> ContractResult>; /// Build the instantiate call and unwrap the result. pub fn build_and_unwrap_result(self) -> InstantiateReturnValue { @@ -164,8 +161,6 @@ builder!( code, data: vec![], salt: Some([0; 32]), - debug: DebugInfo::UnsafeDebug, - collect_events: CollectEvents::Skip, } } ); @@ -201,9 +196,7 @@ builder!( gas_limit: Weight, storage_deposit_limit: DepositLimit>, data: Vec, - debug: DebugInfo, - collect_events: CollectEvents, - ) -> ContractResult, EventRecordOf>; + ) -> ContractResult>; /// Build the call and unwrap the result. pub fn build_and_unwrap_result(self) -> ExecReturnValue { @@ -219,8 +212,6 @@ builder!( gas_limit: GAS_LIMIT, storage_deposit_limit: DepositLimit::Balance(deposit_limit::()), data: vec![], - debug: DebugInfo::UnsafeDebug, - collect_events: CollectEvents::Skip, } } ); diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index cf02d17a4d03..e2b30cf07c8d 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -38,9 +38,9 @@ use crate::{ tests::test_utils::{get_contract, get_contract_checked}, wasm::Memory, weights::WeightInfo, - AccountId32Mapper, BalanceOf, Code, CodeInfoOf, CollectEvents, Config, ContractInfo, - ContractInfoOf, DebugInfo, DeletionQueueCounter, DepositLimit, Error, EthTransactError, - HoldReason, Origin, Pallet, PristineCode, H160, + AccountId32Mapper, BalanceOf, Code, CodeInfoOf, Config, ContractInfo, ContractInfoOf, + DeletionQueueCounter, DepositLimit, Error, EthTransactError, HoldReason, Origin, Pallet, + PristineCode, H160, }; use crate::test_utils::builder::Contract; @@ -2184,58 +2184,6 @@ fn refcounter() { }); } -#[test] -fn debug_message_works() { - let (wasm, _code_hash) = compile_module("debug_message_works").unwrap(); - - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - let _ = ::Currency::set_balance(&ALICE, 1_000_000); - let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(wasm)) - .value(30_000) - .build_and_unwrap_contract(); - let result = builder::bare_call(addr).debug(DebugInfo::UnsafeDebug).build(); - - assert_matches!(result.result, Ok(_)); - assert_eq!(std::str::from_utf8(&result.debug_message).unwrap(), "Hello World!"); - }); -} - -#[test] -fn debug_message_logging_disabled() { - let (wasm, _code_hash) = compile_module("debug_message_logging_disabled").unwrap(); - - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - let _ = ::Currency::set_balance(&ALICE, 1_000_000); - let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(wasm)) - .value(30_000) - .build_and_unwrap_contract(); - // the dispatchables always run without debugging - assert_ok!(Contracts::call( - RuntimeOrigin::signed(ALICE), - addr, - 0, - GAS_LIMIT, - deposit_limit::(), - vec![] - )); - }); -} - -#[test] -fn debug_message_invalid_utf8() { - let (wasm, _code_hash) = compile_module("debug_message_invalid_utf8").unwrap(); - - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - let _ = ::Currency::set_balance(&ALICE, 1_000_000); - let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(wasm)) - .value(30_000) - .build_and_unwrap_contract(); - let result = builder::bare_call(addr).debug(DebugInfo::UnsafeDebug).build(); - assert_ok!(result.result); - assert!(result.debug_message.is_empty()); - }); -} - #[test] fn gas_estimation_for_subcalls() { let (caller_code, _caller_hash) = compile_module("call_with_limit").unwrap(); @@ -2451,79 +2399,6 @@ fn ecdsa_recover() { }) } -#[test] -fn bare_instantiate_returns_events() { - let (wasm, _code_hash) = compile_module("transfer_return_code").unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - let min_balance = Contracts::min_balance(); - let _ = ::Currency::set_balance(&ALICE, 1000 * min_balance); - - let result = builder::bare_instantiate(Code::Upload(wasm)) - .value(min_balance * 100) - .collect_events(CollectEvents::UnsafeCollect) - .build(); - - let events = result.events.unwrap(); - assert!(!events.is_empty()); - assert_eq!(events, System::events()); - }); -} - -#[test] -fn bare_instantiate_does_not_return_events() { - let (wasm, _code_hash) = compile_module("transfer_return_code").unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - let min_balance = Contracts::min_balance(); - let _ = ::Currency::set_balance(&ALICE, 1000 * min_balance); - - let result = builder::bare_instantiate(Code::Upload(wasm)).value(min_balance * 100).build(); - - let events = result.events; - assert!(!System::events().is_empty()); - assert!(events.is_none()); - }); -} - -#[test] -fn bare_call_returns_events() { - let (wasm, _code_hash) = compile_module("transfer_return_code").unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - let min_balance = Contracts::min_balance(); - let _ = ::Currency::set_balance(&ALICE, 1000 * min_balance); - - let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(wasm)) - .value(min_balance * 100) - .build_and_unwrap_contract(); - - let result = builder::bare_call(addr).collect_events(CollectEvents::UnsafeCollect).build(); - - let events = result.events.unwrap(); - assert_return_code!(&result.result.unwrap(), RuntimeReturnCode::Success); - assert!(!events.is_empty()); - assert_eq!(events, System::events()); - }); -} - -#[test] -fn bare_call_does_not_return_events() { - let (wasm, _code_hash) = compile_module("transfer_return_code").unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - let min_balance = Contracts::min_balance(); - let _ = ::Currency::set_balance(&ALICE, 1000 * min_balance); - - let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(wasm)) - .value(min_balance * 100) - .build_and_unwrap_contract(); - - let result = builder::bare_call(addr).build(); - - let events = result.events; - assert_return_code!(&result.result.unwrap(), RuntimeReturnCode::Success); - assert!(!System::events().is_empty()); - assert!(events.is_none()); - }); -} - #[test] fn sr25519_verify() { let (wasm, _code_hash) = compile_module("sr25519_verify").unwrap(); @@ -3327,14 +3202,11 @@ fn set_code_hash() { // First call sets new code_hash and returns 1 let result = builder::bare_call(contract_addr) .data(new_code_hash.as_ref().to_vec()) - .debug(DebugInfo::UnsafeDebug) .build_and_unwrap_result(); assert_return_code!(result, 1); // Second calls new contract code that returns 2 - let result = builder::bare_call(contract_addr) - .debug(DebugInfo::UnsafeDebug) - .build_and_unwrap_result(); + let result = builder::bare_call(contract_addr).build_and_unwrap_result(); assert_return_code!(result, 2); // Checking for the last event only @@ -4810,7 +4682,7 @@ fn skip_transfer_works() { ..Default::default() }, Weight::MAX, - |_| 0u32 + |_| 0u32, ), EthTransactError::Message(format!( "insufficient funds for gas * price + value: address {BOB_ADDR:?} have 0 (supplied gas 1)" @@ -4825,7 +4697,7 @@ fn skip_transfer_works() { ..Default::default() }, Weight::MAX, - |_| 0u32 + |_| 0u32, )); let Contract { addr, .. } = @@ -4844,7 +4716,7 @@ fn skip_transfer_works() { ..Default::default() }, Weight::MAX, - |_| 0u32 + |_| 0u32, ), EthTransactError::Message(format!( "insufficient funds for gas * price + value: address {BOB_ADDR:?} have 0 (supplied gas 1)" @@ -4869,7 +4741,7 @@ fn skip_transfer_works() { assert_ok!(Pallet::::bare_eth_transact( GenericTransaction { from: Some(BOB_ADDR), to: Some(addr), ..Default::default() }, Weight::MAX, - |_| 0u32 + |_| 0u32, )); // works when calling from a contract when no gas is specified. @@ -4881,7 +4753,7 @@ fn skip_transfer_works() { ..Default::default() }, Weight::MAX, - |_| 0u32 + |_| 0u32, )); }); } diff --git a/substrate/frame/revive/src/tests/test_debug.rs b/substrate/frame/revive/src/tests/test_debug.rs index c9e19e52ace1..b1fdb2d47441 100644 --- a/substrate/frame/revive/src/tests/test_debug.rs +++ b/substrate/frame/revive/src/tests/test_debug.rs @@ -119,8 +119,6 @@ fn debugging_works() { Code::Upload(wasm), vec![], Some([0u8; 32]), - DebugInfo::Skip, - CollectEvents::Skip, ) .result .unwrap() @@ -204,8 +202,6 @@ fn call_interception_works() { vec![], // some salt to ensure that the address of this contract is unique among all tests Some([0x41; 32]), - DebugInfo::Skip, - CollectEvents::Skip, ) .result .unwrap() diff --git a/substrate/frame/revive/src/wasm/mod.rs b/substrate/frame/revive/src/wasm/mod.rs index 3bd4bde5679f..b45d7026ba91 100644 --- a/substrate/frame/revive/src/wasm/mod.rs +++ b/substrate/frame/revive/src/wasm/mod.rs @@ -288,7 +288,7 @@ impl WasmBlob { } let engine = polkavm::Engine::new(&config).expect( "on-chain (no_std) use of interpreter is hard coded. - interpreter is available on all plattforms; qed", + interpreter is available on all platforms; qed", ); let mut module_config = polkavm::ModuleConfig::new(); diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 8529c7d9e73b..1ff6a80840a9 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -339,8 +339,6 @@ pub enum RuntimeCosts { Terminate(u32), /// Weight of calling `seal_deposit_event` with the given number of topics and event size. DepositEvent { num_topic: u32, len: u32 }, - /// Weight of calling `seal_debug_message` per byte of passed message. - DebugMessage(u32), /// Weight of calling `seal_set_storage` for the given storage item sizes. SetStorage { old_bytes: u32, new_bytes: u32 }, /// Weight of calling `seal_clear_storage` per cleared byte. @@ -489,7 +487,6 @@ impl Token for RuntimeCosts { WeightToFee => T::WeightInfo::seal_weight_to_fee(), Terminate(locked_dependencies) => T::WeightInfo::seal_terminate(locked_dependencies), DepositEvent { num_topic, len } => T::WeightInfo::seal_deposit_event(num_topic, len), - DebugMessage(len) => T::WeightInfo::seal_debug_message(len), SetStorage { new_bytes, old_bytes } => { cost_storage!(write, seal_set_storage, new_bytes, old_bytes) }, @@ -669,10 +666,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { match result { Ok(_) => Ok(ReturnErrorCode::Success), Err(e) => { - if self.ext.debug_buffer_enabled() { - self.ext.append_debug_buffer("call failed with: "); - self.ext.append_debug_buffer(e.into()); - }; + log::debug!(target: LOG_TARGET, "call failed with: {e:?}"); Ok(ErrorReturnCode::get()) }, } @@ -1832,27 +1826,6 @@ pub mod env { self.contains_storage(memory, flags, key_ptr, key_len) } - /// Emit a custom debug message. - /// See [`pallet_revive_uapi::HostFn::debug_message`]. - fn debug_message( - &mut self, - memory: &mut M, - str_ptr: u32, - str_len: u32, - ) -> Result { - let str_len = str_len.min(limits::DEBUG_BUFFER_BYTES); - self.charge_gas(RuntimeCosts::DebugMessage(str_len))?; - if self.ext.append_debug_buffer("") { - let data = memory.read(str_ptr, str_len)?; - if let Some(msg) = core::str::from_utf8(&data).ok() { - self.ext.append_debug_buffer(msg); - } - Ok(ReturnErrorCode::Success) - } else { - Ok(ReturnErrorCode::LoggingDisabled) - } - } - /// Recovers the ECDSA public key from the given message hash and signature. /// See [`pallet_revive_uapi::HostFn::ecdsa_recover`]. fn ecdsa_recover( @@ -2162,10 +2135,7 @@ pub mod env { Ok(ReturnErrorCode::Success) }, Err(e) => { - if self.ext.append_debug_buffer("") { - self.ext.append_debug_buffer("seal0::xcm_send failed with: "); - self.ext.append_debug_buffer(e.into()); - }; + log::debug!(target: LOG_TARGET, "seal0::xcm_send failed with: {e:?}"); Ok(ReturnErrorCode::XcmSendFailed) }, } diff --git a/substrate/frame/revive/src/weights.rs b/substrate/frame/revive/src/weights.rs index e35ba5ca0766..06495d5d21aa 100644 --- a/substrate/frame/revive/src/weights.rs +++ b/substrate/frame/revive/src/weights.rs @@ -96,7 +96,6 @@ pub trait WeightInfo { fn seal_return(n: u32, ) -> Weight; fn seal_terminate(n: u32, ) -> Weight; fn seal_deposit_event(t: u32, n: u32, ) -> Weight; - fn seal_debug_message(i: u32, ) -> Weight; fn get_storage_empty() -> Weight; fn get_storage_full() -> Weight; fn set_storage_empty() -> Weight; @@ -643,16 +642,6 @@ impl WeightInfo for SubstrateWeight { // Standard Error: 34 .saturating_add(Weight::from_parts(774, 0).saturating_mul(n.into())) } - /// The range of component `i` is `[0, 262144]`. - fn seal_debug_message(i: u32, ) -> Weight { - // Proof Size summary in bytes: - // Measured: `0` - // Estimated: `0` - // Minimum execution time: 340_000 picoseconds. - Weight::from_parts(306_527, 0) - // Standard Error: 1 - .saturating_add(Weight::from_parts(728, 0).saturating_mul(i.into())) - } /// Storage: `Skipped::Metadata` (r:0 w:0) /// Proof: `Skipped::Metadata` (`max_values`: None, `max_size`: None, mode: `Measured`) fn get_storage_empty() -> Weight { @@ -1539,16 +1528,6 @@ impl WeightInfo for () { // Standard Error: 34 .saturating_add(Weight::from_parts(774, 0).saturating_mul(n.into())) } - /// The range of component `i` is `[0, 262144]`. - fn seal_debug_message(i: u32, ) -> Weight { - // Proof Size summary in bytes: - // Measured: `0` - // Estimated: `0` - // Minimum execution time: 340_000 picoseconds. - Weight::from_parts(306_527, 0) - // Standard Error: 1 - .saturating_add(Weight::from_parts(728, 0).saturating_mul(i.into())) - } /// Storage: `Skipped::Metadata` (r:0 w:0) /// Proof: `Skipped::Metadata` (`max_values`: None, `max_size`: None, mode: `Measured`) fn get_storage_empty() -> Weight { diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index ba0a63b15c37..b82393826ddf 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -515,26 +515,6 @@ pub trait HostFn: private::Sealed { #[unstable_hostfn] fn contains_storage(flags: StorageFlags, key: &[u8]) -> Option; - /// Emit a custom debug message. - /// - /// No newlines are added to the supplied message. - /// Specifying invalid UTF-8 just drops the message with no trap. - /// - /// This is a no-op if debug message recording is disabled which is always the case - /// when the code is executing on-chain. The message is interpreted as UTF-8 and - /// appended to the debug buffer which is then supplied to the calling RPC client. - /// - /// # Note - /// - /// Even though no action is taken when debug message recording is disabled there is still - /// a non trivial overhead (and weight cost) associated with calling this function. Contract - /// languages should remove calls to this function (either at runtime or compile time) when - /// not being executed as an RPC. For example, they could allow users to disable logging - /// through compile time flags (cargo features) for on-chain deployment. Additionally, the - /// return value of this function can be cached in order to prevent further calls at runtime. - #[unstable_hostfn] - fn debug_message(str: &[u8]) -> Result; - /// Recovers the ECDSA public key from the given message hash and signature. /// /// Writes the public key into the given output buffer. diff --git a/substrate/frame/revive/uapi/src/host/riscv64.rs b/substrate/frame/revive/uapi/src/host/riscv64.rs index 8c40bc9f48ea..0023b8aa721d 100644 --- a/substrate/frame/revive/uapi/src/host/riscv64.rs +++ b/substrate/frame/revive/uapi/src/host/riscv64.rs @@ -109,7 +109,6 @@ mod sys { out_ptr: *mut u8, out_len_ptr: *mut u32, ) -> ReturnCode; - pub fn debug_message(str_ptr: *const u8, str_len: u32) -> ReturnCode; pub fn call_runtime(call_ptr: *const u8, call_len: u32) -> ReturnCode; pub fn ecdsa_recover( signature_ptr: *const u8, @@ -519,12 +518,6 @@ impl HostFn for HostFnImpl { ret_code.into() } - #[unstable_hostfn] - fn debug_message(str: &[u8]) -> Result { - let ret_code = unsafe { sys::debug_message(str.as_ptr(), str.len() as u32) }; - ret_code.into() - } - #[unstable_hostfn] fn ecdsa_recover( signature: &[u8; 65], diff --git a/substrate/frame/revive/uapi/src/lib.rs b/substrate/frame/revive/uapi/src/lib.rs index ef1798b4bf61..867f35633987 100644 --- a/substrate/frame/revive/uapi/src/lib.rs +++ b/substrate/frame/revive/uapi/src/lib.rs @@ -86,9 +86,8 @@ define_error_codes! { /// Transfer failed for other not further specified reason. Most probably /// reserved or locked balance of the sender that was preventing the transfer. TransferFailed = 4, - /// The call to `debug_message` had no effect because debug message - /// recording was disabled. - LoggingDisabled = 5, + /// The subcall ran out of weight or storage deposit. + OutOfResources = 5, /// The call dispatched by `call_runtime` was executed but returned an error. CallRuntimeFailed = 6, /// ECDSA public key recovery failed. Most probably wrong recovery id or signature. @@ -99,8 +98,6 @@ define_error_codes! { XcmExecutionFailed = 9, /// The `xcm_send` call failed. XcmSendFailed = 10, - /// The subcall ran out of weight or storage deposit. - OutOfResources = 11, } /// The raw return code returned by the host side. From 5be65872188a4ac1bf76333af3958b65f2a9629e Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Wed, 15 Jan 2025 20:23:54 +0100 Subject: [PATCH 3/6] [pallet-revive] Remove revive events (#7164) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove all pallet::events except for the `ContractEmitted` event that is emitted by contracts --------- Co-authored-by: command-bot <> Co-authored-by: Alexander Theißen --- prdoc/pr_7164.prdoc | 8 + substrate/frame/revive/src/exec.rs | 103 +------ substrate/frame/revive/src/lib.rs | 72 ----- substrate/frame/revive/src/storage/meter.rs | 16 +- substrate/frame/revive/src/tests.rs | 323 +------------------- substrate/frame/revive/src/wasm/mod.rs | 18 +- 6 files changed, 40 insertions(+), 500 deletions(-) create mode 100644 prdoc/pr_7164.prdoc diff --git a/prdoc/pr_7164.prdoc b/prdoc/pr_7164.prdoc new file mode 100644 index 000000000000..cb0410a9de79 --- /dev/null +++ b/prdoc/pr_7164.prdoc @@ -0,0 +1,8 @@ +title: '[pallet-revive] Remove revive events' +doc: +- audience: Runtime Dev + description: Remove all pallet::events except for the `ContractEmitted` event that + is emitted by contracts +crates: +- name: pallet-revive + bump: major diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index e20c5dd7786e..1c6ca435aefb 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1092,34 +1092,11 @@ where .enforce_limit(contract) .map_err(|e| ExecError { error: e, origin: ErrorOrigin::Callee })?; - let account_id = T::AddressMapper::to_address(&frame.account_id); - match (entry_point, delegated_code_hash) { - (ExportedFunction::Constructor, _) => { - // It is not allowed to terminate a contract inside its constructor. - if matches!(frame.contract_info, CachedContract::Terminated) { - return Err(Error::::TerminatedInConstructor.into()); - } - - let caller = T::AddressMapper::to_address(self.caller().account_id()?); - // Deposit an instantiation event. - Contracts::::deposit_event(Event::Instantiated { - deployer: caller, - contract: account_id, - }); - }, - (ExportedFunction::Call, Some(code_hash)) => { - Contracts::::deposit_event(Event::DelegateCalled { - contract: account_id, - code_hash, - }); - }, - (ExportedFunction::Call, None) => { - let caller = self.caller(); - Contracts::::deposit_event(Event::Called { - caller: caller.clone(), - contract: account_id, - }); - }, + // It is not allowed to terminate a contract inside its constructor. + if entry_point == ExportedFunction::Constructor && + matches!(frame.contract_info, CachedContract::Terminated) + { + return Err(Error::::TerminatedInConstructor.into()); } Ok(output) @@ -1526,10 +1503,6 @@ where .charge_deposit(frame.account_id.clone(), StorageDeposit::Refund(*deposit)); } - Contracts::::deposit_event(Event::Terminated { - contract: account_address, - beneficiary: *beneficiary, - }); Ok(()) } @@ -1782,11 +1755,6 @@ where Self::increment_refcount(hash)?; Self::decrement_refcount(prev_hash); - Contracts::::deposit_event(Event::ContractCodeUpdated { - contract: T::AddressMapper::to_address(&frame.account_id), - new_code_hash: hash, - old_code_hash: prev_hash, - }); Ok(()) } @@ -2933,13 +2901,6 @@ mod tests { ContractInfo::::load_code_hash(&instantiated_contract_id).unwrap(), dummy_ch ); - assert_eq!( - &events(), - &[Event::Instantiated { - deployer: ALICE_ADDR, - contract: instantiated_contract_address - }] - ); }); } @@ -3055,19 +3016,6 @@ mod tests { ContractInfo::::load_code_hash(&instantiated_contract_id).unwrap(), dummy_ch ); - assert_eq!( - &events(), - &[ - Event::Instantiated { - deployer: BOB_ADDR, - contract: instantiated_contract_address - }, - Event::Called { - caller: Origin::from_account_id(ALICE), - contract: BOB_ADDR - }, - ] - ); }); } @@ -3119,13 +3067,6 @@ mod tests { ), Ok(_) ); - - // The contract wasn't instantiated so we don't expect to see an instantiation - // event here. - assert_eq!( - &events(), - &[Event::Called { caller: Origin::from_account_id(ALICE), contract: BOB_ADDR },] - ); }); } @@ -3465,24 +3406,14 @@ mod tests { let remark_hash = ::Hashing::hash(b"Hello World"); assert_eq!( System::events(), - vec![ - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::System(frame_system::Event::Remarked { - sender: BOB_FALLBACK, - hash: remark_hash - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: BOB_ADDR, - }), - topics: vec![], - }, - ] + vec![EventRecord { + phase: Phase::Initialization, + event: MetaEvent::System(frame_system::Event::Remarked { + sender: BOB_FALLBACK, + hash: remark_hash + }), + topics: vec![], + },] ); }); } @@ -3571,14 +3502,6 @@ mod tests { },), topics: vec![], }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: BOB_ADDR, - }), - topics: vec![], - }, ] ); }); diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 403598ae136e..a9f2842c35f6 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -379,25 +379,6 @@ pub mod pallet { #[pallet::event] pub enum Event { - /// Contract deployed by address at the specified address. - Instantiated { deployer: H160, contract: H160 }, - - /// Contract has been removed. - /// - /// # Note - /// - /// The only way for a contract to be removed and emitting this event is by calling - /// `seal_terminate`. - Terminated { - /// The contract that was terminated. - contract: H160, - /// The account that received the contracts remaining balance - beneficiary: H160, - }, - - /// Code with the specified hash has been stored. - CodeStored { code_hash: H256, deposit_held: BalanceOf, uploader: H160 }, - /// A custom event emitted by the contract. ContractEmitted { /// The contract that emitted the event. @@ -409,54 +390,6 @@ pub mod pallet { /// Number of topics is capped by [`limits::NUM_EVENT_TOPICS`]. topics: Vec, }, - - /// A code with the specified hash was removed. - CodeRemoved { code_hash: H256, deposit_released: BalanceOf, remover: H160 }, - - /// A contract's code was updated. - ContractCodeUpdated { - /// The contract that has been updated. - contract: H160, - /// New code hash that was set for the contract. - new_code_hash: H256, - /// Previous code hash of the contract. - old_code_hash: H256, - }, - - /// A contract was called either by a plain account or another contract. - /// - /// # Note - /// - /// Please keep in mind that like all events this is only emitted for successful - /// calls. This is because on failure all storage changes including events are - /// rolled back. - Called { - /// The caller of the `contract`. - caller: Origin, - /// The contract that was called. - contract: H160, - }, - - /// A contract delegate called a code hash. - /// - /// # Note - /// - /// Please keep in mind that like all events this is only emitted for successful - /// calls. This is because on failure all storage changes including events are - /// rolled back. - DelegateCalled { - /// The contract that performed the delegate call and hence in whose context - /// the `code_hash` is executed. - contract: H160, - /// The code hash that was delegate called. - code_hash: H256, - }, - - /// Some funds have been transferred and held as storage deposit. - StorageDepositTransferredAndHeld { from: H160, to: H160, amount: BalanceOf }, - - /// Some storage deposit funds have been transferred and released. - StorageDepositTransferredAndReleased { from: H160, to: H160, amount: BalanceOf }, } #[pallet::error] @@ -985,11 +918,6 @@ pub mod pallet { }; >>::increment_refcount(code_hash)?; >>::decrement_refcount(contract.code_hash); - Self::deposit_event(Event::ContractCodeUpdated { - contract: dest, - new_code_hash: code_hash, - old_code_hash: contract.code_hash, - }); contract.code_hash = code_hash; Ok(()) }) diff --git a/substrate/frame/revive/src/storage/meter.rs b/substrate/frame/revive/src/storage/meter.rs index 4febcb0c4066..cd390c86f63a 100644 --- a/substrate/frame/revive/src/storage/meter.rs +++ b/substrate/frame/revive/src/storage/meter.rs @@ -18,8 +18,8 @@ //! This module contains functions to meter the storage deposit. use crate::{ - address::AddressMapper, storage::ContractInfo, AccountIdOf, BalanceOf, CodeInfo, Config, Error, - Event, HoldReason, Inspect, Origin, Pallet, StorageDeposit as Deposit, System, LOG_TARGET, + storage::ContractInfo, AccountIdOf, BalanceOf, CodeInfo, Config, Error, HoldReason, Inspect, + Origin, Pallet, StorageDeposit as Deposit, System, LOG_TARGET, }; use alloc::vec::Vec; use core::{fmt::Debug, marker::PhantomData}; @@ -516,12 +516,6 @@ impl Ext for ReservingExt { Preservation::Preserve, Fortitude::Polite, )?; - - Pallet::::deposit_event(Event::StorageDepositTransferredAndHeld { - from: T::AddressMapper::to_address(origin), - to: T::AddressMapper::to_address(contract), - amount: *amount, - }); }, Deposit::Refund(amount) => { let transferred = T::Currency::transfer_on_hold( @@ -534,12 +528,6 @@ impl Ext for ReservingExt { Fortitude::Polite, )?; - Pallet::::deposit_event(Event::StorageDepositTransferredAndReleased { - from: T::AddressMapper::to_address(contract), - to: T::AddressMapper::to_address(origin), - amount: transferred, - }); - if transferred < *amount { // This should never happen, if it does it means that there is a bug in the // runtime logic. In the rare case this happens we try to refund as much as we diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index e2b30cf07c8d..35940f544d00 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -713,25 +713,6 @@ fn instantiate_and_call_and_deposit_event() { }), topics: vec![], }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Instantiated { - deployer: ALICE_ADDR, - contract: addr - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts( - pallet_revive::Event::StorageDepositTransferredAndHeld { - from: ALICE_ADDR, - to: addr, - amount: test_utils::contract_info_storage_deposit(&addr), - } - ), - topics: vec![], - }, ] ); }); @@ -1078,14 +1059,6 @@ fn deploy_and_call_other_contract() { }), topics: vec![], }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Instantiated { - deployer: caller_addr, - contract: callee_addr, - }), - topics: vec![], - }, EventRecord { phase: Phase::Initialization, event: RuntimeEvent::Balances(pallet_balances::Event::Transfer { @@ -1095,33 +1068,6 @@ fn deploy_and_call_other_contract() { }), topics: vec![], }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(caller_account.clone()), - contract: callee_addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: caller_addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts( - pallet_revive::Event::StorageDepositTransferredAndHeld { - from: ALICE_ADDR, - to: callee_addr, - amount: test_utils::contract_info_storage_deposit(&callee_addr), - } - ), - topics: vec![], - }, ] ); }); @@ -1373,8 +1319,6 @@ fn self_destruct_works() { // Check that the BOB contract has been instantiated. let _ = get_contract(&contract.addr); - let info_deposit = test_utils::contract_info_storage_deposit(&contract.addr); - // Drop all previous events initialize_block(2); @@ -1404,33 +1348,6 @@ fn self_destruct_works() { pretty_assertions::assert_eq!( System::events(), vec![ - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Terminated { - contract: contract.addr, - beneficiary: DJANGO_ADDR, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: contract.addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts( - pallet_revive::Event::StorageDepositTransferredAndReleased { - from: contract.addr, - to: ALICE_ADDR, - amount: info_deposit, - } - ), - topics: vec![], - }, EventRecord { phase: Phase::Initialization, event: RuntimeEvent::System(frame_system::Event::KilledAccount { @@ -2512,23 +2429,9 @@ fn upload_code_works() { initialize_block(2); assert!(!PristineCode::::contains_key(&code_hash)); - assert_ok!(Contracts::upload_code(RuntimeOrigin::signed(ALICE), wasm, 1_000,)); // Ensure the contract was stored and get expected deposit amount to be reserved. - let deposit_expected = expected_deposit(ensure_stored(code_hash)); - - assert_eq!( - System::events(), - vec![EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::CodeStored { - code_hash, - deposit_held: deposit_expected, - uploader: ALICE_ADDR - }), - topics: vec![], - },] - ); + expected_deposit(ensure_stored(code_hash)); }); } @@ -2586,32 +2489,8 @@ fn remove_code_works() { assert_ok!(Contracts::upload_code(RuntimeOrigin::signed(ALICE), wasm, 1_000,)); // Ensure the contract was stored and get expected deposit amount to be reserved. - let deposit_expected = expected_deposit(ensure_stored(code_hash)); - + expected_deposit(ensure_stored(code_hash)); assert_ok!(Contracts::remove_code(RuntimeOrigin::signed(ALICE), code_hash)); - assert_eq!( - System::events(), - vec![ - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::CodeStored { - code_hash, - deposit_held: deposit_expected, - uploader: ALICE_ADDR - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::CodeRemoved { - code_hash, - deposit_released: deposit_expected, - remover: ALICE_ADDR - }), - topics: vec![], - }, - ] - ); }); } @@ -2627,25 +2506,12 @@ fn remove_code_wrong_origin() { assert_ok!(Contracts::upload_code(RuntimeOrigin::signed(ALICE), wasm, 1_000,)); // Ensure the contract was stored and get expected deposit amount to be reserved. - let deposit_expected = expected_deposit(ensure_stored(code_hash)); + expected_deposit(ensure_stored(code_hash)); assert_noop!( Contracts::remove_code(RuntimeOrigin::signed(BOB), code_hash), sp_runtime::traits::BadOrigin, ); - - assert_eq!( - System::events(), - vec![EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::CodeStored { - code_hash, - deposit_held: deposit_expected, - uploader: ALICE_ADDR - }), - topics: vec![], - },] - ); }); } @@ -2704,7 +2570,7 @@ fn instantiate_with_zero_balance_works() { builder::bare_instantiate(Code::Upload(wasm)).build_and_unwrap_contract(); // Ensure the contract was stored and get expected deposit amount to be reserved. - let deposit_expected = expected_deposit(ensure_stored(code_hash)); + expected_deposit(ensure_stored(code_hash)); // Make sure the account exists even though no free balance was send assert_eq!(::Currency::free_balance(&account_id), min_balance); @@ -2716,15 +2582,6 @@ fn instantiate_with_zero_balance_works() { assert_eq!( System::events(), vec![ - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::CodeStored { - code_hash, - deposit_held: deposit_expected, - uploader: ALICE_ADDR - }), - topics: vec![], - }, EventRecord { phase: Phase::Initialization, event: RuntimeEvent::System(frame_system::Event::NewAccount { @@ -2749,25 +2606,6 @@ fn instantiate_with_zero_balance_works() { }), topics: vec![], }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Instantiated { - deployer: ALICE_ADDR, - contract: addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts( - pallet_revive::Event::StorageDepositTransferredAndHeld { - from: ALICE_ADDR, - to: addr, - amount: test_utils::contract_info_storage_deposit(&addr), - } - ), - topics: vec![], - }, ] ); }); @@ -2790,7 +2628,7 @@ fn instantiate_with_below_existential_deposit_works() { .build_and_unwrap_contract(); // Ensure the contract was stored and get expected deposit amount to be reserved. - let deposit_expected = expected_deposit(ensure_stored(code_hash)); + expected_deposit(ensure_stored(code_hash)); // Make sure the account exists even though not enough free balance was send assert_eq!(::Currency::free_balance(&account_id), min_balance + value); assert_eq!( @@ -2801,15 +2639,6 @@ fn instantiate_with_below_existential_deposit_works() { assert_eq!( System::events(), vec![ - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::CodeStored { - code_hash, - deposit_held: deposit_expected, - uploader: ALICE_ADDR - }), - topics: vec![], - }, EventRecord { phase: Phase::Initialization, event: RuntimeEvent::System(frame_system::Event::NewAccount { @@ -2843,25 +2672,6 @@ fn instantiate_with_below_existential_deposit_works() { }), topics: vec![], }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Instantiated { - deployer: ALICE_ADDR, - contract: addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts( - pallet_revive::Event::StorageDepositTransferredAndHeld { - from: ALICE_ADDR, - to: addr, - amount: test_utils::contract_info_storage_deposit(&addr), - } - ), - topics: vec![], - }, ] ); }); @@ -2903,74 +2713,15 @@ fn storage_deposit_works() { assert_eq!( System::events(), - vec![ - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Balances(pallet_balances::Event::Transfer { - from: ALICE, - to: account_id.clone(), - amount: 42, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts( - pallet_revive::Event::StorageDepositTransferredAndHeld { - from: ALICE_ADDR, - to: addr, - amount: charged0, - } - ), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts( - pallet_revive::Event::StorageDepositTransferredAndHeld { - from: ALICE_ADDR, - to: addr, - amount: charged1, - } - ), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts( - pallet_revive::Event::StorageDepositTransferredAndReleased { - from: addr, - to: ALICE_ADDR, - amount: refunded0, - } - ), - topics: vec![], - }, - ] + vec![EventRecord { + phase: Phase::Initialization, + event: RuntimeEvent::Balances(pallet_balances::Event::Transfer { + from: ALICE, + to: account_id.clone(), + amount: 42, + }), + topics: vec![], + },] ); }); } @@ -3063,18 +2814,6 @@ fn set_code_extrinsic() { assert_eq!(get_contract(&addr).code_hash, new_code_hash); assert_refcount!(&code_hash, 0); assert_refcount!(&new_code_hash, 1); - assert_eq!( - System::events(), - vec![EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(pallet_revive::Event::ContractCodeUpdated { - contract: addr, - new_code_hash, - old_code_hash: code_hash, - }), - topics: vec![], - },] - ); }); } @@ -3180,7 +2919,7 @@ fn contract_reverted() { #[test] fn set_code_hash() { - let (wasm, code_hash) = compile_module("set_code_hash").unwrap(); + let (wasm, _) = compile_module("set_code_hash").unwrap(); let (new_wasm, new_code_hash) = compile_module("new_set_code_hash_contract").unwrap(); ExtBuilder::default().existential_deposit(100).build().execute_with(|| { @@ -3208,38 +2947,6 @@ fn set_code_hash() { // Second calls new contract code that returns 2 let result = builder::bare_call(contract_addr).build_and_unwrap_result(); assert_return_code!(result, 2); - - // Checking for the last event only - assert_eq!( - &System::events(), - &[ - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::ContractCodeUpdated { - contract: contract_addr, - new_code_hash, - old_code_hash: code_hash, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: contract_addr, - }), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: RuntimeEvent::Contracts(crate::Event::Called { - caller: Origin::from_account_id(ALICE), - contract: contract_addr, - }), - topics: vec![], - }, - ], - ); }); } diff --git a/substrate/frame/revive/src/wasm/mod.rs b/substrate/frame/revive/src/wasm/mod.rs index b45d7026ba91..527cf1630954 100644 --- a/substrate/frame/revive/src/wasm/mod.rs +++ b/substrate/frame/revive/src/wasm/mod.rs @@ -29,14 +29,13 @@ pub use crate::wasm::runtime::{ReturnData, TrapReason}; pub use crate::wasm::runtime::{Memory, Runtime, RuntimeCosts}; use crate::{ - address::AddressMapper, exec::{ExecResult, Executable, ExportedFunction, Ext}, gas::{GasMeter, Token}, limits, storage::meter::Diff, weights::WeightInfo, - AccountIdOf, BadOrigin, BalanceOf, CodeInfoOf, CodeVec, Config, Error, Event, ExecError, - HoldReason, Pallet, PristineCode, Weight, LOG_TARGET, + AccountIdOf, BadOrigin, BalanceOf, CodeInfoOf, CodeVec, Config, Error, ExecError, HoldReason, + PristineCode, Weight, LOG_TARGET, }; use alloc::vec::Vec; use codec::{Decode, Encode, MaxEncodedLen}; @@ -157,16 +156,9 @@ where code_info.deposit, BestEffort, ); - let deposit_released = code_info.deposit; - let remover = T::AddressMapper::to_address(&code_info.owner); *existing = None; >::remove(&code_hash); - >::deposit_event(Event::CodeRemoved { - code_hash, - deposit_released, - remover, - }); Ok(()) } else { Err(>::CodeNotFound.into()) @@ -202,12 +194,6 @@ where self.code_info.refcount = 0; >::insert(code_hash, &self.code); *stored_code_info = Some(self.code_info.clone()); - let uploader = T::AddressMapper::to_address(&self.code_info.owner); - >::deposit_event(Event::CodeStored { - code_hash, - deposit_held: deposit, - uploader, - }); Ok(deposit) }, } From 412aca6c48a01f11318228f4d8a79fec544a22bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20M=C3=BCller?= Date: Wed, 15 Jan 2025 22:58:51 +0100 Subject: [PATCH 4/6] [pallet-revive] Add host function `to_account_id` (#7091) Closes https://github.com/paritytech/polkadot-sdk/issues/6891. cc @athei @xermicus @pgherveou --- prdoc/pr_7091.prdoc | 12 ++++++ .../fixtures/contracts/to_account_id.rs | 40 ++++++++++++++++++ .../frame/revive/src/benchmarking/mod.rs | 32 +++++++++++++++ substrate/frame/revive/src/exec.rs | 41 +++++++++++++++++++ substrate/frame/revive/src/tests.rs | 37 +++++++++++++++++ substrate/frame/revive/src/wasm/runtime.rs | 24 +++++++++++ substrate/frame/revive/src/weights.rs | 21 ++++++++++ substrate/frame/revive/uapi/src/host.rs | 12 ++++++ .../frame/revive/uapi/src/host/riscv64.rs | 6 +++ 9 files changed, 225 insertions(+) create mode 100644 prdoc/pr_7091.prdoc create mode 100644 substrate/frame/revive/fixtures/contracts/to_account_id.rs diff --git a/prdoc/pr_7091.prdoc b/prdoc/pr_7091.prdoc new file mode 100644 index 000000000000..badea4e82fdb --- /dev/null +++ b/prdoc/pr_7091.prdoc @@ -0,0 +1,12 @@ +title: '[pallet-revive] Add new host function `to_account_id`' +doc: +- audience: Runtime Dev + description: A new host function `to_account_id` is added. It allows retrieving + the account id for a `H160` address. +crates: +- name: pallet-revive-fixtures + bump: minor +- name: pallet-revive + bump: minor +- name: pallet-revive-uapi + bump: minor diff --git a/substrate/frame/revive/fixtures/contracts/to_account_id.rs b/substrate/frame/revive/fixtures/contracts/to_account_id.rs new file mode 100644 index 000000000000..c2a8fce3ec99 --- /dev/null +++ b/substrate/frame/revive/fixtures/contracts/to_account_id.rs @@ -0,0 +1,40 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#![no_std] +#![no_main] + +use common::input; +use uapi::{HostFn, HostFnImpl as api}; + +#[no_mangle] +#[polkavm_derive::polkavm_export] +pub extern "C" fn deploy() {} + +#[no_mangle] +#[polkavm_derive::polkavm_export] +pub extern "C" fn call() { + input!( + address: &[u8; 20], + expected_account_id: &[u8; 32], + ); + + let mut account_id = [0u8; 32]; + api::to_account_id(address, &mut account_id); + + assert!(&account_id == expected_account_id); +} diff --git a/substrate/frame/revive/src/benchmarking/mod.rs b/substrate/frame/revive/src/benchmarking/mod.rs index e23554f21ba8..18d7bb0afc31 100644 --- a/substrate/frame/revive/src/benchmarking/mod.rs +++ b/substrate/frame/revive/src/benchmarking/mod.rs @@ -556,6 +556,38 @@ mod benchmarks { assert_eq!(result.unwrap(), 1); } + #[benchmark(pov_mode = Measured)] + fn seal_to_account_id() { + // use a mapped address for the benchmark, to ensure that we bench the worst + // case (and not the fallback case). + let address = { + let caller = account("seal_to_account_id", 0, 0); + T::Currency::set_balance(&caller, caller_funding::()); + T::AddressMapper::map(&caller).unwrap(); + T::AddressMapper::to_address(&caller) + }; + + let len = ::max_encoded_len(); + build_runtime!(runtime, memory: [vec![0u8; len], address.0, ]); + + let result; + #[block] + { + result = runtime.bench_to_account_id(memory.as_mut_slice(), len as u32, 0); + } + + assert_ok!(result); + assert_ne!( + memory.as_slice()[20..32], + [0xEE; 12], + "fallback suffix found where none should be" + ); + assert_eq!( + T::AccountId::decode(&mut memory.as_slice()), + Ok(runtime.ext().to_account_id(&address)) + ); + } + #[benchmark(pov_mode = Measured)] fn seal_code_hash() { let contract = Contract::::with_index(1, WasmModule::dummy(), vec![]).unwrap(); diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 1c6ca435aefb..f696f75a4a13 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -293,6 +293,9 @@ pub trait Ext: sealing::Sealed { /// Check if a contract lives at the specified `address`. fn is_contract(&self, address: &H160) -> bool; + /// Returns the account id for the given `address`. + fn to_account_id(&self, address: &H160) -> AccountIdOf; + /// Returns the code hash of the contract for the given `address`. /// If not a contract but account exists then `keccak_256([])` is returned, otherwise `zero`. fn code_hash(&self, address: &H160) -> H256; @@ -1572,6 +1575,10 @@ where ContractInfoOf::::contains_key(&address) } + fn to_account_id(&self, address: &H160) -> T::AccountId { + T::AddressMapper::to_account_id(address) + } + fn code_hash(&self, address: &H160) -> H256 { >::get(&address) .map(|contract| contract.code_hash) @@ -2582,6 +2589,40 @@ mod tests { }); } + #[test] + fn to_account_id_returns_proper_values() { + let bob_code_hash = MockLoader::insert(Call, |ctx, _| { + let alice_account_id = ::AddressMapper::to_account_id(&ALICE_ADDR); + assert_eq!(ctx.ext.to_account_id(&ALICE_ADDR), alice_account_id); + + const UNMAPPED_ADDR: H160 = H160([99u8; 20]); + let mut unmapped_fallback_account_id = [0xEE; 32]; + unmapped_fallback_account_id[..20].copy_from_slice(UNMAPPED_ADDR.as_bytes()); + assert_eq!( + ctx.ext.to_account_id(&UNMAPPED_ADDR), + AccountId32::new(unmapped_fallback_account_id) + ); + + exec_success() + }); + + ExtBuilder::default().build().execute_with(|| { + place_contract(&BOB, bob_code_hash); + let origin = Origin::from_account_id(ALICE); + let mut storage_meter = storage::meter::Meter::new(&origin, 0, 0).unwrap(); + let result = MockStack::run_call( + origin, + BOB_ADDR, + &mut GasMeter::::new(GAS_LIMIT), + &mut storage_meter, + U256::zero(), + vec![0], + false, + ); + assert_matches!(result, Ok(_)); + }); + } + #[test] fn code_hash_returns_proper_values() { let bob_code_hash = MockLoader::insert(Call, |ctx, _| { diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 35940f544d00..8398bc2cb66f 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4239,6 +4239,43 @@ fn origin_api_works() { }); } +#[test] +fn to_account_id_works() { + let (code_hash_code, _) = compile_module("to_account_id").unwrap(); + + ExtBuilder::default().existential_deposit(1).build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 1_000_000); + let _ = ::Currency::set_balance(&EVE, 1_000_000); + + let Contract { addr, .. } = + builder::bare_instantiate(Code::Upload(code_hash_code)).build_and_unwrap_contract(); + + // mapped account + >::map_account(RuntimeOrigin::signed(EVE)).unwrap(); + let expected_mapped_account_id = &::AddressMapper::to_account_id(&EVE_ADDR); + assert_ne!( + expected_mapped_account_id.encode()[20..32], + [0xEE; 12], + "fallback suffix found where none should be" + ); + assert_ok!(builder::call(addr) + .data((EVE_ADDR, expected_mapped_account_id).encode()) + .build()); + + // fallback for unmapped accounts + let expected_fallback_account_id = + &::AddressMapper::to_account_id(&BOB_ADDR); + assert_eq!( + expected_fallback_account_id.encode()[20..32], + [0xEE; 12], + "no fallback suffix found where one should be" + ); + assert_ok!(builder::call(addr) + .data((BOB_ADDR, expected_fallback_account_id).encode()) + .build()); + }); +} + #[test] fn code_hash_works() { let (code_hash_code, self_code_hash) = compile_module("code_hash").unwrap(); diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 1ff6a80840a9..4fbcfe1b47f5 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -293,6 +293,8 @@ pub enum RuntimeCosts { CallDataSize, /// Weight of calling `seal_return_data_size`. ReturnDataSize, + /// Weight of calling `seal_to_account_id`. + ToAccountId, /// Weight of calling `seal_origin`. Origin, /// Weight of calling `seal_is_contract`. @@ -466,6 +468,7 @@ impl Token for RuntimeCosts { Caller => T::WeightInfo::seal_caller(), Origin => T::WeightInfo::seal_origin(), IsContract => T::WeightInfo::seal_is_contract(), + ToAccountId => T::WeightInfo::seal_to_account_id(), CodeHash => T::WeightInfo::seal_code_hash(), CodeSize => T::WeightInfo::seal_code_size(), OwnCodeHash => T::WeightInfo::seal_own_code_hash(), @@ -2140,4 +2143,25 @@ pub mod env { }, } } + + /// Retrieves the account id for a specified contract address. + /// + /// See [`pallet_revive_uapi::HostFn::to_account_id`]. + fn to_account_id( + &mut self, + memory: &mut M, + addr_ptr: u32, + out_ptr: u32, + ) -> Result<(), TrapReason> { + self.charge_gas(RuntimeCosts::ToAccountId)?; + let address = memory.read_h160(addr_ptr)?; + let account_id = self.ext.to_account_id(&address); + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + &account_id.encode(), + false, + already_charged, + )?) + } } diff --git a/substrate/frame/revive/src/weights.rs b/substrate/frame/revive/src/weights.rs index 06495d5d21aa..52153d74ca75 100644 --- a/substrate/frame/revive/src/weights.rs +++ b/substrate/frame/revive/src/weights.rs @@ -67,6 +67,7 @@ pub trait WeightInfo { fn seal_caller() -> Weight; fn seal_origin() -> Weight; fn seal_is_contract() -> Weight; + fn seal_to_account_id() -> Weight; fn seal_code_hash() -> Weight; fn seal_own_code_hash() -> Weight; fn seal_code_size() -> Weight; @@ -377,6 +378,16 @@ impl WeightInfo for SubstrateWeight { Weight::from_parts(10_336_000, 3771) .saturating_add(T::DbWeight::get().reads(1_u64)) } + /// Storage: `Revive::AddressSuffix` (r:1 w:0) + /// Proof: `Revive::AddressSuffix` (`max_values`: None, `max_size`: Some(32), added: 2507, mode: `Measured`) + fn seal_to_account_id() -> Weight { + // Proof Size summary in bytes: + // Measured: `212` + // Estimated: `3677` + // Minimum execution time: 4_000_000 picoseconds. + Weight::from_parts(4_000_000, 3677) + .saturating_add(T::DbWeight::get().reads(1_u64)) + } /// Storage: `Revive::ContractInfoOf` (r:1 w:0) /// Proof: `Revive::ContractInfoOf` (`max_values`: None, `max_size`: Some(1779), added: 4254, mode: `Measured`) fn seal_code_hash() -> Weight { @@ -1263,6 +1274,16 @@ impl WeightInfo for () { Weight::from_parts(10_336_000, 3771) .saturating_add(RocksDbWeight::get().reads(1_u64)) } + /// Storage: `Revive::AddressSuffix` (r:1 w:0) + /// Proof: `Revive::AddressSuffix` (`max_values`: None, `max_size`: Some(32), added: 2507, mode: `Measured`) + fn seal_to_account_id() -> Weight { + // Proof Size summary in bytes: + // Measured: `212` + // Estimated: `3677` + // Minimum execution time: 4_000_000 picoseconds. + Weight::from_parts(4_000_000, 3677) + .saturating_add(RocksDbWeight::get().reads(1_u64)) + } /// Storage: `Revive::ContractInfoOf` (r:1 w:0) /// Proof: `Revive::ContractInfoOf` (`max_values`: None, `max_size`: Some(1779), added: 4254, mode: `Measured`) fn seal_code_hash() -> Weight { diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index b82393826ddf..3e5cf0eb0c24 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -144,6 +144,18 @@ pub trait HostFn: private::Sealed { /// - `output`: A reference to the output data buffer to write the origin's address. fn origin(output: &mut [u8; 20]); + /// Retrieve the account id for a specified address. + /// + /// # Parameters + /// + /// - `addr`: A `H160` address. + /// - `output`: A reference to the output data buffer to write the account id. + /// + /// # Note + /// + /// If no mapping exists for `addr`, the fallback account id will be returned. + fn to_account_id(addr: &[u8; 20], output: &mut [u8]); + /// Retrieve the code hash for a specified contract address. /// /// # Parameters diff --git a/substrate/frame/revive/uapi/src/host/riscv64.rs b/substrate/frame/revive/uapi/src/host/riscv64.rs index 0023b8aa721d..3726564e26eb 100644 --- a/substrate/frame/revive/uapi/src/host/riscv64.rs +++ b/substrate/frame/revive/uapi/src/host/riscv64.rs @@ -69,6 +69,7 @@ mod sys { pub fn caller(out_ptr: *mut u8); pub fn origin(out_ptr: *mut u8); pub fn is_contract(account_ptr: *const u8) -> ReturnCode; + pub fn to_account_id(address_ptr: *const u8, out_ptr: *mut u8); pub fn code_hash(address_ptr: *const u8, out_ptr: *mut u8); pub fn code_size(address_ptr: *const u8) -> u64; pub fn own_code_hash(out_ptr: *mut u8); @@ -456,6 +457,11 @@ impl HostFn for HostFnImpl { unsafe { sys::ref_time_left() } } + #[unstable_hostfn] + fn to_account_id(address: &[u8; 20], output: &mut [u8]) { + unsafe { sys::to_account_id(address.as_ptr(), output.as_mut_ptr()) } + } + #[unstable_hostfn] fn block_hash(block_number_ptr: &[u8; 32], output: &mut [u8; 32]) { unsafe { sys::block_hash(block_number_ptr.as_ptr(), output.as_mut_ptr()) }; From 2b496e52446cefde67ba28f7136c1008c1aa41c0 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 16 Jan 2025 10:14:49 +0100 Subject: [PATCH 5/6] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 34790eebde43..4f2f6202877e 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -161,7 +161,7 @@ impl DenyExecution for Tuple { let barrier = core::any::type_name::(); match Tuple::deny_execution(origin, instructions, max_weight, properties) { Some(error) => { - tracing::trace!( + tracing::error!( target: "xcm::should_execute", ?origin, ?instructions, From 81ec7f1161fd0651df3ecfe470e600d7ab4741ac Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 16 Jan 2025 10:15:09 +0100 Subject: [PATCH 6/6] Update polkadot/xcm/xcm-executor/src/traits/should_execute.rs --- polkadot/xcm/xcm-executor/src/traits/should_execute.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs index 4f2f6202877e..2e3330c229f1 100644 --- a/polkadot/xcm/xcm-executor/src/traits/should_execute.rs +++ b/polkadot/xcm/xcm-executor/src/traits/should_execute.rs @@ -133,8 +133,8 @@ impl CheckSuspension for Tuple { /// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns /// `Err(())`, the execution stops. Else, `Ok(_)` is returned if all elements accept the message. pub trait DenyExecution { - /// Returns `Ok(())` means there is no reason not to execute the message - /// while Err(e) indicates there is a reason not to execute. + /// Returns `None` if there is no reason to deny execution, + /// while `Some(ProcessMessageError)` indicates there is a reason to deny execution. /// /// - `origin`: The origin (sender) of the message. /// - `instructions`: The message itself.