From d409505ba363df01eaf4189d6fd4e6ee590467cd Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Wed, 30 Oct 2019 15:19:25 +0100 Subject: [PATCH 01/36] Initial transformation towards using weights --- node/runtime/src/lib.rs | 3 + srml/contracts/src/gas.rs | 46 ++----- srml/contracts/src/lib.rs | 171 +++++++++++++++++++------- srml/contracts/src/tests.rs | 60 ++++----- srml/contracts/src/wasm/code_cache.rs | 30 +---- srml/system/src/lib.rs | 11 +- 6 files changed, 177 insertions(+), 144 deletions(-) diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index 4f261431462b3..e51193f28d7d3 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -381,6 +381,7 @@ parameter_types! { pub const RentByteFee: Balance = 1 * DOLLARS; pub const RentDepositOffset: Balance = 1000 * DOLLARS; pub const SurchargeReward: Balance = 150 * DOLLARS; + pub const WeightPerGasUnit: Weight = 1; } impl contracts::Trait for Runtime { @@ -410,6 +411,8 @@ impl contracts::Trait for Runtime { type MaxDepth = contracts::DefaultMaxDepth; type MaxValueSize = contracts::DefaultMaxValueSize; type BlockGasLimit = contracts::DefaultBlockGasLimit; + type WeightToFee = LinearWeightToFee; + type WeightPerGasUnit = WeightPerGasUnit; } impl sudo::Trait for Runtime { diff --git a/srml/contracts/src/gas.rs b/srml/contracts/src/gas.rs index 64997fbe81fdf..7bcf5f3391827 100644 --- a/srml/contracts/src/gas.rs +++ b/srml/contracts/src/gas.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{GasSpent, Module, Trait, BalanceOf, NegativeImbalanceOf}; +use crate::{GasUsageReport, Module, Trait, BalanceOf, NegativeImbalanceOf}; use rstd::convert::TryFrom; use sr_primitives::traits::{ CheckedMul, Zero, SaturatedConversion, SimpleArithmetic, UniqueSaturatedInto, @@ -198,50 +198,20 @@ impl GasMeter { /// /// Cost is calculated by multiplying the gas cost (taken from the storage) by the `gas_limit`. /// The funds are deducted from `transactor`. -pub fn buy_gas( - transactor: &T::AccountId, - gas_limit: Gas, -) -> Result<(GasMeter, NegativeImbalanceOf), &'static str> { - // Buy the specified amount of gas. - let gas_price = >::gas_price(); - let cost = if gas_price.is_zero() { - >::zero() - } else { - as TryFrom>::try_from(gas_limit).ok() - .and_then(|gas_limit| gas_price.checked_mul(&gas_limit)) - .ok_or("overflow multiplying gas limit by price")? - }; - - let imbalance = T::Currency::withdraw( - transactor, - cost, - WithdrawReason::Fee.into(), - ExistenceRequirement::KeepAlive - )?; - - Ok((GasMeter::with_limit(gas_limit, gas_price), imbalance)) +pub fn buy_gas(gas_limit: Gas) -> Result, &'static str> { + Ok(GasMeter::with_limit(gas_limit, 1.into())) } /// Refund the unused gas. -pub fn refund_unused_gas( - transactor: &T::AccountId, - gas_meter: GasMeter, - imbalance: NegativeImbalanceOf, -) { +pub fn refund_unused_gas(gas_meter: GasMeter) { let gas_spent = gas_meter.spent(); let gas_left = gas_meter.gas_left(); - // Increase total spent gas. - // This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which - // also has Gas type. - GasSpent::mutate(|block_gas_spent| *block_gas_spent += gas_spent); + GasUsageReport::put((gas_left, gas_spent)); +} - // Refund gas left by the price it was bought at. - let refund = gas_meter.gas_price * gas_left.unique_saturated_into(); - let refund_imbalance = T::Currency::deposit_creating(transactor, refund); - if let Ok(imbalance) = imbalance.offset(refund_imbalance) { - T::GasPayment::on_unbalanced(imbalance); - } +pub fn take_gas_usage_report() -> (Gas, Gas) { // (gas_left, gas_spent) + GasUsageReport::take() } /// A little handy utility for converting a value in balance units into approximate value in gas units diff --git a/srml/contracts/src/lib.rs b/srml/contracts/src/lib.rs index df38747cc5d52..094087d0bc23c 100644 --- a/srml/contracts/src/lib.rs +++ b/srml/contracts/src/lib.rs @@ -113,19 +113,24 @@ use rstd::{prelude::*, marker::PhantomData, fmt::Debug}; use codec::{Codec, Encode, Decode}; use runtime_io::blake2_256; use sr_primitives::{ - traits::{Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension}, - weights::DispatchInfo, + ApplyError, + RuntimeDebug, + traits::{Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension, Convert}, + weights::{DispatchInfo, Weight}, transaction_validity::{ ValidTransaction, InvalidTransaction, TransactionValidity, TransactionValidityError, }, - RuntimeDebug, }; use support::dispatch::{Result, Dispatchable}; use support::{ Parameter, decl_module, decl_event, decl_storage, storage::child, - parameter_types, IsSubType + traits::{ + OnFreeBalanceZero, OnUnbalanced, Currency, Get, Time, WithdrawReason, ExistenceRequirement, + Imbalance, Randomness, + }, + IsSubType, + parameter_types, }; -use support::traits::{OnFreeBalanceZero, OnUnbalanced, Currency, Get, Time, Randomness}; use system::{ensure_signed, RawOrigin, ensure_root}; use primitives::storage::well_known_keys::CHILD_STORAGE_KEY_PREFIX; @@ -416,6 +421,12 @@ pub trait Trait: system::Trait { /// The maximum amount of gas that could be expended per block. type BlockGasLimit: Get; + + /// Convert a weight value into a deductible fee based on the currency type. + type WeightToFee: Convert>; + + /// The number of weight units used by one gas unit. + type WeightPerGasUnit: Get; } /// Simple contract address determiner. @@ -542,21 +553,16 @@ decl_module! { /// You can instantiate contracts only with stored code. pub fn put_code( origin, - #[compact] gas_limit: Gas, code: Vec ) -> Result { - let origin = ensure_signed(origin)?; - - let (mut gas_meter, imbalance) = gas::buy_gas::(&origin, gas_limit)?; + let _origin = ensure_signed(origin)?; let schedule = >::current_schedule(); - let result = wasm::save_code::(code, &mut gas_meter, &schedule); + let result = wasm::save_code::(code, &schedule); if let Ok(code_hash) = result { Self::deposit_event(RawEvent::CodeStored(code_hash)); } - gas::refund_unused_gas::(&origin, gas_meter, imbalance); - result.map(|_| ()) } @@ -645,7 +651,7 @@ decl_module! { } fn on_finalize() { - GasSpent::kill(); + GasUsageReport::kill(); } } } @@ -706,9 +712,9 @@ impl Module { // // NOTE: it is very important to avoid any state changes before // paying for the gas. - let (mut gas_meter, imbalance) = + let mut gas_meter = try_or_exec_error!( - gas::buy_gas::(&origin, gas_limit), + gas::buy_gas::(gas_limit), // We don't have a spare buffer here in the first place, so create a new empty one. Vec::new() ); @@ -729,7 +735,7 @@ impl Module { // // NOTE: This should go after the commit to the storage, since the storage changes // can alter the balance of the caller. - gas::refund_unused_gas::(&origin, gas_meter, imbalance); + gas::refund_unused_gas::(gas_meter); // Execute deferred actions. ctx.deferred.into_iter().for_each(|deferred| { @@ -867,8 +873,10 @@ decl_event! { decl_storage! { trait Store for Module as Contract { - /// Gas spent so far in this block. - GasSpent get(fn gas_spent): Gas; + /// The amount of gas left from the execution of the latest contract. + /// + /// This value is transient and removed at the block finalization stage. + GasUsageReport: (Gas, Gas); /// Current cost schedule for contracts. CurrentSchedule get(fn current_schedule) config(): Schedule = Schedule::default(); /// A mapping from an original code hash to the original code, untouched by instrumentation. @@ -927,9 +935,6 @@ pub struct Schedule { /// Version of the schedule. pub version: u32, - /// Cost of putting a byte of code into storage. - pub put_code_per_byte_cost: Gas, - /// Gas cost of a growing memory by single page. pub grow_mem_cost: Gas, @@ -987,7 +992,6 @@ impl Default for Schedule { fn default() -> Schedule { Schedule { version: 0, - put_code_per_byte_cost: 1, grow_mem_cost: 1, regular_op_cost: 1, return_data_per_byte_cost: 1, @@ -1008,10 +1012,66 @@ impl Default for Schedule { } } +#[doc(hidden)] +pub struct PreDispatchCheckData { + /// The account id who should receive the refund from the gas leftovers. + transactor: AccountId, + /// The negative imbalance obtained by withdrawing the value up to the requested gas limit. + imbalance: NegativeImbalance, +} + /// `SignedExtension` that checks if a transaction would exhausts the block gas limit. #[derive(Encode, Decode, Clone, Eq, PartialEq)] pub struct CheckBlockGasLimit(PhantomData); +impl CheckBlockGasLimit { + fn perform_pre_dispatch_checks( + who: &T::AccountId, + call: &::Call, + ) -> rstd::result::Result>>, TransactionValidityError> { + let call = match call.is_sub_type() { + Some(call) => call, + None => return Ok(None), + }; + + match *call { + Call::claim_surcharge(_, _) | Call::update_schedule(_) | Call::put_code(_) => Ok(None), + Call::call(_, _, gas_limit, _) | Call::instantiate(_, gas_limit, _, _) => { + // Compute how much block weight this transaction can take up in case if it + // depleted devoted gas to zero. + let gas_weight_limit = gas_to_weight::(gas_limit); + + // Obtain the the available amount of weight left in the current block and discard + // this transaction if it can potentially overrun the available amount. + let weight_available = ::MaximumBlockWeight::get() + .saturating_sub(>::all_extrinsics_weight()); + if gas_weight_limit > weight_available { + return Err(InvalidTransaction::ExhaustsResources.into()); + } + + // Compute the fee corresponding for the given gas_weight_limit and attempt + // withdrawing from the origin of this transaction. + let fee = T::WeightToFee::convert(gas_weight_limit); + let imbalance = match T::Currency::withdraw( + who, + fee, + WithdrawReason::TransactionPayment.into(), + ExistenceRequirement::KeepAlive, + ) { + Ok(imbalance) => imbalance, + Err(_) => return Err(InvalidTransaction::Payment.into()), + }; + + Ok(Some(PreDispatchCheckData { + transactor: who.clone(), + imbalance, + })) + }, + Call::__PhantomItem(_, _) => unreachable!("Variant is never constructed"), + } + } +} + impl Default for CheckBlockGasLimit { fn default() -> Self { Self(PhantomData) @@ -1034,40 +1094,59 @@ impl SignedExtension for CheckBlockGasLimit { type AccountId = T::AccountId; type Call = ::Call; type AdditionalSigned = (); - type Pre = (); + /// Optional pre-dispatch check data. + /// + /// It is present only for the contract calls that operate with gas. + type Pre = Option>>; fn additional_signed(&self) -> rstd::result::Result<(), TransactionValidityError> { Ok(()) } + fn pre_dispatch( + self, + who: &Self::AccountId, + call: &Self::Call, + _: DispatchInfo, + _: usize, + ) -> rstd::result::Result { + Self::perform_pre_dispatch_checks(who, call) + .map_err(Into::into) + } + + fn validate( &self, - _: &Self::AccountId, + who: &Self::AccountId, call: &Self::Call, _: DispatchInfo, _: usize, ) -> TransactionValidity { - let call = match call.is_sub_type() { - Some(call) => call, - None => return Ok(ValidTransaction::default()), - }; + Self::perform_pre_dispatch_checks(who, call) + .map(|_| ValidTransaction::default()) + } - match call { - Call::claim_surcharge(_, _) | Call::update_schedule(_) => - Ok(ValidTransaction::default()), - Call::put_code(gas_limit, _) - | Call::call(_, _, gas_limit, _) - | Call::instantiate(_, gas_limit, _, _) - => { - // Check if the specified amount of gas is available in the current block. - // This cannot underflow since `gas_spent` is never greater than `T::BlockGasLimit`. - let gas_available = T::BlockGasLimit::get() - >::gas_spent(); - if *gas_limit > gas_available { - // gas limit reached, revert the transaction and retry again in the future - InvalidTransaction::ExhaustsResources.into() - } else { - Ok(ValidTransaction::default()) - } - }, - Call::__PhantomItem(_, _) => unreachable!("Variant is never constructed"), + fn post_dispatch(pre: Self::Pre, _info: DispatchInfo, _len: usize) { + if let Some(PreDispatchCheckData { + transactor, + imbalance, + }) = pre { + let (gas_left, gas_spent) = gas::take_gas_usage_report(); + + let unused_weight = gas_to_weight::(gas_left); + let refund = T::WeightToFee::convert(unused_weight); + + // Refund the unused gas. + let refund_imbalance = T::Currency::deposit_creating(&transactor, refund); + if let Ok(imbalance) = imbalance.offset(refund_imbalance) { + T::GasPayment::on_unbalanced(imbalance); + } + + let spent_weight = gas_to_weight::(gas_spent); + let next_weight = >::all_extrinsics_weight().saturating_add(spent_weight); + >::set_extrinsics_weight(next_weight); } } } + +fn gas_to_weight(gas: Gas) -> Weight { + (T::WeightPerGasUnit::get() as Gas * gas) as Weight +} diff --git a/srml/contracts/src/tests.rs b/srml/contracts/src/tests.rs index 7a13a66de26c7..76169c07824c4 100644 --- a/srml/contracts/src/tests.rs +++ b/srml/contracts/src/tests.rs @@ -148,6 +148,7 @@ parameter_types! { pub const InstantiateBaseFee: u64 = 175; pub const MaxDepth: u32 = 100; pub const MaxValueSize: u32 = 16_384; + pub const WeightPerGasUnit: u32 = 1; } impl Trait for Test { type Currency = Balances; @@ -176,6 +177,8 @@ impl Trait for Test { type MaxDepth = MaxDepth; type MaxValueSize = MaxValueSize; type BlockGasLimit = BlockGasLimit; + type WeightToFee = (); + type WeightPerGasUnit = WeightPerGasUnit; } type Balances = balances::Module; @@ -296,17 +299,12 @@ fn compile_module(wabt_module: &str) Ok((wasm, code_hash)) } -// Perform a simple transfer to a non-existent account supplying way more gas than needed. -// Then we check that the all unused gas is refunded. #[test] -fn refunds_unused_gas() { +fn call_doesnt_consume_gas() { ExtBuilder::default().gas_price(2).build().execute_with(|| { Balances::deposit_creating(&ALICE, 100_000_000); - assert_ok!(Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, Vec::new())); - - // 2 * 135 - gas price multiplied by the call base fee. - assert_eq!(Balances::free_balance(&ALICE), 100_000_000 - (2 * 135)); + assert_eq!(Balances::free_balance(&ALICE), 100_000_000); }); } @@ -412,7 +410,7 @@ fn instantiate_and_call_and_deposit_event() { ExtBuilder::default().existential_deposit(100).build().execute_with(|| { Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); // Check at the end to get hash on error easily let creation = Contract::instantiate( @@ -492,7 +490,7 @@ fn dispatch_call() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); // Let's keep this assert even though it's redundant. If you ever need to update the // wasm source this test will fail and will show you the actual hash. @@ -610,7 +608,7 @@ fn dispatch_call_not_dispatched_after_top_level_transaction_failure() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); // Let's keep this assert even though it's redundant. If you ever need to update the // wasm source this test will fail and will show you the actual hash. @@ -809,7 +807,7 @@ fn test_set_rent_code_and_hash() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); // If you ever need to update the wasm source this test will fail // and will show you the actual hash. @@ -836,7 +834,7 @@ fn storage_size() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 30_000, @@ -863,7 +861,7 @@ fn deduct_blocks() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 30_000, @@ -957,7 +955,7 @@ fn claim_surcharge(blocks: u64, trigger_call: impl Fn() -> bool, removes: bool) ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 100, @@ -990,7 +988,7 @@ fn removals(trigger_call: impl Fn() -> bool) { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm.clone())); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm.clone())); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 100, @@ -1026,7 +1024,7 @@ fn removals(trigger_call: impl Fn() -> bool) { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm.clone())); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm.clone())); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 1_000, @@ -1061,7 +1059,7 @@ fn removals(trigger_call: impl Fn() -> bool) { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm.clone())); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm.clone())); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 50+Balances::minimum_balance(), @@ -1105,7 +1103,7 @@ fn call_removed_contract() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm.clone())); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm.clone())); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 100, @@ -1190,7 +1188,7 @@ fn default_rent_allowance_on_instantiate() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 30_000, @@ -1303,8 +1301,8 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: ExtBuilder::default().existential_deposit(50).build().execute_with(|| { Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, restoration_wasm)); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, set_rent_wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), restoration_wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), set_rent_wasm)); // If you ever need to update the wasm source this test will fail // and will show you the actual hash. @@ -1487,7 +1485,7 @@ fn storage_max_value_limit() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 30_000, @@ -1851,8 +1849,8 @@ fn deploy_and_call_other_contract() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, callee_wasm)); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, caller_wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), callee_wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), caller_wasm)); assert_ok!(Contract::instantiate( Origin::signed(ALICE), @@ -1978,7 +1976,7 @@ fn self_destruct_by_draining_balance() { let (wasm, code_hash) = compile_module::(CODE_SELF_DESTRUCT).unwrap(); ExtBuilder::default().existential_deposit(50).build().execute_with(|| { Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); // Instantiate the BOB contract. assert_ok!(Contract::instantiate( @@ -2014,7 +2012,7 @@ fn cannot_self_destruct_while_live() { let (wasm, code_hash) = compile_module::(CODE_SELF_DESTRUCT).unwrap(); ExtBuilder::default().existential_deposit(50).build().execute_with(|| { Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); // Instantiate the BOB contract. assert_ok!(Contract::instantiate( @@ -2214,8 +2212,8 @@ fn destroy_contract_and_transfer_funds() { ExtBuilder::default().existential_deposit(50).build().execute_with(|| { // Create Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, callee_wasm)); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, caller_wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), callee_wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), caller_wasm)); // This deploys the BOB contract, which in turn deploys the CHARLIE contract during // construction. @@ -2309,7 +2307,7 @@ fn cannot_self_destruct_in_constructor() { let (wasm, code_hash) = compile_module::(CODE_SELF_DESTRUCTING_CONSTRUCTOR).unwrap(); ExtBuilder::default().existential_deposit(50).build().execute_with(|| { Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); // Fail to instantiate the BOB contract since its final balance is below existential // deposit. @@ -2326,12 +2324,14 @@ fn cannot_self_destruct_in_constructor() { }); } +// TODO: This test relies on put_code, which no longer requires any gas. +#[ignore] #[test] fn check_block_gas_limit_works() { ExtBuilder::default().block_gas_limit(50).build().execute_with(|| { let info = DispatchInfo { weight: 100, class: DispatchClass::Normal }; let check = CheckBlockGasLimit::(Default::default()); - let call: Call = crate::Call::put_code(1000, vec![]).into(); + let call: Call = crate::Call::put_code(vec![]).into(); assert_eq!( check.validate(&0, &call, info, 0), InvalidTransaction::ExhaustsResources.into(), diff --git a/srml/contracts/src/wasm/code_cache.rs b/srml/contracts/src/wasm/code_cache.rs index e6702d29cf5c7..9edb4c4612f6e 100644 --- a/srml/contracts/src/wasm/code_cache.rs +++ b/srml/contracts/src/wasm/code_cache.rs @@ -30,45 +30,17 @@ use crate::gas::{Gas, GasMeter, Token}; use crate::wasm::{prepare, runtime::Env, PrefabWasmModule}; use crate::{CodeHash, CodeStorage, PristineCode, Schedule, Trait}; use rstd::prelude::*; -use sr_primitives::traits::{Hash, Bounded}; +use sr_primitives::traits::Hash; use support::StorageMap; -/// Gas metering token that used for charging storing code into the code storage. -/// -/// Specifies the code length in bytes. -#[cfg_attr(test, derive(Debug, PartialEq, Eq))] -#[derive(Copy, Clone)] -pub struct PutCodeToken(u32); - -impl Token for PutCodeToken { - type Metadata = Schedule; - - fn calculate_amount(&self, metadata: &Schedule) -> Gas { - metadata - .put_code_per_byte_cost - .checked_mul(self.0.into()) - .unwrap_or_else(|| Bounded::max_value()) - } -} - /// Put code in the storage. The hash of code is used as a key and is returned /// as a result of this function. /// /// This function instruments the given code and caches it in the storage. pub fn save( original_code: Vec, - gas_meter: &mut GasMeter, schedule: &Schedule, ) -> Result, &'static str> { - // The first time instrumentation is on the user. However, consequent reinstrumentation - // due to the schedule changes is on governance system. - if gas_meter - .charge(schedule, PutCodeToken(original_code.len() as u32)) - .is_out_of_gas() - { - return Err("there is not enough gas for storing the code"); - } - let prefab_module = prepare::prepare_contract::(&original_code, schedule)?; let code_hash = T::Hashing::hash(&original_code); diff --git a/srml/system/src/lib.rs b/srml/system/src/lib.rs index e07b937751267..15d1b968d9091 100644 --- a/srml/system/src/lib.rs +++ b/srml/system/src/lib.rs @@ -597,11 +597,20 @@ impl Module { ExtrinsicCount::get().unwrap_or_default() } - /// Gets a total weight of all executed extrinsics. + /// Gets a total weight of all executed extrinsics so far. pub fn all_extrinsics_weight() -> Weight { AllExtrinsicsWeight::get().unwrap_or_default() } + /// Sets the total weight of all executed extrinsics so far. + /// + /// Note that generally the total extrinsics weight is tracked by the this module. You might + /// find this method useful only if you are implementing a custom weight handling logic for + /// transactions. + pub fn set_extrinsics_weight(current_weight: Weight) { + AllExtrinsicsWeight::put(current_weight) + } + pub fn all_extrinsics_len() -> u32 { AllExtrinsicsLen::get().unwrap_or_default() } From 169d461a02c30bc387594f685e8e108ccd5a89e9 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Wed, 30 Oct 2019 16:27:22 +0100 Subject: [PATCH 02/36] Clean --- srml/contracts/src/gas.rs | 9 +++------ srml/contracts/src/lib.rs | 12 ++++++------ srml/contracts/src/wasm/code_cache.rs | 1 - 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/srml/contracts/src/gas.rs b/srml/contracts/src/gas.rs index 7bcf5f3391827..120d59819f00d 100644 --- a/srml/contracts/src/gas.rs +++ b/srml/contracts/src/gas.rs @@ -14,14 +14,11 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{GasUsageReport, Module, Trait, BalanceOf, NegativeImbalanceOf}; -use rstd::convert::TryFrom; +use crate::{GasUsageReport, Trait, BalanceOf}; use sr_primitives::traits::{ - CheckedMul, Zero, SaturatedConversion, SimpleArithmetic, UniqueSaturatedInto, -}; -use support::{ - traits::{Currency, ExistenceRequirement, Imbalance, OnUnbalanced, WithdrawReason}, StorageValue, + Zero, SaturatedConversion, SimpleArithmetic, }; +use support::StorageValue; #[cfg(test)] use std::{any::Any, fmt::Debug}; diff --git a/srml/contracts/src/lib.rs b/srml/contracts/src/lib.rs index 094087d0bc23c..c392f00beacd9 100644 --- a/srml/contracts/src/lib.rs +++ b/srml/contracts/src/lib.rs @@ -1013,7 +1013,7 @@ impl Default for Schedule { } #[doc(hidden)] -pub struct PreDispatchCheckData { +pub struct DynamicWeightData { /// The account id who should receive the refund from the gas leftovers. transactor: AccountId, /// The negative imbalance obtained by withdrawing the value up to the requested gas limit. @@ -1025,10 +1025,11 @@ pub struct PreDispatchCheckData { pub struct CheckBlockGasLimit(PhantomData); impl CheckBlockGasLimit { + /// Perform pre-dispatch checks for the given call and origin. fn perform_pre_dispatch_checks( who: &T::AccountId, call: &::Call, - ) -> rstd::result::Result>>, TransactionValidityError> { + ) -> rstd::result::Result>>, TransactionValidityError> { let call = match call.is_sub_type() { Some(call) => call, None => return Ok(None), @@ -1062,7 +1063,7 @@ impl CheckBlockGasLimit { Err(_) => return Err(InvalidTransaction::Payment.into()), }; - Ok(Some(PreDispatchCheckData { + Ok(Some(DynamicWeightData { transactor: who.clone(), imbalance, })) @@ -1097,7 +1098,7 @@ impl SignedExtension for CheckBlockGasLimit { /// Optional pre-dispatch check data. /// /// It is present only for the contract calls that operate with gas. - type Pre = Option>>; + type Pre = Option>>; fn additional_signed(&self) -> rstd::result::Result<(), TransactionValidityError> { Ok(()) } @@ -1112,7 +1113,6 @@ impl SignedExtension for CheckBlockGasLimit { .map_err(Into::into) } - fn validate( &self, who: &Self::AccountId, @@ -1125,7 +1125,7 @@ impl SignedExtension for CheckBlockGasLimit { } fn post_dispatch(pre: Self::Pre, _info: DispatchInfo, _len: usize) { - if let Some(PreDispatchCheckData { + if let Some(DynamicWeightData { transactor, imbalance, }) = pre { diff --git a/srml/contracts/src/wasm/code_cache.rs b/srml/contracts/src/wasm/code_cache.rs index 9edb4c4612f6e..d48273ab16922 100644 --- a/srml/contracts/src/wasm/code_cache.rs +++ b/srml/contracts/src/wasm/code_cache.rs @@ -26,7 +26,6 @@ //! this guarantees that every instrumented contract code in cache cannot have the version equal to the current one. //! Thus, before executing a contract it should be reinstrument with new schedule. -use crate::gas::{Gas, GasMeter, Token}; use crate::wasm::{prepare, runtime::Env, PrefabWasmModule}; use crate::{CodeHash, CodeStorage, PristineCode, Schedule, Trait}; use rstd::prelude::*; From 64ff1944a835ad5fbd2874cc650b93dabe130ae6 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Wed, 30 Oct 2019 19:55:54 +0100 Subject: [PATCH 03/36] Gas price calculation for weight --- node/cli/src/chain_spec.rs | 1 - node/testing/src/genesis.rs | 1 - srml/contracts/src/gas.rs | 25 ++----------------------- srml/contracts/src/lib.rs | 35 ++++++++++++++++++++++++----------- srml/contracts/src/tests.rs | 11 ++--------- 5 files changed, 28 insertions(+), 45 deletions(-) diff --git a/node/cli/src/chain_spec.rs b/node/cli/src/chain_spec.rs index 04fb41c211009..d966d1fea1af3 100644 --- a/node/cli/src/chain_spec.rs +++ b/node/cli/src/chain_spec.rs @@ -248,7 +248,6 @@ pub fn testnet_genesis( enable_println, // this should only be enabled on development chains ..Default::default() }, - gas_price: 1 * MILLICENTS, }), sudo: Some(SudoConfig { key: root_key, diff --git a/node/testing/src/genesis.rs b/node/testing/src/genesis.rs index 0b99052f25026..ac62b0fb394db 100644 --- a/node/testing/src/genesis.rs +++ b/node/testing/src/genesis.rs @@ -82,7 +82,6 @@ pub fn config(support_changes_trie: bool, code: Option<&[u8]>) -> GenesisConfig }), contracts: Some(ContractsConfig { current_schedule: Default::default(), - gas_price: 1 * MILLICENTS, }), babe: Some(Default::default()), grandpa: Some(GrandpaConfig { diff --git a/srml/contracts/src/gas.rs b/srml/contracts/src/gas.rs index 120d59819f00d..70fde36a37560 100644 --- a/srml/contracts/src/gas.rs +++ b/srml/contracts/src/gas.rs @@ -14,11 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{GasUsageReport, Trait, BalanceOf}; +use crate::{Trait, BalanceOf}; use sr_primitives::traits::{ Zero, SaturatedConversion, SimpleArithmetic, }; -use support::StorageValue; #[cfg(test)] use std::{any::Any, fmt::Debug}; @@ -181,7 +180,7 @@ impl GasMeter { } /// Returns how much gas was spent. - fn spent(&self) -> Gas { + pub fn spent(&self) -> Gas { self.limit - self.gas_left } @@ -191,26 +190,6 @@ impl GasMeter { } } -/// Buy the given amount of gas. -/// -/// Cost is calculated by multiplying the gas cost (taken from the storage) by the `gas_limit`. -/// The funds are deducted from `transactor`. -pub fn buy_gas(gas_limit: Gas) -> Result, &'static str> { - Ok(GasMeter::with_limit(gas_limit, 1.into())) -} - -/// Refund the unused gas. -pub fn refund_unused_gas(gas_meter: GasMeter) { - let gas_spent = gas_meter.spent(); - let gas_left = gas_meter.gas_left(); - - GasUsageReport::put((gas_left, gas_spent)); -} - -pub fn take_gas_usage_report() -> (Gas, Gas) { // (gas_left, gas_spent) - GasUsageReport::take() -} - /// A little handy utility for converting a value in balance units into approximate value in gas units /// at the given gas price. pub fn approx_gas_for_balance(gas_price: Balance, balance: Balance) -> Gas diff --git a/srml/contracts/src/lib.rs b/srml/contracts/src/lib.rs index c392f00beacd9..f3024edabcbe6 100644 --- a/srml/contracts/src/lib.rs +++ b/srml/contracts/src/lib.rs @@ -115,7 +115,10 @@ use runtime_io::blake2_256; use sr_primitives::{ ApplyError, RuntimeDebug, - traits::{Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension, Convert}, + traits::{ + Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension, Convert, + CheckedDiv, + }, weights::{DispatchInfo, Weight}, transaction_validity::{ ValidTransaction, InvalidTransaction, TransactionValidity, TransactionValidityError, @@ -652,6 +655,7 @@ decl_module! { fn on_finalize() { GasUsageReport::kill(); + >::kill(); } } } @@ -708,13 +712,10 @@ impl Module { gas_limit: Gas, func: impl FnOnce(&mut ExecutionContext, &mut GasMeter) -> ExecResult ) -> ExecResult { - // Pay for the gas upfront. - // - // NOTE: it is very important to avoid any state changes before - // paying for the gas. + // TODO: Set the proper gas price. let mut gas_meter = try_or_exec_error!( - gas::buy_gas::(gas_limit), + Ok(GasMeter::::with_limit(gas_limit, 1.into())), // We don't have a spare buffer here in the first place, so create a new empty one. Vec::new() ); @@ -731,11 +732,13 @@ impl Module { DirectAccountDb.commit(ctx.overlay.into_change_set()); } - // Refund cost of the unused gas. + // Save the gas usage report. // // NOTE: This should go after the commit to the storage, since the storage changes // can alter the balance of the caller. - gas::refund_unused_gas::(gas_meter); + let gas_spent = gas_meter.spent(); + let gas_left = gas_meter.gas_left(); + GasUsageReport::put((gas_left, gas_spent)); // Execute deferred actions. ctx.deferred.into_iter().for_each(|deferred| { @@ -875,7 +878,7 @@ decl_storage! { trait Store for Module as Contract { /// The amount of gas left from the execution of the latest contract. /// - /// This value is transient and removed at the block finalization stage. + /// This value is transient and removed before block finalization. GasUsageReport: (Gas, Gas); /// Current cost schedule for contracts. CurrentSchedule get(fn current_schedule) config(): Schedule = Schedule::default(); @@ -888,7 +891,9 @@ decl_storage! { /// The code associated with a given account. pub ContractInfoOf: map T::AccountId => Option>; /// The price of one unit of gas. - GasPrice get(fn gas_price) config(): BalanceOf = 1.into(); + /// + /// This value is transint and remove before block finalization. + GasPrice: BalanceOf = 1.into(); } } @@ -1053,6 +1058,13 @@ impl CheckBlockGasLimit { // Compute the fee corresponding for the given gas_weight_limit and attempt // withdrawing from the origin of this transaction. let fee = T::WeightToFee::convert(gas_weight_limit); + + // Compute and store the effective price per unit of gas. + let gas_price = >::from(gas_weight_limit) + .checked_div(&fee) + .unwrap_or(1.into()); + >::put(gas_price); + let imbalance = match T::Currency::withdraw( who, fee, @@ -1129,7 +1141,8 @@ impl SignedExtension for CheckBlockGasLimit { transactor, imbalance, }) = pre { - let (gas_left, gas_spent) = gas::take_gas_usage_report(); + let (gas_left, gas_spent) = GasUsageReport::take(); + let unused_weight = gas_to_weight::(gas_left); let refund = T::WeightToFee::convert(unused_weight); diff --git a/srml/contracts/src/tests.rs b/srml/contracts/src/tests.rs index 76169c07824c4..d2466cdbe08db 100644 --- a/srml/contracts/src/tests.rs +++ b/srml/contracts/src/tests.rs @@ -228,7 +228,6 @@ const DJANGO: u64 = 4; pub struct ExtBuilder { existential_deposit: u64, - gas_price: u64, block_gas_limit: u64, transfer_fee: u64, instantiation_fee: u64, @@ -237,7 +236,6 @@ impl Default for ExtBuilder { fn default() -> Self { Self { existential_deposit: 0, - gas_price: 2, block_gas_limit: 100_000_000, transfer_fee: 0, instantiation_fee: 0, @@ -249,10 +247,6 @@ impl ExtBuilder { self.existential_deposit = existential_deposit; self } - pub fn gas_price(mut self, gas_price: u64) -> Self { - self.gas_price = gas_price; - self - } pub fn block_gas_limit(mut self, block_gas_limit: u64) -> Self { self.block_gas_limit = block_gas_limit; self @@ -278,12 +272,11 @@ impl ExtBuilder { balances: vec![], vesting: vec![], }.assimilate_storage(&mut t).unwrap(); - GenesisConfig:: { + GenesisConfig { current_schedule: Schedule { enable_println: true, ..Default::default() }, - gas_price: self.gas_price, }.assimilate_storage(&mut t).unwrap(); runtime_io::TestExternalities::new(t) } @@ -301,7 +294,7 @@ fn compile_module(wabt_module: &str) #[test] fn call_doesnt_consume_gas() { - ExtBuilder::default().gas_price(2).build().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { Balances::deposit_creating(&ALICE, 100_000_000); assert_ok!(Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, Vec::new())); assert_eq!(Balances::free_balance(&ALICE), 100_000_000); From 00dd58ef0a8577eab3a1af9a9653e6d35ef07ed3 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Wed, 30 Oct 2019 19:57:20 +0100 Subject: [PATCH 04/36] Doc --- srml/contracts/src/tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/srml/contracts/src/tests.rs b/srml/contracts/src/tests.rs index d2466cdbe08db..6e0ab9ea204e2 100644 --- a/srml/contracts/src/tests.rs +++ b/srml/contracts/src/tests.rs @@ -292,8 +292,9 @@ fn compile_module(wabt_module: &str) Ok((wasm, code_hash)) } +/// The payment for gas is handled outside of contract execution (e.g `call`) functions. #[test] -fn call_doesnt_consume_gas() { +fn call_doesnt_pay_for_gas() { ExtBuilder::default().build().execute_with(|| { Balances::deposit_creating(&ALICE, 100_000_000); assert_ok!(Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, Vec::new())); From c0a89b2355fbf15f988fa42a5b293d7824354071 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Wed, 30 Oct 2019 20:55:43 +0100 Subject: [PATCH 05/36] Eradicate gas block limit and fix the test. --- node/runtime/src/lib.rs | 1 - srml/contracts/src/lib.rs | 10 +--------- srml/contracts/src/tests.rs | 32 +++++++++++--------------------- 3 files changed, 12 insertions(+), 31 deletions(-) diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index e51193f28d7d3..61c3ad31c005e 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -410,7 +410,6 @@ impl contracts::Trait for Runtime { type InstantiateBaseFee = contracts::DefaultInstantiateBaseFee; type MaxDepth = contracts::DefaultMaxDepth; type MaxValueSize = contracts::DefaultMaxValueSize; - type BlockGasLimit = contracts::DefaultBlockGasLimit; type WeightToFee = LinearWeightToFee; type WeightPerGasUnit = WeightPerGasUnit; } diff --git a/srml/contracts/src/lib.rs b/srml/contracts/src/lib.rs index f3024edabcbe6..fc0c89e784d4a 100644 --- a/srml/contracts/src/lib.rs +++ b/srml/contracts/src/lib.rs @@ -333,8 +333,6 @@ parameter_types! { pub const DefaultMaxDepth: u32 = 32; /// A reasonable default value for [`Trait::MaxValueSize`]. pub const DefaultMaxValueSize: u32 = 16_384; - /// A reasonable default value for [`Trait::BlockGasLimit`]. - pub const DefaultBlockGasLimit: u32 = 10_000_000; } pub trait Trait: system::Trait { @@ -422,9 +420,6 @@ pub trait Trait: system::Trait { /// The maximum size of a storage value in bytes. type MaxValueSize: Get; - /// The maximum amount of gas that could be expended per block. - type BlockGasLimit: Get; - /// Convert a weight value into a deductible fee based on the currency type. type WeightToFee: Convert>; @@ -531,10 +526,6 @@ decl_module! { /// The maximum size of a storage value in bytes. A reasonable default is 16 KiB. const MaxValueSize: u32 = T::MaxValueSize::get(); - /// The maximum amount of gas that could be expended per block. A reasonable - /// default value is 10_000_000. - const BlockGasLimit: Gas = T::BlockGasLimit::get(); - fn deposit_event() = default; /// Updates the schedule for metering contracts. @@ -1018,6 +1009,7 @@ impl Default for Schedule { } #[doc(hidden)] +#[cfg_attr(test, derive(Debug))] pub struct DynamicWeightData { /// The account id who should receive the refund from the gas leftovers. transactor: AccountId, diff --git a/srml/contracts/src/tests.rs b/srml/contracts/src/tests.rs index 6e0ab9ea204e2..6242c273122f7 100644 --- a/srml/contracts/src/tests.rs +++ b/srml/contracts/src/tests.rs @@ -67,7 +67,6 @@ thread_local! { static EXISTENTIAL_DEPOSIT: RefCell = RefCell::new(0); static TRANSFER_FEE: RefCell = RefCell::new(0); static INSTANTIATION_FEE: RefCell = RefCell::new(0); - static BLOCK_GAS_LIMIT: RefCell = RefCell::new(0); } pub struct ExistentialDeposit; @@ -85,10 +84,6 @@ impl Get for CreationFee { fn get() -> u64 { INSTANTIATION_FEE.with(|v| *v.borrow()) } } -pub struct BlockGasLimit; -impl Get for BlockGasLimit { - fn get() -> u64 { BLOCK_GAS_LIMIT.with(|v| *v.borrow()) } -} #[derive(Clone, Eq, PartialEq, Debug)] pub struct Test; @@ -176,7 +171,6 @@ impl Trait for Test { type InstantiateBaseFee = InstantiateBaseFee; type MaxDepth = MaxDepth; type MaxValueSize = MaxValueSize; - type BlockGasLimit = BlockGasLimit; type WeightToFee = (); type WeightPerGasUnit = WeightPerGasUnit; } @@ -228,7 +222,6 @@ const DJANGO: u64 = 4; pub struct ExtBuilder { existential_deposit: u64, - block_gas_limit: u64, transfer_fee: u64, instantiation_fee: u64, } @@ -236,7 +229,6 @@ impl Default for ExtBuilder { fn default() -> Self { Self { existential_deposit: 0, - block_gas_limit: 100_000_000, transfer_fee: 0, instantiation_fee: 0, } @@ -247,10 +239,6 @@ impl ExtBuilder { self.existential_deposit = existential_deposit; self } - pub fn block_gas_limit(mut self, block_gas_limit: u64) -> Self { - self.block_gas_limit = block_gas_limit; - self - } pub fn transfer_fee(mut self, transfer_fee: u64) -> Self { self.transfer_fee = transfer_fee; self @@ -263,7 +251,6 @@ impl ExtBuilder { EXISTENTIAL_DEPOSIT.with(|v| *v.borrow_mut() = self.existential_deposit); TRANSFER_FEE.with(|v| *v.borrow_mut() = self.transfer_fee); INSTANTIATION_FEE.with(|v| *v.borrow_mut() = self.instantiation_fee); - BLOCK_GAS_LIMIT.with(|v| *v.borrow_mut() = self.block_gas_limit); } pub fn build(self) -> runtime_io::TestExternalities { self.set_associated_consts(); @@ -2319,19 +2306,22 @@ fn cannot_self_destruct_in_constructor() { } // TODO: This test relies on put_code, which no longer requires any gas. -#[ignore] #[test] fn check_block_gas_limit_works() { - ExtBuilder::default().block_gas_limit(50).build().execute_with(|| { + ExtBuilder::default().build().execute_with(|| { let info = DispatchInfo { weight: 100, class: DispatchClass::Normal }; let check = CheckBlockGasLimit::(Default::default()); let call: Call = crate::Call::put_code(vec![]).into(); + match check.pre_dispatch(&0, &call, info, 0) { + Ok(None) => {}, + _ => panic!("put_code is not dynamic and should pass validation"), + } - assert_eq!( - check.validate(&0, &call, info, 0), InvalidTransaction::ExhaustsResources.into(), - ); - - let call: Call = crate::Call::update_schedule(Default::default()).into(); - assert_eq!(check.validate(&0, &call, info, 0), Ok(Default::default())); + let check = CheckBlockGasLimit::(Default::default()); + let call: Call = crate::Call::call(Default::default(), 0, 100, vec![]).into(); + match check.pre_dispatch(&0, &call, info, 0) { + Ok(Some(_)) => {}, + _ => panic!(), + } }); } From 78bb5254486e6681410659092622c502ecbf0a3e Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Thu, 31 Oct 2019 17:08:54 +0100 Subject: [PATCH 06/36] Set proper gas_price --- srml/contracts/src/lib.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/srml/contracts/src/lib.rs b/srml/contracts/src/lib.rs index fc0c89e784d4a..5ec05e2c1aef9 100644 --- a/srml/contracts/src/lib.rs +++ b/srml/contracts/src/lib.rs @@ -643,11 +643,6 @@ decl_module! { T::Currency::deposit_into_existing(&rewarded, T::SurchargeReward::get())?; } } - - fn on_finalize() { - GasUsageReport::kill(); - >::kill(); - } } } @@ -703,10 +698,12 @@ impl Module { gas_limit: Gas, func: impl FnOnce(&mut ExecutionContext, &mut GasMeter) -> ExecResult ) -> ExecResult { - // TODO: Set the proper gas price. + // Take the gas price prepared by the signed extension. + let gas_price = GasPrice::::take(); + debug_assert!(gas_price != 0.into()); let mut gas_meter = try_or_exec_error!( - Ok(GasMeter::::with_limit(gas_limit, 1.into())), + Ok(GasMeter::::with_limit(gas_limit, gas_price)), // We don't have a spare buffer here in the first place, so create a new empty one. Vec::new() ); @@ -1052,11 +1049,18 @@ impl CheckBlockGasLimit { let fee = T::WeightToFee::convert(gas_weight_limit); // Compute and store the effective price per unit of gas. - let gas_price = >::from(gas_weight_limit) - .checked_div(&fee) + let gas_price = fee + .checked_div(&>::from(gas_weight_limit)) .unwrap_or(1.into()); >::put(gas_price); + if true { + use rstd::convert::TryInto; + runtime_io::print_num(gas_weight_limit.try_into().unwrap_or(0) as u64); + runtime_io::print_num(fee.try_into().unwrap_or(0) as u64); + runtime_io::print_num(gas_price.try_into().unwrap_or(0) as u64); + } + let imbalance = match T::Currency::withdraw( who, fee, From a1340cda377fffcaba07445f1c32b2ffc1c0dadb Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Thu, 31 Oct 2019 20:52:12 +0100 Subject: [PATCH 07/36] Bump runtime version --- node/runtime/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index 61c3ad31c005e..de345cd67a0c7 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -81,8 +81,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to equal spec_version. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 190, - impl_version: 190, + spec_version: 191, + impl_version: 191, apis: RUNTIME_API_VERSIONS, }; From 8212f50b955fa5523056093522b444448d759f81 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Tue, 5 Nov 2019 13:56:10 +0100 Subject: [PATCH 08/36] WIP, workaround by scaling gas usage --- node/executor/src/lib.rs | 2 +- node/runtime/src/lib.rs | 14 ++++++-------- srml/contracts/src/lib.rs | 25 +++++++++++-------------- srml/contracts/src/tests.rs | 2 -- 4 files changed, 18 insertions(+), 25 deletions(-) diff --git a/node/executor/src/lib.rs b/node/executor/src/lib.rs index 39727d1d5503e..4d9c9d086da15 100644 --- a/node/executor/src/lib.rs +++ b/node/executor/src/lib.rs @@ -713,7 +713,7 @@ mod tests { CheckedExtrinsic { signed: Some((charlie(), signed_extra(0, 0))), function: Call::Contracts( - contracts::Call::put_code::(10_000, transfer_code) + contracts::Call::put_code::(transfer_code) ), }, CheckedExtrinsic { diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index de345cd67a0c7..2bbbd9486f0f0 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -372,16 +372,15 @@ impl treasury::Trait for Runtime { } parameter_types! { - pub const ContractTransferFee: Balance = 1 * CENTS; - pub const ContractCreationFee: Balance = 1 * CENTS; - pub const ContractTransactionBaseFee: Balance = 1 * CENTS; - pub const ContractTransactionByteFee: Balance = 10 * MILLICENTS; - pub const ContractFee: Balance = 1 * CENTS; + pub const ContractTransferFee: Balance = 1000; + pub const ContractCreationFee: Balance = 1000; + pub const ContractTransactionBaseFee: Balance = 1000; + pub const ContractTransactionByteFee: Balance = 10; + pub const ContractFee: Balance = 1; pub const TombstoneDeposit: Balance = 1 * DOLLARS; pub const RentByteFee: Balance = 1 * DOLLARS; pub const RentDepositOffset: Balance = 1000 * DOLLARS; pub const SurchargeReward: Balance = 150 * DOLLARS; - pub const WeightPerGasUnit: Weight = 1; } impl contracts::Trait for Runtime { @@ -411,7 +410,6 @@ impl contracts::Trait for Runtime { type MaxDepth = contracts::DefaultMaxDepth; type MaxValueSize = contracts::DefaultMaxValueSize; type WeightToFee = LinearWeightToFee; - type WeightPerGasUnit = WeightPerGasUnit; } impl sudo::Trait for Runtime { @@ -520,7 +518,7 @@ construct_runtime!( FinalityTracker: finality_tracker::{Module, Call, Inherent}, Grandpa: grandpa::{Module, Call, Storage, Config, Event}, Treasury: treasury::{Module, Call, Storage, Config, Event}, - Contracts: contracts, + Contracts: contracts::{Module, Call, Storage, Config, Event}, Sudo: sudo, ImOnline: im_online::{Module, Call, Storage, Event, ValidateUnsigned, Config}, Offences: offences::{Module, Call, Storage, Event}, diff --git a/srml/contracts/src/lib.rs b/srml/contracts/src/lib.rs index 5ec05e2c1aef9..002ae305676d2 100644 --- a/srml/contracts/src/lib.rs +++ b/srml/contracts/src/lib.rs @@ -422,9 +422,6 @@ pub trait Trait: system::Trait { /// Convert a weight value into a deductible fee based on the currency type. type WeightToFee: Convert>; - - /// The number of weight units used by one gas unit. - type WeightPerGasUnit: Get; } /// Simple contract address determiner. @@ -1024,6 +1021,8 @@ impl CheckBlockGasLimit { who: &T::AccountId, call: &::Call, ) -> rstd::result::Result>>, TransactionValidityError> { + use rstd::convert::TryInto; + let call = match call.is_sub_type() { Some(call) => call, None => return Ok(None), @@ -1034,10 +1033,12 @@ impl CheckBlockGasLimit { Call::call(_, _, gas_limit, _) | Call::instantiate(_, gas_limit, _, _) => { // Compute how much block weight this transaction can take up in case if it // depleted devoted gas to zero. - let gas_weight_limit = gas_to_weight::(gas_limit); - - // Obtain the the available amount of weight left in the current block and discard - // this transaction if it can potentially overrun the available amount. + // We are achieving this by obtain the the available amount of weight left in + // the current block and discard this transaction if it can potentially overrun the + // available amount. + let gas_weight_limit = gas_limit + .try_into() + .map_err(|_| InvalidTransaction::ExhaustsResources)?; let weight_available = ::MaximumBlockWeight::get() .saturating_sub(>::all_extrinsics_weight()); if gas_weight_limit > weight_available { @@ -1055,7 +1056,6 @@ impl CheckBlockGasLimit { >::put(gas_price); if true { - use rstd::convert::TryInto; runtime_io::print_num(gas_weight_limit.try_into().unwrap_or(0) as u64); runtime_io::print_num(fee.try_into().unwrap_or(0) as u64); runtime_io::print_num(gas_price.try_into().unwrap_or(0) as u64); @@ -1139,8 +1139,10 @@ impl SignedExtension for CheckBlockGasLimit { }) = pre { let (gas_left, gas_spent) = GasUsageReport::take(); + // These should be OK since we don't buy more + let unused_weight = gas_left as Weight; + let spent_weight = gas_spent as Weight; - let unused_weight = gas_to_weight::(gas_left); let refund = T::WeightToFee::convert(unused_weight); // Refund the unused gas. @@ -1149,13 +1151,8 @@ impl SignedExtension for CheckBlockGasLimit { T::GasPayment::on_unbalanced(imbalance); } - let spent_weight = gas_to_weight::(gas_spent); let next_weight = >::all_extrinsics_weight().saturating_add(spent_weight); >::set_extrinsics_weight(next_weight); } } } - -fn gas_to_weight(gas: Gas) -> Weight { - (T::WeightPerGasUnit::get() as Gas * gas) as Weight -} diff --git a/srml/contracts/src/tests.rs b/srml/contracts/src/tests.rs index 6242c273122f7..9c86ea7c9e3ff 100644 --- a/srml/contracts/src/tests.rs +++ b/srml/contracts/src/tests.rs @@ -143,7 +143,6 @@ parameter_types! { pub const InstantiateBaseFee: u64 = 175; pub const MaxDepth: u32 = 100; pub const MaxValueSize: u32 = 16_384; - pub const WeightPerGasUnit: u32 = 1; } impl Trait for Test { type Currency = Balances; @@ -172,7 +171,6 @@ impl Trait for Test { type MaxDepth = MaxDepth; type MaxValueSize = MaxValueSize; type WeightToFee = (); - type WeightPerGasUnit = WeightPerGasUnit; } type Balances = balances::Module; From b9b1a0864f66c8671fe3e4dca3805d322fff9f9b Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Mon, 18 Nov 2019 14:35:52 +0100 Subject: [PATCH 09/36] Add a screeming TODO --- paint/contracts/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/paint/contracts/src/lib.rs b/paint/contracts/src/lib.rs index e4f135c3df9d4..4a3ae1a90b43c 100644 --- a/paint/contracts/src/lib.rs +++ b/paint/contracts/src/lib.rs @@ -1055,6 +1055,7 @@ impl CheckBlockGasLimit { .unwrap_or(1.into()); >::put(gas_price); + // TODO: Remove this. if true { runtime_io::print_num(gas_weight_limit.try_into().unwrap_or(0) as u64); runtime_io::print_num(fee.try_into().unwrap_or(0) as u64); From 5d63cb23c90e8b5a9386b454421620e7f10ba05b Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Mon, 18 Nov 2019 15:05:34 +0100 Subject: [PATCH 10/36] Migrate to new weight system --- paint/contracts/src/lib.rs | 3 +-- paint/system/src/lib.rs | 9 --------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/paint/contracts/src/lib.rs b/paint/contracts/src/lib.rs index 4a3ae1a90b43c..fc1ebe4c17259 100644 --- a/paint/contracts/src/lib.rs +++ b/paint/contracts/src/lib.rs @@ -1152,8 +1152,7 @@ impl SignedExtension for CheckBlockGasLimit { T::GasPayment::on_unbalanced(imbalance); } - let next_weight = >::all_extrinsics_weight().saturating_add(spent_weight); - >::set_extrinsics_weight(next_weight); + >::register_extra_weight_unchecked(spent_weight); } } } diff --git a/paint/system/src/lib.rs b/paint/system/src/lib.rs index d2b20e74de40e..5206396605103 100644 --- a/paint/system/src/lib.rs +++ b/paint/system/src/lib.rs @@ -602,15 +602,6 @@ impl Module { AllExtrinsicsWeight::get().unwrap_or_default() } - /// Sets the total weight of all executed extrinsics so far. - /// - /// Note that generally the total extrinsics weight is tracked by the this module. You might - /// find this method useful only if you are implementing a custom weight handling logic for - /// transactions. - pub fn set_extrinsics_weight(current_weight: Weight) { - AllExtrinsicsWeight::put(current_weight) - } - pub fn all_extrinsics_len() -> u32 { AllExtrinsicsLen::get().unwrap_or_default() } From a3c1abcb1dea7772acb8592bd2dd90e5a0697d5e Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Mon, 18 Nov 2019 15:05:43 +0100 Subject: [PATCH 11/36] Clean and docs --- paint/contracts/src/lib.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/paint/contracts/src/lib.rs b/paint/contracts/src/lib.rs index fc1ebe4c17259..c3dd63e1299d1 100644 --- a/paint/contracts/src/lib.rs +++ b/paint/contracts/src/lib.rs @@ -1031,17 +1031,19 @@ impl CheckBlockGasLimit { match *call { Call::claim_surcharge(_, _) | Call::update_schedule(_) | Call::put_code(_) => Ok(None), Call::call(_, _, gas_limit, _) | Call::instantiate(_, gas_limit, _, _) => { - // Compute how much block weight this transaction can take up in case if it - // depleted devoted gas to zero. - // We are achieving this by obtain the the available amount of weight left in - // the current block and discard this transaction if it can potentially overrun the - // available amount. + // Compute how much block weight this transaction can take at its limit, i.e. + // if this transaction depleted all provided gas to zero. let gas_weight_limit = gas_limit .try_into() .map_err(|_| InvalidTransaction::ExhaustsResources)?; let weight_available = ::MaximumBlockWeight::get() .saturating_sub(>::all_extrinsics_weight()); if gas_weight_limit > weight_available { + // We discard the transaction if the requested limit exceeds the available + // amount of weight in the current block. + // + // Note that this transaction is left out only from the current block and txq + // might attempt to include this transaction again. return Err(InvalidTransaction::ExhaustsResources.into()); } @@ -1055,13 +1057,6 @@ impl CheckBlockGasLimit { .unwrap_or(1.into()); >::put(gas_price); - // TODO: Remove this. - if true { - runtime_io::print_num(gas_weight_limit.try_into().unwrap_or(0) as u64); - runtime_io::print_num(fee.try_into().unwrap_or(0) as u64); - runtime_io::print_num(gas_price.try_into().unwrap_or(0) as u64); - } - let imbalance = match T::Currency::withdraw( who, fee, From 36c86adafc7c8d6ea484395dd06f32c16b8863e9 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Mon, 18 Nov 2019 23:41:23 +0100 Subject: [PATCH 12/36] Make construct_block less of a footgun --- bin/node/executor/src/lib.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/bin/node/executor/src/lib.rs b/bin/node/executor/src/lib.rs index 40862c949ab20..8779d977e504c 100644 --- a/bin/node/executor/src/lib.rs +++ b/bin/node/executor/src/lib.rs @@ -297,6 +297,10 @@ mod tests { ext } + /// Construct a fake block. + /// + /// `extrinsics` must be a list of valid extrinsics, i.e. none of the extrinsics for example + /// can report `ExhaustResources`. Otherwise, this function panics. fn construct_block( env: &mut TestExternalities, number: BlockNumber, @@ -332,13 +336,17 @@ mod tests { ).0.unwrap(); for i in extrinsics.iter() { - executor_call:: _>( + let r = executor_call:: _>( env, "BlockBuilder_apply_extrinsic", &i.encode(), true, None, - ).0.unwrap(); + ).0.expect("execution failed").into_encoded(); + match ApplyResult::decode(&mut &r[..]).expect("apply result deserialization failed") { + Ok(_) => {}, + Err(e) => panic!("panic while applying extrinsic: {:?}", e), + } } let header = match executor_call:: _>( From f32fd6534fef6c78f17a79bdab779a68353533bf Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Tue, 19 Nov 2019 11:05:02 +0100 Subject: [PATCH 13/36] WIP --- bin/node/executor/src/lib.rs | 4 ++-- bin/node/runtime/src/lib.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/bin/node/executor/src/lib.rs b/bin/node/executor/src/lib.rs index 8779d977e504c..178dcedde54ef 100644 --- a/bin/node/executor/src/lib.rs +++ b/bin/node/executor/src/lib.rs @@ -738,7 +738,7 @@ mod tests { CheckedExtrinsic { signed: Some((charlie(), signed_extra(1, 0))), function: Call::Contracts( - contracts::Call::instantiate::(1 * DOLLARS, 10_000, transfer_ch, Vec::new()) + contracts::Call::instantiate::(1 * DOLLARS, 10_000_000, transfer_ch, Vec::new()) ), }, CheckedExtrinsic { @@ -747,7 +747,7 @@ mod tests { contracts::Call::call::( indices::address::Address::Id(addr.clone()), 10, - 10_000, + 10_000_000, vec![0x00, 0x01, 0x02, 0x03] ) ), diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 2d80a0687e81d..c9233c4664326 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -369,11 +369,11 @@ impl treasury::Trait for Runtime { } parameter_types! { - pub const ContractTransferFee: Balance = 1000; - pub const ContractCreationFee: Balance = 1000; - pub const ContractTransactionBaseFee: Balance = 1000; - pub const ContractTransactionByteFee: Balance = 10; - pub const ContractFee: Balance = 1; + pub const ContractTransferFee: Balance = 1 * CENTS; + pub const ContractCreationFee: Balance = 1 * CENTS; + pub const ContractTransactionBaseFee: Balance = 1 * CENTS; + pub const ContractTransactionByteFee: Balance = 10 * MILLICENTS; + pub const ContractFee: Balance = 1 * CENTS; pub const TombstoneDeposit: Balance = 1 * DOLLARS; pub const RentByteFee: Balance = 1 * DOLLARS; pub const RentDepositOffset: Balance = 1000 * DOLLARS; From 6e925db72e0954e37484f417d5a24d574464a1d2 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Tue, 19 Nov 2019 16:59:32 +0100 Subject: [PATCH 14/36] Fix the test --- bin/node/executor/src/lib.rs | 4 ++-- bin/node/runtime/src/lib.rs | 2 +- paint/contracts/src/lib.rs | 23 +++++++++++++---------- primitives/sr-primitives/src/weights.rs | 2 +- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/bin/node/executor/src/lib.rs b/bin/node/executor/src/lib.rs index 178dcedde54ef..c3546c3d3df02 100644 --- a/bin/node/executor/src/lib.rs +++ b/bin/node/executor/src/lib.rs @@ -738,7 +738,7 @@ mod tests { CheckedExtrinsic { signed: Some((charlie(), signed_extra(1, 0))), function: Call::Contracts( - contracts::Call::instantiate::(1 * DOLLARS, 10_000_000, transfer_ch, Vec::new()) + contracts::Call::instantiate::(1 * DOLLARS, 10_000_000_000, transfer_ch, Vec::new()) ), }, CheckedExtrinsic { @@ -747,7 +747,7 @@ mod tests { contracts::Call::call::( indices::address::Address::Id(addr.clone()), 10, - 10_000_000, + 10_000_000_000, vec![0x00, 0x01, 0x02, 0x03] ) ), diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index c9233c4664326..6f211d99f02fc 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -102,7 +102,7 @@ pub type DealWithFees = SplitTwoWays< parameter_types! { pub const BlockHashCount: BlockNumber = 250; - pub const MaximumBlockWeight: Weight = 1_000_000_000; + pub const MaximumBlockWeight: Weight = 1_000_000_000_000; pub const MaximumBlockLength: u32 = 5 * 1024 * 1024; pub const Version: RuntimeVersion = VERSION; pub const AvailableBlockRatio: Perbill = Perbill::from_percent(75); diff --git a/paint/contracts/src/lib.rs b/paint/contracts/src/lib.rs index c3dd63e1299d1..2f8c81a1359fb 100644 --- a/paint/contracts/src/lib.rs +++ b/paint/contracts/src/lib.rs @@ -117,7 +117,7 @@ use sr_primitives::{ RuntimeDebug, traits::{ Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension, Convert, - CheckedDiv, + CheckedDiv, UniqueSaturatedInto, }, weights::{DispatchInfo, Weight}, transaction_validity::{ @@ -1021,8 +1021,6 @@ impl CheckBlockGasLimit { who: &T::AccountId, call: &::Call, ) -> rstd::result::Result>>, TransactionValidityError> { - use rstd::convert::TryInto; - let call = match call.is_sub_type() { Some(call) => call, None => return Ok(None), @@ -1033,11 +1031,9 @@ impl CheckBlockGasLimit { Call::call(_, _, gas_limit, _) | Call::instantiate(_, gas_limit, _, _) => { // Compute how much block weight this transaction can take at its limit, i.e. // if this transaction depleted all provided gas to zero. - let gas_weight_limit = gas_limit - .try_into() - .map_err(|_| InvalidTransaction::ExhaustsResources)?; + let gas_weight_limit = Weight::from(gas_limit); let weight_available = ::MaximumBlockWeight::get() - .saturating_sub(>::all_extrinsics_weight()); + .saturating_sub(>::all_extrinsics_weight()).into(); if gas_weight_limit > weight_available { // We discard the transaction if the requested limit exceeds the available // amount of weight in the current block. @@ -1052,9 +1048,16 @@ impl CheckBlockGasLimit { let fee = T::WeightToFee::convert(gas_weight_limit); // Compute and store the effective price per unit of gas. - let gas_price = fee - .checked_div(&>::from(gas_weight_limit)) - .unwrap_or(1.into()); + let gas_price = { + let divisor = gas_weight_limit.unique_saturated_into(); + fee + .checked_div(&divisor) + .unwrap_or(1.into()) + }; + + // gas_weight_limit is Gas/Weight which is u64 + // fee is balance. + // GasPrice has to be Balance >::put(gas_price); let imbalance = match T::Currency::withdraw( diff --git a/primitives/sr-primitives/src/weights.rs b/primitives/sr-primitives/src/weights.rs index 6f2a8676798a4..b07099bae8367 100644 --- a/primitives/sr-primitives/src/weights.rs +++ b/primitives/sr-primitives/src/weights.rs @@ -46,7 +46,7 @@ use crate::RuntimeDebug; pub use crate::transaction_validity::TransactionPriority; /// Numeric range of a transaction weight. -pub type Weight = u32; +pub type Weight = u64; /// Means of weighing some particular kind of data (`T`). pub trait WeighData { From dbd2d3e37fc5586bf93c80afc26a1ce658c0944d Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Tue, 19 Nov 2019 18:30:58 +0100 Subject: [PATCH 15/36] Fix the tests --- paint/contracts/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/paint/contracts/src/tests.rs b/paint/contracts/src/tests.rs index 542249bc6cf21..fcbe84afcbf0f 100644 --- a/paint/contracts/src/tests.rs +++ b/paint/contracts/src/tests.rs @@ -2412,7 +2412,7 @@ fn get_runtime_storage() { 0x14144020u32.to_le_bytes().to_vec().as_ref() ); - assert_ok!(Contract::put_code(Origin::signed(ALICE), 100_000, wasm)); + assert_ok!(Contract::put_code(Origin::signed(ALICE), wasm)); assert_ok!(Contract::instantiate( Origin::signed(ALICE), 100, From d9f5e8b4486007415e3d1b729f386c8588eda659 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Mon, 9 Dec 2019 14:46:52 +0100 Subject: [PATCH 16/36] Fix up merge --- frame/contracts/src/gas.rs | 2 +- frame/contracts/src/lib.rs | 21 ++++++++++++++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/frame/contracts/src/gas.rs b/frame/contracts/src/gas.rs index 5811e42886dad..324b5a44dd2d8 100644 --- a/frame/contracts/src/gas.rs +++ b/frame/contracts/src/gas.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{GasSpent, Module, Trait, BalanceOf, NegativeImbalanceOf}; +use crate::{Module, Trait, BalanceOf, NegativeImbalanceOf}; use rstd::convert::TryFrom; use sp_runtime::traits::{ CheckedMul, Zero, SaturatedConversion, SimpleArithmetic, UniqueSaturatedInto, diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index b8a1ca2806b22..247a90ccf47b9 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -113,7 +113,10 @@ use rstd::{prelude::*, marker::PhantomData, fmt::Debug}; use codec::{Codec, Encode, Decode}; use runtime_io::hashing::blake2_256; use sp_runtime::{ - traits::{Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension}, + traits::{ + Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension, Convert, + UniqueSaturatedInto, CheckedDiv, + }, transaction_validity::{ ValidTransaction, InvalidTransaction, TransactionValidity, TransactionValidityError, }, @@ -122,7 +125,11 @@ use support::dispatch::{Result, Dispatchable}; use support::{ Parameter, decl_module, decl_event, decl_storage, storage::child, parameter_types, IsSubType, - weights::DispatchInfo, + weights::{DispatchInfo, Weight}, + traits::{ + Currency, Get, OnFreeBalanceZero, Time, Randomness, OnUnbalanced, ExistenceRequirement, + WithdrawReason, Imbalance, + }, }; use system::{ensure_signed, RawOrigin, ensure_root}; use primitives::storage::well_known_keys::CHILD_STORAGE_KEY_PREFIX; @@ -142,7 +149,7 @@ pub trait ComputeDispatchFee { /// Information for managing an acocunt and its sub trie abstraction. /// This is the required info to cache for an account -#[derive(Encode, Decode, RuntimeDebug)] +#[derive(Encode, Decode, sp_runtime::RuntimeDebug)] pub enum ContractInfo { Alive(AliveContractInfo), Tombstone(TombstoneContractInfo), @@ -205,7 +212,7 @@ pub type AliveContractInfo = /// Information for managing an account and its sub trie abstraction. /// This is the required info to cache for an account. -#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug)] +#[derive(Encode, Decode, Clone, PartialEq, Eq, sp_runtime::RuntimeDebug)] pub struct RawAliveContractInfo { /// Unique ID for the subtree encoded as a bytes vector. pub trie_id: TrieId, @@ -224,7 +231,7 @@ pub struct RawAliveContractInfo { pub type TombstoneContractInfo = RawTombstoneContractInfo<::Hash, ::Hashing>; -#[derive(Encode, Decode, PartialEq, Eq, RuntimeDebug)] +#[derive(Encode, Decode, PartialEq, Eq, sp_runtime::RuntimeDebug)] pub struct RawTombstoneContractInfo(H, PhantomData); impl RawTombstoneContractInfo @@ -910,7 +917,7 @@ impl Config { /// Definition of the cost schedule and other parameterizations for wasm vm. #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] -#[derive(Clone, Encode, Decode, PartialEq, Eq, RuntimeDebug)] +#[derive(Clone, Encode, Decode, PartialEq, Eq, sp_runtime::RuntimeDebug)] pub struct Schedule { /// Version of the schedule. pub version: u32, @@ -1106,7 +1113,7 @@ impl SignedExtension for CheckBlockGasLimit { call: &Self::Call, _: DispatchInfo, _: usize, - ) -> rstd::result::Result { + ) -> rstd::result::Result { Self::perform_pre_dispatch_checks(who, call) .map_err(Into::into) } From aa377744f28baa167bcd7e044433cbb4c3f7bd0c Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Tue, 10 Dec 2019 12:14:15 +0100 Subject: [PATCH 17/36] Clean imports --- frame/contracts/src/gas.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/frame/contracts/src/gas.rs b/frame/contracts/src/gas.rs index 324b5a44dd2d8..90c8bf5b32daf 100644 --- a/frame/contracts/src/gas.rs +++ b/frame/contracts/src/gas.rs @@ -14,13 +14,9 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{Module, Trait, BalanceOf, NegativeImbalanceOf}; -use rstd::convert::TryFrom; +use crate::{Trait, BalanceOf}; use sp_runtime::traits::{ - CheckedMul, Zero, SaturatedConversion, SimpleArithmetic, UniqueSaturatedInto, -}; -use support::{ - traits::{Currency, ExistenceRequirement, Imbalance, OnUnbalanced, WithdrawReason}, StorageValue, + Zero, SaturatedConversion, SimpleArithmetic, }; #[cfg(test)] From 8e4448ca8ced8e9598b741a8c04f129abeb3d9c6 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Tue, 10 Dec 2019 12:24:16 +0100 Subject: [PATCH 18/36] A few cleanings --- frame/contracts/src/lib.rs | 27 ++++++++++++++------------- frame/support/src/weights.rs | 2 +- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 247a90ccf47b9..3f775fa37ec97 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -109,7 +109,7 @@ pub use crate::exec::{ExecResult, ExecReturnValue, ExecError, StatusCode}; #[cfg(feature = "std")] use serde::{Serialize, Deserialize}; use primitives::crypto::UncheckedFrom; -use rstd::{prelude::*, marker::PhantomData, fmt::Debug}; +use rstd::{prelude::*, marker::PhantomData, convert::TryFrom, fmt}; use codec::{Codec, Encode, Decode}; use runtime_io::hashing::blake2_256; use sp_runtime::{ @@ -121,7 +121,7 @@ use sp_runtime::{ ValidTransaction, InvalidTransaction, TransactionValidity, TransactionValidityError, }, }; -use support::dispatch::{Result, Dispatchable}; +use support::dispatch::{self, Dispatchable}; use support::{ Parameter, decl_module, decl_event, decl_storage, storage::child, parameter_types, IsSubType, @@ -236,7 +236,7 @@ pub struct RawTombstoneContractInfo(H, PhantomData); impl RawTombstoneContractInfo where - H: Member + MaybeSerializeDeserialize+ Debug + H: Member + MaybeSerializeDeserialize + fmt::Debug + AsRef<[u8]> + AsMut<[u8]> + Copy + Default + rstd::hash::Hash + Codec, Hasher: Hash, @@ -525,7 +525,7 @@ decl_module! { /// Updates the schedule for metering contracts. /// /// The schedule must have a greater version than the stored schedule. - pub fn update_schedule(origin, schedule: Schedule) -> Result { + pub fn update_schedule(origin, schedule: Schedule) -> dispatch::Result { ensure_root(origin)?; if >::current_schedule().version >= schedule.version { return Err("new schedule must have a greater version than current"); @@ -542,7 +542,7 @@ decl_module! { pub fn put_code( origin, code: Vec - ) -> Result { + ) -> dispatch::Result { let _origin = ensure_signed(origin)?; let schedule = >::current_schedule(); @@ -567,7 +567,7 @@ decl_module! { #[compact] value: BalanceOf, #[compact] gas_limit: Gas, data: Vec - ) -> Result { + ) -> dispatch::Result { let origin = ensure_signed(origin)?; let dest = T::Lookup::lookup(dest)?; @@ -592,7 +592,7 @@ decl_module! { #[compact] gas_limit: Gas, code_hash: CodeHash, data: Vec - ) -> Result { + ) -> dispatch::Result { let origin = ensure_signed(origin)?; Self::execute_wasm(origin, gas_limit, |ctx, gas_meter| { @@ -670,7 +670,7 @@ impl Module { pub fn get_storage( address: T::AccountId, key: [u8; 32], - ) -> rstd::result::Result>, GetStorageError> { + ) -> Result>, GetStorageError> { let contract_info = >::get(&address) .ok_or(GetStorageError::ContractDoesntExist)? .get_alive() @@ -761,7 +761,7 @@ impl Module { code_hash: CodeHash, rent_allowance: BalanceOf, delta: Vec - ) -> Result { + ) -> dispatch::Result { let mut origin_contract = >::get(&origin) .and_then(|c| c.get_alive()) .ok_or("Cannot restore from inexisting or tombstone contract")?; @@ -1017,7 +1017,7 @@ impl CheckBlockGasLimit { fn perform_pre_dispatch_checks( who: &T::AccountId, call: &::Call, - ) -> rstd::result::Result>>, TransactionValidityError> { + ) -> Result>>, TransactionValidityError> { let call = match call.is_sub_type() { Some(call) => call, None => return Ok(None), @@ -1028,7 +1028,8 @@ impl CheckBlockGasLimit { Call::call(_, _, gas_limit, _) | Call::instantiate(_, gas_limit, _, _) => { // Compute how much block weight this transaction can take at its limit, i.e. // if this transaction depleted all provided gas to zero. - let gas_weight_limit = Weight::from(gas_limit); + let gas_weight_limit = Weight::try_from(gas_limit) + .map_err(|_| InvalidTransaction::ExhaustsResources)?; let weight_available = ::MaximumBlockWeight::get() .saturating_sub(>::all_extrinsics_weight()).into(); if gas_weight_limit > weight_available { @@ -1083,9 +1084,9 @@ impl Default for CheckBlockGasLimit { } } -impl rstd::fmt::Debug for CheckBlockGasLimit { +impl fmt::Debug for CheckBlockGasLimit { #[cfg(feature = "std")] - fn fmt(&self, f: &mut rstd::fmt::Formatter) -> rstd::fmt::Result { + fn fmt(&self, f: &mut fmt::Formatter) -> rstd::fmt::Result { write!(f, "CheckBlockGasLimit") } diff --git a/frame/support/src/weights.rs b/frame/support/src/weights.rs index ca827229ee5f4..84198a7ecaffe 100644 --- a/frame/support/src/weights.rs +++ b/frame/support/src/weights.rs @@ -50,7 +50,7 @@ use sp_runtime::{ pub use sp_runtime::transaction_validity::TransactionPriority; /// Numeric range of a transaction weight. -pub type Weight = u64; +pub type Weight = u32; /// Means of weighing some particular kind of data (`T`). pub trait WeighData { From c24de55d6e248c1ce120d153625a684de933f14f Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Tue, 10 Dec 2019 14:03:03 +0100 Subject: [PATCH 19/36] Fix node-executor tests. --- bin/node/executor/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/executor/src/lib.rs b/bin/node/executor/src/lib.rs index 80362bfe40eda..36d6cf312cdbf 100644 --- a/bin/node/executor/src/lib.rs +++ b/bin/node/executor/src/lib.rs @@ -346,7 +346,7 @@ mod tests { true, None, ).0.expect("execution failed").into_encoded(); - match ApplyResult::decode(&mut &r[..]).expect("apply result deserialization failed") { + match ApplyExtrinsicResult::decode(&mut &r[..]).expect("apply result deserialization failed") { Ok(_) => {}, Err(e) => panic!("panic while applying extrinsic: {:?}", e), } From f9db9c005cfc696747adae06b16709d4abe8e915 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Wed, 8 Jan 2020 17:07:22 +0100 Subject: [PATCH 20/36] Remove unnecessary `.map_err`. --- frame/contracts/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index c86630acb1581..aab4be78a3ba6 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -1157,7 +1157,6 @@ impl SignedExtension for CheckBlockGasLimit { _: usize, ) -> Result { Self::perform_pre_dispatch_checks(who, call) - .map_err(Into::into) } fn validate( From 2110696a5732e9ee511b0b5fe6c59c1ff9a77cb9 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Thu, 9 Jan 2020 18:43:28 +0100 Subject: [PATCH 21/36] Fix the tests. --- bin/node/executor/src/lib.rs | 7 ++++--- bin/node/runtime/src/lib.rs | 18 ++++++++++++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/bin/node/executor/src/lib.rs b/bin/node/executor/src/lib.rs index 8a88bde408421..a738e4960e1d9 100644 --- a/bin/node/executor/src/lib.rs +++ b/bin/node/executor/src/lib.rs @@ -47,7 +47,6 @@ mod tests { Fixed64, traits::{Header as HeaderT, Hash as HashT, Convert}, ApplyExtrinsicResult, transaction_validity::InvalidTransaction, }; - use pallet_contracts::ContractAddressFor; use sc_executor::{NativeExecutor, WasmExecutionMethod}; use frame_system::{EventRecord, Phase}; use node_runtime::{ @@ -735,6 +734,8 @@ mod tests { #[test] fn deploying_wasm_contract_should_work() { + use pallet_contracts::ContractAddressFor; + let transfer_code = wabt::wat2wasm(CODE_TRANSFER).unwrap(); let transfer_ch = ::Hashing::hash(&transfer_code); @@ -756,7 +757,7 @@ mod tests { CheckedExtrinsic { signed: Some((charlie(), signed_extra(0, 0))), function: Call::Contracts( - pallet_contracts::Call::put_code::(10_000, transfer_code) + pallet_contracts::Call::put_code::(transfer_code) ), }, CheckedExtrinsic { @@ -771,7 +772,7 @@ mod tests { pallet_contracts::Call::call::( pallet_indices::address::Address::Id(addr.clone()), 10, - 10_000_000_000, + 100_000_000, vec![0x00, 0x01, 0x02, 0x03] ) ), diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 9d6bbee847c34..198e3054887d2 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -391,11 +391,21 @@ impl pallet_treasury::Trait for Runtime { } parameter_types! { - pub const ContractTransferFee: Balance = 1 * CENTS; - pub const ContractCreationFee: Balance = 1 * CENTS; + // NOTE Those are significantly lower than than corresponding fees for transferring funds. + // Since if we charge the fee in gas there is basically not enough gas even to make one simple + // transfer. + // + // This basically means that in order to transfer funds it would make more sense to use the + // contracts module rather than balances module. + // + // This should be fixed in nearest future by using the synchronous transfer directly. It will + // cut fees from the senders account balance directly. + pub const ContractTransferFee: Balance = 1000; + pub const ContractCreationFee: Balance = 1000; + pub const ContractFee: Balance = 1000; + pub const ContractTransactionBaseFee: Balance = 1 * CENTS; pub const ContractTransactionByteFee: Balance = 10 * MILLICENTS; - pub const ContractFee: Balance = 1 * CENTS; pub const TombstoneDeposit: Balance = 1 * DOLLARS; pub const RentByteFee: Balance = 1 * DOLLARS; pub const RentDepositOffset: Balance = 1000 * DOLLARS; @@ -548,7 +558,7 @@ construct_runtime!( FinalityTracker: pallet_finality_tracker::{Module, Call, Inherent}, Grandpa: pallet_grandpa::{Module, Call, Storage, Config, Event}, Treasury: pallet_treasury::{Module, Call, Storage, Config, Event}, - Contracts: pallet_contracts, + Contracts: pallet_contracts::{Module, Call, Storage, Config, Event}, Sudo: pallet_sudo, ImOnline: pallet_im_online::{Module, Call, Storage, Event, ValidateUnsigned, Config}, AuthorityDiscovery: pallet_authority_discovery::{Module, Call, Config}, From 5ef036f1d7ae1402fac5b6aebe90fc05ed126240 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Thu, 9 Jan 2020 18:43:40 +0100 Subject: [PATCH 22/36] Clean up. --- frame/contracts/src/lib.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index aab4be78a3ba6..edbea8e499893 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -722,9 +722,15 @@ impl Module { gas_limit: Gas, func: impl FnOnce(&mut ExecutionContext, &mut GasMeter) -> ExecResult ) -> ExecResult { + // // Take the gas price prepared by the signed extension. + // let gas_price = GasPrice::::take(); - debug_assert!(gas_price != 0.into()); + debug_assert!( + gas_price != 0.into(), + "gas price should have been set by the signed extension", + ); + let mut gas_meter = GasMeter::::with_limit(gas_limit, gas_price); let cfg = Config::preload(); @@ -740,9 +746,6 @@ impl Module { } // Save the gas usage report. - // - // NOTE: This should go after the commit to the storage, since the storage changes - // can alter the balance of the caller. let gas_spent = gas_meter.spent(); let gas_left = gas_meter.gas_left(); GasUsageReport::put((gas_left, gas_spent)); @@ -915,7 +918,7 @@ decl_storage! { pub ContractInfoOf: map T::AccountId => Option>; /// The price of one unit of gas. /// - /// This value is transint and remove before block finalization. + /// This value is set in the signed extension and is removed after the execution. GasPrice: BalanceOf = 1.into(); } } @@ -1094,9 +1097,9 @@ impl CheckBlockGasLimit { .unwrap_or(1.into()) }; - // gas_weight_limit is Gas/Weight which is u64 - // fee is balance. - // GasPrice has to be Balance + // + // The place where we set GasPrice for the execution of this transaction. + // >::put(gas_price); let imbalance = match T::Currency::withdraw( From 49d1ea5138745f2d7af718dd08e0e65ed702d35b Mon Sep 17 00:00:00 2001 From: Sergei Pepyakin Date: Wed, 22 Jan 2020 16:30:27 +0100 Subject: [PATCH 23/36] Apply suggestions from code review Co-Authored-By: thiolliere --- bin/node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 198e3054887d2..72a73b5f90a28 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -391,7 +391,7 @@ impl pallet_treasury::Trait for Runtime { } parameter_types! { - // NOTE Those are significantly lower than than corresponding fees for transferring funds. + // NOTE Those are significantly lower than the corresponding fees for transferring funds. // Since if we charge the fee in gas there is basically not enough gas even to make one simple // transfer. // From 9b18e0e4f2944735dcf9df4b98dd6dd7fae5e37a Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Wed, 22 Jan 2020 16:26:22 +0100 Subject: [PATCH 24/36] Update the expect message and add a comment. --- bin/node/executor/src/lib.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bin/node/executor/src/lib.rs b/bin/node/executor/src/lib.rs index a738e4960e1d9..bcda9da36aa82 100644 --- a/bin/node/executor/src/lib.rs +++ b/bin/node/executor/src/lib.rs @@ -348,14 +348,16 @@ mod tests { None, ).0.unwrap(); - for i in extrinsics.iter() { + for extrinsic in extrinsics.iter() { + // Try to apply the `extrinsic`. It should be valid, in the sense that it passes + // all pre-inclusion checks. let r = executor_call:: _>( env, "BlockBuilder_apply_extrinsic", - &i.encode(), + &extrinsic.encode(), true, None, - ).0.expect("execution failed").into_encoded(); + ).0.expect("application of an extrinsic failed").into_encoded(); match ApplyExtrinsicResult::decode(&mut &r[..]).expect("apply result deserialization failed") { Ok(_) => {}, Err(e) => panic!("panic while applying extrinsic: {:?}", e), From 33a3ea60affc52836a0b5f9cda7f1dc627f1921c Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Wed, 22 Jan 2020 16:31:31 +0100 Subject: [PATCH 25/36] Add associated issue and TODO for tx loophole --- 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 72a73b5f90a28..468c1dd6a40e9 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -400,6 +400,8 @@ parameter_types! { // // This should be fixed in nearest future by using the synchronous transfer directly. It will // cut fees from the senders account balance directly. + // + // TODO: https://github.com/paritytech/substrate/issues/4713 pub const ContractTransferFee: Balance = 1000; pub const ContractCreationFee: Balance = 1000; pub const ContractFee: Balance = 1000; From 74dbd4ae863c35391412d3760825c9b22c70314e Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Wed, 22 Jan 2020 16:41:48 +0100 Subject: [PATCH 26/36] Add some clarifying comments regarding GasUsageReport. --- frame/contracts/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index edbea8e499893..794a951f43df2 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -745,7 +745,7 @@ impl Module { DirectAccountDb.commit(ctx.overlay.into_change_set()); } - // Save the gas usage report. + // Save the gas usage report. It will be inspected later on at the post dispatch stage. let gas_spent = gas_meter.spent(); let gas_left = gas_meter.gas_left(); GasUsageReport::put((gas_left, gas_spent)); @@ -1178,6 +1178,8 @@ impl SignedExtension for CheckBlockGasLimit { transactor, imbalance, }) = pre { + // Take the report of consumed and left gas after the execution of the current + // transaction. let (gas_left, gas_spent) = GasUsageReport::take(); // These should be OK since we don't buy more From a39947237e80f6e6c18e1033ceae8a39299a5154 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Wed, 22 Jan 2020 16:54:26 +0100 Subject: [PATCH 27/36] Added System::remaining_weight convenience function --- frame/contracts/src/lib.rs | 3 +-- frame/system/src/lib.rs | 8 ++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 794a951f43df2..d211d3f6b7bd0 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -1074,8 +1074,7 @@ impl CheckBlockGasLimit { // if this transaction depleted all provided gas to zero. let gas_weight_limit = Weight::try_from(gas_limit) .map_err(|_| InvalidTransaction::ExhaustsResources)?; - let weight_available = ::MaximumBlockWeight::get() - .saturating_sub(>::all_extrinsics_weight()).into(); + let weight_available = >::remaining_weight().into(); if gas_weight_limit > weight_available { // We discard the transaction if the requested limit exceeds the available // amount of weight in the current block. diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 1c401b0918cc5..43c5769653c44 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -604,6 +604,14 @@ impl Module { AllExtrinsicsLen::get().unwrap_or_default() } + /// Returns how much unreserved weight is left for the current block. + /// + /// This is the opposite of `all_extrinsics_weight` provided for convenience. + pub fn remaining_weight() -> Weight { + T::MaximumBlockWeight::get() + .saturating_sub(Self::all_extrinsics_weight()) + } + /// Inform the system module of some additional weight that should be accounted for, in the /// current block. /// From 9fd2b90fc486509ccca70bac1eacb8c7c785a90d Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Fri, 31 Jan 2020 16:28:14 +0100 Subject: [PATCH 28/36] Leverage FunctionOf for setting put_code price. --- bin/node/runtime/src/lib.rs | 6 ++++++ frame/contracts/src/lib.rs | 19 ++++++++++++++++++- frame/contracts/src/tests.rs | 4 ++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index bce802d0e9274..1b1d793afff11 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -423,6 +423,10 @@ parameter_types! { pub const RentByteFee: Balance = 1 * DOLLARS; pub const RentDepositOffset: Balance = 1000 * DOLLARS; pub const SurchargeReward: Balance = 150 * DOLLARS; + + /// These values mean that it is possible to store code up to approximatelly 250Kb. + pub const PutCodeBaseWeight: Weight = 10_000; + pub const PutCodePerByteWeight: Weight = 4; } impl pallet_contracts::Trait for Runtime { @@ -452,6 +456,8 @@ impl pallet_contracts::Trait for Runtime { type MaxDepth = pallet_contracts::DefaultMaxDepth; type MaxValueSize = pallet_contracts::DefaultMaxValueSize; type WeightToFee = LinearWeightToFee; + type PutCodeBaseWeight = PutCodeBaseWeight; + type PutCodePerByteWeight = PutCodePerByteWeight; } impl pallet_sudo::Trait for Runtime { diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 47d7bfebe664e..cdc829ea1be36 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -125,7 +125,7 @@ use frame_support::dispatch::{DispatchResult, Dispatchable}; use frame_support::{ Parameter, decl_module, decl_event, decl_error, decl_storage, storage::child, parameter_types, IsSubType, - weights::{DispatchInfo, Weight}, + weights::{DispatchInfo, Weight, FunctionOf, DispatchClass}, traits::{ Currency, Get, OnFreeBalanceZero, Time, Randomness, OnUnbalanced, ExistenceRequirement, WithdrawReason, Imbalance, @@ -430,6 +430,12 @@ pub trait Trait: frame_system::Trait { /// The maximum size of a storage value in bytes. type MaxValueSize: Get; + /// Weight consume by the `put_code` excluding per byte costs. + type PutCodeBaseWeight: Get; + + /// Weight consumed per one byte of code deployed in `put_code`. + type PutCodePerByteWeight: Get; + /// Convert a weight value into a deductible fee based on the currency type. type WeightToFee: Convert>; } @@ -572,6 +578,17 @@ decl_module! { /// Stores the given binary Wasm code into the chain's storage and returns its `codehash`. /// You can instantiate contracts only with stored code. + #[weight = + FunctionOf( + |args: (&Vec,)| { + let code_bytes = args.0.len() as u32; + code_bytes.saturating_mul(T::PutCodePerByteWeight::get()) + .saturating_add(T::PutCodeBaseWeight::get()) + }, + DispatchClass::Normal, + true, + ) + ] pub fn put_code( origin, code: Vec diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 5859810dbbcd9..827389eb8d15e 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -148,6 +148,8 @@ parameter_types! { pub const InstantiateBaseFee: u64 = 175; pub const MaxDepth: u32 = 100; pub const MaxValueSize: u32 = 16_384; + pub const PutCodeBaseWeight: Weight = 10_000; + pub const PutCodePerByteWeight: Weight = 4; } impl Trait for Test { type Currency = Balances; @@ -175,6 +177,8 @@ impl Trait for Test { type InstantiateBaseFee = InstantiateBaseFee; type MaxDepth = MaxDepth; type MaxValueSize = MaxValueSize; + type PutCodeBaseWeight = PutCodeBaseWeight; + type PutCodePerByteWeight = PutCodePerByteWeight; type WeightToFee = (); } From b39ca97d2400f496fd1fe45676f65271edeb41de Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Fri, 31 Jan 2020 16:33:35 +0100 Subject: [PATCH 29/36] Remove a stale TODO --- 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 827389eb8d15e..b96d65ae4c455 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -2410,7 +2410,6 @@ fn cannot_self_destruct_in_constructor() { }); } -// TODO: This test relies on put_code, which no longer requires any gas. #[test] fn check_block_gas_limit_works() { ExtBuilder::default().build().execute_with(|| { From a5c331db1c62d2e3853f811f77c3777b92ce0d92 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Fri, 31 Jan 2020 16:39:52 +0100 Subject: [PATCH 30/36] Update docs. --- frame/contracts/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index cdc829ea1be36..a9ff1ee01f603 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -430,13 +430,15 @@ pub trait Trait: frame_system::Trait { /// The maximum size of a storage value in bytes. type MaxValueSize: Get; - /// Weight consume by the `put_code` excluding per byte costs. + /// Weight consumed by the `put_code` excluding per byte costs. type PutCodeBaseWeight: Get; /// Weight consumed per one byte of code deployed in `put_code`. type PutCodePerByteWeight: Get; /// Convert a weight value into a deductible fee based on the currency type. + /// + /// For now, only linear weight to fee is suppored. type WeightToFee: Convert>; } From 81930e8da29342e1cbaa7005ab16ca5b871fae05 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Fri, 31 Jan 2020 17:04:58 +0100 Subject: [PATCH 31/36] Rename CheckGasBlockLimit to GasWeightConversion. Besides that, extract it to a separate module. --- bin/node/runtime/src/lib.rs | 2 +- frame/contracts/src/gas_weight_conv.rs | 203 +++++++++++++++++++++++++ frame/contracts/src/lib.rs | 160 +------------------ frame/contracts/src/tests.rs | 7 +- 4 files changed, 211 insertions(+), 161 deletions(-) create mode 100644 frame/contracts/src/gas_weight_conv.rs diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 1b1d793afff11..09afe8a017311 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -662,7 +662,7 @@ pub type SignedExtra = ( frame_system::CheckNonce, frame_system::CheckWeight, pallet_transaction_payment::ChargeTransactionPayment, - pallet_contracts::CheckBlockGasLimit, + pallet_contracts::GasWeightConversion, ); /// Unchecked extrinsic type as expected by this runtime. pub type UncheckedExtrinsic = generic::UncheckedExtrinsic; diff --git a/frame/contracts/src/gas_weight_conv.rs b/frame/contracts/src/gas_weight_conv.rs new file mode 100644 index 0000000000000..4af02fb2fe3eb --- /dev/null +++ b/frame/contracts/src/gas_weight_conv.rs @@ -0,0 +1,203 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +use crate::{Call, Trait, NegativeImbalanceOf, GasPrice, GasUsageReport}; +use sp_std::{prelude::*, marker::PhantomData, convert::TryFrom, fmt}; +use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root}; +use frame_support::dispatch::{DispatchResult, Dispatchable}; +use frame_support::{ + Parameter, decl_module, decl_event, decl_error, decl_storage, storage::{StorageValue, child}, + parameter_types, IsSubType, + weights::{DispatchInfo, Weight, FunctionOf, DispatchClass}, + traits::{ + Currency, Get, OnFreeBalanceZero, Time, Randomness, OnUnbalanced, ExistenceRequirement, + WithdrawReason, Imbalance, + }, +}; +use sp_runtime::{ + traits::{ + Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension, Convert, + UniqueSaturatedInto, CheckedDiv, + }, + transaction_validity::{ + ValidTransaction, InvalidTransaction, TransactionValidity, TransactionValidityError, + }, +}; + +#[doc(hidden)] +#[cfg_attr(test, derive(Debug))] +pub struct DynamicWeightData { + /// The account id who should receive the refund from the gas leftovers. + transactor: AccountId, + /// The negative imbalance obtained by withdrawing the value up to the requested gas limit. + imbalance: NegativeImbalance, +} + +/// A `SignedExtension` required to properly handle gas limits requested by contract executing +/// transactions. +#[derive(codec::Encode, codec::Decode, Clone, Eq, PartialEq)] +pub struct GasWeightConversion(PhantomData); + +impl GasWeightConversion { + pub fn new() -> Self { + Self(PhantomData) + } + + /// Perform pre-dispatch checks for the given call and origin. + fn perform_pre_dispatch_checks( + who: &T::AccountId, + call: &::Call, + ) -> Result< + Option>>, + TransactionValidityError, + > { + // This signed extension only deals with this module's `Call`. + let call = match call.is_sub_type() { + Some(call) => call, + None => return Ok(None), + }; + + match *call { + Call::claim_surcharge(_, _) | Call::update_schedule(_) | Call::put_code(_) => Ok(None), + Call::call(_, _, gas_limit, _) | Call::instantiate(_, gas_limit, _, _) => { + // Compute how much block weight this transaction can take at its limit, i.e. + // if this transaction depleted all provided gas to zero. + let gas_weight_limit = Weight::try_from(gas_limit) + .map_err(|_| InvalidTransaction::ExhaustsResources)?; + let weight_available = >::remaining_weight().into(); + if gas_weight_limit > weight_available { + // We discard the transaction if the requested limit exceeds the available + // amount of weight in the current block. + // + // Note that this transaction is left out only from the current block and txq + // might attempt to include this transaction again. + return Err(InvalidTransaction::ExhaustsResources.into()); + } + + // Compute the fee corresponding for the given gas_weight_limit and attempt + // withdrawing from the origin of this transaction. + let fee = T::WeightToFee::convert(gas_weight_limit); + + // Compute and store the effective price per unit of gas. + let gas_price = { + let divisor = gas_weight_limit.unique_saturated_into(); + fee.checked_div(&divisor).unwrap_or(1.into()) + }; + + // + // The place where we set GasPrice for the execution of this transaction. + // + >::put(gas_price); + + let imbalance = match T::Currency::withdraw( + who, + fee, + WithdrawReason::TransactionPayment.into(), + ExistenceRequirement::KeepAlive, + ) { + Ok(imbalance) => imbalance, + Err(_) => return Err(InvalidTransaction::Payment.into()), + }; + + Ok(Some(DynamicWeightData { + transactor: who.clone(), + imbalance, + })) + } + Call::__PhantomItem(_, _) => unreachable!("Variant is never constructed"), + } + } +} + +impl Default for GasWeightConversion { + fn default() -> Self { + Self(PhantomData) + } +} + +impl fmt::Debug for GasWeightConversion { + #[cfg(feature = "std")] + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "GasWeightConversion") + } + + #[cfg(not(feature = "std"))] + fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + Ok(()) + } +} + +impl SignedExtension for GasWeightConversion { + const IDENTIFIER: &'static str = "GasWeightConversion"; + type AccountId = T::AccountId; + type Call = ::Call; + type AdditionalSigned = (); + type DispatchInfo = DispatchInfo; + /// Optional pre-dispatch check data. + /// + /// It is present only for the contract calls that operate with gas. + type Pre = Option>>; + + fn additional_signed(&self) -> Result<(), TransactionValidityError> { + Ok(()) + } + + fn pre_dispatch( + self, + who: &Self::AccountId, + call: &Self::Call, + _: DispatchInfo, + _: usize, + ) -> Result { + Self::perform_pre_dispatch_checks(who, call) + } + + fn validate( + &self, + who: &Self::AccountId, + call: &Self::Call, + _: Self::DispatchInfo, + _: usize, + ) -> TransactionValidity { + Self::perform_pre_dispatch_checks(who, call).map(|_| ValidTransaction::default()) + } + + fn post_dispatch(pre: Self::Pre, _info: DispatchInfo, _len: usize) { + if let Some(DynamicWeightData { + transactor, + imbalance, + }) = pre + { + // Take the report of consumed and left gas after the execution of the current + // transaction. + let (gas_left, gas_spent) = GasUsageReport::take(); + + // These should be OK since we don't buy more + let unused_weight = gas_left as Weight; + let spent_weight = gas_spent as Weight; + + let refund = T::WeightToFee::convert(unused_weight); + + // Refund the unused gas. + let refund_imbalance = T::Currency::deposit_creating(&transactor, refund); + if let Ok(imbalance) = imbalance.offset(refund_imbalance) { + T::GasPayment::on_unbalanced(imbalance); + } + + >::register_extra_weight_unchecked(spent_weight); + } + } +} diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a9ff1ee01f603..b30010b01a18c 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -68,7 +68,7 @@ //! //! The contracts module defines the following extension: //! -//! - [`CheckBlockGasLimit`]: Ensures that the transaction does not exceeds the block gas limit. +//! - [`GasWeightConversion`]: Ensures that the transaction does not exceeds the block gas limit. //! //! The signed extension needs to be added as signed extra to the transaction type to be used in the //! runtime. @@ -95,6 +95,7 @@ mod account_db; mod exec; mod wasm; mod rent; +mod gas_weight_conv; #[cfg(test)] mod tests; @@ -105,6 +106,7 @@ use crate::wasm::{WasmLoader, WasmVm}; pub use crate::gas::{Gas, GasMeter}; pub use crate::exec::{ExecResult, ExecReturnValue, ExecError, StatusCode}; +pub use gas_weight_conv::GasWeightConversion; #[cfg(feature = "std")] use serde::{Serialize, Deserialize}; @@ -1087,159 +1089,3 @@ impl Default for Schedule { } } } - -#[doc(hidden)] -#[cfg_attr(test, derive(Debug))] -pub struct DynamicWeightData { - /// The account id who should receive the refund from the gas leftovers. - transactor: AccountId, - /// The negative imbalance obtained by withdrawing the value up to the requested gas limit. - imbalance: NegativeImbalance, -} - -/// `SignedExtension` that checks if a transaction would exhausts the block gas limit. -#[derive(Encode, Decode, Clone, Eq, PartialEq)] -pub struct CheckBlockGasLimit(PhantomData); - -impl CheckBlockGasLimit { - /// Perform pre-dispatch checks for the given call and origin. - fn perform_pre_dispatch_checks( - who: &T::AccountId, - call: &::Call, - ) -> Result>>, TransactionValidityError> { - let call = match call.is_sub_type() { - Some(call) => call, - None => return Ok(None), - }; - - match *call { - Call::claim_surcharge(_, _) | Call::update_schedule(_) | Call::put_code(_) => Ok(None), - Call::call(_, _, gas_limit, _) | Call::instantiate(_, gas_limit, _, _) => { - // Compute how much block weight this transaction can take at its limit, i.e. - // if this transaction depleted all provided gas to zero. - let gas_weight_limit = Weight::try_from(gas_limit) - .map_err(|_| InvalidTransaction::ExhaustsResources)?; - let weight_available = >::remaining_weight().into(); - if gas_weight_limit > weight_available { - // We discard the transaction if the requested limit exceeds the available - // amount of weight in the current block. - // - // Note that this transaction is left out only from the current block and txq - // might attempt to include this transaction again. - return Err(InvalidTransaction::ExhaustsResources.into()); - } - - // Compute the fee corresponding for the given gas_weight_limit and attempt - // withdrawing from the origin of this transaction. - let fee = T::WeightToFee::convert(gas_weight_limit); - - // Compute and store the effective price per unit of gas. - let gas_price = { - let divisor = gas_weight_limit.unique_saturated_into(); - fee - .checked_div(&divisor) - .unwrap_or(1.into()) - }; - - // - // The place where we set GasPrice for the execution of this transaction. - // - >::put(gas_price); - - let imbalance = match T::Currency::withdraw( - who, - fee, - WithdrawReason::TransactionPayment.into(), - ExistenceRequirement::KeepAlive, - ) { - Ok(imbalance) => imbalance, - Err(_) => return Err(InvalidTransaction::Payment.into()), - }; - - Ok(Some(DynamicWeightData { - transactor: who.clone(), - imbalance, - })) - }, - Call::__PhantomItem(_, _) => unreachable!("Variant is never constructed"), - } - } -} - -impl Default for CheckBlockGasLimit { - fn default() -> Self { - Self(PhantomData) - } -} - -impl fmt::Debug for CheckBlockGasLimit { - #[cfg(feature = "std")] - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "CheckBlockGasLimit") - } - - #[cfg(not(feature = "std"))] - fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - Ok(()) - } -} - -impl SignedExtension for CheckBlockGasLimit { - const IDENTIFIER: &'static str = "CheckBlockGasLimit"; - type AccountId = T::AccountId; - type Call = ::Call; - type AdditionalSigned = (); - type DispatchInfo = DispatchInfo; - /// Optional pre-dispatch check data. - /// - /// It is present only for the contract calls that operate with gas. - type Pre = Option>>; - - fn additional_signed(&self) -> Result<(), TransactionValidityError> { Ok(()) } - - fn pre_dispatch( - self, - who: &Self::AccountId, - call: &Self::Call, - _: DispatchInfo, - _: usize, - ) -> Result { - Self::perform_pre_dispatch_checks(who, call) - } - - fn validate( - &self, - who: &Self::AccountId, - call: &Self::Call, - _: Self::DispatchInfo, - _: usize, - ) -> TransactionValidity { - Self::perform_pre_dispatch_checks(who, call) - .map(|_| ValidTransaction::default()) - } - - fn post_dispatch(pre: Self::Pre, _info: DispatchInfo, _len: usize) { - if let Some(DynamicWeightData { - transactor, - imbalance, - }) = pre { - // Take the report of consumed and left gas after the execution of the current - // transaction. - let (gas_left, gas_spent) = GasUsageReport::take(); - - // These should be OK since we don't buy more - let unused_weight = gas_left as Weight; - let spent_weight = gas_spent as Weight; - - let refund = T::WeightToFee::convert(unused_weight); - - // Refund the unused gas. - let refund_imbalance = T::Currency::deposit_creating(&transactor, refund); - if let Ok(imbalance) = imbalance.offset(refund_imbalance) { - T::GasPayment::on_unbalanced(imbalance); - } - - >::register_extra_weight_unchecked(spent_weight); - } - } -} diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index b96d65ae4c455..cfc2ad3238981 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -22,8 +22,9 @@ use crate::{ BalanceOf, ComputeDispatchFee, ContractAddressFor, ContractInfo, ContractInfoOf, GenesisConfig, Module, RawAliveContractInfo, RawEvent, Trait, TrieId, TrieIdFromParentCounter, Schedule, - TrieIdGenerator, CheckBlockGasLimit, account_db::{AccountDb, DirectAccountDb, OverlayAccountDb}, + TrieIdGenerator, account_db::{AccountDb, DirectAccountDb, OverlayAccountDb}, }; +use crate::gas_weight_conv::GasWeightConversion; use assert_matches::assert_matches; use hex_literal::*; use codec::{Decode, Encode, KeyedVec}; @@ -2414,14 +2415,14 @@ fn cannot_self_destruct_in_constructor() { fn check_block_gas_limit_works() { ExtBuilder::default().build().execute_with(|| { let info = DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: true }; - let check = CheckBlockGasLimit::(Default::default()); + let check = GasWeightConversion::::new(); let call: Call = crate::Call::put_code(vec![]).into(); match check.pre_dispatch(&0, &call, info, 0) { Ok(None) => {}, _ => panic!("put_code is not dynamic and should pass validation"), } - let check = CheckBlockGasLimit::(Default::default()); + let check = GasWeightConversion::::new(); let call: Call = crate::Call::call(Default::default(), 0, 100, vec![]).into(); match check.pre_dispatch(&0, &call, info, 0) { Ok(Some(_)) => {}, From cc233d34c4dcbe11dded2f7c0e8d20d6070fbf4c Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Fri, 31 Jan 2020 17:13:33 +0100 Subject: [PATCH 32/36] Tidy up. --- frame/contracts/src/gas_weight_conv.rs | 28 +++++++----------- frame/contracts/src/lib.rs | 40 ++++++++++---------------- 2 files changed, 25 insertions(+), 43 deletions(-) diff --git a/frame/contracts/src/gas_weight_conv.rs b/frame/contracts/src/gas_weight_conv.rs index 4af02fb2fe3eb..e9afc09e5fc3e 100644 --- a/frame/contracts/src/gas_weight_conv.rs +++ b/frame/contracts/src/gas_weight_conv.rs @@ -14,28 +14,20 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{Call, Trait, NegativeImbalanceOf, GasPrice, GasUsageReport}; -use sp_std::{prelude::*, marker::PhantomData, convert::TryFrom, fmt}; -use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root}; -use frame_support::dispatch::{DispatchResult, Dispatchable}; +use crate::{Call, GasPrice, GasUsageReport, NegativeImbalanceOf, Trait}; use frame_support::{ - Parameter, decl_module, decl_event, decl_error, decl_storage, storage::{StorageValue, child}, - parameter_types, IsSubType, - weights::{DispatchInfo, Weight, FunctionOf, DispatchClass}, - traits::{ - Currency, Get, OnFreeBalanceZero, Time, Randomness, OnUnbalanced, ExistenceRequirement, - WithdrawReason, Imbalance, - }, + storage::StorageValue, + traits::{Currency, ExistenceRequirement, Imbalance, OnUnbalanced, WithdrawReason}, + weights::{DispatchInfo, Weight}, + IsSubType, }; use sp_runtime::{ - traits::{ - Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension, Convert, - UniqueSaturatedInto, CheckedDiv, - }, + traits::{CheckedDiv, Convert, SignedExtension, UniqueSaturatedInto}, transaction_validity::{ - ValidTransaction, InvalidTransaction, TransactionValidity, TransactionValidityError, + InvalidTransaction, TransactionValidity, TransactionValidityError, ValidTransaction, }, }; +use sp_std::{convert::TryFrom, fmt, marker::PhantomData, prelude::*}; #[doc(hidden)] #[cfg_attr(test, derive(Debug))] @@ -77,7 +69,7 @@ impl GasWeightConversion { // if this transaction depleted all provided gas to zero. let gas_weight_limit = Weight::try_from(gas_limit) .map_err(|_| InvalidTransaction::ExhaustsResources)?; - let weight_available = >::remaining_weight().into(); + let weight_available = >::remaining_weight().into(); if gas_weight_limit > weight_available { // We discard the transaction if the requested limit exceeds the available // amount of weight in the current block. @@ -197,7 +189,7 @@ impl SignedExtension for GasWeightConversion { T::GasPayment::on_unbalanced(imbalance); } - >::register_extra_weight_unchecked(spent_weight); + >::register_extra_weight_unchecked(spent_weight); } } } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index b30010b01a18c..e2d46f24ae536 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -100,41 +100,31 @@ mod gas_weight_conv; #[cfg(test)] mod tests; -use crate::exec::ExecutionContext; use crate::account_db::{AccountDb, DirectAccountDb}; +use crate::exec::ExecutionContext; use crate::wasm::{WasmLoader, WasmVm}; +pub use crate::exec::{ExecError, ExecResult, ExecReturnValue, StatusCode}; pub use crate::gas::{Gas, GasMeter}; -pub use crate::exec::{ExecResult, ExecReturnValue, ExecError, StatusCode}; pub use gas_weight_conv::GasWeightConversion; -#[cfg(feature = "std")] -use serde::{Serialize, Deserialize}; -use sp_core::crypto::UncheckedFrom; -use sp_std::{prelude::*, marker::PhantomData, convert::TryFrom, fmt}; -use codec::{Codec, Encode, Decode}; -use sp_io::hashing::blake2_256; -use sp_runtime::{ - traits::{ - Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, SignedExtension, Convert, - UniqueSaturatedInto, CheckedDiv, - }, - transaction_validity::{ - ValidTransaction, InvalidTransaction, TransactionValidity, TransactionValidityError, - }, -}; +use codec::{Codec, Decode, Encode}; use frame_support::dispatch::{DispatchResult, Dispatchable}; use frame_support::{ - Parameter, decl_module, decl_event, decl_error, decl_storage, storage::child, - parameter_types, IsSubType, - weights::{DispatchInfo, Weight, FunctionOf, DispatchClass}, - traits::{ - Currency, Get, OnFreeBalanceZero, Time, Randomness, OnUnbalanced, ExistenceRequirement, - WithdrawReason, Imbalance, - }, + decl_error, decl_event, decl_module, decl_storage, parameter_types, + storage::child, + traits::{Currency, Get, OnFreeBalanceZero, OnUnbalanced, Randomness, Time}, + weights::{DispatchClass, FunctionOf, Weight}, + IsSubType, Parameter, }; -use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root}; +use frame_system::{self as system, ensure_root, ensure_signed, RawOrigin}; +#[cfg(feature = "std")] +use serde::{Deserialize, Serialize}; +use sp_core::crypto::UncheckedFrom; use sp_core::storage::well_known_keys::CHILD_STORAGE_KEY_PREFIX; +use sp_io::hashing::blake2_256; +use sp_runtime::traits::{Convert, Hash, MaybeSerializeDeserialize, Member, StaticLookup, Zero}; +use sp_std::{fmt, marker::PhantomData, prelude::*}; pub type CodeHash = ::Hash; pub type TrieId = Vec; From 624b3efbdfdafaa4e7fb8d55d451655ae7cce0f7 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Fri, 31 Jan 2020 17:19:15 +0100 Subject: [PATCH 33/36] Renaming --- frame/contracts/src/gas_weight_conv.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/frame/contracts/src/gas_weight_conv.rs b/frame/contracts/src/gas_weight_conv.rs index e9afc09e5fc3e..e7a0a4f730763 100644 --- a/frame/contracts/src/gas_weight_conv.rs +++ b/frame/contracts/src/gas_weight_conv.rs @@ -29,9 +29,9 @@ use sp_runtime::{ }; use sp_std::{convert::TryFrom, fmt, marker::PhantomData, prelude::*}; -#[doc(hidden)] +/// Data which is collected during the pre-dispatch phase and needed at the post_dispatch phase. #[cfg_attr(test, derive(Debug))] -pub struct DynamicWeightData { +pub struct GasCallEnvelope { /// The account id who should receive the refund from the gas leftovers. transactor: AccountId, /// The negative imbalance obtained by withdrawing the value up to the requested gas limit. @@ -53,7 +53,7 @@ impl GasWeightConversion { who: &T::AccountId, call: &::Call, ) -> Result< - Option>>, + Option>>, TransactionValidityError, > { // This signed extension only deals with this module's `Call`. @@ -104,7 +104,7 @@ impl GasWeightConversion { Err(_) => return Err(InvalidTransaction::Payment.into()), }; - Ok(Some(DynamicWeightData { + Ok(Some(GasCallEnvelope { transactor: who.clone(), imbalance, })) @@ -141,7 +141,7 @@ impl SignedExtension for GasWeightConversion { /// Optional pre-dispatch check data. /// /// It is present only for the contract calls that operate with gas. - type Pre = Option>>; + type Pre = Option>>; fn additional_signed(&self) -> Result<(), TransactionValidityError> { Ok(()) @@ -168,7 +168,7 @@ impl SignedExtension for GasWeightConversion { } fn post_dispatch(pre: Self::Pre, _info: DispatchInfo, _len: usize) { - if let Some(DynamicWeightData { + if let Some(GasCallEnvelope { transactor, imbalance, }) = pre From 57850d089a7caab87072414692f474228b943c3b Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Tue, 18 Feb 2020 16:24:13 +0100 Subject: [PATCH 34/36] Clean --- frame/contracts/src/gas.rs | 6 +- frame/contracts/src/lib.rs | 26 +++-- frame/contracts/src/tests.rs | 126 ++++++++++++++++--------- frame/contracts/src/wasm/code_cache.rs | 4 +- frame/system/src/lib.rs | 3 +- 5 files changed, 103 insertions(+), 62 deletions(-) diff --git a/frame/contracts/src/gas.rs b/frame/contracts/src/gas.rs index 9d578bb65ccb9..0d279575d1519 100644 --- a/frame/contracts/src/gas.rs +++ b/frame/contracts/src/gas.rs @@ -14,10 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use crate::{Trait, BalanceOf}; -use sp_runtime::traits::{ - Zero, SaturatedConversion, AtLeast32Bit, -}; +use crate::{BalanceOf, Trait}; +use sp_runtime::traits::{AtLeast32Bit, SaturatedConversion, Zero}; #[cfg(test)] use std::{any::Any, fmt::Debug}; diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index c0e93e13cc6e4..acc1b545d2f7e 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -93,9 +93,9 @@ mod gas; mod account_db; mod exec; -mod wasm; -mod rent; mod gas_weight_conv; +mod rent; +mod wasm; #[cfg(test)] mod tests; @@ -113,11 +113,12 @@ use frame_support::dispatch::{DispatchResult, Dispatchable}; use frame_support::{ decl_error, decl_event, decl_module, decl_storage, parameter_types, storage::child, - traits::{OnReapAccount, Currency, Get, OnUnbalanced, Randomness, Time}, + traits::{Currency, Get, OnReapAccount, OnUnbalanced, Randomness, Time}, weights::{DispatchClass, FunctionOf, Weight}, IsSubType, Parameter, }; -use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root}; +use frame_system::{self as system, ensure_root, ensure_signed, RawOrigin}; +use pallet_contracts_primitives::{ContractAccessError, RentProjection}; #[cfg(feature = "std")] use serde::{Deserialize, Serialize}; use sp_core::crypto::UncheckedFrom; @@ -125,7 +126,6 @@ use sp_core::storage::well_known_keys::CHILD_STORAGE_KEY_PREFIX; use sp_io::hashing::blake2_256; use sp_runtime::traits::{Convert, Hash, MaybeSerializeDeserialize, Member, StaticLookup, Zero}; use sp_std::{fmt, marker::PhantomData, prelude::*}; -use pallet_contracts_primitives::{RentProjection, ContractAccessError}; pub type CodeHash = ::Hash; pub type TrieId = Vec; @@ -242,10 +242,16 @@ pub struct RawTombstoneContractInfo(H, PhantomData); impl RawTombstoneContractInfo where - H: Member + MaybeSerializeDeserialize + fmt::Debug - + AsRef<[u8]> + AsMut<[u8]> + Copy + Default - + sp_std::hash::Hash + Codec, - Hasher: Hash, + H: Member + + MaybeSerializeDeserialize + + fmt::Debug + + AsRef<[u8]> + + AsMut<[u8]> + + Copy + + Default + + sp_std::hash::Hash + + Codec, + Hasher: Hash, { fn new(storage_root: &[u8], code_hash: H) -> Self { let mut buf = Vec::new(); @@ -718,7 +724,7 @@ impl Module { fn execute_wasm( origin: T::AccountId, gas_limit: Gas, - func: impl FnOnce(&mut ExecutionContext, &mut GasMeter) -> ExecResult + func: impl FnOnce(&mut ExecutionContext, &mut GasMeter) -> ExecResult, ) -> ExecResult { // // Take the gas price prepared by the signed extension. diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 076fb91b5bf6f..99923a927e2b8 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -19,28 +19,36 @@ #![allow(unused)] +use crate::gas_weight_conv::GasWeightConversion; use crate::{ + account_db::{AccountDb, DirectAccountDb, OverlayAccountDb}, BalanceOf, ComputeDispatchFee, ContractAddressFor, ContractInfo, ContractInfoOf, GenesisConfig, - Module, RawAliveContractInfo, RawEvent, Trait, TrieId, TrieIdFromParentCounter, Schedule, - TrieIdGenerator, account_db::{AccountDb, DirectAccountDb, OverlayAccountDb}, + Module, RawAliveContractInfo, RawEvent, Schedule, Trait, TrieId, TrieIdFromParentCounter, + TrieIdGenerator, }; -use crate::gas_weight_conv::GasWeightConversion; use assert_matches::assert_matches; -use hex_literal::*; use codec::{Decode, Encode, KeyedVec}; +use frame_support::{ + assert_err, assert_ok, impl_outer_dispatch, impl_outer_event, impl_outer_origin, + parameter_types, + storage::child, + traits::{Currency, Get}, + weights::{DispatchClass, DispatchInfo, Weight}, + StorageMap, StorageValue, +}; +use frame_system::{self as system, EventRecord, Phase}; +use hex_literal::*; +use sp_core::storage::well_known_keys; use sp_runtime::{ - Perbill, BuildStorage, transaction_validity::{InvalidTransaction, ValidTransaction}, - traits::{BlakeTwo256, Hash, IdentityLookup, SignedExtension}, testing::{Digest, DigestItem, Header, UintAuthorityId, H256}, + traits::{BlakeTwo256, Hash, IdentityLookup, SignedExtension}, + transaction_validity::{InvalidTransaction, ValidTransaction}, + BuildStorage, Perbill, }; -use frame_support::{ - assert_ok, assert_err, impl_outer_dispatch, impl_outer_event, impl_outer_origin, parameter_types, - storage::child, StorageMap, StorageValue, traits::{Currency, Get}, - weights::{DispatchInfo, DispatchClass, Weight}, +use std::{ + cell::RefCell, + sync::atomic::{AtomicUsize, Ordering}, }; -use std::{cell::RefCell, sync::atomic::{AtomicUsize, Ordering}}; -use sp_core::storage::well_known_keys; -use frame_system::{self as system, EventRecord, Phase}; mod contracts { // Re-export contents of the root. This basically @@ -87,10 +95,11 @@ impl Get for TransferFee { pub struct CreationFee; impl Get for CreationFee { - fn get() -> u64 { INSTANTIATION_FEE.with(|v| *v.borrow()) } + fn get() -> u64 { + INSTANTIATION_FEE.with(|v| *v.borrow()) + } } - #[derive(Clone, Eq, PartialEq, Debug)] pub struct Test; parameter_types! { @@ -261,23 +270,27 @@ impl ExtBuilder { pub fn build(self) -> sp_io::TestExternalities { self.set_associated_consts(); let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); - pallet_balances::GenesisConfig:: { - balances: vec![], - }.assimilate_storage(&mut t).unwrap(); + pallet_balances::GenesisConfig:: { balances: vec![] } + .assimilate_storage(&mut t) + .unwrap(); GenesisConfig { current_schedule: Schedule { enable_println: true, ..Default::default() }, - }.assimilate_storage(&mut t).unwrap(); + } + .assimilate_storage(&mut t) + .unwrap(); sp_io::TestExternalities::new(t) } } /// Generate Wasm binary and code hash from wabt source. -fn compile_module(wabt_module: &str) - -> Result<(Vec, ::Output), wabt::Error> - where T: frame_system::Trait +fn compile_module( + wabt_module: &str, +) -> Result<(Vec, ::Output), wabt::Error> +where + T: frame_system::Trait, { let wasm = wabt::wat2wasm(wabt_module)?; let code_hash = T::Hashing::hash(&wasm); @@ -289,7 +302,13 @@ fn compile_module(wabt_module: &str) fn call_doesnt_pay_for_gas() { ExtBuilder::default().build().execute_with(|| { Balances::deposit_creating(&ALICE, 100_000_000); - assert_ok!(Contracts::call(Origin::signed(ALICE), BOB, 0, 100_000, Vec::new())); + assert_ok!(Contracts::call( + Origin::signed(ALICE), + BOB, + 0, + 100_000, + Vec::new() + )); assert_eq!(Balances::free_balance(&ALICE), 100_000_000); }); } @@ -881,9 +900,12 @@ fn test_set_rent_code_and_hash() { let (wasm, code_hash) = compile_module::(CODE_SET_RENT).unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); + ExtBuilder::default() + .existential_deposit(50) + .build() + .execute_with(|| { + Balances::deposit_creating(&ALICE, 1_000_000); + assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); // If you ever need to update the wasm source this test will fail // and will show you the actual hash. @@ -1398,10 +1420,13 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: let (restoration_wasm, restoration_code_hash) = compile_module::(CODE_RESTORATION).unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contracts::put_code(Origin::signed(ALICE), restoration_wasm)); - assert_ok!(Contracts::put_code(Origin::signed(ALICE), set_rent_wasm)); + ExtBuilder::default() + .existential_deposit(50) + .build() + .execute_with(|| { + Balances::deposit_creating(&ALICE, 1_000_000); + assert_ok!(Contracts::put_code(Origin::signed(ALICE), restoration_wasm)); + assert_ok!(Contracts::put_code(Origin::signed(ALICE), set_rent_wasm)); // If you ever need to update the wasm source this test will fail // and will show you the actual hash. @@ -2168,11 +2193,14 @@ const CODE_SELF_DESTRUCT: &str = r#" #[test] fn self_destruct_by_draining_balance() { let (wasm, code_hash) = compile_module::(CODE_SELF_DESTRUCT).unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); + ExtBuilder::default() + .existential_deposit(50) + .build() + .execute_with(|| { + Balances::deposit_creating(&ALICE, 1_000_000); + assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); - // Instantiate the BOB contract. + // Instantiate the BOB contract. assert_ok!(Contracts::instantiate( Origin::signed(ALICE), 100_000, @@ -2204,11 +2232,14 @@ fn self_destruct_by_draining_balance() { #[test] fn cannot_self_destruct_while_live() { let (wasm, code_hash) = compile_module::(CODE_SELF_DESTRUCT).unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); + ExtBuilder::default() + .existential_deposit(50) + .build() + .execute_with(|| { + Balances::deposit_creating(&ALICE, 1_000_000); + assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); - // Instantiate the BOB contract. + // Instantiate the BOB contract. assert_ok!(Contracts::instantiate( Origin::signed(ALICE), 100_000, @@ -2499,9 +2530,12 @@ const CODE_SELF_DESTRUCTING_CONSTRUCTOR: &str = r#" #[test] fn cannot_self_destruct_in_constructor() { let (wasm, code_hash) = compile_module::(CODE_SELF_DESTRUCTING_CONSTRUCTOR).unwrap(); - ExtBuilder::default().existential_deposit(50).build().execute_with(|| { - Balances::deposit_creating(&ALICE, 1_000_000); - assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); + ExtBuilder::default() + .existential_deposit(50) + .build() + .execute_with(|| { + Balances::deposit_creating(&ALICE, 1_000_000); + assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); // Fail to instantiate the BOB contract since its final balance is below existential // deposit. @@ -2521,18 +2555,22 @@ fn cannot_self_destruct_in_constructor() { #[test] fn check_block_gas_limit_works() { ExtBuilder::default().build().execute_with(|| { - let info = DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: true }; + let info = DispatchInfo { + weight: 100, + class: DispatchClass::Normal, + pays_fee: true, + }; let check = GasWeightConversion::::new(); let call: Call = crate::Call::put_code(vec![]).into(); match check.pre_dispatch(&0, &call, info, 0) { - Ok(None) => {}, + Ok(None) => {} _ => panic!("put_code is not dynamic and should pass validation"), } let check = GasWeightConversion::::new(); let call: Call = crate::Call::call(Default::default(), 0, 100, vec![]).into(); match check.pre_dispatch(&0, &call, info, 0) { - Ok(Some(_)) => {}, + Ok(Some(_)) => {} _ => panic!(), } }); diff --git a/frame/contracts/src/wasm/code_cache.rs b/frame/contracts/src/wasm/code_cache.rs index ba7a02356d282..6850701afa9cf 100644 --- a/frame/contracts/src/wasm/code_cache.rs +++ b/frame/contracts/src/wasm/code_cache.rs @@ -28,9 +28,9 @@ use crate::wasm::{prepare, runtime::Env, PrefabWasmModule}; use crate::{CodeHash, CodeStorage, PristineCode, Schedule, Trait}; -use sp_std::prelude::*; -use sp_runtime::traits::Hash; use frame_support::StorageMap; +use sp_runtime::traits::Hash; +use sp_std::prelude::*; /// Put code in the storage. The hash of code is used as a key and is returned /// as a result of this function. diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 9f1f95e5fc51e..95bc957748454 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -710,8 +710,7 @@ impl Module { /// /// This is the opposite of `all_extrinsics_weight` provided for convenience. pub fn remaining_weight() -> Weight { - T::MaximumBlockWeight::get() - .saturating_sub(Self::all_extrinsics_weight()) + T::MaximumBlockWeight::get().saturating_sub(Self::all_extrinsics_weight()) } /// Inform the system module of some additional weight that should be accounted for, in the From 4585dc1060f215a443b3f9b8c3d2fde051c20e50 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Tue, 18 Feb 2020 16:26:37 +0100 Subject: [PATCH 35/36] Bump runtime version --- bin/node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 8d8f96dc25fd6..d4f17f36505e5 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -82,7 +82,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to 0. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 221, + spec_version: 222, impl_version: 0, apis: RUNTIME_API_VERSIONS, }; From cc8ec73f62ea01aefd4c89b6c47570b106dd08e1 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Tue, 18 Feb 2020 16:37:23 +0100 Subject: [PATCH 36/36] Fix the build. --- bin/node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index d4f17f36505e5..e1a734b123200 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -638,7 +638,7 @@ construct_runtime!( FinalityTracker: pallet_finality_tracker::{Module, Call, Inherent}, Grandpa: pallet_grandpa::{Module, Call, Storage, Config, Event}, Treasury: pallet_treasury::{Module, Call, Storage, Config, Event}, - Contracts: pallet_contracts::{Module, Call, Config, Storage, Event}, + Contracts: pallet_contracts::{Module, Call, Config, Storage, Event}, Sudo: pallet_sudo::{Module, Call, Config, Storage, Event}, ImOnline: pallet_im_online::{Module, Call, Storage, Event, ValidateUnsigned, Config}, AuthorityDiscovery: pallet_authority_discovery::{Module, Call, Config},