From ed65ab4815389649073028b6474be0811f40c817 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 24 Apr 2024 17:17:21 +0300 Subject: [PATCH] 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