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

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented Jul 10, 2023

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

  • bound CurrenPhase
  • bound SnapshotMetadata
  • 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 paritytech/polkadot-sdk#323 and paritytech/polkadot-sdk#461

@gpestana gpestana added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”. labels Jul 10, 2023
@gpestana gpestana requested a review from a team July 10, 2023 09:11
@gpestana gpestana self-assigned this Jul 10, 2023
@gpestana gpestana requested a review from a team July 10, 2023 09:11
@gpestana gpestana marked this pull request as draft July 10, 2023 09:11
@gpestana gpestana marked this pull request as ready for review July 19, 2023 07:54
@gpestana gpestana requested review from kianenigma and Ank4n July 19, 2023 07:54
@gpestana
Copy link
Contributor Author

gpestana commented Aug 9, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@gpestana gpestana requested a review from Ank4n August 9, 2023 16:11
@gpestana
Copy link
Contributor Author

gpestana commented Aug 9, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Aug 9, 2023

@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
Copy link

command-bot bot commented Aug 9, 2023

@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
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: ✂️ In progress.
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants