Add try-state hook to pallet aura

[pmikolajczyk41]

Description

This PR introduces storage invariants to pallet aura via try-state hook. Checks are also triggered after pallet unit tests.

Completes one of the goals of paritytech/polkadot-sdk#239.

Notes

1. While I went for @kianenigma suggestion and triggered do_try_state after every unit test, I didn't add any tests that would actually trigger panic from these checks. IMHO this would be a bit misleading. For example, suppose that we craft a test that sets CurrentSlot to the maximal value and disallow multiple blocks per slot. Having such a test might give a wrong impression, that the pallet itself guards this and would panic, while in reality, only feature-gated check would do so.
2. I realized that on_initialize will panic if the previous block has cleared Authorities item (e.g. a malfunctioning pallet removed all validators). Is that okay?

Checklist

• My PR includes a detailed description as outlined in the "Description" section above
• My PR follows the labeling requirements of this project (at minimum one label for each A, B, C and D required)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)
• If this PR alters any external APIs or interfaces used by Polkadot, the corresponding Polkadot PR is ready as well as the corresponding Cumulus PR (optional)

[kianenigma]

Not sure sure what this has to be the case -- a manual-seal node (or something similar) that has this pallet in a dormant mode seems valid to me, but woudl not pass this check.

[pmikolajczyk41]

I'm not familiar with either dormant mode nor manual seal, but when authorities_len is zero, then I think that on_initialize would panic at let authority_index = *new_slot % n_authorities as u64;.

[liamaharon]

While I went for @kianenigma suggestion and triggered do_try_state after every unit test, I didn't add any tests that would actually trigger panic from these checks. IMHO this would be a bit misleading. For example, suppose that we craft a test that sets CurrentSlot to the maximal value and disallow multiple blocks per slot. Having such a test might give a wrong impression, that the pallet itself guards this and would panic, while in reality, only feature-gated check would do so.

I think so long as the test is feature gated it's OK. The try-state invariants don't guard against incorrect usage of the pallet, but more so just checks that the pallet doesn't have any internal bugs. I'd personally feel more confident if the fail-cases of our try-state hooks are tested, but curious to hear @kianenigma's thoughts too.

[kianenigma]

The try-state invariants don't guard against incorrect usage of the pallet, but more so just checks that the pallet doesn't have any internal bugs. I'd personally feel more confident if the fail-cases of our try-state hooks are tested, but curious to hear @kianenigma's thoughts too.

I actually have a different opinion. A priori, the end goal, is to write correct code, and make sure we are notified if we have not written correct code.

With that goal in mind, in my experience, I the outcome is better if I craft all tests such that they handle all happy paths, and all validly reachable error paths, and leave what is meant to never reach as-is.

Whatever assumption is in try_state is something that must be held at all times. To prepare a test setup that pokes an invariant usually contains doing some kind of an unorthodox manual operation that is more noise in my mental model of how the pallet works rather than signal that help me understand it better. Moreover, I find such tests to be very hard to maintain.

This is mostly a personal opinion that I acquired after reviewing and maintaining pallet-nomination-pools (which tests some defensive panics).

That being said, I see no big harm in testing invariants and defensive operations either.

[kianenigma]

bot merge