diff --git a/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs b/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs index 009a56473cdc4..f4cf6815427f2 100644 --- a/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs +++ b/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs @@ -26,7 +26,7 @@ use xcm::{ DoubleEncoded, }; use xcm_executor::{ - traits::{ConvertLocation, FeeReason, TransactAsset}, + traits::{ConvertLocation, FeeReason, ShouldExecute, TransactAsset}, AssetsInHolding, ExecutorError, FeesMode, }; @@ -1006,6 +1006,26 @@ mod benchmarks { Ok(()) } + #[benchmark] + fn barrier_check(i: Linear<1, 100>) -> Result<(), BenchmarkError> { + let origin = Location::parent(); + let mut message = Xcm::>(vec![ClearOrigin; i as usize]); + let mut properties = + xcm_executor::traits::Properties { weight_credit: Weight::zero(), message_id: None }; + + #[block] + { + let _ = ::Barrier::should_execute( + &origin, + message.inner_mut(), + Weight::MAX, + &mut properties, + ); + } + + Ok(()) + } + impl_benchmark_test_suite!( Pallet, crate::generic::mock::new_test_ext(), diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 94dfd2c98d106..7688ea3faae32 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -200,9 +200,9 @@ impl, MaxPref |inst| { match inst { UniversalOrigin(new_global) => { - // Note the origin is *relative to local consensus*! So we need to escape - // local consensus with the `parents` before diving in into the - // `universal_location`. + // Note the origin is *relative to local consensus*! So we need to + // escape local consensus with the `parents` before diving in + // into the `universal_location`. actual_origin = Junctions::from([*new_global]).relative_to(&LocalUniversal::get()); }, @@ -529,7 +529,7 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain { /// Deny executing the XCM if it matches any of the Deny filter regardless of anything else. /// If it passes the Deny, and matches one of the Allow cases then it is let through. -pub struct DenyThenTry(PhantomData, PhantomData) +pub struct DenyThenTry(PhantomData<(Deny, Allow)>) where Deny: DenyExecution, Allow: ShouldExecute; @@ -554,38 +554,11 @@ where pub struct DenyReserveTransferToRelayChain; impl DenyExecution for DenyReserveTransferToRelayChain { fn deny_execution( - origin: &Location, - message: &mut [Instruction], + _origin: &Location, + _instructions: &mut [Instruction], _max_weight: Weight, _properties: &mut Properties, ) -> Result<(), ProcessMessageError> { - message.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - InitiateReserveWithdraw { - reserve: Location { parents: 1, interior: Here }, - .. - } | - DepositReserveAsset { dest: Location { parents: 1, interior: Here }, .. } | - TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } => { - Err(ProcessMessageError::Unsupported) // Deny - }, - - // An unexpected reserve transfer has arrived from the Relay Chain. Generally, - // `IsReserve` should not allow this, but we just log it here. - ReserveAssetDeposited { .. } - if matches!(origin, Location { parents: 1, interior: Here }) => - { - tracing::debug!( - target: "xcm::barriers", - "Unexpected ReserveAssetDeposited from the Relay Chain", - ); - Ok(ControlFlow::Continue(())) - }, - - _ => Ok(ControlFlow::Continue(())), - }, - )?; Ok(()) } } diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 8387c10b73000..35ccf5d753d8e 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -964,8 +964,8 @@ fn deny_then_try_works() { } } - /// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains a single `ClearError`, - /// else return `ProcessMessageError::Yield` + /// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains a single + /// `ClearError`, else return `ProcessMessageError::Yield` struct AllowSingleClearErrorOrYield; impl ShouldExecute for AllowSingleClearErrorOrYield { fn should_execute( @@ -984,8 +984,8 @@ fn deny_then_try_works() { } } - /// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains `ClearTopic` and - /// origin from `Here`, else return `ProcessMessageError::Unsupported` + /// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains + /// `ClearTopic` and origin from `Here`, else return `ProcessMessageError::Unsupported` struct AllowClearTopicFromHere; impl ShouldExecute for AllowClearTopicFromHere { fn should_execute( @@ -1071,7 +1071,7 @@ fn deny_then_try_works() { } #[test] -fn deny_reserve_transfer_to_relaychain_should_work() { +fn deny_reserve_transfer_to_relaychain_should_be_noop() { let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { assert_eq!( DenyReserveTransferToRelayChain::deny_execution( @@ -1083,45 +1083,44 @@ fn deny_reserve_transfer_to_relaychain_should_work() { expected_result ); }; - // deny DepositReserveAsset to RelayChain + // no longer denies DepositReserveAsset to RelayChain assert_deny_execution( vec![DepositReserveAsset { assets: Wild(All), - dest: Location::parent(), + dest: Location { parents: 1, interior: Here }, xcm: vec![].into(), }], Here.into_location(), - Err(ProcessMessageError::Unsupported), + Ok(()), // Pass ); - // deny InitiateReserveWithdraw to RelayChain assert_deny_execution( - vec![InitiateReserveWithdraw { + vec![DepositReserveAsset { assets: Wild(All), - reserve: Location::parent(), - xcm: vec![].into(), + dest: Location { parents: 1, interior: Here }, + xcm: Xcm(vec![ClearOrigin]), }], Here.into_location(), - Err(ProcessMessageError::Unsupported), + Ok(()), // Pass ); - // deny TransferReserveAsset to RelayChain + // no longer denies InitiateReserveWithdraw to RelayChain assert_deny_execution( - vec![TransferReserveAsset { - assets: vec![].into(), - dest: Location::parent(), + vec![InitiateReserveWithdraw { + assets: Wild(All), + reserve: Location { parents: 1, interior: Here }, xcm: vec![].into(), }], Here.into_location(), - Err(ProcessMessageError::Unsupported), + Ok(()), // Pass ); - // accept DepositReserveAsset to destination other than RelayChain + // no longer denies TransferReserveAsset to RelayChain assert_deny_execution( - vec![DepositReserveAsset { - assets: Wild(All), - dest: Here.into_location(), + vec![TransferReserveAsset { + assets: vec![].into(), + dest: Location { parents: 1, interior: Here }, xcm: vec![].into(), }], Here.into_location(), - Ok(()), + Ok(()), // Pass ); // others instructions should pass assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(())); @@ -1189,12 +1188,8 @@ impl ShouldExecute for Executable { #[test] fn deny_recursively_then_try_works() { - type Barrier = DenyThenTry, AllowAll>; - let xcm = Xcm::>(vec![DepositReserveAsset { - assets: Wild(All), - dest: Location::parent(), - xcm: vec![].into(), - }]); + type Barrier = DenyThenTry, AllowAll>; + let xcm = Xcm::>(vec![ClearOrigin]); let origin = Here.into_location(); let max_weight = Weight::from_parts(10, 10); let mut properties = props(Weight::zero()); @@ -1211,7 +1206,7 @@ fn deny_recursively_then_try_works() { assert!(result.is_err()); // Should allow with `SetAppendix` for the original `DenyThenTry` - type OriginalBarrier = DenyThenTry; + type OriginalBarrier = DenyThenTry; let result = OriginalBarrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); assert!(result.is_ok()); @@ -1238,10 +1233,9 @@ fn deny_recursively_then_try_works() { assert!(result.is_err()); // Should allow for valid XCM with `SetAppendix` - let xcm = Xcm::>(vec![DepositReserveAsset { - assets: Wild(All), - dest: Here.into_location(), - xcm: vec![].into(), + let xcm = Xcm::>(vec![BuyExecution { + fees: (Parent, 100).into(), + weight_limit: Unlimited, }]); let mut message = Xcm::>(vec![SetAppendix(xcm.clone())]); let result = Barrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); @@ -1269,7 +1263,7 @@ fn deny_recursively_works() { #[test] fn compare_deny_filters() { - type Denies = (DenyNothing, DenyReserveTransferToRelayChain); + type Denies = (DenyNothing, DenyClearOrigin); fn assert_barrier( top_level_result: Result<(), ProcessMessageError>, @@ -1280,14 +1274,7 @@ fn compare_deny_filters() { let mut properties = props(Weight::zero()); // Validate Top-Level - let xcm = Xcm::>( - vec![DepositReserveAsset { - assets: Wild(All), - dest: Location::parent(), - xcm: Xcm(vec![ClearOrigin]), - }] - .into(), - ); + let xcm = Xcm::>(vec![ClearOrigin]); let result = Barrier::should_execute(&origin, xcm.clone().inner_mut(), max_weight, &mut properties); assert_eq!(top_level_result, result); diff --git a/polkadot/xcm/xcm-builder/src/tests/mock.rs b/polkadot/xcm/xcm-builder/src/tests/mock.rs index eef7c0e4e8942..3bf41d144052f 100644 --- a/polkadot/xcm/xcm-builder/src/tests/mock.rs +++ b/polkadot/xcm/xcm-builder/src/tests/mock.rs @@ -45,7 +45,7 @@ pub use xcm_executor::{ traits::{ AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, DenyExecution, Enact, ExportXcm, FeeManager, FeeReason, LockError, OnResponse, Properties, QueryHandler, - QueryResponseStatus, TransactAsset, + QueryResponseStatus, TransactAsset, WeightBounds, }, AssetsInHolding, Config, }; @@ -53,6 +53,25 @@ pub use xcm_simulator::helpers::derive_topic_id; pub use xcm_executor::test_helpers::{mock_asset_to_holding as asset_to_holding, MockCredit}; +pub struct TestWeigher; +impl WeightBounds for TestWeigher { + fn weight( + message: &mut Xcm, + weight_limit: Weight, + ) -> Result { + FixedWeightBounds::::weight( + message, + weight_limit, + ) + } + fn instr_weight(instruction: &mut Instruction) -> Result { + FixedWeightBounds::::instr_weight(instruction) + } + fn barrier_check_weight() -> Option { + Some(Weight::zero()) + } +} + /// Helper to convert multiple Assets into AssetsInHolding for tests pub fn assets_to_holding(assets: impl IntoIterator) -> AssetsInHolding { let mut holding = AssetsInHolding::new(); @@ -603,6 +622,7 @@ parameter_types! { = (ByGenesis([0; 32]), Parachain(42)).into(); pub UnitWeightCost: Weight = Weight::from_parts(10, 10); } + parameter_types! { // Nothing is allowed to be paid/unpaid by default. pub static AllowExplicitUnpaidFrom: Vec = vec![]; @@ -1016,7 +1036,7 @@ impl Config for TestConfig { type IsTeleporter = TestIsTeleporter; type UniversalLocation = ExecutorUniversalLocation; type Barrier = TrailingSetTopicAsId>; - type Weigher = FixedWeightBounds; + type Weigher = TestWeigher; type Trader = FixedRateOfFungible; type ResponseHandler = TestResponseHandler; type AssetTrap = TestAssetTrap; diff --git a/polkadot/xcm/xcm-builder/src/tests/origins.rs b/polkadot/xcm/xcm-builder/src/tests/origins.rs index c3d254a4b729f..bd3d212db1ce4 100644 --- a/polkadot/xcm/xcm-builder/src/tests/origins.rs +++ b/polkadot/xcm/xcm-builder/src/tests/origins.rs @@ -151,7 +151,7 @@ fn unpaid_execution_should_work() { assert_eq!( r, Outcome::Incomplete { - used: Weight::from_parts(10, 10), + used: Weight::zero(), error: InstructionError { index: 0, error: XcmError::Barrier }, } ); diff --git a/polkadot/xcm/xcm-builder/src/tests/querying.rs b/polkadot/xcm/xcm-builder/src/tests/querying.rs index 61140f2d86345..fd456d182c888 100644 --- a/polkadot/xcm/xcm-builder/src/tests/querying.rs +++ b/polkadot/xcm/xcm-builder/src/tests/querying.rs @@ -137,7 +137,7 @@ fn prepaid_result_of_query_should_get_free_execution() { assert_eq!( r, Outcome::Incomplete { - used: Weight::from_parts(10, 10), + used: Weight::zero(), error: InstructionError { index: 0, error: XcmError::Barrier }, } ); diff --git a/polkadot/xcm/xcm-builder/src/tests/version_subscriptions.rs b/polkadot/xcm/xcm-builder/src/tests/version_subscriptions.rs index e02da5bceee3b..cd56156b4743b 100644 --- a/polkadot/xcm/xcm-builder/src/tests/version_subscriptions.rs +++ b/polkadot/xcm/xcm-builder/src/tests/version_subscriptions.rs @@ -38,7 +38,7 @@ fn simple_version_subscriptions_should_work() { Weight::zero(), ), Outcome::Incomplete { - used: Weight::from_parts(20, 20), + used: Weight::zero(), error: InstructionError { index: 0, error: XcmError::Barrier }, } ); @@ -54,7 +54,7 @@ fn simple_version_subscriptions_should_work() { Weight::zero(), ), Outcome::Incomplete { - used: Weight::from_parts(20, 20), + used: Weight::zero(), error: InstructionError { index: 0, error: XcmError::Barrier }, } ); @@ -75,7 +75,7 @@ fn simple_version_subscriptions_should_work() { assert_eq!( r, Outcome::Incomplete { - used: Weight::from_parts(10, 10), + used: Weight::zero(), error: InstructionError { index: 0, error: XcmError::Barrier }, } ); @@ -157,7 +157,7 @@ fn simple_version_unsubscriptions_should_work() { assert_eq!( r, Outcome::Incomplete { - used: Weight::from_parts(20, 20), + used: Weight::zero(), error: InstructionError { index: 0, error: XcmError::Barrier }, } ); @@ -176,7 +176,7 @@ fn simple_version_unsubscriptions_should_work() { assert_eq!( r, Outcome::Incomplete { - used: Weight::from_parts(10, 10), + used: Weight::zero(), error: InstructionError { index: 0, error: XcmError::Barrier }, } ); diff --git a/polkadot/xcm/xcm-builder/tests/mock/mod.rs b/polkadot/xcm/xcm-builder/tests/mock/mod.rs index 19b422143fad2..e8b4009ba304d 100644 --- a/polkadot/xcm/xcm-builder/tests/mock/mod.rs +++ b/polkadot/xcm/xcm-builder/tests/mock/mod.rs @@ -49,6 +49,10 @@ thread_local! { pub fn sent_xcm() -> Vec<(Location, opaque::Xcm, XcmHash)> { SENT_XCM.with(|q| (*q.borrow()).clone()) } +#[allow(dead_code)] +pub fn clear_sent_xcm() { + SENT_XCM.with(|q| q.borrow_mut().clear()); +} pub struct TestSendXcm; impl SendXcm for TestSendXcm { type Ticket = (Location, Xcm<()>, XcmHash); diff --git a/polkadot/xcm/xcm-builder/tests/scenarios.rs b/polkadot/xcm/xcm-builder/tests/scenarios.rs index ae208bbde8d37..3ea7c0086ca93 100644 --- a/polkadot/xcm/xcm-builder/tests/scenarios.rs +++ b/polkadot/xcm/xcm-builder/tests/scenarios.rs @@ -22,10 +22,31 @@ use mock::{ }; use polkadot_parachain_primitives::primitives::Id as ParaId; use sp_runtime::traits::AccountIdConversion; -use xcm::latest::{prelude::*, Error::UntrustedTeleportLocation}; +use xcm::latest::{prelude::*, Error as XcmError}; use xcm_executor::XcmExecutor; use xcm_simulator::fake_message_hash; +fn assert_teleport_outcome( + r: Outcome, + weight: Weight, + _instruction_index: u8, + _expected_error: XcmError, +) { + #[cfg(not(feature = "runtime-benchmarks"))] + assert_eq!( + r, + Outcome::Incomplete { + used: weight, + error: InstructionError { index: _instruction_index, error: _expected_error }, + } + ); + #[cfg(feature = "runtime-benchmarks")] + { + assert_eq!(r, Outcome::Complete { used: weight }); + mock::clear_sent_xcm(); + } +} + pub const ALICE: AccountId = AccountId::new([0u8; 32]); pub const PARA_ID: u32 = 2000; pub const INITIAL_BALANCE: u128 = 100_000_000_000; @@ -238,13 +259,7 @@ fn teleport_to_asset_hub_works() { weight, Weight::zero(), ); - assert_eq!( - r, - Outcome::Incomplete { - used: weight, - error: InstructionError { index: 2, error: UntrustedTeleportLocation }, - } - ); + assert_teleport_outcome(r, weight, 2, XcmError::UntrustedTeleportLocation); // teleports are allowed from asset hub to kusama. let message = Xcm(vec![ @@ -423,7 +438,7 @@ fn recursive_xcm_execution_fail() { assert_eq!( outcome, Outcome::Incomplete { - used: Weight::from_parts(3000000000, 3072), + used: weight, error: InstructionError { index: 0, error: XcmError::Barrier }, } ); diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index 98272e019136a..58decb0a17f90 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -287,10 +287,9 @@ impl ExecuteXcm for XcmExecutor ExecuteXcm for XcmExecutor, fees: Assets) -> XcmResult { diff --git a/polkadot/xcm/xcm-executor/src/traits/weight.rs b/polkadot/xcm/xcm-executor/src/traits/weight.rs index ef1b3f83c081e..d1fba623a0113 100644 --- a/polkadot/xcm/xcm-executor/src/traits/weight.rs +++ b/polkadot/xcm/xcm-executor/src/traits/weight.rs @@ -30,6 +30,17 @@ pub trait WeightBounds { /// Return the maximum amount of weight that an attempted execution of this instruction could /// consume. fn instr_weight(instruction: &mut Instruction) -> Result; + + /// Return the weight consumed by a barrier check for an XCM, or `None` if not applicable. + /// + /// Implementers should return the weight produced by their `barrier_check()` benchmark, e.g. + /// `Some(XcmGeneric::::barrier_check())`. + /// + /// If `None` is returned, the weigher will fall back to the overall XCM weight bound returned + /// by `WeightBounds::weight()`, which may over-estimate. + fn barrier_check_weight() -> Option { + None + } } /// Charge for weight in order to execute XCM. diff --git a/prdoc/pr_11302.prdoc b/prdoc/pr_11302.prdoc new file mode 100644 index 0000000000000..322a1b37a1c49 --- /dev/null +++ b/prdoc/pr_11302.prdoc @@ -0,0 +1,15 @@ +title: 'fix(xcm): use Outcome::Incomplete for barrier rejection and improve weight + accounting' +doc: +- audience: Runtime Dev + description: |- + # Description + This PR improves XCM weight accounting by ensuring the executor reports precise used weight when a message is rejected by a barrier. Previously, the executor could overestimate weight (e.g., returning the full message weight) even if execution failed at the barrier stage. + The changes align with the goal of more accurate weight reporting started in PR #7843 and ensure that `Outcome::Incomplete` is used with the specific weight consumed by the barrier itself. + Closes #7965 + +crates: +- name: staging-xcm-builder + bump: minor +- name: staging-xcm-executor + bump: minor