From ece32e38a1a37aa354d51b16c07a42c66f23976e Mon Sep 17 00:00:00 2001 From: PG Herveou <pgherveou@gmail.com> Date: Wed, 15 Jan 2025 18:37:59 +0100 Subject: [PATCH] [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 <cyrill@parity.io> Co-authored-by: Alexander Theißen <alex.theissen@me.com> --- .../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 7844b0d885e..5966dd01f18 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< - <Runtime as frame_system::Config>::RuntimeEvent, - <Runtime as frame_system::Config>::Hash, ->; - impl pallet_revive::Config for Runtime { type Time = Timestamp; type Currency = Balances; @@ -2073,7 +2068,7 @@ impl_runtime_apis! { } } - impl pallet_revive::ReviveApi<Block, AccountId, Balance, Nonce, BlockNumber, EventRecord> for Runtime + impl pallet_revive::ReviveApi<Block, AccountId, Balance, Nonce, BlockNumber> for Runtime { fn balance(address: H160) -> U256 { Revive::evm_balance(&address) @@ -2108,7 +2103,7 @@ impl_runtime_apis! { gas_limit: Option<Weight>, storage_deposit_limit: Option<Balance>, input_data: Vec<u8>, - ) -> pallet_revive::ContractResult<pallet_revive::ExecReturnValue, Balance, EventRecord> { + ) -> pallet_revive::ContractResult<pallet_revive::ExecReturnValue, Balance> { let blockweights= <Runtime as frame_system::Config>::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<u8>, salt: Option<[u8; 32]>, - ) -> pallet_revive::ContractResult<pallet_revive::InstantiateReturnValue, Balance, EventRecord> + ) -> pallet_revive::ContractResult<pallet_revive::InstantiateReturnValue, Balance> { let blockweights= <Runtime as frame_system::Config>::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 00000000000..669c480b835 --- /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 e11a009c1c3..97728f12f5f 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<Block, AccountId, Balance, Nonce, BlockNumber, EventRecord> for Runtime + impl pallet_revive::ReviveApi<Block, AccountId, Balance, Nonce, BlockNumber> for Runtime { fn balance(address: H160) -> U256 { Revive::evm_balance(&address) @@ -3247,7 +3247,7 @@ impl_runtime_apis! { gas_limit: Option<Weight>, storage_deposit_limit: Option<Balance>, input_data: Vec<u8>, - ) -> pallet_revive::ContractResult<pallet_revive::ExecReturnValue, Balance, EventRecord> { + ) -> pallet_revive::ContractResult<pallet_revive::ExecReturnValue, Balance> { 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<u8>, salt: Option<[u8; 32]>, - ) -> pallet_revive::ContractResult<pallet_revive::InstantiateReturnValue, Balance, EventRecord> + ) -> pallet_revive::ContractResult<pallet_revive::InstantiateReturnValue, Balance> { 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 575920dfaac..7538f77d10b 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 6c850a9ec66..00000000000 --- 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 0ce2b6b5628..00000000000 --- 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 3a2509509d8..00000000000 --- 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 b09bdef1463..6e38063d20a 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 1177d47aadc..077e18ff5f0 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<T: Config> { gas_meter: GasMeter<T>, storage_meter: Meter<T>, value: BalanceOf<T>, - debug_message: Option<DebugBuffer>, data: Vec<u8>, 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<DebugBuffer> { - self.debug_message.clone() - } - /// Get the call's input data. pub fn data(&self) -> Vec<u8> { 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 1796348ff32..e23554f21ba 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::<T>::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::<Vec<_>>(); - - 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 c069216d6cc..e20c5dd7786 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: <Self::T as Config>::RuntimeCall) -> DispatchResultWithPostInfo; @@ -555,11 +542,6 @@ pub struct Stack<'a, T: Config, E> { frames: BoundedVec<Frame<T>, ConstU32<{ limits::CALL_STACK_DEPTH }>>, /// Statically guarantee that each call stack has at least one frame. first_frame: Frame<T>, - /// 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<T>, /// 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<u8>, 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<u8>, 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<T>, storage_meter: &'a mut storage::meter::Meter<T>, value: BalanceOf<T>, - 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<T>, value: U256, skip_transfer: bool, - debug_message: Option<&'a mut DebugBuffer>, ) -> Result<Option<(Self, E)>, ExecError> { origin.ensure_mapped()?; let Some((first_frame, executable)) = Self::new_frame( @@ -903,7 +868,6 @@ where block_number: <frame_system::Pallet<T>>::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("<Invalid UTF8>"), - ); - } 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: <Self::T as Config>::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::<Test>::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::<Test>::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 = <Test as Config>::Currency::minimum_balance(); - - let mut gas_meter = GasMeter::<Test>::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 = <Test as Config>::Currency::minimum_balance(); - - let mut gas_meter = GasMeter::<Test>::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 = <Test as Config>::Currency::minimum_balance(); - let mut gas_meter = GasMeter::<Test>::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), <Error<Test>>::ReentranceDenied, @@ -3623,7 +3425,6 @@ mod tests { U256::zero(), vec![0], false, - None, ) .map_err(|e| e.error), <Error<Test>>::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 bdb4b92edd9..403598ae136 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<T> = <<T as Config>::Currency as Inspect<<T as frame_system::Config>::AccountId>>::Balance; type OnChargeTransactionBalanceOf<T> = <<T as pallet_transaction_payment::Config>::OnChargeTransaction as OnChargeTransaction<T>>::Balance; type CodeVec = BoundedVec<u8, ConstU32<{ limits::code::BLOB_BYTES }>>; -type EventRecordOf<T> = - EventRecord<<T as frame_system::Config>::RuntimeEvent, <T as frame_system::Config>::Hash>; -type DebugBuffer = BoundedVec<u8, ConstU32<{ limits::DEBUG_BUFFER_BYTES }>>; type ImmutableData = BoundedVec<u8, ConstU32<{ limits::IMMUTABLE_BYTES }>>; /// 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<Self::RuntimeOrigin, Success = Self::AccountId>; - /// 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<Self>; @@ -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(<Error<T>>::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<BalanceOf<T>>, data: Vec<u8>, - debug: DebugInfo, - collect_events: CollectEvents, - ) -> ContractResult<ExecReturnValue, BalanceOf<T>, EventRecordOf<T>> { + ) -> ContractResult<ExecReturnValue, BalanceOf<T>> { 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::<T>::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<T>, value: BalanceOf<T>, @@ -1148,14 +1125,9 @@ where code: Code, data: Vec<u8>, salt: Option<[u8; 32]>, - debug: DebugInfo, - collect_events: CollectEvents, - ) -> ContractResult<InstantiateReturnValue, BalanceOf<T>, EventRecordOf<T>> { + ) -> ContractResult<InstantiateReturnValue, BalanceOf<T>> { 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::<T>::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::<T>::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<AccountId, Balance, Nonce, BlockNumber, EventRecord> where + pub trait ReviveApi<AccountId, Balance, Nonce, BlockNumber> 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<Weight>, storage_deposit_limit: Option<Balance>, input_data: Vec<u8>, - ) -> ContractResult<ExecReturnValue, Balance, EventRecord>; + ) -> ContractResult<ExecReturnValue, Balance>; /// Instantiate a new contract. /// @@ -1571,7 +1528,7 @@ sp_api::decl_runtime_apis! { code: Code, data: Vec<u8>, salt: Option<[u8; 32]>, - ) -> ContractResult<InstantiateReturnValue, Balance, EventRecord>; + ) -> ContractResult<InstantiateReturnValue, Balance>; /// Perform an Ethereum call. diff --git a/substrate/frame/revive/src/limits.rs b/substrate/frame/revive/src/limits.rs index 3b55106c67d..f101abf0ea7 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 452d2c8a306..9c149c7cc38 100644 --- a/substrate/frame/revive/src/primitives.rs +++ b/substrate/frame/revive/src/primitives.rs @@ -63,7 +63,7 @@ impl<T> From<T> for DepositLimit<T> { /// `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<R, Balance, EventRecord> { +pub struct ContractResult<R, Balance> { /// 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<R, Balance, EventRecord> { /// is `Err`. This is because on error all storage changes are rolled back including the /// payment of the deposit. pub storage_deposit: StorageDeposit<Balance>, - /// 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<u8>, /// The execution result of the wasm code. pub result: Result<R, DispatchError>, - /// The events that were emitted during execution. It is an option as event collection is - /// optional. - pub events: Option<Vec<EventRecord>>, } /// 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 8ba5e738407..7fbb5b67643 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<u8>, salt: Option<[u8; 32]>, - debug: DebugInfo, - collect_events: CollectEvents, - ) -> ContractResult<InstantiateReturnValue, BalanceOf<T>, EventRecordOf<T>>; + ) -> ContractResult<InstantiateReturnValue, BalanceOf<T>>; /// 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<BalanceOf<T>>, data: Vec<u8>, - debug: DebugInfo, - collect_events: CollectEvents, - ) -> ContractResult<ExecReturnValue, BalanceOf<T>, EventRecordOf<T>>; + ) -> ContractResult<ExecReturnValue, BalanceOf<T>>; /// 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::<T>()), 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 cf02d17a4d0..e2b30cf07c8 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 _ = <Test as Config>::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 _ = <Test as Config>::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::<Test>(), - 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 _ = <Test as Config>::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 _ = <Test as Config>::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 _ = <Test as Config>::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 _ = <Test as Config>::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 _ = <Test as Config>::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::<Test>::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 c9e19e52ace..b1fdb2d4744 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 3bd4bde5679..b45d7026ba9 100644 --- a/substrate/frame/revive/src/wasm/mod.rs +++ b/substrate/frame/revive/src/wasm/mod.rs @@ -288,7 +288,7 @@ impl<T: Config> WasmBlob<T> { } 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 8529c7d9e73..1ff6a80840a 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<T: Config> Token<T> 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<E::T>> 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<ReturnErrorCode, TrapReason> { - 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 e35ba5ca076..06495d5d21a 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<T: frame_system::Config> WeightInfo for SubstrateWeight<T> { // 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 ba0a63b15c3..b82393826dd 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<u32>; - /// 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 8c40bc9f48e..0023b8aa721 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 ef1798b4bf6..867f3563398 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. -- GitLab