Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
f5b9a70
Add debug message
raymondkfcheung May 14, 2025
e467f10
Merge branch 'master' into ray-check-weight-with-limit
raymondkfcheung May 14, 2025
06afe71
Test debug message
raymondkfcheung May 14, 2025
1710c7a
Merge branch 'master' into ray-check-weight-with-limit
raymondkfcheung May 15, 2025
5376e6e
Test debug message
raymondkfcheung May 15, 2025
a05f5f9
Update from github-actions[bot] running command 'prdoc --audience run…
github-actions[bot] May 15, 2025
977fdc4
Update PRDoc
raymondkfcheung May 15, 2025
bd42b82
Merge branch 'master' into ray-check-weight-with-limit
raymondkfcheung May 15, 2025
b286e8d
Merge branch 'master' into ray-check-weight-with-limit-v2
raymondkfcheung May 16, 2025
125da04
Use XcmError
raymondkfcheung May 16, 2025
6cba861
Merge branch 'master' into ray-check-weight-with-limit
raymondkfcheung May 16, 2025
341778b
Fix fmt
raymondkfcheung May 16, 2025
c1a89ef
Merge branch 'ray-check-weight-with-limit' into ray-check-weight-with…
raymondkfcheung May 16, 2025
67647c9
Return XcmError as Err
raymondkfcheung May 16, 2025
0dc29c3
Update from github-actions[bot] running command 'prdoc --audience run…
github-actions[bot] May 16, 2025
1d5152b
Update PRDoc
raymondkfcheung May 16, 2025
0688bca
Merge branch 'master' into ray-check-weight-with-limit-v2
raymondkfcheung May 16, 2025
79c34aa
Use inspect_err
raymondkfcheung May 16, 2025
8b8d37e
Merge branch 'master' into ray-check-weight-with-limit
raymondkfcheung May 16, 2025
47011f4
Merge branch 'master' into ray-check-weight-with-limit-v2
raymondkfcheung May 19, 2025
f86d355
Merge branch 'master' into ray-check-weight-with-limit-v2
raymondkfcheung May 19, 2025
c9b5af5
Merge branch 'master' into ray-check-weight-with-limit
raymondkfcheung May 19, 2025
2ea36a9
Make `WeightBounds` return `Result<Weight, XcmError>` to surface XCM …
raymondkfcheung May 22, 2025
9f2fa2b
Merge remote-tracking branch 'origin/ray-check-weight-with-limit' int…
raymondkfcheung May 22, 2025
1d5589e
Merge branch 'ray-check-weight-with-limit-v2' into ray-check-weight-w…
raymondkfcheung May 22, 2025
5014064
Merge branch 'master' into ray-check-weight-with-limit
raymondkfcheung May 22, 2025
e85d6f1
Update PRDoc
raymondkfcheung May 22, 2025
ff6eafa
Update PRDoc
raymondkfcheung May 22, 2025
3d5e4ff
Use generic errors as placeholders
raymondkfcheung May 22, 2025
f88c1dd
Merge branch 'master' into ray-check-weight-with-limit
raymondkfcheung May 27, 2025
5ec9559
Commit suggestions
raymondkfcheung May 27, 2025
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.

