substrate icon indicating copy to clipboard operation
substrate copied to clipboard

Bound uses of `Call`

Open gavofyork opened this issue 3 years ago • 4 comments

Polkadot companion: paritytech/polkadot#5729 Related: https://github.com/paritytech/substrate/issues/12070

Introduces a new utility type Bounded<T> and two new traits QueryPreimage and StorePreimage which are meant to be used instead of the old (and probably soon to be deprecated) PreimageProvider and PreimageRecipient. A new Scheduler API is also introduced (under traits::scheduler::v3) which uses the Bounded<Call> type for being passed the dispatchable (rather than the previous ValueOrHash<Call>).

As a result, Referenda, Democracy, Scheduler and Preimage pallets are all now bounded in storage access footprint. Preimage pallet takes a complexity parameter in the note API allowing the expected PoV weight to be properly profiled without using the impractically pessimistic MEL for the storage item which is degeneratively large. The QueryPreimage and StorePreimage trait APIs also ensure that a length is retained to allow any users of them to similarly profile and bound their weight. This is in use in Scheduler pallet, but will need similar logic introducing for any other pallets which expect to do hash lookups of wildly varying preimage sizes.

Specific Changes

Runtime Primitives

General

  • Introduce DispatchError variants Exhausted, Corruption, Unavailable.

Bounded

  • Implement Default for BoundedSlice
  • Introduce BoundedSlice::truncate_from
  • In BoundedVec, try_insert & try_push return the item to be introduced if the case of error. (NOTE: This has resulted in some minor alterations throughout the codebase.)
  • Similarly BoundedVec::try_from returns the source Vec in case of error.

Frame Support

Schedule

Introduce v3 which removes unbounded components from the previous v2 API.

  • The call param is now Bounded<Call>.
  • The id param is now TaskName = [u8; 32]

Preimages

Introduce preimages.rs containing:

  • New traits StorePreimage and QueryPreimage with more succinct language in use for the operations;
  • New type Bounded<T> which takes a MEL-unbounded type T and provides a MEL-bounded wrapper which can yield the original T through a preimage-based API.
    • Bounded<T> may be created with a StorePreimage::into_bounded(T) -> Result<Bounded<T>>, or alternatively with Bounded<T>::unrequested(hash: Hash, len: u32) or Bounded<T>::requested(hash: Hash, len: u32) and efficiently placed in storage. If the latter API is used, then it cannot be retrieved until the hash's preimage is introduced into the preimages backend (e.g. the Preimage pallet). This makes it quite useful for governance use-cases like Referenda where only the hash is supplied initially.
    • When the original T value is needed, it can be retrieved with QueryPreimage::into_value(b: &Bounded<T>) -> Result<T>.

CallerTrait

  • Introduce a new trait for the Origin::Caller which allows attempting conversion into the RawOrigin (referenced or consuming).
  • Introduce convenience functions into_caller() and as_system_ref to OriginTrait which make use of CallerTrait.
  • Reimplement OriginTrait::as_signed to use these functions.

Frame System

frame_system::Config::Call is now bounded sensibly (Parameter and Dispatchable with the right Origin) to avoid the need to re-bound in other pallets which may need to use it.

Scheduler

  • Functionally decompose on_initialize into four calls which can be weighted in isolation service_agendas, service_agenda, service_task_* and execute_dispatch_*. This is primarily due to the two-stage weight check (first we check PoV-weight for the possible lookup and bail if too much, then we check overall weight for dispatch and bail if too much). It's also quite a lot easier to comprehend.
  • Remove on_initialize weight/benchmark combination in favour of individual benchmarks for the decomposed functions.
  • Refactor tests.
  • For named tasks, use [u8; 32] rather than an (unbounded) Vec.
  • Introduce IncompleteSince allowing O(1) postponing of agenda items.
  • Introduce migrations.
  • Some other refactoring (e.g. introduce place_task).

Referenda

Refactor to ensure well-bounded in PoV footprint.

  • Use scheduler API v3 with Bounded<Call> instead of using call or its hash directly.

