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

Migration hook fixes #14174

Merged
merged 13 commits into from
May 23, 2023
Merged

Migration hook fixes #14174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5c35454
fix offences pre_upgrade hook
liamaharon May 18, 2023
7224dfa
identify source of ensure! failures
liamaharon May 18, 2023
4f9f99a
stop migration hooks breaking post migration
liamaharon May 18, 2023
42fbea2
add childbounties storage version
liamaharon May 18, 2023
25c7921
init child bounties version to zero
liamaharon May 18, 2023
a868c55
Merge branch 'master' of github.com:paritytech/substrate into liam-mi…
liamaharon May 22, 2023
716f36e
Update frame/child-bounties/src/lib.rs
liamaharon May 22, 2023
c6cee45
remove redundant preupgrade version checks
liamaharon May 22, 2023
e9d2ae8
update test
liamaharon May 22, 2023
226450e
fix nom pools v3 migration
liamaharon May 22, 2023
3031bca
Merge branch 'master' of github.com:paritytech/substrate into liam-mi…
liamaharon May 23, 2023
4108d95
kick ci
liamaharon May 23, 2023
cd536b6
kick ci
liamaharon May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 37 additions & 33 deletions frame/nomination-pools/src/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,14 +416,14 @@ pub mod v3 {
let current = Pallet::<T>::current_storage_version();
let onchain = Pallet::<T>::on_chain_storage_version();

log!(
info,
"Running migration with current storage version {:?} / onchain {:?}",
current,
onchain
);
if onchain == 2 {
log!(
info,
"Running migration with current storage version {:?} / onchain {:?}",
current,
onchain
);

if current > onchain {
let mut metadata_iterated = 0u64;
let mut metadata_removed = 0u64;
Metadata::<T>::iter_keys()
Expand All @@ -437,7 +437,7 @@ pub mod v3 {
metadata_removed += 1;
Metadata::<T>::remove(&id);
});
current.put::<Pallet<T>>();
StorageVersion::new(3).put::<Pallet<T>>();
// metadata iterated + bonded pools read + a storage version read
let total_reads = metadata_iterated * 2 + 1;
// metadata removed + a storage version write
Expand All @@ -451,10 +451,6 @@ pub mod v3 {

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
ensure!(
Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
"the on_chain version is equal or more than the current one"
);
Ok(Vec::new())
}

Expand All @@ -464,7 +460,10 @@ pub mod v3 {
Metadata::<T>::iter_keys().all(|id| BondedPools::<T>::contains_key(&id)),
"not all of the stale metadata has been removed"
);
ensure!(Pallet::<T>::on_chain_storage_version() == 3, "wrong storage version");
ensure!(
Pallet::<T>::on_chain_storage_version() >= 3,
"nomination-pools::migration::v3: wrong storage version"
);
Ok(())
}
}
Expand Down Expand Up @@ -551,28 +550,35 @@ pub mod v4 {

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
ensure!(
Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
"the on_chain version is equal or more than the current one"
);
Ok(Vec::new())
}

#[cfg(feature = "try-runtime")]
fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
// ensure all BondedPools items now contain an `inner.commission: Commission` field.
ensure!(
BondedPools::<T>::iter().all(|(_, inner)| inner.commission.current.is_none() &&
inner.commission.max.is_none() &&
inner.commission.change_rate.is_none() &&
inner.commission.throttle_from.is_none()),
"a commission value has been incorrectly set"
BondedPools::<T>::iter().all(|(_, inner)|
// Check current
(inner.commission.current.is_none() ||
inner.commission.current.is_some()) &&
// Check max
(inner.commission.max.is_none() || inner.commission.max.is_some()) &&
// Check change_rate
(inner.commission.change_rate.is_none() ||
inner.commission.change_rate.is_some()) &&
// Check throttle_from
(inner.commission.throttle_from.is_none() ||
inner.commission.throttle_from.is_some())),
"a commission value has not been set correctly"
);
Comment on lines 559 to 573
Copy link
Contributor Author

@liamaharon liamaharon May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensure! was failing on Rococo I guess because commission became no longer None for everything after people started using the pallet.

But the checks seem kind of like a noop (checking is_some || is_none). Can we just remove them?

ensure!(
GlobalMaxCommission::<T>::get() == Some(U::get()),
"global maximum commission error"
);
ensure!(Pallet::<T>::on_chain_storage_version() == 4, "wrong storage version");
ensure!(
Pallet::<T>::on_chain_storage_version() >= 4,
"nomination-pools::migration::v4: wrong storage version"
);
Ok(())
}
}
Expand Down Expand Up @@ -636,11 +642,6 @@ pub mod v5 {

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
ensure!(
Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
"the on_chain version is equal or more than the current one"
);

let rpool_keys = RewardPools::<T>::iter_keys().count();
let rpool_values = RewardPools::<T>::iter_values().count();
if rpool_keys != rpool_values {
Expand Down Expand Up @@ -690,13 +691,16 @@ pub mod v5 {
// `total_commission_claimed` field.
ensure!(
RewardPools::<T>::iter().all(|(_, reward_pool)| reward_pool
.total_commission_pending
.is_zero() && reward_pool
.total_commission_claimed
.is_zero()),
.total_commission_pending >=
Zero::zero() && reward_pool
.total_commission_claimed >=
Zero::zero()),
"a commission value has been incorrectly set"
);
ensure!(Pallet::<T>::on_chain_storage_version() == 5, "wrong storage version");
ensure!(
Pallet::<T>::on_chain_storage_version() >= 5,
"nomination-pools::migration::v5: wrong storage version"
);

// These should not have been touched - just in case.
ensure!(
Expand Down
14 changes: 4 additions & 10 deletions frame/offences/src/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,6 @@ pub mod v1 {
impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
let onchain = Pallet::<T>::on_chain_storage_version();
ensure!(onchain < 1, "pallet_offences::MigrateToV1 migration can be deleted");

log::info!(
target: LOG_TARGET,
"Number of reports to refund and delete: {}",
Expand All @@ -67,19 +64,16 @@ pub mod v1 {
}

fn on_runtime_upgrade() -> Weight {
let onchain = Pallet::<T>::on_chain_storage_version();

if onchain > 0 {
if Pallet::<T>::on_chain_storage_version() > 0 {
log::info!(target: LOG_TARGET, "pallet_offences::MigrateToV1 should be removed");
return T::DbWeight::get().reads(1)
}

let keys_removed = v0::ReportsByKindIndex::<T>::clear(u32::MAX, None).unique as u64;
let weight = T::DbWeight::get().reads_writes(keys_removed, keys_removed);

StorageVersion::new(1).put::<Pallet<T>>();

weight
// + 1 for reading/writing the new storage version
T::DbWeight::get().reads_writes(keys_removed + 1, keys_removed + 1)
}

#[cfg(feature = "try-runtime")]
Expand Down Expand Up @@ -147,7 +141,7 @@ mod test {
ext.execute_with(|| {
assert_eq!(
v1::MigrateToV1::<T>::on_runtime_upgrade(),
<T as frame_system::Config>::DbWeight::get().reads_writes(1, 1),
<T as frame_system::Config>::DbWeight::get().reads_writes(2, 2),
);

assert!(<v0::ReportsByKindIndex<T>>::iter_values().count() == 0);
Expand Down