Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pallet-xcm: ensure xcm outcome is complete, or discard all state changes done by extrinsic #2405

Merged
merged 5 commits into from
Nov 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ pub fn teleports_for_native_asset_works<
hash,
RuntimeHelper::<Runtime>::xcm_max_weight(XcmReceivedFrom::Parent),
);
assert_eq!(outcome.ensure_complete(), Ok(()));
assert_ok!(outcome.ensure_complete());

// check Balances after
assert_ne!(<pallet_balances::Pallet<Runtime>>::free_balance(&target_account), 0.into());
Expand Down Expand Up @@ -499,7 +499,7 @@ pub fn teleports_for_foreign_assets_works<
hash,
RuntimeHelper::<Runtime>::xcm_max_weight(XcmReceivedFrom::Sibling),
);
assert_eq!(outcome.ensure_complete(), Ok(()));
assert_ok!(outcome.ensure_complete());

// checks target_account after
assert_eq!(
Expand Down Expand Up @@ -1211,7 +1211,7 @@ pub fn create_and_manage_foreign_assets_for_local_consensus_parachain_assets_wor
hash,
RuntimeHelper::<Runtime>::xcm_max_weight(XcmReceivedFrom::Sibling),
);
assert_eq!(outcome.ensure_complete(), Ok(()));
assert_ok!(outcome.ensure_complete());

// check events
let mut events = <frame_system::Pallet<Runtime>>::events()
Expand Down Expand Up @@ -1319,7 +1319,7 @@ pub fn create_and_manage_foreign_assets_for_local_consensus_parachain_assets_wor
hash,
RuntimeHelper::<Runtime>::xcm_max_weight(XcmReceivedFrom::Sibling),
);
assert_eq!(outcome.ensure_complete(), Ok(()));
assert_ok!(outcome.ensure_complete());

additional_checks_after();
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ pub fn receive_reserve_asset_deposited_from_different_consensus_works<
XcmReceivedFrom::Sibling,
),
);
assert_eq!(outcome.ensure_complete(), Ok(()));
assert_ok!(outcome.ensure_complete());

// author actual balance after (received fees from Trader for ForeignAssets)
let author_received_fees =
Expand Down Expand Up @@ -588,7 +588,7 @@ pub fn report_bridge_status_from_xcm_bridge_router_works<
hash,
RuntimeHelper::<Runtime, AllPalletsWithoutSystem>::xcm_max_weight(XcmReceivedFrom::Sibling),
);
assert_eq!(outcome.ensure_complete(), Ok(()));
assert_ok!(outcome.ensure_complete());
assert_eq!(is_congested, pallet_xcm_bridge_hub_router::Pallet::<Runtime, XcmBridgeHubRouterInstance>::bridge().is_congested);
};

Expand Down
28 changes: 23 additions & 5 deletions polkadot/xcm/pallet-xcm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ pub mod pallet {
InvalidAssetUnsupportedReserve,
/// Too many assets with different reserve locations have been attempted for transfer.
TooManyReserves,
/// Local XCM execution of asset transfer incomplete.
/// Local XCM execution incomplete.
LocalExecutionIncomplete,
}

Expand Down Expand Up @@ -1009,8 +1009,14 @@ pub mod pallet {
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)?;
Ok(Some(outcome.weight_used().saturating_add(T::WeightInfo::execute())).into())
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
})?;
Ok(Some(weight_used.saturating_add(T::WeightInfo::execute())).into())
}

