Skip to content

Commit

Permalink
Contracts: xcm host fn fixes (#3086)
Browse files Browse the repository at this point in the history
## Xcm changes:
- Fix `pallet_xcm::execute`, move the logic into The `ExecuteController`
so it can be shared with anything that implement that trait.
- Make `ExecuteController::execute` retursn `DispatchErrorWithPostInfo`
instead of `DispatchError`, so that we don't charge the full
`max_weight` provided if the execution is incomplete (useful for
force_batch or contracts calls)
- Fix docstring for `pallet_xcm::execute`, to reflect the changes from
#2405
- Update the signature for `ExecuteController::execute`, we don't need
to return the `Outcome` anymore since we only care about
`Outcome::Complete`

## Contracts changes:

- Update host fn `xcm_exexute`, we don't need to write the `Outcome` to
the sandbox memory anymore. This was also not charged as well before so
it if fixes this too.
- One of the issue was that the dry_run of a contract that call
`xcm_execute` would exhaust the `gas_limit`.

This is because `XcmExecuteController::execute` takes a `max_weight`
argument, and since we don't want the user to specify it manually we
were passing everything left by pre-charghing
`ctx.ext.gas_meter().gas_left()`

- To fix it I added a `fn influence_lowest_limit` on the `Token` trait
and make it return false for `RuntimeCost::XcmExecute`.
- Got rid of the `RuntimeToken` indirection, we can just use
`RuntimeCost` directly.

---------

Co-authored-by: command-bot <>
  • Loading branch information
pgherveou authored Feb 19, 2024
1 parent 320863a commit ca382f3
Show file tree
Hide file tree
Showing 13 changed files with 192 additions and 133 deletions.
60 changes: 34 additions & 26 deletions polkadot/xcm/pallet-xcm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub mod migration;

use codec::{Decode, Encode, EncodeLike, MaxEncodedLen};
use frame_support::{
dispatch::GetDispatchInfo,
dispatch::{DispatchErrorWithPostInfo, GetDispatchInfo, WithPostDispatchInfo},
pallet_prelude::*,
traits::{
Contains, ContainsPair, Currency, Defensive, EnsureOrigin, Get, LockableCurrency,
Expand Down Expand Up @@ -291,22 +291,38 @@ pub mod pallet {
origin: OriginFor<T>,
message: Box<VersionedXcm<<T as Config>::RuntimeCall>>,
max_weight: Weight,
) -> Result<Outcome, DispatchError> {
let origin_location = T::ExecuteXcmOrigin::ensure_origin(origin)?;
let mut hash = message.using_encoded(sp_io::hashing::blake2_256);
let message = (*message).try_into().map_err(|()| Error::<T>::BadVersion)?;
let value = (origin_location, message);
ensure!(T::XcmExecuteFilter::contains(&value), Error::<T>::Filtered);
let (origin_location, message) = value;
let outcome = T::XcmExecutor::prepare_and_execute(
origin_location,
message,
&mut hash,
max_weight,
max_weight,
);
) -> Result<Weight, DispatchErrorWithPostInfo> {
log::trace!(target: "xcm::pallet_xcm::execute", "message {:?}, max_weight {:?}", message, max_weight);
let outcome = (|| {
let origin_location = T::ExecuteXcmOrigin::ensure_origin(origin)?;
let mut hash = message.using_encoded(sp_io::hashing::blake2_256);
let message = (*message).try_into().map_err(|()| Error::<T>::BadVersion)?;
let value = (origin_location, message);
ensure!(T::XcmExecuteFilter::contains(&value), Error::<T>::Filtered);
let (origin_location, message) = value;
Ok(T::XcmExecutor::prepare_and_execute(
origin_location,
message,
&mut hash,
max_weight,
max_weight,
))
})()
.map_err(|e: DispatchError| {
e.with_weight(<Self::WeightInfo as ExecuteControllerWeightInfo>::execute())
})?;

Self::deposit_event(Event::Attempted { outcome: outcome.clone() });
Ok(outcome)
let weight_used = outcome.weight_used();
outcome.ensure_complete().map_err(|error| {
log::error!(target: "xcm::pallet_xcm::execute", "XCM execution failed with error {:?}", error);
Error::<T>::LocalExecutionIncomplete.with_weight(
weight_used.saturating_add(
<Self::WeightInfo as ExecuteControllerWeightInfo>::execute(),
),
)
})?;
Ok(weight_used)
}
}

Expand Down Expand Up @@ -1009,23 +1025,15 @@ pub mod pallet {
/// No more than `max_weight` will be used in its attempted execution. If this is less than
/// the maximum amount of weight that the message could take to be executed, then no
/// execution attempt will be made.
///
/// NOTE: A successful return to this does *not* imply that the `msg` was executed
/// successfully to completion; only that it was attempted.
#[pallet::call_index(3)]
#[pallet::weight(max_weight.saturating_add(T::WeightInfo::execute()))]
pub fn execute(
origin: OriginFor<T>,
message: Box<VersionedXcm<<T as Config>::RuntimeCall>>,
max_weight: Weight,
) -> DispatchResultWithPostInfo {
log::trace!(target: "xcm::pallet_xcm::execute", "message {:?}, max_weight {:?}", message, max_weight);
let outcome = <Self as ExecuteController<_, _>>::execute(origin, message, max_weight)?;
let weight_used = outcome.weight_used();
outcome.ensure_complete().map_err(|error| {
log::error!(target: "xcm::pallet_xcm::execute", "XCM execution failed with error {:?}", error);
Error::<T>::LocalExecutionIncomplete
})?;
let weight_used =
<Self as ExecuteController<_, _>>::execute(origin, message, max_weight)?;
Ok(Some(weight_used.saturating_add(T::WeightInfo::execute())).into())
}

Expand Down
38 changes: 21 additions & 17 deletions polkadot/xcm/pallet-xcm/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ pub(crate) mod assets_transfer;

use crate::{
mock::*, pallet::SupportedVersion, AssetTraps, Config, CurrentMigration, Error,
LatestVersionedLocation, Pallet, Queries, QueryStatus, VersionDiscoveryQueue,
VersionMigrationStage, VersionNotifiers, VersionNotifyTargets, WeightInfo,
ExecuteControllerWeightInfo, LatestVersionedLocation, Pallet, Queries, QueryStatus,
VersionDiscoveryQueue, VersionMigrationStage, VersionNotifiers, VersionNotifyTargets,
WeightInfo,
};
use frame_support::{
assert_noop, assert_ok,
assert_err_ignore_postinfo, assert_noop, assert_ok,
traits::{Currency, Hooks},
weights::Weight,
};
Expand Down Expand Up @@ -450,19 +451,19 @@ fn trapped_assets_can_be_claimed() {
assert_eq!(Balances::total_balance(&BOB), INITIAL_BALANCE + SEND_AMOUNT);
assert_eq!(AssetTraps::<Test>::iter().collect::<Vec<_>>(), vec![]);

let weight = BaseXcmWeight::get() * 3;
assert_ok!(<XcmPallet as xcm_builder::ExecuteController<_, _>>::execute(
RuntimeOrigin::signed(ALICE),
Box::new(VersionedXcm::from(Xcm(vec![
ClaimAsset { assets: (Here, SEND_AMOUNT).into(), ticket: Here.into() },
buy_execution((Here, SEND_AMOUNT)),
DepositAsset { assets: AllCounted(1).into(), beneficiary: dest },
]))),
weight
));
let outcome =
Outcome::Incomplete { used: BaseXcmWeight::get(), error: XcmError::UnknownClaim };
assert_eq!(last_event(), RuntimeEvent::XcmPallet(crate::Event::Attempted { outcome }));
// Can't claim twice.
assert_err_ignore_postinfo!(
XcmPallet::execute(
RuntimeOrigin::signed(ALICE),
Box::new(VersionedXcm::from(Xcm(vec![
ClaimAsset { assets: (Here, SEND_AMOUNT).into(), ticket: Here.into() },
buy_execution((Here, SEND_AMOUNT)),
DepositAsset { assets: AllCounted(1).into(), beneficiary: dest },
]))),
weight
),
Error::<Test>::LocalExecutionIncomplete
);
});
}

Expand Down Expand Up @@ -495,11 +496,14 @@ fn incomplete_execute_reverts_side_effects() {
// all effects are reverted and balances unchanged for either sender or receiver
assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE);
assert_eq!(Balances::total_balance(&BOB), INITIAL_BALANCE);

assert_eq!(
result,
Err(sp_runtime::DispatchErrorWithPostInfo {
post_info: frame_support::dispatch::PostDispatchInfo {
actual_weight: None,
actual_weight: Some(
<Pallet<Test> as ExecuteControllerWeightInfo>::execute() + weight
),
pays_fee: frame_support::dispatch::Pays::Yes,
},
error: sp_runtime::DispatchError::Module(sp_runtime::ModuleError {
Expand Down
15 changes: 10 additions & 5 deletions polkadot/xcm/xcm-builder/src/controller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@
//! Controller traits defined in this module are high-level traits that will rely on other traits
//! from `xcm-executor` to perform their tasks.
use frame_support::pallet_prelude::DispatchError;
use frame_support::{
dispatch::{DispatchErrorWithPostInfo, WithPostDispatchInfo},
pallet_prelude::DispatchError,
};
use sp_std::boxed::Box;
use xcm::prelude::*;
pub use xcm_executor::traits::QueryHandler;
Expand Down Expand Up @@ -52,7 +55,8 @@ pub trait ExecuteController<Origin, RuntimeCall> {
/// Weight information for ExecuteController functions.
type WeightInfo: ExecuteControllerWeightInfo;

/// Attempt to execute an XCM locally, and return the outcome.
/// Attempt to execute an XCM locally, returns Ok with the weight consumed if the execution
/// complete successfully, Err otherwise.
///
/// # Parameters
///
Expand All @@ -63,7 +67,7 @@ pub trait ExecuteController<Origin, RuntimeCall> {
origin: Origin,
message: Box<VersionedXcm<RuntimeCall>>,
max_weight: Weight,
) -> Result<Outcome, DispatchError>;
) -> Result<Weight, DispatchErrorWithPostInfo>;
}

/// Weight functions needed for [`SendController`].
Expand Down Expand Up @@ -137,8 +141,9 @@ impl<Origin, RuntimeCall> ExecuteController<Origin, RuntimeCall> for () {
_origin: Origin,
_message: Box<VersionedXcm<RuntimeCall>>,
_max_weight: Weight,
) -> Result<Outcome, DispatchError> {
Ok(Outcome::Error { error: XcmError::Unimplemented })
) -> Result<Weight, DispatchErrorWithPostInfo> {
Err(DispatchError::Other("ExecuteController::execute not implemented")
.with_weight(Weight::zero()))
}
}

Expand Down
11 changes: 6 additions & 5 deletions substrate/frame/contracts/fixtures/contracts/xcm_execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ pub extern "C" fn deploy() {}
pub extern "C" fn call() {
input!(512, msg: [u8],);

let mut outcome = [0u8; 512];
let outcome = &mut &mut outcome[..];

#[allow(deprecated)]
api::xcm_execute(msg, outcome).unwrap();
api::return_value(uapi::ReturnFlags::empty(), outcome);
let err_code = match api::xcm_execute(msg) {
Ok(_) => 0u32,
Err(code) => code as u32,
};

api::return_value(uapi::ReturnFlags::empty(), &err_code.to_le_bytes());
}
3 changes: 2 additions & 1 deletion substrate/frame/contracts/mock-network/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ use crate::primitives::{AccountId, UNITS};
use sp_runtime::BuildStorage;
use xcm::latest::prelude::*;
use xcm_executor::traits::ConvertLocation;
use xcm_simulator::{decl_test_network, decl_test_parachain, decl_test_relay_chain, TestExt};
pub use xcm_simulator::TestExt;
use xcm_simulator::{decl_test_network, decl_test_parachain, decl_test_relay_chain};

// Accounts
pub const ADMIN: sp_runtime::AccountId32 = sp_runtime::AccountId32::new([0u8; 32]);
Expand Down
71 changes: 54 additions & 17 deletions substrate/frame/contracts/mock-network/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ use crate::{
primitives::{AccountId, CENTS},
relay_chain, MockNet, ParaA, ParachainBalances, Relay, ALICE, BOB, INITIAL_BALANCE,
};
use assert_matches::assert_matches;
use codec::{Decode, Encode};
use frame_support::{
assert_err,
Expand All @@ -31,11 +30,18 @@ use frame_support::{
use pallet_balances::{BalanceLock, Reasons};
use pallet_contracts::{Code, CollectEvents, DebugInfo, Determinism};
use pallet_contracts_fixtures::compile_module;
use pallet_contracts_uapi::ReturnErrorCode;
use xcm::{v4::prelude::*, VersionedLocation, VersionedXcm};
use xcm_simulator::TestExt;

type ParachainContracts = pallet_contracts::Pallet<parachain::Runtime>;

macro_rules! assert_return_code {
( $x:expr , $y:expr $(,)? ) => {{
assert_eq!(u32::from_le_bytes($x.data[..].try_into().unwrap()), $y as u32);
}};
}

/// Instantiate the tests contract, and fund it with some balance and assets.
fn instantiate_test_contract(name: &str) -> AccountId {
let (wasm, _) = compile_module::<Runtime>(name).unwrap();
Expand Down Expand Up @@ -100,19 +106,57 @@ fn test_xcm_execute() {
DebugInfo::UnsafeDebug,
CollectEvents::UnsafeCollect,
Determinism::Enforced,
)
.result
.unwrap();
);

let mut data = &result.data[..];
let outcome = Outcome::decode(&mut data).expect("Failed to decode xcm_execute Outcome");
assert_matches!(outcome, Outcome::Complete { .. });
assert_eq!(result.gas_consumed, result.gas_required);
assert_return_code!(&result.result.unwrap(), ReturnErrorCode::Success);

// Check if the funds are subtracted from the account of Alice and added to the account of
// Bob.
let initial = INITIAL_BALANCE;
assert_eq!(parachain::Assets::balance(0, contract_addr), initial);
assert_eq!(ParachainBalances::free_balance(BOB), initial + amount);
assert_eq!(ParachainBalances::free_balance(&contract_addr), initial - amount);
});
}

#[test]
fn test_xcm_execute_incomplete() {
MockNet::reset();

let contract_addr = instantiate_test_contract("xcm_execute");
let amount = 10 * CENTS;

// Execute XCM instructions through the contract.
ParaA::execute_with(|| {
// The XCM used to transfer funds to Bob.
let message: Xcm<()> = Xcm(vec![
WithdrawAsset(vec![(Here, amount).into()].into()),
// This will fail as the contract does not have enough balance to complete both
// withdrawals.
WithdrawAsset(vec![(Here, INITIAL_BALANCE).into()].into()),
DepositAsset {
assets: All.into(),
beneficiary: AccountId32 { network: None, id: BOB.clone().into() }.into(),
},
]);

let result = ParachainContracts::bare_call(
ALICE,
contract_addr.clone(),
0,
Weight::MAX,
None,
VersionedXcm::V4(message).encode(),
DebugInfo::UnsafeDebug,
CollectEvents::UnsafeCollect,
Determinism::Enforced,
);

assert_eq!(result.gas_consumed, result.gas_required);
assert_return_code!(&result.result.unwrap(), ReturnErrorCode::XcmExecutionFailed);

assert_eq!(ParachainBalances::free_balance(BOB), INITIAL_BALANCE);
assert_eq!(ParachainBalances::free_balance(&contract_addr), INITIAL_BALANCE - amount);
});
}

Expand Down Expand Up @@ -182,17 +226,10 @@ fn test_xcm_execute_reentrant_call() {
DebugInfo::UnsafeDebug,
CollectEvents::UnsafeCollect,
Determinism::Enforced,
)
.result
.unwrap();

let mut data = &result.data[..];
let outcome = Outcome::decode(&mut data).expect("Failed to decode xcm_execute Outcome");
assert_matches!(
outcome,
Outcome::Incomplete { used: _, error: XcmError::ExpectationFalse }
);

assert_return_code!(&result.result.unwrap(), ReturnErrorCode::XcmExecutionFailed);

// Funds should not change hands as the XCM transact failed.
assert_eq!(ParachainBalances::free_balance(BOB), INITIAL_BALANCE);
});
Expand Down
7 changes: 7 additions & 0 deletions substrate/frame/contracts/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,9 @@ pub trait Ext: sealing::Sealed {
/// Returns `true` if debug message recording is enabled. Otherwise `false` is returned.
fn append_debug_buffer(&mut self, msg: &str) -> bool;

/// Returns `true` if debug message recording is enabled. Otherwise `false` is returned.
fn debug_buffer_enabled(&self) -> bool;

/// Call some dispatchable and return the result.
fn call_runtime(&self, call: <Self::T as Config>::RuntimeCall) -> DispatchResultWithPostInfo;

Expand Down Expand Up @@ -1429,6 +1432,10 @@ where
self.top_frame_mut().nested_storage.charge(diff)
}

fn debug_buffer_enabled(&self) -> bool {
self.debug_message.is_some()
}

fn append_debug_buffer(&mut self, msg: &str) -> bool {
if let Some(buffer) = &mut self.debug_message {
buffer
Expand Down
9 changes: 8 additions & 1 deletion substrate/frame/contracts/src/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ pub trait Token<T: Config>: Copy + Clone + TestAuxiliaries {
/// while calculating the amount. In this case it is ok to use saturating operations
/// since on overflow they will return `max_value` which should consume all gas.
fn weight(&self) -> Weight;

/// Returns true if this token is expected to influence the lowest gas limit.
fn influence_lowest_gas_limit(&self) -> bool {
true
}
}

/// A wrapper around a type-erased trait object of what used to be a `Token`.
Expand Down Expand Up @@ -160,7 +165,9 @@ impl<T: Config> GasMeter<T> {
/// This is when a maximum a priori amount was charged and then should be partially
/// refunded to match the actual amount.
pub fn adjust_gas<Tok: Token<T>>(&mut self, charged_amount: ChargedAmount, token: Tok) {
self.gas_left_lowest = self.gas_left_lowest();
if token.influence_lowest_gas_limit() {
self.gas_left_lowest = self.gas_left_lowest();
}
let adjustment = charged_amount.0.saturating_sub(token.weight());
self.gas_left = self.gas_left.saturating_add(adjustment).min(self.gas_limit);
}
Expand Down
4 changes: 4 additions & 0 deletions substrate/frame/contracts/src/wasm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,10 @@ mod tests {
&mut self.gas_meter
}
fn charge_storage(&mut self, _diff: &crate::storage::meter::Diff) {}

fn debug_buffer_enabled(&self) -> bool {
true
}
fn append_debug_buffer(&mut self, msg: &str) -> bool {
self.debug_buffer.extend(msg.as_bytes());
true
Expand Down
Loading

0 comments on commit ca382f3

Please sign in to comment.