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

Add TryState hook for MessageQueue #13115

Closed
ggwpez opened this issue Jan 10, 2023 · 7 comments · Fixed by #13502
Closed

Add TryState hook for MessageQueue #13115

ggwpez opened this issue Jan 10, 2023 · 7 comments · Fixed by #13502
Assignees
Labels
I5-tests Tests need fixing, improving or augmenting. T1-runtime This PR/Issue is related to the topic “runtime”. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@ggwpez
Copy link
Member

ggwpez commented Jan 10, 2023

The MessageQueue pallet has quite a few storage assumptions which could be checked in a try_state hook.

👉 Mentor issues are meant for new-comers. Please ask before picking them up.

@ggwpez ggwpez added I5-tests Tests need fixing, improving or augmenting. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder labels Jan 10, 2023
@mahmudsudo
Copy link

i am a newcomer, can i pick this up?

@ggwpez
Copy link
Member Author

ggwpez commented Jan 12, 2023

Yes @mahmudsudo !
Please familiarize yourself with the try_state hook implementations that we have in other pallets and how to write them.
For example in the bags-list pallet or in nomination-pools.
Then take a look at the message-queue and especially all the debug_assert!+defensive! in it. Ideally we check all these invariants in the try_state function.
So for example check that every page can be decoded into peek_* functions etc.

@gitofdeepanshu
Copy link
Contributor

Hey @mahmudsudo , are you still working on this?
If not, I would like to take this over.

@ggwpez
Copy link
Member Author

ggwpez commented Feb 23, 2023

Looks stake, please go ahead @gitofdeepanshu

@gitofdeepanshu
Copy link
Contributor

Hey @ggwpez
I have written the function taking following 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

If all these assumptions are correct then all Queue present in ReadyRing must have atleast 1 message. (Is this correct?)
But there are some tests which add Queue with empty book into ReadyRing which causes try_state function to panic. Should I leave those tests or is there something wrong with my assumptions?

@ggwpez
Copy link
Member Author

ggwpez commented Mar 1, 2023

If all these assumptions are correct then all Queue present in ReadyRing must have atleast 1 message. (Is this correct?)
But there are some tests which add Queue with empty book into ReadyRing which causes try_state function to panic. Should I leave those tests or is there something wrong with my assumptions?

Looks good so far. Please open a Merge request with your changes. I will then go over them in more details.
But AFAIK yes; there should be no empty books in the ready ring.

@gitofdeepanshu
Copy link
Contributor

@ggwpez
I have opened the merge request, kindly let me know if you want me to change anything.

@juangirini juangirini added the T1-runtime This PR/Issue is related to the topic “runtime”. label Jun 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I5-tests Tests need fixing, improving or augmenting. T1-runtime This PR/Issue is related to the topic “runtime”. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants