pallet-xcm: ensure xcm outcome is always complete, revert effects otherwise (#2405)

On extrinsics/call, ensure local XCM execution is complete/successful.
Otherwise, fail the extrinsic so that state changes don't get committed
to the db.

Added regression tests that fail without the fix.

fixes #2237

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>

polkadot/xcm/pallet-xcm/src/lib.rs

@@ -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,
 	}
 
@@ -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
@@ -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)?;
+				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)?;
 

polkadot/xcm/pallet-xcm/src/mock.rs

@@ -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())
@@ -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 {
@@ -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()))
 	}

polkadot/xcm/pallet-xcm/src/tests/assets_transfer.rs

@@ -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![]);
+	});
+}

polkadot/xcm/pallet-xcm/src/tests/mod.rs

@@ -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() },
@@ -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;