diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 22f6514c8e36c..25e0fb3cfab6f 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -1630,6 +1630,42 @@ impl UpwardMessageSender for Pallet { fn send_upward_message(message: UpwardMessage) -> Result<(u32, XcmHash), MessageSendError> { Self::send_upward_message(message) } + + fn can_send_upward_message(message: &UpwardMessage) -> Result<(), MessageSendError> { + let max_upward_message_size = HostConfiguration::::get() + .map(|cfg| cfg.max_upward_message_size) + .ok_or(MessageSendError::Other)?; + if message.len() > max_upward_message_size as usize { + Err(MessageSendError::TooBig) + } else { + Ok(()) + } + } + + #[cfg(any(feature = "std", feature = "runtime-benchmarks", test))] + fn ensure_successful_delivery() { + const MAX_UPWARD_MESSAGE_SIZE: u32 = 65_531 * 3; + const MAX_CODE_SIZE: u32 = 3 * 1024 * 1024; + HostConfiguration::::mutate(|cfg| match cfg { + Some(cfg) => cfg.max_upward_message_size = MAX_UPWARD_MESSAGE_SIZE, + None => + *cfg = Some(AbridgedHostConfiguration { + max_code_size: MAX_CODE_SIZE, + max_head_data_size: 32 * 1024, + max_upward_queue_count: 8, + max_upward_queue_size: 1024 * 1024, + max_upward_message_size: MAX_UPWARD_MESSAGE_SIZE, + max_upward_message_num_per_candidate: 2, + hrmp_max_message_num_per_candidate: 2, + validation_upgrade_cooldown: 2, + validation_upgrade_delay: 2, + async_backing_params: relay_chain::AsyncBackingParams { + allowed_ancestry_len: 0, + max_candidate_depth: 0, + }, + }), + }) + } } impl InspectMessageQueues for Pallet { diff --git a/cumulus/pallets/parachain-system/src/tests.rs b/cumulus/pallets/parachain-system/src/tests.rs index 1f9cb6d54de4f..fee6d7811b9d8 100755 --- a/cumulus/pallets/parachain-system/src/tests.rs +++ b/cumulus/pallets/parachain-system/src/tests.rs @@ -557,7 +557,7 @@ fn aborted_upgrade() { } #[test] -fn checks_size() { +fn checks_code_size() { BlockTests::new() .with_relay_sproof_builder(|_, _, builder| { builder.host_config.max_code_size = 8; @@ -706,6 +706,34 @@ fn send_upward_message_relay_bottleneck() { ); } +#[test] +fn send_upwards_message_checks_size_on_validate() { + BlockTests::new() + .with_relay_sproof_builder(|_, _, sproof| { + sproof.host_config.max_upward_message_size = 128; + }) + .add(1, || { + assert_eq!( + ParachainSystem::can_send_upward_message(vec![0u8; 129].as_ref()), + Err(MessageSendError::TooBig) + ); + }); +} + +#[test] +fn send_upward_message_check_size() { + BlockTests::new() + .with_relay_sproof_builder(|_, _, sproof| { + sproof.host_config.max_upward_message_size = 128; + }) + .add(1, || { + assert_eq!( + ParachainSystem::send_upward_message(vec![0u8; 129]), + Err(MessageSendError::TooBig) + ); + }); +} + #[test] fn send_hrmp_message_buffer_channel_close() { BlockTests::new() diff --git a/cumulus/parachains/runtimes/assets/test-utils/src/test_cases.rs b/cumulus/parachains/runtimes/assets/test-utils/src/test_cases.rs index 3279aba88d5cc..36494098edd87 100644 --- a/cumulus/parachains/runtimes/assets/test-utils/src/test_cases.rs +++ b/cumulus/parachains/runtimes/assets/test-utils/src/test_cases.rs @@ -19,7 +19,7 @@ use super::xcm_helpers; use crate::{assert_matches_reserve_asset_deposited_instructions, get_fungible_delivery_fees}; use codec::Encode; use core::ops::Mul; -use cumulus_primitives_core::XcmpMessageSource; +use cumulus_primitives_core::{UpwardMessageSender, XcmpMessageSource}; use frame_support::{ assert_noop, assert_ok, traits::{ @@ -171,6 +171,8 @@ pub fn teleports_for_native_asset_works< // 2. try to teleport asset back to the relaychain { + as UpwardMessageSender>::ensure_successful_delivery(); + let dest = Location::parent(); let mut dest_beneficiary = Location::parent() .appended_with(AccountId32 { diff --git a/cumulus/parachains/runtimes/assets/test-utils/src/xcm_helpers.rs b/cumulus/parachains/runtimes/assets/test-utils/src/xcm_helpers.rs index 36c99449500ad..b81785fec6906 100644 --- a/cumulus/parachains/runtimes/assets/test-utils/src/xcm_helpers.rs +++ b/cumulus/parachains/runtimes/assets/test-utils/src/xcm_helpers.rs @@ -97,8 +97,9 @@ fn teleport_assets_dummy_message( /// Given a message, a sender, and a destination, it returns the delivery fees fn get_fungible_delivery_fees(destination: Location, message: Xcm<()>) -> u128 { - let Ok((_, delivery_fees)) = validate_send::(destination, message) else { - unreachable!("message can be sent; qed") + let delivery_fees = match validate_send::(destination, message) { + Ok((_, delivery_fees)) => delivery_fees, + Err(e) => unreachable!("message can be sent - {:?}; qed", e), }; if let Some(delivery_fee) = delivery_fees.inner().first() { let Fungible(delivery_fee_amount) = delivery_fee.fun else { diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs index 26a44104bb98d..071380993693e 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs @@ -140,6 +140,7 @@ mod bridge_hub_westend_tests { BridgeHubWestendLocation, WestendGlobalConsensusNetwork, WithBridgeHubWestendMessagesInstance, XcmOverBridgeHubWestendInstance, }; + use cumulus_primitives_core::UpwardMessageSender; // Random para id of sibling chain used in tests. pub const SIBLING_PARACHAIN_ID: u32 = 2053; @@ -386,7 +387,7 @@ mod bridge_hub_westend_tests { _ => None, } }), - || (), + || ::ensure_successful_delivery(), ) } @@ -533,6 +534,7 @@ mod bridge_hub_bulletin_tests { RococoBulletinGlobalConsensusNetwork, RococoBulletinGlobalConsensusNetworkLocation, WithRococoBulletinMessagesInstance, XcmOverPolkadotBulletinInstance, }; + use cumulus_primitives_core::UpwardMessageSender; // Random para id of sibling chain used in tests. pub const SIBLING_PEOPLE_PARACHAIN_ID: u32 = @@ -668,7 +670,7 @@ mod bridge_hub_bulletin_tests { _ => None, } }), - || (), + || ::ensure_successful_delivery(), ) } diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/tests/tests.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/tests/tests.rs index 704a20b58401f..7f5efbf295de0 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/tests/tests.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/tests/tests.rs @@ -40,6 +40,7 @@ use bridge_to_rococo_config::{ DeliveryRewardInBalance, WithBridgeHubRococoMessagesInstance, XcmOverBridgeHubRococoInstance, }; use codec::{Decode, Encode}; +use cumulus_primitives_core::UpwardMessageSender; use frame_support::{ assert_err, assert_ok, dispatch::GetDispatchInfo, @@ -315,7 +316,7 @@ fn message_dispatch_routing_works() { _ => None, } }), - || (), + || ::ensure_successful_delivery(), ) } diff --git a/cumulus/primitives/core/src/lib.rs b/cumulus/primitives/core/src/lib.rs index bc707bc251c87..3866cabed0257 100644 --- a/cumulus/primitives/core/src/lib.rs +++ b/cumulus/primitives/core/src/lib.rs @@ -154,12 +154,24 @@ pub trait UpwardMessageSender { /// Send the given UMP message; return the expected number of blocks before the message will /// be dispatched or an error if the message cannot be sent. /// return the hash of the message sent - fn send_upward_message(msg: UpwardMessage) -> Result<(u32, XcmHash), MessageSendError>; + fn send_upward_message(message: UpwardMessage) -> Result<(u32, XcmHash), MessageSendError>; + + /// Pre-check the given UMP message. + fn can_send_upward_message(message: &UpwardMessage) -> Result<(), MessageSendError>; + + /// Ensure `[Self::send_upward_message]` is successful when called in benchmarks/tests. + #[cfg(any(feature = "std", feature = "runtime-benchmarks", test))] + fn ensure_successful_delivery() {} } + impl UpwardMessageSender for () { - fn send_upward_message(_msg: UpwardMessage) -> Result<(u32, XcmHash), MessageSendError> { + fn send_upward_message(_message: UpwardMessage) -> Result<(u32, XcmHash), MessageSendError> { Err(MessageSendError::NoChannel) } + + fn can_send_upward_message(_message: &UpwardMessage) -> Result<(), MessageSendError> { + Err(MessageSendError::Other) + } } /// The status of a channel. diff --git a/cumulus/primitives/utility/src/lib.rs b/cumulus/primitives/utility/src/lib.rs index dd80335caaaab..60b925c69453b 100644 --- a/cumulus/primitives/utility/src/lib.rs +++ b/cumulus/primitives/utility/src/lib.rs @@ -77,6 +77,9 @@ where .map_err(|()| SendError::ExceedsMaxMessageSize)?; let data = versioned_xcm.encode(); + // Pre-check with our message sender if everything else is okay. + T::can_send_upward_message(&data).map_err(Self::map_upward_sender_err)?; + Ok((data, price)) } else { // Anything else is unhandled. This includes a message that is not meant for us. @@ -87,12 +90,24 @@ where } fn deliver(data: Vec) -> Result { - let (_, hash) = T::send_upward_message(data).map_err(|e| match e { + let (_, hash) = T::send_upward_message(data).map_err(Self::map_upward_sender_err)?; + Ok(hash) + } + + #[cfg(feature = "runtime-benchmarks")] + fn ensure_successful_delivery(location: Option) { + if location.as_ref().map_or(false, |l| l.contains_parents_only(1)) { + T::ensure_successful_delivery(); + } + } +} + +impl ParentAsUmp { + fn map_upward_sender_err(message_send_error: MessageSendError) -> SendError { + match message_send_error { MessageSendError::TooBig => SendError::ExceedsMaxMessageSize, e => SendError::Transport(e.into()), - })?; - - Ok(hash) + } } } @@ -596,12 +611,16 @@ mod test_xcm_router { } } - /// Impl [`UpwardMessageSender`] that return `Other` error - struct OtherErrorUpwardMessageSender; - impl UpwardMessageSender for OtherErrorUpwardMessageSender { + /// Impl [`UpwardMessageSender`] that return `Ok` for `can_send_upward_message`. + struct CanSendUpwardMessageSender; + impl UpwardMessageSender for CanSendUpwardMessageSender { fn send_upward_message(_: UpwardMessage) -> Result<(u32, XcmHash), MessageSendError> { Err(MessageSendError::Other) } + + fn can_send_upward_message(_: &UpwardMessage) -> Result<(), MessageSendError> { + Ok(()) + } } #[test] @@ -641,7 +660,7 @@ mod test_xcm_router { let dest = (Parent, Here); let mut dest_wrapper = Some(dest.clone().into()); let mut msg_wrapper = Some(message.clone()); - assert!( as SendXcm>::validate( + assert!( as SendXcm>::validate( &mut dest_wrapper, &mut msg_wrapper ) @@ -655,7 +674,7 @@ mod test_xcm_router { assert_eq!( Err(SendError::Transport("Other")), send_xcm::<( - ParentAsUmp, + ParentAsUmp, OkFixedXcmHashWithAssertingRequiredInputsSender )>(dest.into(), message) ); @@ -665,7 +684,7 @@ mod test_xcm_router { fn parent_as_ump_validate_nested_xcm_works() { let dest = Parent; - type Router = ParentAsUmp<(), (), ()>; + type Router = ParentAsUmp; // Message that is not too deeply nested: let mut good = Xcm(vec![ClearOrigin]); @@ -853,6 +872,9 @@ impl< return (None, None); } + // Ensure routers + XcmConfig::XcmSender::ensure_successful_delivery(Some(Location::parent())); + let mut fees_mode = None; if !XcmConfig::FeeManager::is_waived(Some(origin_ref), fee_reason) { // if not waived, we need to set up accounts for paying and receiving fees diff --git a/prdoc/pr_8409.prdoc b/prdoc/pr_8409.prdoc new file mode 100644 index 0000000000000..272f3a52eabb0 --- /dev/null +++ b/prdoc/pr_8409.prdoc @@ -0,0 +1,14 @@ +title: check XCM size in VMP routing +doc: +- audience: Runtime Dev + description: |- + This PR adds the ability for a UMP (para -> relay), when using `UpwardMessageSender` to also + check the size of the message within validate. This is exposed into the existing `validate` of + `ParentAsUmp`. +crates: +- name: cumulus-pallet-parachain-system + bump: minor +- name: cumulus-primitives-core + bump: minor +- name: cumulus-primitives-utility + bump: minor