From 7d73b31002b1cba7cb1aeec2aaaa9816ab888c40 Mon Sep 17 00:00:00 2001 From: Frank Bell Date: Wed, 6 Mar 2024 21:03:33 +0000 Subject: [PATCH 1/7] test(pop-api): read_state chain extension test --- pop-api/primitives/src/storage_keys.rs | 6 +- runtime/src/extensions.rs | 97 +++++++++++++++++++++----- 2 files changed, 84 insertions(+), 19 deletions(-) diff --git a/pop-api/primitives/src/storage_keys.rs b/pop-api/primitives/src/storage_keys.rs index 63595cf3f..8b107eecf 100644 --- a/pop-api/primitives/src/storage_keys.rs +++ b/pop-api/primitives/src/storage_keys.rs @@ -1,11 +1,11 @@ -use scale::{Decode, Encode}; +use scale::{Decode, Encode, MaxEncodedLen}; -#[derive(Encode, Decode, Debug)] +#[derive(Encode, Decode, Debug, MaxEncodedLen)] pub enum RuntimeStateKeys { ParachainSystem(ParachainSystemKeys), } -#[derive(Encode, Decode, Debug)] +#[derive(Encode, Decode, Debug, MaxEncodedLen)] pub enum ParachainSystemKeys { LastRelayChainBlockNumber, } diff --git a/runtime/src/extensions.rs b/runtime/src/extensions.rs index a11ab1816..f232efcec 100644 --- a/runtime/src/extensions.rs +++ b/runtime/src/extensions.rs @@ -77,7 +77,7 @@ impl TryFrom for v0::FuncId { } } -pub(crate) fn dispatch(env: Environment) -> Result<(), DispatchError> +fn dispatch(env: Environment) -> Result<(), DispatchError> where T: pallet_contracts::Config + frame_system::Config, ::AccountId: UncheckedFrom<::Hash> + AsRef<[u8]>, @@ -133,7 +133,7 @@ where Ok(()) } -pub(crate) fn read_state(env: Environment) -> Result<(), DispatchError> +fn read_state(env: Environment) -> Result<(), DispatchError> where T: pallet_contracts::Config + frame_system::Config, E: Ext, @@ -142,13 +142,14 @@ where let mut env = env.buf_in_buf_out(); - // TODO: Substitute len u32 with pop_api::src::impls::pop_network::StringLimit. - // Move StringLimit to pop_api_primitives first. - let len: u32 = env.in_len(); - let key: ParachainSystemKeys = env.read_as_unbounded(len)?; + // TODO: replace with benchmark once available + env.charge_weight(T::DbWeight::get().reads(1_u64))?; + + let key: ParachainSystemKeys = env.read_as()?; let result = match key { ParachainSystemKeys::LastRelayChainBlockNumber => { + env.charge_weight(T::DbWeight::get().reads(1_u64))?; let relay_block_num: BlockNumber = crate::ParachainSystem::last_relay_block_number(); log::debug!( target:LOG_TARGET, @@ -157,7 +158,13 @@ where relay_block_num }, } + .encode() + // Double-encode result for extension return type of bytes .encode(); + log::trace!( + target:LOG_TARGET, + "read state result: {:?}.", result + ); env.write(&result, false, None).map_err(|e| { log::trace!(target: LOG_TARGET, "{:?}", e); DispatchError::Other("unable to write results to contract memory") @@ -174,14 +181,14 @@ mod tests { use parachains_common::CollectionId; pub use sp_runtime::{traits::Hash, AccountId32}; - pub const DEBUG_OUTPUT: pallet_contracts::DebugInfo = pallet_contracts::DebugInfo::UnsafeDebug; + const DEBUG_OUTPUT: pallet_contracts::DebugInfo = pallet_contracts::DebugInfo::UnsafeDebug; - pub const ALICE: AccountId32 = AccountId32::new([1_u8; 32]); - pub const BOB: AccountId32 = AccountId32::new([2_u8; 32]); - pub const INITIAL_AMOUNT: u128 = 100_000 * UNIT; - pub const GAS_LIMIT: Weight = Weight::from_parts(100_000_000_000, 3 * 1024 * 1024); + const ALICE: AccountId32 = AccountId32::new([1_u8; 32]); + const BOB: AccountId32 = AccountId32::new([2_u8; 32]); + const INITIAL_AMOUNT: u128 = 100_000 * UNIT; + const GAS_LIMIT: Weight = Weight::from_parts(100_000_000_000, 3 * 1024 * 1024); - pub fn new_test_ext() -> sp_io::TestExternalities { + fn new_test_ext() -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::::default() .build_storage() .expect("Frame system builds valid default genesis config"); @@ -197,9 +204,7 @@ mod tests { ext } - pub fn load_wasm_module( - path: &str, - ) -> std::io::Result<(Vec, ::Output)> + fn load_wasm_module(path: &str) -> std::io::Result<(Vec, ::Output)> where T: frame_system::Config, { @@ -208,7 +213,7 @@ mod tests { Ok((wasm_binary, code_hash)) } - pub fn function_selector(name: &str) -> Vec { + fn function_selector(name: &str) -> Vec { let hash = sp_io::hashing::blake2_256(name.as_bytes()); [hash[0..4].to_vec()].concat() } @@ -439,4 +444,64 @@ mod tests { assert!(result.did_revert()); }); } + + #[test] + #[ignore] + fn reading_last_relay_chain_block_number_works() { + new_test_ext().execute_with(|| { + let _ = env_logger::try_init(); + + let (wasm_binary, _) = load_wasm_module::("../contracts/pop-api-examples/read-runtime-state/target/ink/pop_api_extension_demo.wasm").unwrap(); + + let init_value = 100; + + let contract = Contracts::bare_instantiate( + ALICE, + init_value, + GAS_LIMIT, + None, + Code::Upload(wasm_binary), + function_selector("new"), + vec![], + DEBUG_OUTPUT, + pallet_contracts::CollectEvents::Skip, + ) + .result + .unwrap(); + + assert!( + !contract.result.did_revert(), + "deploying contract reverted {:?}", + contract + ); + + let addr = contract.account_id; + + let function = function_selector("read_relay_block_number"); + let params = [function].concat(); + + let result = Contracts::bare_call( + ALICE, + addr.clone(), + 0, + Weight::from_parts(100_000_000_000, 3 * 1024 * 1024), + None, + params, + DEBUG_OUTPUT, + pallet_contracts::CollectEvents::UnsafeCollect, + pallet_contracts::Determinism::Relaxed, + ); + + if DEBUG_OUTPUT == pallet_contracts::DebugInfo::UnsafeDebug { + log::debug!( + "Contract debug buffer - {:?}", + String::from_utf8(result.debug_message.clone()) + ); + log::debug!("result: {:?}", result); + } + + // check for revert + assert!(!result.result.unwrap().did_revert(), "Contract reverted!"); + }); + } } From 3e984f5896b06734f0dd025983c41f857dffd214 Mon Sep 17 00:00:00 2001 From: Peter White Date: Wed, 6 Mar 2024 16:55:50 -0700 Subject: [PATCH 2/7] refactor: use generic T for ParachainSystem pallet --- runtime/src/extensions.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/src/extensions.rs b/runtime/src/extensions.rs index f232efcec..b521af47b 100644 --- a/runtime/src/extensions.rs +++ b/runtime/src/extensions.rs @@ -135,7 +135,7 @@ where fn read_state(env: Environment) -> Result<(), DispatchError> where - T: pallet_contracts::Config + frame_system::Config, + T: pallet_contracts::Config + cumulus_pallet_parachain_system::Config + frame_system::Config, E: Ext, { const LOG_TARGET: &str = "pop-api::extension::read_state"; @@ -150,7 +150,7 @@ where let result = match key { ParachainSystemKeys::LastRelayChainBlockNumber => { env.charge_weight(T::DbWeight::get().reads(1_u64))?; - let relay_block_num: BlockNumber = crate::ParachainSystem::last_relay_block_number(); + let relay_block_num: BlockNumber = cumulus_pallet_parachain_system::Pallet::::last_relay_block_number(); log::debug!( target:LOG_TARGET, "last relay chain block number is: {:?}.", relay_block_num From 3deaa6fa1966ac8b5cb40f4cab8a728361dac44e Mon Sep 17 00:00:00 2001 From: Peter White Date: Wed, 6 Mar 2024 18:22:38 -0700 Subject: [PATCH 3/7] feat(pop-api): add proper weight handling to extension --- runtime/src/extensions.rs | 46 ++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/runtime/src/extensions.rs b/runtime/src/extensions.rs index b521af47b..406690c41 100644 --- a/runtime/src/extensions.rs +++ b/runtime/src/extensions.rs @@ -13,6 +13,8 @@ use sp_runtime::{traits::Dispatchable, DispatchError}; const LOG_TARGET: &str = "pop-api::extension"; +type ContractSchedule = ::Schedule; + #[derive(Default)] pub struct PopApiExtension; @@ -90,40 +92,41 @@ where const LOG_TARGET: &str = "pop-api::extension::dispatch"; let mut env = env.buf_in_buf_out(); - - // charge max weight before reading contract memory - // TODO: causing "1010: block limits exhausted" error - // let weight_limit = env.ext().gas_meter().gas_left(); - // let charged_weight = env.charge_weight(weight_limit)?; - - // TODO: debug_message weight is a good approximation of the additional overhead of going - // from contract layer to substrate layer. + let contract_host_weight = ContractSchedule::::get().host_fn_weights; // input length let len = env.in_len(); - let call: ::RuntimeCall = env.read_as_unbounded(len)?; - // conservative weight estimate for deserializing the input. The actual weight is less and should utilize a custom benchmark - let base_weight: Weight = T::DbWeight::get().reads(len.into()); + // calculate weight for reading bytes of `len` + // reference: https://github.com/paritytech/polkadot-sdk/blob/117a9433dac88d5ac00c058c9b39c511d47749d2/substrate/frame/contracts/src/wasm/runtime.rs#L2159 + let base_weight: Weight = contract_host_weight.return_per_byte.saturating_mul(len.into()); - // weight for dispatching the call - let dispatch_weight = call.get_dispatch_info().weight; + // debug_message weight is a good approximation of the additional overhead of going + // from contract layer to substrate layer. + // reference: https://github.com/paritytech/ink-examples/blob/b8d2caa52cf4691e0ddd7c919e4462311deb5ad0/psp22-extension/runtime/psp22-extension-example.rs#L236 + let overhead = contract_host_weight.debug_message; - // charge weight for the cost of the deserialization and the dispatch - let _ = env.charge_weight(base_weight.saturating_add(dispatch_weight))?; + let _ = env.charge_weight(base_weight.saturating_add(overhead))?; + + // read the input as RuntimeCall + let call: ::RuntimeCall = env.read_as_unbounded(len)?; + + let charged_dispatch_weight = env.charge_weight(call.get_dispatch_info().weight)?; log::debug!(target:LOG_TARGET, " dispatch inputted RuntimeCall: {:?}", call); let sender = env.ext().caller(); let origin: T::RuntimeOrigin = RawOrigin::Signed(sender.account_id()?.clone()).into(); - // TODO: uncomment once charged_weight is fixed - // let actual_weight = call.get_dispatch_info().weight; - // env.adjust_weight(charged_weight, actual_weight); let result = call.dispatch(origin); match result { Ok(info) => { log::debug!(target:LOG_TARGET, "dispatch success, actual weight: {:?}", info.actual_weight); + + // refund weight if the actual weight is less than the charged weight + if let Some(actual_weight) = info.actual_weight { + env.adjust_weight(charged_dispatch_weight, actual_weight); + } }, Err(err) => { log::debug!(target:LOG_TARGET, "dispatch failed: error: {:?}", err.error); @@ -142,8 +145,11 @@ where let mut env = env.buf_in_buf_out(); - // TODO: replace with benchmark once available - env.charge_weight(T::DbWeight::get().reads(1_u64))?; + // To be conservative, we charge the weight for reading the input bytes. + // However, charge weight is not strictly necessary here as reading a fixed value's weight is included in the contract call. + // Source: https://github.com/paritytech/polkadot-sdk/blob/117a9433dac88d5ac00c058c9b39c511d47749d2/substrate/frame/contracts/src/wasm/runtime.rs#L563 + let base_weight: Weight = ContractSchedule::::get().host_fn_weights.return_per_byte.saturating_mul(env.in_len().into()); + let _ = env.charge_weight(base_weight)?; let key: ParachainSystemKeys = env.read_as()?; From d25fa7d58795b6f12a771141a5eec0972682a21d Mon Sep 17 00:00:00 2001 From: Peter White Date: Wed, 6 Mar 2024 18:38:01 -0700 Subject: [PATCH 4/7] refactor: fix NFT contract and update redundant LOG_TARGET --- contracts/pop-api-examples/nfts/lib.rs | 1 + runtime/src/extensions.rs | 21 ++++++++++++--------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/contracts/pop-api-examples/nfts/lib.rs b/contracts/pop-api-examples/nfts/lib.rs index 5691f868b..77c5820a8 100755 --- a/contracts/pop-api-examples/nfts/lib.rs +++ b/contracts/pop-api-examples/nfts/lib.rs @@ -14,6 +14,7 @@ impl From for ContractError { } } +#[ink::contract(env = pop_api::Environment)] mod pop_api_extension_demo { use super::ContractError; diff --git a/runtime/src/extensions.rs b/runtime/src/extensions.rs index 406690c41..ea2480b1c 100644 --- a/runtime/src/extensions.rs +++ b/runtime/src/extensions.rs @@ -89,7 +89,7 @@ where + From>, E: Ext, { - const LOG_TARGET: &str = "pop-api::extension::dispatch"; + const LOG_PREFIX: &str = " dispatch |"; let mut env = env.buf_in_buf_out(); let contract_host_weight = ContractSchedule::::get().host_fn_weights; @@ -106,14 +106,15 @@ where // reference: https://github.com/paritytech/ink-examples/blob/b8d2caa52cf4691e0ddd7c919e4462311deb5ad0/psp22-extension/runtime/psp22-extension-example.rs#L236 let overhead = contract_host_weight.debug_message; - let _ = env.charge_weight(base_weight.saturating_add(overhead))?; + let charged_weight = env.charge_weight(base_weight.saturating_add(overhead))?; + log::debug!(target:LOG_TARGET, "{} charged weight: {:?}", LOG_PREFIX, charged_weight); // read the input as RuntimeCall let call: ::RuntimeCall = env.read_as_unbounded(len)?; let charged_dispatch_weight = env.charge_weight(call.get_dispatch_info().weight)?; - log::debug!(target:LOG_TARGET, " dispatch inputted RuntimeCall: {:?}", call); + log::debug!(target:LOG_TARGET, "{} inputted RuntimeCall: {:?}", LOG_PREFIX, call); let sender = env.ext().caller(); let origin: T::RuntimeOrigin = RawOrigin::Signed(sender.account_id()?.clone()).into(); @@ -121,7 +122,7 @@ where let result = call.dispatch(origin); match result { Ok(info) => { - log::debug!(target:LOG_TARGET, "dispatch success, actual weight: {:?}", info.actual_weight); + log::debug!(target:LOG_TARGET, "{} success, actual weight: {:?}", LOG_PREFIX, info.actual_weight); // refund weight if the actual weight is less than the charged weight if let Some(actual_weight) = info.actual_weight { @@ -129,7 +130,7 @@ where } }, Err(err) => { - log::debug!(target:LOG_TARGET, "dispatch failed: error: {:?}", err.error); + log::debug!(target:LOG_TARGET, "{} failed: error: {:?}", LOG_PREFIX, err.error); return Err(err.error); }, } @@ -141,7 +142,7 @@ where T: pallet_contracts::Config + cumulus_pallet_parachain_system::Config + frame_system::Config, E: Ext, { - const LOG_TARGET: &str = "pop-api::extension::read_state"; + const LOG_PREFIX: &str = " read_state |"; let mut env = env.buf_in_buf_out(); @@ -149,7 +150,9 @@ where // However, charge weight is not strictly necessary here as reading a fixed value's weight is included in the contract call. // Source: https://github.com/paritytech/polkadot-sdk/blob/117a9433dac88d5ac00c058c9b39c511d47749d2/substrate/frame/contracts/src/wasm/runtime.rs#L563 let base_weight: Weight = ContractSchedule::::get().host_fn_weights.return_per_byte.saturating_mul(env.in_len().into()); - let _ = env.charge_weight(base_weight)?; + let charged_weight = env.charge_weight(base_weight)?; + + log::debug!(target:LOG_TARGET, "{} charged weight: {:?}", LOG_PREFIX, charged_weight); let key: ParachainSystemKeys = env.read_as()?; @@ -159,7 +162,7 @@ where let relay_block_num: BlockNumber = cumulus_pallet_parachain_system::Pallet::::last_relay_block_number(); log::debug!( target:LOG_TARGET, - "last relay chain block number is: {:?}.", relay_block_num + "{} last relay chain block number is: {:?}.", LOG_PREFIX, relay_block_num ); relay_block_num }, @@ -169,7 +172,7 @@ where .encode(); log::trace!( target:LOG_TARGET, - "read state result: {:?}.", result + "{} result: {:?}.", LOG_PREFIX, result ); env.write(&result, false, None).map_err(|e| { log::trace!(target: LOG_TARGET, "{:?}", e); From 59e4e9900695cbc81048422aa7fc378c3e6756eb Mon Sep 17 00:00:00 2001 From: Peter White Date: Wed, 6 Mar 2024 18:40:43 -0700 Subject: [PATCH 5/7] style: cargo fmt --- runtime/src/extensions.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/runtime/src/extensions.rs b/runtime/src/extensions.rs index ea2480b1c..c76b048ac 100644 --- a/runtime/src/extensions.rs +++ b/runtime/src/extensions.rs @@ -101,10 +101,10 @@ where // reference: https://github.com/paritytech/polkadot-sdk/blob/117a9433dac88d5ac00c058c9b39c511d47749d2/substrate/frame/contracts/src/wasm/runtime.rs#L2159 let base_weight: Weight = contract_host_weight.return_per_byte.saturating_mul(len.into()); - // debug_message weight is a good approximation of the additional overhead of going - // from contract layer to substrate layer. + // debug_message weight is a good approximation of the additional overhead of going + // from contract layer to substrate layer. // reference: https://github.com/paritytech/ink-examples/blob/b8d2caa52cf4691e0ddd7c919e4462311deb5ad0/psp22-extension/runtime/psp22-extension-example.rs#L236 - let overhead = contract_host_weight.debug_message; + let overhead = contract_host_weight.debug_message; let charged_weight = env.charge_weight(base_weight.saturating_add(overhead))?; log::debug!(target:LOG_TARGET, "{} charged weight: {:?}", LOG_PREFIX, charged_weight); @@ -149,7 +149,10 @@ where // To be conservative, we charge the weight for reading the input bytes. // However, charge weight is not strictly necessary here as reading a fixed value's weight is included in the contract call. // Source: https://github.com/paritytech/polkadot-sdk/blob/117a9433dac88d5ac00c058c9b39c511d47749d2/substrate/frame/contracts/src/wasm/runtime.rs#L563 - let base_weight: Weight = ContractSchedule::::get().host_fn_weights.return_per_byte.saturating_mul(env.in_len().into()); + let base_weight: Weight = ContractSchedule::::get() + .host_fn_weights + .return_per_byte + .saturating_mul(env.in_len().into()); let charged_weight = env.charge_weight(base_weight)?; log::debug!(target:LOG_TARGET, "{} charged weight: {:?}", LOG_PREFIX, charged_weight); @@ -159,7 +162,8 @@ where let result = match key { ParachainSystemKeys::LastRelayChainBlockNumber => { env.charge_weight(T::DbWeight::get().reads(1_u64))?; - let relay_block_num: BlockNumber = cumulus_pallet_parachain_system::Pallet::::last_relay_block_number(); + let relay_block_num: BlockNumber = + cumulus_pallet_parachain_system::Pallet::::last_relay_block_number(); log::debug!( target:LOG_TARGET, "{} last relay chain block number is: {:?}.", LOG_PREFIX, relay_block_num From 747ebcb0c9898171a13e6fe27c42b94310dd3706 Mon Sep 17 00:00:00 2001 From: Frank Bell Date: Thu, 7 Mar 2024 10:08:58 +0000 Subject: [PATCH 6/7] refactor: simplify return --- runtime/src/extensions.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/src/extensions.rs b/runtime/src/extensions.rs index c76b048ac..10984b65d 100644 --- a/runtime/src/extensions.rs +++ b/runtime/src/extensions.rs @@ -119,8 +119,7 @@ where let sender = env.ext().caller(); let origin: T::RuntimeOrigin = RawOrigin::Signed(sender.account_id()?.clone()).into(); - let result = call.dispatch(origin); - match result { + match call.dispatch(origin) { Ok(info) => { log::debug!(target:LOG_TARGET, "{} success, actual weight: {:?}", LOG_PREFIX, info.actual_weight); @@ -128,13 +127,14 @@ where if let Some(actual_weight) = info.actual_weight { env.adjust_weight(charged_dispatch_weight, actual_weight); } + + Ok(()) }, Err(err) => { log::debug!(target:LOG_TARGET, "{} failed: error: {:?}", LOG_PREFIX, err.error); - return Err(err.error); + Err(err.error) }, } - Ok(()) } fn read_state(env: Environment) -> Result<(), DispatchError> From 6b9921337ce8d698e18054535ee2c22e0b96daf4 Mon Sep 17 00:00:00 2001 From: Peter White Date: Thu, 7 Mar 2024 11:41:29 -0700 Subject: [PATCH 7/7] chore: update comments --- runtime/src/extensions.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/runtime/src/extensions.rs b/runtime/src/extensions.rs index 10984b65d..4b3eefbb6 100644 --- a/runtime/src/extensions.rs +++ b/runtime/src/extensions.rs @@ -98,7 +98,7 @@ where let len = env.in_len(); // calculate weight for reading bytes of `len` - // reference: https://github.com/paritytech/polkadot-sdk/blob/117a9433dac88d5ac00c058c9b39c511d47749d2/substrate/frame/contracts/src/wasm/runtime.rs#L2159 + // reference: https://github.com/paritytech/polkadot-sdk/blob/117a9433dac88d5ac00c058c9b39c511d47749d2/substrate/frame/contracts/src/wasm/runtime.rs#L267 let base_weight: Weight = contract_host_weight.return_per_byte.saturating_mul(len.into()); // debug_message weight is a good approximation of the additional overhead of going @@ -146,9 +146,7 @@ where let mut env = env.buf_in_buf_out(); - // To be conservative, we charge the weight for reading the input bytes. - // However, charge weight is not strictly necessary here as reading a fixed value's weight is included in the contract call. - // Source: https://github.com/paritytech/polkadot-sdk/blob/117a9433dac88d5ac00c058c9b39c511d47749d2/substrate/frame/contracts/src/wasm/runtime.rs#L563 + // To be conservative, we charge the weight for reading the input bytes of a fixed-size type. let base_weight: Weight = ContractSchedule::::get() .host_fn_weights .return_per_byte