From 28e906dffcaa91e85f59aff628d953ebeb036ae2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Sat, 5 Aug 2023 16:15:03 +0200 Subject: [PATCH] Cross-contract calling: simple debugger (#14678) * Provide basic breakpoints * Rename to Observer * Rename feature. Single trait. Borrow-checker * : frame_system::Config * Confused type name * Minor bugs * pub trait * No unnecessary cloning * Make node compile with all features * Move everything debug-related to a single module * Add docs and implementation for () * fmt * Make it feature-gated or for tests * Prepare testing kit * Testcase * Fmt * Use feature in dev-deps * ? * feature propagation * AAAA * lol, that doesn't make much sense to me * Turn on * clippy * Remove self dep * fmt, feature-gating test * Noop to trigger CI * idk * add feature to pipeline * Corrupt test to see if it is actually being run * Revert change * Doc for conf type * Review * Imports * ... * Remove debug for kitchen-sink * Move test * Fix imports * I must have already tried this one... --- bin/node/runtime/Cargo.toml | 1 + bin/node/runtime/src/lib.rs | 2 + frame/contracts/Cargo.toml | 3 +- frame/contracts/src/exec.rs | 14 ++- frame/contracts/src/lib.rs | 9 ++ frame/contracts/src/tests.rs | 3 + frame/contracts/src/tests/unsafe_debug.rs | 138 ++++++++++++++++++++++ frame/contracts/src/unsafe_debug.rs | 47 ++++++++ scripts/ci/gitlab/pipeline/test.yml | 2 +- 9 files changed, 216 insertions(+), 3 deletions(-) create mode 100644 frame/contracts/src/tests/unsafe_debug.rs create mode 100644 frame/contracts/src/unsafe_debug.rs diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index 4a9bb9a769597..de0ec1a5489a8 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -373,3 +373,4 @@ try-runtime = [ "frame-election-provider-support/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 69dedfb599583..29aaaf5fdad5a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1258,6 +1258,8 @@ 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 = (); } impl pallet_sudo::Config for Runtime { diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index feaa7cf73fbcc..a5c309adc97bc 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -112,5 +112,6 @@ try-runtime = [ "pallet-proxy/try-runtime", "pallet-timestamp/try-runtime", "pallet-utility/try-runtime", - "sp-runtime/try-runtime" + "sp-runtime/try-runtime", ] +unsafe-debug = [] diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 6203b31f67a5b..c540e932cc24b 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -15,6 +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::{ gas::GasMeter, storage::{self, DepositAccount, WriteOutcome}, @@ -344,7 +346,7 @@ pub trait Ext: sealing::Sealed { } /// Describes the different functions that can be exported by an [`Executable`]. -#[derive(Clone, Copy, PartialEq)] +#[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum ExportedFunction { /// The constructor function which is executed on deployment of a contract. Constructor, @@ -904,11 +906,21 @@ 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()) + }; + // 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); + // Avoid useless work that would be reverted anyways. if output.did_revert() { return Ok(output) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 158e4802cf186..0186e190fb785 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -97,6 +97,7 @@ mod wasm; pub mod chain_extension; pub mod migration; +pub mod unsafe_debug; pub mod weights; #[cfg(test)] @@ -351,6 +352,14 @@ pub mod pallet { /// type Migrations = (v10::Migration,); /// ``` 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; } #[pallet::hooks] diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 2e6161dcff544..3132b8e39f7da 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -16,6 +16,7 @@ // limitations under the License. mod pallet_dummy; +mod unsafe_debug; use self::test_utils::{ensure_stored, expected_deposit, hash}; use crate::{ @@ -464,6 +465,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; } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]); diff --git a/frame/contracts/src/tests/unsafe_debug.rs b/frame/contracts/src/tests/unsafe_debug.rs new file mode 100644 index 0000000000000..160a6ed6dc54f --- /dev/null +++ b/frame/contracts/src/tests/unsafe_debug.rs @@ -0,0 +1,138 @@ +#![cfg(feature = "unsafe-debug")] + +use super::*; +use crate::unsafe_debug::{ExecutionObserver, ExportedFunction}; +use frame_support::traits::Currency; +use pallet_contracts_primitives::ExecReturnValue; +use pretty_assertions::assert_eq; +use std::cell::RefCell; + +#[derive(Clone, PartialEq, Eq, Debug)] +struct DebugFrame { + code_hash: CodeHash, + call: ExportedFunction, + input: Vec, + result: Option>, +} + +thread_local! { + static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); +} + +pub struct TestDebugger; + +impl ExecutionObserver> for TestDebugger { + fn before_call(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { + DEBUG_EXECUTION_TRACE.with(|d| { + d.borrow_mut().push(DebugFrame { + code_hash: code_hash.clone(), + call: entry_point, + input: input_data.to_vec(), + result: None, + }) + }); + } + + fn after_call( + code_hash: &CodeHash, + entry_point: ExportedFunction, + input_data: Vec, + output: &ExecReturnValue, + ) { + DEBUG_EXECUTION_TRACE.with(|d| { + d.borrow_mut().push(DebugFrame { + code_hash: code_hash.clone(), + call: entry_point, + input: input_data, + result: Some(output.data.clone()), + }) + }); + } +} + +#[test] +fn unsafe_debugging_works() { + let (wasm_caller, code_hash_caller) = compile_module::("call").unwrap(); + let (wasm_callee, code_hash_callee) = compile_module::("store_call").unwrap(); + + fn current_stack() -> Vec { + DEBUG_EXECUTION_TRACE.with(|stack| stack.borrow().clone()) + } + + fn deploy(wasm: Vec) -> AccountId32 { + Contracts::bare_instantiate( + ALICE, + 0, + GAS_LIMIT, + None, + Code::Upload(wasm), + vec![], + vec![], + DebugInfo::Skip, + CollectEvents::Skip, + ) + .result + .unwrap() + .account_id + } + + fn constructor_frame(hash: CodeHash, after: bool) -> DebugFrame { + DebugFrame { + code_hash: hash, + call: ExportedFunction::Constructor, + input: vec![], + result: if after { Some(vec![]) } else { None }, + } + } + + fn call_frame(hash: CodeHash, args: Vec, after: bool) -> DebugFrame { + DebugFrame { + code_hash: hash, + call: ExportedFunction::Call, + input: args, + result: if after { Some(vec![]) } else { None }, + } + } + + ExtBuilder::default().existential_deposit(200).build().execute_with(|| { + let _ = Balances::deposit_creating(&ALICE, 1_000_000); + + assert_eq!(current_stack(), vec![]); + + let addr_caller = deploy(wasm_caller); + let addr_callee = deploy(wasm_callee); + + assert_eq!( + current_stack(), + vec![ + constructor_frame(code_hash_caller, false), + constructor_frame(code_hash_caller, true), + constructor_frame(code_hash_callee, false), + constructor_frame(code_hash_callee, true), + ] + ); + + let main_args = (100u32, &addr_callee).encode(); + let inner_args = (100u32).encode(); + + assert_ok!(Contracts::call( + RuntimeOrigin::signed(ALICE), + addr_caller, + 0, + GAS_LIMIT, + None, + main_args.clone() + )); + + let stack_top = current_stack()[4..].to_vec(); + assert_eq!( + stack_top, + vec![ + call_frame(code_hash_caller, main_args.clone(), false), + call_frame(code_hash_callee, inner_args.clone(), false), + call_frame(code_hash_callee, inner_args, true), + call_frame(code_hash_caller, main_args, true), + ] + ); + }); +} diff --git a/frame/contracts/src/unsafe_debug.rs b/frame/contracts/src/unsafe_debug.rs new file mode 100644 index 0000000000000..418af5e605d28 --- /dev/null +++ b/frame/contracts/src/unsafe_debug.rs @@ -0,0 +1,47 @@ +#![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 69d53012f79e7..1b246258ff768 100644 --- a/scripts/ci/gitlab/pipeline/test.yml +++ b/scripts/ci/gitlab/pipeline/test.yml @@ -221,7 +221,7 @@ test-linux-stable: --locked --release --verbose - --features runtime-benchmarks,try-runtime,experimental + --features runtime-benchmarks,try-runtime,experimental,unsafe-debug --manifest-path ./bin/node/cli/Cargo.toml --partition count:${CI_NODE_INDEX}/${CI_NODE_TOTAL} # we need to update cache only from one job