Contracts: Update Config::Debug (#14789)

* Update Debug trait

* Rename

* tweak

* fmt

* Better namings

* rm unsafe-debug

* rework doc

* nit

* fix comment

* clippy

* update naming

* Rename file

* fmt fixes

* rename

* Move tracing behind umbrella Debugging trait

* fix

* fix comment

* reorder imports

* comment

* update doc

* add missing doc

* add missing doc

* Update Debugging -> Debugger

* Update bin/node/runtime/Cargo.toml

substrate/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" ]

substrate/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 = ();
 }

substrate/frame/contracts/Cargo.toml

@@ -114,4 +114,3 @@ try-runtime = [
 	"pallet-utility/try-runtime",
 	"sp-runtime/try-runtime",
 ]
-unsafe-debug = []

substrate/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<T: Config>: Tracing<T> {}
+
+impl<T: Config, V> Debugger<T> for V where V: Tracing<T> {}
+
+/// Defines methods to capture contract calls, enabling external observers to
+/// measure, trace, and react to contract interactions.
+pub trait Tracing<T: Config> {
+	/// 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<T>,
+		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<T: Config> Tracing<T> for () {
+	type CallSpan = ();
+
+	fn new_call_span(code_hash: &CodeHash<T>, 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:?}")
+	}
+}

substrate/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 call_span =
-			let (code_hash, input_clone) = {
+				T::Debug::new_call_span(executable.code_hash(), entry_point, &input_data);
-				let code_hash = *executable.code_hash();
-				T::Debug::before_call(&code_hash, entry_point, &input_data);
-				(code_hash, input_data.clone())
-			};
 
 			// 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")]
+			call_span.after_call(&output);
-			T::Debug::after_call(&code_hash, entry_point, input_clone, &output);
 
 			// Avoid useless work that would be reverted anyways.
 			if output.did_revert() {

substrate/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<T: Config> {
 #[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.
+		/// # Note
-		///
+		/// For most production chains, it's recommended to use the `()` implementation of this
-		/// # Warning
+		/// trait. This implementation offers additional logging when the log target
-		///
+		/// "runtime::contracts" is set to trace.
-		/// Do **not** use it in a production environment or for benchmarking purposes.
+		type Debug: Debugger<Self>;
-		#[cfg(feature = "unsafe-debug")]
-		type Debug: unsafe_debug::UnsafeDebug<Self>;
 
 		/// Type that bundles together all the runtime configurable interface types.
 		///

substrate/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 = TestDebug;
-	type Debug = unsafe_debug::TestDebugger;
 	type Environment = ();
 }
 

substrate/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<Vec<DebugFrame>> = RefCell::new(Vec::new());
 }
 
-pub struct TestDebugger;
+pub struct TestDebug;
+pub struct TestCallSpan {
+	code_hash: CodeHash<Test>,
+	call: ExportedFunction,
+	input: Vec<u8>,
+}
 
-impl ExecutionObserver<CodeHash<Test>> for TestDebugger {
+impl Tracing<Test> for TestDebug {
-	fn before_call(code_hash: &CodeHash<Test>, entry_point: ExportedFunction, input_data: &[u8]) {
+	type CallSpan = TestCallSpan;
+
+	fn new_call_span(
+		code_hash: &CodeHash<Test>,
+		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(
+impl CallSpan for TestCallSpan {
-		code_hash: &CodeHash<Test>,
+	fn after_call(self, output: &ExecReturnValue) {
-		entry_point: ExportedFunction,
-		input_data: Vec<u8>,
-		output: &ExecReturnValue,
-	) {
 		DEBUG_EXECUTION_TRACE.with(|d| {
 			d.borrow_mut().push(DebugFrame {
-				code_hash: code_hash.clone(),
+				code_hash: self.code_hash,
-				call: entry_point,
+				call: self.call,
-				input: input_data,
+				input: self.input,
 				result: Some(output.data.clone()),
 			})
 		});

substrate/frame/contracts/src/unsafe_debug.rs

@@ -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<T: frame_system::Config>: ExecutionObserver<CodeHash<T>> {}
-
-impl<T: frame_system::Config, D> UnsafeDebug<T> for D where D: ExecutionObserver<CodeHash<T>> {}
-
-/// 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<CodeHash> {
-	/// 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<u8>,
-		_output: &ExecReturnValue,
-	) {
-	}
-}
-
-impl<CodeHash> ExecutionObserver<CodeHash> for () {}

substrate/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