/// Extoll that a particular destination can be communicated with through a particular
Expand Down Expand Up @@ -1495,13 +1501,25 @@ impl<T: Config> Pallet<T> {
let outcome =
T::XcmExecutor::execute_xcm_in_credit(origin, local_xcm, hash, weight, weight);
Self::deposit_event(Event::Attempted { outcome: outcome.clone() });
if let Some(remote_xcm) = remote_xcm {
outcome.ensure_complete().map_err(|_| Error::<T>::LocalExecutionIncomplete)?;
outcome.ensure_complete().map_err(|error| {
log::error!(
target: "xcm::pallet_xcm::build_and_execute_xcm_transfer_type",
"XCM execution failed with error {:?}", error
);
Error::<T>::LocalExecutionIncomplete
})?;

if let Some(remote_xcm) = remote_xcm {
let (ticket, price) = validate_send::<T::XcmRouter>(dest, remote_xcm.clone())
.map_err(Error::<T>::from)?;
if origin != Here.into_location() {
Self::charge_fees(origin, price).map_err(|_| Error::<T>::FeesNotMet)?;
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
Self::charge_fees(origin, price).map_err(|error| {
log::error!(
target: "xcm::pallet_xcm::build_and_execute_xcm_transfer_type",
"Unable to charge fee with error {:?}", error
);
Error::<T>::FeesNotMet
})?;
}
let message_id = T::XcmRouter::deliver(ticket).map_err(Error::<T>::from)?;

Expand Down
7 changes: 7 additions & 0 deletions polkadot/xcm/pallet-xcm/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ construct_runtime!(

thread_local! {
pub static SENT_XCM: RefCell<Vec<(MultiLocation, Xcm<()>)>> = RefCell::new(Vec::new());
pub static FAIL_SEND_XCM: RefCell<bool> = RefCell::new(false);
}
pub(crate) fn sent_xcm() -> Vec<(MultiLocation, Xcm<()>)> {
SENT_XCM.with(|q| (*q.borrow()).clone())
Expand All @@ -164,6 +165,9 @@ pub(crate) fn take_sent_xcm() -> Vec<(MultiLocation, Xcm<()>)> {
r
})
}
pub(crate) fn set_send_xcm_artificial_failure(should_fail: bool) {
FAIL_SEND_XCM.with(|q| *q.borrow_mut() = should_fail);
}
/// Sender that never returns error.
pub struct TestSendXcm;
impl SendXcm for TestSendXcm {
Expand All @@ -172,6 +176,9 @@ impl SendXcm for TestSendXcm {
dest: &mut Option<MultiLocation>,
msg: &mut Option<Xcm<()>>,
) -> SendResult<(MultiLocation, Xcm<()>)> {
if FAIL_SEND_XCM.with(|q| *q.borrow()) {
return Err(SendError::Transport("Intentional send failure used in tests"))
}
let pair = (dest.take().unwrap(), msg.take().unwrap());
Ok((pair, MultiAssets::new()))
}
Expand Down
58 changes: 58 additions & 0 deletions polkadot/xcm/pallet-xcm/src/tests/assets_transfer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1457,3 +1457,61 @@ fn reserve_transfer_assets_with_filtered_teleported_fee_disallowed() {
);
});
}

/// Test failure to complete execution of local XCM instructions reverts intermediate side-effects.
///
/// Extrinsic will execute XCM to withdraw & burn reserve-based assets, then fail sending XCM to
/// reserve chain for releasing reserve assets. Assert that the previous instructions (withdraw &
/// burn) effects are reverted.
#[test]
fn intermediary_error_reverts_side_effects() {
let balances = vec![(ALICE, INITIAL_BALANCE)];
let beneficiary: MultiLocation =
Junction::AccountId32 { network: None, id: ALICE.into() }.into();
new_test_ext_with_balances(balances).execute_with(|| {
// create sufficient foreign asset USDC (0 total issuance)
let usdc_initial_local_amount = 142;
let (_, usdc_chain_sovereign_account, usdc_id_multilocation) = set_up_foreign_asset(
USDC_RESERVE_PARA_ID,
Some(USDC_INNER_JUNCTION),
usdc_initial_local_amount,
true,
);

// transfer destination is some other parachain
let dest = RelayLocation::get().pushed_with_interior(Parachain(OTHER_PARA_ID)).unwrap();

let assets: MultiAssets = vec![(usdc_id_multilocation, SEND_AMOUNT).into()].into();
let fee_index = 0;

// balances checks before
assert_eq!(Assets::balance(usdc_id_multilocation, ALICE), usdc_initial_local_amount);
assert_eq!(Balances::free_balance(ALICE), INITIAL_BALANCE);

// introduce artificial error in sending outbound XCM
set_send_xcm_artificial_failure(true);

// do the transfer - extrinsic should completely fail on xcm send failure
assert!(XcmPallet::limited_reserve_transfer_assets(
RuntimeOrigin::signed(ALICE),
Box::new(dest.into()),
Box::new(beneficiary.into()),
Box::new(assets.into()),
fee_index as u32,
Unlimited,
)
.is_err());

// Alice no changes
assert_eq!(Assets::balance(usdc_id_multilocation, ALICE), usdc_initial_local_amount);
assert_eq!(Balances::free_balance(ALICE), INITIAL_BALANCE);
// Destination account (parachain account) no changes
assert_eq!(Balances::free_balance(usdc_chain_sovereign_account.clone()), 0);
assert_eq!(Assets::balance(usdc_id_multilocation, usdc_chain_sovereign_account), 0);
// Verify total and active issuance of USDC has not changed
assert_eq!(Assets::total_issuance(usdc_id_multilocation), usdc_initial_local_amount);
assert_eq!(Assets::active_issuance(usdc_id_multilocation), usdc_initial_local_amount);
// Verify no XCM program sent
assert_eq!(sent_xcm(), vec![]);
});
}
48 changes: 47 additions & 1 deletion polkadot/xcm/pallet-xcm/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ fn trapped_assets_can_be_claimed() {
assert_eq!(AssetTraps::<Test>::iter().collect::<Vec<_>>(), vec![]);

let weight = BaseXcmWeight::get() * 3;
assert_ok!(XcmPallet::execute(
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() },
Expand All @@ -459,6 +459,52 @@ fn trapped_assets_can_be_claimed() {
});
}

/// Test failure to complete execution reverts intermediate side-effects.
///
/// XCM program will withdraw and deposit some assets, then fail execution of a further withdraw.
/// Assert that the previous instructions effects are reverted.
#[test]
fn incomplete_execute_reverts_side_effects() {
let balances = vec![(ALICE, INITIAL_BALANCE), (BOB, INITIAL_BALANCE)];
new_test_ext_with_balances(balances).execute_with(|| {
let weight = BaseXcmWeight::get() * 4;
let dest: MultiLocation = Junction::AccountId32 { network: None, id: BOB.into() }.into();
assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE);
let amount_to_send = INITIAL_BALANCE - ExistentialDeposit::get();
let assets: MultiAssets = (Here, amount_to_send).into();
let result = XcmPallet::execute(
RuntimeOrigin::signed(ALICE),
Box::new(VersionedXcm::from(Xcm(vec![
// Withdraw + BuyExec + Deposit should work
WithdrawAsset(assets.clone()),
buy_execution(assets.inner()[0].clone()),
DepositAsset { assets: assets.clone().into(), beneficiary: dest },
// Withdrawing once more will fail because of InsufficientBalance, and we expect to
// revert the effects of the above instructions as well
WithdrawAsset(assets),
]))),
weight,
);
// 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,
pays_fee: frame_support::dispatch::Pays::Yes,
},
error: sp_runtime::DispatchError::Module(sp_runtime::ModuleError {
index: 4,
error: [24, 0, 0, 0,],
message: Some("LocalExecutionIncomplete")
})
})
);
});
}

