Skip to content

Commit

Permalink
contracts: Don't fail fast if the Weight limit of a cross contract …
Browse files Browse the repository at this point in the history 
…call is too big (paritytech#3243)

When doing a cross contract call you can supply an optional Weight limit
for that call. If one doesn't specify the limit (setting it to 0) the
sub call will have all the remaining gas available. If one does specify
the limit we subtract that amount eagerly from the Weight meter and fail
fast if not enough `Weight` is available.

This is quite annoying because setting a fixed limit will set the
`gas_required` in the gas estimation according to the specified limit.
Even if in that dry-run the actual call didn't consume that whole
amount. It effectively discards the more precise measurement it should
have from the dry-run.

This PR changes the behaviour so that the supplied limit is an actual
limit: We do the cross contract call even if the limit is higher than
the remaining `Weight`. We then fail and roll back in the cub call in
case there is not enough weight.

This makes the weight estimation in the dry-run no longer dependent on
the weight limit supplied when doing a cross contract call.

---------

Co-authored-by: PG Herveou <[email protected]>
  • Loading branch information
@athei @pgherveou
athei and pgherveou authored Feb 8, 2024
1 parent 5b22c03 commit 1e3de51
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 103 deletions.
4 changes: 3 additions & 1 deletion substrate/frame/contracts/fixtures/contracts/call_with_limit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@ pub extern "C" fn deploy() {}
#[polkavm_derive::polkavm_export]
pub extern "C" fn call() {
input!(
256,
callee_addr: [u8; 32],
ref_time: u64,
proof_size: u64,
forwarded_input: [u8],
);

#[allow(deprecated)]
Expand All @@ -44,7 +46,7 @@ pub extern "C" fn call() {
proof_size,
None, // No deposit limit.
&0u64.to_le_bytes(), // value transferred to the contract.
&[0u8; 0], // input data.
forwarded_input,
None,
)
.unwrap();
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/contracts/src/exec.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ where
contract_info: CachedContract::Cached(contract_info),
account_id,
entry_point,
nested_gas: gas_meter.nested(gas_limit)?,
nested_gas: gas_meter.nested(gas_limit),
nested_storage: storage_meter.nested(deposit_limit),
allows_reentry: true,
};
Expand Down
34 changes: 8 additions & 26 deletions substrate/frame/contracts/src/gas.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,7 @@ impl<T: Config> GasMeter<T> {
/// # Note
///
/// Passing `0` as amount is interpreted as "all remaining gas".
pub fn nested(&mut self, amount: Weight) -> Result<Self, DispatchError> {
// NOTE that it is ok to allocate all available gas since it still ensured
// by `charge` that it doesn't reach zero.
pub fn nested(&mut self, amount: Weight) -> Self {
let amount = Weight::from_parts(
if amount.ref_time().is_zero() {
self.gas_left().ref_time()
Expand All @@ -118,33 +116,17 @@ impl<T: Config> GasMeter<T> {
} else {
amount.proof_size()
},
);
self.gas_left = self.gas_left.checked_sub(&amount).ok_or_else(|| <Error<T>>::OutOfGas)?;
Ok(GasMeter::new(amount))
)
.min(self.gas_left);
self.gas_left -= amount;
GasMeter::new(amount)
}

/// Absorb the remaining gas of a nested meter after we are done using it.
pub fn absorb_nested(&mut self, nested: Self) {
if self.gas_left.ref_time().is_zero() {
// All of the remaining gas was inherited by the nested gas meter. When absorbing
// we can therefore safely inherit the lowest gas that the nested gas meter experienced
// as long as it is lower than the lowest gas that was experienced by the parent.
// We cannot call `self.gas_left_lowest()` here because in the state that this
// code is run the parent gas meter has `0` gas left.
*self.gas_left_lowest.ref_time_mut() =
nested.gas_left_lowest().ref_time().min(self.gas_left_lowest.ref_time());
} else {
// The nested gas meter was created with a fixed amount that did not consume all of the
// parents (self) gas. The lowest gas that self will experience is when the nested
// gas was pre charged with the fixed amount.
*self.gas_left_lowest.ref_time_mut() = self.gas_left_lowest().ref_time();
}
if self.gas_left.proof_size().is_zero() {
*self.gas_left_lowest.proof_size_mut() =
nested.gas_left_lowest().proof_size().min(self.gas_left_lowest.proof_size());
} else {
*self.gas_left_lowest.proof_size_mut() = self.gas_left_lowest().proof_size();
}
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;
}

Expand Down
203 changes: 128 additions & 75 deletions substrate/frame/contracts/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use crate::{
MigrationInProgress, Origin, Pallet, PristineCode, Schedule,
};
use assert_matches::assert_matches;
use codec::Encode;
use codec::{Decode, Encode};
use frame_support::{
assert_err, assert_err_ignore_postinfo, assert_err_with_weight, assert_noop, assert_ok,
derive_impl,
Expand Down Expand Up @@ -217,8 +217,6 @@ impl ChainExtension<Test> for TestExtension {
where
E: Ext<T = Test>,
{
use codec::Decode;

let func_id = env.func_id();
let id = env.ext_id() as u32 | func_id as u32;
match func_id {
Expand Down Expand Up @@ -2924,12 +2922,13 @@ fn debug_message_invalid_utf8() {
}

#[test]
fn gas_estimation_nested_call_fixed_limit() {
fn gas_estimation_for_subcalls() {
let (caller_code, _caller_hash) = compile_module::<Test>("call_with_limit").unwrap();
let (callee_code, _callee_hash) = compile_module::<Test>("dummy").unwrap();
let (call_runtime_code, _caller_hash) = compile_module::<Test>("call_runtime").unwrap();
let (dummy_code, _callee_hash) = compile_module::<Test>("dummy").unwrap();
ExtBuilder::default().existential_deposit(50).build().execute_with(|| {
let min_balance = Contracts::min_balance();
let _ = <Test as Config>::Currency::set_balance(&ALICE, 1000 * min_balance);
let _ = <Test as Config>::Currency::set_balance(&ALICE, 2_000 * min_balance);

let addr_caller = Contracts::bare_instantiate(
ALICE,
Expand All @@ -2938,91 +2937,159 @@ fn gas_estimation_nested_call_fixed_limit() {
None,
Code::Upload(caller_code),
vec![],
vec![0],
vec![],
DebugInfo::Skip,
CollectEvents::Skip,
)
.result
.unwrap()
.account_id;

let addr_callee = Contracts::bare_instantiate(
let addr_dummy = Contracts::bare_instantiate(
ALICE,
min_balance * 100,
GAS_LIMIT,
None,
Code::Upload(callee_code),
Code::Upload(dummy_code),
vec![],
vec![],
vec![1],
DebugInfo::Skip,
CollectEvents::Skip,
)
.result
.unwrap()
.account_id;

let input: Vec<u8> = AsRef::<[u8]>::as_ref(&addr_callee)
.iter()
.cloned()
.chain((GAS_LIMIT / 5).ref_time().to_le_bytes())
.chain((GAS_LIMIT / 5).proof_size().to_le_bytes())
.collect();

// Call in order to determine the gas that is required for this call
let result = Contracts::bare_call(
let addr_call_runtime = Contracts::bare_instantiate(
ALICE,
addr_caller.clone(),
0,
min_balance * 100,
GAS_LIMIT,
None,
input.clone(),
Code::Upload(call_runtime_code),
vec![],
vec![],
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
);
assert_ok!(&result.result);
)
.result
.unwrap()
.account_id;

// We have a subcall with a fixed gas limit. This constitutes precharging.
assert!(result.gas_required.all_gt(result.gas_consumed));
// Run the test for all of those weight limits for the subcall
let weights = [
Weight::zero(),
GAS_LIMIT,
GAS_LIMIT * 2,
GAS_LIMIT / 5,
Weight::from_parts(0, GAS_LIMIT.proof_size()),
Weight::from_parts(GAS_LIMIT.ref_time(), 0),
];

// Make the same call using the estimated gas. Should succeed.
assert_ok!(
Contracts::bare_call(
ALICE,
addr_caller.clone(),
0,
result.gas_required,
Some(result.storage_deposit.charge_or_zero()),
input.clone(),
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
)
.result
);
// This call is passed to the sub call in order to create a large `required_weight`
let runtime_call = RuntimeCall::Dummy(pallet_dummy::Call::overestimate_pre_charge {
pre_charge: Weight::from_parts(10_000_000_000, 512 * 1024),
actual_weight: Weight::from_parts(1, 1),
})
.encode();

// Make the same call using proof_size but less than estimated. Should fail with OutOfGas.
let result = Contracts::bare_call(
ALICE,
addr_caller,
0,
result.gas_required.sub_proof_size(1),
Some(result.storage_deposit.charge_or_zero()),
input,
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
)
.result;
assert_err!(result, <Error<Test>>::OutOfGas);
// Encodes which contract should be sub called with which input
let sub_calls: [(&[u8], Vec<_>, bool); 2] = [
(addr_dummy.as_ref(), vec![], false),
(addr_call_runtime.as_ref(), runtime_call, true),
];

for weight in weights {
for (sub_addr, sub_input, out_of_gas_in_subcall) in &sub_calls {
let input: Vec<u8> = sub_addr
.iter()
.cloned()
.chain(weight.ref_time().to_le_bytes())
.chain(weight.proof_size().to_le_bytes())
.chain(sub_input.clone())
.collect();

// Call in order to determine the gas that is required for this call
let result = Contracts::bare_call(
ALICE,
addr_caller.clone(),
0,
GAS_LIMIT,
None,
input.clone(),
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
);
assert_ok!(&result.result);

// If the out of gas happens in the subcall the caller contract
// will just trap. Otherwise we would need to forward an error
// code to signal that the sub contract ran out of gas.
let error: DispatchError = if *out_of_gas_in_subcall {
assert!(result.gas_required.all_gt(result.gas_consumed));
<Error<Test>>::ContractTrapped.into()
} else {
assert_eq!(result.gas_required, result.gas_consumed);
<Error<Test>>::OutOfGas.into()
};

// Make the same call using the estimated gas. Should succeed.
assert_ok!(
Contracts::bare_call(
ALICE,
addr_caller.clone(),
0,
result.gas_required,
Some(result.storage_deposit.charge_or_zero()),
input.clone(),
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
)
.result
);

// Check that it fails with too little ref_time
assert_err!(
Contracts::bare_call(
ALICE,
addr_caller.clone(),
0,
result.gas_required.sub_ref_time(1),
Some(result.storage_deposit.charge_or_zero()),
input.clone(),
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
)
.result,
error,
);

// Check that it fails with too little proof_size
assert_err!(
Contracts::bare_call(
ALICE,
addr_caller.clone(),
0,
result.gas_required.sub_proof_size(1),
Some(result.storage_deposit.charge_or_zero()),
input,
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
)
.result,
error,
);
}
}
});
}

#[test]
fn gas_estimation_call_runtime() {
use codec::Decode;
let (caller_code, _caller_hash) = compile_module::<Test>("call_runtime").unwrap();
let (callee_code, _callee_hash) = compile_module::<Test>("dummy").unwrap();
ExtBuilder::default().existential_deposit(50).build().execute_with(|| {
let min_balance = Contracts::min_balance();
let _ = <Test as Config>::Currency::set_balance(&ALICE, 1000 * min_balance);
Expand All @@ -3043,25 +3110,11 @@ fn gas_estimation_call_runtime() {
.unwrap()
.account_id;

Contracts::bare_instantiate(
ALICE,
min_balance * 100,
GAS_LIMIT,
None,
Code::Upload(callee_code),
vec![],
vec![1],
DebugInfo::Skip,
CollectEvents::Skip,
)
.result
.unwrap();

// Call something trivial with a huge gas limit so that we can observe the effects
// of pre-charging. This should create a difference between consumed and required.
let call = RuntimeCall::Dummy(pallet_dummy::Call::overestimate_pre_charge {
pre_charge: Weight::from_parts(10_000_000, 0),
actual_weight: Weight::from_parts(100, 0),
pre_charge: Weight::from_parts(10_000_000, 1_000),
actual_weight: Weight::from_parts(100, 100),
});
let result = Contracts::bare_call(
ALICE,
Expand All @@ -3077,7 +3130,7 @@ fn gas_estimation_call_runtime() {
// contract encodes the result of the dispatch runtime
let outcome = u32::decode(&mut result.result.unwrap().data.as_ref()).unwrap();
assert_eq!(outcome, 0);
assert!(result.gas_required.ref_time() > result.gas_consumed.ref_time());
assert!(result.gas_required.all_gt(result.gas_consumed));

// Make the same call using the required gas. Should succeed.
assert_ok!(
Expand Down

0 comments on commit 1e3de51

Please sign in to comment.