From a0c94a4fb6e7a7c01d69025b3faae63e3ff47039 Mon Sep 17 00:00:00 2001 From: Deepanshu Hooda Date: Wed, 1 Mar 2023 17:08:50 +0530 Subject: [PATCH 01/10] feat: Add try_state for message_queue --- frame/message-queue/src/integration_test.rs | 7 +- frame/message-queue/src/lib.rs | 99 +++++++++++++++++++++ frame/message-queue/src/mock.rs | 10 +++ frame/message-queue/src/tests.rs | 79 ++++++++-------- 4 files changed, 154 insertions(+), 41 deletions(-) diff --git a/frame/message-queue/src/integration_test.rs b/frame/message-queue/src/integration_test.rs index 255098b3b1415..a0e6b2bf99dd8 100644 --- a/frame/message-queue/src/integration_test.rs +++ b/frame/message-queue/src/integration_test.rs @@ -22,8 +22,9 @@ use crate::{ mock::{ - new_test_ext, CountingMessageProcessor, IntoWeight, MockedWeightInfo, NumMessagesProcessed, - SuspendedQueues, + build_and_execute, new_test_ext, CountingMessageProcessor, IntoWeight, MockedWeightInfo, + NumMessagesProcessed, + }, mock_helpers::MessageOrigin, *, @@ -130,7 +131,7 @@ fn stress_test_enqueue_and_service() { let max_msg_len = MaxMessageLenOf::::get(); let mut rng = StdRng::seed_from_u64(42); - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let mut msgs_remaining = 0; for _ in 0..blocks { // Start by enqueuing a large number of messages. diff --git a/frame/message-queue/src/lib.rs b/frame/message-queue/src/lib.rs index bed131e5f0669..c9792da1dbbcb 100644 --- a/frame/message-queue/src/lib.rs +++ b/frame/message-queue/src/lib.rs @@ -571,6 +571,11 @@ pub mod pallet { } } + #[cfg(feature = "try-runtime")] + fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { + Self::do_try_state() + } + /// Check all assumptions about [`crate::Config`]. fn integrity_test() { assert!(!MaxMessageLenOf::::get().is_zero(), "HeapSize too low"); @@ -1085,6 +1090,100 @@ impl Pallet { ItemExecutionStatus::Executed(is_processed) } + /// Ensure the correctness of state of this pallet. + /// + /// # Assumptions- + /// + /// If `serviceHead` points to a ready Queue, then BookState of that Queue has: + /// + /// * `message_count` > 0 + /// * `size` > 0 + /// * `end` > begin + /// * Some(ready_neighbours) + /// * If `ready_neighbours.next` == self.origin, then `ready_neighbours.prev` == self.origin + /// (only queue in ring) + /// + /// For Pages(begin to end-1) in BookState: + /// + /// * `remaining` > 0 + /// * `remaining_size` > 0 + /// * `first` <= `last` + /// * Every page can be decoded into peek_* functions + + pub fn do_try_state() -> Result<(), &'static str> { + // No state to check + if ServiceHead::::get().is_none() { + return Ok(()) + } + + //loop around this origin + let starting_origin = ServiceHead::::get().unwrap(); + + let mut service_head = ServiceHead::::get(); + + loop { + if let Some(head) = service_head { + let head_book_state = BookStateFor::::get(&head); + assert!( + head_book_state.message_count > 0, + "There must be some messages if in ReadyRing" + ); + assert!(head_book_state.size > 0, "There must be some messages if in ReadyRing"); + assert!( + head_book_state.end > head_book_state.begin, + "End > Begin if unprocessed messages exists" + ); + assert!( + head_book_state.ready_neighbours.is_some(), + "There must be neighbours if in ReadyRing" + ); + + if head_book_state.ready_neighbours.as_ref().unwrap().next == head { + assert_eq!( + head_book_state.ready_neighbours.as_ref().unwrap().prev, + head, + "Can only happen if only queue in ReadyRing" + ); + } + + for page_index in head_book_state.begin..head_book_state.end { + let page = Pages::::get(&head, page_index).unwrap(); + let remaining_messages = page.remaining; + let mut counted_remaining_messages = 0; + assert!( + remaining_messages > 0.into(), + "These must be some messages that have not been processed yet!" + ); + + for i in 0..u32::MAX { + if let Some((_, processed, _)) = page.peek_index(i as usize) { + if !processed { + counted_remaining_messages += 1; + } + } else { + break + } + } + + assert_eq!( + remaining_messages, + counted_remaining_messages.into(), + "Memory Corruption" + ); + } + + if head_book_state.ready_neighbours.as_ref().unwrap().next == starting_origin { + break + } else { + service_head = + Some(head_book_state.ready_neighbours.as_ref().unwrap().next.clone()); + } + } + } + + Ok(()) + } + /// Print the pages in each queue and the messages in each page. /// /// Processed messages are prefixed with a `*` and the current `begin`ning page with a `>`. diff --git a/frame/message-queue/src/mock.rs b/frame/message-queue/src/mock.rs index a0fe0105671e0..05df394fcc210 100644 --- a/frame/message-queue/src/mock.rs +++ b/frame/message-queue/src/mock.rs @@ -285,6 +285,16 @@ where ext } +pub fn build_and_execute(test: impl FnOnce() -> ()) +where + ::BlockNumber: From, +{ + new_test_ext::().execute_with(|| { + test(); + MessageQueue::do_try_state().unwrap(); + }) +} + /// Set the weight of a specific weight function. pub fn set_weight(name: &str, w: Weight) { MockedWeightInfo::set_weight::(name, w); diff --git a/frame/message-queue/src/tests.rs b/frame/message-queue/src/tests.rs index 15bb905738531..0101aa0bf1b54 100644 --- a/frame/message-queue/src/tests.rs +++ b/frame/message-queue/src/tests.rs @@ -26,22 +26,22 @@ use rand::{rngs::StdRng, Rng, SeedableRng}; #[test] fn mocked_weight_works() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { assert!(::WeightInfo::service_queue_base().is_zero()); }); - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("service_queue_base", Weight::MAX); assert_eq!(::WeightInfo::service_queue_base(), Weight::MAX); }); // The externalities reset it. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { assert!(::WeightInfo::service_queue_base().is_zero()); }); } #[test] fn enqueue_within_one_page_works() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { use MessageOrigin::*; MessageQueue::enqueue_message(msg("a"), Here); MessageQueue::enqueue_message(msg("b"), Here); @@ -76,7 +76,7 @@ fn enqueue_within_one_page_works() { #[test] fn queue_priority_retains() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { use MessageOrigin::*; assert_ring(&[]); MessageQueue::enqueue_message(msg("a"), Everywhere(1)); @@ -107,11 +107,13 @@ fn queue_priority_retains() { #[test] fn queue_priority_reset_once_serviced() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { use MessageOrigin::*; MessageQueue::enqueue_message(msg("a"), Everywhere(1)); MessageQueue::enqueue_message(msg("b"), Everywhere(2)); MessageQueue::enqueue_message(msg("c"), Everywhere(3)); + MessageQueue::do_try_state().unwrap(); + println!("{}", MessageQueue::debug_info()); // service head is 1, it will process a, leaving service head at 2. it also processes b and // empties queue 2, so service head will end at 3. assert_eq!(MessageQueue::service_queues(2.into_weight()), 2.into_weight()); @@ -134,7 +136,7 @@ fn queue_priority_reset_once_serviced() { #[test] fn service_queues_basic_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { MessageQueue::enqueue_messages(vec![msg("a"), msg("ab"), msg("abc")].into_iter(), Here); MessageQueue::enqueue_messages(vec![msg("x"), msg("xy"), msg("xyz")].into_iter(), There); assert_eq!(QueueChanges::take(), vec![(Here, 3, 6), (There, 3, 6)]); @@ -160,13 +162,14 @@ fn service_queues_basic_works() { assert_eq!(MessageQueue::service_queues(Weight::MAX), 2.into_weight()); assert_eq!(MessagesProcessed::take(), vec![(vmsg("xy"), There), (vmsg("xyz"), There)]); assert_eq!(QueueChanges::take(), vec![(There, 0, 0)]); + MessageQueue::do_try_state().unwrap(); }); } #[test] fn service_queues_failing_messages_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("service_page_item", 1.into_weight()); MessageQueue::enqueue_message(msg("badformat"), Here); MessageQueue::enqueue_message(msg("corrupt"), Here); @@ -267,7 +270,7 @@ fn service_queues_suspension_works() { #[test] fn reap_page_permanent_overweight_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // Create 10 pages more than the stale limit. let n = (MaxStale::get() + 10) as usize; for _ in 0..n { @@ -307,7 +310,7 @@ fn reaping_overweight_fails_properly() { use MessageOrigin::*; assert_eq!(MaxStale::get(), 2, "The stale limit is two"); - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // page 0 MessageQueue::enqueue_message(msg("weight=4"), Here); MessageQueue::enqueue_message(msg("a"), Here); @@ -377,7 +380,7 @@ fn reaping_overweight_fails_properly() { #[test] fn service_queue_bails() { // Not enough weight for `service_queue_base`. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("service_queue_base", 2.into_weight()); let mut meter = WeightMeter::from_limit(1.into_weight()); @@ -385,7 +388,7 @@ fn service_queue_bails() { assert!(meter.consumed.is_zero()); }); // Not enough weight for `ready_ring_unknit`. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("ready_ring_unknit", 2.into_weight()); let mut meter = WeightMeter::from_limit(1.into_weight()); @@ -393,7 +396,7 @@ fn service_queue_bails() { assert!(meter.consumed.is_zero()); }); // Not enough weight for `service_queue_base` and `ready_ring_unknit`. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("service_queue_base", 2.into_weight()); set_weight("ready_ring_unknit", 2.into_weight()); @@ -408,7 +411,7 @@ fn service_page_works() { use super::integration_test::Test; // Run with larger page size. use MessageOrigin::*; use PageExecutionStatus::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("service_page_base_completion", 2.into_weight()); set_weight("service_page_item", 3.into_weight()); @@ -445,7 +448,7 @@ fn service_page_works() { #[test] fn service_page_bails() { // Not enough weight for `service_page_base_completion`. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("service_page_base_completion", 2.into_weight()); let mut meter = WeightMeter::from_limit(1.into_weight()); @@ -462,7 +465,7 @@ fn service_page_bails() { assert!(meter.consumed.is_zero()); }); // Not enough weight for `service_page_base_no_completion`. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("service_page_base_no_completion", 2.into_weight()); let mut meter = WeightMeter::from_limit(1.into_weight()); @@ -482,7 +485,7 @@ fn service_page_bails() { #[test] fn service_page_item_bails() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let _guard = StorageNoopGuard::default(); let (mut page, _) = full_page::(); let mut weight = WeightMeter::from_limit(10.into_weight()); @@ -557,7 +560,7 @@ fn service_page_suspension_works() { #[test] fn bump_service_head_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // Create a ready ring with three queues. BookStateFor::::insert(Here, empty_book::()); knit(&Here); @@ -580,7 +583,7 @@ fn bump_service_head_works() { /// `bump_service_head` does nothing when called with an insufficient weight limit. #[test] fn bump_service_head_bails() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("bump_service_head", 2.into_weight()); setup_bump_service_head::(0.into(), 10.into()); @@ -593,7 +596,7 @@ fn bump_service_head_bails() { #[test] fn bump_service_head_trivial_works() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("bump_service_head", 2.into_weight()); let mut meter = WeightMeter::max_limit(); @@ -614,7 +617,7 @@ fn bump_service_head_trivial_works() { #[test] fn bump_service_head_no_head_noops() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // Create a ready ring with three queues. BookStateFor::::insert(Here, empty_book::()); knit(&Here); @@ -633,7 +636,7 @@ fn bump_service_head_no_head_noops() { #[test] fn service_page_item_consumes_correct_weight() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let mut page = page::(b"weight=3"); let mut weight = WeightMeter::from_limit(10.into_weight()); let overweight_limit = 0.into_weight(); @@ -657,7 +660,7 @@ fn service_page_item_consumes_correct_weight() { /// `service_page_item` skips a permanently `Overweight` message and marks it as `unprocessed`. #[test] fn service_page_item_skips_perm_overweight_message() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let mut page = page::(b"TooMuch"); let mut weight = WeightMeter::from_limit(2.into_weight()); let overweight_limit = 0.into_weight(); @@ -696,7 +699,7 @@ fn service_page_item_skips_perm_overweight_message() { #[test] fn peek_index_works() { use super::integration_test::Test; // Run with larger page size. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // Fill a page with messages. let (mut page, msgs) = full_page::(); let msg_enc_len = ItemHeader::<::Size>::max_encoded_len() + 4; @@ -717,7 +720,7 @@ fn peek_index_works() { #[test] fn peek_first_and_skip_first_works() { use super::integration_test::Test; // Run with larger page size. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // Fill a page with messages. let (mut page, msgs) = full_page::(); @@ -740,7 +743,7 @@ fn peek_first_and_skip_first_works() { #[test] fn note_processed_at_pos_works() { use super::integration_test::Test; // Run with larger page size. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let (mut page, msgs) = full_page::(); for i in 0..msgs { @@ -776,7 +779,7 @@ fn note_processed_at_pos_idempotent() { #[test] fn is_complete_works() { use super::integration_test::Test; // Run with larger page size. - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let (mut page, msgs) = full_page::(); assert!(msgs > 3, "Boring"); let msg_enc_len = ItemHeader::<::Size>::max_encoded_len() + 4; @@ -932,7 +935,7 @@ fn page_from_message_max_len_works() { #[test] fn sweep_queue_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { build_triple_ring(); let book = BookStateFor::::get(Here); @@ -968,7 +971,7 @@ fn sweep_queue_works() { #[test] fn sweep_queue_wraps_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { BookStateFor::::insert(Here, empty_book::()); knit(&Here); @@ -981,14 +984,14 @@ fn sweep_queue_wraps_works() { #[test] fn sweep_queue_invalid_noops() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { assert_storage_noop!(MessageQueue::sweep_queue(Here)); }); } #[test] fn footprint_works() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let origin = MessageOrigin::Here; let (page, msgs) = full_page::(); let book = book_for::(&page); @@ -1006,7 +1009,7 @@ fn footprint_works() { /// The footprint of an invalid queue is the default footprint. #[test] fn footprint_invalid_works() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let origin = MessageOrigin::Here; assert_eq!(MessageQueue::footprint(origin), Default::default()); }) @@ -1016,7 +1019,7 @@ fn footprint_invalid_works() { #[test] fn footprint_on_swept_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let mut book = empty_book::(); book.message_count = 3; book.size = 10; @@ -1032,7 +1035,7 @@ fn footprint_on_swept_works() { #[test] fn execute_overweight_works() { - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("bump_service_head", 1.into_weight()); set_weight("service_queue_base", 1.into_weight()); set_weight("service_page_base_completion", 1.into_weight()); @@ -1208,7 +1211,7 @@ fn ready_but_perm_overweight_does_not_panic() { fn ready_ring_knit_basic_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { BookStateFor::::insert(Here, empty_book::()); for i in 0..10 { @@ -1228,7 +1231,7 @@ fn ready_ring_knit_basic_works() { fn ready_ring_knit_and_unknit_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // Place three queues into the storage. BookStateFor::::insert(Here, empty_book::()); BookStateFor::::insert(There, empty_book::()); @@ -1259,7 +1262,7 @@ fn enqueue_message_works() { let max_msg_per_page = ::HeapSize::get() as u64 / (ItemHeader::<::Size>::max_encoded_len() as u64 + 1); - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // Enqueue messages which should fill three pages. let n = max_msg_per_page * 3; for i in 1..=n { @@ -1289,7 +1292,7 @@ fn enqueue_messages_works() { let max_msg_per_page = ::HeapSize::get() as u64 / (ItemHeader::<::Size>::max_encoded_len() as u64 + 1); - new_test_ext::().execute_with(|| { + build_and_execute::(|| { // Enqueue messages which should fill three pages. let n = max_msg_per_page * 3; let msgs = vec![msg("a"); n as usize]; From 6a4a26efb169cacc2e54ca62ec3287f352f55472 Mon Sep 17 00:00:00 2001 From: Deepanshu Hooda <43631678+gitofdeepanshu@users.noreply.github.com> Date: Thu, 2 Mar 2023 13:19:29 +0530 Subject: [PATCH 02/10] Update frame/message-queue/src/lib.rs change if let to while let and modify bump_service_head function Co-authored-by: Oliver Tale-Yazdi --- frame/message-queue/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/message-queue/src/lib.rs b/frame/message-queue/src/lib.rs index c9792da1dbbcb..a3bddab3e68c2 100644 --- a/frame/message-queue/src/lib.rs +++ b/frame/message-queue/src/lib.rs @@ -1121,7 +1121,7 @@ impl Pallet { let mut service_head = ServiceHead::::get(); - loop { + while let Some(head) = Self::bump_service_head(&mut WeightMeter::max_limit()) { if let Some(head) = service_head { let head_book_state = BookStateFor::::get(&head); assert!( From 73b8e3d08cbf9bb852153c9cf13e94eb92f38a02 Mon Sep 17 00:00:00 2001 From: Deepanshu Hooda Date: Thu, 2 Mar 2023 16:19:43 +0530 Subject: [PATCH 03/10] feat: update try_state, add checks for storage --- frame/message-queue/src/integration_test.rs | 4 +- frame/message-queue/src/lib.rs | 115 +++++++++++--------- frame/message-queue/src/tests.rs | 4 +- 3 files changed, 67 insertions(+), 56 deletions(-) diff --git a/frame/message-queue/src/integration_test.rs b/frame/message-queue/src/integration_test.rs index a0e6b2bf99dd8..da563752fd84e 100644 --- a/frame/message-queue/src/integration_test.rs +++ b/frame/message-queue/src/integration_test.rs @@ -23,7 +23,7 @@ use crate::{ mock::{ build_and_execute, new_test_ext, CountingMessageProcessor, IntoWeight, MockedWeightInfo, - NumMessagesProcessed, + NumMessagesProcessed, SuspendedQueues, }, mock_helpers::MessageOrigin, @@ -179,7 +179,7 @@ fn stress_test_queue_suspension() { let max_msg_len = MaxMessageLenOf::::get(); let mut rng = StdRng::seed_from_u64(41); - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let mut suspended = BTreeSet::::new(); let mut msgs_remaining = 0; diff --git a/frame/message-queue/src/lib.rs b/frame/message-queue/src/lib.rs index a3bddab3e68c2..27f2037a3314d 100644 --- a/frame/message-queue/src/lib.rs +++ b/frame/message-queue/src/lib.rs @@ -1098,7 +1098,7 @@ impl Pallet { /// /// * `message_count` > 0 /// * `size` > 0 - /// * `end` > begin + /// * `end` > `begin` /// * Some(ready_neighbours) /// * If `ready_neighbours.next` == self.origin, then `ready_neighbours.prev` == self.origin /// (only queue in ring) @@ -1111,6 +1111,20 @@ impl Pallet { /// * Every page can be decoded into peek_* functions pub fn do_try_state() -> Result<(), &'static str> { + // Checking memory corruption for BookStateFor + assert_eq!( + BookStateFor::::iter_keys().count(), + BookStateFor::::iter_values().count(), + "Memory Corruption in BookStateFor" + ); + + // Checking memory corruption for Pages + assert_eq!( + Pages::::iter_keys().count(), + Pages::::iter_values().count(), + "Memory Corruption in Pages" + ); + // No state to check if ServiceHead::::get().is_none() { return Ok(()) @@ -1119,68 +1133,65 @@ impl Pallet { //loop around this origin let starting_origin = ServiceHead::::get().unwrap(); - let mut service_head = ServiceHead::::get(); - while let Some(head) = Self::bump_service_head(&mut WeightMeter::max_limit()) { - if let Some(head) = service_head { - let head_book_state = BookStateFor::::get(&head); - assert!( - head_book_state.message_count > 0, - "There must be some messages if in ReadyRing" - ); - assert!(head_book_state.size > 0, "There must be some messages if in ReadyRing"); - assert!( - head_book_state.end > head_book_state.begin, - "End > Begin if unprocessed messages exists" + assert!( + BookStateFor::::contains_key(&head), + "Service head must point to an existing book" + ); + + let head_book_state = BookStateFor::::get(&head); + assert!( + head_book_state.message_count > 0, + "There must be some messages if in ReadyRing" + ); + assert!(head_book_state.size > 0, "There must be some messages if in ReadyRing"); + assert!( + head_book_state.end > head_book_state.begin, + "End > Begin if unprocessed messages exists" + ); + assert!( + head_book_state.ready_neighbours.is_some(), + "There must be neighbours if in ReadyRing" + ); + + if head_book_state.ready_neighbours.as_ref().unwrap().next == head { + assert_eq!( + head_book_state.ready_neighbours.as_ref().unwrap().prev, + head, + "Can only happen if only queue in ReadyRing" ); + } + + for page_index in head_book_state.begin..head_book_state.end { + let page = Pages::::get(&head, page_index).unwrap(); + let remaining_messages = page.remaining; + let mut counted_remaining_messages = 0; assert!( - head_book_state.ready_neighbours.is_some(), - "There must be neighbours if in ReadyRing" + remaining_messages > 0.into(), + "These must be some messages that have not been processed yet!" ); - if head_book_state.ready_neighbours.as_ref().unwrap().next == head { - assert_eq!( - head_book_state.ready_neighbours.as_ref().unwrap().prev, - head, - "Can only happen if only queue in ReadyRing" - ); - } - - for page_index in head_book_state.begin..head_book_state.end { - let page = Pages::::get(&head, page_index).unwrap(); - let remaining_messages = page.remaining; - let mut counted_remaining_messages = 0; - assert!( - remaining_messages > 0.into(), - "These must be some messages that have not been processed yet!" - ); - - for i in 0..u32::MAX { - if let Some((_, processed, _)) = page.peek_index(i as usize) { - if !processed { - counted_remaining_messages += 1; - } - } else { - break + for i in 0..u32::MAX { + if let Some((_, processed, _)) = page.peek_index(i as usize) { + if !processed { + counted_remaining_messages += 1; } + } else { + break } - - assert_eq!( - remaining_messages, - counted_remaining_messages.into(), - "Memory Corruption" - ); } - if head_book_state.ready_neighbours.as_ref().unwrap().next == starting_origin { - break - } else { - service_head = - Some(head_book_state.ready_neighbours.as_ref().unwrap().next.clone()); - } + assert_eq!( + remaining_messages, + counted_remaining_messages.into(), + "Memory Corruption" + ); } - } + if head_book_state.ready_neighbours.as_ref().unwrap().next == starting_origin { + break + } + } Ok(()) } diff --git a/frame/message-queue/src/tests.rs b/frame/message-queue/src/tests.rs index 0101aa0bf1b54..d28dd5f7c23c9 100644 --- a/frame/message-queue/src/tests.rs +++ b/frame/message-queue/src/tests.rs @@ -560,7 +560,7 @@ fn service_page_suspension_works() { #[test] fn bump_service_head_works() { use MessageOrigin::*; - build_and_execute::(|| { + new_test_ext::().execute_with(|| { // Create a ready ring with three queues. BookStateFor::::insert(Here, empty_book::()); knit(&Here); @@ -583,7 +583,7 @@ fn bump_service_head_works() { /// `bump_service_head` does nothing when called with an insufficient weight limit. #[test] fn bump_service_head_bails() { - build_and_execute::(|| { + new_test_ext::().execute_with(|| { set_weight("bump_service_head", 2.into_weight()); setup_bump_service_head::(0.into(), 10.into()); From a9624a32e103b5b1b8bd4ffc112130f1b7963821 Mon Sep 17 00:00:00 2001 From: Deepanshu Hooda Date: Mon, 13 Mar 2023 15:22:55 +0530 Subject: [PATCH 04/10] fix: add try_state builder for remaining tests --- frame/message-queue/src/tests.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/frame/message-queue/src/tests.rs b/frame/message-queue/src/tests.rs index d28dd5f7c23c9..b7e0b98232a69 100644 --- a/frame/message-queue/src/tests.rs +++ b/frame/message-queue/src/tests.rs @@ -215,7 +215,7 @@ fn service_queues_failing_messages_works() { #[test] fn service_queues_suspension_works() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { MessageQueue::enqueue_messages(vec![msg("a"), msg("b"), msg("c")].into_iter(), Here); MessageQueue::enqueue_messages(vec![msg("x"), msg("y"), msg("z")].into_iter(), There); MessageQueue::enqueue_messages( @@ -512,7 +512,7 @@ fn service_page_suspension_works() { use MessageOrigin::*; use PageExecutionStatus::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { let (page, mut msgs) = full_page::(); assert!(msgs >= 10, "pre-condition: need at least 10 msgs per page"); let mut book = book_for::(&page); @@ -1095,7 +1095,7 @@ fn execute_overweight_works() { fn permanently_overweight_book_unknits() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("bump_service_head", 1.into_weight()); set_weight("service_queue_base", 1.into_weight()); set_weight("service_page_base_completion", 1.into_weight()); @@ -1132,7 +1132,7 @@ fn permanently_overweight_book_unknits() { fn permanently_overweight_book_unknits_multiple() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { set_weight("bump_service_head", 1.into_weight()); set_weight("service_queue_base", 1.into_weight()); set_weight("service_page_base_completion", 1.into_weight()); @@ -1171,7 +1171,7 @@ fn permanently_overweight_book_unknits_multiple() { fn ready_but_empty_does_not_panic() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { BookStateFor::::insert(Here, empty_book::()); BookStateFor::::insert(There, empty_book::()); @@ -1191,7 +1191,7 @@ fn ready_but_empty_does_not_panic() { fn ready_but_perm_overweight_does_not_panic() { use MessageOrigin::*; - new_test_ext::().execute_with(|| { + build_and_execute::(|| { MessageQueue::enqueue_message(msg("weight=9"), Here); assert_eq!(MessageQueue::service_queues(8.into_weight()), 0.into_weight()); assert_ring(&[]); From d7f3f33b0f7b9f7e34e91455edcb223aaa82ffa4 Mon Sep 17 00:00:00 2001 From: Deepanshu Hooda <43631678+gitofdeepanshu@users.noreply.github.com> Date: Tue, 2 May 2023 09:40:23 +0530 Subject: [PATCH 05/10] Update frame/message-queue/src/mock.rs Co-authored-by: Oliver Tale-Yazdi --- frame/message-queue/src/mock.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/message-queue/src/mock.rs b/frame/message-queue/src/mock.rs index 05df394fcc210..2873ca2179cdc 100644 --- a/frame/message-queue/src/mock.rs +++ b/frame/message-queue/src/mock.rs @@ -285,6 +285,7 @@ where ext } +/// Run the function pointer inside externalities and asserts the try_state hook at the end. pub fn build_and_execute(test: impl FnOnce() -> ()) where ::BlockNumber: From, From a67f631151ee8d8b6ad01e01aa76a5848b7e40e5 Mon Sep 17 00:00:00 2001 From: Deepanshu Hooda <43631678+gitofdeepanshu@users.noreply.github.com> Date: Tue, 2 May 2023 10:01:34 +0530 Subject: [PATCH 06/10] chore: assert statement to ensure --- frame/message-queue/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/message-queue/src/lib.rs b/frame/message-queue/src/lib.rs index 27f2037a3314d..6bc59e5a2257f 100644 --- a/frame/message-queue/src/lib.rs +++ b/frame/message-queue/src/lib.rs @@ -1134,22 +1134,22 @@ impl Pallet { let starting_origin = ServiceHead::::get().unwrap(); while let Some(head) = Self::bump_service_head(&mut WeightMeter::max_limit()) { - assert!( + ensure!( BookStateFor::::contains_key(&head), "Service head must point to an existing book" ); let head_book_state = BookStateFor::::get(&head); - assert!( + ensure!( head_book_state.message_count > 0, "There must be some messages if in ReadyRing" ); - assert!(head_book_state.size > 0, "There must be some messages if in ReadyRing"); - assert!( + ensure!(head_book_state.size > 0, "There must be some messages if in ReadyRing"); + ensure!( head_book_state.end > head_book_state.begin, "End > Begin if unprocessed messages exists" ); - assert!( + ensure!( head_book_state.ready_neighbours.is_some(), "There must be neighbours if in ReadyRing" ); From 3684bc06653a6b45b06aac0382233110bea41119 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 14 Aug 2023 17:48:52 +0200 Subject: [PATCH 07/10] Fix tests Signed-off-by: Oliver Tale-Yazdi --- frame/message-queue/src/integration_test.rs | 2 +- frame/message-queue/src/lib.rs | 2 +- frame/message-queue/src/mock.rs | 6 +++--- frame/message-queue/src/mock_helpers.rs | 8 +++---- frame/message-queue/src/tests.rs | 23 +++++++++------------ 5 files changed, 18 insertions(+), 23 deletions(-) diff --git a/frame/message-queue/src/integration_test.rs b/frame/message-queue/src/integration_test.rs index fb84beee04a84..a1003edf3c92f 100644 --- a/frame/message-queue/src/integration_test.rs +++ b/frame/message-queue/src/integration_test.rs @@ -22,7 +22,7 @@ use crate::{ mock::{ - build_and_execute, new_test_ext, CountingMessageProcessor, IntoWeight, MockedWeightInfo, + build_and_execute, CountingMessageProcessor, IntoWeight, MockedWeightInfo, NumMessagesProcessed, YieldingQueues, }, mock_helpers::MessageOrigin, diff --git a/frame/message-queue/src/lib.rs b/frame/message-queue/src/lib.rs index 0566922cc96ce..57a7cd09eb3a0 100644 --- a/frame/message-queue/src/lib.rs +++ b/frame/message-queue/src/lib.rs @@ -1164,7 +1164,7 @@ impl Pallet { head_book_state.message_count > 0, "There must be some messages if in ReadyRing" ); - ensure!(head_book_state.size > 0, "There must be some messages if in ReadyRing"); + ensure!(head_book_state.size > 0, "There must be some message size if in ReadyRing"); ensure!( head_book_state.end > head_book_state.begin, "End > Begin if unprocessed messages exists" diff --git a/frame/message-queue/src/mock.rs b/frame/message-queue/src/mock.rs index 9ccd21d5034fd..473c5faac4c5d 100644 --- a/frame/message-queue/src/mock.rs +++ b/frame/message-queue/src/mock.rs @@ -298,12 +298,12 @@ where /// Run the function pointer inside externalities and asserts the try_state hook at the end. pub fn build_and_execute(test: impl FnOnce() -> ()) where - ::BlockNumber: From, + BlockNumberFor: From, { new_test_ext::().execute_with(|| { test(); - MessageQueue::do_try_state().unwrap(); - }) + MessageQueue::do_try_state().expect("All invariants must hold after a test"); + }); } /// Set the weight of a specific weight function. diff --git a/frame/message-queue/src/mock_helpers.rs b/frame/message-queue/src/mock_helpers.rs index 4e3cb323be729..d09dcaf1403cb 100644 --- a/frame/message-queue/src/mock_helpers.rs +++ b/frame/message-queue/src/mock_helpers.rs @@ -89,7 +89,7 @@ pub fn page(msg: &[u8]) -> PageOf { } pub fn single_page_book() -> BookStateOf { - BookState { begin: 0, end: 1, count: 1, ..Default::default() } + BookState { begin: 0, end: 1, count: 1, message_count: 1, size: 1, ..Default::default() } } pub fn empty_book() -> BookStateOf { @@ -139,10 +139,8 @@ pub fn setup_bump_service_head( current: <::MessageProcessor as ProcessMessage>::Origin, next: <::MessageProcessor as ProcessMessage>::Origin, ) { - let mut book = single_page_book::(); - book.ready_neighbours = Some(Neighbours::> { prev: next.clone(), next }); - ServiceHead::::put(¤t); - BookStateFor::::insert(¤t, &book); + crate::Pallet::::enqueue_message(msg("1"), current); + crate::Pallet::::enqueue_message(msg("1"), next); } /// Knit a queue into the ready-ring and write it back to storage. diff --git a/frame/message-queue/src/tests.rs b/frame/message-queue/src/tests.rs index aac56548a98f3..b71fc821223f6 100644 --- a/frame/message-queue/src/tests.rs +++ b/frame/message-queue/src/tests.rs @@ -559,14 +559,11 @@ fn service_page_suspension_works() { #[test] fn bump_service_head_works() { use MessageOrigin::*; - test_closure(|| { + build_and_execute::(|| { // Create a ready ring with three queues. - BookStateFor::::insert(Here, empty_book::()); - knit(&Here); - BookStateFor::::insert(There, empty_book::()); - knit(&There); - BookStateFor::::insert(Everywhere(0), empty_book::()); - knit(&Everywhere(0)); + MessageQueue::enqueue_message(msg(";)"), Here); + MessageQueue::enqueue_message(msg(";)"), There); + MessageQueue::enqueue_message(msg(";)"), Everywhere(0)); // Bump 99 times. for i in 0..99 { @@ -582,9 +579,9 @@ fn bump_service_head_works() { /// `bump_service_head` does nothing when called with an insufficient weight limit. #[test] fn bump_service_head_bails() { - test_closure(|| { + build_and_execute::(|| { set_weight("bump_service_head", 2.into_weight()); - setup_bump_service_head::(0.into(), 10.into()); + setup_bump_service_head::(0.into(), 1.into()); let _guard = StorageNoopGuard::default(); let mut meter = WeightMeter::from_limit(1.into_weight()); @@ -608,7 +605,7 @@ fn bump_service_head_trivial_works() { assert_eq!(ServiceHead::::get().unwrap(), 1.into(), "Bumped the head"); assert_eq!(meter.consumed(), 4.into_weight()); - assert_eq!(MessageQueue::bump_service_head(&mut meter), None, "Cannot bump"); + assert_eq!(MessageQueue::bump_service_head(&mut meter), Some(1.into()), "Its a ring"); assert_eq!(meter.consumed(), 6.into_weight()); }); } @@ -1323,7 +1320,7 @@ fn enqueue_messages_works() { #[test] fn service_queues_suspend_works() { use MessageOrigin::*; - test_closure(|| { + build_and_execute::(|| { MessageQueue::enqueue_messages(vec![msg("a"), msg("ab"), msg("abc")].into_iter(), Here); MessageQueue::enqueue_messages(vec![msg("x"), msg("xy"), msg("xyz")].into_iter(), There); assert_eq!(QueueChanges::take(), vec![(Here, 3, 6), (There, 3, 6)]); @@ -1390,7 +1387,7 @@ fn service_queues_suspend_works() { /// Tests that manual overweight execution on a suspended queue errors with `QueueSuspended`. #[test] fn execute_overweight_respects_suspension() { - test_closure(|| { + build_and_execute::(|| { let origin = MessageOrigin::Here; MessageQueue::enqueue_message(msg("weight=5"), origin); // Mark the message as permanently overweight. @@ -1436,7 +1433,7 @@ fn execute_overweight_respects_suspension() { #[test] fn service_queue_suspension_ready_ring_works() { - test_closure(|| { + build_and_execute::(|| { let origin = MessageOrigin::Here; PausedQueues::set(vec![origin]); MessageQueue::enqueue_message(msg("weight=5"), origin); From 1c8da37448a3953ae6fe774b791c9dd568a90ab2 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 14 Aug 2023 17:56:21 +0200 Subject: [PATCH 08/10] Use ensure instead of assert Signed-off-by: Oliver Tale-Yazdi --- frame/message-queue/src/lib.rs | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/frame/message-queue/src/lib.rs b/frame/message-queue/src/lib.rs index 57a7cd09eb3a0..7e3379d0179da 100644 --- a/frame/message-queue/src/lib.rs +++ b/frame/message-queue/src/lib.rs @@ -583,7 +583,7 @@ pub mod pallet { Self::do_try_state() } - /// Check all assumptions about [`crate::Config`]. + /// Check all compile-time assumptions about [`crate::Config`]. fn integrity_test() { assert!(!MaxMessageLenOf::::get().is_zero(), "HeapSize too low"); } @@ -1132,16 +1132,13 @@ impl Pallet { pub fn do_try_state() -> Result<(), &'static str> { // Checking memory corruption for BookStateFor - assert_eq!( - BookStateFor::::iter_keys().count(), - BookStateFor::::iter_values().count(), + ensure!( + BookStateFor::::iter_keys().count() == BookStateFor::::iter_values().count(), "Memory Corruption in BookStateFor" ); - // Checking memory corruption for Pages - assert_eq!( - Pages::::iter_keys().count(), - Pages::::iter_values().count(), + ensure!( + Pages::::iter_keys().count() == Pages::::iter_values().count(), "Memory Corruption in Pages" ); @@ -1175,9 +1172,8 @@ impl Pallet { ); if head_book_state.ready_neighbours.as_ref().unwrap().next == head { - assert_eq!( - head_book_state.ready_neighbours.as_ref().unwrap().prev, - head, + ensure!( + head_book_state.ready_neighbours.as_ref().unwrap().prev == head, "Can only happen if only queue in ReadyRing" ); } @@ -1186,7 +1182,7 @@ impl Pallet { let page = Pages::::get(&head, page_index).unwrap(); let remaining_messages = page.remaining; let mut counted_remaining_messages = 0; - assert!( + ensure!( remaining_messages > 0.into(), "These must be some messages that have not been processed yet!" ); @@ -1201,9 +1197,8 @@ impl Pallet { } } - assert_eq!( - remaining_messages, - counted_remaining_messages.into(), + ensure!( + remaining_messages == counted_remaining_messages.into(), "Memory Corruption" ); } From 980249f265d97dfc0e84184105ea04c3643e4a8b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 14 Aug 2023 18:07:12 +0200 Subject: [PATCH 09/10] Fix function signature and feature gate Signed-off-by: Oliver Tale-Yazdi --- frame/message-queue/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/message-queue/src/lib.rs b/frame/message-queue/src/lib.rs index 7e3379d0179da..5acc3e9d5a138 100644 --- a/frame/message-queue/src/lib.rs +++ b/frame/message-queue/src/lib.rs @@ -579,7 +579,7 @@ pub mod pallet { } #[cfg(feature = "try-runtime")] - fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { + fn try_state(_: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { Self::do_try_state() } @@ -1129,8 +1129,8 @@ impl Pallet { /// * `remaining_size` > 0 /// * `first` <= `last` /// * Every page can be decoded into peek_* functions - - pub fn do_try_state() -> Result<(), &'static str> { + #[cfg(any(test, feature = "try-runtime"))] + pub fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { // Checking memory corruption for BookStateFor ensure!( BookStateFor::::iter_keys().count() == BookStateFor::::iter_values().count(), From 0b0567e8a75a91ccfd6ce5e3e845b8c7ea23ef3d Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 14 Aug 2023 18:17:36 +0200 Subject: [PATCH 10/10] Cleanup code Signed-off-by: Oliver Tale-Yazdi --- frame/message-queue/src/mock_helpers.rs | 7 ++----- frame/message-queue/src/tests.rs | 18 ++++-------------- 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/frame/message-queue/src/mock_helpers.rs b/frame/message-queue/src/mock_helpers.rs index d09dcaf1403cb..f6109c127be12 100644 --- a/frame/message-queue/src/mock_helpers.rs +++ b/frame/message-queue/src/mock_helpers.rs @@ -162,11 +162,8 @@ pub fn unknit(o: &<::MessageProcessor as ProcessMessage> pub fn build_ring( queues: &[<::MessageProcessor as ProcessMessage>::Origin], ) { - for queue in queues { - BookStateFor::::insert(queue, empty_book::()); - } - for queue in queues { - knit::(queue); + for queue in queues.iter() { + crate::Pallet::::enqueue_message(msg("1"), queue.clone()); } assert_ring::(queues); } diff --git a/frame/message-queue/src/tests.rs b/frame/message-queue/src/tests.rs index b71fc821223f6..bcb099a6accd1 100644 --- a/frame/message-queue/src/tests.rs +++ b/frame/message-queue/src/tests.rs @@ -560,10 +560,7 @@ fn service_page_suspension_works() { fn bump_service_head_works() { use MessageOrigin::*; build_and_execute::(|| { - // Create a ready ring with three queues. - MessageQueue::enqueue_message(msg(";)"), Here); - MessageQueue::enqueue_message(msg(";)"), There); - MessageQueue::enqueue_message(msg(";)"), Everywhere(0)); + build_triple_ring(); // Bump 99 times. for i in 0..99 { @@ -612,15 +609,8 @@ fn bump_service_head_trivial_works() { #[test] fn bump_service_head_no_head_noops() { - use MessageOrigin::*; build_and_execute::(|| { - // Create a ready ring with three queues. - BookStateFor::::insert(Here, empty_book::()); - knit(&Here); - BookStateFor::::insert(There, empty_book::()); - knit(&There); - BookStateFor::::insert(Everywhere(0), empty_book::()); - knit(&Everywhere(0)); + build_triple_ring(); // But remove the service head. ServiceHead::::kill(); @@ -933,6 +923,7 @@ fn sweep_queue_works() { use MessageOrigin::*; build_and_execute::(|| { build_triple_ring(); + QueueChanges::take(); let book = BookStateFor::::get(Here); assert!(book.begin != book.end); @@ -968,8 +959,7 @@ fn sweep_queue_works() { fn sweep_queue_wraps_works() { use MessageOrigin::*; build_and_execute::(|| { - BookStateFor::::insert(Here, empty_book::()); - knit(&Here); + build_ring::(&[Here]); MessageQueue::sweep_queue(Here); let book = BookStateFor::::get(Here);