diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index f8ecf656c65bd..0c1514375481e 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -373,4 +373,3 @@ try-runtime = [ "pallet-whitelist/try-runtime", "sp-runtime/try-runtime", ] -unsafe-debug = [ "pallet-contracts/unsafe-debug" ] diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index b536b9547b71f..9b943d2f66ce9 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1264,7 +1264,6 @@ impl pallet_contracts::Config for Runtime { type Migrations = pallet_contracts::migration::codegen::BenchMigrations; type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; - #[cfg(feature = "unsafe-debug")] type Debug = (); type Environment = (); } diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 3ad5367678dde..75a6093dffac8 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -114,4 +114,3 @@ try-runtime = [ "pallet-utility/try-runtime", "sp-runtime/try-runtime", ] -unsafe-debug = [] diff --git a/frame/contracts/src/debug.rs b/frame/contracts/src/debug.rs new file mode 100644 index 0000000000000..a92f428c8f8a4 --- /dev/null +++ b/frame/contracts/src/debug.rs @@ -0,0 +1,55 @@ +pub use crate::exec::ExportedFunction; +use crate::{CodeHash, Config, LOG_TARGET}; +use pallet_contracts_primitives::ExecReturnValue; + +/// Umbrella trait for all interfaces that serves for debugging. +pub trait Debugger: Tracing {} + +impl Debugger for V where V: Tracing {} + +/// Defines methods to capture contract calls, enabling external observers to +/// measure, trace, and react to contract interactions. +pub trait Tracing { + /// The type of [`CallSpan`] that is created by this trait. + type CallSpan: CallSpan; + + /// Creates a new call span to encompass the upcoming contract execution. + /// + /// This method should be invoked just before the execution of a contract and + /// marks the beginning of a traceable span of execution. + /// + /// # Arguments + /// + /// * `code_hash` - The code hash of the contract being called. + /// * `entry_point` - Describes whether the call is the constructor or a regular call. + /// * `input_data` - The raw input data of the call. + fn new_call_span( + code_hash: &CodeHash, + entry_point: ExportedFunction, + input_data: &[u8], + ) -> Self::CallSpan; +} + +/// Defines a span of execution for a contract call. +pub trait CallSpan { + /// Called just after the execution of a contract. + /// + /// # Arguments + /// + /// * `output` - The raw output of the call. + fn after_call(self, output: &ExecReturnValue); +} + +impl Tracing for () { + type CallSpan = (); + + fn new_call_span(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { + log::trace!(target: LOG_TARGET, "call {entry_point:?} hash: {code_hash:?}, input_data: {input_data:?}") + } +} + +impl CallSpan for () { + fn after_call(self, output: &ExecReturnValue) { + log::trace!(target: LOG_TARGET, "call result {output:?}") + } +} diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index a4d0760e6c0ac..1ba44220ff8dc 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -15,9 +15,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -#[cfg(feature = "unsafe-debug")] -use crate::unsafe_debug::ExecutionObserver; use crate::{ + debug::{CallSpan, Tracing}, gas::GasMeter, storage::{self, meter::Diff, WriteOutcome}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, @@ -908,20 +907,15 @@ where // Every non delegate call or instantiate also optionally transfers the balance. self.initial_transfer()?; - #[cfg(feature = "unsafe-debug")] - let (code_hash, input_clone) = { - let code_hash = *executable.code_hash(); - T::Debug::before_call(&code_hash, entry_point, &input_data); - (code_hash, input_data.clone()) - }; + let call_span = + T::Debug::new_call_span(executable.code_hash(), entry_point, &input_data); // Call into the Wasm blob. let output = executable .execute(self, &entry_point, input_data) .map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?; - #[cfg(feature = "unsafe-debug")] - T::Debug::after_call(&code_hash, entry_point, input_clone, &output); + call_span.after_call(&output); // Avoid useless work that would be reverted anyways. if output.did_revert() { diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index f67e4d7035ef8..2b9dd07b3f6fe 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -96,8 +96,8 @@ mod storage; mod wasm; pub mod chain_extension; +pub mod debug; pub mod migration; -pub mod unsafe_debug; pub mod weights; #[cfg(test)] @@ -144,6 +144,7 @@ use sp_std::{fmt::Debug, prelude::*}; pub use crate::{ address::{AddressGenerator, DefaultAddressGenerator}, + debug::Tracing, exec::Frame, migration::{MigrateSequence, Migration, NoopMigration}, pallet::*, @@ -219,6 +220,7 @@ pub struct Environment { #[frame_support::pallet] pub mod pallet { use super::*; + use crate::debug::Debugger; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; use sp_runtime::Perbill; @@ -390,13 +392,11 @@ pub mod pallet { /// ``` type Migrations: MigrateSequence; - /// Type that provides debug handling for the contract execution process. - /// - /// # Warning - /// - /// Do **not** use it in a production environment or for benchmarking purposes. - #[cfg(feature = "unsafe-debug")] - type Debug: unsafe_debug::UnsafeDebug; + /// # Note + /// For most production chains, it's recommended to use the `()` implementation of this + /// trait. This implementation offers additional logging when the log target + /// "runtime::contracts" is set to trace. + type Debug: Debugger; /// Type that bundles together all the runtime configurable interface types. /// diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index b421bbf3e3093..8cc6d00b3d45d 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -16,9 +16,12 @@ // limitations under the License. mod pallet_dummy; -mod unsafe_debug; +mod test_debug; -use self::test_utils::{ensure_stored, expected_deposit, hash}; +use self::{ + test_debug::TestDebug, + test_utils::{ensure_stored, expected_deposit, hash}, +}; use crate::{ self as pallet_contracts, chain_extension::{ @@ -479,8 +482,7 @@ impl Config for Test { type Migrations = crate::migration::codegen::BenchMigrations; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; type MaxDelegateDependencies = MaxDelegateDependencies; - #[cfg(feature = "unsafe-debug")] - type Debug = unsafe_debug::TestDebugger; + type Debug = TestDebug; type Environment = (); } diff --git a/frame/contracts/src/tests/unsafe_debug.rs b/frame/contracts/src/tests/test_debug.rs similarity index 83% rename from frame/contracts/src/tests/unsafe_debug.rs rename to frame/contracts/src/tests/test_debug.rs index 160a6ed6dc54f..ba936a4588d18 100644 --- a/frame/contracts/src/tests/unsafe_debug.rs +++ b/frame/contracts/src/tests/test_debug.rs @@ -1,7 +1,5 @@ -#![cfg(feature = "unsafe-debug")] - use super::*; -use crate::unsafe_debug::{ExecutionObserver, ExportedFunction}; +use crate::debug::{CallSpan, ExportedFunction, Tracing}; use frame_support::traits::Currency; use pallet_contracts_primitives::ExecReturnValue; use pretty_assertions::assert_eq; @@ -19,31 +17,40 @@ thread_local! { static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); } -pub struct TestDebugger; +pub struct TestDebug; +pub struct TestCallSpan { + code_hash: CodeHash, + call: ExportedFunction, + input: Vec, +} + +impl Tracing for TestDebug { + type CallSpan = TestCallSpan; -impl ExecutionObserver> for TestDebugger { - fn before_call(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { + fn new_call_span( + code_hash: &CodeHash, + entry_point: ExportedFunction, + input_data: &[u8], + ) -> TestCallSpan { DEBUG_EXECUTION_TRACE.with(|d| { d.borrow_mut().push(DebugFrame { - code_hash: code_hash.clone(), + code_hash: *code_hash, call: entry_point, input: input_data.to_vec(), result: None, }) }); + TestCallSpan { code_hash: *code_hash, call: entry_point, input: input_data.to_vec() } } +} - fn after_call( - code_hash: &CodeHash, - entry_point: ExportedFunction, - input_data: Vec, - output: &ExecReturnValue, - ) { +impl CallSpan for TestCallSpan { + fn after_call(self, output: &ExecReturnValue) { DEBUG_EXECUTION_TRACE.with(|d| { d.borrow_mut().push(DebugFrame { - code_hash: code_hash.clone(), - call: entry_point, - input: input_data, + code_hash: self.code_hash, + call: self.call, + input: self.input, result: Some(output.data.clone()), }) }); diff --git a/frame/contracts/src/unsafe_debug.rs b/frame/contracts/src/unsafe_debug.rs deleted file mode 100644 index 418af5e605d28..0000000000000 --- a/frame/contracts/src/unsafe_debug.rs +++ /dev/null @@ -1,47 +0,0 @@ -#![cfg(feature = "unsafe-debug")] - -pub use crate::exec::ExportedFunction; -use crate::{CodeHash, Vec}; -use pallet_contracts_primitives::ExecReturnValue; - -/// Umbrella trait for all interfaces that serves for debugging, but are not suitable for any -/// production or benchmarking use. -pub trait UnsafeDebug: ExecutionObserver> {} - -impl UnsafeDebug for D where D: ExecutionObserver> {} - -/// Defines the interface between pallet contracts and the outside observer. -/// -/// The intended use is the environment, where the observer holds directly the whole runtime -/// (externalities) and thus can react to the execution breakpoints synchronously. -/// -/// This definitely *should not* be used in any production or benchmarking setting, since handling -/// callbacks might be arbitrarily expensive and thus significantly influence performance. -pub trait ExecutionObserver { - /// Called just before the execution of a contract. - /// - /// # Arguments - /// - /// * `code_hash` - The code hash of the contract being called. - /// * `entry_point` - Describes whether the call is the constructor or a regular call. - /// * `input_data` - The raw input data of the call. - fn before_call(_code_hash: &CodeHash, _entry_point: ExportedFunction, _input_data: &[u8]) {} - - /// Called just after the execution of a contract. - /// - /// # Arguments - /// - /// * `code_hash` - The code hash of the contract being called. - /// * `entry_point` - Describes whether the call was the constructor or a regular call. - /// * `input_data` - The raw input data of the call. - /// * `output` - The raw output of the call. - fn after_call( - _code_hash: &CodeHash, - _entry_point: ExportedFunction, - _input_data: Vec, - _output: &ExecReturnValue, - ) { - } -} - -impl ExecutionObserver for () {} diff --git a/scripts/ci/gitlab/pipeline/test.yml b/scripts/ci/gitlab/pipeline/test.yml index 71ae5105a61d8..d986fa3402265 100644 --- a/scripts/ci/gitlab/pipeline/test.yml +++ b/scripts/ci/gitlab/pipeline/test.yml @@ -234,7 +234,7 @@ test-linux-stable: --locked --release --verbose - --features runtime-benchmarks,try-runtime,experimental,unsafe-debug + --features runtime-benchmarks,try-runtime,experimental --manifest-path ./bin/node/cli/Cargo.toml --partition count:${CI_NODE_INDEX}/${CI_NODE_TOTAL} # we need to update cache only from one job