From 8fdffa19c79c12da78e91d883c1488f5f4c772af Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 18:18:58 +0200 Subject: [PATCH 01/24] Update Debug trait --- frame/contracts/src/debug.rs | 48 +++++++++++++++++++ frame/contracts/src/exec.rs | 14 ++---- frame/contracts/src/lib.rs | 10 ++-- frame/contracts/src/tests.rs | 11 +++-- .../tests/{unsafe_debug.rs => test_debug.rs} | 32 ++++++++----- frame/contracts/src/unsafe_debug.rs | 47 ------------------ 6 files changed, 81 insertions(+), 81 deletions(-) create mode 100644 frame/contracts/src/debug.rs rename frame/contracts/src/tests/{unsafe_debug.rs => test_debug.rs} (84%) delete mode 100644 frame/contracts/src/unsafe_debug.rs diff --git a/frame/contracts/src/debug.rs b/frame/contracts/src/debug.rs new file mode 100644 index 0000000000000..8478fce7ce862 --- /dev/null +++ b/frame/contracts/src/debug.rs @@ -0,0 +1,48 @@ +pub use crate::exec::ExportedFunction; +use crate::{CodeHash, Config, LOG_TARGET}; +use pallet_contracts_primitives::ExecReturnValue; + +pub trait CallSpan +where + Self: Sized, +{ + fn after_call(self, output: &ExecReturnValue) { + log::trace!(target: LOG_TARGET, "call result {output:?}") + } +} + +impl CallSpan for () {} + +/// 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 CallObserver { + /// 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], + ) -> Span; +} + +pub struct CallLogger; + +impl CallObserver for CallLogger { + fn before_call( + code_hash: &CodeHash, + entry_point: ExportedFunction, + input_data: &[u8], + ) -> () { + log::trace!(target: LOG_TARGET, "call {entry_point:?} hash: {code_hash:?}, input_data: {input_data:?}") + } +} diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index a4d0760e6c0ac..2f8ddf9e429b0 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::{CallObserver, CallSpan}, 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 code_hash = *executable.code_hash(); + let call_span = T::Debug::before_call(&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 a511ac81a73b7..a2afb19817368 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)] @@ -354,12 +354,8 @@ 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; + type DebugSpan: debug::CallSpan; + type Debug: debug::CallObserver; } #[pallet::hooks] diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 5c8fca43ab746..3c1dd4f5a07c2 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::{TestCallSpan, TestDebugger}, + test_utils::{ensure_stored, expected_deposit, hash}, +}; use crate::{ self as pallet_contracts, chain_extension::{ @@ -479,8 +482,8 @@ 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 = TestDebugger; + type DebugSpan = TestCallSpan; } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]); diff --git a/frame/contracts/src/tests/unsafe_debug.rs b/frame/contracts/src/tests/test_debug.rs similarity index 84% rename from frame/contracts/src/tests/unsafe_debug.rs rename to frame/contracts/src/tests/test_debug.rs index 160a6ed6dc54f..92a330f94ddc4 100644 --- a/frame/contracts/src/tests/unsafe_debug.rs +++ b/frame/contracts/src/tests/test_debug.rs @@ -1,7 +1,6 @@ -#![cfg(feature = "unsafe-debug")] +use crate::debug::{CallObserver, CallSpan, ExportedFunction}; use super::*; -use crate::unsafe_debug::{ExecutionObserver, ExportedFunction}; use frame_support::traits::Currency; use pallet_contracts_primitives::ExecReturnValue; use pretty_assertions::assert_eq; @@ -20,9 +19,18 @@ thread_local! { } pub struct TestDebugger; +pub struct TestCallSpan { + code_hash: CodeHash, + call: ExportedFunction, + input: Vec, +} -impl ExecutionObserver> for TestDebugger { - fn before_call(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { +impl CallObserver for TestDebugger { + fn before_call( + 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(), @@ -31,19 +39,17 @@ impl ExecutionObserver> for TestDebugger { result: None, }) }); + TestCallSpan { code_hash: code_hash.clone(), 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.clone(), + call: self.call, + input: self.input.clone(), 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 () {} From c12935901be7f5bf3ea74e9ce2f00bd89e4365f3 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 18:26:38 +0200 Subject: [PATCH 02/24] Rename --- frame/contracts/src/{debug.rs => call_observability.rs} | 0 frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 7 ++++--- frame/contracts/src/tests.rs | 4 ++-- .../src/tests/{test_debug.rs => test_observability.rs} | 2 +- 5 files changed, 8 insertions(+), 7 deletions(-) rename frame/contracts/src/{debug.rs => call_observability.rs} (100%) rename frame/contracts/src/tests/{test_debug.rs => test_observability.rs} (97%) diff --git a/frame/contracts/src/debug.rs b/frame/contracts/src/call_observability.rs similarity index 100% rename from frame/contracts/src/debug.rs rename to frame/contracts/src/call_observability.rs diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 2f8ddf9e429b0..1ff9705c57a70 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -16,7 +16,7 @@ // limitations under the License. use crate::{ - debug::{CallObserver, CallSpan}, + call_observability::{CallObserver, CallSpan}, gas::GasMeter, storage::{self, meter::Diff, WriteOutcome}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a2afb19817368..2c2f177d3cfd1 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -95,8 +95,8 @@ mod schedule; mod storage; mod wasm; +pub mod call_observability; pub mod chain_extension; -pub mod debug; pub mod migration; pub mod weights; @@ -108,6 +108,7 @@ use crate::{ storage::{meter::Meter as StorageMeter, ContractInfo, DeletionQueueManager}, wasm::{CodeInfo, WasmBlob}, }; +use call_observability::{CallObserver, CallSpan}; use codec::{Codec, Decode, Encode, HasCompact, MaxEncodedLen}; use environmental::*; use frame_support::{ @@ -354,8 +355,8 @@ pub mod pallet { type Migrations: MigrateSequence; /// Type that provides debug handling for the contract execution process. - type DebugSpan: debug::CallSpan; - type Debug: debug::CallObserver; + type DebugSpan: CallSpan; + type Debug: CallObserver; } #[pallet::hooks] diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 3c1dd4f5a07c2..94c5e21bf695d 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -16,10 +16,10 @@ // limitations under the License. mod pallet_dummy; -mod test_debug; +mod test_observability; use self::{ - test_debug::{TestCallSpan, TestDebugger}, + test_observability::{TestCallSpan, TestDebugger}, test_utils::{ensure_stored, expected_deposit, hash}, }; use crate::{ diff --git a/frame/contracts/src/tests/test_debug.rs b/frame/contracts/src/tests/test_observability.rs similarity index 97% rename from frame/contracts/src/tests/test_debug.rs rename to frame/contracts/src/tests/test_observability.rs index 92a330f94ddc4..b614a3ea075c9 100644 --- a/frame/contracts/src/tests/test_debug.rs +++ b/frame/contracts/src/tests/test_observability.rs @@ -1,4 +1,4 @@ -use crate::debug::{CallObserver, CallSpan, ExportedFunction}; +use crate::call_observability::{CallObserver, CallSpan, ExportedFunction}; use super::*; use frame_support::traits::Currency; From 341b47b3df34b33694b94e900b75ab6500eeadf4 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 18:45:07 +0200 Subject: [PATCH 03/24] tweak --- frame/contracts/src/call_observability.rs | 36 +++++++++---------- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 5 ++- frame/contracts/src/tests.rs | 3 +- .../contracts/src/tests/test_observability.rs | 13 +++---- 5 files changed, 26 insertions(+), 33 deletions(-) diff --git a/frame/contracts/src/call_observability.rs b/frame/contracts/src/call_observability.rs index 8478fce7ce862..5c3ef7d8c4417 100644 --- a/frame/contracts/src/call_observability.rs +++ b/frame/contracts/src/call_observability.rs @@ -2,25 +2,11 @@ pub use crate::exec::ExportedFunction; use crate::{CodeHash, Config, LOG_TARGET}; use pallet_contracts_primitives::ExecReturnValue; -pub trait CallSpan -where - Self: Sized, -{ - fn after_call(self, output: &ExecReturnValue) { - log::trace!(target: LOG_TARGET, "call result {output:?}") - } -} - -impl CallSpan for () {} - /// 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 CallObserver { +pub trait CallSpan where Self: Sized { /// Called just before the execution of a contract. /// /// # Arguments @@ -32,17 +18,29 @@ pub trait CallObserver { code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8], - ) -> Span; + ) -> Self; + + /// Called just after the execution of a contract. + /// + /// # Arguments + /// + /// * `output` - The raw output of the call. + fn after_call(self, output: &ExecReturnValue) { + log::trace!(target: LOG_TARGET, "call result {output:?}") + } } -pub struct CallLogger; -impl CallObserver for CallLogger { +impl CallSpan for () { fn before_call( code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8], - ) -> () { + ) { log::trace!(target: LOG_TARGET, "call {entry_point:?} hash: {code_hash:?}, input_data: {input_data:?}") } + + 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 1ff9705c57a70..4bd03cdd23705 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -16,7 +16,7 @@ // limitations under the License. use crate::{ - call_observability::{CallObserver, CallSpan}, + call_observability::CallSpan, gas::GasMeter, storage::{self, meter::Diff, WriteOutcome}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 2c2f177d3cfd1..5ceae41626ed1 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -108,7 +108,7 @@ use crate::{ storage::{meter::Meter as StorageMeter, ContractInfo, DeletionQueueManager}, wasm::{CodeInfo, WasmBlob}, }; -use call_observability::{CallObserver, CallSpan}; +use call_observability::CallSpan; use codec::{Codec, Decode, Encode, HasCompact, MaxEncodedLen}; use environmental::*; use frame_support::{ @@ -355,8 +355,7 @@ pub mod pallet { type Migrations: MigrateSequence; /// Type that provides debug handling for the contract execution process. - type DebugSpan: CallSpan; - type Debug: CallObserver; + type Debug: CallSpan; } #[pallet::hooks] diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 94c5e21bf695d..99e71f219e2b7 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -19,7 +19,7 @@ mod pallet_dummy; mod test_observability; use self::{ - test_observability::{TestCallSpan, TestDebugger}, + test_observability::TestDebugger, test_utils::{ensure_stored, expected_deposit, hash}, }; use crate::{ @@ -483,7 +483,6 @@ impl Config for Test { type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; type MaxDelegateDependencies = MaxDelegateDependencies; type Debug = TestDebugger; - type DebugSpan = TestCallSpan; } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]); diff --git a/frame/contracts/src/tests/test_observability.rs b/frame/contracts/src/tests/test_observability.rs index b614a3ea075c9..4630a3cb4c03b 100644 --- a/frame/contracts/src/tests/test_observability.rs +++ b/frame/contracts/src/tests/test_observability.rs @@ -1,4 +1,4 @@ -use crate::call_observability::{CallObserver, CallSpan, ExportedFunction}; +use crate::call_observability::{CallSpan, ExportedFunction}; use super::*; use frame_support::traits::Currency; @@ -18,19 +18,18 @@ thread_local! { static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); } -pub struct TestDebugger; -pub struct TestCallSpan { +pub struct TestDebugger { code_hash: CodeHash, call: ExportedFunction, input: Vec, } -impl CallObserver for TestDebugger { +impl CallSpan for TestDebugger { fn before_call( code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8], - ) -> TestCallSpan { + ) -> TestDebugger { DEBUG_EXECUTION_TRACE.with(|d| { d.borrow_mut().push(DebugFrame { code_hash: code_hash.clone(), @@ -39,11 +38,9 @@ impl CallObserver for TestDebugger { result: None, }) }); - TestCallSpan { code_hash: code_hash.clone(), call: entry_point, input: input_data.to_vec() } + TestDebugger { code_hash: code_hash.clone(), call: entry_point, input: input_data.to_vec() } } -} -impl CallSpan for TestCallSpan { fn after_call(self, output: &ExecReturnValue) { DEBUG_EXECUTION_TRACE.with(|d| { d.borrow_mut().push(DebugFrame { From 9df4a594d748626696dfdfb2a23a51d5409d0880 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 18:49:23 +0200 Subject: [PATCH 04/24] fmt --- frame/contracts/src/call_observability.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/frame/contracts/src/call_observability.rs b/frame/contracts/src/call_observability.rs index 5c3ef7d8c4417..11feafeb85976 100644 --- a/frame/contracts/src/call_observability.rs +++ b/frame/contracts/src/call_observability.rs @@ -6,7 +6,10 @@ use pallet_contracts_primitives::ExecReturnValue; /// /// The intended use is the environment, where the observer holds directly the whole runtime /// (externalities) and thus can react to the execution breakpoints synchronously. -pub trait CallSpan where Self: Sized { +pub trait CallSpan +where + Self: Sized, +{ /// Called just before the execution of a contract. /// /// # Arguments @@ -30,13 +33,8 @@ pub trait CallSpan where Self: Sized { } } - impl CallSpan for () { - fn before_call( - code_hash: &CodeHash, - entry_point: ExportedFunction, - input_data: &[u8], - ) { + fn before_call(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { log::trace!(target: LOG_TARGET, "call {entry_point:?} hash: {code_hash:?}, input_data: {input_data:?}") } From 7658396ba32e54e45bba4b98810f1f0426d08590 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 23:06:32 +0200 Subject: [PATCH 05/24] Better namings --- bin/node/runtime/src/lib.rs | 3 +-- frame/contracts/src/call_observability.rs | 18 +++++------------- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 5 +++-- frame/contracts/src/tests.rs | 4 ++-- .../contracts/src/tests/test_observability.rs | 10 +++++----- 6 files changed, 17 insertions(+), 25 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 4e1b6d4e8bec0..a151990c32bac 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1263,8 +1263,7 @@ 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 TraceableCallSpan = (); } impl pallet_sudo::Config for Runtime { diff --git a/frame/contracts/src/call_observability.rs b/frame/contracts/src/call_observability.rs index 11feafeb85976..0a0bab85fd6b3 100644 --- a/frame/contracts/src/call_observability.rs +++ b/frame/contracts/src/call_observability.rs @@ -2,10 +2,8 @@ pub use crate::exec::ExportedFunction; use crate::{CodeHash, Config, LOG_TARGET}; use pallet_contracts_primitives::ExecReturnValue; -/// 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. +/// CallSpan defines methods to capture contract calls, enabling external observers to +/// measure, trace, and react to contract interactions. pub trait CallSpan where Self: Sized, @@ -17,24 +15,18 @@ where /// * `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], - ) -> Self; + fn new(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) -> Self; /// Called just after the execution of a contract. /// /// # Arguments /// /// * `output` - The raw output of the call. - fn after_call(self, output: &ExecReturnValue) { - log::trace!(target: LOG_TARGET, "call result {output:?}") - } + fn after_call(self, output: &ExecReturnValue); } impl CallSpan for () { - fn before_call(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { + fn new(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { log::trace!(target: LOG_TARGET, "call {entry_point:?} hash: {code_hash:?}, input_data: {input_data:?}") } diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 4bd03cdd23705..e2694a68f6995 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -908,7 +908,7 @@ where self.initial_transfer()?; let code_hash = *executable.code_hash(); - let call_span = T::Debug::before_call(&code_hash, entry_point, &input_data); + let call_span = T::TraceableCallSpan::new(&code_hash, entry_point, &input_data); // Call into the Wasm blob. let output = executable diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 5ceae41626ed1..ffa6966bbf424 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -354,8 +354,9 @@ pub mod pallet { /// ``` type Migrations: MigrateSequence; - /// Type that provides debug handling for the contract execution process. - type Debug: CallSpan; + /// TraceableCallSpan defines methods to capture and trace contract calls + /// for improved observability. + type TraceableCallSpan: CallSpan; } #[pallet::hooks] diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 99e71f219e2b7..3d894d32b6261 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -19,7 +19,7 @@ mod pallet_dummy; mod test_observability; use self::{ - test_observability::TestDebugger, + test_observability::TestCallSpan, test_utils::{ensure_stored, expected_deposit, hash}, }; use crate::{ @@ -482,7 +482,7 @@ impl Config for Test { type Migrations = crate::migration::codegen::BenchMigrations; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; type MaxDelegateDependencies = MaxDelegateDependencies; - type Debug = TestDebugger; + type TraceableCallSpan = TestCallSpan; } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]); diff --git a/frame/contracts/src/tests/test_observability.rs b/frame/contracts/src/tests/test_observability.rs index 4630a3cb4c03b..1fdb1ba07d69d 100644 --- a/frame/contracts/src/tests/test_observability.rs +++ b/frame/contracts/src/tests/test_observability.rs @@ -18,18 +18,18 @@ thread_local! { static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); } -pub struct TestDebugger { +pub struct TestCallSpan { code_hash: CodeHash, call: ExportedFunction, input: Vec, } -impl CallSpan for TestDebugger { - fn before_call( +impl CallSpan for TestCallSpan { + fn new( code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8], - ) -> TestDebugger { + ) -> TestCallSpan { DEBUG_EXECUTION_TRACE.with(|d| { d.borrow_mut().push(DebugFrame { code_hash: code_hash.clone(), @@ -38,7 +38,7 @@ impl CallSpan for TestDebugger { result: None, }) }); - TestDebugger { code_hash: code_hash.clone(), call: entry_point, input: input_data.to_vec() } + TestCallSpan { code_hash: code_hash.clone(), call: entry_point, input: input_data.to_vec() } } fn after_call(self, output: &ExecReturnValue) { From 662cd057cf2f7c21cdbe90b52cfb8671d6820bdf Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 23:24:25 +0200 Subject: [PATCH 06/24] rm unsafe-debug --- bin/node/runtime/Cargo.toml | 1 - frame/contracts/Cargo.toml | 1 - scripts/ci/gitlab/pipeline/test.yml | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index de0ec1a5489a8..4a9bb9a769597 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -373,4 +373,3 @@ try-runtime = [ "frame-election-provider-support/try-runtime", "sp-runtime/try-runtime" ] -unsafe-debug = ["pallet-contracts/unsafe-debug"] diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index a5c309adc97bc..0f9e94b53c519 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/scripts/ci/gitlab/pipeline/test.yml b/scripts/ci/gitlab/pipeline/test.yml index 61c1aa876f306..00d06eefc030b 100644 --- a/scripts/ci/gitlab/pipeline/test.yml +++ b/scripts/ci/gitlab/pipeline/test.yml @@ -223,7 +223,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 From 965ae4f04654f3f0ace4ddfdfe011f790995d9ed Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 23:30:27 +0200 Subject: [PATCH 07/24] rework doc --- frame/contracts/src/call_observability.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/call_observability.rs b/frame/contracts/src/call_observability.rs index 0a0bab85fd6b3..e3e4468dc8549 100644 --- a/frame/contracts/src/call_observability.rs +++ b/frame/contracts/src/call_observability.rs @@ -8,7 +8,7 @@ pub trait CallSpan where Self: Sized, { - /// Called just before the execution of a contract. + /// A new call span is created just before the execution of a contract, to capture the call. /// /// # Arguments /// From 6b2b8e590767e94f9463d53e11587c0b9e53db55 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 23:33:31 +0200 Subject: [PATCH 08/24] nit --- frame/contracts/src/exec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index e2694a68f6995..feffed5b9849d 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -907,8 +907,8 @@ where // Every non delegate call or instantiate also optionally transfers the balance. self.initial_transfer()?; - let code_hash = *executable.code_hash(); - let call_span = T::TraceableCallSpan::new(&code_hash, entry_point, &input_data); + let call_span = + T::TraceableCallSpan::new(executable.code_hash(), entry_point, &input_data); // Call into the Wasm blob. let output = executable From 2f8793f1138dac285a350a10e23c3ee895b468ab Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 23:38:02 +0200 Subject: [PATCH 09/24] fix comment --- frame/contracts/src/call_observability.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/call_observability.rs b/frame/contracts/src/call_observability.rs index e3e4468dc8549..fd8fb42161638 100644 --- a/frame/contracts/src/call_observability.rs +++ b/frame/contracts/src/call_observability.rs @@ -8,7 +8,10 @@ pub trait CallSpan where Self: Sized, { - /// A new call span is created just before the execution of a contract, to capture the call. + /// Creates a new call span to encompass the upcoming contract execution. + /// + /// This method is invoked just before the execution of a contract and + /// marks the beginning of a traceable span of execution. /// /// # Arguments /// From b9ecef5f35aec0bba6cce3cdc0c28e86ddf2b10f Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 18 Aug 2023 23:53:22 +0200 Subject: [PATCH 10/24] clippy --- frame/contracts/src/tests/test_observability.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/contracts/src/tests/test_observability.rs b/frame/contracts/src/tests/test_observability.rs index 1fdb1ba07d69d..f51edc8c4c939 100644 --- a/frame/contracts/src/tests/test_observability.rs +++ b/frame/contracts/src/tests/test_observability.rs @@ -32,21 +32,21 @@ impl CallSpan for TestCallSpan { ) -> 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.clone(), call: entry_point, input: input_data.to_vec() } + TestCallSpan { code_hash: *code_hash, call: entry_point, input: input_data.to_vec() } } fn after_call(self, output: &ExecReturnValue) { DEBUG_EXECUTION_TRACE.with(|d| { d.borrow_mut().push(DebugFrame { - code_hash: self.code_hash.clone(), + code_hash: self.code_hash, call: self.call, - input: self.input.clone(), + input: self.input, result: Some(output.data.clone()), }) }); From 69fce499e6314d32d764b267e2c41367ce138efa Mon Sep 17 00:00:00 2001 From: pgherveou Date: Sun, 20 Aug 2023 11:07:41 +0200 Subject: [PATCH 11/24] update naming --- bin/node/runtime/src/lib.rs | 2 +- frame/contracts/src/call_observability.rs | 25 +++++++++++++------ frame/contracts/src/exec.rs | 5 ++-- frame/contracts/src/lib.rs | 7 +++--- frame/contracts/src/tests.rs | 4 +-- .../contracts/src/tests/test_observability.rs | 11 +++++--- 6 files changed, 33 insertions(+), 21 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index a151990c32bac..1210de7cd00bc 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1263,7 +1263,7 @@ impl pallet_contracts::Config for Runtime { type Migrations = pallet_contracts::migration::codegen::BenchMigrations; type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; - type TraceableCallSpan = (); + type Tracing = (); } impl pallet_sudo::Config for Runtime { diff --git a/frame/contracts/src/call_observability.rs b/frame/contracts/src/call_observability.rs index fd8fb42161638..c847fe2b39768 100644 --- a/frame/contracts/src/call_observability.rs +++ b/frame/contracts/src/call_observability.rs @@ -4,13 +4,12 @@ use pallet_contracts_primitives::ExecReturnValue; /// CallSpan defines methods to capture contract calls, enabling external observers to /// measure, trace, and react to contract interactions. -pub trait CallSpan -where - Self: Sized, -{ +pub trait Tracing { + type CallSpan: CallSpan; + /// Creates a new call span to encompass the upcoming contract execution. /// - /// This method is invoked just before the execution of a contract and + /// This method should be invoked just before the execution of a contract and /// marks the beginning of a traceable span of execution. /// /// # Arguments @@ -18,8 +17,14 @@ where /// * `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(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) -> Self; + fn call_span( + code_hash: &CodeHash, + entry_point: ExportedFunction, + input_data: &[u8], + ) -> Self::CallSpan; +} +pub trait CallSpan { /// Called just after the execution of a contract. /// /// # Arguments @@ -28,11 +33,15 @@ where fn after_call(self, output: &ExecReturnValue); } -impl CallSpan for () { - fn new(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { +impl Tracing for () { + type CallSpan = (); + + fn 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 feffed5b9849d..1471fe1105f83 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -16,7 +16,7 @@ // limitations under the License. use crate::{ - call_observability::CallSpan, + call_observability::{CallSpan, Tracing}, gas::GasMeter, storage::{self, meter::Diff, WriteOutcome}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, @@ -907,8 +907,7 @@ where // Every non delegate call or instantiate also optionally transfers the balance. self.initial_transfer()?; - let call_span = - T::TraceableCallSpan::new(executable.code_hash(), entry_point, &input_data); + let call_span = T::Tracing::call_span(executable.code_hash(), entry_point, &input_data); // Call into the Wasm blob. let output = executable diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index ffa6966bbf424..ba20a172993aa 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -108,7 +108,6 @@ use crate::{ storage::{meter::Meter as StorageMeter, ContractInfo, DeletionQueueManager}, wasm::{CodeInfo, WasmBlob}, }; -use call_observability::CallSpan; use codec::{Codec, Decode, Encode, HasCompact, MaxEncodedLen}; use environmental::*; use frame_support::{ @@ -138,6 +137,7 @@ use sp_std::{fmt::Debug, prelude::*}; pub use crate::{ address::{AddressGenerator, DefaultAddressGenerator}, + call_observability::Tracing, exec::Frame, migration::{MigrateSequence, Migration, NoopMigration}, pallet::*, @@ -354,9 +354,8 @@ pub mod pallet { /// ``` type Migrations: MigrateSequence; - /// TraceableCallSpan defines methods to capture and trace contract calls - /// for improved observability. - type TraceableCallSpan: CallSpan; + /// Defines methods to capture and trace contract calls for improved observability. + type Tracing: Tracing; } #[pallet::hooks] diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 3d894d32b6261..78637d165080b 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -19,7 +19,7 @@ mod pallet_dummy; mod test_observability; use self::{ - test_observability::TestCallSpan, + test_observability::TestTracing, test_utils::{ensure_stored, expected_deposit, hash}, }; use crate::{ @@ -482,7 +482,7 @@ impl Config for Test { type Migrations = crate::migration::codegen::BenchMigrations; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; type MaxDelegateDependencies = MaxDelegateDependencies; - type TraceableCallSpan = TestCallSpan; + type Tracing = TestTracing; } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]); diff --git a/frame/contracts/src/tests/test_observability.rs b/frame/contracts/src/tests/test_observability.rs index f51edc8c4c939..403d7c7037160 100644 --- a/frame/contracts/src/tests/test_observability.rs +++ b/frame/contracts/src/tests/test_observability.rs @@ -1,4 +1,4 @@ -use crate::call_observability::{CallSpan, ExportedFunction}; +use crate::call_observability::{CallSpan, ExportedFunction, Tracing}; use super::*; use frame_support::traits::Currency; @@ -18,14 +18,17 @@ thread_local! { static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); } +pub struct TestTracing; pub struct TestCallSpan { code_hash: CodeHash, call: ExportedFunction, input: Vec, } -impl CallSpan for TestCallSpan { - fn new( +impl Tracing for TestTracing { + type CallSpan = TestCallSpan; + + fn call_span( code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8], @@ -40,7 +43,9 @@ impl CallSpan for TestCallSpan { }); TestCallSpan { code_hash: *code_hash, call: entry_point, input: input_data.to_vec() } } +} +impl CallSpan for TestCallSpan { fn after_call(self, output: &ExecReturnValue) { DEBUG_EXECUTION_TRACE.with(|d| { d.borrow_mut().push(DebugFrame { From 48b009d22dd879cf0d37e1f23640301252108281 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Sun, 20 Aug 2023 19:59:48 +0200 Subject: [PATCH 12/24] Rename file --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 4 ++-- frame/contracts/src/tests.rs | 4 ++-- .../src/tests/{test_observability.rs => test_tracing.rs} | 2 +- frame/contracts/src/{call_observability.rs => tracing.rs} | 0 5 files changed, 6 insertions(+), 6 deletions(-) rename frame/contracts/src/tests/{test_observability.rs => test_tracing.rs} (97%) rename frame/contracts/src/{call_observability.rs => tracing.rs} (100%) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 1471fe1105f83..feae2494b27d6 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -16,7 +16,7 @@ // limitations under the License. use crate::{ - call_observability::{CallSpan, Tracing}, + tracing::{CallSpan, Tracing}, gas::GasMeter, storage::{self, meter::Diff, WriteOutcome}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index ba20a172993aa..4df6bcd095470 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -95,7 +95,7 @@ mod schedule; mod storage; mod wasm; -pub mod call_observability; +pub mod tracing; pub mod chain_extension; pub mod migration; pub mod weights; @@ -137,7 +137,7 @@ use sp_std::{fmt::Debug, prelude::*}; pub use crate::{ address::{AddressGenerator, DefaultAddressGenerator}, - call_observability::Tracing, + tracing::Tracing, exec::Frame, migration::{MigrateSequence, Migration, NoopMigration}, pallet::*, diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 78637d165080b..ddeea2a8c7d53 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -16,10 +16,10 @@ // limitations under the License. mod pallet_dummy; -mod test_observability; +mod test_tracing; use self::{ - test_observability::TestTracing, + test_tracing::TestTracing, test_utils::{ensure_stored, expected_deposit, hash}, }; use crate::{ diff --git a/frame/contracts/src/tests/test_observability.rs b/frame/contracts/src/tests/test_tracing.rs similarity index 97% rename from frame/contracts/src/tests/test_observability.rs rename to frame/contracts/src/tests/test_tracing.rs index 403d7c7037160..e46bc7db99d6b 100644 --- a/frame/contracts/src/tests/test_observability.rs +++ b/frame/contracts/src/tests/test_tracing.rs @@ -1,4 +1,4 @@ -use crate::call_observability::{CallSpan, ExportedFunction, Tracing}; +use crate::tracing::{CallSpan, ExportedFunction, Tracing}; use super::*; use frame_support::traits::Currency; diff --git a/frame/contracts/src/call_observability.rs b/frame/contracts/src/tracing.rs similarity index 100% rename from frame/contracts/src/call_observability.rs rename to frame/contracts/src/tracing.rs From 9f7aae0645df2ba23382e1cf6adac1679f306698 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 21 Aug 2023 07:37:24 +0200 Subject: [PATCH 13/24] fmt fixes --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 4 ++-- frame/contracts/src/tests/test_tracing.rs | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index feae2494b27d6..f6fdc8602ce6a 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -16,9 +16,9 @@ // limitations under the License. use crate::{ - tracing::{CallSpan, Tracing}, gas::GasMeter, storage::{self, meter::Diff, WriteOutcome}, + tracing::{CallSpan, Tracing}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, DebugBufferVec, Determinism, Error, Event, Nonce, Origin, Pallet as Contracts, Schedule, WasmBlob, LOG_TARGET, diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 4df6bcd095470..47597c56b6a3d 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -95,9 +95,9 @@ mod schedule; mod storage; mod wasm; -pub mod tracing; pub mod chain_extension; pub mod migration; +pub mod tracing; pub mod weights; #[cfg(test)] @@ -137,11 +137,11 @@ use sp_std::{fmt::Debug, prelude::*}; pub use crate::{ address::{AddressGenerator, DefaultAddressGenerator}, - tracing::Tracing, exec::Frame, migration::{MigrateSequence, Migration, NoopMigration}, pallet::*, schedule::{HostFnWeights, InstructionWeights, Limits, Schedule}, + tracing::Tracing, wasm::Determinism, }; pub use weights::WeightInfo; diff --git a/frame/contracts/src/tests/test_tracing.rs b/frame/contracts/src/tests/test_tracing.rs index e46bc7db99d6b..251b1316d303e 100644 --- a/frame/contracts/src/tests/test_tracing.rs +++ b/frame/contracts/src/tests/test_tracing.rs @@ -1,6 +1,5 @@ -use crate::tracing::{CallSpan, ExportedFunction, Tracing}; - use super::*; +use crate::tracing::{CallSpan, ExportedFunction, Tracing}; use frame_support::traits::Currency; use pallet_contracts_primitives::ExecReturnValue; use pretty_assertions::assert_eq; From 807334df02a3c9a9c3b6304a1f87902a7fdfd04b Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 21 Aug 2023 07:45:24 +0200 Subject: [PATCH 14/24] rename --- frame/contracts/src/exec.rs | 3 ++- frame/contracts/src/tests/test_tracing.rs | 2 +- frame/contracts/src/tracing.rs | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index f6fdc8602ce6a..eb7ace76a6f8d 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -907,7 +907,8 @@ where // Every non delegate call or instantiate also optionally transfers the balance. self.initial_transfer()?; - let call_span = T::Tracing::call_span(executable.code_hash(), entry_point, &input_data); + let call_span = + T::Tracing::new_call_span(executable.code_hash(), entry_point, &input_data); // Call into the Wasm blob. let output = executable diff --git a/frame/contracts/src/tests/test_tracing.rs b/frame/contracts/src/tests/test_tracing.rs index 251b1316d303e..9a08166947967 100644 --- a/frame/contracts/src/tests/test_tracing.rs +++ b/frame/contracts/src/tests/test_tracing.rs @@ -27,7 +27,7 @@ pub struct TestCallSpan { impl Tracing for TestTracing { type CallSpan = TestCallSpan; - fn call_span( + fn new_call_span( code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8], diff --git a/frame/contracts/src/tracing.rs b/frame/contracts/src/tracing.rs index c847fe2b39768..02234abd77f49 100644 --- a/frame/contracts/src/tracing.rs +++ b/frame/contracts/src/tracing.rs @@ -17,7 +17,7 @@ pub trait Tracing { /// * `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 call_span( + fn new_call_span( code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8], @@ -36,7 +36,7 @@ pub trait CallSpan { impl Tracing for () { type CallSpan = (); - fn call_span(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { + 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:?}") } } From 0b1ecc11e8baa29337f85887a95f79d7fd465c4c Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 21 Aug 2023 11:57:10 +0200 Subject: [PATCH 15/24] Move tracing behind umbrella Debugging trait --- frame/contracts/src/{tracing.rs => debug.rs} | 5 +++++ frame/contracts/src/exec.rs | 4 ++-- frame/contracts/src/lib.rs | 8 +++++--- frame/contracts/src/tests.rs | 6 +++--- .../src/tests/{test_tracing.rs => test_debug.rs} | 6 +++--- 5 files changed, 18 insertions(+), 11 deletions(-) rename frame/contracts/src/{tracing.rs => debug.rs} (89%) rename frame/contracts/src/tests/{test_tracing.rs => test_debug.rs} (96%) diff --git a/frame/contracts/src/tracing.rs b/frame/contracts/src/debug.rs similarity index 89% rename from frame/contracts/src/tracing.rs rename to frame/contracts/src/debug.rs index 02234abd77f49..239a7ff0d6250 100644 --- a/frame/contracts/src/tracing.rs +++ b/frame/contracts/src/debug.rs @@ -2,6 +2,11 @@ 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 Debugging: Tracing {} + +impl Debugging for V where V: Tracing {} + /// CallSpan defines methods to capture contract calls, enabling external observers to /// measure, trace, and react to contract interactions. pub trait Tracing { diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index eb7ace76a6f8d..1ba44220ff8dc 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -16,9 +16,9 @@ // limitations under the License. use crate::{ + debug::{CallSpan, Tracing}, gas::GasMeter, storage::{self, meter::Diff, WriteOutcome}, - tracing::{CallSpan, Tracing}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, DebugBufferVec, Determinism, Error, Event, Nonce, Origin, Pallet as Contracts, Schedule, WasmBlob, LOG_TARGET, @@ -908,7 +908,7 @@ where self.initial_transfer()?; let call_span = - T::Tracing::new_call_span(executable.code_hash(), entry_point, &input_data); + T::Debug::new_call_span(executable.code_hash(), entry_point, &input_data); // Call into the Wasm blob. let output = executable diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 47597c56b6a3d..f17e5f70bede5 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 tracing; pub mod weights; #[cfg(test)] @@ -137,11 +137,11 @@ use sp_std::{fmt::Debug, prelude::*}; pub use crate::{ address::{AddressGenerator, DefaultAddressGenerator}, + debug::Tracing, exec::Frame, migration::{MigrateSequence, Migration, NoopMigration}, pallet::*, schedule::{HostFnWeights, InstructionWeights, Limits, Schedule}, - tracing::Tracing, wasm::Determinism, }; pub use weights::WeightInfo; @@ -182,6 +182,8 @@ const LOG_TARGET: &str = "runtime::contracts"; #[frame_support::pallet] pub mod pallet { + use crate::debug::Debugging; + use super::*; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; @@ -355,7 +357,7 @@ pub mod pallet { type Migrations: MigrateSequence; /// Defines methods to capture and trace contract calls for improved observability. - type Tracing: Tracing; + type Debug: Debugging; } #[pallet::hooks] diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index ddeea2a8c7d53..ed3b779b881e8 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -16,10 +16,10 @@ // limitations under the License. mod pallet_dummy; -mod test_tracing; +mod test_debug; use self::{ - test_tracing::TestTracing, + test_debug::TestDebug, test_utils::{ensure_stored, expected_deposit, hash}, }; use crate::{ @@ -482,7 +482,7 @@ impl Config for Test { type Migrations = crate::migration::codegen::BenchMigrations; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; type MaxDelegateDependencies = MaxDelegateDependencies; - type Tracing = TestTracing; + type Debug = TestDebug; } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]); diff --git a/frame/contracts/src/tests/test_tracing.rs b/frame/contracts/src/tests/test_debug.rs similarity index 96% rename from frame/contracts/src/tests/test_tracing.rs rename to frame/contracts/src/tests/test_debug.rs index 9a08166947967..ba936a4588d18 100644 --- a/frame/contracts/src/tests/test_tracing.rs +++ b/frame/contracts/src/tests/test_debug.rs @@ -1,5 +1,5 @@ use super::*; -use crate::tracing::{CallSpan, ExportedFunction, Tracing}; +use crate::debug::{CallSpan, ExportedFunction, Tracing}; use frame_support::traits::Currency; use pallet_contracts_primitives::ExecReturnValue; use pretty_assertions::assert_eq; @@ -17,14 +17,14 @@ thread_local! { static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); } -pub struct TestTracing; +pub struct TestDebug; pub struct TestCallSpan { code_hash: CodeHash, call: ExportedFunction, input: Vec, } -impl Tracing for TestTracing { +impl Tracing for TestDebug { type CallSpan = TestCallSpan; fn new_call_span( From baac6d406ac4a10f4c0ea7a526012e184bd034d0 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 21 Aug 2023 11:59:32 +0200 Subject: [PATCH 16/24] fix --- bin/node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 1210de7cd00bc..8750e5ef62e0d 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1263,7 +1263,7 @@ impl pallet_contracts::Config for Runtime { type Migrations = pallet_contracts::migration::codegen::BenchMigrations; type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; - type Tracing = (); + type Debug = (); } impl pallet_sudo::Config for Runtime { From 73f8a85ff95b54443e945fd5c2719ce80290cd72 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 21 Aug 2023 12:00:39 +0200 Subject: [PATCH 17/24] fix comment --- frame/contracts/src/debug.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/debug.rs b/frame/contracts/src/debug.rs index 239a7ff0d6250..845c6a3de3f6f 100644 --- a/frame/contracts/src/debug.rs +++ b/frame/contracts/src/debug.rs @@ -7,7 +7,7 @@ pub trait Debugging: Tracing {} impl Debugging for V where V: Tracing {} -/// CallSpan defines methods to capture contract calls, enabling external observers to +/// Defines methods to capture contract calls, enabling external observers to /// measure, trace, and react to contract interactions. pub trait Tracing { type CallSpan: CallSpan; From c4e52cf943614090e2ba753705ff145dd58c1f15 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 21 Aug 2023 12:03:57 +0200 Subject: [PATCH 18/24] reorder imports --- frame/contracts/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index f17e5f70bede5..a6be10e242528 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -182,9 +182,8 @@ const LOG_TARGET: &str = "runtime::contracts"; #[frame_support::pallet] pub mod pallet { - use crate::debug::Debugging; - use super::*; + use crate::debug::Debugging; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; use sp_runtime::Perbill; From acfcb5a1fe010517a2a702aa38de38ef31b57697 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 21 Aug 2023 12:06:08 +0200 Subject: [PATCH 19/24] comment --- frame/contracts/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a6be10e242528..1e9b23117532e 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -355,7 +355,7 @@ pub mod pallet { /// ``` type Migrations: MigrateSequence; - /// Defines methods to capture and trace contract calls for improved observability. + /// Type that provides debug handling for the contract execution process. type Debug: Debugging; } From 33d8ec29874b9c003fbe8f0b135c1f6adfa08ee6 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 21 Aug 2023 12:12:35 +0200 Subject: [PATCH 20/24] update doc --- frame/contracts/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 1e9b23117532e..cf0fa84b51174 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -356,6 +356,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 + /// trait. This implementation offers additional logging when the log target + /// "runtime::contracts" is set to trace. type Debug: Debugging; } From 13bd9b0e7a6e5aef2a9edc6924ad62b30d4950c8 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 22 Aug 2023 17:57:20 +0200 Subject: [PATCH 21/24] add missing doc --- frame/contracts/src/debug.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/contracts/src/debug.rs b/frame/contracts/src/debug.rs index 845c6a3de3f6f..6929b6b786e12 100644 --- a/frame/contracts/src/debug.rs +++ b/frame/contracts/src/debug.rs @@ -29,6 +29,7 @@ pub trait Tracing { ) -> Self::CallSpan; } +/// Defines a span of execution for a contract call. pub trait CallSpan { /// Called just after the execution of a contract. /// From 6b3d1655ff0c21420de13fd4140d747ab49088ff Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 22 Aug 2023 18:10:09 +0200 Subject: [PATCH 22/24] add missing doc --- frame/contracts/src/debug.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/contracts/src/debug.rs b/frame/contracts/src/debug.rs index 6929b6b786e12..48ab70c1b68b0 100644 --- a/frame/contracts/src/debug.rs +++ b/frame/contracts/src/debug.rs @@ -10,6 +10,7 @@ impl Debugging 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. From 8bdfe8d2252d65341dc467d9fc06b127f23fdc93 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Wed, 23 Aug 2023 14:05:21 +0200 Subject: [PATCH 23/24] Update Debugging -> Debugger --- frame/contracts/src/debug.rs | 4 ++-- frame/contracts/src/lib.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/contracts/src/debug.rs b/frame/contracts/src/debug.rs index 48ab70c1b68b0..a92f428c8f8a4 100644 --- a/frame/contracts/src/debug.rs +++ b/frame/contracts/src/debug.rs @@ -3,9 +3,9 @@ use crate::{CodeHash, Config, LOG_TARGET}; use pallet_contracts_primitives::ExecReturnValue; /// Umbrella trait for all interfaces that serves for debugging. -pub trait Debugging: Tracing {} +pub trait Debugger: Tracing {} -impl Debugging for V where V: 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. diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index cf0fa84b51174..dd1db6305b0f9 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -183,7 +183,7 @@ const LOG_TARGET: &str = "runtime::contracts"; #[frame_support::pallet] pub mod pallet { use super::*; - use crate::debug::Debugging; + use crate::debug::Debugger; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; use sp_runtime::Perbill; @@ -361,7 +361,7 @@ pub mod pallet { /// 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: Debugging; + type Debug: Debugger; } #[pallet::hooks] From 4ae8c35dc2c823aba68d95f4c210651249f10e81 Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Wed, 23 Aug 2023 16:57:12 +0200 Subject: [PATCH 24/24] Update bin/node/runtime/Cargo.toml --- bin/node/runtime/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index 3b9236bd62573..0c1514375481e 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -372,4 +372,4 @@ try-runtime = [ "pallet-vesting/try-runtime", "pallet-whitelist/try-runtime", "sp-runtime/try-runtime", -] \ No newline at end of file +]