From 136155609ee40081a8b4810510d55a41e2340561 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Fri, 28 Jul 2023 16:26:27 +0200 Subject: [PATCH 01/39] Provide basic breakpoints --- frame/contracts/Cargo.toml | 1 + frame/contracts/src/exec.rs | 13 ++++++++ frame/contracts/src/execution_reporter.rs | 36 +++++++++++++++++++++++ frame/contracts/src/lib.rs | 7 ++++- 4 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 frame/contracts/src/execution_reporter.rs diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index f23e44741a6cb..1d93e27f934f6 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -89,3 +89,4 @@ runtime-benchmarks = [ "wasm-instrument", ] try-runtime = ["frame-support/try-runtime"] +execution-debug = [] diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 97fdd17e21aa8..715ab4a5772cb 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 = "execution-debug")] +use crate::execution_reporter::ExecutionReporter; use crate::{ gas::GasMeter, storage::{self, DepositAccount, WriteOutcome}, @@ -903,11 +905,22 @@ where // Every non delegate call or instantiate also optionally transfers the balance. self.initial_transfer()?; + #[cfg(feature = "execution-debug")] + T::ExecutionReporter::before_call(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 = "execution-debug")] + T::ExecutionReporter::after_call( + executable.code_hash(), + entry_point, + &input_data, + &output, + ); + // Avoid useless work that would be reverted anyways. if output.did_revert() { return Ok(output) diff --git a/frame/contracts/src/execution_reporter.rs b/frame/contracts/src/execution_reporter.rs new file mode 100644 index 0000000000000..a9a0093eeb372 --- /dev/null +++ b/frame/contracts/src/execution_reporter.rs @@ -0,0 +1,36 @@ +use crate::{exec::ExportedFunction, CodeHash, Config}; +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. +/// +/// 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 ExecutionReporter { + /// 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: &[u8], + _output: &ExecReturnValue, + ) { + } +} diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index bd01bb232ec6d..e74b761caeb71 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -96,6 +96,8 @@ mod storage; mod wasm; pub mod chain_extension; +#[cfg(feature = "execution-debug")] +pub mod execution_reporter; pub mod migration; pub mod weights; @@ -137,7 +139,7 @@ pub use weights::WeightInfo; pub use crate::{ address::{AddressGenerator, DefaultAddressGenerator}, - exec::Frame, + exec::{ExportedFunction, Frame}, migration::{MigrateSequence, Migration, NoopMigration}, pallet::*, schedule::{HostFnWeights, InstructionWeights, Limits, Schedule}, @@ -350,6 +352,9 @@ pub mod pallet { /// type Migrations = (v10::Migration,); /// ``` type Migrations: MigrateSequence; + + #[cfg(feature = "execution-debug")] + type ExecutionReporter: execution_reporter::ExecutionReporter>; } #[pallet::hooks] From 4ce4d351fa1ebc2f630dc629d540a179a5d7f467 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Fri, 28 Jul 2023 16:27:57 +0200 Subject: [PATCH 02/39] Rename to Observer --- frame/contracts/src/exec.rs | 6 +++--- .../src/{execution_reporter.rs => execution_observer.rs} | 2 +- frame/contracts/src/lib.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) rename frame/contracts/src/{execution_reporter.rs => execution_observer.rs} (97%) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 715ab4a5772cb..c7d4f1e87b89b 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -16,7 +16,7 @@ // limitations under the License. #[cfg(feature = "execution-debug")] -use crate::execution_reporter::ExecutionReporter; +use crate::execution_observer::ExecutionObserver; use crate::{ gas::GasMeter, storage::{self, DepositAccount, WriteOutcome}, @@ -906,7 +906,7 @@ where self.initial_transfer()?; #[cfg(feature = "execution-debug")] - T::ExecutionReporter::before_call(executable.code_hash(), entry_point, &input_data); + T::ExecutionObserver::before_call(executable.code_hash(), entry_point, &input_data); // Call into the Wasm blob. let output = executable @@ -914,7 +914,7 @@ where .map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?; #[cfg(feature = "execution-debug")] - T::ExecutionReporter::after_call( + T::ExecutionObserver::after_call( executable.code_hash(), entry_point, &input_data, diff --git a/frame/contracts/src/execution_reporter.rs b/frame/contracts/src/execution_observer.rs similarity index 97% rename from frame/contracts/src/execution_reporter.rs rename to frame/contracts/src/execution_observer.rs index a9a0093eeb372..02bc2e4947756 100644 --- a/frame/contracts/src/execution_reporter.rs +++ b/frame/contracts/src/execution_observer.rs @@ -8,7 +8,7 @@ use pallet_contracts_primitives::ExecReturnValue; /// /// 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 ExecutionReporter { +pub trait ExecutionObserver { /// Called just before the execution of a contract. /// /// # Arguments diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index e74b761caeb71..4e8c73a849cbd 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -97,7 +97,7 @@ mod wasm; pub mod chain_extension; #[cfg(feature = "execution-debug")] -pub mod execution_reporter; +pub mod execution_observer; pub mod migration; pub mod weights; @@ -354,7 +354,7 @@ pub mod pallet { type Migrations: MigrateSequence; #[cfg(feature = "execution-debug")] - type ExecutionReporter: execution_reporter::ExecutionReporter>; + type ExecutionObserver: execution_observer::ExecutionObserver>; } #[pallet::hooks] From f1bbb0c0b756a5f7be3d7cadcc0f7d0beb96ce60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Mon, 31 Jul 2023 07:44:49 +0200 Subject: [PATCH 03/39] Rename feature. Single trait. Borrow-checker --- frame/contracts/Cargo.toml | 2 +- frame/contracts/src/exec.rs | 20 +++++++++----------- frame/contracts/src/lib.rs | 11 ++++++++--- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 1d93e27f934f6..93ee7440ed8bb 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -89,4 +89,4 @@ runtime-benchmarks = [ "wasm-instrument", ] try-runtime = ["frame-support/try-runtime"] -execution-debug = [] +unsafe-debug = [] diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index c7d4f1e87b89b..4f7354b4e3e05 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -15,7 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#[cfg(feature = "execution-debug")] +#[cfg(feature = "unsafe-debug")] use crate::execution_observer::ExecutionObserver; use crate::{ gas::GasMeter, @@ -905,21 +905,19 @@ where // Every non delegate call or instantiate also optionally transfers the balance. self.initial_transfer()?; - #[cfg(feature = "execution-debug")] - T::ExecutionObserver::before_call(executable.code_hash(), entry_point, &input_data); + #[cfg(feature = "unsafe-debug")] + { + let code_hash = executable.code_hash().clone(); + T::UnsafeDebug::before_call(&code_hash, entry_point, &input_data); + } // Call into the Wasm blob. let output = executable - .execute(self, &entry_point, input_data) + .execute(self, &entry_point, input_data.clone()) .map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?; - #[cfg(feature = "execution-debug")] - T::ExecutionObserver::after_call( - executable.code_hash(), - entry_point, - &input_data, - &output, - ); + #[cfg(feature = "unsafe-debug")] + T::UnsafeDebug::after_call(&code_hash, entry_point, &input_data, &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 4e8c73a849cbd..8364c9f40a9da 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -96,7 +96,7 @@ mod storage; mod wasm; pub mod chain_extension; -#[cfg(feature = "execution-debug")] +#[cfg(feature = "unsafe-debug")] pub mod execution_observer; pub mod migration; pub mod weights; @@ -159,6 +159,11 @@ type DebugBufferVec = BoundedVec::MaxDebugBufferLen>; type EventRecordOf = EventRecord<::RuntimeEvent, ::Hash>; +#[cfg(feature = "unsafe-debug")] +trait UnsafeDebug: execution_observer::ExecutionObserver> {} +#[cfg(feature = "unsafe-debug")] +impl UnsafeDebug for D where D: execution_observer::ExecutionObserver> {} + /// The old weight type. /// /// This is a copy of the [`frame_support::weights::OldWeight`] type since the contracts pallet @@ -353,8 +358,8 @@ pub mod pallet { /// ``` type Migrations: MigrateSequence; - #[cfg(feature = "execution-debug")] - type ExecutionObserver: execution_observer::ExecutionObserver>; + #[cfg(feature = "unsafe-debug")] + type Debug: UnsafeDebug; } #[pallet::hooks] From 1e8707d395824d07a31145a7081b6cc8d6425a0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Mon, 31 Jul 2023 07:59:34 +0200 Subject: [PATCH 04/39] : frame_system::Config --- frame/contracts/src/lib.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 8364c9f40a9da..51d2e961c79b2 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -160,9 +160,12 @@ type EventRecordOf = EventRecord<::RuntimeEvent, ::Hash>; #[cfg(feature = "unsafe-debug")] -trait UnsafeDebug: execution_observer::ExecutionObserver> {} +trait UnsafeDebug: execution_observer::ExecutionObserver> {} #[cfg(feature = "unsafe-debug")] -impl UnsafeDebug for D where D: execution_observer::ExecutionObserver> {} +impl UnsafeDebug for D where + D: execution_observer::ExecutionObserver> +{ +} /// The old weight type. /// From 2c1dd10c6a651347ac4c1c436a4ce6ab942079da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Mon, 31 Jul 2023 08:12:54 +0200 Subject: [PATCH 05/39] Confused type name --- 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 4f7354b4e3e05..3db9114ea5bb9 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -908,7 +908,7 @@ where #[cfg(feature = "unsafe-debug")] { let code_hash = executable.code_hash().clone(); - T::UnsafeDebug::before_call(&code_hash, entry_point, &input_data); + T::Debug::before_call(&code_hash, entry_point, &input_data); } // Call into the Wasm blob. @@ -917,7 +917,7 @@ where .map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?; #[cfg(feature = "unsafe-debug")] - T::UnsafeDebug::after_call(&code_hash, entry_point, &input_data, &output); + T::Debug::after_call(&code_hash, entry_point, &input_data, &output); // Avoid useless work that would be reverted anyways. if output.did_revert() { From 76c2c8c299d518608585e459ece42f0551dce8c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Mon, 31 Jul 2023 08:26:53 +0200 Subject: [PATCH 06/39] Minor bugs --- frame/contracts/src/exec.rs | 7 +++---- frame/contracts/src/execution_observer.rs | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 3db9114ea5bb9..9fdf08aafeb10 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -906,10 +906,9 @@ where self.initial_transfer()?; #[cfg(feature = "unsafe-debug")] - { - let code_hash = executable.code_hash().clone(); - T::Debug::before_call(&code_hash, entry_point, &input_data); - } + let code_hash = executable.code_hash().clone(); + #[cfg(feature = "unsafe-debug")] + T::Debug::before_call(&code_hash, entry_point, &input_data); // Call into the Wasm blob. let output = executable diff --git a/frame/contracts/src/execution_observer.rs b/frame/contracts/src/execution_observer.rs index 02bc2e4947756..a800d624b0d40 100644 --- a/frame/contracts/src/execution_observer.rs +++ b/frame/contracts/src/execution_observer.rs @@ -1,4 +1,4 @@ -use crate::{exec::ExportedFunction, CodeHash, Config}; +use crate::exec::ExportedFunction; use pallet_contracts_primitives::ExecReturnValue; /// Defines the interface between pallet contracts and the outside observer. From c75d8d51ff7301e27cc645d25485eec8fb2f59e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Mon, 31 Jul 2023 08:59:46 +0200 Subject: [PATCH 07/39] pub trait --- frame/contracts/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 51d2e961c79b2..e8f57fe39c62c 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -160,7 +160,10 @@ type EventRecordOf = EventRecord<::RuntimeEvent, ::Hash>; #[cfg(feature = "unsafe-debug")] -trait UnsafeDebug: execution_observer::ExecutionObserver> {} +pub trait UnsafeDebug: + execution_observer::ExecutionObserver> +{ +} #[cfg(feature = "unsafe-debug")] impl UnsafeDebug for D where D: execution_observer::ExecutionObserver> From 7a89df0ae753b311e20c18409c2e73ebf0b8ae4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 14:33:53 +0200 Subject: [PATCH 08/39] No unnecessary cloning --- frame/contracts/src/exec.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 9fdf08aafeb10..6c80ddf92637e 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -906,17 +906,19 @@ where self.initial_transfer()?; #[cfg(feature = "unsafe-debug")] - let code_hash = executable.code_hash().clone(); - #[cfg(feature = "unsafe-debug")] - T::Debug::before_call(&code_hash, entry_point, &input_data); + let (code_hash, input_clone) = { + let code_hash = executable.code_hash().clone(); + 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.clone()) + .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_data, &output); + T::Debug::after_call(&code_hash, entry_point, &input_clone, &output); // Avoid useless work that would be reverted anyways. if output.did_revert() { From 39bff7dcda1c0f8d592a56988c5b46bd2027e604 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 16:25:15 +0200 Subject: [PATCH 09/39] Make node compile with all features --- bin/node/runtime/Cargo.toml | 3 +++ bin/node/runtime/src/lib.rs | 30 ++++++++++++++++-------------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index 30f0052c6eaa6..558ef171717c4 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -366,3 +366,6 @@ try-runtime = [ "pallet-vesting/try-runtime", "pallet-whitelist/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 94ae7e53237e2..40697af0f405d 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -311,21 +311,21 @@ impl InstanceFilter for ProxyType { ProxyType::Any => true, ProxyType::NonTransfer => !matches!( c, - RuntimeCall::Balances(..) | - RuntimeCall::Assets(..) | - RuntimeCall::Uniques(..) | - RuntimeCall::Nfts(..) | - RuntimeCall::Vesting(pallet_vesting::Call::vested_transfer { .. }) | - RuntimeCall::Indices(pallet_indices::Call::transfer { .. }) + RuntimeCall::Balances(..) + | RuntimeCall::Assets(..) + | RuntimeCall::Uniques(..) + | RuntimeCall::Nfts(..) + | RuntimeCall::Vesting(pallet_vesting::Call::vested_transfer { .. }) + | RuntimeCall::Indices(pallet_indices::Call::transfer { .. }) ), ProxyType::Governance => matches!( c, - RuntimeCall::Democracy(..) | - RuntimeCall::Council(..) | - RuntimeCall::Society(..) | - RuntimeCall::TechnicalCommittee(..) | - RuntimeCall::Elections(..) | - RuntimeCall::Treasury(..) + RuntimeCall::Democracy(..) + | RuntimeCall::Council(..) + | RuntimeCall::Society(..) + | RuntimeCall::TechnicalCommittee(..) + | RuntimeCall::Elections(..) + | RuntimeCall::Treasury(..) ), ProxyType::Staking => { matches!(c, RuntimeCall::Staking(..) | RuntimeCall::FastUnstake(..)) @@ -693,8 +693,8 @@ impl Get> for OffchainRandomBalancing { max => { let seed = sp_io::offchain::random_seed(); let random = ::decode(&mut TrailingZeroInput::new(&seed)) - .expect("input is padded with zeroes; qed") % - max.saturating_add(1); + .expect("input is padded with zeroes; qed") + % max.saturating_add(1); random as usize }, }; @@ -1260,6 +1260,8 @@ impl pallet_contracts::Config for Runtime { type Migrations = (NoopMigration<1>, NoopMigration<2>); type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; + #[cfg(feature = "unsafe-debug")] + type Debug = (); } impl pallet_sudo::Config for Runtime { From bd32939695f65106de47a37af5f6febb730caa17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 16:31:10 +0200 Subject: [PATCH 10/39] Move everything debug-related to a single module --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 15 ++------------- .../{execution_observer.rs => unsafe_debug.rs} | 5 +++++ 3 files changed, 8 insertions(+), 14 deletions(-) rename frame/contracts/src/{execution_observer.rs => unsafe_debug.rs} (87%) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 6c80ddf92637e..9644ab9c73ff4 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -16,7 +16,7 @@ // limitations under the License. #[cfg(feature = "unsafe-debug")] -use crate::execution_observer::ExecutionObserver; +use crate::unsafe_debug::ExecutionObserver; use crate::{ gas::GasMeter, storage::{self, DepositAccount, WriteOutcome}, diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index e8f57fe39c62c..2bc1a8b0a1f4f 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -96,9 +96,9 @@ mod storage; mod wasm; pub mod chain_extension; -#[cfg(feature = "unsafe-debug")] -pub mod execution_observer; pub mod migration; +#[cfg(feature = "unsafe-debug")] +pub mod unsafe_debug; pub mod weights; #[cfg(test)] @@ -159,17 +159,6 @@ type DebugBufferVec = BoundedVec::MaxDebugBufferLen>; type EventRecordOf = EventRecord<::RuntimeEvent, ::Hash>; -#[cfg(feature = "unsafe-debug")] -pub trait UnsafeDebug: - execution_observer::ExecutionObserver> -{ -} -#[cfg(feature = "unsafe-debug")] -impl UnsafeDebug for D where - D: execution_observer::ExecutionObserver> -{ -} - /// The old weight type. /// /// This is a copy of the [`frame_support::weights::OldWeight`] type since the contracts pallet diff --git a/frame/contracts/src/execution_observer.rs b/frame/contracts/src/unsafe_debug.rs similarity index 87% rename from frame/contracts/src/execution_observer.rs rename to frame/contracts/src/unsafe_debug.rs index a800d624b0d40..8970937708bb4 100644 --- a/frame/contracts/src/execution_observer.rs +++ b/frame/contracts/src/unsafe_debug.rs @@ -1,6 +1,11 @@ use crate::exec::ExportedFunction; +use crate::CodeHash; use pallet_contracts_primitives::ExecReturnValue; +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 From a6e6d6e551d6827ac530dec183a7d202b79d87a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 16:35:10 +0200 Subject: [PATCH 11/39] Add docs and implementation for () --- frame/contracts/src/lib.rs | 2 +- frame/contracts/src/unsafe_debug.rs | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 2bc1a8b0a1f4f..2bb8ab0814d47 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -354,7 +354,7 @@ pub mod pallet { type Migrations: MigrateSequence; #[cfg(feature = "unsafe-debug")] - type Debug: UnsafeDebug; + type Debug: unsafe_debug::UnsafeDebug; } #[pallet::hooks] diff --git a/frame/contracts/src/unsafe_debug.rs b/frame/contracts/src/unsafe_debug.rs index 8970937708bb4..2d067bd434c27 100644 --- a/frame/contracts/src/unsafe_debug.rs +++ b/frame/contracts/src/unsafe_debug.rs @@ -2,6 +2,8 @@ use crate::exec::ExportedFunction; use crate::CodeHash; 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> {} @@ -39,3 +41,5 @@ pub trait ExecutionObserver { ) { } } + +impl ExecutionObserver for () {} From 386c7eec1c755230ee1b88a6d4560da0f6776b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 16:42:41 +0200 Subject: [PATCH 12/39] fmt --- bin/node/runtime/src/lib.rs | 28 ++++++++++++++-------------- frame/contracts/src/unsafe_debug.rs | 3 +-- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 40697af0f405d..9f5072c370282 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -311,21 +311,21 @@ impl InstanceFilter for ProxyType { ProxyType::Any => true, ProxyType::NonTransfer => !matches!( c, - RuntimeCall::Balances(..) - | RuntimeCall::Assets(..) - | RuntimeCall::Uniques(..) - | RuntimeCall::Nfts(..) - | RuntimeCall::Vesting(pallet_vesting::Call::vested_transfer { .. }) - | RuntimeCall::Indices(pallet_indices::Call::transfer { .. }) + RuntimeCall::Balances(..) | + RuntimeCall::Assets(..) | + RuntimeCall::Uniques(..) | + RuntimeCall::Nfts(..) | + RuntimeCall::Vesting(pallet_vesting::Call::vested_transfer { .. }) | + RuntimeCall::Indices(pallet_indices::Call::transfer { .. }) ), ProxyType::Governance => matches!( c, - RuntimeCall::Democracy(..) - | RuntimeCall::Council(..) - | RuntimeCall::Society(..) - | RuntimeCall::TechnicalCommittee(..) - | RuntimeCall::Elections(..) - | RuntimeCall::Treasury(..) + RuntimeCall::Democracy(..) | + RuntimeCall::Council(..) | + RuntimeCall::Society(..) | + RuntimeCall::TechnicalCommittee(..) | + RuntimeCall::Elections(..) | + RuntimeCall::Treasury(..) ), ProxyType::Staking => { matches!(c, RuntimeCall::Staking(..) | RuntimeCall::FastUnstake(..)) @@ -693,8 +693,8 @@ impl Get> for OffchainRandomBalancing { max => { let seed = sp_io::offchain::random_seed(); let random = ::decode(&mut TrailingZeroInput::new(&seed)) - .expect("input is padded with zeroes; qed") - % max.saturating_add(1); + .expect("input is padded with zeroes; qed") % + max.saturating_add(1); random as usize }, }; diff --git a/frame/contracts/src/unsafe_debug.rs b/frame/contracts/src/unsafe_debug.rs index 2d067bd434c27..288e827b004e8 100644 --- a/frame/contracts/src/unsafe_debug.rs +++ b/frame/contracts/src/unsafe_debug.rs @@ -1,5 +1,4 @@ -use crate::exec::ExportedFunction; -use crate::CodeHash; +use crate::{exec::ExportedFunction, CodeHash}; use pallet_contracts_primitives::ExecReturnValue; /// Umbrella trait for all interfaces that serves for debugging, but are not suitable for any From 19e9da0078bff756cf1eaff227a579aebbb7efc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 16:44:41 +0200 Subject: [PATCH 13/39] Make it feature-gated or for tests --- bin/node/runtime/src/lib.rs | 2 +- frame/contracts/src/exec.rs | 6 +++--- frame/contracts/src/lib.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 9f5072c370282..4685b3b2d03d1 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1260,7 +1260,7 @@ impl pallet_contracts::Config for Runtime { type Migrations = (NoopMigration<1>, NoopMigration<2>); type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; - #[cfg(feature = "unsafe-debug")] + #[cfg(any(test, feature = "unsafe-debug"))] type Debug = (); } diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 9644ab9c73ff4..fe73f54db7df5 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -15,7 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#[cfg(feature = "unsafe-debug")] +#[cfg(any(test, feature = "unsafe-debug"))] use crate::unsafe_debug::ExecutionObserver; use crate::{ gas::GasMeter, @@ -905,7 +905,7 @@ where // Every non delegate call or instantiate also optionally transfers the balance. self.initial_transfer()?; - #[cfg(feature = "unsafe-debug")] + #[cfg(any(test, feature = "unsafe-debug"))] let (code_hash, input_clone) = { let code_hash = executable.code_hash().clone(); T::Debug::before_call(&code_hash, entry_point, &input_data); @@ -917,7 +917,7 @@ where .execute(self, &entry_point, input_data) .map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?; - #[cfg(feature = "unsafe-debug")] + #[cfg(any(test, feature = "unsafe-debug"))] T::Debug::after_call(&code_hash, entry_point, &input_clone, &output); // Avoid useless work that would be reverted anyways. diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 2bb8ab0814d47..78cb56d64b131 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -97,7 +97,7 @@ mod wasm; pub mod chain_extension; pub mod migration; -#[cfg(feature = "unsafe-debug")] +#[cfg(any(test, feature = "unsafe-debug"))] pub mod unsafe_debug; pub mod weights; @@ -353,7 +353,7 @@ pub mod pallet { /// ``` type Migrations: MigrateSequence; - #[cfg(feature = "unsafe-debug")] + #[cfg(any(test, feature = "unsafe-debug"))] type Debug: unsafe_debug::UnsafeDebug; } From 9d28accbb4d3adf26a77d24ba4f3be206972eff9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 17:43:50 +0200 Subject: [PATCH 14/39] Prepare testing kit --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/tests.rs | 52 +++++++++++++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index fe73f54db7df5..5fc4108ab07e7 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -345,7 +345,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)] pub enum ExportedFunction { /// The constructor function which is executed on deployment of a contract. Constructor, diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 70ea5d91ccb88..ec14ddccd56ab 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -26,11 +26,12 @@ use crate::{ exec::{Frame, Key}, storage::DeletionQueueManager, tests::test_utils::{get_contract, get_contract_checked}, + unsafe_debug::ExecutionObserver, wasm::{Determinism, ReturnCode as RuntimeReturnCode}, weights::WeightInfo, BalanceOf, Code, CodeHash, CodeInfoOf, CollectEvents, Config, ContractInfo, ContractInfoOf, - DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, MigrationInProgress, - NoopMigration, Origin, Pallet, PristineCode, Schedule, + DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, ExportedFunction, + MigrationInProgress, NoopMigration, Origin, Pallet, PristineCode, Schedule, }; use assert_matches::assert_matches; use codec::Encode; @@ -46,7 +47,7 @@ use frame_support::{ weights::{constants::WEIGHT_REF_TIME_PER_SECOND, Weight}, }; use frame_system::{EventRecord, Phase}; -use pallet_contracts_primitives::CodeUploadReturnValue; +use pallet_contracts_primitives::{CodeUploadReturnValue, ExecReturnValue}; use pretty_assertions::{assert_eq, assert_ne}; use sp_core::ByteArray; use sp_io::hashing::blake2_256; @@ -56,7 +57,7 @@ use sp_runtime::{ traits::{BlakeTwo256, Convert, Hash, IdentityLookup}, AccountId32, BuildStorage, Perbill, TokenError, }; -use std::ops::Deref; +use std::{cell::RefCell, ops::Deref}; type Block = frame_system::mocking::MockBlock; @@ -436,6 +437,48 @@ parameter_types! { pub static UnstableInterface: bool = true; } +#[derive(Clone, PartialEq, Eq)] +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: &[u8], + output: &ExecReturnValue, + ) { + DEBUG_EXECUTION_TRACE.with(|d| { + d.borrow_mut().push(DebugFrame { + code_hash: code_hash.clone(), + call: entry_point, + input: input_data.to_vec(), + result: Some(output.data.clone()), + }) + }); + } +} + impl Config for Test { type Time = Timestamp; type Randomness = Randomness; @@ -460,6 +503,7 @@ impl Config for Test { type Migrations = (NoopMigration<1>, NoopMigration<2>); type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; type MaxDelegateDependencies = MaxDelegateDependencies; + type Debug = TestDebugger; } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]); From 55962ea1fb5597225b0dc9beb9acadb1fba38013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 18:24:54 +0200 Subject: [PATCH 15/39] Testcase --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/tests.rs | 90 +++++++++++++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 5fc4108ab07e7..dfe9e1fa7c1a3 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -345,7 +345,7 @@ pub trait Ext: sealing::Sealed { } /// Describes the different functions that can be exported by an [`Executable`]. -#[derive(Clone, Copy, PartialEq, Eq)] +#[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum ExportedFunction { /// The constructor function which is executed on deployment of a contract. Constructor, diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index ec14ddccd56ab..bb8c0bb849242 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -437,7 +437,7 @@ parameter_types! { pub static UnstableInterface: bool = true; } -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq, Debug)] struct DebugFrame { code_hash: CodeHash, call: ExportedFunction, @@ -5936,3 +5936,91 @@ fn root_cannot_instantiate() { ); }); } + +#[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), + ] + ); + }); +} From 52b267d10108fd88e51ec47d28dfca5b6e47d9e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Tue, 1 Aug 2023 18:27:09 +0200 Subject: [PATCH 16/39] Fmt --- frame/contracts/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index cd1fb490ea024..f32daa3b34ab6 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -32,8 +32,8 @@ use crate::{ wasm::{Determinism, ReturnCode as RuntimeReturnCode}, weights::WeightInfo, BalanceOf, Code, CodeHash, CodeInfoOf, CollectEvents, Config, ContractInfo, ContractInfoOf, - DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, MigrationInProgress, Origin, - Pallet, PristineCode, Schedule, ExportedFunction + DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, ExportedFunction, + MigrationInProgress, Origin, Pallet, PristineCode, Schedule, }; use assert_matches::assert_matches; use codec::Encode; From 8387b60bd1a7c86b1a5a6a1bdbe4da4178ed0347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 09:12:15 +0200 Subject: [PATCH 17/39] Use feature in dev-deps --- Cargo.lock | 1 + frame/contracts/Cargo.toml | 3 +++ frame/contracts/src/exec.rs | 6 +++--- frame/contracts/src/lib.rs | 4 ++-- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 93a8e5e988305..e21b3a4060d12 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5977,6 +5977,7 @@ dependencies = [ "impl-trait-for-tuples", "log", "pallet-balances", + "pallet-contracts", "pallet-contracts-primitives", "pallet-contracts-proc-macro", "pallet-insecure-randomness-collective-flip", diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 5f2824f7d0c21..8097199c35393 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -54,6 +54,9 @@ env_logger = "0.9" pretty_assertions = "1" wat = "1" +# Include this package, so that `unsafe-debug` feature is included for testing. +pallet-contracts = { path = ".", default-features = false, features = ["unsafe-debug"] } + # Substrate Dependencies pallet-balances = { version = "4.0.0-dev", path = "../balances" } pallet-timestamp = { version = "4.0.0-dev", path = "../timestamp" } diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index dfe9e1fa7c1a3..a60c299043288 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -15,7 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#[cfg(any(test, feature = "unsafe-debug"))] +#[cfg(feature = "unsafe-debug")] use crate::unsafe_debug::ExecutionObserver; use crate::{ gas::GasMeter, @@ -905,7 +905,7 @@ where // Every non delegate call or instantiate also optionally transfers the balance. self.initial_transfer()?; - #[cfg(any(test, feature = "unsafe-debug"))] + #[cfg(feature = "unsafe-debug")] let (code_hash, input_clone) = { let code_hash = executable.code_hash().clone(); T::Debug::before_call(&code_hash, entry_point, &input_data); @@ -917,7 +917,7 @@ where .execute(self, &entry_point, input_data) .map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?; - #[cfg(any(test, feature = "unsafe-debug"))] + #[cfg(feature = "unsafe-debug")] T::Debug::after_call(&code_hash, entry_point, &input_clone, &output); // Avoid useless work that would be reverted anyways. diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 7bf6d6ffb6a56..5d65294ba2e35 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -97,7 +97,7 @@ mod wasm; pub mod chain_extension; pub mod migration; -#[cfg(any(test, feature = "unsafe-debug"))] +#[cfg(feature = "unsafe-debug")] pub mod unsafe_debug; pub mod weights; @@ -348,7 +348,7 @@ pub mod pallet { /// ``` type Migrations: MigrateSequence; - #[cfg(any(test, feature = "unsafe-debug"))] + #[cfg(feature = "unsafe-debug")] type Debug: unsafe_debug::UnsafeDebug; } From 04780a7384cb1de102f9173608257032b6c540ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 09:17:18 +0200 Subject: [PATCH 18/39] ? --- bin/node/runtime/src/lib.rs | 2 +- frame/contracts/Cargo.toml | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index d6e72a71ea2c8..45a35975c13ad 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1258,7 +1258,7 @@ impl pallet_contracts::Config for Runtime { type Migrations = pallet_contracts::migration::codegen::BenchMigrations; type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; - #[cfg(any(test, feature = "unsafe-debug"))] + #[cfg(feature = "unsafe-debug")] type Debug = (); } diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 3a62ffc93488b..3725e77653c13 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -110,10 +110,11 @@ try-runtime = [ "frame-support/try-runtime", "frame-system/try-runtime", "pallet-balances/try-runtime", + "pallet-contracts/try-runtime", "pallet-insecure-randomness-collective-flip/try-runtime", "pallet-proxy/try-runtime", "pallet-timestamp/try-runtime", "pallet-utility/try-runtime", - "sp-runtime/try-runtime" + "sp-runtime/try-runtime", ] unsafe-debug = [] From 725cfcc8544429810627e7195e9931f6fb22c1c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 09:19:03 +0200 Subject: [PATCH 19/39] feature propagation --- frame/contracts/Cargo.toml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 3725e77653c13..ae047cdc60f51 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -101,6 +101,7 @@ runtime-benchmarks = [ "frame-support/runtime-benchmarks", "frame-system/runtime-benchmarks", "pallet-balances/runtime-benchmarks", + "pallet-contracts/runtime-benchmarks", # needed because of self-dependency "pallet-proxy/runtime-benchmarks", "pallet-timestamp/runtime-benchmarks", "pallet-utility/runtime-benchmarks", @@ -110,7 +111,7 @@ try-runtime = [ "frame-support/try-runtime", "frame-system/try-runtime", "pallet-balances/try-runtime", - "pallet-contracts/try-runtime", + "pallet-contracts/try-runtime", # needed because of self-dependency "pallet-insecure-randomness-collective-flip/try-runtime", "pallet-proxy/try-runtime", "pallet-timestamp/try-runtime", From 232f12cef0e6216688fdde4aec2e29c0d9b934a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 09:21:01 +0200 Subject: [PATCH 20/39] AAAA --- frame/contracts/Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index ae047cdc60f51..35878b03714bd 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -86,6 +86,7 @@ std = [ "rand/std", "environmental/std", "pallet-balances/std", + "pallet-contracts/std", # needed because of self-dependency "pallet-insecure-randomness-collective-flip/std", "pallet-proxy/std", "pallet-timestamp/std", From e8ac68a8fe408b906878763f310eaceec39328cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 12:03:13 +0200 Subject: [PATCH 21/39] lol, that doesn't make much sense to me --- bin/node/runtime/Cargo.toml | 3 --- bin/node/runtime/src/lib.rs | 1 - 2 files changed, 4 deletions(-) diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index e319711fd8b8a..4a9bb9a769597 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -373,6 +373,3 @@ 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 45a35975c13ad..2fc2144815c67 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1258,7 +1258,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 = (); } From d1a80276b443f5f3dc1451cfc87db24cbe3328df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 12:37:51 +0200 Subject: [PATCH 22/39] Turn on --- 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 4a9bb9a769597..b28d85ee9047b 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -68,7 +68,7 @@ pallet-balances = { version = "4.0.0-dev", default-features = false, path = "../ pallet-bounties = { version = "4.0.0-dev", default-features = false, path = "../../../frame/bounties" } pallet-child-bounties = { version = "4.0.0-dev", default-features = false, path = "../../../frame/child-bounties" } pallet-collective = { version = "4.0.0-dev", default-features = false, path = "../../../frame/collective" } -pallet-contracts = { version = "4.0.0-dev", default-features = false, path = "../../../frame/contracts" } +pallet-contracts = { version = "4.0.0-dev", default-features = false, path = "../../../frame/contracts", features = ["unsafe-debug"] } pallet-contracts-primitives = { version = "24.0.0", default-features = false, path = "../../../frame/contracts/primitives/" } pallet-conviction-voting = { version = "4.0.0-dev", default-features = false, path = "../../../frame/conviction-voting" } pallet-core-fellowship = { version = "4.0.0-dev", default-features = false, path = "../../../frame/core-fellowship" } From 5a951516edcab4b0ef9c9fcdda76c81ce72dc036 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 12:48:51 +0200 Subject: [PATCH 23/39] clippy --- frame/contracts/src/exec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index a60c299043288..9ddafd9a83792 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -907,7 +907,7 @@ where #[cfg(feature = "unsafe-debug")] let (code_hash, input_clone) = { - let code_hash = executable.code_hash().clone(); + let code_hash = *executable.code_hash(); T::Debug::before_call(&code_hash, entry_point, &input_data); (code_hash, input_data.clone()) }; From 539e6b6566e64b0b1556e7f3d490b3a124fcce26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 16:33:34 +0200 Subject: [PATCH 24/39] Remove self dep --- Cargo.lock | 1 - frame/contracts/Cargo.toml | 6 ------ 2 files changed, 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e21b3a4060d12..93a8e5e988305 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5977,7 +5977,6 @@ dependencies = [ "impl-trait-for-tuples", "log", "pallet-balances", - "pallet-contracts", "pallet-contracts-primitives", "pallet-contracts-proc-macro", "pallet-insecure-randomness-collective-flip", diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 35878b03714bd..ee8195e59bb34 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -54,9 +54,6 @@ env_logger = "0.9" pretty_assertions = "1" wat = "1" -# Include this package, so that `unsafe-debug` feature is included for testing. -pallet-contracts = { path = ".", default-features = false, features = ["unsafe-debug"] } - # Substrate Dependencies pallet-balances = { version = "4.0.0-dev", path = "../balances" } pallet-timestamp = { version = "4.0.0-dev", path = "../timestamp" } @@ -86,7 +83,6 @@ std = [ "rand/std", "environmental/std", "pallet-balances/std", - "pallet-contracts/std", # needed because of self-dependency "pallet-insecure-randomness-collective-flip/std", "pallet-proxy/std", "pallet-timestamp/std", @@ -102,7 +98,6 @@ runtime-benchmarks = [ "frame-support/runtime-benchmarks", "frame-system/runtime-benchmarks", "pallet-balances/runtime-benchmarks", - "pallet-contracts/runtime-benchmarks", # needed because of self-dependency "pallet-proxy/runtime-benchmarks", "pallet-timestamp/runtime-benchmarks", "pallet-utility/runtime-benchmarks", @@ -112,7 +107,6 @@ try-runtime = [ "frame-support/try-runtime", "frame-system/try-runtime", "pallet-balances/try-runtime", - "pallet-contracts/try-runtime", # needed because of self-dependency "pallet-insecure-randomness-collective-flip/try-runtime", "pallet-proxy/try-runtime", "pallet-timestamp/try-runtime", From eb8f34cea9b2aebab076e2feedfc81914ab647af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Wed, 2 Aug 2023 16:37:22 +0200 Subject: [PATCH 25/39] fmt, feature-gating test --- frame/contracts/src/tests.rs | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index f32daa3b34ab6..5f09a0d45cd03 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -28,13 +28,14 @@ use crate::{ migration::codegen::LATEST_MIGRATION_VERSION, storage::DeletionQueueManager, tests::test_utils::{get_contract, get_contract_checked}, - unsafe_debug::ExecutionObserver, wasm::{Determinism, ReturnCode as RuntimeReturnCode}, weights::WeightInfo, BalanceOf, Code, CodeHash, CodeInfoOf, CollectEvents, Config, ContractInfo, ContractInfoOf, - DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, ExportedFunction, - MigrationInProgress, Origin, Pallet, PristineCode, Schedule, + DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, MigrationInProgress, Origin, + Pallet, PristineCode, Schedule, }; +#[cfg(feature = "unsafe-debug")] +use crate::{unsafe_debug::ExecutionObserver, ExportedFunction}; use assert_matches::assert_matches; use codec::Encode; use frame_support::{ @@ -49,7 +50,9 @@ use frame_support::{ weights::{constants::WEIGHT_REF_TIME_PER_SECOND, Weight}, }; use frame_system::{EventRecord, Phase}; -use pallet_contracts_primitives::{CodeUploadReturnValue, ExecReturnValue}; +use pallet_contracts_primitives::CodeUploadReturnValue; +#[cfg(feature = "unsafe-debug")] +use pallet_contracts_primitives::ExecReturnValue; use pretty_assertions::{assert_eq, assert_ne}; use sp_core::ByteArray; use sp_io::hashing::blake2_256; @@ -59,7 +62,9 @@ use sp_runtime::{ traits::{BlakeTwo256, Convert, Hash, IdentityLookup}, AccountId32, BuildStorage, Perbill, TokenError, }; -use std::{cell::RefCell, ops::Deref}; +#[cfg(feature = "unsafe-debug")] +use std::cell::RefCell; +use std::ops::Deref; type Block = frame_system::mocking::MockBlock; @@ -439,6 +444,7 @@ parameter_types! { pub static UnstableInterface: bool = true; } +#[cfg(feature = "unsafe-debug")] #[derive(Clone, PartialEq, Eq, Debug)] struct DebugFrame { code_hash: CodeHash, @@ -447,11 +453,14 @@ struct DebugFrame { result: Option>, } +#[cfg(feature = "unsafe-debug")] thread_local! { -static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); - } + static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); +} +#[cfg(feature = "unsafe-debug")] pub struct TestDebugger; +#[cfg(feature = "unsafe-debug")] impl ExecutionObserver> for TestDebugger { fn before_call(code_hash: &CodeHash, entry_point: ExportedFunction, input_data: &[u8]) { DEBUG_EXECUTION_TRACE.with(|d| { @@ -505,6 +514,7 @@ impl Config for Test { type Migrations = crate::migration::codegen::BenchMigrations; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; type MaxDelegateDependencies = MaxDelegateDependencies; + #[cfg(feature = "unsafe-debug")] type Debug = TestDebugger; } @@ -5942,6 +5952,7 @@ fn root_cannot_instantiate() { }); } +#[cfg(feature = "unsafe-debug")] #[test] fn unsafe_debugging_works() { let (wasm_caller, code_hash_caller) = compile_module::("call").unwrap(); From 9dd8b332c495c68ac886e18ab96e7e731a283168 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 09:25:51 +0200 Subject: [PATCH 26/39] Noop to trigger CI --- frame/contracts/src/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 5f09a0d45cd03..98c808f7fc3b2 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -6028,7 +6028,6 @@ fn unsafe_debugging_works() { )); let stack_top = current_stack()[4..].to_vec(); - assert_eq!( stack_top, vec![ From 3a3aa4500a88c41d4df5932d29bf5b2776becc70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 11:29:12 +0200 Subject: [PATCH 27/39] idk --- frame/contracts/src/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 98c808f7fc3b2..b77f3845abdf2 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -5997,6 +5997,8 @@ fn unsafe_debugging_works() { } } + use frame_support::traits::Currency; + ExtBuilder::default().existential_deposit(200).build().execute_with(|| { let _ = Balances::deposit_creating(&ALICE, 1_000_000); From cae093f81c79ecd47a1d1e9715e8159d4e9f9d12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 12:12:33 +0200 Subject: [PATCH 28/39] add feature to pipeline --- scripts/ci/gitlab/pipeline/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 17a25bae64b453ba0a6cf3a9b94cc4a907a56072 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 12:30:00 +0200 Subject: [PATCH 29/39] Corrupt test to see if it is actually being run --- frame/contracts/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index b77f3845abdf2..9970b20edb4f7 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -6036,7 +6036,7 @@ fn unsafe_debugging_works() { 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), + call_frame(code_hash_caller, main_args, false), ] ); }); From a3f84613660297750904b5b0593bff4f637655c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 12:30:20 +0200 Subject: [PATCH 30/39] Revert change --- frame/contracts/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 9970b20edb4f7..b77f3845abdf2 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -6036,7 +6036,7 @@ fn unsafe_debugging_works() { 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, false), + call_frame(code_hash_caller, main_args, true), ] ); }); From 986e9e20f023c2930f9febfc544fa3e58863dff8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 15:27:33 +0200 Subject: [PATCH 31/39] Doc for conf type --- 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 5d65294ba2e35..d7c75ce7a54d3 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -348,6 +348,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; } From 23b03b734d11d1d5934e0fffe89c8019e405a88b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 15:29:50 +0200 Subject: [PATCH 32/39] Review --- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 1 - frame/contracts/src/tests.rs | 4 ++-- frame/contracts/src/unsafe_debug.rs | 4 +++- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 9ddafd9a83792..db721211c4fdb 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -918,7 +918,7 @@ where .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); + T::Debug::after_call(&code_hash, entry_point, input_clone, &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 d7c75ce7a54d3..586a5f36d3e15 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -97,7 +97,6 @@ mod wasm; pub mod chain_extension; pub mod migration; -#[cfg(feature = "unsafe-debug")] pub mod unsafe_debug; pub mod weights; diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index b77f3845abdf2..bd5149892cd75 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -476,14 +476,14 @@ impl ExecutionObserver> for TestDebugger { fn after_call( code_hash: &CodeHash, entry_point: ExportedFunction, - input_data: &[u8], + 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.to_vec(), + input: input_data, result: Some(output.data.clone()), }) }); diff --git a/frame/contracts/src/unsafe_debug.rs b/frame/contracts/src/unsafe_debug.rs index 288e827b004e8..3043c2ef2656a 100644 --- a/frame/contracts/src/unsafe_debug.rs +++ b/frame/contracts/src/unsafe_debug.rs @@ -1,3 +1,5 @@ +#![cfg(feature = "unsafe-debug")] + use crate::{exec::ExportedFunction, CodeHash}; use pallet_contracts_primitives::ExecReturnValue; @@ -35,7 +37,7 @@ pub trait ExecutionObserver { fn after_call( _code_hash: &CodeHash, _entry_point: ExportedFunction, - _input_data: &[u8], + _input_data: Vec, _output: &ExecReturnValue, ) { } From 5970e8a39ed1ddfff02c373aec72512effbab855 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 15:36:47 +0200 Subject: [PATCH 33/39] Imports --- frame/contracts/src/lib.rs | 2 +- frame/contracts/src/unsafe_debug.rs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 586a5f36d3e15..577dab6fcae7e 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -138,7 +138,7 @@ pub use weights::WeightInfo; pub use crate::{ address::{AddressGenerator, DefaultAddressGenerator}, - exec::{ExportedFunction, Frame}, + exec::Frame, migration::{MigrateSequence, Migration, NoopMigration}, pallet::*, schedule::{HostFnWeights, InstructionWeights, Limits, Schedule}, diff --git a/frame/contracts/src/unsafe_debug.rs b/frame/contracts/src/unsafe_debug.rs index 3043c2ef2656a..418af5e605d28 100644 --- a/frame/contracts/src/unsafe_debug.rs +++ b/frame/contracts/src/unsafe_debug.rs @@ -1,6 +1,7 @@ #![cfg(feature = "unsafe-debug")] -use crate::{exec::ExportedFunction, CodeHash}; +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 From 19c74f9266401f92ecbecb15ba6ee452dc6af538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Thu, 3 Aug 2023 15:42:45 +0200 Subject: [PATCH 34/39] ... --- frame/contracts/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index bd5149892cd75..e6d018a518715 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -18,6 +18,8 @@ mod pallet_dummy; use self::test_utils::{ensure_stored, expected_deposit, hash}; +#[cfg(feature = "unsafe-debug")] +use crate::unsafe_debug::{ExecutionObserver, ExportedFunction}; use crate::{ self as pallet_contracts, chain_extension::{ @@ -34,8 +36,6 @@ use crate::{ DebugInfo, DefaultAddressGenerator, DeletionQueueCounter, Error, MigrationInProgress, Origin, Pallet, PristineCode, Schedule, }; -#[cfg(feature = "unsafe-debug")] -use crate::{unsafe_debug::ExecutionObserver, ExportedFunction}; use assert_matches::assert_matches; use codec::Encode; use frame_support::{ From 6485e4dd02fea5fd0c9aaaaaeffe2f523f0c9cf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Fri, 4 Aug 2023 12:19:39 +0200 Subject: [PATCH 35/39] Remove debug for kitchen-sink --- bin/node/runtime/Cargo.toml | 2 +- bin/node/runtime/src/lib.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index b28d85ee9047b..4a9bb9a769597 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -68,7 +68,7 @@ pallet-balances = { version = "4.0.0-dev", default-features = false, path = "../ pallet-bounties = { version = "4.0.0-dev", default-features = false, path = "../../../frame/bounties" } pallet-child-bounties = { version = "4.0.0-dev", default-features = false, path = "../../../frame/child-bounties" } pallet-collective = { version = "4.0.0-dev", default-features = false, path = "../../../frame/collective" } -pallet-contracts = { version = "4.0.0-dev", default-features = false, path = "../../../frame/contracts", features = ["unsafe-debug"] } +pallet-contracts = { version = "4.0.0-dev", default-features = false, path = "../../../frame/contracts" } pallet-contracts-primitives = { version = "24.0.0", default-features = false, path = "../../../frame/contracts/primitives/" } pallet-conviction-voting = { version = "4.0.0-dev", default-features = false, path = "../../../frame/conviction-voting" } pallet-core-fellowship = { version = "4.0.0-dev", default-features = false, path = "../../../frame/core-fellowship" } diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 4cbcc6d462361..69dedfb599583 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1258,7 +1258,6 @@ impl pallet_contracts::Config for Runtime { type Migrations = pallet_contracts::migration::codegen::BenchMigrations; type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; - type Debug = (); } impl pallet_sudo::Config for Runtime { From 01ef583ecd9162f7cc44f011e1a80529a11e87c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Fri, 4 Aug 2023 12:26:27 +0200 Subject: [PATCH 36/39] Move test --- frame/contracts/src/tests.rs | 145 +--------------------- frame/contracts/src/tests/unsafe_debug.rs | 144 +++++++++++++++++++++ 2 files changed, 146 insertions(+), 143 deletions(-) create mode 100644 frame/contracts/src/tests/unsafe_debug.rs diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index c26a0dab31360..3132b8e39f7da 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -16,10 +16,9 @@ // limitations under the License. mod pallet_dummy; +mod unsafe_debug; use self::test_utils::{ensure_stored, expected_deposit, hash}; -#[cfg(feature = "unsafe-debug")] -use crate::unsafe_debug::{ExecutionObserver, ExportedFunction}; use crate::{ self as pallet_contracts, chain_extension::{ @@ -52,8 +51,6 @@ use frame_support::{ }; use frame_system::{EventRecord, Phase}; use pallet_contracts_primitives::CodeUploadReturnValue; -#[cfg(feature = "unsafe-debug")] -use pallet_contracts_primitives::ExecReturnValue; use pretty_assertions::{assert_eq, assert_ne}; use sp_core::ByteArray; use sp_io::hashing::blake2_256; @@ -63,8 +60,6 @@ use sp_runtime::{ traits::{BlakeTwo256, Convert, Hash, IdentityLookup}, AccountId32, BuildStorage, Perbill, TokenError, }; -#[cfg(feature = "unsafe-debug")] -use std::cell::RefCell; use std::ops::Deref; type Block = frame_system::mocking::MockBlock; @@ -445,52 +440,6 @@ parameter_types! { pub static UnstableInterface: bool = true; } -#[cfg(feature = "unsafe-debug")] -#[derive(Clone, PartialEq, Eq, Debug)] -struct DebugFrame { - code_hash: CodeHash, - call: ExportedFunction, - input: Vec, - result: Option>, -} - -#[cfg(feature = "unsafe-debug")] -thread_local! { - static DEBUG_EXECUTION_TRACE: RefCell> = RefCell::new(Vec::new()); -} - -#[cfg(feature = "unsafe-debug")] -pub struct TestDebugger; -#[cfg(feature = "unsafe-debug")] -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()), - }) - }); - } -} - impl Config for Test { type Time = Timestamp; type Randomness = Randomness; @@ -517,7 +466,7 @@ impl Config for Test { type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; type MaxDelegateDependencies = MaxDelegateDependencies; #[cfg(feature = "unsafe-debug")] - type Debug = TestDebugger; + type Debug = unsafe_debug::TestDebugger; } pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]); @@ -5938,93 +5887,3 @@ fn root_cannot_instantiate() { ); }); } - -#[cfg(feature = "unsafe-debug")] -#[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 }, - } - } - - use frame_support::traits::Currency; - - 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/tests/unsafe_debug.rs b/frame/contracts/src/tests/unsafe_debug.rs new file mode 100644 index 0000000000000..a990ec9180309 --- /dev/null +++ b/frame/contracts/src/tests/unsafe_debug.rs @@ -0,0 +1,144 @@ +#![cfg(feature = "unsafe-debug")] + +use crate::{ + tests::{compile_module, ExtBuilder, ALICE, GAS_LIMIT}, + unsafe_debug::{ExecutionObserver, ExportedFunction}, + CodeHash, CollectEvents, DebugInfo, +}; +use codec::Encode; +use frame_support::assert_ok; +use pallet_contracts_primitives::{Code, ExecReturnValue}; +use sp_core::crypto::AccountId32; +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 }, + } + } + + use frame_support::traits::Currency; + + 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), + ] + ); + }); +} From 1eaabb184c3d6048d5a0f11be7a15fbeb1cbb967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Miko=C5=82ajczyk?= Date: Fri, 4 Aug 2023 13:17:43 +0200 Subject: [PATCH 37/39] Fix imports --- frame/contracts/src/tests/unsafe_debug.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/frame/contracts/src/tests/unsafe_debug.rs b/frame/contracts/src/tests/unsafe_debug.rs index a990ec9180309..160a6ed6dc54f 100644 --- a/frame/contracts/src/tests/unsafe_debug.rs +++ b/frame/contracts/src/tests/unsafe_debug.rs @@ -1,14 +1,10 @@ #![cfg(feature = "unsafe-debug")] -use crate::{ - tests::{compile_module, ExtBuilder, ALICE, GAS_LIMIT}, - unsafe_debug::{ExecutionObserver, ExportedFunction}, - CodeHash, CollectEvents, DebugInfo, -}; -use codec::Encode; -use frame_support::assert_ok; -use pallet_contracts_primitives::{Code, ExecReturnValue}; -use sp_core::crypto::AccountId32; +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)] @@ -98,8 +94,6 @@ fn unsafe_debugging_works() { } } - use frame_support::traits::Currency; - ExtBuilder::default().existential_deposit(200).build().execute_with(|| { let _ = Balances::deposit_creating(&ALICE, 1_000_000); From 92f29be06daff9c34bfc102799dcaa22aa741a82 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 4 Aug 2023 15:17:36 +0200 Subject: [PATCH 38/39] fix build issue --- bin/node/runtime/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 69dedfb599583..919677700fd4f 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1256,6 +1256,8 @@ impl pallet_contracts::Config for Runtime { type Migrations = (); #[cfg(feature = "runtime-benchmarks")] type Migrations = pallet_contracts::migration::codegen::BenchMigrations; + #[cfg(feature = "unsafe-debug")] + type Debug = (); type MaxDelegateDependencies = ConstU32<32>; type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; } From 02b0d5bc4078fab2132b0fd7937ab27b3e5da44b Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 4 Aug 2023 15:23:16 +0200 Subject: [PATCH 39/39] Probably need that as well... --- bin/node/runtime/Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index 4a9bb9a769597..317bd4546827d 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -134,6 +134,7 @@ substrate-wasm-builder = { version = "5.0.0-dev", path = "../../../utils/wasm-bu [features] default = ["std"] with-tracing = ["frame-executive/with-tracing"] +unsafe-debug = ["pallet-contracts/unsafe-debug"] std = [ "pallet-whitelist/std", "pallet-offences-benchmarking?/std",