Tracking issue for adding `try_state` hook to all pallets.

[kianenigma]

This hook was added in https://github.com/paritytech/substrate/pull/10174. The idea is as follows:

/// Execute some checks to ensure the internal state of a pallet is consistent.
///
/// Usually, these checks should check all of the invariants that are expected to be held on all of
/// the storage items of your pallet.
///
/// This hook should not alter any storage.
pub trait TryState<BlockNumber> {
	/// Execute the state checks.
	fn try_state(_: BlockNumber) -> Result<(), &'static str>;
}

(source)

In other words, the goal is to focus on invariants that must always be held in a pallet. A naive, yet important example of this is that in pallet-balances, iterating over all users should yield the correct total issuance.

If in doubt, see a number of other pallets that already have this hook to get further inspiration.

Implementations should provide a clear and concise documentation as to why the given invariant must always hold.

Next, we want all pallets to call their try_state after each test. This can easily be achieved with a helper function like:

https://github.com/paritytech/polkadot-sdk/blob/53f615de503c8936c2639f51b99019c64eab1018/substrate/frame/nomination-pools/src/mock.rs#L401-L407

Implementing this hook will allow these checks to be called in try-runtime-cli, and other testing tools that compile a runtime with feature = try-runtime.

Note that for those who want to tackle a particular pallet, you must for sure deeply understand a pallet in order to correctly identify its list of invariants. Moreover, you might do your future generation a favor by documenting the invariants as good as you can.

TLDR Checklist

• [ ] Read the pallet code carefully, and find invariants that must always hold. This is usually in #[pallet::storage] types, but sometimes also in #[pallet::config] trait.
• [ ] Implement the invariants with respect to details mentioned on the trait: everything must be #[cfg(any(feature = "try-runtime"), test)], and no storage must be altered.
• [ ] tweak the test setup to call try_state after each test, as per the example above.

Pallet list

• [ ] alliance
• [ ] assets
• [ ] atomic-swap
• [x] aura https://github.com/paritytech/substrate/pull/14363
• [ ] authority-discovery
• [ ] authorship
• [ ] babe
• [x] bags-list
• [ ] balances
• [x] beefy https://github.com/paritytech/polkadot-sdk/pull/3246
• [ ] beefy-mmr
• [ ] bounties
• [ ] child-bounties
• [x] collective https://github.com/paritytech/substrate/pull/13645
• [ ] contracts
• [ ] conviction-voting
• [ ] democracy
• [x] election-provider-multi-phase https://github.com/paritytech/substrate/pull/13979
• [x] elections-phragmen https://github.com/paritytech/substrate/pull/13979
• [ ] fast-unstake
• [ ] gilt
• [ ] grandpa
• [ ] identity
• [ ] im-online
• [ ] indices https://github.com/paritytech/polkadot-sdk/pull/1789
• [ ] lottery
• [ ] membership
• [ ] merkle-mountain-range
• [x] message-queue https://github.com/paritytech/substrate/issues/13115
• [x] nicks
• [ ] multisig
• [ ] node-authorization
• [x] nomination-pools
• [ ] offences
• [ ] preimage
• [ ] proxy
• [ ] randomness-collective-flip
• [x] ranked-collective https://github.com/paritytech/polkadot-sdk/pull/3007
• [ ] recovery
• [x] referenda @Szegoo https://github.com/paritytech/substrate/pull/13949
• [ ] remark
• [x] root-offences noop
• [x] root-testing noop
• [ ] scheduler
• [ ] session
• [ ] society
• [x] staking
• [ ] state-trie-migration
• [ ] sudo
• [ ] system
• [x] timestamp
• [x] tips @Doordashcon https://github.com/paritytech/substrate/pull/13515
• [ ] transaction-payment
• [ ] transaction-storage
• [x] treasury #1820
• [ ] uniques
• [x] utility
• [x] vesting @Doordashcon https://github.com/paritytech/substrate/pull/13515
• [ ] whitelist

[vjgaur]

I want to take this up. Can you please assign this to me ?

[kianenigma]

I want to take this up. Can you please assign this to me ?

This is a tracking issue, you can go ahead and work on any item, no need to assign 👍

[Doordashcon]

@kianenigma The idea of the try_state hook is to reduce testing code?

So removing duplicate tests after try_state inclusion is mandatory?

[ggwpez]

The idea of the try_state hook is to reduce testing code?

There are actually two advantages that try_state has. Once for invariant assertion in unit tests, but also in try-runtime migration testing. This is very nice since currently we dont know if a pallet's state is valid after a migration ran.
So the goal is to increase test coverage by always checking the invariants. We would probably end up with more testing code, not less :laughing:
There is some more explanation here https://github.com/paritytech/substrate/issues/13115#issuecomment-1380218189

[Doordashcon]

Hello @ggwpez could I list out pallets I think won't require try_state ? For now pallet-timestamp which uses the #[pallet::inherent] and pallet-utility which is stateless.

Hoping this might speed things up for other contributors :)

[ggwpez]

Yes sure @Doordashcon. Marked them together with the nicks pallet.

[Szegoo]

I will start working on the try-state for the referenda pallet if no one else picked that already.

[Polkadot-Forum]

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/discussion-shaping-the-future-of-try-runtime-and-try-runtime-cli/2657/4

[pmikolajczyk41]

aura implemented here: https://github.com/paritytech/substrate/pull/14363

[ayushmishra2005]

Recovery implemented here https://github.com/paritytech/polkadot-sdk/pull/5290

[rainbowpromise]

is the asset-conversion pallet part of this list? @kianenigma

[kianenigma]

All pallets with non-trivial logic are.

[nmammeri]

I like the idea of being able to call the try-state logic from unit tests. However, it looks like there is a lot of code duplication when implementing this for all the pallets. Why not have a DoTryState trait or something that will enable us to not repeat the same testing logic for all pallets. Many thanks

[kianenigma]

I like the idea of being able to call the try-state logic from unit tests. However, it looks like there is a lot of code duplication when implementing this for all the pallets. Why not have a DoTryState trait or something that will enable us to not repeat the same testing logic for all pallets. Many thanks

Can you give an example of what DoTryState looks like and how it would remove duplicate code? Thanks!

[Krayt78]

This is an old issue but i guess its still usefull ? if so i might try some

[kianenigma]

If you find a useful case where a pallet should have invariants, but it doesn't, yes.