Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Judgement proxy migration #4

Merged
Changes from all commits
Commits
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
221 changes: 215 additions & 6 deletions relay/kusama/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1832,10 +1832,7 @@ pub mod migrations {
log::info!(target: "runtime::session_keys", "Collecting pre-upgrade session keys state");
let key_ids = SessionKeys::key_ids();
frame_support::ensure!(
key_ids
.into_iter()
.find(|&k| *k == sp_core::crypto::key_types::IM_ONLINE)
.is_none(),
!key_ids.iter().any(|k| *k == sp_core::crypto::key_types::IM_ONLINE),
"New session keys contain the ImOnline key that should have been removed",
);
let storage_key = pallet_session::QueuedKeys::<Runtime>::hashed_key();
Expand All @@ -1851,7 +1848,7 @@ pub mod migrations {
state.extend_from_slice(keys.get_raw(*key_id));
}
});
frame_support::ensure!(state.len() > 0, "Queued keys are not empty before upgrade");
frame_support::ensure!(!state.is_empty(), "Queued keys are not empty before upgrade");
Ok(state)
}

Expand Down Expand Up @@ -1882,7 +1879,10 @@ pub mod migrations {
new_state.extend_from_slice(keys.get_raw(*key_id));
}
});
frame_support::ensure!(new_state.len() > 0, "Queued keys are not empty after upgrade");
frame_support::ensure!(
!new_state.is_empty(),
"Queued keys are not empty after upgrade"
);
frame_support::ensure!(
old_state == new_state,
"Pre-upgrade and post-upgrade keys do not match!"
Expand Down Expand Up @@ -1927,6 +1927,214 @@ pub mod migrations {
}
}

/// Migration to remove deprecated judgement proxies.
mod clear_judgement_proxies {
use super::*;

use frame_support::{
pallet_prelude::ValueQuery,
storage_alias,
traits::{Currency, ReservableCurrency},
Twox64Concat,
};
use frame_system::pallet_prelude::BlockNumberFor;
use pallet_proxy::ProxyDefinition;
use sp_runtime::{BoundedVec, Saturating};

/// ProxyType including the deprecated `IdentityJudgement`.
#[derive(
Copy,
Clone,
Eq,
PartialEq,
Ord,
PartialOrd,
Encode,
Decode,
RuntimeDebug,
MaxEncodedLen,
TypeInfo,
)]
pub enum PrevProxyType {
Any,
NonTransfer,
Governance,
Staking,
IdentityJudgement,
CancelProxy,
Auction,
Society,
NominationPools,
}

type BalanceOf<T> = <<T as pallet_proxy::Config>::Currency as Currency<
<T as frame_system::Config>::AccountId,
>>::Balance;

type PrevProxiesValue<T> = (
BoundedVec<ProxyDefinition<AccountId, PrevProxyType, BlockNumberFor<T>>, MaxProxies>,
BalanceOf<T>,
);

/// Proxies including the deprecated `IdentityJudgement` type.
#[storage_alias]
pub type Proxies<T: pallet_proxy::Config> = StorageMap<
pallet_proxy::Pallet<T>,
Twox64Concat,
AccountId,
PrevProxiesValue<T>,
ValueQuery,
>;

pub struct Migration;
impl OnRuntimeUpgrade for Migration {
/// Compute the expected post-upgrade state for Proxies stroage, and the reserved value
/// for all accounts with a proxy.
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
let mut expected_proxies: BTreeMap<AccountId, PrevProxiesValue<Runtime>> =
BTreeMap::new();
let mut expected_reserved_amounts: BTreeMap<AccountId, Balance> = BTreeMap::new();

for (who, (mut proxies, old_deposit)) in
Proxies::<Runtime>::iter().collect::<Vec<_>>()
{
let proxies_len_before = proxies.len() as u64;
proxies.retain(|proxy| proxy.proxy_type != PrevProxyType::IdentityJudgement);
let proxies_len_after = proxies.len() as u64;

let new_deposit =
pallet_proxy::Pallet::<Runtime>::deposit(proxies.len() as u32);

let current_reserved =
<Balances as ReservableCurrency<AccountId>>::reserved_balance(&who);

// Update the deposit only if proxies were removed and the deposit decreased.
if new_deposit < old_deposit && proxies_len_after < proxies_len_before {
// If there're no proxies left, they should be removed
if proxies.len() > 0 {
expected_proxies.insert(who.clone(), (proxies, new_deposit));
}
expected_reserved_amounts.insert(
who,
current_reserved.saturating_sub(old_deposit - new_deposit),
);
} else {
// Deposit should not change. If any proxies needed to be removed, this
// won't impact that.
expected_proxies.insert(who.clone(), (proxies, old_deposit));
expected_reserved_amounts.insert(who, current_reserved);
Copy link

Choose a reason for hiding this comment

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

Is this correct? The above if only triggers if both were modified. Maybe there needs to be an else if to trigger for either of them changed as well.

Copy link
Author

Choose a reason for hiding this comment

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

It's deliberate, we only make changes if both were modified.

Copy link

Choose a reason for hiding this comment

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

Lets suppose one proxy was removed, so first the user had 5 and now they have 4. But at the same time the deposit function was increased from 20 to 25. Then the num proxies decreased but the deposit stays the same, or?
But its a very edge case 😆

Copy link
Author

Choose a reason for hiding this comment

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

I think that's fine, in that case we just remove the invalid proxy and leave the deposit at 20.

Now that you mention it though my comment is misleading, since the proxies could still have changed. I'll adjust it.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the comment.

}
}

let pre_upgrade_state = (expected_proxies, expected_reserved_amounts);
Ok(pre_upgrade_state.encode())
}

