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

Revamp nomination pool reward scheme #11669

Merged
merged 93 commits into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from 81 commits
Commits
Show all changes
93 commits
Select commit Hold shift + click to select a range
ea30833
make pool roles optional
kianenigma May 13, 2022
00e8c87
undo lock file changes?
kianenigma May 13, 2022
fa5e95a
add migration
kianenigma May 13, 2022
f2df79e
add the ability for pools to chill themselves
kianenigma May 14, 2022
56cf997
boilerplate of tests
kianenigma May 14, 2022
508bc0a
new system works, tests need fixing
kianenigma May 15, 2022
f83944a
somewhat stable, but I think I found another bug as well
kianenigma May 15, 2022
ac48c37
Fix it all
kianenigma May 16, 2022
cbeb9fb
Master.into()
kianenigma May 16, 2022
031040f
Add more more sophisticated test + capture one more bug.
kianenigma May 16, 2022
efc7b4f
Update frame/staking/src/lib.rs
kianenigma May 16, 2022
60d42f1
reduce the diff a little bit
kianenigma May 17, 2022
02aa7d4
add some test for the slashing bug
kianenigma May 19, 2022
90db26e
cleanup
kianenigma May 19, 2022
5bf6d9c
Merge branch 'kiz-pool-chill' of github.com:paritytech/substrate into…
kianenigma May 19, 2022
1d8c940
Master.into()
kianenigma May 19, 2022
2ec4857
Merge branch 'master' of github.com:paritytech/substrate into kiz-poo…
kianenigma May 20, 2022
a51e408
fix lock file?
kianenigma May 20, 2022
28c8852
Fix
kianenigma May 20, 2022
c9413a2
fmt
kianenigma May 20, 2022
433476d
Merge branch 'master' of github.com:paritytech/substrate into kiz-poo…
kianenigma May 23, 2022
c34b655
Update frame/nomination-pools/src/lib.rs
kianenigma May 23, 2022
d0d75a1
Update frame/nomination-pools/src/lib.rs
kianenigma May 23, 2022
a6afb06
Update frame/nomination-pools/src/lib.rs
kianenigma May 23, 2022
a3a43e7
Update frame/nomination-pools/src/mock.rs
kianenigma May 23, 2022
4b7b0c7
Fix build
kianenigma May 23, 2022
3f66688
Merge branch 'kiz-pool-chill' of github.com:paritytech/substrate into…
kianenigma May 23, 2022
640ec31
fix some fishy tests..
kianenigma May 23, 2022
398ddfe
add one last integrity check for MinCreateBond
kianenigma May 24, 2022
d5dc697
remove bad assertion -- needs to be dealt with later
kianenigma May 24, 2022
05fb517
Merge branch 'master' of github.com:paritytech/substrate into kiz-poo…
kianenigma May 24, 2022
f4dbd0a
nits
shawntabrizi May 25, 2022
0a79c80
Master.into()
kianenigma Jun 8, 2022
78c0310
Merge branch 'kiz-pool-chill' of github.com:paritytech/substrate into…
kianenigma Jun 9, 2022
0fb1125
fix tests and add benchmarks for chill
kianenigma Jun 9, 2022
12773bb
remove stuff
kianenigma Jun 9, 2022
ca475df
fix benchmarks
kianenigma Jun 10, 2022
44a2722
Merge branch 'master' of https://github.com/paritytech/substrate into…
Jun 10, 2022
f027faf
cargo run --quiet --profile=production --features=runtime-benchmarks…
Jun 10, 2022
33b581c
Master.into()
kianenigma Jun 11, 2022
c77613f
Merge branch 'kiz-pool-chill' of github.com:paritytech/substrate into…
kianenigma Jun 11, 2022
d69af2c
remove defensive
kianenigma Jun 11, 2022
d318197
Merge branch 'master' of github.com:paritytech/substrate into kiz-poo…
kianenigma Jun 13, 2022
486a0e9
first working version
kianenigma Jun 13, 2022
9b2113f
Master.into()
kianenigma Jun 14, 2022
36cb484
bring back all tests
kianenigma Jun 14, 2022
723574b
ALL new tests work now
kianenigma Jun 14, 2022
624abe9
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Jun 14, 2022
82287b0
cleanup
kianenigma Jun 14, 2022
e403fb1
make sure benchmarks and all work
kianenigma Jun 15, 2022
4cad93a
Merge branch 'master' of https://github.com/paritytech/substrate into…
Jun 15, 2022
221369b
cargo run --quiet --profile=production --features=runtime-benchmarks…
Jun 15, 2022
696a55e
round of self-review, make arithmetic safe
kianenigma Jun 15, 2022
0689b58
Merge branch 'kiz-fix-reward-pools-2' of github.com:paritytech/substr…
kianenigma Jun 15, 2022
bcb413c
fix warn
kianenigma Jun 15, 2022
bce40f7
add migration code
kianenigma Jun 16, 2022
8fc25ac
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Jun 16, 2022
fc3ad18
Fix doc
kianenigma Jun 17, 2022
03107f3
Merge branches 'kiz-fix-reward-pools-2' and 'master' of github.com:pa…
kianenigma Jun 21, 2022
9f875a9
add precision notes
kianenigma Jun 21, 2022
0513284
make arithmetic fallible
kianenigma Jun 21, 2022
1c43f09
fix node runtime
kianenigma Jun 21, 2022
ef56db6
a lot of precision tests and notes and stuff
kianenigma Jun 22, 2022
3da2364
document MaxPOintsToBalance better
kianenigma Jun 22, 2022
ed5083f
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Jun 23, 2022
3690489
:round of self-review
kianenigma Jun 23, 2022
62d35c8
fmt
kianenigma Jun 23, 2022
1c840b2
fix some comments
kianenigma Jun 23, 2022
7d9d403
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Jun 26, 2022
78b79f2
Master.into()
kianenigma Jul 1, 2022
a8ccd71
Fix proportional slashing logic
kianenigma Jul 5, 2022
60b7641
Update frame/nomination-pools/src/tests.rs
kianenigma Jul 6, 2022
a2082cd
Update frame/nomination-pools/src/tests.rs
kianenigma Jul 6, 2022
ecb7890
Update frame/nomination-pools/src/lib.rs
kianenigma Jul 6, 2022
7e56e1a
track poinst in migration
kianenigma Jul 6, 2022
80b31c0
Merge branch 'kiz-fix-reward-pools-2' of github.com:paritytech/substr…
kianenigma Jul 6, 2022
579da37
fix
kianenigma Jul 6, 2022
51c1608
fmt
kianenigma Jul 6, 2022
3779081
fix migration
kianenigma Jul 6, 2022
9171a13
remove event read
kianenigma Jul 6, 2022
b9ab747
Apply suggestions from code review
kianenigma Jul 7, 2022
d4f45e7
Update frame/staking/src/lib.rs
kianenigma Jul 9, 2022
ca47b05
Update frame/nomination-pools/src/lib.rs
kianenigma Jul 9, 2022
f3e10a9
Update frame/nomination-pools/src/lib.rs
kianenigma Jul 9, 2022
463ddfd
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Jul 10, 2022
51873c7
Merge branch 'kiz-fix-reward-pools-2' of github.com:paritytech/substr…
kianenigma Jul 10, 2022
c205ac3
update
kianenigma Jul 10, 2022
69d1f9c
fmt
kianenigma Jul 10, 2022
3efe9f5
fmt
kianenigma Jul 10, 2022
84a2639
add one last test
kianenigma Jul 10, 2022
e7673f8
fmt
kianenigma Jul 12, 2022
ed2c225
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Jul 12, 2022
de9886d
merged
kianenigma Jul 13, 2022
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
9 changes: 6 additions & 3 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ use sp_runtime::{
SaturatedConversion, StaticLookup,
},
transaction_validity::{TransactionPriority, TransactionSource, TransactionValidity},
ApplyExtrinsicResult, FixedPointNumber, Perbill, Percent, Permill, Perquintill,
ApplyExtrinsicResult, FixedPointNumber, FixedU128, Perbill, Percent, Permill, Perquintill,
};
use sp_std::prelude::*;
#[cfg(any(feature = "std", test))]
Expand Down Expand Up @@ -730,7 +730,7 @@ impl pallet_bags_list::Config for Runtime {
parameter_types! {
pub const PostUnbondPoolsWindow: u32 = 4;
pub const NominationPoolsPalletId: PalletId = PalletId(*b"py/nopls");
pub const MinPointsToBalance: u32 = 10;
pub const MaxPointsToBalance: u32 = 10;
}

use sp_runtime::traits::Convert;
Expand All @@ -751,14 +751,16 @@ impl pallet_nomination_pools::Config for Runtime {
type WeightInfo = ();
type Event = Event;
type Currency = Balances;
type CurrencyBalance = Balance;
type RewardCounter = FixedU128;
type BalanceToU256 = BalanceToU256;
type U256ToBalance = U256ToBalance;
type StakingInterface = pallet_staking::Pallet<Self>;
type PostUnbondingPoolsWindow = PostUnbondPoolsWindow;
type MaxMetadataLen = ConstU32<256>;
type MaxUnbonding = ConstU32<8>;
type PalletId = NominationPoolsPalletId;
type MinPointsToBalance = MinPointsToBalance;
type MaxPointsToBalance = MaxPointsToBalance;
}

parameter_types! {
Expand Down Expand Up @@ -1675,6 +1677,7 @@ pub type Executive = frame_executive::Executive<
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
pallet_nomination_pools::migration::v2::MigrateToV2<Runtime>,
>;

/// MMR helper types.
Expand Down
11 changes: 8 additions & 3 deletions frame/nomination-pools/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@

use frame_election_provider_support::VoteWeight;
use frame_support::{pallet_prelude::*, parameter_types, traits::ConstU64, PalletId};
use sp_runtime::traits::{Convert, IdentityLookup};
use sp_runtime::{
traits::{Convert, IdentityLookup},
FixedU128,
};

type AccountId = u128;
type AccountIndex = u32;
Expand Down Expand Up @@ -144,21 +147,23 @@ impl Convert<sp_core::U256, Balance> for U256ToBalance {
parameter_types! {
pub static PostUnbondingPoolsWindow: u32 = 10;
pub const PoolsPalletId: PalletId = PalletId(*b"py/nopls");
pub const MinPointsToBalance: u32 = 10;
pub const MaxPointsToBalance: u32 = 10;
}

impl pallet_nomination_pools::Config for Runtime {
type Event = Event;
type WeightInfo = ();
type Currency = Balances;
type CurrencyBalance = Balance;
type RewardCounter = FixedU128;
type BalanceToU256 = BalanceToU256;
type U256ToBalance = U256ToBalance;
type StakingInterface = Staking;
type PostUnbondingPoolsWindow = PostUnbondingPoolsWindow;
type MaxMetadataLen = ConstU32<256>;
type MaxUnbonding = ConstU32<8>;
type PalletId = PoolsPalletId;
type MinPointsToBalance = MinPointsToBalance;
type MaxPointsToBalance = MaxPointsToBalance;
}

impl crate::Config for Runtime {}
Expand Down
582 changes: 295 additions & 287 deletions frame/nomination-pools/src/lib.rs

Large diffs are not rendered by default.

267 changes: 265 additions & 2 deletions frame/nomination-pools/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@
// limitations under the License.

use super::*;
use crate::log;
use frame_support::traits::OnRuntimeUpgrade;
use sp_std::collections::btree_map::BTreeMap;

pub mod v1 {
use super::*;
use crate::log;
use frame_support::traits::OnRuntimeUpgrade;

#[derive(Decode)]
pub struct OldPoolRoles<AccountId> {
Expand Down Expand Up @@ -103,3 +104,265 @@ pub mod v1 {
}
}
}

pub mod v2 {
use super::*;
use sp_runtime::Perbill;

#[test]
fn migration_assumption_is_correct() {
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
// this migrations cleans all the reward accounts to contain exactly ed, and all members
// having no claimable rewards. In this state, all fields of the `RewardPool` and
// `member.last_recorded_reward_counter` are all zero.
use crate::mock::*;
ExtBuilder::default().build_and_execute(|| {
let join = |x| {
Balances::make_free_balance_be(&x, Balances::minimum_balance() + 10);
frame_support::assert_ok!(Pools::join(Origin::signed(x), 10, 1));
};

assert_eq!(BondedPool::<Runtime>::get(1).unwrap().points, 10);
assert_eq!(
RewardPools::<Runtime>::get(1).unwrap(),
RewardPool { ..Default::default() }
);
assert_eq!(
PoolMembers::<Runtime>::get(10).unwrap().last_recorded_reward_counter,
Zero::zero()
);

join(20);
assert_eq!(BondedPool::<Runtime>::get(1).unwrap().points, 20);
assert_eq!(
RewardPools::<Runtime>::get(1).unwrap(),
RewardPool { ..Default::default() }
);
assert_eq!(
PoolMembers::<Runtime>::get(10).unwrap().last_recorded_reward_counter,
Zero::zero()
);
assert_eq!(
PoolMembers::<Runtime>::get(20).unwrap().last_recorded_reward_counter,
Zero::zero()
);

join(30);
assert_eq!(BondedPool::<Runtime>::get(1).unwrap().points, 30);
assert_eq!(
RewardPools::<Runtime>::get(1).unwrap(),
RewardPool { ..Default::default() }
);
assert_eq!(
PoolMembers::<Runtime>::get(10).unwrap().last_recorded_reward_counter,
Zero::zero()
);
assert_eq!(
PoolMembers::<Runtime>::get(20).unwrap().last_recorded_reward_counter,
Zero::zero()
);
assert_eq!(
PoolMembers::<Runtime>::get(30).unwrap().last_recorded_reward_counter,
Zero::zero()
);
});
}

#[derive(Decode)]
pub struct OldRewardPool<B> {
pub balance: B,
pub total_earnings: B,
pub points: U256,
}

#[derive(Decode)]
pub struct OldPoolMember<T: Config> {
pub pool_id: PoolId,
pub points: BalanceOf<T>,
pub reward_pool_total_earnings: BalanceOf<T>,
pub unbonding_eras: BoundedBTreeMap<EraIndex, BalanceOf<T>, T::MaxUnbonding>,
}

/// Migrate the pool reward scheme to the new version, as per
/// <https://github.com/paritytech/substrate/pull/11669.>.
pub struct MigrateToV2<T>(sp_std::marker::PhantomData<T>);
impl<T: Config> MigrateToV2<T> {
fn run(current: StorageVersion) -> Weight {
let mut reward_pools_translated = 0u64;
let mut members_translated = 0u64;
// just for logging.
let mut total_value_locked = BalanceOf::<T>::zero();
let mut total_points_locked = BalanceOf::<T>::zero();

// store each member of the pool, with their active points. In the process, migrate
// their data as well.
let mut temp_members = BTreeMap::<PoolId, Vec<(T::AccountId, BalanceOf<T>)>>::new();
PoolMembers::<T>::translate::<OldPoolMember<T>, _>(|key, old_member| {
let id = old_member.pool_id;
temp_members.entry(id).or_default().push((key, old_member.points));

total_points_locked += old_member.points;
members_translated += 1;
Some(PoolMember::<T> {
last_recorded_reward_counter: Zero::zero(),
pool_id: old_member.pool_id,
points: old_member.points,
unbonding_eras: old_member.unbonding_eras,
})
});

// translate all reward pools. In the process, do the last payout as well.
RewardPools::<T>::translate::<OldRewardPool<BalanceOf<T>>, _>(
|id, _old_reward_pool| {
// each pool should have at least one member.
let members = match temp_members.get(&id) {
Some(x) => x,
None => {
log!(error, "pool {} has no member! deleting it..", id);
Copy link
Member

Choose a reason for hiding this comment

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

Is there an advantage of making migrations lenient in the error case?
I think this makes it harder to verify that it worked correctly, since the log must be checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What else can be done about it though?

Copy link
Member

Choose a reason for hiding this comment

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

Huh… maybe for future migrations we can add a strict bool, which returns an error in case of corrupt storage.
Then it could be re-run with struct=false if its deemed non-critical.
Since when such a case happens, it means that there is a serous bug in a different part of the code that we should not miss. I dont know if the release engineers always check the log, i think its quite a lot of output.

return None
},
};
let bonded_pool = match BondedPools::<T>::get(id) {
Some(x) => x,
None => {
log!(error, "pool {} has no bonded pool! deleting it..", id);
return None
},
};

let accumulated_reward = RewardPool::<T>::current_balance(id);
let reward_account = Pallet::<T>::create_reward_account(id);
let mut sum_paid_out = BalanceOf::<T>::zero();

members
.into_iter()
.filter_map(|(who, points)| {
let bonded_pool = match BondedPool::<T>::get(id) {
Some(x) => x,
None => {
log!(error, "pool {} for member {:?} does not exist!", id, who);
return None
},
};

total_value_locked += bonded_pool.points_to_balance(points.clone());
let portion = Perbill::from_rational(*points, bonded_pool.points);
let last_claim = portion * accumulated_reward;

log!(
debug,
"{:?} has {:?} ({:?}) of pool {} with total reward of {:?}",
who,
portion,
last_claim,
id,
accumulated_reward
);

if last_claim.is_zero() {
None
} else {
Some((who, last_claim))
}
})
.for_each(|(who, last_claim)| {
let outcome = T::Currency::transfer(
&reward_account,
&who,
last_claim,
ExistenceRequirement::KeepAlive,
);

if let Err(reason) = outcome {
log!(warn, "last reward claim failed due to {:?}", reason,);
} else {
sum_paid_out = sum_paid_out.saturating_add(last_claim);
}

Pallet::<T>::deposit_event(Event::<T>::PaidOut {
member: who.clone(),
pool_id: id,
payout: last_claim,
});
});

// this can only be because of rounding down, or because the person we
// wanted to pay their reward to could not accept it (dust).
let leftover = accumulated_reward.saturating_sub(sum_paid_out);
if !leftover.is_zero() {
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
// pay it all to depositor.
let o = T::Currency::transfer(
&reward_account,
&bonded_pool.roles.depositor,
leftover,
ExistenceRequirement::KeepAlive,
);
log!(warn, "paying {:?} leftover to the depositor: {:?}", leftover, o);
}

// finally, migrate the reward pool.
reward_pools_translated += 1;

Some(RewardPool {
last_recorded_reward_counter: Zero::zero(),
last_recorded_total_payouts: Zero::zero(),
total_rewards_claimed: Zero::zero(),
})
},
);

log!(
info,
"Upgraded {} members, {} reward pools, TVL {:?} TPL {:?}, storage to version {:?}",
members_translated,
reward_pools_translated,
total_value_locked,
total_points_locked,
current
);
current.put::<Pallet<T>>();
T::DbWeight::get().reads_writes(members_translated + 1, reward_pools_translated + 1)
}
}

impl<T: Config> OnRuntimeUpgrade for MigrateToV2<T> {
fn on_runtime_upgrade() -> Weight {
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 current == 2 && onchain == 1 {
Self::run(current)
} else {
log!(info, "MigrateToV2 did not executed. This probably should be removed");
T::DbWeight::get().reads(1)
}
}

#[cfg(feature = "try-runtime")]
fn post_upgrade() -> Result<(), &'static str> {
// new version must be set.
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(Pallet::<T>::on_chain_storage_version(), 2);

// all reward pools must have exactly ED in them. This means no reward can be claimed,
// and that setting reward counters all over the board to zero will work henceforth.
RewardPools::<T>::iter().for_each(|(id, _reward_pool)| {
assert_eq!(
RewardPool::<T>::current_balance(id),
Zero::zero(),
"reward pool({}) balance is {:?}",
id,
RewardPool::<T>::current_balance(id)
);
});

log!(info, "post upgrade hook for MigrateToV2 executed.");
Ok(())
}
}
}
Loading