From c762b231ac63e7e2b773a7fc42a7eb1ebd855f3b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 18 Nov 2023 15:42:15 +0100 Subject: [PATCH 1/8] Add ready_pages to queue footprint Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/message-queue/src/lib.rs | 6 ++++-- substrate/frame/support/src/traits/messages.rs | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index 12d289478b37c..8e8e716694759 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -194,8 +194,9 @@ use frame_support::{ defensive, pallet_prelude::*, traits::{ - DefensiveTruncateFrom, EnqueueMessage, ExecuteOverweightError, Footprint, ProcessMessage, - ProcessMessageError, QueueFootprint, QueuePausedQuery, ServiceQueues, + DefensiveSaturating, DefensiveTruncateFrom, EnqueueMessage, ExecuteOverweightError, + Footprint, ProcessMessage, ProcessMessageError, QueueFootprint, QueuePausedQuery, + ServiceQueues, }, BoundedSlice, CloneNoBound, DefaultNoBound, }; @@ -427,6 +428,7 @@ impl From> for QueueFootprint { fn from(book: BookState) -> Self { QueueFootprint { pages: book.count, + ready_pages: book.end.defensive_saturating_sub(book.begin), storage: Footprint { count: book.message_count, size: book.size }, } } diff --git a/substrate/frame/support/src/traits/messages.rs b/substrate/frame/support/src/traits/messages.rs index 58815b107c829..470567aba1036 100644 --- a/substrate/frame/support/src/traits/messages.rs +++ b/substrate/frame/support/src/traits/messages.rs @@ -121,6 +121,8 @@ impl ServiceQueues for NoopServiceQueues { pub struct QueueFootprint { /// The number of pages in the queue (including overweight pages). pub pages: u32, + /// The number of pages that are ready (not yet processed and also not overweight). + pub ready_pages: u32, /// The storage footprint of the queue (including overweight messages). pub storage: Footprint, } From abd7005b172a4fa689bf5550dd06502dd1491141 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 18 Nov 2023 15:42:41 +0100 Subject: [PATCH 2/8] Use ready_pages in XCMP queue Signed-off-by: Oliver Tale-Yazdi --- cumulus/pallets/xcmp-queue/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs index d687f83d8b3e3..ff83914f43f26 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -610,7 +610,7 @@ impl Pallet { let QueueConfigData { drop_threshold, .. } = >::get(); let fp = T::XcmpQueue::footprint(sender); // Assume that it will not fit into the current page: - let new_pages = fp.pages.saturating_add(1); + let new_pages = fp.ready_pages.saturating_add(1); if new_pages > drop_threshold { // This should not happen since the channel should have been suspended in // [`on_queue_changed`]. @@ -673,12 +673,12 @@ impl OnQueueChanged for Pallet { let mut suspended_channels = >::get(); let suspended = suspended_channels.contains(¶); - if suspended && fp.pages <= resume_threshold { + if suspended && fp.ready_pages <= resume_threshold { Self::send_signal(para, ChannelSignal::Resume); suspended_channels.remove(¶); >::put(suspended_channels); - } else if !suspended && fp.pages >= suspend_threshold { + } else if !suspended && fp.ready_pages >= suspend_threshold { log::warn!("XCMP queue for sibling {:?} is full; suspending channel.", para); Self::send_signal(para, ChannelSignal::Suspend); From e842cc62161f3187965d2eb012afd4a76d2cf17e Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 18 Nov 2023 15:42:50 +0100 Subject: [PATCH 3/8] Fix tests Signed-off-by: Oliver Tale-Yazdi --- cumulus/pallets/xcmp-queue/src/mock.rs | 1 + cumulus/pallets/xcmp-queue/src/tests.rs | 6 +++++- substrate/frame/message-queue/src/mock.rs | 4 ++-- substrate/frame/message-queue/src/tests.rs | 6 +++--- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cumulus/pallets/xcmp-queue/src/mock.rs b/cumulus/pallets/xcmp-queue/src/mock.rs index 7c3a3bd1bd02c..552344b2dd424 100644 --- a/cumulus/pallets/xcmp-queue/src/mock.rs +++ b/cumulus/pallets/xcmp-queue/src/mock.rs @@ -246,6 +246,7 @@ impl> EnqueueMessage for EnqueueToLocalStorage } } footprint.pages = footprint.storage.size as u32 / 16; // Number does not matter + footprint.ready_pages = footprint.pages; footprint } } diff --git a/cumulus/pallets/xcmp-queue/src/tests.rs b/cumulus/pallets/xcmp-queue/src/tests.rs index 30dba6ead3407..a42f55cc6d095 100644 --- a/cumulus/pallets/xcmp-queue/src/tests.rs +++ b/cumulus/pallets/xcmp-queue/src/tests.rs @@ -116,7 +116,11 @@ fn xcm_enqueueing_starts_dropping_on_overflow() { // The drop threshold for pages is 48, the others numbers dont really matter: assert_eq!( ::XcmpQueue::footprint(1000.into()), - QueueFootprint { storage: Footprint { count: 256, size: 768 }, pages: 48 } + QueueFootprint { + storage: Footprint { count: 256, size: 768 }, + pages: 48, + ready_pages: 48 + } ); }) } diff --git a/substrate/frame/message-queue/src/mock.rs b/substrate/frame/message-queue/src/mock.rs index 55a6457435423..6af76f649fab9 100644 --- a/substrate/frame/message-queue/src/mock.rs +++ b/substrate/frame/message-queue/src/mock.rs @@ -367,6 +367,6 @@ pub fn num_overweight_enqueued_events() -> u32 { .count() as u32 } -pub fn fp(pages: u32, count: u64, size: u64) -> QueueFootprint { - QueueFootprint { storage: Footprint { count, size }, pages } +pub fn fp(ready_pages: u32, pages: u32, count: u64, size: u64) -> QueueFootprint { + QueueFootprint { storage: Footprint { count, size }, pages, ready_pages } } diff --git a/substrate/frame/message-queue/src/tests.rs b/substrate/frame/message-queue/src/tests.rs index d94ad581ea0d5..a2644d6353030 100644 --- a/substrate/frame/message-queue/src/tests.rs +++ b/substrate/frame/message-queue/src/tests.rs @@ -1064,13 +1064,13 @@ fn footprint_num_pages_works() { MessageQueue::enqueue_message(msg("weight=2"), Here); MessageQueue::enqueue_message(msg("weight=3"), Here); - assert_eq!(MessageQueue::footprint(Here), fp(2, 2, 16)); + assert_eq!(MessageQueue::footprint(Here), fp(2, 2, 2, 16)); // Mark the messages as overweight. assert_eq!(MessageQueue::service_queues(1.into_weight()), 0.into_weight()); assert_eq!(System::events().len(), 2); // Overweight does not change the footprint. - assert_eq!(MessageQueue::footprint(Here), fp(2, 2, 16)); + assert_eq!(MessageQueue::footprint(Here), fp(0, 2, 2, 16)); // Now execute the second message. assert_eq!( @@ -1078,7 +1078,7 @@ fn footprint_num_pages_works() { .unwrap(), 3.into_weight() ); - assert_eq!(MessageQueue::footprint(Here), fp(1, 1, 8)); + assert_eq!(MessageQueue::footprint(Here), fp(0, 1, 1, 8)); // And the first one: assert_eq!( ::execute_overweight(2.into_weight(), (Here, 0, 0)) From 081b4dc7eb6f0a03485e20e888a37f7d9f05640b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 20 Nov 2023 11:30:51 +0100 Subject: [PATCH 4/8] Amend tests Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/message-queue/src/integration_test.rs | 5 +++++ substrate/frame/message-queue/src/tests.rs | 7 ++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/substrate/frame/message-queue/src/integration_test.rs b/substrate/frame/message-queue/src/integration_test.rs index 965b96a99ca52..11ff1c1448ae4 100644 --- a/substrate/frame/message-queue/src/integration_test.rs +++ b/substrate/frame/message-queue/src/integration_test.rs @@ -288,6 +288,11 @@ fn process_some_messages(num_msgs: u32) { ServiceWeight::set(Some(weight)); let consumed = next_block(); + for origin in BookStateFor::::iter_keys() { + let fp = MessageQueue::footprint(origin); + assert_eq!(fp.pages, fp.ready_pages); + } + assert_eq!(consumed, weight, "\n{}", MessageQueue::debug_info()); assert_eq!(NumMessagesProcessed::take(), num_msgs as usize); } diff --git a/substrate/frame/message-queue/src/tests.rs b/substrate/frame/message-queue/src/tests.rs index a2644d6353030..6be8a6ac90550 100644 --- a/substrate/frame/message-queue/src/tests.rs +++ b/substrate/frame/message-queue/src/tests.rs @@ -1069,7 +1069,7 @@ fn footprint_num_pages_works() { // Mark the messages as overweight. assert_eq!(MessageQueue::service_queues(1.into_weight()), 0.into_weight()); assert_eq!(System::events().len(), 2); - // Overweight does not change the footprint. + // `ready_pages` decreases but `page` count does not. assert_eq!(MessageQueue::footprint(Here), fp(0, 2, 2, 16)); // Now execute the second message. @@ -1086,6 +1086,11 @@ fn footprint_num_pages_works() { 2.into_weight() ); assert_eq!(MessageQueue::footprint(Here), Default::default()); + assert_eq!(MessageQueue::footprint(Here), fp(0, 0, 0, 0)); + + // `ready_pages` and normal `pages` increases again: + MessageQueue::enqueue_message(msg("weight=3"), Here); + assert_eq!(MessageQueue::footprint(Here), fp(1, 1, 1, 8)); }) } From 46fd4e655ee37bf4435bb4473ab1c812d594a31d Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 22 Nov 2023 13:53:57 +0100 Subject: [PATCH 5/8] Fix mock function Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/message-queue/src/mock.rs | 2 +- substrate/frame/message-queue/src/tests.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/message-queue/src/mock.rs b/substrate/frame/message-queue/src/mock.rs index 6af76f649fab9..9f9180f235892 100644 --- a/substrate/frame/message-queue/src/mock.rs +++ b/substrate/frame/message-queue/src/mock.rs @@ -367,6 +367,6 @@ pub fn num_overweight_enqueued_events() -> u32 { .count() as u32 } -pub fn fp(ready_pages: u32, pages: u32, count: u64, size: u64) -> QueueFootprint { +pub fn fp(pages: u32, ready_pages: u32, count: u64, size: u64) -> QueueFootprint { QueueFootprint { storage: Footprint { count, size }, pages, ready_pages } } diff --git a/substrate/frame/message-queue/src/tests.rs b/substrate/frame/message-queue/src/tests.rs index 6be8a6ac90550..fdcf4cb73b0a0 100644 --- a/substrate/frame/message-queue/src/tests.rs +++ b/substrate/frame/message-queue/src/tests.rs @@ -1070,7 +1070,7 @@ fn footprint_num_pages_works() { assert_eq!(MessageQueue::service_queues(1.into_weight()), 0.into_weight()); assert_eq!(System::events().len(), 2); // `ready_pages` decreases but `page` count does not. - assert_eq!(MessageQueue::footprint(Here), fp(0, 2, 2, 16)); + assert_eq!(MessageQueue::footprint(Here), fp(2, 0, 2, 16)); // Now execute the second message. assert_eq!( @@ -1078,7 +1078,7 @@ fn footprint_num_pages_works() { .unwrap(), 3.into_weight() ); - assert_eq!(MessageQueue::footprint(Here), fp(0, 1, 1, 8)); + assert_eq!(MessageQueue::footprint(Here), fp(1, 0, 1, 8)); // And the first one: assert_eq!( ::execute_overweight(2.into_weight(), (Here, 0, 0)) From 470266065eaf93282cc3e633bd7ee5c4cc243992 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 6 Dec 2023 16:25:22 +0100 Subject: [PATCH 6/8] Add simple try_state check for queue footprints Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/message-queue/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index 8e8e716694759..cf813f9e2173a 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -1183,6 +1183,13 @@ impl Pallet { "Memory Corruption in Pages" ); + // Basic checks for each book + for book in BookStateFor::::iter_values() { + let fp: QueueFootprint = book.into(); + + ensure!(fp.ready_pages <= fp.pages, "There can be no more ready than total pages"); + } + // No state to check if ServiceHead::::get().is_none() { return Ok(()) From 485d61b55c7e52d3721f09903e906d1cbedb2851 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 5 Mar 2024 13:54:03 +0100 Subject: [PATCH 7/8] fmt Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/message-queue/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/substrate/frame/message-queue/src/lib.rs b/substrate/frame/message-queue/src/lib.rs index 43f8c426be939..61028057394f6 100644 --- a/substrate/frame/message-queue/src/lib.rs +++ b/substrate/frame/message-queue/src/lib.rs @@ -208,8 +208,9 @@ use frame_support::{ defensive, pallet_prelude::*, traits::{ -DefensiveSaturating, Defensive, DefensiveTruncateFrom, EnqueueMessage, ExecuteOverweightError, Footprint, - ProcessMessage, ProcessMessageError, QueueFootprint, QueuePausedQuery, ServiceQueues, + Defensive, DefensiveSaturating, DefensiveTruncateFrom, EnqueueMessage, + ExecuteOverweightError, Footprint, ProcessMessage, ProcessMessageError, QueueFootprint, + QueuePausedQuery, ServiceQueues, }, BoundedSlice, CloneNoBound, DefaultNoBound, }; From c0c74710d8d6dea7de63cdf8929205a9a209153c Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 5 Mar 2024 14:52:03 +0100 Subject: [PATCH 8/8] Add prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_2393.prdoc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 prdoc/pr_2393.prdoc diff --git a/prdoc/pr_2393.prdoc b/prdoc/pr_2393.prdoc new file mode 100644 index 0000000000000..708d017fafd50 --- /dev/null +++ b/prdoc/pr_2393.prdoc @@ -0,0 +1,16 @@ +title: "[XCMP] Use the number of 'ready' pages in XCMP suspend logic" + +doc: + - audience: Runtime Dev + description: | + Semantics of the suspension logic in the XCMP queue pallet change from using the number of + total pages to the number of 'ready' pages. The number of ready pages is now also exposed by + the `MessageQueue` pallet to downstream via the queue `footprint`. + +crates: + - name: cumulus-pallet-xcmp-queue + bump: patch + - name: pallet-message-queue + bump: patch + - name: frame-support + bump: major