Groundwork to store validators also in a bags-list instance (2) #10944
Conversation
kianenigma
added
A0-please_review
| //! well-known origin, derivable deterministically from the set of account IDs and the threshold | ||
| //! number of accounts from the set that must approve it. In the case that the threshold is just one | ||
| //! then this is a stateless operation. This is useful for multisig wallets where cryptographic | ||
| //! threshold signatures are not available or desired. |
There was a problem hiding this comment.
Looks like just a line re-wrap? IMO it would be preferable to keep this out since it seems totally unrelated
| V9_0_0, // inject validators into `NPoSVoteProvider` as well. | ||
| V10_0_0, // inject validator into `NPoSTargetList`. |
There was a problem hiding this comment.
| V9_0_0, // inject validators into `NPoSVoteProvider` as well. | |
| V10_0_0, // inject validator into `NPoSTargetList`. | |
| V9_0_0, // inject validators into `VoterList` as well. | |
| V10_0_0, // inject validator into `TargetList`. |
| T::BlockWeights::get().max_block | ||
| } else { | ||
| log!(warn, "InjectValidatorsIntoSortedListProvider being executed on the wrong storage version, expected Releases::V8_0_0"); | ||
| log!(warn, "InjectValidatorsIntoVoterList being executed on the wrong storage version, expected Releases::V8_0_0"); |
There was a problem hiding this comment.
| log!(warn, "InjectValidatorsIntoVoterList being executed on the wrong storage version, expected Releases::V8_0_0"); | |
| log!(warn, "InjectValidatorsIntoVoterList being attempted on the wrong storage version, expected Releases::V8_0_0"); |
maybe? Executed makes it sound like we did the migration anyways
| StorageVersion::<T>::put(Releases::V10_0_0); | ||
| T::BlockWeights::get().max_block | ||
| } else { | ||
| log!(warn, "InjectValidatorsIntoTargetList being executed on the wrong storage version, expected Releases::V9_0_0"); |
There was a problem hiding this comment.
| log!(warn, "InjectValidatorsIntoTargetList being executed on the wrong storage version, expected Releases::V9_0_0"); | |
| log!(warn, "InjectValidatorsIntoTargetList being attempted on the wrong storage version, expected Releases::V9_0_0"); |
Same rationale as above - although not really a big deal
|
|
||
| #[cfg(feature = "try-runtime")] | ||
| fn post_upgrade() -> Result<(), &'static str> { | ||
| ensure!(StorageVersion::<T>::get(), Releases::V10_0_0, "must upgrade linearly"); |
There was a problem hiding this comment.
We should ensure!(Validators::count() == TargetList::count(), "each validator must be in the target list")
There was a problem hiding this comment.
Also T::TargetList::sanity_check().map_err(|_| "TargetList is not in a sane state.")?;
| for (v, _) in Validators::<T>::iter() { | ||
| let weight = Pallet::<T>::vote_weight(&v); | ||
| let _ = T::TargetList::on_insert(v.clone(), weight).map_err(|err| { | ||
| log!(warn, "failed to insert {:?} into TargetList: {:?}", v, err) | ||
| }); | ||
| } |
There was a problem hiding this comment.
To avoid duplicate code we can instead use:
TargetList::unsafe_regenerate(
Validators::<T>::iter().map(|(id, _)| id),
Pallet::<T>::weight_of_fn(),
)There isn't any performance benefit though (originally there was, but that never made it into master because we refactored insert_many to just use the normal insert)
| }) | ||
| pub fn get_npos_targets(maybe_max_len: Option<usize>) -> Vec<T::AccountId> { | ||
| let targets = T::TargetList::iter() | ||
| .take(maybe_max_len.unwrap_or_else(Bounded::max_value)) |
There was a problem hiding this comment.
Bounded::max_value is usize::MAX? This is a bit scary, especially since the point of using the bags list here is to allow it to be virtually unbounded. IMO we should either hardcode a fallback or have a configurable fallback value.
|
|
||
| /// Perform all checks related to both [`Config::TargetList`] and [`Config::VoterList`]. | ||
| #[cfg(debug_assertions)] | ||
| fn sanity_check_list_providers() { |
| } | ||
|
|
||
| /// Perform all checks related to both [`Config::TargetList`] and [`Config::VoterList`]. | ||
| #[cfg(debug_assertions)] |
There was a problem hiding this comment.
Since this function is gated by #[cfg(debug_assertions)], could we remove that same expression from the calls sites i.e. change
#[cfg(debug_assertions)]
Self::sanity_check_list_providers();to simply
Self::sanity_check_list_providers();
| if maybe_max_len.map_or(false, |max_len| target_count > max_len as u32) { | ||
| return Err("Target snapshot too big") | ||
| let targets = Self::get_npos_targets(maybe_max_len); | ||
| if maybe_max_len.map_or(false, |m| targets.len() > m) { |
There was a problem hiding this comment.
If you take my suggestion: https://github.com/paritytech/substrate/pull/10944/files#r816353287, then we should also make sure to check that fallback the upper bound respected if maybe_max_len is None
| <Nominators<T>>::remove_all(); | ||
|
|
||
| T::SortedListProvider::unsafe_clear(); | ||
| T::VoterList::unsafe_clear(); |
There was a problem hiding this comment.
| T::VoterList::unsafe_clear(); | |
| T::VoterList::unsafe_clear(); | |
| T::TargetList::unsafe_clear(); |
|
|
||
| /// A simple voter list implementation that does not require any additional pallets. Note, this | ||
| /// A simple sorted list implementation that does not require any additional pallets. Note, this | ||
| /// does not provided nominators in sorted ordered. If you desire nominators in a sorted order take |
There was a problem hiding this comment.
| /// does not provided nominators in sorted ordered. If you desire nominators in a sorted order take | |
| /// does not provide voters in sorted ordered. If you desire voters in a sorted order take |
| } | ||
|
|
||
| /// A simple sorted list implementation that does not require any additional pallets. Note, this | ||
| /// does not provided validators in sorted ordered. If you desire nominators in a sorted order take |
There was a problem hiding this comment.
| /// does not provided validators in sorted ordered. If you desire nominators in a sorted order take | |
| /// does not provide validators in sorted ordered. If you desire nominators in a sorted order take |
|
|
||
| // NOTE: safe to call outside block production | ||
| T::SortedListProvider::unsafe_clear(); | ||
| T::VoterList::unsafe_clear(); |
There was a problem hiding this comment.
| T::VoterList::unsafe_clear(); | |
| T::VoterList::unsafe_clear(); | |
| T::TargetList::unsafe_clear(); |
|
Check list for making sure target list (not voter list) is correctly called. Columns are the stakers state, rows are the extrinsics that can be applied to a staker.
I checked and this PR looks good against the above requirements. Would be good if others could double check, and also add anything that might be missing here |
| } | ||
| if T::TargetList::contains(&stash) { | ||
| T::TargetList::on_update(&stash, Self::weight_of(&ledger.stash)); | ||
| debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); |
There was a problem hiding this comment.
| debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); | |
| debug_assert_eq!(T::TargetList::sanity_check(), Ok(())); |
We also can remove the debug assert here and above and instead put Self::sanity_check_list_providers(); after these two if statements
| // NOTE: ledger must be updated prior to calling `Self::weight_of`. | ||
| Self::update_ledger(&controller, &ledger); | ||
|
|
||
| // update this staker in the sorted list, if they exist in it. |
There was a problem hiding this comment.
| // update this staker in the sorted list, if they exist in it. | |
| // update this staker in the relevant list(s). |
| // NOTE: ledger must be updated prior to calling `Self::weight_of`. | ||
| Self::update_ledger(&controller, &ledger); | ||
|
|
||
| // update this staker in the sorted list, if they exist in it. |
There was a problem hiding this comment.
| // update this staker in the sorted list, if they exist in it. | |
| // update this staker in the relevant list(s). |
|
This will be re-opened again with a new version that keeps track of approval stakes, not self-stakes. |
louismerlin
removed
the
D9-needsaudit 👮
This PR adds the groundwork to store validators in bags-list, sorted based on stake. This builds on top of #10821 and should be merged after it.
Similar to what we did with nominators, we can deploy this in multiple stages:
Release::V10, and migrate all validators to the bags-list, but use a hypotheticalUseValidatorsMapAndUpdateBagsList.type TargetList = ValidatorsBagsList.