Skip to content
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.

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

[HRMP] Dont partially modify pages #4710

Merged
merged 5 commits into from
Jun 21, 2024
Merged

[HRMP] Dont partially modify pages #4710

merged 5 commits into from
Jun 21, 2024

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jun 5, 2024

Changes:

  • The XCMP queue does not partially modify pages anymore by using try_mutate instead of mutate.
  • The XCMP queue max page size is now the min between the value that the relay reports and the local limit.

Thanks to whom pointed this out to me via DM.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez changed the title [HRMP] Dont write partial fragments to page [HRMP] Dont partially modify pages Jun 5, 2024
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6406660

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

nice!

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added the T6-XCM This PR/Issue is related to XCM. label Jun 5, 2024
@@ -491,7 +491,7 @@ impl<T: Config> Pallet<T> {
let channel_info =
T::ChannelInfo::get_channel_info(recipient).ok_or(MessageSendError::NoChannel)?;
// Max message size refers to aggregates, or pages. Not to individual fragments.
let max_message_size = channel_info.max_message_size as usize;
let max_message_size = channel_info.max_message_size.min(T::MaxPageSize::get()) as usize;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let max_message_size = channel_info.max_message_size.min(T::MaxPageSize::get()) as usize;
let max_message_size = channel_info.max_message_size.min.saturating_add(XcmpMessageFormat::max_encoded_len())(T::MaxPageSize::get()) as usize;

The XcmpMessageFormat is part of the page. So, if we don't do this, we will never be able to storage a message of max_message_size (when this is smaller than T::MaxPageSize).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but i think this is a hard limit from the relay chain that we cannot go over. So the "max message size" is for a HRMP message - not for the XCM inside of it.

@ggwpez ggwpez enabled auto-merge June 21, 2024 14:05
@ggwpez ggwpez added this pull request to the merge queue Jun 21, 2024
Merged via the queue into master with commit d18d362 Jun 21, 2024
152 of 158 checks passed
@ggwpez ggwpez deleted the oty-xcmp-queue-page branch June 21, 2024 15:11
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Changes:
- The XCMP queue does not partially modify pages anymore by using
`try_mutate` instead of `mutate`.
- The XCMP queue max page size is now the min between the value that the
relay reports and the local limit.

Thanks to whom pointed this out to me via DM.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants