diff --git a/substrate/node/runtime/src/lib.rs b/substrate/node/runtime/src/lib.rs index 6d07e3b974..c640139630 100644 --- a/substrate/node/runtime/src/lib.rs +++ b/substrate/node/runtime/src/lib.rs @@ -59,7 +59,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { impl_name: create_runtime_str!("substrate-node"), authoring_version: 10, spec_version: 99, - impl_version: 103, + impl_version: 104, apis: RUNTIME_API_VERSIONS, }; diff --git a/substrate/srml/contracts/src/gas.rs b/substrate/srml/contracts/src/gas.rs index 634b11d3db..8f8ebd3604 100644 --- a/substrate/srml/contracts/src/gas.rs +++ b/substrate/srml/contracts/src/gas.rs @@ -93,7 +93,6 @@ pub struct GasMeter { tokens: Vec, } impl GasMeter { - #[cfg(test)] pub fn with_limit(gas_limit: Gas, gas_price: BalanceOf) -> GasMeter { GasMeter { limit: gas_limit, @@ -162,13 +161,7 @@ impl GasMeter { f(None) } else { self.gas_left = self.gas_left - amount; - let mut nested = GasMeter { - limit: amount, - gas_left: amount, - gas_price: self.gas_price, - #[cfg(test)] - tokens: Vec::new(), - }; + let mut nested = GasMeter::with_limit(amount, self.gas_price); let r = f(Some(&mut nested)); @@ -231,14 +224,7 @@ pub fn buy_gas( ExistenceRequirement::KeepAlive )?; - Ok((GasMeter { - limit: gas_limit, - gas_left: gas_limit, - gas_price, - - #[cfg(test)] - tokens: Vec::new(), - }, imbalance)) + Ok((GasMeter::with_limit(gas_limit, gas_price), imbalance)) } /// Refund the unused gas. diff --git a/substrate/srml/contracts/src/rent.rs b/substrate/srml/contracts/src/rent.rs index 3baf043b90..3be39af6aa 100644 --- a/substrate/srml/contracts/src/rent.rs +++ b/substrate/srml/contracts/src/rent.rs @@ -17,7 +17,7 @@ use crate::{BalanceOf, ContractInfo, ContractInfoOf, Module, TombstoneContractInfo, Trait}; use runtime_primitives::traits::{Bounded, CheckedDiv, CheckedMul, Saturating, Zero, SaturatedConversion}; -use srml_support::traits::{Currency, ExistenceRequirement, Imbalance, WithdrawReason}; +use srml_support::traits::{Currency, ExistenceRequirement, WithdrawReason}; use srml_support::StorageMap; #[derive(PartialEq, Eq, Copy, Clone)] @@ -95,14 +95,26 @@ fn try_evict_or_and_pay_rent( // The minimal amount of funds required for a contract not to be evicted. let subsistence_threshold = T::Currency::minimum_balance() + >::tombstone_deposit(); + if balance < subsistence_threshold { + // The contract cannot afford to leave a tombstone, so remove the contract info altogether. + >::remove(account); + runtime_io::kill_child_storage(&contract.trie_id); + return RentOutcome::Evicted; + } + let dues = fee_per_block .checked_mul(&blocks_passed.saturated_into::().into()) .unwrap_or(>::max_value()); + let rent_budget = contract.rent_allowance.min(balance - subsistence_threshold); + let insufficient_rent = rent_budget < dues; - let dues_limited = dues.min(contract.rent_allowance); - let rent_allowance_exceeded = dues > contract.rent_allowance; - let is_below_subsistence = balance < subsistence_threshold; - let go_below_subsistence = balance.saturating_sub(dues_limited) < subsistence_threshold; + // If the rent payment cannot be withdrawn due to locks on the account balance, then evict the + // account. + // + // NOTE: This seems problematic because it provides a way to tombstone an account while + // avoiding the last rent payment. In effect, someone could retroactively set rent_allowance + // for their contract to 0. + let dues_limited = dues.min(rent_budget); let can_withdraw_rent = T::Currency::ensure_can_withdraw( account, dues_limited, @@ -111,68 +123,52 @@ fn try_evict_or_and_pay_rent( ) .is_ok(); - if !rent_allowance_exceeded && can_withdraw_rent && !go_below_subsistence { - // Collect dues - - if pay_rent { - let imbalance = T::Currency::withdraw( - account, - dues, - WithdrawReason::Fee, - ExistenceRequirement::KeepAlive, - ) - .expect( - "Withdraw has been checked above; - go_below_subsistence is false and subsistence > existencial_deposit; - qed", - ); - - >::mutate(account, |contract| { - let contract = contract - .as_mut() - .and_then(|c| c.as_alive_mut()) - .expect("Dead or inexistent account has been exempt above; qed"); - - contract.rent_allowance -= imbalance.peek(); // rent_allowance is not exceeded - contract.deduct_block = current_block_number; - }) - } - - RentOutcome::Ok - } else { - // Evict - - if can_withdraw_rent && !go_below_subsistence { - T::Currency::withdraw( - account, - dues, - WithdrawReason::Fee, - ExistenceRequirement::KeepAlive, - ) - .expect("Can withdraw and don't go below subsistence"); - } else if !is_below_subsistence { - T::Currency::make_free_balance_be(account, subsistence_threshold); - } else { - T::Currency::make_free_balance_be(account, >::zero()); - } - - if !is_below_subsistence { - // The contract has funds above subsistence deposit and that means it can afford to - // leave tombstone. - - // Note: this operation is heavy. - let child_storage_root = runtime_io::child_storage_root(&contract.trie_id); - - let tombstone = >::new( - &child_storage_root[..], - contract.code_hash, - ); - >::insert(account, ContractInfo::Tombstone(tombstone)); - runtime_io::kill_child_storage(&contract.trie_id); - } - - RentOutcome::Evicted + if can_withdraw_rent && (insufficient_rent || pay_rent) { + // Collect dues. + let _ = T::Currency::withdraw( + account, + dues_limited, + WithdrawReason::Fee, + ExistenceRequirement::KeepAlive, + ) + .expect( + "Withdraw has been checked above; + dues_limited < rent_budget < balance - subsistence < balance - existential_deposit; + qed", + ); } + + if insufficient_rent || !can_withdraw_rent { + // The contract cannot afford the rent payment and has a balance above the subsistence + // threshold, so it leaves a tombstone. + + // Note: this operation is heavy. + let child_storage_root = runtime_io::child_storage_root(&contract.trie_id); + + let tombstone = >::new( + &child_storage_root[..], + contract.code_hash, + ); + >::insert(account, ContractInfo::Tombstone(tombstone)); + + runtime_io::kill_child_storage(&contract.trie_id); + + return RentOutcome::Evicted; + } + + if pay_rent { + >::mutate(account, |contract| { + let contract = contract + .as_mut() + .and_then(|c| c.as_alive_mut()) + .expect("Dead or nonexistent account has been exempt above; qed"); + + contract.rent_allowance -= dues; // rent_allowance is not exceeded + contract.deduct_block = current_block_number; + }) + } + + RentOutcome::Ok } /// Make account paying the rent for the current block number diff --git a/substrate/srml/contracts/src/tests.rs b/substrate/srml/contracts/src/tests.rs index adc462d445..e4f435817b 100644 --- a/substrate/srml/contracts/src/tests.rs +++ b/substrate/srml/contracts/src/tests.rs @@ -742,7 +742,7 @@ mod call { pub fn null() -> Vec { vec![0, 0, 0] } } -/// Test correspondance of set_rent code and its hash. +/// Test correspondence of set_rent code and its hash. /// Also test that encoded extrinsic in code correspond to the correct transfer #[test] fn test_set_rent_code_and_hash() { @@ -954,9 +954,12 @@ fn removals(trigger_call: impl Fn() -> bool) { ::Balance::from(1_000u32).encode() // rent allowance )); + let subsistence_threshold = 50 /*existential_deposit*/ + 16 /*tombstone_deposit*/; + // Trigger rent must have no effect assert!(trigger_call()); assert_eq!(ContractInfoOf::::get(BOB).unwrap().get_alive().unwrap().rent_allowance, 1_000); + assert_eq!(Balances::free_balance(&BOB), 100); // Advance blocks System::initialize(&10, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default()); @@ -964,6 +967,7 @@ fn removals(trigger_call: impl Fn() -> bool) { // Trigger rent through call assert!(trigger_call()); assert!(ContractInfoOf::::get(BOB).unwrap().get_tombstone().is_some()); + assert_eq!(Balances::free_balance(&BOB), subsistence_threshold); // Advance blocks System::initialize(&20, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default()); @@ -971,6 +975,7 @@ fn removals(trigger_call: impl Fn() -> bool) { // Trigger rent must have no effect assert!(trigger_call()); assert!(ContractInfoOf::::get(BOB).unwrap().get_tombstone().is_some()); + assert_eq!(Balances::free_balance(&BOB), subsistence_threshold); } ); @@ -991,6 +996,7 @@ fn removals(trigger_call: impl Fn() -> bool) { // Trigger rent must have no effect assert!(trigger_call()); assert_eq!(ContractInfoOf::::get(BOB).unwrap().get_alive().unwrap().rent_allowance, 100); + assert_eq!(Balances::free_balance(&BOB), 1_000); // Advance blocks System::initialize(&10, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default()); @@ -998,6 +1004,8 @@ fn removals(trigger_call: impl Fn() -> bool) { // Trigger rent through call assert!(trigger_call()); assert!(ContractInfoOf::::get(BOB).unwrap().get_tombstone().is_some()); + // Balance should be initial balance - initial rent_allowance + assert_eq!(Balances::free_balance(&BOB), 900); // Advance blocks System::initialize(&20, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default()); @@ -1005,6 +1013,7 @@ fn removals(trigger_call: impl Fn() -> bool) { // Trigger rent must have no effect assert!(trigger_call()); assert!(ContractInfoOf::::get(BOB).unwrap().get_tombstone().is_some()); + assert_eq!(Balances::free_balance(&BOB), 900); } ); @@ -1025,10 +1034,12 @@ fn removals(trigger_call: impl Fn() -> bool) { // Trigger rent must have no effect assert!(trigger_call()); assert_eq!(ContractInfoOf::::get(BOB).unwrap().get_alive().unwrap().rent_allowance, 1_000); + assert_eq!(Balances::free_balance(&BOB), 50 + Balances::minimum_balance()); // Transfer funds assert_ok!(Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::transfer())); assert_eq!(ContractInfoOf::::get(BOB).unwrap().get_alive().unwrap().rent_allowance, 1_000); + assert_eq!(Balances::free_balance(&BOB), Balances::minimum_balance()); // Advance blocks System::initialize(&10, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default()); @@ -1036,6 +1047,7 @@ fn removals(trigger_call: impl Fn() -> bool) { // Trigger rent through call assert!(trigger_call()); assert!(ContractInfoOf::::get(BOB).is_none()); + assert_eq!(Balances::free_balance(&BOB), Balances::minimum_balance()); // Advance blocks System::initialize(&20, &[0u8; 32].into(), &[0u8; 32].into(), &Default::default()); @@ -1043,6 +1055,7 @@ fn removals(trigger_call: impl Fn() -> bool) { // Trigger rent must have no effect assert!(trigger_call()); assert!(ContractInfoOf::::get(BOB).is_none()); + assert_eq!(Balances::free_balance(&BOB), Balances::minimum_balance()); } ); } diff --git a/substrate/srml/contracts/src/wasm/mod.rs b/substrate/srml/contracts/src/wasm/mod.rs index 40e873542b..71c1a1d3f4 100644 --- a/substrate/srml/contracts/src/wasm/mod.rs +++ b/substrate/srml/contracts/src/wasm/mod.rs @@ -175,7 +175,7 @@ mod tests { use std::collections::HashMap; use substrate_primitives::H256; use crate::exec::{CallReceipt, Ext, InstantiateReceipt, EmptyOutputBuf, StorageKey}; - use crate::gas::GasMeter; + use crate::gas::{Gas, GasMeter}; use crate::tests::{Test, Call}; use crate::wasm::prepare::prepare_contract; use crate::CodeHash; @@ -906,14 +906,20 @@ mod tests { fn gas_left() { let mut mock_ext = MockExt::default(); let mut gas_meter = GasMeter::with_limit(50_000, 1312); + + let mut return_buf = Vec::new(); execute( CODE_GAS_LEFT, &[], - &mut Vec::new(), + &mut return_buf, &mut mock_ext, &mut gas_meter, ) .unwrap(); + + let gas_left = Gas::decode(&mut &return_buf[..]).unwrap(); + assert!(gas_left < 50_000, "gas_left must be less than initial"); + assert!(gas_left > gas_meter.gas_left(), "gas_left must be greater than final"); } const CODE_VALUE_TRANSFERRED: &str = r#" diff --git a/substrate/srml/contracts/src/wasm/prepare.rs b/substrate/srml/contracts/src/wasm/prepare.rs index d141cd6b52..0aed3655e8 100644 --- a/substrate/srml/contracts/src/wasm/prepare.rs +++ b/substrate/srml/contracts/src/wasm/prepare.rs @@ -30,11 +30,7 @@ use runtime_primitives::traits::{SaturatedConversion}; struct ContractModule<'a> { /// A deserialized module. The module is valid (this is Guaranteed by `new` method). - /// - /// An `Option` is used here for loaning (`take()`-ing) the module. - /// Invariant: Can't be `None` (i.e. on enter and on exit from the function - /// the value *must* be `Some`). - module: Option, + module: elements::Module, schedule: &'a Schedule, } @@ -58,7 +54,7 @@ impl<'a> ContractModule<'a> { // Return a `ContractModule` instance with // __valid__ module. Ok(ContractModule { - module: Some(module), + module, schedule, }) } @@ -69,11 +65,7 @@ impl<'a> ContractModule<'a> { /// Memory section contains declarations of internal linear memories, so if we find one /// we reject such a module. fn ensure_no_internal_memory(&self) -> Result<(), &'static str> { - let module = self - .module - .as_ref() - .expect("On entry to the function `module` can't be None; qed"); - if module + if self.module .memory_section() .map_or(false, |ms| ms.entries().len() > 0) { @@ -82,7 +74,7 @@ impl<'a> ContractModule<'a> { Ok(()) } - fn inject_gas_metering(&mut self) -> Result<(), &'static str> { + fn inject_gas_metering(self) -> Result { let gas_rules = rules::Set::new( self.schedule.regular_op_cost.clone().saturated_into(), @@ -91,30 +83,22 @@ impl<'a> ContractModule<'a> { .with_grow_cost(self.schedule.grow_mem_cost.clone().saturated_into()) .with_forbidden_floats(); - let module = self - .module - .take() - .expect("On entry to the function `module` can't be `None`; qed"); - - let contract_module = pwasm_utils::inject_gas_counter(module, &gas_rules) + let contract_module = pwasm_utils::inject_gas_counter(self.module, &gas_rules) .map_err(|_| "gas instrumentation failed")?; - - self.module = Some(contract_module); - Ok(()) + Ok(ContractModule { + module: contract_module, + schedule: self.schedule, + }) } - fn inject_stack_height_metering(&mut self) -> Result<(), &'static str> { - let module = self - .module - .take() - .expect("On entry to the function `module` can't be `None`; qed"); - + fn inject_stack_height_metering(self) -> Result { let contract_module = - pwasm_utils::stack_height::inject_limiter(module, self.schedule.max_stack_height) + pwasm_utils::stack_height::inject_limiter(self.module, self.schedule.max_stack_height) .map_err(|_| "stack height instrumentation failed")?; - - self.module = Some(contract_module); - Ok(()) + Ok(ContractModule { + module: contract_module, + schedule: self.schedule, + }) } /// Check that the module has required exported functions. For now @@ -128,10 +112,7 @@ impl<'a> ContractModule<'a> { let mut deploy_found = false; let mut call_found = false; - let module = self - .module - .as_ref() - .expect("On entry to the function `module` can't be `None`; qed"); + let module = &self.module; let types = module.type_section().map(|ts| ts.types()).unwrap_or(&[]); let export_entries = module @@ -213,10 +194,7 @@ impl<'a> ContractModule<'a> { /// their signatures. /// - if there is a memory import, returns it's descriptor fn scan_imports(&self) -> Result, &'static str> { - let module = self - .module - .as_ref() - .expect("On entry to the function `module` can't be `None`; qed"); + let module = &self.module; let types = module.type_section().map(|ts| ts.types()).unwrap_or(&[]); let import_entries = module @@ -269,13 +247,9 @@ impl<'a> ContractModule<'a> { Ok(imported_mem_type) } - fn into_wasm_code(mut self) -> Result, &'static str> { - elements::serialize( - self.module - .take() - .expect("On entry to the function `module` can't be `None`; qed"), - ) - .map_err(|_| "error serializing instrumented module") + fn into_wasm_code(self) -> Result, &'static str> { + elements::serialize(self.module) + .map_err(|_| "error serializing instrumented module") } } @@ -332,8 +306,9 @@ pub fn prepare_contract( } }; - contract_module.inject_gas_metering()?; - contract_module.inject_stack_height_metering()?; + contract_module = contract_module + .inject_gas_metering()? + .inject_stack_height_metering()?; Ok(PrefabWasmModule { schedule_version: schedule.version, diff --git a/substrate/srml/contracts/src/wasm/runtime.rs b/substrate/srml/contracts/src/wasm/runtime.rs index 479b296d81..d05b716478 100644 --- a/substrate/srml/contracts/src/wasm/runtime.rs +++ b/substrate/srml/contracts/src/wasm/runtime.rs @@ -276,7 +276,7 @@ define_env!(Env, , Ok(()) }, - // Retrieve the value at the given location from the strorage and return 0. + // Retrieve the value at the given location from the storage and return 0. // If there is no entry at the given location then this function will return 1 and // clear the scratch buffer. //