substrate icon indicating copy to clipboard operation
substrate copied to clipboard

Bounds EPM's `CurrentPhase`, `SnapshotMetadata` and `Snapshot` storage structs

Open gpestana opened this issue 2 years ago • 6 comments

This PR starts bounding the EPM storage structs. This is a pre-requisite for the staking parachain since the worst case PoV calculation depends on bounded storage types (the StorageInfoTrait must be implemented) and overall just good design even when running on the relay chain.

The initial plan is to remove the pallet-level #[pallet::without_storage_info] annotation and replace with #[pallet::unbounded] on the individual storage structs. From there, we selectively remove the individual #[pallet::unbounded] annotations and refactor the code to bound the storage. This way we can break down this work in multiple PRs/releases.

Design discussion

Some of the type stored in storage that require bounds are returned by primitives/traits implemented in frame-election-provider-support (and elsewhere).

The approach we took here is that the results returned from a dependency that must be stored in storage, must be bounded with the correct bounds without the caller needing to truncate_from or perform checks on the returned results.

For example, the ElectionDataProvider returns the electing voters and electable targets which need to be bounded since they will be stored in the Snapshot storage. The ElectionDataProvider has been refactored to return bounded vecs on electing_voters and electable_targets. Now, the trait ElectionProviderBase has two new associated types (MaxElectingVoters and MaxElectableTargets) which set the bounds for the returned bounded vecs and the EPM ElectionProvider is constraint to use the EPM limits. Note that both ElectionDataProvider::electing_voters and ElectionDataProvider::electable_targets still accept "soft bounds" as input, which allow the caller to reduce further the amount of voters/targets returned.

However, the flow the other way around (caller provides a bounded vec as input to a dependency that currently only accepts vanilla vecs) does not require bounding the inputs. The caller only calls the dependency with bounded_vec.into_inner(). We can do this safely since the caller is controlling the bounds and into_inner() will be within bound limits.

Task list

  • [x] bound CurrenPhase
  • [x] bound SnapshotMetadata
  • [x] bound Snapshot

for another PR(s):

  • bound QueuedSolution
  • bound SignedSubmissionsMap

Migrations

In this PR, no migrations are required, since the storage items refactored have soft bounds that are re-used when setting the new storage bound. As far as the current soft bounds don't change in lock-step with the release of this PR, there is no need for migrations. However, in the future, whenever the bounds of storage items decrease, a migration is required.

Related to https://github.com/paritytech/polkadot-sdk/issues/323 and https://github.com/paritytech/polkadot-sdk/issues/461

gpestana avatar Jul 10 '23 09:07 gpestana

bot rebase

gpestana avatar Aug 09 '23 15:08 gpestana

Rebased

bot fmt

gpestana avatar Aug 09 '23 16:08 gpestana

@gpestana https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3354502 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 20-acb58b23-efd1-4336-ac9c-d6b8fb65462b to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot[bot] avatar Aug 09 '23 16:08 command-bot[bot]

@gpestana Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3354502 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3354502/artifacts/download.

command-bot[bot] avatar Aug 09 '23 16:08 command-bot[bot]

The CI pipeline was cancelled due to failure one of the required jobs. Job name: cargo-check-each-crate-macos Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3354742

paritytech-cicd-pr avatar Aug 09 '23 16:08 paritytech-cicd-pr