paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

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

Open gpestana opened 1 year ago

gpestana commented 1 year ago

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

for another PR(s):

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 commented 1 year ago

bot rebase

paritytech-processbot[bot] commented 1 year ago

Rebased

gpestana commented 1 year ago

bot fmt

command-bot[bot] commented 1 year ago

@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] commented 1 year ago

@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.

paritytech-cicd-pr commented 1 year ago

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