#[test]
fn fake_latest_versioned_multilocation_works() {
use codec::Encode;
Expand Down
4 changes: 2 additions & 2 deletions polkadot/xcm/src/v3/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,9 @@ pub enum Outcome {
}

impl Outcome {
pub fn ensure_complete(self) -> Result {
pub fn ensure_complete(self) -> result::Result<Weight, Error> {
match self {
Outcome::Complete(_) => Ok(()),
Outcome::Complete(weight) => Ok(weight),
Outcome::Incomplete(_, e) => Err(e),
Outcome::Error(e) => Err(e),
}
Expand Down
1 change: 1 addition & 0 deletions polkadot/xcm/xcm-executor/integration-tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ codec = { package = "parity-scale-codec", version = "3.6.1" }
frame-support = { path = "../../../../substrate/frame/support", default-features = false }
frame-system = { path = "../../../../substrate/frame/system" }
futures = "0.3.21"
pallet-transaction-payment = { path = "../../../../substrate/frame/transaction-payment" }
pallet-xcm = { path = "../../pallet-xcm" }
polkadot-test-client = { path = "../../../node/test/client" }
polkadot-test-runtime = { path = "../../../runtime/test-runtime" }
Expand Down
86 changes: 65 additions & 21 deletions polkadot/xcm/xcm-executor/integration-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,33 +76,59 @@ fn transact_recursion_limit_works() {
sp_tracing::try_init_simple();
let mut client = TestClientBuilder::new().build();

let mut msg = Xcm(vec![ClearOrigin]);
let max_weight = <XcmConfig as xcm_executor::Config>::Weigher::weight(&mut msg).unwrap();
let mut call = polkadot_test_runtime::RuntimeCall::Xcm(pallet_xcm::Call::execute {
message: Box::new(VersionedXcm::from(msg)),
max_weight,
});

for _ in 0..11 {
let mut msg = Xcm(vec![
WithdrawAsset((Parent, 1_000).into()),
BuyExecution { fees: (Parent, 1).into(), weight_limit: Unlimited },
let base_xcm = |call: polkadot_test_runtime::RuntimeCall| {
Xcm(vec![
WithdrawAsset((Here, 1_000).into()),
BuyExecution { fees: (Here, 1).into(), weight_limit: Unlimited },
Transact {
origin_kind: OriginKind::Native,
require_weight_at_most: call.get_dispatch_info().weight,
call: call.encode().into(),
},
]);
])
};
let mut call: Option<polkadot_test_runtime::RuntimeCall> = None;
// set up transacts with recursive depth of 11
for depth in (1..12).rev() {
let mut msg;
match depth {
// this one should fail with `XcmError::ExceedsStackLimit`
11 => {
msg = Xcm(vec![ClearOrigin]);
},
// this one checks that the inner one (depth 11) fails as expected,
// itself should not fail => should have outcome == Complete
10 => {
let inner_call = call.take().unwrap();
let expected_transact_status =
sp_runtime::DispatchError::Module(sp_runtime::ModuleError {
index: 27,
error: [24, 0, 0, 0],
message: Some("LocalExecutionIncomplete"),
})
.encode()
.into();
msg = base_xcm(inner_call);
msg.inner_mut().push(ExpectTransactStatus(expected_transact_status));
},
// these are the outer 9 calls that expect `ExpectTransactStatus(Success)`
d if d >= 1 && d <= 9 => {
let inner_call = call.take().unwrap();
msg = base_xcm(inner_call);
msg.inner_mut().push(ExpectTransactStatus(MaybeErrorCode::Success));
},
_ => unreachable!(),
}
let max_weight = <XcmConfig as xcm_executor::Config>::Weigher::weight(&mut msg).unwrap();
call = polkadot_test_runtime::RuntimeCall::Xcm(pallet_xcm::Call::execute {
message: Box::new(VersionedXcm::from(msg)),
call = Some(polkadot_test_runtime::RuntimeCall::Xcm(pallet_xcm::Call::execute {
message: Box::new(VersionedXcm::from(msg.clone())),
max_weight,
});
}));
}

let mut block_builder = client.init_polkadot_block_builder();

let execute = construct_extrinsic(&client, call, sp_keyring::Sr25519Keyring::Alice, 0);
let execute = construct_extrinsic(&client, call.unwrap(), sp_keyring::Sr25519Keyring::Alice, 0);

block_builder.push_polkadot_extrinsic(execute).expect("pushes extrinsic");

Expand All @@ -113,11 +139,29 @@ fn transact_recursion_limit_works() {
.expect("imports the block");

client.state_at(block_hash).expect("state should exist").inspect_state(|| {
assert!(polkadot_test_runtime::System::events().iter().any(|r| matches!(
r.event,
polkadot_test_runtime::RuntimeEvent::Xcm(pallet_xcm::Event::Attempted {
outcome: Outcome::Incomplete(_, XcmError::ExceedsStackLimit)
}),
let events = polkadot_test_runtime::System::events();
// verify 10 pallet_xcm calls were successful
assert_eq!(
polkadot_test_runtime::System::events()
.iter()
.filter(|r| matches!(
r.event,
polkadot_test_runtime::RuntimeEvent::Xcm(pallet_xcm::Event::Attempted {
outcome: Outcome::Complete(_)
}),
))
.count(),
10
);
// verify transaction fees have been paid
assert!(events.iter().any(|r| matches!(
&r.event,
polkadot_test_runtime::RuntimeEvent::TransactionPayment(
pallet_transaction_payment::Event::TransactionFeePaid {
who: payer,
..
}
) if *payer == sp_keyring::Sr25519Keyring::Alice.into(),
)));
});
}
Expand Down