17 changes: 11 additions & 6 deletions polkadot/xcm/pallet-xcm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1437,8 +1437,10 @@ pub mod pallet {
ClaimAsset { assets, ticket },
DepositAsset { assets: AllCounted(number_of_assets).into(), beneficiary },
]);
let weight =
T::Weigher::weight(&mut message).map_err(|()| Error::<T>::UnweighableMessage)?;
let weight = T::Weigher::weight(&mut message).map_err(|error| {
tracing::debug!(target: "xcm::pallet_xcm::claim_assets", ?error, "Failed to calculate weight");
Error::<T>::UnweighableMessage
})?;
let mut hash = message.using_encoded(sp_io::hashing::blake2_256);
let outcome = T::XcmExecutor::prepare_and_execute(
origin_location,
Expand Down Expand Up @@ -2069,7 +2071,10 @@ impl<T: Config> Pallet<T> {
);

let weight =
T::Weigher::weight(&mut local_xcm).map_err(|()| Error::<T>::UnweighableMessage)?;
T::Weigher::weight(&mut local_xcm).map_err(|error| {
tracing::debug!(target: "xcm::pallet_xcm::execute_xcm_transfer", ?error, "Failed to calculate weight");
Error::<T>::UnweighableMessage
})?;
let mut hash = local_xcm.using_encoded(sp_io::hashing::blake2_256);
let outcome = T::XcmExecutor::prepare_and_execute(
origin.clone(),
Expand Down Expand Up @@ -2947,12 +2952,12 @@ impl<T: Config> Pallet<T> {
pub fn query_xcm_weight(message: VersionedXcm<()>) -> Result<Weight, XcmPaymentApiError> {
let message = Xcm::<()>::try_from(message.clone())
.map_err(|e| {
tracing::error!(target: "xcm::pallet_xcm::query_xcm_weight", ?e, ?message, "Failed to convert versioned message");
tracing::debug!(target: "xcm::pallet_xcm::query_xcm_weight", ?e, ?message, "Failed to convert versioned message");
XcmPaymentApiError::VersionedConversionFailed
})?;

T::Weigher::weight(&mut message.clone().into()).map_err(|()| {
tracing::error!(target: "xcm::pallet_xcm::query_xcm_weight", ?message, "Error when querying XCM weight");
T::Weigher::weight(&mut message.clone().into()).map_err(|error| {
tracing::debug!(target: "xcm::pallet_xcm::query_xcm_weight", ?error, ?message, "Error when querying XCM weight");
XcmPaymentApiError::WeightNotComputable
})
}
Expand Down
1 change: 1 addition & 0 deletions polkadot/xcm/xcm-builder/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ polkadot-primitives = { workspace = true, default-features = true }
polkadot-runtime-parachains = { workspace = true, default-features = true }
polkadot-test-runtime = { workspace = true }
primitive-types = { features = ["codec", "num-traits", "scale-info"], workspace = true }
sp-tracing = { features = ["test-utils"], workspace = true, default-features = true }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious why you add sp-tracing as a dev dependency not as a regular one?

xcm-simulator = { workspace = true, default-features = true }

[features]
Expand Down
74 changes: 52 additions & 22 deletions polkadot/xcm/xcm-builder/src/tests/weight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,30 +153,60 @@ fn errors_should_return_unused_weight() {

#[test]
fn weight_bounds_should_respect_instructions_limit() {
use sp_tracing::capture_test_logs;

sp_tracing::init_for_tests();
MaxInstructions::set(3);
let mut message = Xcm(vec![ClearOrigin; 4]);
// 4 instructions are too many.
assert_eq!(<TestConfig as Config>::Weigher::weight(&mut message), Err(()));

let mut message =
Xcm(vec![SetErrorHandler(Xcm(vec![ClearOrigin])), SetAppendix(Xcm(vec![ClearOrigin]))]);
// 4 instructions are too many, even when hidden within 2.
assert_eq!(<TestConfig as Config>::Weigher::weight(&mut message), Err(()));

let mut message =
Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(
vec![ClearOrigin],
))]))]))]);
// 4 instructions are too many, even when it's just one that's 3 levels deep.
assert_eq!(<TestConfig as Config>::Weigher::weight(&mut message), Err(()));

let mut message =
Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(vec![ClearOrigin]))]))]);
// 3 instructions are OK.
assert_eq!(
<TestConfig as Config>::Weigher::weight(&mut message),
Ok(Weight::from_parts(30, 30))
);
let log_capture = capture_test_logs!({
let mut message = Xcm(vec![ClearOrigin; 4]);
assert_eq!(
<TestConfig as Config>::Weigher::weight(&mut message),
Err(XcmError::ExceedsStackLimit)
);
});
assert!(log_capture.contains(
"Weight calculation failed for message error=ExceedsStackLimit instructions_left=3 message_length=4"
));

