Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add try_state check to Pallet MessageQueue #13502

Conversation

gitofdeepanshu
Copy link
Contributor

@gitofdeepanshu gitofdeepanshu commented Mar 1, 2023

Fix #13115

Description

Add try_state hook implementation for pallet message-queue.

Assumptions:

If serviceHead points to a ready Queue (i.e. has some value), 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 a particular BookState of Queue present in ReadyRing, all pages from begin to end-1 should have:

  • remaining > 0
  • remaining_size > 0
  • first <= last
  • Every page can be decoded into peek_* functions

Test Status

5 tests are failing, if assumptions are correct then these tests are not a part of valid state.

failures:
tests::bump_service_head_bails
tests::bump_service_head_trivial_works
tests::bump_service_head_works
tests::execute_overweight_works
tests::reap_page_permanent_overweight_works

Polkadot address: 12y2pdexQRzCvR7rj6HhMYkLbL3atjikGptSit78eeGWn6ma

@ggwpez ggwpez self-requested a review March 1, 2023 16:43
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I will take a look at the failing tests. Probably not all of them keep the invariants because of mocked storage.

frame/message-queue/src/integration_test.rs Show resolved Hide resolved
frame/message-queue/src/lib.rs Show resolved Hide resolved
frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
frame/message-queue/src/tests.rs Outdated Show resolved Hide resolved
frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
frame/message-queue/src/tests.rs Outdated Show resolved Hide resolved
frame/message-queue/src/tests.rs Show resolved Hide resolved
@dmitry-markin dmitry-markin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 6, 2023
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks.
Please put your address for a tip like described here: https://github.com/paritytech/substrate-tip-bot#pull-request-body

The CI will also need a format, then we can fix the remaining tests.


// No state to check
if ServiceHead::<T>::get().is_none() {
return Ok(())
Copy link
Member

Choose a reason for hiding this comment

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

I think there are still a few assumptions about books even if they are not in the ready ring, but it is okay for a first version.

frame/message-queue/src/mock.rs Show resolved Hide resolved
@stale
Copy link

stale bot commented Apr 15, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Apr 15, 2023
frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
@stale stale bot removed the A3-stale label Apr 15, 2023
@juangirini
Copy link
Contributor

/tip small

@substrate-tip-bot
Copy link

@juangirini A small tip was successfully submitted for gitofdeepanshu (12y2pdexQRzCvR7rj6HhMYkLbL3atjikGptSit78eeGWn6ma on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips tip

@juangirini
Copy link
Contributor

Hey @gitofdeepanshu thanks for your contribution! Would you apply those changes requested by @ggwpez so we can give a final review before merging it?

@juangirini juangirini added the T1-runtime This PR/Issue is related to the topic “runtime”. label May 22, 2023
@juangirini
Copy link
Contributor

Hey @gitofdeepanshu would you be able to take this over again?

@juangirini juangirini requested a review from a team June 9, 2023 14:42
@gitofdeepanshu
Copy link
Contributor Author

I will do it this week, apologies for delay!

{
new_test_ext::<T>().execute_with(|| {
test();
MessageQueue::do_try_state().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

perfecto 👍

@stale
Copy link

stale bot commented Jul 12, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Jul 12, 2023
@juangirini juangirini requested a review from a team July 12, 2023 13:25
@juangirini juangirini requested a review from ggwpez July 12, 2023 13:28
@stale stale bot removed A3-stale labels Jul 12, 2023
@juangirini
Copy link
Contributor

@gitofdeepanshu I am very sorry this took ages to get merged and had a few rebase conflicts now resolved but still it is falling due to the fact that the try_state signature has changed. Would you be able to give it another look and fix that?

@stale
Copy link

stale bot commented Aug 11, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@ggwpez
Copy link
Member

ggwpez commented Aug 14, 2023

bot rebase

@paritytech-processbot
Copy link

Error: Command 'Command { std: cd "/storage/repositories/substrate" && "git" "push" "--porcelain" "gitofdeepanshu" "feat/try-state_message-queue", kill_on_drop: false }' failed with status Some(1); output: error: failed to push some refs to 'https://github.com/gitofdeepanshu/substrate.git'

@ggwpez ggwpez removed the A3-stale label Aug 14, 2023
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez changed the title feat: Add try_state for message_queue Add try_state check to Pallet MessageQueue Aug 14, 2023
@ggwpez
Copy link
Member

ggwpez commented Aug 14, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 34f45c6 into paritytech:master Aug 14, 2023
5 checks passed
Ank4n pushed a commit that referenced this pull request Aug 20, 2023
* feat: Add try_state for message_queue

* 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 <[email protected]>

* feat: update try_state, add checks for storage

* fix: add try_state builder for remaining tests

* Update frame/message-queue/src/mock.rs

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

* chore: assert statement to ensure

* Fix tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use ensure instead of assert

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix function signature and feature gate

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Cleanup code

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Juan <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add TryState hook for MessageQueue
6 participants