Remove redundant XCMs from dry run's forwarded xcms (#5913)

# Description

This PR addresses
#5878.

After dry running an xcm on asset hub, we had redundant xcms showing up
in the `forwarded_xcms` field of the dry run effects returned.
These were caused by two things:
- The `UpwardMessageSender` router always added an element even if there
were no messages.
- The two routers on asset hub westend related to bridging (to rococo
and sepolia) getting the message from their queues when their queues is
actually the same xcmp queue that was already contemplated.

In order to fix this, we check for no messages in UMP and clear the
implementation of `InspectMessageQueues` for these bridging routers.
Keep in mind that the bridged message is still sent, as normal via the
xcmp-queue to Bridge Hub.
To keep on dry-running the journey of the message, the next hop to
dry-run is Bridge Hub.
That'll be tackled in a different PR.

Added a test in `bridge-hub-westend-integration-tests` and
`bridge-hub-rococo-integration-tests` that show that dry-running a
transfer across the bridge from asset hub results in one and only one
message sent to bridge hub.

## TODO
- [x] Functionality
- [x] Test

---------

Co-authored-by: command-bot <>

bridges/modules/xcm-bridge-hub-router/src/lib.rs

@@ -99,7 +99,7 @@ pub mod pallet {
 		type DestinationVersion: GetVersion;
 
 		/// Actual message sender (`HRMP` or `DMP`) to the sibling bridge hub location.
-		type ToBridgeHubSender: SendXcm + InspectMessageQueues;
+		type ToBridgeHubSender: SendXcm;
 		/// Local XCM channel manager.
 		type LocalXcmChannelManager: XcmChannelStatusProvider;
 
@@ -408,12 +408,12 @@ impl<T: Config<I>, I: 'static> SendXcm for Pallet<T, I> {
 }
 
 impl<T: Config<I>, I: 'static> InspectMessageQueues for Pallet<T, I> {
-	fn clear_messages() {
-		ViaBridgeHubExporter::<T, I>::clear_messages()
-	}
+	fn clear_messages() {}
 
+	/// This router needs to implement `InspectMessageQueues` but doesn't have to
+	/// return any messages, since it just reuses the `XcmpQueue` router.
 	fn get_messages() -> Vec<(VersionedLocation, Vec<VersionedXcm<()>>)> {
-		ViaBridgeHubExporter::<T, I>::get_messages()
+		Vec::new()
 	}
 }
 
@@ -646,34 +646,13 @@ mod tests {
 	}
 
 	#[test]
-	fn get_messages_works() {
+	fn get_messages_does_not_return_anything() {
 		run_test(|| {
 			assert_ok!(send_xcm::<XcmBridgeHubRouter>(
 				(Parent, Parent, GlobalConsensus(BridgedNetworkId::get()), Parachain(1000)).into(),
 				vec![ClearOrigin].into()
 			));
-			assert_eq!(
-				XcmBridgeHubRouter::get_messages(),
-				vec![(
-					VersionedLocation::V4((Parent, Parachain(1002)).into()),
-					vec![VersionedXcm::V4(
-						Xcm::builder()
-							.withdraw_asset((Parent, 1_002_000))
-							.buy_execution((Parent, 1_002_000), Unlimited)
-							.set_appendix(
-								Xcm::builder_unsafe()
-									.deposit_asset(AllCounted(1), (Parent, Parachain(1000)))
-									.build()
-							)
-							.export_message(
-								Kusama,
-								Parachain(1000),
-								Xcm::builder_unsafe().clear_origin().build()
-							)
-							.build()
-					)],
-				),],
-			);
+			assert_eq!(XcmBridgeHubRouter::get_messages(), vec![]);
 		});
 	}
 }

cumulus/pallets/parachain-system/src/lib.rs

@@ -1627,7 +1627,11 @@ impl<T: Config> InspectMessageQueues for Pallet<T> {
 			.map(|encoded_message| VersionedXcm::<()>::decode(&mut &encoded_message[..]).unwrap())
 			.collect();
 
-		vec![(VersionedLocation::V4(Parent.into()), messages)]
+		if messages.is_empty() {
+			vec![]
+		} else {
+			vec![(VersionedLocation::from(Location::parent()), messages)]
+		}
 	}
 }
 

cumulus/parachains/integration-tests/emulated/common/src/macros.rs

@@ -403,3 +403,51 @@ macro_rules! test_chain_can_claim_assets {
 		}
 	};
 }
+
+#[macro_export]
+macro_rules! test_dry_run_transfer_across_pk_bridge {
+	( $sender_asset_hub:ty, $sender_bridge_hub:ty, $destination:expr ) => {
+		$crate::macros::paste::paste! {
+			use frame_support::{dispatch::RawOrigin, traits::fungible};
+			use sp_runtime::AccountId32;
+			use xcm::prelude::*;
+			use xcm_runtime_apis::dry_run::runtime_decl_for_dry_run_api::DryRunApiV1;
+
+			let who = AccountId32::new([1u8; 32]);
+			let transfer_amount = 10_000_000_000_000u128;
+			let initial_balance = transfer_amount * 10;
+
+			// Bridge setup.
+			$sender_asset_hub::force_xcm_version($destination, XCM_VERSION);
+			open_bridge_between_asset_hub_rococo_and_asset_hub_westend();
+
+			<$sender_asset_hub as TestExt>::execute_with(|| {
+				type Runtime = <$sender_asset_hub as Chain>::Runtime;
+				type RuntimeCall = <$sender_asset_hub as Chain>::RuntimeCall;
+				type OriginCaller = <$sender_asset_hub as Chain>::OriginCaller;
+				type Balances = <$sender_asset_hub as [<$sender_asset_hub Pallet>]>::Balances;
+
+				// Give some initial funds.
+				<Balances as fungible::Mutate<_>>::set_balance(&who, initial_balance);
+
+				let call = RuntimeCall::PolkadotXcm(pallet_xcm::Call::transfer_assets {
+					dest: Box::new(VersionedLocation::from($destination)),
+					beneficiary: Box::new(VersionedLocation::from(Junction::AccountId32 {
+						id: who.clone().into(),
+						network: None,
+					})),
+					assets: Box::new(VersionedAssets::from(vec![
+						(Parent, transfer_amount).into(),
+					])),
+					fee_asset_item: 0,
+					weight_limit: Unlimited,
+				});
+				let result = Runtime::dry_run_call(OriginCaller::system(RawOrigin::Signed(who)), call).unwrap();
+				// We assert the dry run succeeds and sends only one message to the local bridge hub.
+				assert!(result.execution_result.is_ok());
+				assert_eq!(result.forwarded_xcms.len(), 1);
+				assert_eq!(result.forwarded_xcms[0].0, VersionedLocation::from(Location::new(1, [Parachain($sender_bridge_hub::para_id().into())])));
+			});
+		}
+	};
+}

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/Cargo.toml

@@ -28,6 +28,7 @@ sp-runtime = { workspace = true }
 xcm = { workspace = true }
 pallet-xcm = { workspace = true }
 xcm-executor = { workspace = true }
+xcm-runtime-apis = { workspace = true }
 
 # Bridges
 pallet-bridge-messages = { workspace = true }

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs

