Skip to content

Fix for Issue 4762 #4803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bkchr merged 12 commits into from
Oct 16, 2024
+80 −45
Merged

Fix for Issue 4762 #4803

Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8973908
Fix issue 4762
gotnoshoeson Jun 14, 2024
c39556e
Ready for PR
gotnoshoeson Jun 15, 2024
05197d1
Update inline docs
gotnoshoeson Jun 16, 2024
43add5d
Updates per code review
gotnoshoeson Jun 26, 2024
d3e03ff
Add more notes per review
gotnoshoeson Jun 30, 2024
71d8171
Merge branch 'master' into issue-4762
bkchr Jul 1, 2024
8264ab8
Add prdoc
gotnoshoeson Jul 4, 2024
1a04152
Merge branch 'master' into issue-4762
bkchr Sep 22, 2024
6bbf1a3
Apply suggestions from code review
bkchr Sep 22, 2024
b3551d6
Fix issues
bkchr Oct 16, 2024
2927182
Merge branch 'master' into issue-4762
bkchr Oct 16, 2024
eaba016
Update prdoc/pr_4803.prdoc
bkchr Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions substrate/frame/message-queue/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ pub mod pallet {
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn on_initialize(_n: BlockNumberFor<T>) -> Weight {
if let Some(weight_limit) = T::ServiceWeight::get() {
Self::service_queues(weight_limit)
Self::service_queues(weight_limit, ServiceQueuesContext::OnInitialize)
} else {
Weight::zero()
}
Expand All @@ -655,7 +655,7 @@ pub mod pallet {
fn on_idle(_n: BlockNumberFor<T>, 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))
Self::service_queues(weight_limit.min(remaining_weight), ServiceQueuesContext::OnIdle)
} else {
Weight::zero()
}
Expand Down Expand Up @@ -774,6 +774,16 @@ enum MessageExecutionStatus {
StackLimitReached,
}

/// 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 {
/// Context of on_idle hook
OnIdle,
/// Context of on_initialize hook
OnInitialize,
}

impl<T: Config> Pallet<T> {
/// Knit `origin` into the ready ring right at the end.
///
Expand Down Expand Up @@ -1554,12 +1564,16 @@ impl<T: Get<O>, O: Into<u32>> Get<u32> for IntoU32<T, O> {
impl<T: Config> ServiceQueues for Pallet<T> {
type OverweightMessageAddress = (MessageOriginOf<T>, PageIndex, T::Size);

fn service_queues(weight_limit: Weight) -> Weight {
fn service_queues(weight_limit: Weight, context: ServiceQueuesContext) -> Weight {

@ggwpez ggwpez Jul 3, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that this is implementing a trait function. Can you maybe put the inner logic into a new function and call that from here?

Having the ServiceQueuesContext in the interface would be ugly otherwise.

@gotnoshoeson gotnoshoeson Jul 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I pushed a new commit last night (PST). Now that I'm re-reading this I might not have implemented the way you requested.

@ggwpez ggwpez Sep 23, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, the code does not compile? You are implementing a trait function. Please try to run the pallet tests locally.

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.");
// throw defensive message when service_queues is called from on_initialize
// don't throw message when service_queues is called from on_idle

@franciscoaguirre franciscoaguirre Jun 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add the reason here, that it doesn't matter if on_idle doesn't have enough weight

@gotnoshoeson gotnoshoeson Jun 26, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes sense. Thank you

if matches(context, ServiceQueuesContext::OnInitialize) {
defensive!("Not enough weight to service a single message.");
}
Weight::zero()
});

Expand Down