Preimage

  • Implements the two new preimages traits.
  • Remove the MaxSize parameter and just handle all reasonable sizes.
  • Index preimage by hash and length.
  • Store the length (if known) in the hash status.
  • This allows for operations using preimage-lookups to have their PoV-footprint benchmarked and known ahead of time.
  • Do tests and migrations.
  • Slight change to API:
    • note_preimage records the preimage and also requests it if it is coming from a superuser (i.e. if there is no depositor). The assumption being that if you're the superuser and are noting a preimage then you probably want it to stick around until you're done with it.

Democracy

Refactor to ensure well-bounded in PoV footprint.

  • Use Bounded<Call> instead of a Hash of the proposal throughout except when the preimage is unneeded for the function (e.g. for blacklisting).
  • Introduce MEL to types throughout.
  • Refactor tests & benchmarks accordingly.
  • Completely remove any preimage handling code.
  • Remove unused complexity parameters from benchmark/weight functions.
  • Introduce EncodeInto (though this should probably be moved to parity_scale_codec in due course).
  • Rearrange the config trait items to keep system-level stuff at the top, then constants, then origins & imbalance handlers.

Migration notes

Runtime storage migrations must be run for:

  • Preimage pallet
  • Scheduler pallet
  • Democracy pallet

Democracy

Three items in the Config trait are removed Proposal, PreimageByteDeposit, OperationalPreimageOrigin.

Three items have been added Preimages, MaxDeposits, MaxBlacklisted; assuming your runtime already uses the Preimages pallet, here are example changes:

impl pallet_democracy::Config for Runtime {
    // snip
    type Proposal = /* snip */;
    type PreimageByteDeposit = /* snip */;
    type OperationalPreimageOrigin = /* snip */;
}

becomes:

impl pallet_democracy::Config for Runtime {
    // snip
    type Preimages = Preimage;
    type MaxDeposits = ConstU32<100>;
    type MaxBlacklisted = ConstU32<100>;
}

There is migration code which moves existing proposals and referenda over to the new format. However IT DOES NOT MIGRATE EVERYTHING:

  • Preimages are NOT migrated. Any registered preimages in Democracy at the time of migration are dropped. Their balance is NOT UNRESERVED.
  • The re-dispatcher used in the old Democracy implementation is removed. Any proposals scheduled for dispatch by Democracy WILL NOT EXECUTE.

This means you SHOULD ensure that:

  • the preimage for the runtime upgrade is placed as an imminent preimage, not with a deposit;
  • no other preimages are in place at the time of upgrade;
  • there are no other proposals scheduled for dispatch by Democracy at the time of upgrade.

The Democracy pallet will be marked as deprecated immediately once Referenda is considered production-ready. ALL TEAMS ARE RECOMMENDED TO SWITCH SWAY FROM DEMOCRACY PALLET TO REFERENDA/CONVICTION-VOTING PALLETS ASAP

Preimage

Preimage pallet has had MaxSize type removed from its Config trait. Example code changes:

impl pallet_preimage::Config for Runtime {
    // snip
    type MaxSize = /* snip */;
}

becomes

impl pallet_preimage::Config for Runtime {
    // snip
}

Scheduler

In Scheduler's Config trait, PreimageProvider has been renamed to Preimages and NoPreimagePostponement has been removed (since its corresponding feature is also removed), so:

impl pallet_scheduler::Config for Runtime {
    // snip
    type PreimageProvider = Preimage;
    type NoPreimagePostponement = NoPreimagePostponement;
}

becomes

impl pallet_scheduler::Config for Runtime {
    // snip
    type Preimages = Preimage;
}

If you find tests are breaking, and especially where Scheduler appears not to be executing scheduled items check the MaximumWeight config parameter of Scheduler and the weight of whatever function is being called. Scheduler has been changed to remove the concept of a "hard deadline" or weight-override priority and no longer guarantees that at least one scheduled item will be executed per block (since these are both dangerous to parachains which have a strict need of weight limits). This means you must ensure that scheduled items are below the MaximumWeight or they will not be executed.

