From a86ed5e9e37e89938775f68fc7272d71dfc323e9 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 18 Apr 2024 18:49:14 +0300 Subject: [PATCH 1/8] [XCM] Treat recursion limit error as transient in the MQ Signed-off-by: Oliver Tale-Yazdi --- polkadot/xcm/xcm-builder/src/barriers.rs | 6 +-- .../xcm-builder/src/process_xcm_message.rs | 49 +++++++++++++++++-- .../xcm/xcm-builder/src/tests/barriers.rs | 12 ++--- substrate/frame/message-queue/src/lib.rs | 30 ++++++------ substrate/frame/message-queue/src/mock.rs | 4 +- .../frame/message-queue/src/mock_helpers.rs | 2 +- .../frame/support/src/traits/messages.rs | 4 +- 7 files changed, 76 insertions(+), 31 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index c0b328f38e96..232e160d5b33 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -47,7 +47,7 @@ impl ShouldExecute for TakeWeightCredit { properties.weight_credit = properties .weight_credit .checked_sub(&max_weight) - .ok_or(ProcessMessageError::Overweight(max_weight))?; + .ok_or(ProcessMessageError::Overweight(Some(max_weight)))?; Ok(()) } } @@ -104,7 +104,7 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFrom *weight_limit = Limited(max_weight); Ok(()) }, - _ => Err(ProcessMessageError::Overweight(max_weight)), + _ => Err(ProcessMessageError::Overweight(Some(max_weight))), })?; Ok(()) } @@ -304,7 +304,7 @@ impl> ShouldExecute for AllowExplicitUnpaidExecutionFrom Ok(()), UnpaidExecution { weight_limit: Unlimited, .. } => Ok(()), - _ => Err(ProcessMessageError::Overweight(max_weight)), + _ => Err(ProcessMessageError::Overweight(Some(max_weight))), })?; Ok(()) } diff --git a/polkadot/xcm/xcm-builder/src/process_xcm_message.rs b/polkadot/xcm/xcm-builder/src/process_xcm_message.rs index bcf91d8e68c3..3816daf78340 100644 --- a/polkadot/xcm/xcm-builder/src/process_xcm_message.rs +++ b/polkadot/xcm/xcm-builder/src/process_xcm_message.rs @@ -77,7 +77,7 @@ impl< meter.remaining(), ); - return Err(ProcessMessageError::Overweight(required)) + return Err(ProcessMessageError::Overweight(Some(required))) } let (consumed, result) = match XcmExecutor::execute(origin.into(), pre, id, Weight::zero()) @@ -102,7 +102,12 @@ impl< target: LOG_TARGET, "XCM message execution error: {error:?}", ); - (required, Err(ProcessMessageError::Unsupported)) + let error = match error { + xcm::latest::Error::ExceedsStackLimit => ProcessMessageError::Overweight(None), + _ => ProcessMessageError::Unsupported, + }; + + (required, Err(error)) }, }; meter.consume(consumed); @@ -159,7 +164,7 @@ mod tests { let mut id = [0; 32]; assert_err!( Processor::process_message(msg, ORIGIN, meter, &mut id), - Overweight(1000.into()) + Overweight(Some(1000.into())) ); assert_eq!(meter.consumed(), 0.into()); } @@ -172,6 +177,44 @@ mod tests { } } + #[test] + fn process_message_recursion_limit_exceeded_fails() { + use frame_support::__private::sp_tracing; + sp_tracing::try_init_simple(); + let mut xcm = VersionedXcm::V4(xcm::v4::Xcm::(vec![ + BuyExecution { fees: (Here, 1).into(), weight_limit: Unlimited }, + xcm::v4::Instruction::::Transact { + origin_kind: xcm::v4::OriginKind::Xcm, + require_weight_at_most: Weight::zero(), + call: RuntimeCall::System(frame_system::Call::::remark_with_event { + remark: b"Hello, World!".to_vec(), + }) + .encode() + .into(), + }, + ])); + + for _ in 0..300 { + let call = RuntimeCall::Xcm(pallet_xcm::Call::::execute { + message: xcm.into(), + max_weight: Weight::MAX, + }); + xcm = VersionedXcm::V4(xcm::v4::Xcm::(vec![xcm::v4::Instruction::< + RuntimeCall, + >::Transact { + origin_kind: xcm::v4::OriginKind::Xcm, + require_weight_at_most: Weight::zero(), + call: call.encode().into(), + }])); + } + + let msg = &xcm.encode()[..]; + let meter = &mut WeightMeter::new(); + let mut id = [0; 32]; + // FAIL-CI why does this work? + assert_ok!(Processor::process_message(msg, ORIGIN, meter, &mut id)); + } + fn v2_xcm(success: bool) -> VersionedXcm { let instr = if success { v3::Instruction::::ClearOrigin diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 6516263f57a0..823b64ea4b0c 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -42,7 +42,7 @@ fn take_weight_credit_barrier_should_work() { Weight::from_parts(10, 10), &mut properties, ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(10, 10)))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(10, 10))))); assert_eq!(properties.weight_credit, Weight::zero()); } @@ -163,7 +163,7 @@ fn allow_explicit_unpaid_should_work() { Weight::from_parts(20, 20), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20)))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(20, 20))))); let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( &Parent.into(), @@ -171,7 +171,7 @@ fn allow_explicit_unpaid_should_work() { Weight::from_parts(20, 20), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20)))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(20, 20))))); let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( &Parent.into(), @@ -195,7 +195,7 @@ fn allow_explicit_unpaid_should_work() { Weight::from_parts(20, 20), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20)))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(20, 20))))); let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( &Parent.into(), @@ -234,7 +234,7 @@ fn allow_paid_should_work() { Weight::from_parts(30, 30), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(30, 30)))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(30, 30))))); let fees = (Parent, 1).into(); let mut paying_message = Xcm::<()>(vec![ @@ -272,7 +272,7 @@ fn allow_paid_should_work() { Weight::from_parts(20, 20), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20)))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(20, 20))))); let r = AllowTopLevelPaidExecutionFrom::>::should_execute( &Parent.into(), diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index ec85c785f79e..2e5a2a5292ed 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -1437,20 +1437,22 @@ impl Pallet { let prev_consumed = meter.consumed(); match T::MessageProcessor::process_message(message, origin.clone(), meter, &mut id) { - Err(Overweight(w)) if w.any_gt(overweight_limit) => { - // Permanently overweight. - Self::deposit_event(Event::::OverweightEnqueued { - id, - origin, - page_index, - message_index, - }); - MessageExecutionStatus::Overweight - }, - Err(Overweight(_)) => { - // Temporarily overweight - save progress and stop processing this - // queue. - MessageExecutionStatus::InsufficientWeight + Err(Overweight(required)) => { + if required.map_or(true, |r| r.any_gt(overweight_limit)) { + // Permanently overweight. + Self::deposit_event(Event::::OverweightEnqueued { + id, + origin, + page_index, + message_index, + }); + + MessageExecutionStatus::Overweight + } else { + // Temporarily overweight - save progress and stop processing this + // queue. + MessageExecutionStatus::InsufficientWeight + } }, Err(Yield) => { // Processing should be reattempted later. diff --git a/substrate/frame/message-queue/src/mock.rs b/substrate/frame/message-queue/src/mock.rs index 1281de6b0a66..d14a152ba324 100644 --- a/substrate/frame/message-queue/src/mock.rs +++ b/substrate/frame/message-queue/src/mock.rs @@ -191,7 +191,7 @@ impl ProcessMessage for RecordingMessageProcessor { MessagesProcessed::set(m); Ok(true) } else { - Err(ProcessMessageError::Overweight(required)) + Err(ProcessMessageError::Overweight(Some(required))) } } } @@ -254,7 +254,7 @@ impl ProcessMessage for CountingMessageProcessor { NumMessagesProcessed::set(NumMessagesProcessed::get() + 1); Ok(true) } else { - Err(ProcessMessageError::Overweight(required)) + Err(ProcessMessageError::Overweight(Some(required))) } } } diff --git a/substrate/frame/message-queue/src/mock_helpers.rs b/substrate/frame/message-queue/src/mock_helpers.rs index 28395e27cdd2..3f2e13628003 100644 --- a/substrate/frame/message-queue/src/mock_helpers.rs +++ b/substrate/frame/message-queue/src/mock_helpers.rs @@ -71,7 +71,7 @@ where if meter.try_consume(required).is_ok() { Ok(true) } else { - Err(ProcessMessageError::Overweight(required)) + Err(ProcessMessageError::Overweight(Some(required))) } } } diff --git a/substrate/frame/support/src/traits/messages.rs b/substrate/frame/support/src/traits/messages.rs index f3d893bcc1d8..3ca233438768 100644 --- a/substrate/frame/support/src/traits/messages.rs +++ b/substrate/frame/support/src/traits/messages.rs @@ -37,8 +37,8 @@ pub enum ProcessMessageError { Unsupported, /// Message processing was not attempted because it was not certain that the weight limit /// would be respected. The parameter gives the maximum weight which the message could take - /// to process. - Overweight(Weight), + /// to process. If none is given, then the worst case is assumed. + Overweight(Option), /// The queue wants to give up its current processing slot. /// /// Hints the message processor to cease servicing this queue and proceed to the next From 4b5f48b33e7e7051402d5a397b4a007bc795d60b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 18 Apr 2024 18:58:50 +0300 Subject: [PATCH 2/8] prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_4202.prdoc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 prdoc/pr_4202.prdoc diff --git a/prdoc/pr_4202.prdoc b/prdoc/pr_4202.prdoc new file mode 100644 index 000000000000..2b0c3a868116 --- /dev/null +++ b/prdoc/pr_4202.prdoc @@ -0,0 +1,14 @@ +title: "Treat XCM ExceedsStackLimit errors as transient in the MQ pallet" + +doc: + - audience: Runtime User + description: | + Fixes an issue where the MessageQueue can incorrectly assume that a message will permanently fail to process and disallow retrial of it. + +crates: + - name: frame-support + bump: major + - name: pallet-message-queue + bump: patch + - name: staging-xcm-builder + bump: patch From 6878f4b40512ffa9c2678ce4be9ef6c648a78d92 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 18 Apr 2024 23:16:35 +0300 Subject: [PATCH 3/8] Fix clippy Signed-off-by: Oliver Tale-Yazdi --- .../pallets/outbound-queue/src/process_message_impl.rs | 2 +- bridges/snowbridge/pallets/outbound-queue/src/test.rs | 4 +++- polkadot/runtime/parachains/src/mock.rs | 2 +- polkadot/xcm/xcm-simulator/src/lib.rs | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/bridges/snowbridge/pallets/outbound-queue/src/process_message_impl.rs b/bridges/snowbridge/pallets/outbound-queue/src/process_message_impl.rs index 731aa6fa6d5c..98cadefaa94d 100644 --- a/bridges/snowbridge/pallets/outbound-queue/src/process_message_impl.rs +++ b/bridges/snowbridge/pallets/outbound-queue/src/process_message_impl.rs @@ -18,7 +18,7 @@ impl ProcessMessage for Pallet { ) -> Result { let weight = T::WeightInfo::do_process_message(); if meter.try_consume(weight).is_err() { - return Err(ProcessMessageError::Overweight(weight)) + return Err(ProcessMessageError::Overweight(Some(weight))) } Self::do_process_message(origin, message) } diff --git a/bridges/snowbridge/pallets/outbound-queue/src/test.rs b/bridges/snowbridge/pallets/outbound-queue/src/test.rs index 4e9ea36e24bc..5d6ecfa5e185 100644 --- a/bridges/snowbridge/pallets/outbound-queue/src/test.rs +++ b/bridges/snowbridge/pallets/outbound-queue/src/test.rs @@ -139,7 +139,9 @@ fn process_message_fails_on_overweight_message() { let mut meter = WeightMeter::with_limit(Weight::from_parts(1, 1)); assert_noop!( OutboundQueue::process_message(encoded.as_slice(), origin, &mut meter, &mut [0u8; 32]), - ProcessMessageError::Overweight(::WeightInfo::do_process_message()) + ProcessMessageError::Overweight(Some( + ::WeightInfo::do_process_message() + )) ); }) } diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index c91f5be127cd..e41e7c7a9021 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -603,7 +603,7 @@ impl ProcessMessage for TestProcessMessage { Err(_) => return Err(ProcessMessageError::Corrupt), // same as the real `ProcessMessage` }; if meter.try_consume(required).is_err() { - return Err(ProcessMessageError::Overweight(required)) + return Err(ProcessMessageError::Overweight(Some(required))) } let mut processed = Processed::get(); diff --git a/polkadot/xcm/xcm-simulator/src/lib.rs b/polkadot/xcm/xcm-simulator/src/lib.rs index 7efbc658bbfb..e04d7ce63d26 100644 --- a/polkadot/xcm/xcm-simulator/src/lib.rs +++ b/polkadot/xcm/xcm-simulator/src/lib.rs @@ -329,7 +329,7 @@ macro_rules! decl_test_network { ); match r { Err($crate::ProcessMessageError::Overweight(required)) => - return Err($crate::XcmError::WeightLimitReached(required)), + return Err($crate::XcmError::WeightLimitReached(required.expect("Processor must know the max weight."))), // Not really the correct error, but there is no "undecodable". Err(_) => return Err($crate::XcmError::Unimplemented), Ok(_) => (), From 9d4aff71846b55c3b8ed0747ed24c11ecff26c79 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 19 Apr 2024 00:00:58 +0300 Subject: [PATCH 4/8] Fix test Signed-off-by: Oliver Tale-Yazdi --- .../xcm-builder/src/process_xcm_message.rs | 77 ++++++++++--------- polkadot/xcm/xcm-executor/src/lib.rs | 7 ++ 2 files changed, 46 insertions(+), 38 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/process_xcm_message.rs b/polkadot/xcm/xcm-builder/src/process_xcm_message.rs index 3816daf78340..4bb27fabf528 100644 --- a/polkadot/xcm/xcm-builder/src/process_xcm_message.rs +++ b/polkadot/xcm/xcm-builder/src/process_xcm_message.rs @@ -153,6 +153,45 @@ mod tests { } } + #[test] + fn process_message_exceeds_limits_fails() { + struct MockedExecutor; + impl ExecuteXcm<()> for MockedExecutor { + type Prepared = xcm_executor::WeighedMessage<()>; + fn prepare( + message: xcm::latest::Xcm<()>, + ) -> core::result::Result> { + Ok(xcm_executor::WeighedMessage::new(Weight::zero(), message)) + } + fn execute( + _: impl Into, + _: Self::Prepared, + _: &mut XcmHash, + _: Weight, + ) -> Outcome { + Outcome::Error { error: xcm::latest::Error::ExceedsStackLimit } + } + fn charge_fees(_location: impl Into, _fees: Assets) -> xcm::latest::Result { + unreachable!() + } + } + + type Processor = ProcessXcmMessage; + + let xcm = VersionedXcm::V4(xcm::latest::Xcm::<()>(vec![ + xcm::latest::Instruction::<()>::ClearOrigin, + ])); + assert_err!( + Processor::process_message( + &xcm.encode(), + ORIGIN, + &mut WeightMeter::new(), + &mut [0; 32] + ), + ProcessMessageError::Overweight(None) + ); + } + #[test] fn process_message_overweight_fails() { for msg in [v3_xcm(true), v3_xcm(false), v3_xcm(false), v2_xcm(false)] { @@ -177,44 +216,6 @@ mod tests { } } - #[test] - fn process_message_recursion_limit_exceeded_fails() { - use frame_support::__private::sp_tracing; - sp_tracing::try_init_simple(); - let mut xcm = VersionedXcm::V4(xcm::v4::Xcm::(vec![ - BuyExecution { fees: (Here, 1).into(), weight_limit: Unlimited }, - xcm::v4::Instruction::::Transact { - origin_kind: xcm::v4::OriginKind::Xcm, - require_weight_at_most: Weight::zero(), - call: RuntimeCall::System(frame_system::Call::::remark_with_event { - remark: b"Hello, World!".to_vec(), - }) - .encode() - .into(), - }, - ])); - - for _ in 0..300 { - let call = RuntimeCall::Xcm(pallet_xcm::Call::::execute { - message: xcm.into(), - max_weight: Weight::MAX, - }); - xcm = VersionedXcm::V4(xcm::v4::Xcm::(vec![xcm::v4::Instruction::< - RuntimeCall, - >::Transact { - origin_kind: xcm::v4::OriginKind::Xcm, - require_weight_at_most: Weight::zero(), - call: call.encode().into(), - }])); - } - - let msg = &xcm.encode()[..]; - let meter = &mut WeightMeter::new(); - let mut id = [0; 32]; - // FAIL-CI why does this work? - assert_ok!(Processor::process_message(msg, ORIGIN, meter, &mut id)); - } - fn v2_xcm(success: bool) -> VersionedXcm { let instr = if success { v3::Instruction::::ClearOrigin diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index e673a46c4ac6..d4ff094e2d59 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -182,6 +182,13 @@ impl PreparedMessage for WeighedMessage { } } +#[cfg(feature = "std")] +impl WeighedMessage { + pub fn new(weight: Weight, message: Xcm) -> Self { + Self(weight, message) + } +} + impl ExecuteXcm for XcmExecutor { type Prepared = WeighedMessage; fn prepare( From 48bed5b5ee74d7d9e0370d89a6e8381f3a7cd105 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 19 Apr 2024 12:30:47 +0300 Subject: [PATCH 5/8] Update prdoc/pr_4202.prdoc Co-authored-by: Branislav Kontur --- prdoc/pr_4202.prdoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/prdoc/pr_4202.prdoc b/prdoc/pr_4202.prdoc index 2b0c3a868116..6469c3c78407 100644 --- a/prdoc/pr_4202.prdoc +++ b/prdoc/pr_4202.prdoc @@ -12,3 +12,5 @@ crates: bump: patch - name: staging-xcm-builder bump: patch + - name: staging-xcm-executor + bump: patch From 2373ac7bde872514cfafa4062d359e7357e28937 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 19 Apr 2024 12:30:54 +0300 Subject: [PATCH 6/8] Update substrate/frame/message-queue/src/lib.rs Co-authored-by: Branislav Kontur --- substrate/frame/message-queue/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index 2e5a2a5292ed..7b50f6ef2b7e 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -1437,8 +1437,8 @@ impl Pallet { let prev_consumed = meter.consumed(); match T::MessageProcessor::process_message(message, origin.clone(), meter, &mut id) { - Err(Overweight(required)) => { - if required.map_or(true, |r| r.any_gt(overweight_limit)) { + Err(Overweight(maybe_required)) => { + if maybe_required.map_or(true, |r| r.any_gt(overweight_limit)) { // Permanently overweight. Self::deposit_event(Event::::OverweightEnqueued { id, From 313fe1f6c3735d7de2b647aa6f3010be7c4c65b5 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 19 Apr 2024 12:38:20 +0300 Subject: [PATCH 7/8] Add test feature gate Signed-off-by: Oliver Tale-Yazdi --- polkadot/xcm/xcm-executor/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index d4ff094e2d59..a7052328da00 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -182,7 +182,7 @@ impl PreparedMessage for WeighedMessage { } } -#[cfg(feature = "std")] +#[cfg(any(test, feature = "std"))] impl WeighedMessage { pub fn new(weight: Weight, message: Xcm) -> Self { Self(weight, message) From ed65ab4815389649073028b6474be0811f40c817 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 24 Apr 2024 17:17:21 +0300 Subject: [PATCH 8/8] Revert and use dedicated error variant Signed-off-by: Oliver Tale-Yazdi --- .../src/process_message_impl.rs | 2 +- .../pallets/outbound-queue/src/test.rs | 4 +- polkadot/runtime/parachains/src/mock.rs | 2 +- polkadot/xcm/xcm-builder/src/barriers.rs | 6 +- .../xcm-builder/src/process_xcm_message.rs | 8 +- .../xcm/xcm-builder/src/tests/barriers.rs | 12 +-- polkadot/xcm/xcm-simulator/src/lib.rs | 2 +- substrate/frame/message-queue/src/lib.rs | 46 ++++---- substrate/frame/message-queue/src/mock.rs | 7 +- .../frame/message-queue/src/mock_helpers.rs | 2 +- substrate/frame/message-queue/src/tests.rs | 101 +++++++++++++++++- .../frame/support/src/traits/messages.rs | 8 +- 12 files changed, 155 insertions(+), 45 deletions(-) diff --git a/bridges/snowbridge/pallets/outbound-queue/src/process_message_impl.rs b/bridges/snowbridge/pallets/outbound-queue/src/process_message_impl.rs index 98cadefaa94d..731aa6fa6d5c 100644 --- a/bridges/snowbridge/pallets/outbound-queue/src/process_message_impl.rs +++ b/bridges/snowbridge/pallets/outbound-queue/src/process_message_impl.rs @@ -18,7 +18,7 @@ impl ProcessMessage for Pallet { ) -> Result { let weight = T::WeightInfo::do_process_message(); if meter.try_consume(weight).is_err() { - return Err(ProcessMessageError::Overweight(Some(weight))) + return Err(ProcessMessageError::Overweight(weight)) } Self::do_process_message(origin, message) } diff --git a/bridges/snowbridge/pallets/outbound-queue/src/test.rs b/bridges/snowbridge/pallets/outbound-queue/src/test.rs index 5d6ecfa5e185..4e9ea36e24bc 100644 --- a/bridges/snowbridge/pallets/outbound-queue/src/test.rs +++ b/bridges/snowbridge/pallets/outbound-queue/src/test.rs @@ -139,9 +139,7 @@ fn process_message_fails_on_overweight_message() { let mut meter = WeightMeter::with_limit(Weight::from_parts(1, 1)); assert_noop!( OutboundQueue::process_message(encoded.as_slice(), origin, &mut meter, &mut [0u8; 32]), - ProcessMessageError::Overweight(Some( - ::WeightInfo::do_process_message() - )) + ProcessMessageError::Overweight(::WeightInfo::do_process_message()) ); }) } diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index e41e7c7a9021..c91f5be127cd 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -603,7 +603,7 @@ impl ProcessMessage for TestProcessMessage { Err(_) => return Err(ProcessMessageError::Corrupt), // same as the real `ProcessMessage` }; if meter.try_consume(required).is_err() { - return Err(ProcessMessageError::Overweight(Some(required))) + return Err(ProcessMessageError::Overweight(required)) } let mut processed = Processed::get(); diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 232e160d5b33..c0b328f38e96 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -47,7 +47,7 @@ impl ShouldExecute for TakeWeightCredit { properties.weight_credit = properties .weight_credit .checked_sub(&max_weight) - .ok_or(ProcessMessageError::Overweight(Some(max_weight)))?; + .ok_or(ProcessMessageError::Overweight(max_weight))?; Ok(()) } } @@ -104,7 +104,7 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFrom *weight_limit = Limited(max_weight); Ok(()) }, - _ => Err(ProcessMessageError::Overweight(Some(max_weight))), + _ => Err(ProcessMessageError::Overweight(max_weight)), })?; Ok(()) } @@ -304,7 +304,7 @@ impl> ShouldExecute for AllowExplicitUnpaidExecutionFrom Ok(()), UnpaidExecution { weight_limit: Unlimited, .. } => Ok(()), - _ => Err(ProcessMessageError::Overweight(Some(max_weight))), + _ => Err(ProcessMessageError::Overweight(max_weight)), })?; Ok(()) } diff --git a/polkadot/xcm/xcm-builder/src/process_xcm_message.rs b/polkadot/xcm/xcm-builder/src/process_xcm_message.rs index 4bb27fabf528..7760274f6e24 100644 --- a/polkadot/xcm/xcm-builder/src/process_xcm_message.rs +++ b/polkadot/xcm/xcm-builder/src/process_xcm_message.rs @@ -77,7 +77,7 @@ impl< meter.remaining(), ); - return Err(ProcessMessageError::Overweight(Some(required))) + return Err(ProcessMessageError::Overweight(required)) } let (consumed, result) = match XcmExecutor::execute(origin.into(), pre, id, Weight::zero()) @@ -103,7 +103,7 @@ impl< "XCM message execution error: {error:?}", ); let error = match error { - xcm::latest::Error::ExceedsStackLimit => ProcessMessageError::Overweight(None), + xcm::latest::Error::ExceedsStackLimit => ProcessMessageError::StackLimitReached, _ => ProcessMessageError::Unsupported, }; @@ -188,7 +188,7 @@ mod tests { &mut WeightMeter::new(), &mut [0; 32] ), - ProcessMessageError::Overweight(None) + ProcessMessageError::StackLimitReached, ); } @@ -203,7 +203,7 @@ mod tests { let mut id = [0; 32]; assert_err!( Processor::process_message(msg, ORIGIN, meter, &mut id), - Overweight(Some(1000.into())) + Overweight(1000.into()) ); assert_eq!(meter.consumed(), 0.into()); } diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 823b64ea4b0c..6516263f57a0 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -42,7 +42,7 @@ fn take_weight_credit_barrier_should_work() { Weight::from_parts(10, 10), &mut properties, ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(10, 10))))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(10, 10)))); assert_eq!(properties.weight_credit, Weight::zero()); } @@ -163,7 +163,7 @@ fn allow_explicit_unpaid_should_work() { Weight::from_parts(20, 20), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(20, 20))))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20)))); let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( &Parent.into(), @@ -171,7 +171,7 @@ fn allow_explicit_unpaid_should_work() { Weight::from_parts(20, 20), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(20, 20))))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20)))); let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( &Parent.into(), @@ -195,7 +195,7 @@ fn allow_explicit_unpaid_should_work() { Weight::from_parts(20, 20), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(20, 20))))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20)))); let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( &Parent.into(), @@ -234,7 +234,7 @@ fn allow_paid_should_work() { Weight::from_parts(30, 30), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(30, 30))))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(30, 30)))); let fees = (Parent, 1).into(); let mut paying_message = Xcm::<()>(vec![ @@ -272,7 +272,7 @@ fn allow_paid_should_work() { Weight::from_parts(20, 20), &mut props(Weight::zero()), ); - assert_eq!(r, Err(ProcessMessageError::Overweight(Some(Weight::from_parts(20, 20))))); + assert_eq!(r, Err(ProcessMessageError::Overweight(Weight::from_parts(20, 20)))); let r = AllowTopLevelPaidExecutionFrom::>::should_execute( &Parent.into(), diff --git a/polkadot/xcm/xcm-simulator/src/lib.rs b/polkadot/xcm/xcm-simulator/src/lib.rs index e04d7ce63d26..7efbc658bbfb 100644 --- a/polkadot/xcm/xcm-simulator/src/lib.rs +++ b/polkadot/xcm/xcm-simulator/src/lib.rs @@ -329,7 +329,7 @@ macro_rules! decl_test_network { ); match r { Err($crate::ProcessMessageError::Overweight(required)) => - return Err($crate::XcmError::WeightLimitReached(required.expect("Processor must know the max weight."))), + return Err($crate::XcmError::WeightLimitReached(required)), // Not really the correct error, but there is no "undecodable". Err(_) => return Err($crate::XcmError::Unimplemented), Ok(_) => (), diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index 7b50f6ef2b7e..ef3420d21be5 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -765,6 +765,13 @@ enum MessageExecutionStatus { Processed, /// The message was processed and resulted in a, possibly permanent, error. Unprocessable { permanent: bool }, + /// The stack depth limit was reached. + /// + /// We cannot just return `Unprocessable` in this case, because the processability of the + /// message depends on how the function was called. This may be a permanent error if it was + /// called by a top-level function, or a transient error if it was already called in a nested + /// function. + StackLimitReached, } impl Pallet { @@ -984,7 +991,8 @@ impl Pallet { // additional overweight event being deposited. ) { Overweight | InsufficientWeight => Err(Error::::InsufficientWeight), - Unprocessable { permanent: false } => Err(Error::::TemporarilyUnprocessable), + StackLimitReached | Unprocessable { permanent: false } => + Err(Error::::TemporarilyUnprocessable), Unprocessable { permanent: true } | Processed => { page.note_processed_at_pos(pos); book_state.message_count.saturating_dec(); @@ -1250,7 +1258,7 @@ impl Pallet { let is_processed = match res { InsufficientWeight => return ItemExecutionStatus::Bailed, Unprocessable { permanent: false } => return ItemExecutionStatus::NoProgress, - Processed | Unprocessable { permanent: true } => true, + Processed | Unprocessable { permanent: true } | StackLimitReached => true, Overweight => false, }; @@ -1437,22 +1445,20 @@ impl Pallet { let prev_consumed = meter.consumed(); match T::MessageProcessor::process_message(message, origin.clone(), meter, &mut id) { - Err(Overweight(maybe_required)) => { - if maybe_required.map_or(true, |r| r.any_gt(overweight_limit)) { - // Permanently overweight. - Self::deposit_event(Event::::OverweightEnqueued { - id, - origin, - page_index, - message_index, - }); - - MessageExecutionStatus::Overweight - } else { - // Temporarily overweight - save progress and stop processing this - // queue. - MessageExecutionStatus::InsufficientWeight - } + Err(Overweight(w)) if w.any_gt(overweight_limit) => { + // Permanently overweight. + Self::deposit_event(Event::::OverweightEnqueued { + id, + origin, + page_index, + message_index, + }); + MessageExecutionStatus::Overweight + }, + Err(Overweight(_)) => { + // Temporarily overweight - save progress and stop processing this + // queue. + MessageExecutionStatus::InsufficientWeight }, Err(Yield) => { // Processing should be reattempted later. @@ -1463,6 +1469,10 @@ impl Pallet { Self::deposit_event(Event::::ProcessingFailed { id: id.into(), origin, error }); MessageExecutionStatus::Unprocessable { permanent: true } }, + Err(error @ StackLimitReached) => { + Self::deposit_event(Event::::ProcessingFailed { id: id.into(), origin, error }); + MessageExecutionStatus::StackLimitReached + }, Ok(success) => { // Success let weight_used = meter.consumed().saturating_sub(prev_consumed); diff --git a/substrate/frame/message-queue/src/mock.rs b/substrate/frame/message-queue/src/mock.rs index d14a152ba324..66a242d5a18f 100644 --- a/substrate/frame/message-queue/src/mock.rs +++ b/substrate/frame/message-queue/src/mock.rs @@ -191,13 +191,14 @@ impl ProcessMessage for RecordingMessageProcessor { MessagesProcessed::set(m); Ok(true) } else { - Err(ProcessMessageError::Overweight(Some(required))) + Err(ProcessMessageError::Overweight(required)) } } } parameter_types! { pub static Callback: Box = Box::new(|_, _| {}); + pub static IgnoreStackOvError: bool = false; } /// Processed a mocked message. Messages that end with `badformat`, `corrupt`, `unsupported` or @@ -216,6 +217,8 @@ fn processing_message(msg: &[u8], origin: &MessageOrigin) -> Result<(), ProcessM Err(ProcessMessageError::Unsupported) } else if msg.ends_with("yield") { Err(ProcessMessageError::Yield) + } else if msg.ends_with("stacklimitreached") && !IgnoreStackOvError::get() { + Err(ProcessMessageError::StackLimitReached) } else { Ok(()) } @@ -254,7 +257,7 @@ impl ProcessMessage for CountingMessageProcessor { NumMessagesProcessed::set(NumMessagesProcessed::get() + 1); Ok(true) } else { - Err(ProcessMessageError::Overweight(Some(required))) + Err(ProcessMessageError::Overweight(required)) } } } diff --git a/substrate/frame/message-queue/src/mock_helpers.rs b/substrate/frame/message-queue/src/mock_helpers.rs index 3f2e13628003..28395e27cdd2 100644 --- a/substrate/frame/message-queue/src/mock_helpers.rs +++ b/substrate/frame/message-queue/src/mock_helpers.rs @@ -71,7 +71,7 @@ where if meter.try_consume(required).is_ok() { Ok(true) } else { - Err(ProcessMessageError::Overweight(Some(required))) + Err(ProcessMessageError::Overweight(required)) } } } diff --git a/substrate/frame/message-queue/src/tests.rs b/substrate/frame/message-queue/src/tests.rs index d6788847d571..e89fdb8b3208 100644 --- a/substrate/frame/message-queue/src/tests.rs +++ b/substrate/frame/message-queue/src/tests.rs @@ -174,9 +174,10 @@ fn service_queues_failing_messages_works() { MessageQueue::enqueue_message(msg("badformat"), Here); MessageQueue::enqueue_message(msg("corrupt"), Here); MessageQueue::enqueue_message(msg("unsupported"), Here); + MessageQueue::enqueue_message(msg("stacklimitreached"), Here); MessageQueue::enqueue_message(msg("yield"), Here); // Starts with four pages. - assert_pages(&[0, 1, 2, 3]); + assert_pages(&[0, 1, 2, 3, 4]); assert_eq!(MessageQueue::service_queues(1.into_weight()), 1.into_weight()); assert_last_event::( @@ -206,9 +207,9 @@ fn service_queues_failing_messages_works() { .into(), ); assert_eq!(MessageQueue::service_queues(1.into_weight()), 1.into_weight()); - assert_eq!(System::events().len(), 3); + assert_eq!(System::events().len(), 4); // Last page with the `yield` stays in. - assert_pages(&[3]); + assert_pages(&[4]); }); } @@ -1880,3 +1881,97 @@ fn process_enqueued_on_idle_requires_enough_weight() { assert_eq!(MessagesProcessed::take(), vec![]); }) } + +/// A message that reports `StackLimitReached` will not be put into the overweight queue when +/// executed from the top level. +#[test] +fn process_discards_stack_ov_message() { + use MessageOrigin::*; + build_and_execute::(|| { + MessageQueue::enqueue_message(msg("stacklimitreached"), Here); + + MessageQueue::service_queues(10.into_weight()); + + assert_last_event::( + Event::ProcessingFailed { + id: blake2_256(b"stacklimitreached").into(), + origin: MessageOrigin::Here, + error: ProcessMessageError::StackLimitReached, + } + .into(), + ); + + assert!(MessagesProcessed::take().is_empty()); + // Message is gone and not overweight: + assert_pages(&[]); + }); +} + +/// A message that reports `StackLimitReached` will stay in the overweight queue when it is executed +/// by `execute_overweight`. +#[test] +fn execute_overweight_keeps_stack_ov_message() { + use MessageOrigin::*; + build_and_execute::(|| { + // We need to create a mocked message that first reports insufficient weight, and then + // `StackLimitReached`: + IgnoreStackOvError::set(true); + MessageQueue::enqueue_message(msg("stacklimitreached"), Here); + MessageQueue::service_queues(0.into_weight()); + + assert_last_event::( + Event::OverweightEnqueued { + id: blake2_256(b"stacklimitreached"), + origin: MessageOrigin::Here, + message_index: 0, + page_index: 0, + } + .into(), + ); + // Does not count as 'processed': + assert!(MessagesProcessed::take().is_empty()); + assert_pages(&[0]); + + // Now let it return `StackLimitReached`. Note that this case would normally not happen, + // since we assume that the top-level execution is the one with the most remaining stack + // depth. + IgnoreStackOvError::set(false); + // Ensure that trying to execute the message does not change any state (besides events). + System::reset_events(); + let storage_noop = StorageNoopGuard::new(); + assert_eq!( + ::execute_overweight(3.into_weight(), (Here, 0, 0)), + Err(ExecuteOverweightError::Other) + ); + assert_last_event::( + Event::ProcessingFailed { + id: blake2_256(b"stacklimitreached").into(), + origin: MessageOrigin::Here, + error: ProcessMessageError::StackLimitReached, + } + .into(), + ); + System::reset_events(); + drop(storage_noop); + + // Now let's process it normally: + IgnoreStackOvError::set(true); + assert_eq!( + ::execute_overweight(1.into_weight(), (Here, 0, 0)) + .unwrap(), + 1.into_weight() + ); + + assert_last_event::( + Event::Processed { + id: blake2_256(b"stacklimitreached").into(), + origin: MessageOrigin::Here, + weight_used: 1.into_weight(), + success: true, + } + .into(), + ); + assert_pages(&[]); + System::reset_events(); + }); +} diff --git a/substrate/frame/support/src/traits/messages.rs b/substrate/frame/support/src/traits/messages.rs index 3ca233438768..2eec606b6d18 100644 --- a/substrate/frame/support/src/traits/messages.rs +++ b/substrate/frame/support/src/traits/messages.rs @@ -37,8 +37,8 @@ pub enum ProcessMessageError { Unsupported, /// Message processing was not attempted because it was not certain that the weight limit /// would be respected. The parameter gives the maximum weight which the message could take - /// to process. If none is given, then the worst case is assumed. - Overweight(Option), + /// to process. + Overweight(Weight), /// The queue wants to give up its current processing slot. /// /// Hints the message processor to cease servicing this queue and proceed to the next @@ -46,6 +46,8 @@ pub enum ProcessMessageError { /// the case that a queue is re-serviced within the same block after *yielding*. A queue is /// not required to *yield* again when it is being re-serviced withing the same block. Yield, + /// The message could not be processed for reaching the stack depth limit. + StackLimitReached, } /// Can process messages from a specific origin. @@ -96,6 +98,8 @@ pub trait ServiceQueues { /// - `weight_limit`: The maximum amount of dynamic weight that this call can use. /// /// Returns the dynamic weight used by this call; is never greater than `weight_limit`. + /// Should only be called in top-level runtime entry points like `on_initialize` or `on_idle`. + /// Otherwise, stack depth limit errors may be miss-handled. fn service_queues(weight_limit: Weight) -> Weight; /// Executes a message that could not be executed by [`Self::service_queues()`] because it was