Conversation
There was a problem hiding this comment.
Found 2 logic issues and 1 documentation issue in pallet-bags-list and election-provider-support.
Suggestions that couldn't be attached to a specific line
substrate/frame/bags-list/src/lib.rs:314, 325-326, 330
There's an inconsistency in error reporting for the locked state. The direct check in rebag (line 314) results in Error::Locked. However, when rebag calls on_insert or on_remove (lines 325, 330), a lock error from ensure_unlocked is converted to Error::List(ListError::Locked). This means the same condition produces two different errors. The error handling should be made consistent to always return Error::Locked when the pallet is locked.
substrate/frame/election-provider-support/src/lib.rs:204
The ElectionDataProvider trait now includes a generic parameter T: Config = (). This is an unusual pattern that makes the trait's signature confusing, especially since existing implementations do not use it. The purpose of this parameter should be clearly documented with an example, or it should be removed to simplify the trait.
| /// If any nodes needs updating, removal or addition due to a temporary lock, the | ||
| /// [`Call::rebag`] can be used. |
There was a problem hiding this comment.
The documentation for the Lock storage item at lines 269-270 states that Call::rebag can be used when the pallet is locked. However, the implementation of rebag at line 314 explicitly checks if the pallet is unlocked. This contradiction can mislead developers. The documentation should be corrected to state that rebag is also locked.
Moved from: paritytech#7601.
Follow ups to: paritytech#7282.
Closes: paritytech#8146
This PR is the final outcome of a multi-month development period, with a lot of background work
since 2022. Its main aim is to make pallet-staking, alongside its
type ElectionProvidercompatible to be used in a parachain, and report back the validator set to a relay-chain.
This setup is intended to be used for Polkadot, Kusama and Westend relay-chains, with the
corresponding AssetHubs hosting the staking system.
While this PR is quite big, a lot of the diffs are due to adding a relay and parachain runtime
for testing. The following is a guide to help reviewers/auditors distinguish what has actually
changed in this PR.
Added
This PR adds the following new pallets, all of which are not used anywhere yet, with the
exception of one (see
westend-runtimechanges below).pallet-election-provider-multi-blockThis is a set of 4 pallets, capable of implementing an async, multi-page
ElectionProvider.This pallet is not used in any real runtime yet, and is intended to be used in
AssetHub, nextto
pallet-staking-async.pallet-staking-asyncA fork of the old
pallet-staking, with a number of key differences, making it suitable to beused in a parachain:
pallet-session.pallet-authorship.ElectionProvider, aka.pallet-election-provider-multi-block.To compensate for the above, this pallet relies on XCM messages coming from the relay-chain,
informing the pallet of:
validators during that period.
pallet-staking-async-ah-clientandpallet-staking-async-rc-clientAre the two new pallets that facilitate the above communication.
pallet-ahm-testA test-only crate that contains e2e rust-based unit test for all of the above.
pallet-staking-async-rc-runtimeandpallet-staking-async-parachain-runtimeForks of westend and westend-asset-hub, customized to be used for testing all of the above with
Zombienet. It contains a lot of unrelated code as well.
Changed
IdentificationThis mechanism, which lives on the relay-chain, is expressed by
type FullIdentificationandtype FullIdentificationOfin runtimes. It is a way to identify the full data needed to slash a validator. Historically, it was pointing to a validator, and theirstruct Exposure. With the move to Asset-Hub, this is no longer possible for two reasons:Instead, runtimes now move to a new
type FullIdentificationOf = DefaultExposureOf, which will identify a validator with aExposure::default(). This is suboptimal, as it forces us to still store a number of bytes. Yet, it allows any oldFullIdentification, pertaining to an old slash, to be decoded. This compromise is only needed to cater for slashes that happen around the time of AHM.westend-runtimeThis runtime already has the
pallet-staking-async-ah-client, integrated into all the places such that:SessionManagerYet, it is delegating all of the above to its
type Fallback, which is the oldpallet-staking. This is a preparatory step for AHM, and should not be any logical change.pallet-election-provider-multi-phaseThis is the old single-page
ElectionProvider. It has been updated to work with multi-page traits, yet it only supportspage-size = 1for now. It should not have seen any logical changes.pallet-bags-listNow has two new features. 1. It can be
Locked, in which case all updates to it fail with anErr(_), even deletion of a node. This is needed because we cannot alter any nodes in thispallet during a multi-page iteration, aka. multi-page snapshot. 2. To combat this, the same
rebagtransaction can be also be used to remove a node from the list, or add a node to thelist. This is done through the
score_ofapi.See the file changes and tests under
./substrate/frame/bags-listfor more info.RuntimeDebug -> Debug
To facilitate debugging, a number of types'
RuntimeDebugimpl has been changed toDebug. Seeparitytech#3107
Weights
Below is a summary of the weights. These are generated using
staking-async/runtimes/parachain, which assumes 22_500 nominators divided by32pages for Polkadot, and 12_500 nominators divided by16pages in Kusama, both leading to ~700 nominators snapshotted and exported per page. Doubling these parameters would easily slash the PoV weights by half, but with 10MB PoV, these numbers should be good. Also noting that with PoV clawback, we migth get even more proof_size weight back in the runtime. Although, afaik this reclaimed value does not take compression into account.note for PR authors
Details
TODO
Add custom decoder for OffenceDetails.TODO before finalizing PR
Migration Notes
RC::pallet_staking_async_ah_client::on_migration_start()staking::bondstaking::ForcingtoForceNone.RC::pallet_staking_async_ah_client::on_migration_end()AH::pallet_staking_async::ForceEratoForcing::NotForcing.Follow-up
✄ -----------------------------------------------------------------------------
Thank you for your Pull Request! 🙏 Please make sure it follows the contribution guidelines outlined in this
document and fill out the
sections below. Once you're ready to submit your PR for review, please delete this section and leave only the text under
the "Description" heading.
Description
A concise description of what your PR is doing, and what potential issue it is solving. Use Github semantic
linking
to link the PR to an issue that must be closed once this is merged.
Integration
In depth notes about how this PR should be integrated by downstream projects. This part is mandatory, and should be
reviewed by reviewers, if the PR does NOT have the
R0-Silentlabel. In case of aR0-Silent, it can be ignored.Review Notes
In depth notes about the implementation details of your PR. This should be the main guide for reviewers to
understand your approach and effectively review it. If too long, use
<details>.Imagine that someone who is depending on the old code wants to integrate your new code and the only information that
they get is this section. It helps to include example usage and default value here, with a
diffcode-block to showpossibly integration.
Include your leftover TODOs, if any, here.
Checklist
Trequired)You can remove the "Checklist" section once all have been checked. Thank you for your contribution!
✄ -----------------------------------------------------------------------------