diff --git a/.github/workflows/tests-evm.yml b/.github/workflows/tests-evm.yml index 4cc49626bf1ff..af0d7eb22b008 100644 --- a/.github/workflows/tests-evm.yml +++ b/.github/workflows/tests-evm.yml @@ -43,46 +43,30 @@ jobs: uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 with: repository: paritytech/evm-test-suite - ref: 79144d85626f97e481b84a16d2b8f0813d03548b + ref: 84e536af80513f87bc16f6c7b7dbf796fe9010ab path: evm-test-suite - - uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 # v5.0.0 + - uses: denoland/setup-deno@v1 with: - node-version: 22 + deno-version: v2.x - name: script - env: - # EVM tests don't work with batchSize 300 on self-hosted runners in docker container - BATCH_SIZE: 100 run: | echo "Change to the evm-test-suite directory" cd evm-test-suite - echo "Download the resolc binary" - wget -O resolc https://github.com/paritytech/revive/releases/download/v0.3.0/resolc-x86_64-unknown-linux-musl -q - chmod +x resolc - mv resolc /usr/local/bin - resolc --version + + deno --version echo "Check that binaries are in place" - export NODE_BIN_PATH=$(readlink -f ../target/release/revive-dev-node) + export REVIVE_DEV_NODE_PATH=$(readlink -f ../target/release/revive-dev-node) export ETH_RPC_PATH=$(readlink -f ../target/release/eth-rpc) - export RESOLC_PATH=/usr/local/bin/resolc - echo $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH + echo $REVIVE_DEV_NODE_PATH $ETH_RPC_PATH - echo "Install npm dependencies" - npm install - # cat matter-labs-tests/hardhat.config.ts | grep batchSize + echo "== Running pvm tests ==j" + START_REVIVE_DEV_NODE=true START_ETH_RPC=true deno task test:pvm - echo "Installing solc" - wget https://github.com/ethereum/solidity/releases/download/v0.8.30/solc-static-linux -q - chmod +x solc-static-linux - mv solc-static-linux /usr/local/bin/solc - # TODO restore once tests can be run against the revive-dev-node - # echo "Run the tests" - # echo "bash init.sh --kitchensink -- --matter-labs -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH" - # bash init.sh --kitchensink -- --matter-labs -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH - echo "Run eth-rpc tests" - bash init.sh --kitchensink http://localhost:9944 --eth-rpc -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH + echo "== Running evm tests ==" + START_REVIVE_DEV_NODE=true START_ETH_RPC=true deno task test:evm - name: Collect tests results if: always() diff --git a/Cargo.lock b/Cargo.lock index fd4b72ce36c90..74cadcb1d239c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13211,6 +13211,7 @@ dependencies = [ "hex-literal", "humantime-serde", "impl-trait-for-tuples", + "itertools 0.11.0", "log", "num-bigint", "num-integer", diff --git a/prdoc/pr_10018.prdoc b/prdoc/pr_10018.prdoc new file mode 100644 index 0000000000000..6fad1e352df74 --- /dev/null +++ b/prdoc/pr_10018.prdoc @@ -0,0 +1,23 @@ +title: 'pallet_revive: Change EVM call opcodes to respect the gas limit passed' +doc: +- audience: Runtime Dev + description: |- + So far the EVM family of call opcodes did ignore the `gas` argument passed to them. The consequence was that we were not able to limit the resource usage of sub contract calls. This PR changes that. **Gas is now fully functional on the EVM backend.** + + The resources of any sub contract call are now effectively limited. This is both true for `Weight` and storage deposit. The algorithm works in a way that if you pass `x%` of the current `GAS` the the `CALL` opcode the sub call will have `x%` of currently available `Weight` and storage deposit available. This allows the caller to always make sure to execute code after retuning from a sub call. + + ### Changes to the gas meter + + I needed to change the gas meter to track `gas_consumed` instead of `gas_left`. Otherwise it is not possible to know the total amount of gas spent for a call stack that is not unwinded, yet. + + ### Followup + - Implement a new PVM syscall that takes the new unified gas instead of `Weight` and storage deposit limit + - Change resolc to use this new syscall + - Enable the test added here to run on resolc +crates: +- name: pallet-revive + bump: major +- name: pallet-revive-eth-rpc + bump: major +- name: pallet-revive-fixtures + bump: major diff --git a/substrate/frame/revive/Cargo.toml b/substrate/frame/revive/Cargo.toml index 01e9f76b16240..ef3ecbc98ce04 100644 --- a/substrate/frame/revive/Cargo.toml +++ b/substrate/frame/revive/Cargo.toml @@ -63,6 +63,7 @@ subxt-signer = { workspace = true, optional = true, features = ["unstable-eth"] [dev-dependencies] array-bytes = { workspace = true, default-features = true } assert_matches = { workspace = true } +itertools = { workspace = true } pretty_assertions = { workspace = true } secp256k1 = { workspace = true, features = ["recovery"] } serde_json = { workspace = true } diff --git a/substrate/frame/revive/fixtures/contracts/Callee.sol b/substrate/frame/revive/fixtures/contracts/Callee.sol index 8407f7962bdb2..eef33113cb444 100644 --- a/substrate/frame/revive/fixtures/contracts/Callee.sol +++ b/substrate/frame/revive/fixtures/contracts/Callee.sol @@ -32,4 +32,8 @@ contract Callee { stop() } } + + function consumeAllReftime() external { + while (true) {} + } } diff --git a/substrate/frame/revive/fixtures/contracts/Caller.sol b/substrate/frame/revive/fixtures/contracts/Caller.sol index 3a4f1c440ff1d..dca1e6861c5e9 100644 --- a/substrate/frame/revive/fixtures/contracts/Caller.sol +++ b/substrate/frame/revive/fixtures/contracts/Caller.sol @@ -9,6 +9,8 @@ contract ChildRevert { } contract Caller { + uint256 public data; + function normal(address _callee, uint64 _value, bytes memory _data, uint64 _gas) external returns (bool success, bytes memory output) @@ -65,4 +67,22 @@ contract Caller { } } } + + function callPartialGas(address _callee, bytes memory _data, uint64 _gasDivisor, uint8 _callType) + external + returns (bool success) + { + uint256 gas = gasleft() / _gasDivisor; + bytes memory output; + if (_callType == 0) { + (success, output) = _callee.call{gas: gas }(_data); + } else if (_callType == 1) { + (success, output) = _callee.staticcall{gas: gas }(_data); + } else if (_callType == 2) { + (success, output) = _callee.delegatecall{gas: gas }(_data); + } else { + revert("unknown call type"); + } + data = 42; + } } diff --git a/substrate/frame/revive/rpc/src/lib.rs b/substrate/frame/revive/rpc/src/lib.rs index bcb9e9b388b2e..285adb0510c21 100644 --- a/substrate/frame/revive/rpc/src/lib.rs +++ b/substrate/frame/revive/rpc/src/lib.rs @@ -23,7 +23,6 @@ use jsonrpsee::{ types::{ErrorCode, ErrorObjectOwned}, }; use pallet_revive::evm::*; -use sp_arithmetic::Permill; use sp_core::{keccak_256, H160, H256, U256}; use thiserror::Error; use tokio::time::Duration; @@ -265,9 +264,9 @@ impl EthRpcServer for EthRpcServerImpl { } async fn max_priority_fee_per_gas(&self) -> RpcResult { - // TODO: Provide better estimation - let gas_price = self.gas_price().await?; - Ok(Permill::from_percent(20).mul_ceil(gas_price)) + // We do not support tips. Hence the recommended priority fee is + // always zero. The effective gas price will always be the base price. + Ok(Default::default()) } async fn get_code(&self, address: H160, block: BlockNumberOrTagOrHash) -> RpcResult { diff --git a/substrate/frame/revive/src/evm/fees.rs b/substrate/frame/revive/src/evm/fees.rs index 8e3724fe09693..2a4ec341fb7d5 100644 --- a/substrate/frame/revive/src/evm/fees.rs +++ b/substrate/frame/revive/src/evm/fees.rs @@ -130,7 +130,7 @@ pub trait InfoT: seal::Sealed { } /// Convert a weight to an unadjusted fee. - fn weight_to_fee(_weight: &Weight, _combinator: Combinator) -> BalanceOf { + fn weight_to_fee(_weight: &Weight) -> BalanceOf { Zero::zero() } @@ -158,14 +158,6 @@ pub trait InfoT: seal::Sealed { } } -/// Which function to use in order to combine `ref_time` and `proof_size` to a fee. -pub enum Combinator { - /// Minimum function. - Min, - /// Maximum function. - Max, -} - impl BlockRatioFee { const REF_TIME_TO_FEE: FixedU128 = { assert!(P > 0 && Q > 0); @@ -179,26 +171,17 @@ impl BlockRatioFee { FixedU128::from_rational(max_weight.ref_time().into(), max_weight.proof_size().into()); Self::REF_TIME_TO_FEE.saturating_mul(ratio) } - - /// Calculate the fee for a weight. - fn weight_to_fee(weight: &Weight, combinator: Combinator) -> BalanceOf { - let ref_time_fee = Self::REF_TIME_TO_FEE - .saturating_mul_int(BalanceOf::::saturated_from(weight.ref_time())); - let proof_size_fee = Self::proof_size_to_fee() - .saturating_mul_int(BalanceOf::::saturated_from(weight.proof_size())); - - match combinator { - Combinator::Max => ref_time_fee.max(proof_size_fee), - Combinator::Min => ref_time_fee.min(proof_size_fee), - } - } } impl WeightToFee for BlockRatioFee { type Balance = BalanceOf; fn weight_to_fee(weight: &Weight) -> Self::Balance { - Self::weight_to_fee(weight, Combinator::Max) + let ref_time_fee = Self::REF_TIME_TO_FEE + .saturating_mul_int(BalanceOf::::saturated_from(weight.ref_time())); + let proof_size_fee = Self::proof_size_to_fee() + .saturating_mul_int(BalanceOf::::saturated_from(weight.proof_size())); + ref_time_fee.max(proof_size_fee) } } @@ -244,8 +227,8 @@ where /// Calculate the fee using the weight instead of a dispatch info. fn tx_fee_from_weight(encoded_len: u32, weight: &Weight) -> BalanceOf { let fixed_fee = Self::fixed_fee(encoded_len); - let weight_fee = Self::next_fee_multiplier() - .saturating_mul_int(Self::weight_to_fee(weight, Combinator::Max)); + let weight_fee = + Self::next_fee_multiplier().saturating_mul_int(Self::weight_to_fee(weight)); fixed_fee.saturating_add(weight_fee) } @@ -254,7 +237,6 @@ where &::BlockWeights::get() .get(DispatchClass::Normal) .base_extrinsic, - Combinator::Max, ) .saturating_add(Self::length_to_fee(encoded_len)) } @@ -316,8 +298,8 @@ where uxt.encoded_size() as u32 } - fn weight_to_fee(weight: &Weight, combinator: Combinator) -> BalanceOf { - ::WeightToFee::weight_to_fee(&weight, combinator) + fn weight_to_fee(weight: &Weight) -> BalanceOf { + ::WeightToFee::weight_to_fee(weight) } /// Convert an unadjusted fee back to a weight. diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 743bb84a32a7b..36d9c281a1779 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -17,7 +17,7 @@ use crate::{ address::{self, AddressMapper}, - evm::fees::{Combinator, InfoT}, + evm::fees::InfoT, gas::GasMeter, limits, precompiles::{All as AllPrecompiles, Instance as PrecompileInstance, Precompiles}, @@ -56,7 +56,7 @@ use sp_core::{ use sp_io::{crypto::secp256k1_ecdsa_recover_compressed, hashing::blake2_256}; use sp_runtime::{ traits::{BadOrigin, Bounded, Saturating, TrailingZeroInput, Zero}, - DispatchError, SaturatedConversion, + DispatchError, FixedPointNumber, FixedU128, SaturatedConversion, }; #[cfg(test)] @@ -186,6 +186,22 @@ impl Origin { } } } + +/// Argument passed by a contact to describe the amount of resources allocated to a cross contact +/// call. +pub enum CallResources { + /// Resources encoded using their actual values. + Precise { weight: Weight, deposit_limit: U256 }, + /// Resources encoded as unified ethereum gas. + Ethereum(U256), +} + +impl Default for CallResources { + fn default() -> Self { + Self::Precise { weight: Default::default(), deposit_limit: Default::default() } + } +} + /// Environment functions only available to host functions. pub trait Ext: PrecompileWithInfoExt { /// Execute code in the current frame. @@ -193,8 +209,7 @@ pub trait Ext: PrecompileWithInfoExt { /// Returns the code size of the called contract. fn delegate_call( &mut self, - gas_limit: Weight, - deposit_limit: U256, + call_resources: &CallResources, address: H160, input_data: Vec, ) -> Result<(), ExecError>; @@ -278,8 +293,7 @@ pub trait PrecompileExt: sealing::Sealed { /// Call (possibly transferring some amount of funds) into the specified account. fn call( &mut self, - gas_limit: Weight, - deposit_limit: U256, + call_resources: &CallResources, to: &H160, value: U256, input_data: Vec, @@ -435,6 +449,7 @@ pub trait PrecompileExt: sealing::Sealed { /// The amount of gas left in eth gas units. fn gas_left(&self) -> u64; + /// Returns the storage entry of the executing account by the given `key`. /// /// Returns `None` if the `key` wasn't previously set by `set_storage` or @@ -1675,6 +1690,45 @@ where } true } + + /// Calc the limits for a cross contract call. + fn calc_limits(&self, call_resources: &CallResources) -> (Weight, BalanceOf) { + match call_resources { + CallResources::Precise { weight, deposit_limit } => + (*weight, (*deposit_limit).saturated_into()), + CallResources::Ethereum(gas_limit) => { + // the resources of the subcall are relative to the available resources + let available: BalanceOf = self.gas_left().saturated_into(); + let gas_limit = (*gas_limit).saturated_into::>().min(available); + let ratio = FixedU128::from_rational( + gas_limit.saturated_into(), + available.max(1u32.into()).saturated_into(), + ); + + // weight limit is expected to be set to we can just multiply directly + let weight_limit = { + let weight_left = self.top_frame().nested_gas.gas_left(); + Weight::from_parts( + ratio.saturating_mul_int(weight_left.ref_time()), + ratio.saturating_mul_int(weight_left.proof_size()), + ) + }; + + // the gas_limit is in unadjusted fee + let deposit_limit = { + let weight_fee = T::FeeInfo::weight_to_fee(&weight_limit); + gas_limit.saturating_sub(weight_fee).min( + ratio.saturating_mul_int( + T::FeeInfo::next_fee_multiplier_reciprocal() + .saturating_mul_int(self.top_frame().nested_storage.available()), + ), + ) + }; + + (weight_limit, T::FeeInfo::next_fee_multiplier().saturating_mul_int(deposit_limit)) + }, + } + } } impl<'a, T, E> Ext for Stack<'a, T, E> @@ -1684,8 +1738,7 @@ where { fn delegate_call( &mut self, - gas_limit: Weight, - deposit_limit: U256, + call_resources: &CallResources, address: H160, input_data: Vec, ) -> Result<(), ExecError> { @@ -1693,6 +1746,8 @@ where // This is for example the case for unknown code hashes or creating the frame fails. *self.last_frame_output_mut() = Default::default(); + let (weight, deposit_limit) = self.calc_limits(call_resources); + let top_frame = self.top_frame_mut(); let contract_info = top_frame.contract_info().clone(); let account_id = top_frame.account_id.clone(); @@ -1707,8 +1762,8 @@ where }), }, value, - gas_limit, - deposit_limit.saturated_into::>(), + weight, + deposit_limit, self.is_read_only(), )? { self.run(executable, input_data) @@ -1868,8 +1923,7 @@ where fn call( &mut self, - gas_limit: Weight, - deposit_limit: U256, + call_resources: &CallResources, dest_addr: &H160, value: U256, input_data: Vec, @@ -1885,6 +1939,8 @@ where // This is for example the case for balance transfers or when creating the frame fails. *self.last_frame_output_mut() = Default::default(); + let (weight, deposit_limit) = self.calc_limits(call_resources); + let try_call = || { // Enable read-only access if requested; cannot disable it if already set. let is_read_only = read_only || self.is_read_only(); @@ -1914,8 +1970,8 @@ where if let Some(executable) = self.push_frame( FrameArgs::Call { dest: dest.clone(), cached_info, delegated_call: None }, value, - gas_limit, - deposit_limit.saturated_into::>(), + weight, + deposit_limit, is_read_only, )? { self.run(executable, input_data) @@ -2192,40 +2248,56 @@ where fn gas_left(&self) -> u64 { let frame = self.top_frame(); - if let Some((encoded_len, base_weight)) = self.exec_config.collect_deposit_from_hold { - // when using the txhold we know the overall available fee by looking at the tx credit + + // when using the txhold we know the overall available fee by looking at the tx credit + // we need to use that to limit gas_left because in that case no storage deposit limit is + // set + let max_by_credit_hold = if let Some((encoded_len, base_weight)) = + self.exec_config.collect_deposit_from_hold + { // we work backwards: the gas_left is the overall fee minus what was already consumed + let (deposit_consumed, weight_consumed) = + self.frames.iter().chain(core::iter::once(&self.first_frame)).fold( + (StorageDeposit::default(), Weight::default()), + |(deposit, weight), frame| { + ( + deposit.saturating_add(&frame.nested_storage.consumed()), + weight.saturating_add(frame.nested_gas.gas_consumed()), + ) + }, + ); let weight_fee_consumed = T::FeeInfo::tx_fee_from_weight( encoded_len, - &frame.nested_gas.gas_consumed().saturating_add(base_weight), + &weight_consumed.saturating_add(base_weight), ); let available = T::FeeInfo::remaining_txfee().saturating_sub(weight_fee_consumed); - let deposit_consumed = self - .frames - .iter() - .chain(core::iter::once(&self.first_frame)) - .fold(StorageDeposit::default(), |acc, frame| { - acc.saturating_add(&frame.nested_storage.consumed()) - }); deposit_consumed.available(&available) } else { - // when not using the hold we expect the transaction to contain a limit for the storage - // deposit we work forwards: add up what is left from both meters - // in case no storage limit is set we limit by all the free balance of the signer + BalanceOf::::max_value() + }; + + // this is the actual limit derived from what is set in the meter + let max_by_meter = { use frame_support::traits::tokens::{Fortitude, Preservation}; - let weight_fee_available = - T::FeeInfo::weight_to_fee(&frame.nested_gas.gas_left(), Combinator::Min); - let available_balance = self + let weight_fee_available = T::FeeInfo::weight_to_fee(&frame.nested_gas.gas_left()); + + // in the dry run no deposit limit is set. + // this means its only limited by the origins free balance + let deposit_available = self .origin .account_id() .map(|acc| { T::Currency::reducible_balance(acc, Preservation::Preserve, Fortitude::Polite) }) - .unwrap_or(BalanceOf::::max_value()); - let deposit_available = frame.nested_storage.available().min(available_balance); + .unwrap_or(BalanceOf::::max_value()) + .min(frame.nested_storage.available()); + weight_fee_available.saturating_add(deposit_available) - } - .saturated_into() + }; + + T::FeeInfo::next_fee_multiplier_reciprocal() + .saturating_mul_int(max_by_credit_hold.min(max_by_meter)) + .saturated_into() } fn get_storage(&mut self, key: &Key) -> Option> { diff --git a/substrate/frame/revive/src/exec/mock_ext.rs b/substrate/frame/revive/src/exec/mock_ext.rs index b0ed00dbb19ae..3592350580a1d 100644 --- a/substrate/frame/revive/src/exec/mock_ext.rs +++ b/substrate/frame/revive/src/exec/mock_ext.rs @@ -18,7 +18,10 @@ #![cfg(test)] use crate::{ - exec::{AccountIdOf, ExecError, Ext, Key, Origin, PrecompileExt, PrecompileWithInfoExt}, + exec::{ + AccountIdOf, CallResources, ExecError, Ext, Key, Origin, PrecompileExt, + PrecompileWithInfoExt, + }, gas::GasMeter, precompiles::Diff, storage::{ContractInfo, WriteOutcome}, @@ -48,8 +51,7 @@ impl PrecompileExt for MockExt { fn call( &mut self, - _gas_limit: Weight, - _deposit_limit: U256, + _call_resources: &CallResources, _to: &H160, _value: U256, _input_data: Vec, @@ -259,8 +261,7 @@ impl PrecompileWithInfoExt for MockExt { impl Ext for MockExt { fn delegate_call( &mut self, - _gas_limit: Weight, - _deposit_limit: U256, + _call_resources: &CallResources, _address: H160, _input_data: Vec, ) -> Result<(), ExecError> { diff --git a/substrate/frame/revive/src/exec/tests.rs b/substrate/frame/revive/src/exec/tests.rs index 88d06265dfbb6..502d7eff7a1e3 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -375,7 +375,7 @@ fn correct_transfer_on_delegate_call() { let delegate_ch = MockLoader::insert(Call, move |ctx, _| { assert_eq!(ctx.ext.value_transferred(), evm_value); - ctx.ext.delegate_call(Weight::zero(), U256::zero(), CHARLIE_ADDR, Vec::new())?; + ctx.ext.delegate_call(&Default::default(), CHARLIE_ADDR, Vec::new())?; Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() }) }); @@ -409,7 +409,7 @@ fn delegate_call_missing_contract() { }); let delegate_ch = MockLoader::insert(Call, move |ctx, _| { - ctx.ext.delegate_call(Weight::zero(), U256::zero(), CHARLIE_ADDR, Vec::new())?; + ctx.ext.delegate_call(&Default::default(), CHARLIE_ADDR, Vec::new())?; Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() }) }); @@ -633,15 +633,7 @@ fn max_depth() { let value = 0; let recurse_ch = MockLoader::insert(Call, |ctx, _| { // Try to call into yourself. - let r = ctx.ext.call( - Weight::zero(), - U256::zero(), - &BOB_ADDR, - U256::zero(), - vec![], - true, - false, - ); + let r = ctx.ext.call(&Default::default(), &BOB_ADDR, U256::zero(), vec![], true, false); ReachedBottom::mutate(|reached_bottom| { if !*reached_bottom { @@ -696,15 +688,8 @@ fn caller_returns_proper_values() { // Call into CHARLIE contract. assert_matches!( - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false - ), + ctx.ext + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false), Ok(_) ); exec_success() @@ -760,15 +745,8 @@ fn origin_returns_proper_values() { // Call into CHARLIE contract. assert_matches!( - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false - ), + ctx.ext + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false), Ok(_) ); exec_success() @@ -916,7 +894,7 @@ fn caller_is_origin_returns_proper_values() { assert!(ctx.ext.caller_is_origin(false)); // BOB calls CHARLIE ctx.ext - .call(Weight::zero(), U256::zero(), &CHARLIE_ADDR, U256::zero(), vec![], true, false) + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false) .map(|_| ctx.ext.last_frame_output().clone()) }); @@ -1004,7 +982,7 @@ fn root_caller_succeeds_with_consecutive_calls() { assert!(ctx.ext.caller_is_root(false)); // BOB calls CHARLIE. ctx.ext - .call(Weight::zero(), U256::zero(), &CHARLIE_ADDR, U256::zero(), vec![], true, false) + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false) .map(|_| ctx.ext.last_frame_output().clone()) }); @@ -1035,15 +1013,8 @@ fn address_returns_proper_values() { // Call into charlie contract. assert_matches!( - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false - ), + ctx.ext + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false), Ok(_) ); exec_success() @@ -1368,15 +1339,7 @@ fn in_memory_changes_not_discarded() { info.storage_byte_deposit = 42; assert_eq!( ctx.ext - .call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false - ) + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false) .map(|_| ctx.ext.last_frame_output().clone()), exec_trapped() ); @@ -1387,7 +1350,7 @@ fn in_memory_changes_not_discarded() { let code_charlie = MockLoader::insert(Call, |ctx, _| { assert!(ctx .ext - .call(Weight::zero(), U256::zero(), &BOB_ADDR, U256::zero(), vec![99], true, false) + .call(&Default::default(), &BOB_ADDR, U256::zero(), vec![99], true, false) .is_ok()); exec_trapped() }); @@ -1423,8 +1386,7 @@ fn recursive_call_during_constructor_is_balance_transfer() { // Calling ourselves during the constructor will trigger a balance // transfer since no contract exist yet. assert_ok!(ctx.ext.call( - Weight::zero(), - U256::zero(), + &Default::default(), &addr, (balance - 1).into(), vec![], @@ -1435,8 +1397,7 @@ fn recursive_call_during_constructor_is_balance_transfer() { // Should also work with call data set as it is ignored when no // contract is deployed. assert_ok!(ctx.ext.call( - Weight::zero(), - U256::zero(), + &Default::default(), &addr, 1u32.into(), vec![1, 2, 3, 4], @@ -1480,15 +1441,8 @@ fn cannot_send_more_balance_than_available_to_self() { let balance = ctx.ext.balance(); assert_err!( - ctx.ext.call( - Weight::zero(), - U256::zero(), - &addr, - (balance + 1).into(), - vec![], - true, - false - ), + ctx.ext + .call(&Default::default(), &addr, (balance + 1).into(), vec![], true, false), >::TransferFailed, ); exec_success() @@ -1523,7 +1477,7 @@ fn call_reentry_direct_recursion() { let code_bob = MockLoader::insert(Call, |ctx, _| { let dest = H160::from_slice(ctx.input_data.as_ref()); ctx.ext - .call(Weight::zero(), U256::zero(), &dest, U256::zero(), vec![], false, false) + .call(&Default::default(), &dest, U256::zero(), vec![], false, false) .map(|_| ctx.ext.last_frame_output().clone()) }); @@ -1568,15 +1522,7 @@ fn call_deny_reentry() { let code_bob = MockLoader::insert(Call, |ctx, _| { if ctx.input_data[0] == 0 { ctx.ext - .call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - false, - false, - ) + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], false, false) .map(|_| ctx.ext.last_frame_output().clone()) } else { exec_success() @@ -1586,7 +1532,7 @@ fn call_deny_reentry() { // call BOB with input set to '1' let code_charlie = MockLoader::insert(Call, |ctx, _| { ctx.ext - .call(Weight::zero(), U256::zero(), &BOB_ADDR, U256::zero(), vec![1], true, false) + .call(&Default::default(), &BOB_ADDR, U256::zero(), vec![1], true, false) .map(|_| ctx.ext.last_frame_output().clone()) }); @@ -1690,7 +1636,7 @@ fn nonce() { // a plain call should not influence the account counter ctx.ext - .call(Weight::zero(), U256::zero(), &addr, U256::zero(), vec![], false, false) + .call(&Default::default(), &addr, U256::zero(), vec![], false, false) .unwrap(); assert_eq!(System::account_nonce(ALICE), alice_nonce); @@ -2197,15 +2143,7 @@ fn get_transient_storage_works() { ); assert_eq!( ctx.ext - .call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false, - ) + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false,) .map(|_| ctx.ext.last_frame_output().clone()), exec_success() ); @@ -2227,7 +2165,7 @@ fn get_transient_storage_works() { let code_charlie = MockLoader::insert(Call, |ctx, _| { assert!(ctx .ext - .call(Weight::zero(), U256::zero(), &BOB_ADDR, U256::zero(), vec![99], true, false) + .call(&Default::default(), &BOB_ADDR, U256::zero(), vec![99], true, false) .is_ok()); // CHARLIE can not read BOB`s storage. assert_eq!(ctx.ext.get_transient_storage(storage_key_1), None); @@ -2303,15 +2241,7 @@ fn rollback_transient_storage_works() { ); assert_eq!( ctx.ext - .call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false - ) + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false) .map(|_| ctx.ext.last_frame_output().clone()), exec_trapped() ); @@ -2329,7 +2259,7 @@ fn rollback_transient_storage_works() { let code_charlie = MockLoader::insert(Call, |ctx, _| { assert!(ctx .ext - .call(Weight::zero(), U256::zero(), &BOB_ADDR, U256::zero(), vec![99], true, false) + .call(&Default::default(), &BOB_ADDR, U256::zero(), vec![99], true, false) .is_ok()); exec_trapped() }); @@ -2412,8 +2342,7 @@ fn last_frame_output_works_on_instantiate() { // Balance transfers should reset the output ctx.ext .call( - Weight::MAX, - U256::MAX, + &CallResources::Precise { weight: Weight::MAX, deposit_limit: U256::MAX }, &address, Pallet::::convert_native_to_evm(1), vec![], @@ -2495,15 +2424,7 @@ fn last_frame_output_works_on_nested_call() { ); ctx.ext - .call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false, - ) + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false) .unwrap(); assert_eq!( ctx.ext.last_frame_output(), @@ -2522,7 +2443,7 @@ fn last_frame_output_works_on_nested_call() { assert!(ctx .ext - .call(Weight::zero(), U256::zero(), &BOB_ADDR, U256::zero(), vec![99], true, false) + .call(&Default::default(), &BOB_ADDR, U256::zero(), vec![99], true, false) .is_ok()); assert_eq!( ctx.ext.last_frame_output(), @@ -2561,8 +2482,7 @@ fn last_frame_output_is_always_reset() { *ctx.ext.last_frame_output_mut() = output_revert(); assert_eq!( ctx.ext.call( - Weight::zero(), - U256::zero(), + &Default::default(), &H160::zero(), U256::max_value(), vec![], @@ -2576,8 +2496,7 @@ fn last_frame_output_is_always_reset() { // An unknown code hash should succeed but clear the output. *ctx.ext.last_frame_output_mut() = output_revert(); assert_ok!(ctx.ext.delegate_call( - Weight::zero(), - U256::zero(), + &Default::default(), H160([0xff; 20]), Default::default() )); @@ -2680,24 +2599,18 @@ fn correct_immutable_data_in_delegate_call() { // In a regular call, we should witness the callee immutable data assert_eq!( ctx.ext - .call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false, - ) + .call(&Default::default(), &CHARLIE_ADDR, U256::zero(), vec![], true, false,) .map(|_| ctx.ext.last_frame_output().data.clone()), Ok(vec![2]), ); // Also in a delegate call, we should witness the callee immutable data assert_eq!( - ctx.ext - .delegate_call(Weight::zero(), U256::zero(), CHARLIE_ADDR, Vec::new()) - .map(|_| ctx.ext.last_frame_output().data.clone()), + ctx.ext.delegate_call(&Default::default(), CHARLIE_ADDR, Vec::new()).map(|_| ctx + .ext + .last_frame_output() + .data + .clone()), Ok(vec![2]) ); diff --git a/substrate/frame/revive/src/gas.rs b/substrate/frame/revive/src/gas.rs index 038e58b3d93cc..9da6ce7266b93 100644 --- a/substrate/frame/revive/src/gas.rs +++ b/substrate/frame/revive/src/gas.rs @@ -21,7 +21,7 @@ use frame_support::{ weights::Weight, DefaultNoBound, }; -use sp_runtime::{traits::Zero, DispatchError}; +use sp_runtime::DispatchError; #[cfg(test)] use std::{any::Any, fmt::Debug}; @@ -142,10 +142,11 @@ pub struct ErasedToken { #[derive(DefaultNoBound)] pub struct GasMeter { gas_limit: Weight, - /// Amount of gas left from initial gas limit. Can reach zero. - gas_left: Weight, - /// Due to `adjust_gas` and `nested` the `gas_left` can temporarily dip below its final value. - gas_left_lowest: Weight, + /// Amount of gas already consumed. Must be < `gas_limit`. + gas_consumed: Weight, + /// Due to `adjust_gas` and `nested` the `gas_consumed` can temporarily peak above its final + /// value. + gas_consumed_highest: Weight, /// The amount of resources that was consumed by the execution engine. /// We have to track it separately in order to avoid the loss of precision that happens when /// converting from ref_time to the execution engine unit. @@ -159,8 +160,8 @@ impl GasMeter { pub fn new(gas_limit: Weight) -> Self { GasMeter { gas_limit, - gas_left: gas_limit, - gas_left_lowest: gas_limit, + gas_consumed: Default::default(), + gas_consumed_highest: Default::default(), engine_meter: EngineMeter::new(gas_limit), _phantom: PhantomData, #[cfg(test)] @@ -173,24 +174,21 @@ impl GasMeter { /// This should only be used by the primordial frame in a sequence of calls - every subsequent /// frame should use [`nested`](Self::nested). pub fn nested_take_all(&mut self) -> Self { - let gas_left = self.gas_left; - self.gas_left -= gas_left; - GasMeter::new(gas_left) + GasMeter::new(self.gas_left()) } /// Create a new gas meter for a nested call by removing gas from the current meter. pub fn nested(&mut self, amount: Weight) -> Self { - let amount = amount.min(self.gas_left); - self.gas_left -= amount; - GasMeter::new(amount) + GasMeter::new(self.gas_left().min(amount)) } /// Absorb the remaining gas of a nested meter after we are done using it. pub fn absorb_nested(&mut self, nested: Self) { - self.gas_left_lowest = (self.gas_left + nested.gas_limit) - .saturating_sub(nested.gas_required()) - .min(self.gas_left_lowest); - self.gas_left += nested.gas_left; + self.gas_consumed_highest = self + .gas_consumed + .saturating_add(nested.gas_required()) + .max(self.gas_consumed_highest); + self.gas_consumed += nested.gas_consumed; } /// Account for used gas. @@ -214,7 +212,14 @@ impl GasMeter { let amount = token.weight(); // It is OK to not charge anything on failure because we always charge _before_ we perform // any action - self.gas_left = self.gas_left.checked_sub(&amount).ok_or_else(|| Error::::OutOfGas)?; + let consumed = { + let consumed = self.gas_consumed.saturating_add(amount); + if consumed.any_gt(self.gas_limit) { + Err(>::OutOfGas)?; + } + consumed + }; + self.gas_consumed = consumed; Ok(ChargedAmount(amount)) } @@ -233,10 +238,10 @@ impl GasMeter { /// refunded to match the actual amount. pub fn adjust_gas>(&mut self, charged_amount: ChargedAmount, token: Tok) { if token.influence_lowest_gas_limit() { - self.gas_left_lowest = self.gas_left_lowest(); + self.gas_consumed_highest = self.gas_required(); } let adjustment = charged_amount.0.saturating_sub(token.weight()); - self.gas_left = self.gas_left.saturating_add(adjustment).min(self.gas_limit); + self.gas_consumed = self.gas_consumed.saturating_sub(adjustment); } /// Hand over the gas metering responsibility from the executor to this meter. @@ -251,10 +256,12 @@ impl GasMeter { let weight_consumed = self .engine_meter .set_fuel(engine_fuel.try_into().map_err(|_| Error::::OutOfGas)?); - self.gas_left - .checked_reduce(weight_consumed) - .ok_or_else(|| Error::::OutOfGas)?; - Ok(RefTimeLeft(self.gas_left.ref_time())) + self.gas_consumed.saturating_accrue(weight_consumed); + if self.gas_consumed.any_gt(self.gas_limit) { + self.gas_consumed = self.gas_limit; + Err(>::OutOfGas)?; + } + Ok(RefTimeLeft(self.gas_left().ref_time())) } /// Hand over the gas metering responsibility from this meter to the executor. @@ -275,17 +282,17 @@ impl GasMeter { /// This can be different from `gas_spent` because due to `adjust_gas` the amount of /// spent gas can temporarily drop and be refunded later. pub fn gas_required(&self) -> Weight { - self.gas_limit.saturating_sub(self.gas_left_lowest()) + self.gas_consumed_highest.max(self.gas_consumed) } /// Returns how much gas was spent pub fn gas_consumed(&self) -> Weight { - self.gas_limit.saturating_sub(self.gas_left) + self.gas_consumed } /// Returns how much gas left from the initial budget. pub fn gas_left(&self) -> Weight { - self.gas_left + self.gas_limit.saturating_sub(self.gas_consumed) } /// The amount of gas in terms of engine gas. @@ -312,17 +319,13 @@ impl GasMeter { .map_err(|e| DispatchErrorWithPostInfo { post_info, error: e.into().error }) } - fn gas_left_lowest(&self) -> Weight { - self.gas_left_lowest.min(self.gas_left) - } - #[cfg(test)] pub fn tokens(&self) -> &[ErasedToken] { &self.tokens } pub fn consume_all(&mut self) { - self.gas_left = Zero::zero(); + self.gas_consumed = self.gas_limit; } } @@ -419,7 +422,7 @@ mod tests { let mut gas_meter = GasMeter::::new(test_weight); let gas_for_nested_call = gas_meter.nested(10000.into()); - assert_eq!(gas_meter.gas_left(), 40000.into()); + assert_eq!(gas_meter.gas_consumed(), 0.into()); assert_eq!(gas_for_nested_call.gas_left(), 10000.into()) } @@ -429,7 +432,7 @@ mod tests { let mut gas_meter = GasMeter::::new(test_weight); let gas_for_nested_call = gas_meter.nested(test_weight); - assert_eq!(gas_meter.gas_left(), Weight::from_parts(0, 0)); + assert_eq!(gas_meter.gas_consumed(), Weight::from_parts(0, 0)); assert_eq!(gas_for_nested_call.gas_left(), 50_000.into()) } @@ -439,7 +442,7 @@ mod tests { let mut gas_meter = GasMeter::::new(test_weight); let gas_for_nested_call = gas_meter.nested(test_weight + 10000.into()); - assert_eq!(gas_meter.gas_left(), Weight::from_parts(0, 0)); + assert_eq!(gas_meter.gas_consumed(), Weight::from_parts(0, 0)); assert_eq!(gas_for_nested_call.gas_left(), 50_000.into()) } diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 9b1c8fa538411..ba0421f777b40 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -46,10 +46,8 @@ pub mod weights; use crate::{ evm::{ - create_call, - fees::{Combinator, InfoT as FeeInfo}, - runtime::SetWeightLimit, - CallTracer, GenericTransaction, PrestateTracer, Trace, Tracer, TracerType, TYPE_EIP1559, + create_call, fees::InfoT as FeeInfo, runtime::SetWeightLimit, CallTracer, + GenericTransaction, PrestateTracer, Trace, Tracer, TracerType, TYPE_EIP1559, }, exec::{AccountIdOf, ExecError, Executable, Stack as ExecStack}, gas::GasMeter, @@ -82,7 +80,10 @@ use frame_system::{ }; use scale_info::TypeInfo; use sp_runtime::{ - traits::{BadOrigin, Bounded, Convert, Dispatchable, Saturating, UniqueSaturatedInto, Zero}, + traits::{ + BadOrigin, Bounded, Convert, Dispatchable, Saturating, UniqueSaturatedFrom, + UniqueSaturatedInto, Zero, + }, AccountId32, DispatchError, FixedPointNumber, FixedU128, }; @@ -150,7 +151,13 @@ pub mod pallet { /// /// Just added here to add additional trait bounds. #[pallet::no_default] - type Balance: Balance + TryFrom + Into + Bounded + UniqueSaturatedInto; + type Balance: Balance + + TryFrom + + Into + + Bounded + + UniqueSaturatedInto + + UniqueSaturatedFrom + + UniqueSaturatedInto; /// The fungible in which fees are paid and contract balances are held. #[pallet::no_default] @@ -1914,7 +1921,7 @@ impl Pallet { /// Convert a weight to a gas value. fn evm_gas_from_weight(weight: Weight) -> U256 { - T::FeeInfo::weight_to_fee(&weight, Combinator::Max).into() + T::FeeInfo::weight_to_fee(&weight).into() } /// Ensure the origin has no code deplyoyed if it is a signed origin. diff --git a/substrate/frame/revive/src/tests/precompiles.rs b/substrate/frame/revive/src/tests/precompiles.rs index bb1840d5f762c..dbe94f5f60581 100644 --- a/substrate/frame/revive/src/tests/precompiles.rs +++ b/substrate/frame/revive/src/tests/precompiles.rs @@ -18,7 +18,7 @@ //! Precompiles added to the test runtime. use crate::{ - exec::{ErrorOrigin, ExecError}, + exec::{CallResources, ErrorOrigin, ExecError}, precompiles::{AddressMatcher, Error, Ext, ExtWithInfo, Precompile, Token}, Config, DispatchError, ExecOrigin as Origin, Weight, U256, }; @@ -109,8 +109,7 @@ impl Precompile for NoInfo { }, INoInfoCalls::passData(INoInfo::passDataCall { inputLen }) => { env.call( - Weight::MAX, - U256::MAX, + &CallResources::Precise { weight: Weight::MAX, deposit_limit: U256::MAX }, &env.address(), 0.into(), vec![42; *inputLen as usize], diff --git a/substrate/frame/revive/src/tests/pvm.rs b/substrate/frame/revive/src/tests/pvm.rs index 4fa7b0c7167a0..82a28b9c176a9 100644 --- a/substrate/frame/revive/src/tests/pvm.rs +++ b/substrate/frame/revive/src/tests/pvm.rs @@ -40,9 +40,8 @@ use crate::{ }, tracing::trace, weights::WeightInfo, - AccountInfo, AccountInfoOf, BalanceWithDust, Code, Combinator, Config, ContractInfo, - DeletionQueueCounter, Error, ExecConfig, HoldReason, Origin, Pallet, PristineCode, - StorageDeposit, H160, + AccountInfo, AccountInfoOf, BalanceWithDust, Code, Config, ContractInfo, DeletionQueueCounter, + Error, ExecConfig, HoldReason, Origin, Pallet, PristineCode, StorageDeposit, H160, }; use assert_matches::assert_matches; use codec::Encode; @@ -1495,13 +1494,12 @@ fn gas_left_api_works() { let received = builder::bare_call(addr).build_and_unwrap_result(); assert_eq!(received.flags, ReturnFlags::empty()); let gas_left = U256::from_little_endian(received.data.as_ref()); - let gas_left_max = - ::FeeInfo::weight_to_fee(&GAS_LIMIT, Combinator::Min) + 1_000_000; + let gas_left_max = ::FeeInfo::weight_to_fee(&GAS_LIMIT) + 1_000_000; assert!(gas_left > 0u32.into()); assert!(gas_left < gas_left_max.into()); // Call the contract using the hold - let hold_initial = ::FeeInfo::weight_to_fee(&GAS_LIMIT, Combinator::Max); + let hold_initial = ::FeeInfo::weight_to_fee(&GAS_LIMIT); ::FeeInfo::deposit_txfee(::Currency::issue(hold_initial)); let mut exec_config = ExecConfig::new_substrate_tx(); exec_config.collect_deposit_from_hold = Some((0u32.into(), Default::default())); diff --git a/substrate/frame/revive/src/tests/sol/contract.rs b/substrate/frame/revive/src/tests/sol/contract.rs index 3eeef87675236..c0efa89f9e75b 100644 --- a/substrate/frame/revive/src/tests/sol/contract.rs +++ b/substrate/frame/revive/src/tests/sol/contract.rs @@ -18,16 +18,20 @@ //! The pallet-revive shared VM integration test suite. use crate::{ - evm::decode_revert_reason, - test_utils::{builder::Contract, ALICE, ALICE_ADDR}, + evm::{decode_revert_reason, fees::InfoT}, + test_utils::{builder::Contract, deposit_limit, ALICE, ALICE_ADDR, GAS_LIMIT}, tests::{builder, ExtBuilder, Test}, - Code, Config, Error, + BalanceOf, Code, Config, DispatchError, Error, ExecConfig, }; use alloy_core::{ primitives::{Bytes, FixedBytes}, sol_types::{Revert, SolCall, SolError, SolInterface}, }; -use frame_support::{assert_err, traits::fungible::Mutate}; +use frame_support::{ + assert_err, + traits::fungible::{Balanced, Mutate}, +}; +use itertools::Itertools; use pallet_revive_fixtures::{compile_module_with_type, Callee, Caller, FixtureType}; use pretty_assertions::assert_eq; use sp_core::H160; @@ -457,3 +461,123 @@ fn instantiate_from_constructor_works() { assert_eq!(result, 42u64); }); } + +/// No resolc caller since the subcall limiting is not implemented on resolc, yet. +#[test_case(FixtureType::Solc, FixtureType::Solc; "solc->solc")] +#[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] +fn subcall_effectively_limited_substrate_tx(caller_type: FixtureType, callee_type: FixtureType) { + let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); + let (callee_code, _) = compile_module_with_type("Callee", callee_type).unwrap(); + + let no_collection_config = ExecConfig::new_substrate_tx(); + let mut collection_config = ExecConfig::new_substrate_tx(); + collection_config.collect_deposit_from_hold = Some(Default::default()); + let configs = [no_collection_config, collection_config]; + + let call_types = [0u8, 1, 2]; + + struct Case { + deposit_limit: BalanceOf, + gas_divisor: u64, + callee_input: Vec, + result: Result, + is_store_call: bool, + } + + let test_cases = [ + Case { + deposit_limit: deposit_limit::(), + gas_divisor: 1, + callee_input: Callee::consumeAllReftimeCall {}.abi_encode(), + result: Err(>::OutOfGas.into()), + is_store_call: false, + }, + Case { + deposit_limit: deposit_limit::(), + gas_divisor: 2, + callee_input: Callee::consumeAllReftimeCall {}.abi_encode(), + result: Ok(false), + is_store_call: false, + }, + Case { + deposit_limit: deposit_limit::(), + gas_divisor: u64::MAX, + callee_input: Callee::consumeAllReftimeCall {}.abi_encode(), + result: Ok(false), + is_store_call: false, + }, + Case { + deposit_limit: 130, + gas_divisor: 1, + callee_input: Callee::storeCall { _data: 42 }.abi_encode(), + result: Err(>::StorageDepositLimitExhausted.into()), + is_store_call: true, + }, + Case { + deposit_limit: 130, + gas_divisor: 2, + callee_input: Callee::storeCall { _data: 42 }.abi_encode(), + result: Ok(false), + is_store_call: true, + }, + Case { + deposit_limit: 130, + gas_divisor: u64::MAX, + callee_input: Callee::storeCall { _data: 42 }.abi_encode(), + result: Ok(false), + is_store_call: true, + }, + Case { + deposit_limit: deposit_limit::(), + gas_divisor: 2, + callee_input: Callee::storeCall { _data: 42 }.abi_encode(), + result: Ok(true), + is_store_call: true, + }, + ]; + + for ((case, config), call_type) in + test_cases.iter().cartesian_product(configs).cartesian_product(call_types) + { + // the storage stuff won't work on static or delegate call + if case.is_store_call && call_type != 0 { + continue + } + + ExtBuilder::default().build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); + let fees = + ::FeeInfo::tx_fee_from_weight(0, &GAS_LIMIT) + case.deposit_limit; + ::FeeInfo::deposit_txfee(::Currency::issue(fees)); + + // Instantiate the callee contract, which can echo a value. + let Contract { addr: callee_addr, .. } = + builder::bare_instantiate(Code::Upload(callee_code.clone())) + .build_and_unwrap_contract(); + + // Instantiate the caller contract. + let Contract { addr: caller_addr, .. } = + builder::bare_instantiate(Code::Upload(caller_code.clone())) + .build_and_unwrap_contract(); + + let output = builder::bare_call(caller_addr) + .data( + Caller::callPartialGasCall { + _callee: callee_addr.0.into(), + _data: case.callee_input.clone().into(), + _gasDivisor: case.gas_divisor, + _callType: call_type, + } + .abi_encode(), + ) + .exec_config(config) + .storage_deposit_limit(case.deposit_limit) + .build(); + + let result = output.result.map(|result| { + Caller::callPartialGasCall::abi_decode_returns(&result.data).unwrap() + }); + assert_eq!(case.result, result); + }); + } +} diff --git a/substrate/frame/revive/src/tests/sol/system.rs b/substrate/frame/revive/src/tests/sol/system.rs index 3e571bd85b5a7..078f55f64c592 100644 --- a/substrate/frame/revive/src/tests/sol/system.rs +++ b/substrate/frame/revive/src/tests/sol/system.rs @@ -21,7 +21,7 @@ use crate::{ evm::fees::InfoT, test_utils::{builder::Contract, ALICE, ALICE_ADDR, GAS_LIMIT}, tests::{builder, Contracts, ExtBuilder, Test}, - Code, Combinator, Config, ExecConfig, U256, + Code, Config, ExecConfig, U256, }; use alloy_core::sol_types::SolCall; use frame_support::traits::fungible::{Balanced, Mutate}; @@ -213,7 +213,7 @@ fn gas_works(fixture_type: FixtureType) { builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract(); // enable txhold collection which we expect to be on when using the evm backend - let hold_initial = ::FeeInfo::weight_to_fee(&GAS_LIMIT, Combinator::Max); + let hold_initial = ::FeeInfo::weight_to_fee(&GAS_LIMIT); ::FeeInfo::deposit_txfee(::Currency::issue(hold_initial)); let mut exec_config = ExecConfig::new_substrate_tx(); exec_config.collect_deposit_from_hold = Some((0u32.into(), Default::default())); diff --git a/substrate/frame/revive/src/vm/evm/instructions/contract.rs b/substrate/frame/revive/src/vm/evm/instructions/contract.rs index 421f592b8462f..d93806d564d4c 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/contract.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/contract.rs @@ -19,6 +19,7 @@ mod call_helpers; use super::utility::IntoAddress; use crate::{ + exec::CallResources, vm::{ evm::{interpreter::Halt, util::as_usize_or_halt, Interpreter}, Ext, RuntimeCosts, @@ -26,7 +27,7 @@ use crate::{ Code, Error, Pallet, Weight, H160, LOG_TARGET, U256, }; use alloc::{vec, vec::Vec}; -pub use call_helpers::{calc_call_gas, get_memory_in_and_out_ranges}; +pub use call_helpers::{charge_call_gas, get_memory_in_and_out_ranges}; use core::{ cmp::min, ops::{ControlFlow, Range}, @@ -46,9 +47,6 @@ pub fn create( let [value, code_offset, len] = interpreter.stack.popn()?; let len = as_usize_or_halt::(len)?; - // TODO: We do not charge for the new code in storage. When implementing the new gas: - // Introduce EthInstantiateWithCode, which shall charge gas based on the code length. - // See #9577 for more context. interpreter.ext.charge_or_halt(RuntimeCosts::Instantiate { input_data_len: len as u32, // We charge for initcode execution balance_transfer: Pallet::::has_balance(value), @@ -75,7 +73,7 @@ pub fn create( }; let call_result = interpreter.ext.instantiate( - Weight::from_parts(u64::MAX, u64::MAX), // TODO: set the right limit + Weight::from_parts(u64::MAX, u64::MAX), U256::MAX, Code::Upload(code), value, @@ -107,26 +105,22 @@ pub fn create( /// /// Message call with value transfer to another account. pub fn call(interpreter: &mut Interpreter) -> ControlFlow { - let [_local_gas_limit, to, value] = interpreter.stack.popn()?; + let [gas_limit, to, value] = interpreter.stack.popn()?; let to = to.into_address(); - // TODO: Max gas limit is not possible in a real Ethereum situation. This issue will be - // addressed in #9577. - let has_transfer = !value.is_zero(); if interpreter.ext.is_read_only() && has_transfer { return ControlFlow::Break(Error::::StateChangeDenied.into()); } - let (input, return_memory_range) = get_memory_in_and_out_ranges(interpreter)?; let scheme = CallScheme::Call; - let gas_limit = calc_call_gas(interpreter, to, scheme, input.len(), value)?; + charge_call_gas(interpreter, to, scheme, input.len(), value)?; run_call( interpreter, to, + gas_limit, interpreter.memory.slice(input).to_vec(), scheme, - Weight::from_parts(gas_limit, u64::MAX), value, return_memory_range, ) @@ -146,22 +140,19 @@ pub fn call_code(_interpreter: &mut Interpreter) -> ControlFlow /// /// Message call with alternative account's code but same sender and value. pub fn delegate_call(interpreter: &mut Interpreter) -> ControlFlow { - let [_local_gas_limit, to] = interpreter.stack.popn()?; + let [gas_limit, to] = interpreter.stack.popn()?; let to = to.into_address(); - // TODO: Max gas limit is not possible in a real Ethereum situation. This issue will be - // addressed in #9577. - let (input, return_memory_range) = get_memory_in_and_out_ranges(interpreter)?; let scheme = CallScheme::DelegateCall; let value = U256::zero(); - let gas_limit = calc_call_gas(interpreter, to, scheme, input.len(), value)?; + charge_call_gas(interpreter, to, scheme, input.len(), value)?; run_call( interpreter, to, + gas_limit, interpreter.memory.slice(input).to_vec(), scheme, - Weight::from_parts(gas_limit, u64::MAX), value, return_memory_range, ) @@ -171,21 +162,19 @@ pub fn delegate_call(interpreter: &mut Interpreter) -> ControlFlow(interpreter: &mut Interpreter) -> ControlFlow { - let [_local_gas_limit, to] = interpreter.stack.popn()?; + let [gas_limit, to] = interpreter.stack.popn()?; let to = to.into_address(); - // TODO: Max gas limit is not possible in a real Ethereum situation. This issue will be - // addressed in #9577. let (input, return_memory_range) = get_memory_in_and_out_ranges(interpreter)?; let scheme = CallScheme::StaticCall; let value = U256::zero(); - let gas_limit = calc_call_gas(interpreter, to, scheme, input.len(), value)?; + charge_call_gas(interpreter, to, scheme, input.len(), value)?; run_call( interpreter, to, + gas_limit, interpreter.memory.slice(input).to_vec(), scheme, - Weight::from_parts(gas_limit, u64::MAX), value, return_memory_range, ) @@ -194,16 +183,15 @@ pub fn static_call(interpreter: &mut Interpreter) -> ControlFlow( interpreter: &mut Interpreter<'a, E>, callee: H160, + gas_limit: U256, input: Vec, scheme: CallScheme, - gas_limit: Weight, value: U256, return_memory_range: Range, ) -> ControlFlow { let call_result = match scheme { CallScheme::Call | CallScheme::StaticCall => interpreter.ext.call( - gas_limit, - U256::MAX, + &CallResources::Ethereum(gas_limit), &callee, value, input, @@ -211,7 +199,9 @@ fn run_call<'a, E: Ext>( scheme.is_static_call(), ), CallScheme::DelegateCall => - interpreter.ext.delegate_call(gas_limit, U256::MAX, callee, input), + interpreter + .ext + .delegate_call(&CallResources::Ethereum(gas_limit), callee, input), CallScheme::CallCode => { unreachable!() }, diff --git a/substrate/frame/revive/src/vm/evm/instructions/contract/call_helpers.rs b/substrate/frame/revive/src/vm/evm/instructions/contract/call_helpers.rs index 9796006882846..4b6cfbc990f72 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/contract/call_helpers.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/contract/call_helpers.rs @@ -56,13 +56,13 @@ pub fn resize_memory<'a, E: Ext>( } /// Calculates gas cost and limit for call instructions. -pub fn calc_call_gas<'a, E: Ext>( +pub fn charge_call_gas<'a, E: Ext>( interpreter: &mut Interpreter<'a, E>, callee: H160, scheme: CallScheme, input_len: usize, value: U256, -) -> ControlFlow { +) -> ControlFlow { let precompile = >::get::(&callee.as_fixed_bytes()); match precompile { @@ -106,5 +106,5 @@ pub fn calc_call_gas<'a, E: Ext>( })?; } - ControlFlow::Continue(u64::MAX) // TODO: Set the right gas limit + ControlFlow::Continue(()) } diff --git a/substrate/frame/revive/src/vm/evm/instructions/system.rs b/substrate/frame/revive/src/vm/evm/instructions/system.rs index 411ed142e07f5..a112fc35b569e 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/system.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/system.rs @@ -28,7 +28,6 @@ use core::ops::ControlFlow; use revm::interpreter::gas::{BASE, VERYLOW}; use sp_core::H256; use sp_io::hashing::keccak_256; -// TODO: Fix the gas handling for the memory operations /// The Keccak-256 hash of the empty string `""`. pub const KECCAK_EMPTY: [u8; 32] = diff --git a/substrate/frame/revive/src/vm/pvm.rs b/substrate/frame/revive/src/vm/pvm.rs index 089479bfda88d..63813ab8609a9 100644 --- a/substrate/frame/revive/src/vm/pvm.rs +++ b/substrate/frame/revive/src/vm/pvm.rs @@ -23,7 +23,7 @@ pub mod env; pub use env::SyscallDoc; use crate::{ - exec::{ExecError, ExecResult, Ext, Key}, + exec::{CallResources, ExecError, ExecResult, Ext, Key}, gas::ChargedAmount, limits, precompiles::{All as AllPrecompiles, Precompiles}, @@ -687,8 +687,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { })?; } self.ext.call( - weight, - deposit_limit, + &CallResources::Precise { weight, deposit_limit }, &callee, value, input_data, @@ -700,7 +699,11 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { if flags.intersects(CallFlags::ALLOW_REENTRY | CallFlags::READ_ONLY) { return Err(Error::::InvalidCallFlags.into()); } - self.ext.delegate_call(weight, deposit_limit, callee, input_data) + self.ext.delegate_call( + &CallResources::Precise { weight, deposit_limit }, + callee, + input_data, + ) }, };