Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Contracts: Per block gas limit #506

Merged
merged 6 commits into from
Aug 28, 2018
Merged

Contracts: Per block gas limit #506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e0f993c
Add block gas limit check
pepyakin Aug 7, 2018
d3872e4
Fix formatting
pepyakin Aug 7, 2018
192667b
Add block_gas_limit test.
pepyakin Aug 7, 2018
ae34fa9
Use ExtBuilder in tests
pepyakin Aug 7, 2018
51d30a2
Docs and clean-up.
pepyakin Aug 7, 2018
72ba19b
Correct style
pepyakin Aug 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 27 additions & 1 deletion substrate/runtime/contract/src/gas.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
// You should have received a copy of the GNU General Public License
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.

use {Trait, Module};
use {Trait, Module, GasSpent};
use runtime_primitives::traits::{As, CheckedMul, CheckedSub, Zero};
use runtime_support::StorageValue;
use staking;

#[must_use]
Expand All @@ -35,13 +36,16 @@ impl GasMeterResult {
}

pub struct GasMeter<T: Trait> {
limit: T::Gas,
/// Amount of gas left from initial gas limit. Can reach zero.
gas_left: T::Gas,
gas_price: T::Balance,
}
impl<T: Trait> GasMeter<T> {
#[cfg(test)]
pub fn with_limit(gas_limit: T::Gas, gas_price: T::Balance) -> GasMeter<T> {
GasMeter {
limit: gas_limit,
gas_left: gas_limit,
gas_price,
}
Expand Down Expand Up @@ -101,6 +105,7 @@ impl<T: Trait> GasMeter<T> {
} else {
self.gas_left = self.gas_left - amount;
let mut nested = GasMeter {
limit: amount,
gas_left: amount,
gas_price: self.gas_price,
};
Expand All @@ -117,6 +122,11 @@ impl<T: Trait> GasMeter<T> {
pub fn gas_left(&self) -> T::Gas {
self.gas_left
}

/// Returns how much gas was spent.
fn spent(&self) -> T::Gas {
self.limit - self.gas_left
}
}

/// Buy the given amount of gas.
Expand All @@ -127,6 +137,14 @@ pub fn buy_gas<T: Trait>(
transactor: &T::AccountId,
gas_limit: T::Gas,
) -> Result<GasMeter<T>, &'static str> {
// Check if the specified amount of gas is available in the current block.
// This cannot underflow since `gas_spent` is never greater than `block_gas_limit`.
let gas_available = <Module<T>>::block_gas_limit() - <Module<T>>::gas_spent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not store gas_left instead of the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean instead of block_gas_limit and gas_spent make just once gas_left?
Hm, I don't see how that could work. At the end of the block we need to replenish gas_left to the initial per block limit. We can hardcode it or... add block_gas_limit storage variable : )

By the way, I considered using block_gas_limit + gas_left combo. The reason why I chose gas_spent is because you can reset it to zero at the finalization stage. With gas_left you need to replenish it before executing the first transaction in the block and it is not obvious to me how to make it cleanly.

if gas_limit > gas_available {
return Err("block gas limit is reached");
}

// Buy the specified amount of gas.
let gas_price = <Module<T>>::gas_price();
let b = <staking::Module<T>>::free_balance(transactor);
let cost = <T::Gas as As<T::Balance>>::as_(gas_limit.clone())
Expand All @@ -138,13 +156,21 @@ pub fn buy_gas<T: Trait>(
<staking::Module<T>>::set_free_balance(transactor, b - cost);
<staking::Module<T>>::decrease_total_stake_by(cost);
Ok(GasMeter {
limit: gas_limit,
gas_left: gas_limit,
gas_price,
})
}

/// Refund the unused gas.
pub fn refund_unused_gas<T: Trait>(transactor: &T::AccountId, gas_meter: GasMeter<T>) {
// Increase total spent gas.
// This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which
// also has T::Gas type.
let gas_spent = <Module<T>>::gas_spent() + gas_meter.spent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? Are we sure to never add the same gas_meter.spent() twice here?

My point is that:

  1. You spend some gas and store it here (so you have B + spent in the storage)
  2. You spend some more, say X and you add it here, so you will have (B + spent + (spent + X)) in the storage

I suppose it's not really a case, since we consume gas_meter and probably take care of proper accounting on some other layer, but wouldn't it be simpler to just store global gas_left in the storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's not really the case and yes, I've tried hard to achieve the design where you have the construct like:

1. gas_meter = buy_gas(price)
... do stuff with gas meter
n. refund_unused_gas(gas_meter)

so it is an invariant that we do refund and spent gas accounting only once per transaction and the mechanism for that, as you noticed, is consuming the GasMeter.

Let's move our discussion why I used gas_spent instead of gas_left in this thread!

<GasSpent<T>>::put(gas_spent);

// Refund gas left by the price it was bought.
let b = <staking::Module<T>>::free_balance(transactor);
let refund = <T::Gas as As<T::Balance>>::as_(gas_meter.gas_left) * gas_meter.gas_price;
<staking::Module<T>>::set_free_balance(transactor, b + refund);
Expand Down
6 changes: 4 additions & 2 deletions substrate/runtime/contract/src/genesis_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

//! Build the contract module part of the genesis block storage.

use {Trait, ContractFee, CallBaseFee, CreateBaseFee, GasPrice, MaxDepth};
use {Trait, ContractFee, CallBaseFee, CreateBaseFee, GasPrice, MaxDepth, BlockGasLimit};

use runtime_primitives;
use runtime_io::{self, twox_128};
Expand All @@ -34,6 +34,7 @@ pub struct GenesisConfig<T: Trait> {
pub create_base_fee: T::Gas,
pub gas_price: T::Balance,
pub max_depth: u32,
pub block_gas_limit: T::Gas,
}

impl<T: Trait> runtime_primitives::BuildStorage for GenesisConfig<T> {
Expand All @@ -43,7 +44,8 @@ impl<T: Trait> runtime_primitives::BuildStorage for GenesisConfig<T> {
twox_128(<CallBaseFee<T>>::key()).to_vec() => self.call_base_fee.encode(),
twox_128(<CreateBaseFee<T>>::key()).to_vec() => self.create_base_fee.encode(),
twox_128(<GasPrice<T>>::key()).to_vec() => self.gas_price.encode(),
twox_128(<MaxDepth<T>>::key()).to_vec() => self.max_depth.encode()
twox_128(<MaxDepth<T>>::key()).to_vec() => self.max_depth.encode(),
twox_128(<BlockGasLimit<T>>::key()).to_vec() => self.block_gas_limit.encode()
];
Ok(r.into())
}
Expand Down
43 changes: 36 additions & 7 deletions substrate/runtime/contract/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,31 @@
//! create smart-contracts or send messages to existing smart-contracts.
//!
//! For any actions invoked by the smart-contracts fee must be paid. The fee is paid in gas.
//! Gas is bought upfront. Any unused is refunded after the transaction (regardless of the
//! execution outcome). If all gas is used, then changes made for the specific call or create
//! are reverted (including balance transfers).
//! Gas is bought upfront up to the, specified in transaction, limit. Any unused gas is refunded
Copy link
Member

@gavofyork gavofyork Aug 28, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should read "upfront for the limit, which is specified in transaction"

//! after the transaction (regardless of the execution outcome). If all gas is used,
//! then changes made for the specific call or create are reverted (including balance transfers).
//!
//! Failures are typically not cascading. That, for example, means that if contract A calls B and B errors
//! somehow, then A can decide if it should proceed or error.
//! TODO: That is not the case now, since call/create externalities traps on any error now.
//!
//! # Interaction with the system
//!
//! ## Finalization
//!
//! This module requires performing some finalization steps at the end of the block. If not performed
//! the module will have incorrect behavior.
//!
//! Call [`Module::execute`] at the end of the block. The order in relation to
//! the other module doesn't matter.
//!
//! ## Account killing
//!
//! When `staking` module determines that account is dead (e.g. account's balance fell below
//! exsistential deposit) then it reaps the account. That will lead to deletion of the associated
//! code and storage of the account.
//!
//! [`Module::execute`]: struct.Module.html#impl-Executable

#![cfg_attr(not(feature = "std"), no_std)]

Expand Down Expand Up @@ -90,16 +108,16 @@ use account_db::{AccountDb, OverlayAccountDb};
use double_map::StorageDoubleMap;

use codec::Codec;
use runtime_primitives::traits::{As, RefInto, SimpleArithmetic};
use runtime_primitives::traits::{As, RefInto, SimpleArithmetic, Executable};
use runtime_support::dispatch::Result;
use runtime_support::{Parameter, StorageMap};
use runtime_support::{Parameter, StorageMap, StorageValue};

pub trait Trait: system::Trait + staking::Trait + consensus::Trait {
/// Function type to get the contract address given the creator.
type DetermineContractAddress: ContractAddressFor<Self::AccountId>;

// As<u32> is needed for wasm-utils
type Gas: Parameter + Codec + SimpleArithmetic + Copy + As<Self::Balance> + As<u64> + As<u32>;
type Gas: Parameter + Default + Codec + SimpleArithmetic + Copy + As<Self::Balance> + As<u64> + As<u32>;
}

pub trait ContractAddressFor<AccountId: Sized> {
Expand Down Expand Up @@ -144,9 +162,13 @@ decl_storage! {
GasPrice get(gas_price): b"con:gas_price" => required T::Balance;
// The maximum nesting level of a call/create stack.
MaxDepth get(max_depth): b"con:max_depth" => required u32;
// The maximum amount of gas that could be expended per block.
BlockGasLimit get(block_gas_limit): b"con:block_gas_limit" => required T::Gas;
// Gas spent so far in this block.
GasSpent get(gas_spent): b"con:gas_spent" => default T::Gas;

// The code associated with an account.
pub CodeOf: b"con:cod:" => default map [ T::AccountId => Vec<u8> ]; // TODO Vec<u8> values should be optimised to not do a length prefix.
CodeOf: b"con:cod:" => default map [ T::AccountId => Vec<u8> ]; // TODO Vec<u8> values should be optimised to not do a length prefix.
}

// TODO: consider storing upper-bound for contract's gas limit in fixed-length runtime
Expand Down Expand Up @@ -253,3 +275,10 @@ impl<T: Trait> staking::OnFreeBalanceZero<T::AccountId> for Module<T> {
<StorageOf<T>>::remove_prefix(who.clone());
}
}

/// Finalization hook for the smart-contract module.
impl<T: Trait> Executable for Module<T> {
fn execute() {
<GasSpent<T>>::kill();
}
}