let log_capture = capture_test_logs!({
let mut message =
Xcm(vec![SetErrorHandler(Xcm(vec![ClearOrigin])), SetAppendix(Xcm(vec![ClearOrigin]))]);
// 4 instructions are too many, even when hidden within 2.
assert_eq!(
<TestConfig as Config>::Weigher::weight(&mut message),
Err(XcmError::ExceedsStackLimit)
);
});
assert!(log_capture.contains(
"Weight calculation failed for message error=ExceedsStackLimit instructions_left=0 message_length=2"
));

let log_capture = capture_test_logs!({
let mut message =
Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(
Xcm(vec![ClearOrigin]),
)]))]))]);
// 4 instructions are too many, even when it's just one that's 3 levels deep.
assert_eq!(
<TestConfig as Config>::Weigher::weight(&mut message),
Err(XcmError::ExceedsStackLimit)
);
});
assert!(log_capture.contains(
"Weight calculation failed for message error=ExceedsStackLimit instructions_left=0 message_length=1"
));

let log_capture = capture_test_logs!({
let mut message =
Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(vec![ClearOrigin]))]))]);
// 3 instructions are OK.
assert_eq!(
<TestConfig as Config>::Weigher::weight(&mut message),
Ok(Weight::from_parts(30, 30))
);
});
assert!(!log_capture.contains("Weight calculation failed for message"));
}

