From 8973908d4e8bb62dd6b746d370b4c33c4851c9fd Mon Sep 17 00:00:00 2001 From: gotnoshoeson Date: Fri, 14 Jun 2024 08:57:26 -0700 Subject: [PATCH 1/9] Fix issue 4762 --- substrate/frame/message-queue/src/lib.rs | 28 +++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index ef3420d21be5..f1a446b93412 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -645,17 +645,19 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { fn on_initialize(_n: BlockNumberFor) -> Weight { + let status = ServiceQueuesContext::OnInitialize; if let Some(weight_limit) = T::ServiceWeight::get() { - Self::service_queues(weight_limit) + Self::service_queues(weight_limit, status) } else { Weight::zero() } } fn on_idle(_n: BlockNumberFor, remaining_weight: Weight) -> Weight { + let status = ServiceQueuesContext::OnIdle; if let Some(weight_limit) = T::IdleMaxServiceWeight::get() { // Make use of the remaining weight to process enqueued messages. - Self::service_queues(weight_limit.min(remaining_weight)) + Self::service_queues(weight_limit.min(remaining_weight), status) } else { Weight::zero() } @@ -774,6 +776,15 @@ enum MessageExecutionStatus { StackLimitReached, } +/// The status of an attempt to process a message. +#[derive(PartialEq)] +enum ServiceQueuesContext { + /// There is not enough weight remaining at present. + OnIdle, + /// There will never be enough weight. + OnInitialize, +} + impl Pallet { /// Knit `origin` into the ready ring right at the end. /// @@ -1554,13 +1565,20 @@ impl, O: Into> Get for IntoU32 { impl ServiceQueues for Pallet { type OverweightMessageAddress = (MessageOriginOf, PageIndex, T::Size); - fn service_queues(weight_limit: Weight) -> Weight { + fn service_queues(weight_limit: Weight, status: ServiceQueuesContext) -> Weight { let mut weight = WeightMeter::with_limit(weight_limit); // Get the maximum weight that processing a single message may take: let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| { - defensive!("Not enough weight to service a single message."); - Weight::zero() + // throw defensive message when service_queues is called from on_initialize + // don't throw message when service_queues is called from on_idle + match status { + ServiceQueuesContext::OnInitialize => { + defensive!("Not enough weight to service a single message."); + Weight::zero() + }, + ServiceQueuesContext::OnIdle => Weight::zero(), + } }); match with_service_mutex(|| { From c39556e8582148bc6bc8df5e77c75dfbf286e241 Mon Sep 17 00:00:00 2001 From: gotnoshoeson Date: Sat, 15 Jun 2024 11:00:47 -0700 Subject: [PATCH 2/9] Ready for PR --- substrate/frame/message-queue/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index f1a446b93412..fa5051589ad3 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -645,19 +645,19 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { fn on_initialize(_n: BlockNumberFor) -> Weight { - let status = ServiceQueuesContext::OnInitialize; + let context = ServiceQueuesContext::OnInitialize; if let Some(weight_limit) = T::ServiceWeight::get() { - Self::service_queues(weight_limit, status) + Self::service_queues(weight_limit, context) } else { Weight::zero() } } fn on_idle(_n: BlockNumberFor, remaining_weight: Weight) -> Weight { - let status = ServiceQueuesContext::OnIdle; + let context = ServiceQueuesContext::OnIdle; if let Some(weight_limit) = T::IdleMaxServiceWeight::get() { // Make use of the remaining weight to process enqueued messages. - Self::service_queues(weight_limit.min(remaining_weight), status) + Self::service_queues(weight_limit.min(remaining_weight), context) } else { Weight::zero() } @@ -1565,14 +1565,14 @@ impl, O: Into> Get for IntoU32 { impl ServiceQueues for Pallet { type OverweightMessageAddress = (MessageOriginOf, PageIndex, T::Size); - fn service_queues(weight_limit: Weight, status: ServiceQueuesContext) -> Weight { + fn service_queues(weight_limit: Weight, context: ServiceQueuesContext) -> Weight { let mut weight = WeightMeter::with_limit(weight_limit); // Get the maximum weight that processing a single message may take: let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| { // throw defensive message when service_queues is called from on_initialize // don't throw message when service_queues is called from on_idle - match status { + match context { ServiceQueuesContext::OnInitialize => { defensive!("Not enough weight to service a single message."); Weight::zero() From 05197d1d96bc093c6c2e61fe1448716536690b08 Mon Sep 17 00:00:00 2001 From: gotnoshoeson Date: Sun, 16 Jun 2024 15:06:25 -0700 Subject: [PATCH 3/9] Update inline docs --- substrate/frame/message-queue/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index fa5051589ad3..a32a5caaf5da 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -776,12 +776,13 @@ enum MessageExecutionStatus { StackLimitReached, } -/// The status of an attempt to process a message. +/// The context to pass to service_queues through on_idle and on_initialize hooks +/// We don't want to throw the defensive message if called from on_idle hook #[derive(PartialEq)] enum ServiceQueuesContext { - /// There is not enough weight remaining at present. + /// Context of on_idle hook OnIdle, - /// There will never be enough weight. + /// Context of on_initialize hook OnInitialize, } From 43add5dc48cc1823cde1e08f0e80659ff4178d0c Mon Sep 17 00:00:00 2001 From: gotnoshoeson Date: Tue, 25 Jun 2024 18:49:53 -0700 Subject: [PATCH 4/9] Updates per code review --- substrate/frame/message-queue/src/lib.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index a32a5caaf5da..582caa672dd7 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -645,19 +645,17 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { fn on_initialize(_n: BlockNumberFor) -> Weight { - let context = ServiceQueuesContext::OnInitialize; if let Some(weight_limit) = T::ServiceWeight::get() { - Self::service_queues(weight_limit, context) + Self::service_queues(weight_limit, ServiceQueuesContext::OnInitialize) } else { Weight::zero() } } fn on_idle(_n: BlockNumberFor, remaining_weight: Weight) -> Weight { - let context = ServiceQueuesContext::OnIdle; if let Some(weight_limit) = T::IdleMaxServiceWeight::get() { // Make use of the remaining weight to process enqueued messages. - Self::service_queues(weight_limit.min(remaining_weight), context) + Self::service_queues(weight_limit.min(remaining_weight), ServiceQueuesContext::OnIdle) } else { Weight::zero() } @@ -1573,13 +1571,10 @@ impl ServiceQueues for Pallet { let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| { // throw defensive message when service_queues is called from on_initialize // don't throw message when service_queues is called from on_idle - match context { - ServiceQueuesContext::OnInitialize => { - defensive!("Not enough weight to service a single message."); - Weight::zero() - }, - ServiceQueuesContext::OnIdle => Weight::zero(), + if matches(context, ServiceQueuesContext::OnInitialize) { + defensive!("Not enough weight to service a single message."); } + Weight::zero() }); match with_service_mutex(|| { From d3e03ff54e05570918b6ca3701215bfe5b1a2746 Mon Sep 17 00:00:00 2001 From: gotnoshoeson Date: Sun, 30 Jun 2024 14:40:14 -0700 Subject: [PATCH 5/9] Add more notes per review --- substrate/frame/message-queue/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index 582caa672dd7..a0beff4c5283 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -1570,7 +1570,8 @@ impl ServiceQueues for Pallet { // Get the maximum weight that processing a single message may take: let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| { // throw defensive message when service_queues is called from on_initialize - // don't throw message when service_queues is called from on_idle + // it doesn't matter if there is not enough weight when called in the context of on_idle + // therefore, don't throw message when service_queues is called from on_idle if matches(context, ServiceQueuesContext::OnInitialize) { defensive!("Not enough weight to service a single message."); } From 8264ab84eed2ef02c24ce905bf4639bd74cfaf39 Mon Sep 17 00:00:00 2001 From: gotnoshoeson Date: Wed, 3 Jul 2024 21:43:10 -0700 Subject: [PATCH 6/9] Add prdoc --- prdoc/pr_4803.prdoc | 13 +++++++++++++ substrate/frame/message-queue/src/lib.rs | 18 +++++++++++------- 2 files changed, 24 insertions(+), 7 deletions(-) create mode 100644 prdoc/pr_4803.prdoc diff --git a/prdoc/pr_4803.prdoc b/prdoc/pr_4803.prdoc new file mode 100644 index 000000000000..87473f8b02ed --- /dev/null +++ b/prdoc/pr_4803.prdoc @@ -0,0 +1,13 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Fix for issue #4762 + +doc: + - audience: Runtime Dev + description: | + When the status of the queue is on_initialize, throw a defensive message and return weight of 0, + however when status is on_idle, do not throw a defensive message, only return weight of 0 + +crates: + - name: MQ pallet diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index a0beff4c5283..306acc05ae77 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -1569,13 +1569,7 @@ impl ServiceQueues for Pallet { // Get the maximum weight that processing a single message may take: let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| { - // throw defensive message when service_queues is called from on_initialize - // it doesn't matter if there is not enough weight when called in the context of on_idle - // therefore, don't throw message when service_queues is called from on_idle - if matches(context, ServiceQueuesContext::OnInitialize) { - defensive!("Not enough weight to service a single message."); - } - Weight::zero() + check_queue_context(context); }); match with_service_mutex(|| { @@ -1644,6 +1638,16 @@ impl ServiceQueues for Pallet { }, ) } + + // Check the message queue context for on_idle and on_initialize status + // throw defensive message in `on_initialize` status + // don't throw defensive message in `on_idle` status + fn check_queue_context(context: ServiceQueuesContext) -> Weight { + if matches(context, ServiceQueuesContext::OnInitialize) { + defensive!("Not enough weight to service a single message."); + } + Weight::zero() + } } impl EnqueueMessage> for Pallet { From 6bbf1a3ae5621125d78689fa3e29493930d1c38f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sun, 22 Sep 2024 09:47:34 +0200 Subject: [PATCH 7/9] Apply suggestions from code review --- substrate/frame/message-queue/src/lib.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index 2f7616dad146..d1a104b74abe 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -1594,7 +1594,10 @@ impl ServiceQueues for Pallet { // Get the maximum weight that processing a single message may take: let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| { - check_queue_context(context); + if matches(context, ServiceQueuesContext::OnInitialize) { + defensive!("Not enough weight to service a single message."); + } + Weight::zero() }); match with_service_mutex(|| { @@ -1663,16 +1666,6 @@ impl ServiceQueues for Pallet { }, ) } - - // Check the message queue context for on_idle and on_initialize status - // throw defensive message in `on_initialize` status - // don't throw defensive message in `on_idle` status - fn check_queue_context(context: ServiceQueuesContext) -> Weight { - if matches(context, ServiceQueuesContext::OnInitialize) { - defensive!("Not enough weight to service a single message."); - } - Weight::zero() - } } impl EnqueueMessage> for Pallet { From b3551d6ae5633225fba20161a27bd8b4913191ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 16 Oct 2024 17:57:01 +0200 Subject: [PATCH 8/9] Fix issues --- substrate/frame/message-queue/src/lib.rs | 109 +++++++++++---------- substrate/frame/message-queue/src/tests.rs | 2 +- 2 files changed, 60 insertions(+), 51 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index d1a104b74abe..31402f2a9d81 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -649,7 +649,7 @@ pub mod pallet { impl Hooks> for Pallet { fn on_initialize(_n: BlockNumberFor) -> Weight { if let Some(weight_limit) = T::ServiceWeight::get() { - Self::service_queues(weight_limit, ServiceQueuesContext::OnInitialize) + Self::service_queues_impl(weight_limit, ServiceQueuesContext::OnInitialize) } else { Weight::zero() } @@ -658,7 +658,10 @@ pub mod pallet { fn on_idle(_n: BlockNumberFor, remaining_weight: Weight) -> Weight { if let Some(weight_limit) = T::IdleMaxServiceWeight::get() { // Make use of the remaining weight to process enqueued messages. - Self::service_queues(weight_limit.min(remaining_weight), ServiceQueuesContext::OnIdle) + Self::service_queues_impl( + weight_limit.min(remaining_weight), + ServiceQueuesContext::OnIdle, + ) } else { Weight::zero() } @@ -777,14 +780,16 @@ enum MessageExecutionStatus { StackLimitReached, } -/// The context to pass to service_queues through on_idle and on_initialize hooks +/// The context to pass to [`Pallet::service_queues_impl`] through on_idle and on_initialize hooks /// We don't want to throw the defensive message if called from on_idle hook #[derive(PartialEq)] enum ServiceQueuesContext { - /// Context of on_idle hook + /// Context of on_idle hook. OnIdle, - /// Context of on_initialize hook + /// Context of on_initialize hook. OnInitialize, + /// Context `service_queues` trait function. + ServiceQueues, } impl Pallet { @@ -1521,6 +1526,53 @@ impl Pallet { }, } } + + fn service_queues_impl(weight_limit: Weight, context: ServiceQueuesContext) -> Weight { + let mut weight = WeightMeter::with_limit(weight_limit); + + // Get the maximum weight that processing a single message may take: + let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| { + if matches!(context, ServiceQueuesContext::OnInitialize) { + defensive!("Not enough weight to service a single message."); + } + Weight::zero() + }); + + match with_service_mutex(|| { + let mut next = match Self::bump_service_head(&mut weight) { + Some(h) => h, + None => return weight.consumed(), + }; + // The last queue that did not make any progress. + // The loop aborts as soon as it arrives at this queue again without making any progress + // on other queues in between. + let mut last_no_progress = None; + + loop { + let (progressed, n) = Self::service_queue(next.clone(), &mut weight, max_weight); + next = match n { + Some(n) => + if !progressed { + if last_no_progress == Some(n.clone()) { + break + } + if last_no_progress.is_none() { + last_no_progress = Some(next.clone()) + } + n + } else { + last_no_progress = None; + n + }, + None => break, + } + } + weight.consumed() + }) { + Err(()) => weight.consumed(), + Ok(w) => w, + } + } } /// Run a closure that errors on re-entrance. Meant to be used by anything that services queues. @@ -1589,51 +1641,8 @@ impl, O: Into> Get for IntoU32 { impl ServiceQueues for Pallet { type OverweightMessageAddress = (MessageOriginOf, PageIndex, T::Size); - fn service_queues(weight_limit: Weight, context: ServiceQueuesContext) -> Weight { - let mut weight = WeightMeter::with_limit(weight_limit); - - // Get the maximum weight that processing a single message may take: - let max_weight = Self::max_message_weight(weight_limit).unwrap_or_else(|| { - if matches(context, ServiceQueuesContext::OnInitialize) { - defensive!("Not enough weight to service a single message."); - } - Weight::zero() - }); - - match with_service_mutex(|| { - let mut next = match Self::bump_service_head(&mut weight) { - Some(h) => h, - None => return weight.consumed(), - }; - // The last queue that did not make any progress. - // The loop aborts as soon as it arrives at this queue again without making any progress - // on other queues in between. - let mut last_no_progress = None; - - loop { - let (progressed, n) = Self::service_queue(next.clone(), &mut weight, max_weight); - next = match n { - Some(n) => - if !progressed { - if last_no_progress == Some(n.clone()) { - break - } - if last_no_progress.is_none() { - last_no_progress = Some(next.clone()) - } - n - } else { - last_no_progress = None; - n - }, - None => break, - } - } - weight.consumed() - }) { - Err(()) => weight.consumed(), - Ok(w) => w, - } + fn service_queues(weight_limit: Weight) -> Weight { + Self::service_queues_impl(weight_limit, ServiceQueuesContext::ServiceQueues) } /// Execute a single overweight message. diff --git a/substrate/frame/message-queue/src/tests.rs b/substrate/frame/message-queue/src/tests.rs index fac135f135ce..b75764b67bea 100644 --- a/substrate/frame/message-queue/src/tests.rs +++ b/substrate/frame/message-queue/src/tests.rs @@ -279,7 +279,7 @@ fn service_queues_low_weight_defensive() { assert!(MessageQueue::do_integrity_test().is_err()); MessageQueue::enqueue_message(msg("weight=0"), Here); - MessageQueue::service_queues(104.into_weight()); + MessageQueue::service_queues_impl(104.into_weight(), ServiceQueuesContext::OnInitialize); }); } From eaba016593f67ef7b9d48f8009dab654c09fb776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 16 Oct 2024 21:35:33 +0200 Subject: [PATCH 9/9] Update prdoc/pr_4803.prdoc --- prdoc/pr_4803.prdoc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_4803.prdoc b/prdoc/pr_4803.prdoc index 87473f8b02ed..0d2ad08d610f 100644 --- a/prdoc/pr_4803.prdoc +++ b/prdoc/pr_4803.prdoc @@ -10,4 +10,5 @@ doc: however when status is on_idle, do not throw a defensive message, only return weight of 0 crates: - - name: MQ pallet + - name: pallet-message-queue + bump: patch