TODO

  • [x] Remove the different Preimage maps; just use real benchmarks for PoV-weight.
  • [x] Consider fixing Democracy pallet's dispatch
  • [x] Migration in Democracy for ReferendumInfoOf, NextExternal, PublicProps.
  • [x] Tests in Preimage pallet should use new preimage trait APIs
  • [x] Tests in Scheduler pallet for changed functionality
  • [x] Test Democracy's migration
  • [x] Scheduler migration test needs a closer look @ggwpez
  • [ ] Re-generate weights and use use new weight functions
  • [ ] Discussions resolved

gavofyork avatar Jun 12 '22 14:06 gavofyork

@ggwpez Should be ready for a full review now.

gavofyork avatar Jun 19 '22 17:06 gavofyork

ill try to do a second pass tmrw focusing on tests, benchmarks, and migration

shawntabrizi avatar Jun 24 '22 03:06 shawntabrizi

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

https://forum.polkadot.network/t/parachain-technical-summit-next-steps/51/12

Polkadot-Forum avatar Sep 08 '22 10:09 Polkadot-Forum

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

https://forum.polkadot.network/t/parachain-technical-summit-next-steps/51/15

Polkadot-Forum avatar Sep 15 '22 08:09 Polkadot-Forum

bot rebase

ggwpez avatar Sep 27 '22 10:09 ggwpez

Rebased

bot rebase

ruseinov avatar Sep 29 '22 07:09 ruseinov

Rebased

CI blocked by https://github.com/paritytech/polkadot/pull/6061

ggwpez avatar Sep 30 '22 11:09 ggwpez

/cmd queue -c bench-bot $ pallet dev pallet_democracy /cmd queue -c bench-bot $ pallet dev pallet_preimage /cmd queue -c bench-bot $ pallet dev pallet_scheduler

ggwpez avatar Oct 03 '22 13:10 ggwpez

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909327 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_democracy. 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 /cmd cancel 2-becafb42-6f03-4678-ad49-75c7c8dbfdcd to cancel this command or /cmd cancel to cancel all commands in this pull request.

command-bot[bot] avatar Oct 03 '22 13:10 command-bot[bot]

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909328 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_preimage. 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 /cmd cancel 3-4b0e470b-5a39-4245-8e37-0db8a5380e70 to cancel this command or /cmd cancel to cancel all commands in this pull request.

command-bot[bot] avatar Oct 03 '22 13:10 command-bot[bot]

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909329 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_scheduler. 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 /cmd cancel 4-18602fea-b27e-4dd8-9581-7720746f57cf to cancel this command or /cmd cancel to cancel all commands in this pull request.

command-bot[bot] avatar Oct 03 '22 13:10 command-bot[bot]

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_preimage has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909328 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909328/artifacts/download.

command-bot[bot] avatar Oct 03 '22 13:10 command-bot[bot]

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_democracy has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909327 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909327/artifacts/download.

command-bot[bot] avatar Oct 03 '22 13:10 command-bot[bot]

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_scheduler has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909329 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909329/artifacts/download.

command-bot[bot] avatar Oct 03 '22 13:10 command-bot[bot]

/cmd queue -c bench-bot $ pallet dev pallet_preimage

ggwpez avatar Oct 03 '22 14:10 ggwpez

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909723 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_preimage. 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 /cmd cancel 5-53e84f22-e73b-44a3-a7b2-e91d5da5f6bf to cancel this command or /cmd cancel to cancel all commands in this pull request.

command-bot[bot] avatar Oct 03 '22 14:10 command-bot[bot]

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_preimage has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909723 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1909723/artifacts/download.

command-bot[bot] avatar Oct 03 '22 14:10 command-bot[bot]

bot merge

gavofyork avatar Oct 05 '22 17:10 gavofyork

Error: "Check reviews" status is not passing for https://github.com/paritytech/polkadot/pull/5729

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

https://forum.polkadot.network/t/polkadot-release-analysis/1026/2

Polkadot-Forum avatar Nov 09 '22 17:11 Polkadot-Forum

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

https://forum.polkadot.network/t/polkadot-digest-11-jan-2023/1654/1

Polkadot-Forum avatar Jan 11 '23 07:01 Polkadot-Forum

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

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-34/1432/8

Polkadot-Forum avatar Jan 11 '23 14:01 Polkadot-Forum

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

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-38/2122/1

Polkadot-Forum avatar Feb 22 '23 16:02 Polkadot-Forum