#[test]
Expand Down
89 changes: 69 additions & 20 deletions polkadot/xcm/xcm-builder/src/weight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,38 +38,61 @@ pub struct FixedWeightBounds<T, C, M>(PhantomData<(T, C, M)>);
impl<T: Get<Weight>, C: Decode + GetDispatchInfo, M: Get<u32>> WeightBounds<C>
for FixedWeightBounds<T, C, M>
{
fn weight(message: &mut Xcm<C>) -> Result<Weight, ()> {
fn weight(message: &mut Xcm<C>) -> Result<Weight, XcmError> {
tracing::trace!(target: "xcm::weight", ?message, "FixedWeightBounds");
let mut instructions_left = M::get();
Self::weight_with_limit(message, &mut instructions_left)
Self::weight_with_limit(message, &mut instructions_left).inspect_err(|&error| {
tracing::debug!(
target: "xcm::weight",
?error,
?instructions_left,
message_length = ?message.0.len(),
"Weight calculation failed for message"
);
})
}
fn instr_weight(instruction: &mut Instruction<C>) -> Result<Weight, ()> {
Self::instr_weight_with_limit(instruction, &mut u32::max_value())
fn instr_weight(instruction: &mut Instruction<C>) -> Result<Weight, XcmError> {
let mut max_value = u32::MAX;
Self::instr_weight_with_limit(instruction, &mut max_value).inspect_err(|&error| {
tracing::debug!(
target: "xcm::weight",
?error,
?instruction,
instrs_limit = ?max_value,
"Weight calculation failed for instruction"
);
})
}
}

impl<T: Get<Weight>, C: Decode + GetDispatchInfo, M> FixedWeightBounds<T, C, M> {
fn weight_with_limit(message: &mut Xcm<C>, instrs_limit: &mut u32) -> Result<Weight, ()> {
fn weight_with_limit(message: &mut Xcm<C>, instrs_limit: &mut u32) -> Result<Weight, XcmError> {
let mut r: Weight = Weight::zero();
*instrs_limit = instrs_limit.checked_sub(message.0.len() as u32).ok_or(())?;
*instrs_limit = instrs_limit
.checked_sub(message.0.len() as u32)
.ok_or_else(|| XcmError::ExceedsStackLimit)?;
for instruction in message.0.iter_mut() {
r = r
.checked_add(&Self::instr_weight_with_limit(instruction, instrs_limit)?)
.ok_or(())?;
.ok_or_else(|| XcmError::Overflow)?;
}
Ok(r)
}
fn instr_weight_with_limit(
instruction: &mut Instruction<C>,
instrs_limit: &mut u32,
) -> Result<Weight, ()> {
) -> Result<Weight, XcmError> {
let instr_weight = match instruction {
Transact { ref mut call, .. } => call.ensure_decoded()?.get_dispatch_info().call_weight,
Transact { ref mut call, .. } =>
call.ensure_decoded()
.map_err(|_| XcmError::FailedToDecode)?
.get_dispatch_info()
.call_weight,
SetErrorHandler(xcm) | SetAppendix(xcm) | ExecuteWithOrigin { xcm, .. } =>
Self::weight_with_limit(xcm, instrs_limit)?,
_ => Weight::zero(),
};
T::get().checked_add(&instr_weight).ok_or(())
T::get().checked_add(&instr_weight).ok_or_else(|| XcmError::Overflow)
}
}

Expand All @@ -81,13 +104,30 @@ where
M: Get<u32>,
Instruction<C>: xcm::latest::GetWeight<W>,
{
fn weight(message: &mut Xcm<C>) -> Result<Weight, ()> {
fn weight(message: &mut Xcm<C>) -> Result<Weight, XcmError> {
tracing::trace!(target: "xcm::weight", ?message, "WeightInfoBounds");
let mut instructions_left = M::get();
Self::weight_with_limit(message, &mut instructions_left)
Self::weight_with_limit(message, &mut instructions_left).inspect_err(|&error| {
tracing::debug!(
target: "xcm::weight",
?error,
?instructions_left,
message_length = ?message.0.len(),
"Weight calculation failed for message"
);
})
}
fn instr_weight(instruction: &mut Instruction<C>) -> Result<Weight, ()> {
Self::instr_weight_with_limit(instruction, &mut u32::max_value())
fn instr_weight(instruction: &mut Instruction<C>) -> Result<Weight, XcmError> {
let mut max_value = u32::MAX;
Self::instr_weight_with_limit(instruction, &mut max_value).inspect_err(|&error| {
tracing::debug!(
target: "xcm::weight",
?error,
?instruction,
instrs_limit = ?max_value,
"Weight calculation failed for instruction"
);
})
}
}

Expand All @@ -98,26 +138,35 @@ where
M: Get<u32>,
Instruction<C>: xcm::latest::GetWeight<W>,
{
fn weight_with_limit(message: &mut Xcm<C>, instrs_limit: &mut u32) -> Result<Weight, ()> {
fn weight_with_limit(message: &mut Xcm<C>, instrs_limit: &mut u32) -> Result<Weight, XcmError> {
let mut r: Weight = Weight::zero();
*instrs_limit = instrs_limit.checked_sub(message.0.len() as u32).ok_or(())?;
*instrs_limit = instrs_limit
.checked_sub(message.0.len() as u32)
.ok_or_else(|| XcmError::ExceedsStackLimit)?;
for instruction in message.0.iter_mut() {
r = r
.checked_add(&Self::instr_weight_with_limit(instruction, instrs_limit)?)
.ok_or(())?;
.ok_or_else(|| XcmError::Overflow)?;
}
Ok(r)
}
fn instr_weight_with_limit(
instruction: &mut Instruction<C>,
instrs_limit: &mut u32,
) -> Result<Weight, ()> {
) -> Result<Weight, XcmError> {
let instr_weight = match instruction {
Transact { ref mut call, .. } => call.ensure_decoded()?.get_dispatch_info().call_weight,
Transact { ref mut call, .. } =>
call.ensure_decoded()
.map_err(|_| XcmError::FailedToDecode)?
.get_dispatch_info()
.call_weight,
SetErrorHandler(xcm) | SetAppendix(xcm) => Self::weight_with_limit(xcm, instrs_limit)?,
_ => Weight::zero(),
};
instruction.weight().checked_add(&instr_weight).ok_or(())
instruction
.weight()
.checked_add(&instr_weight)
.ok_or_else(|| XcmError::Overflow)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XcmError::Overflow will be changed to the proper error enum from #7730 when available, as discussed.

}
}

Expand Down
30 changes: 27 additions & 3 deletions polkadot/xcm/xcm-executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,15 @@ impl<Config: config::Config> ExecuteXcm<Config::RuntimeCall> for XcmExecutor<Con
) -> Result<Self::Prepared, Xcm<Config::RuntimeCall>> {
match Config::Weigher::weight(&mut message) {
Ok(weight) => Ok(WeighedMessage(weight, message)),
Err(_) => Err(message),
Err(error) => {
tracing::debug!(
target: "xcm::prepare",
?error,
?message,
"Failed to calculate weight for XCM message; execution aborted"
);
Err(message)
},
}
}
fn execute(
Expand Down Expand Up @@ -1426,15 +1434,31 @@ impl<Config: config::Config> XcmExecutor<Config> {
RefundSurplus => self.refund_surplus(),
SetErrorHandler(mut handler) => {
let handler_weight = Config::Weigher::weight(&mut handler)
.map_err(|()| XcmError::WeightNotComputable)?;
.map_err(|error| {
tracing::debug!(
target: "xcm::executor::SetErrorHandler",
?error,
?handler,
"Failed to calculate weight"
);
XcmError::WeightNotComputable
})?;
self.total_surplus.saturating_accrue(self.error_handler_weight);
self.error_handler = handler;
self.error_handler_weight = handler_weight;
Ok(())
},
SetAppendix(mut appendix) => {
let appendix_weight = Config::Weigher::weight(&mut appendix)
.map_err(|()| XcmError::WeightNotComputable)?;
.map_err(|error| {
tracing::debug!(
target: "xcm::executor::SetErrorHandler",
?error,
?appendix,
"Failed to calculate weight"
);
XcmError::WeightNotComputable
})?;
self.total_surplus.saturating_accrue(self.appendix_weight);
self.appendix = appendix;
self.appendix_weight = appendix_weight;
Expand Down
4 changes: 2 additions & 2 deletions polkadot/xcm/xcm-executor/src/tests/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,11 @@ impl GetDispatchInfo for TestCall {
/// Test weigher that just returns a fixed weight for every program.
pub struct TestWeigher;
impl<C> WeightBounds<C> for TestWeigher {
fn weight(_message: &mut Xcm<C>) -> Result<Weight, ()> {
fn weight(_message: &mut Xcm<C>) -> Result<Weight, XcmError> {
Ok(Weight::from_parts(2, 2))
}

fn instr_weight(_instruction: &mut Instruction<C>) -> Result<Weight, ()> {
fn instr_weight(_instruction: &mut Instruction<C>) -> Result<Weight, XcmError> {
Ok(Weight::from_parts(2, 2))
}
}
Expand Down
4 changes: 2 additions & 2 deletions polkadot/xcm/xcm-executor/src/traits/weight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ use xcm::latest::{prelude::*, Weight};
pub trait WeightBounds<RuntimeCall> {
/// Return the maximum amount of weight that an attempted execution of this message could
/// consume.
fn weight(message: &mut Xcm<RuntimeCall>) -> Result<Weight, ()>;
fn weight(message: &mut Xcm<RuntimeCall>) -> Result<Weight, XcmError>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this return the index of the problematic instruction as well? It'd be very useful to bubble it up in #7730


/// Return the maximum amount of weight that an attempted execution of this instruction could
/// consume.
fn instr_weight(instruction: &mut Instruction<RuntimeCall>) -> Result<Weight, ()>;
fn instr_weight(instruction: &mut Instruction<RuntimeCall>) -> Result<Weight, XcmError>;
}

/// Charge for weight in order to execute XCM.
Expand Down
12 changes: 12 additions & 0 deletions prdoc/pr_8535.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
title: Make `WeightBounds` return `XcmError` to surface failures
doc:
- audience: Runtime Dev
description: |-
Improved XCM weight calculation error handling and traceability. The `WeightBounds` trait now returns detailed `XcmError` types instead of opaque results, allowing downstream consumers to access specific error context for failures like instruction decoding issues, weight overflows, and instruction limit violations. Added structured debug logging with contextual information to aid in diagnosing weight estimation failures during message preparation and execution.
crates:
- name: pallet-xcm
bump: patch
- name: staging-xcm-builder
bump: major
- name: staging-xcm-executor
bump: major
Loading