From cd464f9cfdc76811fae75a9487efe3f7546eaa26 Mon Sep 17 00:00:00 2001
From: PG Herveou <pgherveou@gmail.com>
Date: Thu, 24 Aug 2023 09:56:28 +0200
Subject: [PATCH] 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 | 1 -
substrate/bin/node/runtime/src/lib.rs | 1 -
substrate/frame/contracts/Cargo.toml | 1 -
substrate/frame/contracts/src/debug.rs | 55 +++++++++++++++++++
substrate/frame/contracts/src/exec.rs | 14 ++---
substrate/frame/contracts/src/lib.rs | 16 +++---
substrate/frame/contracts/src/tests.rs | 10 ++--
.../tests/{unsafe_debug.rs => test_debug.rs} | 39 +++++++------
substrate/frame/contracts/src/unsafe_debug.rs | 47 ----------------
substrate/scripts/ci/gitlab/pipeline/test.yml | 2 +-
10 files changed, 97 insertions(+), 89 deletions(-)
create mode 100644 substrate/frame/contracts/src/debug.rs
rename substrate/frame/contracts/src/tests/{unsafe_debug.rs => test_debug.rs} (83%)
delete mode 100644 substrate/frame/contracts/src/unsafe_debug.rs
diff --git a/substrate/bin/node/runtime/Cargo.toml b/substrate/bin/node/runtime/Cargo.toml
index f8ecf656c65..0c151437548 100644
--- a/substrate/bin/node/runtime/Cargo.toml
+++ b/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" ]
diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs
index b536b9547b7..9b943d2f66c 100644
--- a/substrate/bin/node/runtime/src/lib.rs
+++ b/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 = ();
}
diff --git a/substrate/frame/contracts/Cargo.toml b/substrate/frame/contracts/Cargo.toml
index 3ad5367678d..75a6093dffa 100644
--- a/substrate/frame/contracts/Cargo.toml
+++ b/substrate/frame/contracts/Cargo.toml
@@ -114,4 +114,3 @@ try-runtime = [
"pallet-utility/try-runtime",
"sp-runtime/try-runtime",
]
-unsafe-debug = []
diff --git a/substrate/frame/contracts/src/debug.rs b/substrate/frame/contracts/src/debug.rs
new file mode 100644
index 00000000000..a92f428c8f8
--- /dev/null
+++ b/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:?}")
+ }
+}
diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs
index a4d0760e6c0..1ba44220ff8 100644
--- a/substrate/frame/contracts/src/exec.rs
+++ b/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 (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/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs
index f67e4d7035e..2b9dd07b3f6 100644
--- a/substrate/frame/contracts/src/lib.rs
+++ b/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.
- ///
- /// # Warning
- ///
- /// Do **not** use it in a production environment or for benchmarking purposes.
- #[cfg(feature = "unsafe-debug")]
- type Debug: unsafe_debug::UnsafeDebug<Self>;
+ /// # 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<Self>;
/// Type that bundles together all the runtime configurable interface types.
///
diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs
index b421bbf3e30..8cc6d00b3d4 100644
--- a/substrate/frame/contracts/src/tests.rs
+++ b/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 = unsafe_debug::TestDebugger;
+ type Debug = TestDebug;
type Environment = ();
}
diff --git a/substrate/frame/contracts/src/tests/unsafe_debug.rs b/substrate/frame/contracts/src/tests/test_debug.rs
similarity index 83%
rename from substrate/frame/contracts/src/tests/unsafe_debug.rs
rename to substrate/frame/contracts/src/tests/test_debug.rs
index 160a6ed6dc5..ba936a4588d 100644
--- a/substrate/frame/contracts/src/tests/unsafe_debug.rs
+++ b/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 Tracing<Test> for TestDebug {
+ type CallSpan = TestCallSpan;
-impl ExecutionObserver<CodeHash<Test>> for TestDebugger {
- fn before_call(code_hash: &CodeHash<Test>, entry_point: ExportedFunction, input_data: &[u8]) {
+ 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(
- code_hash: &CodeHash<Test>,
- entry_point: ExportedFunction,
- input_data: Vec<u8>,
- 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/substrate/frame/contracts/src/unsafe_debug.rs b/substrate/frame/contracts/src/unsafe_debug.rs
deleted file mode 100644
index 418af5e605d..00000000000
--- a/substrate/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<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 () {}
diff --git a/substrate/scripts/ci/gitlab/pipeline/test.yml b/substrate/scripts/ci/gitlab/pipeline/test.yml
index 71ae5105a61..d986fa34022 100644
--- a/substrate/scripts/ci/gitlab/pipeline/test.yml
+++ b/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
--
GitLab