fn on_runtime_upgrade() -> Weight {
let mut reads = 0u64;
let mut writes = 0u64;
let mut proxies_removed_total = 0u64;

Proxies::<Runtime>::translate(
|who: AccountId, (mut proxies, old_deposit): PrevProxiesValue<Runtime>| {
// Remove filter out IdentityJudgement proxies.
let proxies_len_before = proxies.len() as u64;
proxies
.retain(|proxy| proxy.proxy_type != PrevProxyType::IdentityJudgement);
let proxies_len_after = proxies.len() as u64;

let deposit = if proxies_len_before > proxies_len_after {
log::info!(
"Removing {} IdentityJudgement proxies for {:?}",
proxies_len_before - proxies_len_after,
&who
);
proxies_removed_total
.saturating_accrue(proxies_len_before - proxies_len_after);

let new_deposit =
pallet_proxy::Pallet::<Runtime>::deposit(proxies.len() as u32);

// Be kind and don't increase the deposit in case it increased (can
// happen if param change).
let deposit = new_deposit.min(old_deposit);
if deposit < old_deposit {
writes.saturating_inc();
<Balances as ReservableCurrency<AccountId>>::unreserve(
&who,
old_deposit - deposit,
);
}

deposit
} else {
// Nothing to do, use the old deposit.
old_deposit
};

reads.saturating_accrue(proxies_len_before + 1);
writes.saturating_accrue(proxies_len_after + 1);

// No need to keep the k/v around if there're no proxies left.
match proxies.is_empty() {
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
true => {
debug_assert_eq!(deposit, 0);
None
},
false => Some((proxies, deposit)),
}
},
);

log::info!("Removed {} IdentityJudgement proxies in total", proxies_removed_total);
<Runtime as frame_system::Config>::DbWeight::get().reads_writes(reads, writes)
}

#[cfg(feature = "try-runtime")]
fn post_upgrade(state: Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
use frame_support::ensure;
use sp_runtime::TryRuntimeError;

let (expected_proxies, expected_total_reserved): (
BTreeMap<AccountId, PrevProxiesValue<Runtime>>,
BTreeMap<AccountId, Balance>,
) = Decode::decode(&mut &state[..]).expect("Failed to decode pre-upgrade state");

// Check Proxies storage is as expected
for (who, (proxies, deposit)) in Proxies::<Runtime>::iter() {
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
match expected_proxies.get(&who) {
Some((expected_proxies, expected_deposit)) => {
ensure!(&proxies == expected_proxies, "Unexpected Proxy");
ensure!(&deposit == expected_deposit, "Unexpected deposit");
},
None => {
return Err(TryRuntimeError::Other("Missing Proxy"));
},
}
}

// Check the total reserved amounts for every account is as expected
for (who, expected_reserved) in expected_total_reserved.iter() {
let current_reserved =
<Balances as ReservableCurrency<AccountId>>::reserved_balance(who);

ensure!(current_reserved == *expected_reserved, "Reserved balance mismatch");
}

// Check there are no extra entries in the expected state that are not in the
// current state
for (who, _) in expected_proxies.iter() {
if !Proxies::<Runtime>::contains_key(who) {
return Err(TryRuntimeError::Other("Extra entry in expected state"));
}
}

Ok(())
}
}
}

/// Unreleased migrations. Add new ones here:
pub type Unreleased = (
frame_support::migrations::RemovePallet<StateTrieMigrationName, RocksDbWeight>,
Expand Down Expand Up @@ -1960,6 +2168,7 @@ pub mod migrations {
IdentityMigratorPalletName,
<Runtime as frame_system::Config>::DbWeight,
>,
clear_judgement_proxies::Migration,
);

/// Migrations/checks that do not need to be versioned and can run on every update.
Expand Down
Loading