@@ -33,8 +33,8 @@ mod imports {
 	pub use emulated_integration_tests_common::{
 		accounts::ALICE,
 		impls::Inspect,
-		test_parachain_is_trusted_teleporter, test_parachain_is_trusted_teleporter_for_relay,
-		test_relay_is_trusted_teleporter,
+		test_dry_run_transfer_across_pk_bridge, test_parachain_is_trusted_teleporter,
+		test_parachain_is_trusted_teleporter_for_relay, test_relay_is_trusted_teleporter,
 		xcm_emulator::{
 			assert_expected_events, bx, Chain, Parachain as Para, RelayChain as Relay, TestExt,
 		},

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs

@@ -534,3 +534,12 @@ fn send_back_wnds_from_penpal_rococo_through_asset_hub_rococo_to_asset_hub_weste
 	assert!(receiver_wnds_after > receiver_wnds_before);
 	assert!(receiver_wnds_after <= receiver_wnds_before + amount);
 }
+
+#[test]
+fn dry_run_transfer_to_westend_sends_xcm_to_bridge_hub() {
+	test_dry_run_transfer_across_pk_bridge!(
+		AssetHubRococo,
+		BridgeHubRococo,
+		asset_hub_westend_location()
+	);
+}

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/Cargo.toml

@@ -29,6 +29,7 @@ sp-runtime = { workspace = true }
 xcm = { workspace = true }
 pallet-xcm = { workspace = true }
 xcm-executor = { workspace = true }
+xcm-runtime-apis = { workspace = true }
 
 # Bridges
 pallet-bridge-messages = { workspace = true }

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/lib.rs

@@ -32,8 +32,8 @@ mod imports {
 	pub use emulated_integration_tests_common::{
 		accounts::ALICE,
 		impls::Inspect,
-		test_parachain_is_trusted_teleporter, test_parachain_is_trusted_teleporter_for_relay,
-		test_relay_is_trusted_teleporter,
+		test_dry_run_transfer_across_pk_bridge, test_parachain_is_trusted_teleporter,
+		test_parachain_is_trusted_teleporter_for_relay, test_relay_is_trusted_teleporter,
 		xcm_emulator::{
 			assert_expected_events, bx, Chain, Parachain as Para, RelayChain as Relay, TestExt,
 		},

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs

@@ -554,3 +554,12 @@ fn send_back_rocs_from_penpal_westend_through_asset_hub_westend_to_asset_hub_roc
 	assert!(receiver_rocs_after > receiver_rocs_before);
 	assert!(receiver_rocs_after <= receiver_rocs_before + amount);
 }
+
+#[test]
+fn dry_run_transfer_to_rococo_sends_xcm_to_bridge_hub() {
+	test_dry_run_transfer_across_pk_bridge!(
+		AssetHubWestend,
+		BridgeHubWestend,
+		asset_hub_rococo_location()
+	);
+}

polkadot/xcm/xcm-builder/src/universal_exports.rs

@@ -337,15 +337,15 @@ impl<Bridges: ExporterFor, Router: SendXcm, UniversalLocation: Get<InteriorLocat
 	}
 }
 
-impl<Bridges, Router: InspectMessageQueues, UniversalLocation> InspectMessageQueues
+impl<Bridges, Router, UniversalLocation> InspectMessageQueues
 	for SovereignPaidRemoteExporter<Bridges, Router, UniversalLocation>
 {
-	fn clear_messages() {
-		Router::clear_messages()
-	}
+	fn clear_messages() {}
 
+	/// This router needs to implement `InspectMessageQueues` but doesn't have to
+	/// return any messages, since it just reuses the `XcmpQueue` router.
 	fn get_messages() -> Vec<(VersionedLocation, Vec<VersionedXcm<()>>)> {
-		Router::get_messages()
+		Vec::new()
 	}
 }
 

prdoc/pr_5913.prdoc

@@ -0,0 +1,20 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Remove redundant XCMs from dry run's forwarded xcms
+
+doc:
+  - audience: Runtime User
+    description: |
+      The DryRunApi was returning the same message repeated multiple times in the
+      `forwarded_xcms` field. This is no longer the case.
+
+crates:
+  - name: pallet-xcm-bridge-hub-router
+    bump: patch
+  - name: cumulus-pallet-parachain-system
+    bump: patch
+  - name: staging-xcm-builder
+    bump: patch
+  - name: emulated-integration-tests-common
+    bump: minor