From 8ed1470abccd975ff3a0fe8075804b1ed10d1f5a Mon Sep 17 00:00:00 2001 From: Amar Singh Date: Mon, 30 Aug 2021 11:02:07 -0400 Subject: [PATCH] Fix nominator_bond_less deletes bottom nomination bug (#748) * in progress * update top nominators list upon changes and do not remove any bottom nominations * test fix * init migration to start to fix inconsistent state * more progress on migration * improve nomination increase decrease events * fmt * review and comment all uses of pop * add two different errors for each source of inconsistency and init migration unit test * add unreserve stakers extrinsic gated by root to unreserve the reserved non nominators * in middle of testing migration and found bug wherein nominator can have no nominations after it runs * fix migration, still needs weights * rename root unreserve function and finish migration test * last few changes to public visibility to keep the test without exposing storage write functionality to outside of crate * fix comment * Fix max nominators per collator upgrade bug (#751) * init naive not tested * fix compilation errors, still needs tests * test * fix total staked in hotfix_unreserve_nomination as well * accept review comments * fix weight returned in on runtime upgrade * remove accounts due unreserved balance storage item and use btreemap in memory instead * try fix * no filtering condition on first migration loop * merge migration functions into one function * conservative weight estimate * Update pallets/parachain-staking/src/lib.rs Co-authored-by: girazoki Co-authored-by: girazoki --- pallets/parachain-staking/src/lib.rs | 211 +++++++++-- pallets/parachain-staking/src/set.rs | 2 +- pallets/parachain-staking/src/tests.rs | 393 ++++++++++++++++++++- precompiles/parachain-staking/src/tests.rs | 22 +- 4 files changed, 573 insertions(+), 55 deletions(-) diff --git a/pallets/parachain-staking/src/lib.rs b/pallets/parachain-staking/src/lib.rs index 94f6aa1027..6e3bc85020 100644 --- a/pallets/parachain-staking/src/lib.rs +++ b/pallets/parachain-staking/src/lib.rs @@ -259,6 +259,7 @@ pub mod pallet { new_total: self.total_counted, }) } else { + // >pop requires push to reset in case isn't pushed to bottom let last_nomination_in_top = self .top_nominators .pop() @@ -274,7 +275,7 @@ pub mod pallet { new_total: self.total_counted, }) } else { - // push previously popped last nomination into top_nominators + // >required push to previously popped last nomination into top_nominators self.top_nominators.push(last_nomination_in_top); self.add_bottom_nominator(Bond { owner: acc, amount }); Ok(NominatorAdded::AddedToBottom) @@ -286,7 +287,10 @@ pub mod pallet { &mut self, nominator: A, ) -> Result<(bool, B), DispatchError> { - ensure!(self.nominators.remove(&nominator), Error::::NominatorDNE); + ensure!( + self.nominators.remove(&nominator), + Error::::NominatorDNEInNominatorSet + ); let mut nominator_stake: Option = None; self.top_nominators = self .top_nominators @@ -301,17 +305,20 @@ pub mod pallet { } }) .collect(); + // item removed from the top => highest bottom is popped from bottom and pushed to top if let Some(s) = nominator_stake { // last element has largest amount as per ordering if let Some(last) = self.bottom_nominators.pop() { self.total_counted -= s - last.amount; self.add_top_nominator(last); } else { + // no item in bottom nominators so no item from bottom to pop and push up self.total_counted -= s; } self.total_backing -= s; return Ok((true, s)); } + // else (no item removed from the top) self.bottom_nominators = self .bottom_nominators .clone() @@ -325,7 +332,8 @@ pub mod pallet { } }) .collect(); - let stake = nominator_stake.ok_or(Error::::NominatorDNE)?; + // if err, no item with account exists in top || bottom + let stake = nominator_stake.ok_or(Error::::NominatorDNEinTopNorBottom)?; self.total_backing -= stake; Ok((false, stake)) } @@ -342,10 +350,13 @@ pub mod pallet { break; } } + // if nominator was increased in top nominators if in_top { self.sort_top_nominators(); return true; } + // else nominator to increase must exist in bottom + // >pop requires push later on to reset in case it isn't used let lowest_top = self .top_nominators .pop() @@ -367,7 +378,7 @@ pub mod pallet { self.add_bottom_nominator(lowest_top); true } else { - // reset top_nominators from earlier pop + // >required push to reset top_nominators from earlier pop self.top_nominators.push(lowest_top); self.sort_bottom_nominators(); false @@ -376,15 +387,19 @@ pub mod pallet { /// Return true if in_top after call pub fn dec_nominator(&mut self, nominator: A, less: B) -> bool { let mut in_top = false; - let mut new_top: Option> = None; + let mut new_lowest_top: Option> = None; for x in &mut self.top_nominators { if x.owner == nominator { x.amount -= less; - self.total_counted -= less; - self.total_backing -= less; - if let Some(top_bottom) = self.bottom_nominators.pop() { - if top_bottom.amount > x.amount { - new_top = Some(top_bottom); + // if there is at least 1 nominator in bottom nominators, compare it to check + // if it should be swapped with lowest top nomination and put in top + // >pop requires push later on to reset in case it isn't used + if let Some(highest_bottom) = self.bottom_nominators.pop() { + if highest_bottom.amount > x.amount { + new_lowest_top = Some(highest_bottom); + } else { + // >required push to reset self.bottom_nominators + self.bottom_nominators.push(highest_bottom); } } in_top = true; @@ -393,14 +408,22 @@ pub mod pallet { } if in_top { self.sort_top_nominators(); - if let Some(new) = new_top { - let lowest_top = self.top_nominators.pop().expect("just updated => exists"); - self.total_counted -= lowest_top.amount; - self.total_counted += new.amount; - self.add_top_nominator(new); + if let Some(highest_bottom) = new_lowest_top { + // pop last in top to swap it with top bottom + let lowest_top = self + .top_nominators + .pop() + .expect("must have >1 item to update, assign in_top = true"); + self.total_counted -= lowest_top.amount + less; + self.total_counted += highest_bottom.amount; + self.total_backing -= less; + self.add_top_nominator(highest_bottom); self.add_bottom_nominator(lowest_top); return false; } else { + // no existing bottom nominators so update both counters the same magnitude + self.total_counted -= less; + self.total_backing -= less; return true; } } @@ -443,7 +466,7 @@ pub mod pallet { Leaving(RoundIndex), } - #[derive(Encode, Decode, RuntimeDebug)] + #[derive(Clone, Encode, Decode, RuntimeDebug)] /// Nominator state pub struct Nominator2 { /// All current nominations @@ -745,8 +768,9 @@ pub mod pallet { #[pallet::error] pub enum Error { - // Nominator Does Not Exist NominatorDNE, + NominatorDNEinTopNorBottom, + NominatorDNEInNominatorSet, CandidateDNE, NominationDNE, NominatorExists, @@ -792,17 +816,17 @@ pub mod pallet { CollatorScheduledExit(RoundIndex, T::AccountId, RoundIndex), /// Account, Amount Unlocked, New Total Amt Locked CollatorLeft(T::AccountId, BalanceOf, BalanceOf), - // Nominator, Collator, Old Nomination, Counted in Top, New Nomination - NominationIncreased(T::AccountId, T::AccountId, BalanceOf, bool, BalanceOf), - // Nominator, Collator, Old Nomination, Counted in Top, New Nomination - NominationDecreased(T::AccountId, T::AccountId, BalanceOf, bool, BalanceOf), + // Nominator, Collator, Amount, If in top nominations for collator after increase + NominationIncreased(T::AccountId, T::AccountId, BalanceOf, bool), + // Nominator, Collator, Amount, If in top nominations for collator after decrease + NominationDecreased(T::AccountId, T::AccountId, BalanceOf, bool), /// Round, Nominator, Scheduled Exit NominatorExitScheduled(RoundIndex, T::AccountId, RoundIndex), /// Round, Nominator, Collator, Scheduled Exit NominationRevocationScheduled(RoundIndex, T::AccountId, T::AccountId, RoundIndex), /// Nominator, Amount Unstaked NominatorLeft(T::AccountId, BalanceOf), - /// Nominator, Amount Locked, Collator, Nominator Position with New Total Backing if in Top + /// Nominator, Amount Locked, Collator, Nominator Position with New Total Counted if in Top Nomination( T::AccountId, BalanceOf, @@ -837,10 +861,119 @@ pub mod pallet { Perbill, Perbill, ), + /// Account, Amount Unreserved by Democracy + HotfixUnreservedNomination(T::AccountId, BalanceOf), + } + + pub(crate) fn correct_bond_less_removes_bottom_nomination_inconsistencies( + ) -> (u64, u64) { + let (mut reads, mut writes) = (0u64, 0u64); + let mut map: BTreeMap<(T::AccountId, T::AccountId), ()> = BTreeMap::new(); + let top_n = T::MaxNominatorsPerCollator::get() as usize; + // 1. for collator state, check if there is a nominator not in top or bottom + for (account, state) in >::iter() { + reads += 1u64; + // remove all accounts not in self.top_nominators && self.bottom_nominators + let mut nominator_set = Vec::new(); + for Bond { owner, .. } in &state.top_nominators { + nominator_set.push(owner.clone()); + } + for Bond { owner, .. } in &state.bottom_nominators { + nominator_set.push(owner.clone()); + } + for nominator in &state.nominators.0 { + if !nominator_set.contains(nominator) { + // these accounts were removed without being unreserved so we track it + // with this map which will hold the due amount + map.insert((account.clone(), nominator.clone()), ()); + } + } + // RESET incorrect storage state + let mut all_nominators = state.top_nominators.clone(); + let mut starting_bottom_nominators = state.bottom_nominators.clone(); + all_nominators.append(&mut starting_bottom_nominators); + // sort all nominators from greatest to least + all_nominators.sort_unstable_by(|a, b| b.amount.cmp(&a.amount)); + // 2. split them into top and bottom using the T::MaxNominatorsPerCollator + let top_nominators: Vec>> = + all_nominators.clone().into_iter().take(top_n).collect(); + let bottom_nominators = if all_nominators.len() > top_n { + let rest = all_nominators.len() - top_n; + let bottom: Vec>> = all_nominators + .clone() + .into_iter() + .rev() + .take(rest) + .collect(); + bottom + } else { + // empty, all nominations are in top + Vec::new() + }; + let (mut total_counted, mut total_backing): (BalanceOf, BalanceOf) = + (0u32.into(), 0u32.into()); + for Bond { amount, .. } in &top_nominators { + total_counted += *amount; + total_backing += *amount; + } + for Bond { amount, .. } in &bottom_nominators { + total_backing += *amount; + } + // update candidate pool with new total counted if it changed + if state.total_counted != total_counted && state.is_active() { + reads += 1u64; + writes += 1u64; + >::update_active(account.clone(), total_counted); + } + let new_state = Collator2 { + nominators: OrderedSet::from(nominator_set), + top_nominators, + bottom_nominators, + total_counted, + total_backing, + ..state + }; + >::insert(&account, new_state); + writes += 1u64; + log::warn!("CORRECTED INCONSISTENT COLLATOR STATE FOR {:?}", account); + } + // 2. for nominator state, check if there are nominations that were inadvertently bumped + // -> this allows us to recover the due unreserved balances for cases of (1) that + // did not have the nominator state removed (it does not account for when it was removed) + for (account, mut state) in >::iter() { + reads += 1u64; + for Bond { owner, .. } in state.nominations.0.clone() { + if map.get(&(owner.clone(), account.clone())).is_some() { + if state.rm_nomination(owner).is_some() { + if state.nominations.0.is_empty() { + >::remove(&account); + } else { + >::insert(&account, state.clone()); + } + writes += 1u64; + } + } + } + } + (reads, writes) } #[pallet::hooks] impl Hooks> for Pallet { + fn on_runtime_upgrade() -> Weight { + let weight = T::DbWeight::get(); + if !>::get() { + let (mut reads, mut writes) = + correct_bond_less_removes_bottom_nomination_inconsistencies::(); + reads += 1u64; + writes += 1u64; + >::put(true); + // 50% of the max block weight as safety margin for computation + weight.reads(reads) + weight.writes(writes) + 250_000_000_000 + } else { + weight.reads(1u64) + } + } fn on_initialize(n: T::BlockNumber) -> Weight { let mut round = >::get(); if round.should_update(n) { @@ -872,6 +1005,11 @@ pub mod pallet { } } + #[pallet::storage] + #[pallet::getter(fn fix_bond_less_migration_executed)] + /// Temporary to check if migration has run + pub(crate) type FixBondLessMigrationExecuted = StorageValue<_, bool, ValueQuery>; + #[pallet::storage] #[pallet::getter(fn collator_commission)] /// Commission percent taken off of rewards for all collators @@ -907,7 +1045,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn collator_state2)] /// Get collator state associated with an account if account is collating else None - type CollatorState2 = StorageMap< + pub(crate) type CollatorState2 = StorageMap< _, Twox64Concat, T::AccountId, @@ -1085,6 +1223,27 @@ pub mod pallet { #[pallet::call] impl Pallet { + #[pallet::weight(1_000_000_000)] + /// Temporary root function to return nominations + /// - charges uniform 0.2% of block weight as fee per call + pub fn hotfix_unreserve_nomination( + origin: OriginFor, + stakers: Vec<(T::AccountId, BalanceOf)>, + ) -> DispatchResultWithPostInfo { + frame_system::ensure_root(origin)?; + let mut sum_unreserved: BalanceOf = 0u32.into(); + for (due_account, due_unreserve) in stakers { + T::Currency::unreserve(&due_account, due_unreserve); + Self::deposit_event(Event::HotfixUnreservedNomination( + due_account, + due_unreserve, + )); + sum_unreserved += due_unreserve; + } + let new_total = >::get().saturating_sub(sum_unreserved); + >::put(new_total); + Ok(().into()) + } /// Set the expectations for total staked. These expectations determine the issuance for /// the round according to logic in `fn compute_issuance` #[pallet::weight(::WeightInfo::set_staking_expectations())] @@ -1573,7 +1732,7 @@ pub mod pallet { let new_total_staked = >::get().saturating_add(more); >::put(new_total_staked); Self::deposit_event(Event::NominationIncreased( - nominator, candidate, before, in_top, after, + nominator, candidate, more, in_top, )); Ok(().into()) } @@ -1618,7 +1777,7 @@ pub mod pallet { let new_total_staked = >::get().saturating_sub(less); >::put(new_total_staked); Self::deposit_event(Event::NominationDecreased( - nominator, candidate, before, in_top, after, + nominator, candidate, less, in_top, )); Ok(().into()) } @@ -1657,7 +1816,7 @@ pub mod pallet { round_issuance.ideal } } - fn nominator_leaves_collator( + pub(crate) fn nominator_leaves_collator( nominator: T::AccountId, collator: T::AccountId, ) -> DispatchResultWithPostInfo { diff --git a/pallets/parachain-staking/src/set.rs b/pallets/parachain-staking/src/set.rs index 5ea3dcf614..3b8ae0cbdb 100644 --- a/pallets/parachain-staking/src/set.rs +++ b/pallets/parachain-staking/src/set.rs @@ -23,7 +23,7 @@ use sp_std::prelude::*; /// An ordered set backed by `Vec` #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] -#[derive(RuntimeDebug, PartialEq, Eq, Encode, Decode, Default)] +#[derive(RuntimeDebug, PartialEq, Eq, Encode, Decode, Default, Clone)] pub struct OrderedSet(pub Vec); impl OrderedSet { diff --git a/pallets/parachain-staking/src/tests.rs b/pallets/parachain-staking/src/tests.rs index 97b8842329..807a2ff8b5 100644 --- a/pallets/parachain-staking/src/tests.rs +++ b/pallets/parachain-staking/src/tests.rs @@ -25,7 +25,7 @@ use crate::mock::{ events, last_event, roll_to, set_author, Balances, Event as MetaEvent, ExtBuilder, Origin, Stake, Test, }; -use crate::{Bond, CollatorStatus, Error, Event, NominatorAdded, Range}; +use crate::{Bond, CollatorState2, CollatorStatus, Error, Event, NominatorAdded, Range}; use frame_support::{assert_noop, assert_ok}; use sp_runtime::{traits::Zero, DispatchError, Perbill, Percent}; @@ -1930,6 +1930,102 @@ fn cannot_revoke_nomination_leaving_nominator_below_min_nominator_stake() { }); } +#[test] +fn revoke_nomination_after_leave_candidates_executes_during_leave_candidates() { + ExtBuilder::default() + .with_balances(vec![(1, 30), (2, 10)]) + .with_candidates(vec![(1, 30)]) + .with_nominations(vec![(2, 1, 10)]) + .build() + .execute_with(|| { + roll_to(1); + assert_ok!(Stake::leave_candidates(Origin::signed(1), 1)); + assert_ok!(Stake::revoke_nomination(Origin::signed(2), 1)); + assert_eq!( + last_event(), + MetaEvent::Stake(Event::NominatorExitScheduled(1, 2, 3)) + ); + roll_to(10); + assert!(!Stake::is_nominator(&2)); + assert_eq!(Balances::reserved_balance(&2), 0); + assert_eq!(Balances::free_balance(&2), 10); + }); +} + +#[test] +fn revoke_nomination_before_leave_candidates_executes_during_leave_candidates() { + ExtBuilder::default() + .with_balances(vec![(1, 30), (2, 10)]) + .with_candidates(vec![(1, 30)]) + .with_nominations(vec![(2, 1, 10)]) + .build() + .execute_with(|| { + roll_to(1); + assert_ok!(Stake::revoke_nomination(Origin::signed(2), 1)); + assert_eq!( + last_event(), + MetaEvent::Stake(Event::NominatorExitScheduled(1, 2, 3)) + ); + assert_ok!(Stake::leave_candidates(Origin::signed(1), 1)); + roll_to(10); + assert!(!Stake::is_nominator(&2)); + assert_eq!(Balances::reserved_balance(&2), 0); + assert_eq!(Balances::free_balance(&2), 10); + }); +} + +#[test] +fn nominator_bond_more_after_revoke_nomination_does_not_effect_exit() { + ExtBuilder::default() + .with_balances(vec![(1, 30), (2, 30), (3, 30)]) + .with_candidates(vec![(1, 30), (3, 30)]) + .with_nominations(vec![(2, 1, 10), (2, 3, 10)]) + .build() + .execute_with(|| { + roll_to(1); + assert_ok!(Stake::revoke_nomination(Origin::signed(2), 1)); + assert_eq!( + last_event(), + MetaEvent::Stake(Event::NominationRevocationScheduled(1, 2, 1, 3)) + ); + assert_noop!( + Stake::nominator_bond_more(Origin::signed(2), 1, 10), + Error::::CannotActBecauseRevoking + ); + assert_ok!(Stake::nominator_bond_more(Origin::signed(2), 3, 10)); + roll_to(10); + assert!(Stake::is_nominator(&2)); + assert_eq!(Balances::reserved_balance(&2), 20); + assert_eq!(Balances::free_balance(&2), 10); + }); +} + +#[test] +fn nominator_bond_less_after_revoke_nomination_does_not_effect_exit() { + ExtBuilder::default() + .with_balances(vec![(1, 30), (2, 30), (3, 30)]) + .with_candidates(vec![(1, 30), (3, 30)]) + .with_nominations(vec![(2, 1, 10), (2, 3, 10)]) + .build() + .execute_with(|| { + roll_to(1); + assert_ok!(Stake::revoke_nomination(Origin::signed(2), 1)); + assert_eq!( + last_event(), + MetaEvent::Stake(Event::NominationRevocationScheduled(1, 2, 1, 3)) + ); + assert_noop!( + Stake::nominator_bond_less(Origin::signed(2), 1, 2), + Error::::CannotActBecauseRevoking + ); + assert_ok!(Stake::nominator_bond_less(Origin::signed(2), 3, 2)); + roll_to(10); + assert!(Stake::is_nominator(&2)); + assert_eq!(Balances::reserved_balance(&2), 8); + assert_eq!(Balances::free_balance(&2), 22); + }); +} + // NOMINATOR BOND MORE #[test] @@ -1943,7 +2039,7 @@ fn nominator_bond_more_event_emits_correctly() { assert_ok!(Stake::nominator_bond_more(Origin::signed(2), 1, 5)); assert_eq!( last_event(), - MetaEvent::Stake(Event::NominationIncreased(2, 1, 40, true, 45)) + MetaEvent::Stake(Event::NominationIncreased(2, 1, 5, true)) ); }); } @@ -1993,7 +2089,7 @@ fn nominator_bond_more_updates_nominator_state() { } #[test] -fn nominator_bond_more_updates_candidate_state() { +fn nominator_bond_more_updates_candidate_state_top_nominators() { ExtBuilder::default() .with_balances(vec![(1, 30), (2, 15)]) .with_candidates(vec![(1, 30)]) @@ -2018,6 +2114,42 @@ fn nominator_bond_more_updates_candidate_state() { }); } +#[test] +fn nominator_bond_more_updates_candidate_state_bottom_nominators() { + ExtBuilder::default() + .with_balances(vec![(1, 30), (2, 20), (3, 20), (4, 20), (5, 20), (6, 20)]) + .with_candidates(vec![(1, 30)]) + .with_nominations(vec![ + (2, 1, 10), + (3, 1, 20), + (4, 1, 20), + (5, 1, 20), + (6, 1, 20), + ]) + .build() + .execute_with(|| { + assert_eq!( + Stake::collator_state2(1).expect("exists").bottom_nominators[0], + Bond { + owner: 2, + amount: 10 + } + ); + assert_ok!(Stake::nominator_bond_more(Origin::signed(2), 1, 5)); + assert_eq!( + last_event(), + MetaEvent::Stake(Event::NominationIncreased(2, 1, 5, false)) + ); + assert_eq!( + Stake::collator_state2(1).expect("exists").bottom_nominators[0], + Bond { + owner: 2, + amount: 15 + } + ); + }); +} + #[test] fn nominator_bond_more_increases_total() { ExtBuilder::default() @@ -2136,7 +2268,7 @@ fn nominator_bond_less_event_emits_correctly() { assert_ok!(Stake::nominator_bond_less(Origin::signed(2), 1, 5)); assert_eq!( last_event(), - MetaEvent::Stake(Event::NominationDecreased(2, 1, 40, true, 35)) + MetaEvent::Stake(Event::NominationDecreased(2, 1, 5, true)) ); }); } @@ -2342,6 +2474,130 @@ fn cannot_nominator_bond_less_below_min_nomination() { }); } +#[test] +fn nominator_bond_less_updates_just_bottom_nominations() { + ExtBuilder::default() + .with_balances(vec![(1, 20), (2, 10), (3, 11), (4, 12), (5, 14), (6, 15)]) + .with_candidates(vec![(1, 20)]) + .with_nominations(vec![ + (2, 1, 10), + (3, 1, 11), + (4, 1, 12), + (5, 1, 14), + (6, 1, 15), + ]) + .build() + .execute_with(|| { + let pre_call_collator_state = + Stake::collator_state2(&1).expect("nominated by all so exists"); + assert_ok!(Stake::nominator_bond_less(Origin::signed(2), 1, 2)); + let post_call_collator_state = + Stake::collator_state2(&1).expect("nominated by all so exists"); + let mut not_equal = false; + for Bond { owner, amount } in pre_call_collator_state.bottom_nominators { + for Bond { + owner: post_owner, + amount: post_amount, + } in &post_call_collator_state.bottom_nominators + { + if &owner == post_owner { + if &amount != post_amount { + not_equal = true; + break; + } + } + } + } + assert!(not_equal); + let mut equal = true; + for Bond { owner, amount } in pre_call_collator_state.top_nominators { + for Bond { + owner: post_owner, + amount: post_amount, + } in &post_call_collator_state.top_nominators + { + if &owner == post_owner { + if &amount != post_amount { + equal = false; + break; + } + } + } + } + assert!(equal); + assert_eq!( + pre_call_collator_state.total_backing - 2, + post_call_collator_state.total_backing + ); + assert_eq!( + pre_call_collator_state.total_counted, + post_call_collator_state.total_counted + ); + }); +} + +#[test] +fn nominator_bond_less_does_not_delete_bottom_nominations() { + ExtBuilder::default() + .with_balances(vec![(1, 20), (2, 10), (3, 11), (4, 12), (5, 14), (6, 15)]) + .with_candidates(vec![(1, 20)]) + .with_nominations(vec![ + (2, 1, 10), + (3, 1, 11), + (4, 1, 12), + (5, 1, 14), + (6, 1, 15), + ]) + .build() + .execute_with(|| { + let pre_call_collator_state = + Stake::collator_state2(&1).expect("nominated by all so exists"); + assert_ok!(Stake::nominator_bond_less(Origin::signed(6), 1, 4)); + let post_call_collator_state = + Stake::collator_state2(&1).expect("nominated by all so exists"); + let mut equal = true; + for Bond { owner, amount } in pre_call_collator_state.bottom_nominators { + for Bond { + owner: post_owner, + amount: post_amount, + } in &post_call_collator_state.bottom_nominators + { + if &owner == post_owner { + if &amount != post_amount { + equal = false; + break; + } + } + } + } + assert!(equal); + let mut not_equal = false; + for Bond { owner, amount } in pre_call_collator_state.top_nominators { + for Bond { + owner: post_owner, + amount: post_amount, + } in &post_call_collator_state.top_nominators + { + if &owner == post_owner { + if &amount != post_amount { + not_equal = true; + break; + } + } + } + } + assert!(not_equal); + assert_eq!( + pre_call_collator_state.total_backing - 4, + post_call_collator_state.total_backing + ); + assert_eq!( + pre_call_collator_state.total_counted - 4, + post_call_collator_state.total_counted + ); + }); +} + // ~~ PROPERTY-BASED TESTS ~~ #[test] @@ -3423,7 +3679,7 @@ fn only_top_collators_are_counted() { ); // bump bottom to the top assert_ok!(Stake::nominator_bond_more(Origin::signed(3), 1, 8)); - expected_events.push(Event::NominationIncreased(3, 1, 86, true, 90)); + expected_events.push(Event::NominationIncreased(3, 1, 8, true)); assert_eq!(events(), expected_events); let collator_state = Stake::collator_state2(1).unwrap(); // 16 + 17 + 18 + 19 + 20 = 90 (top 4 + self bond) @@ -3435,7 +3691,7 @@ fn only_top_collators_are_counted() { ); // bump bottom to the top assert_ok!(Stake::nominator_bond_more(Origin::signed(4), 1, 8)); - expected_events.push(Event::NominationIncreased(4, 1, 90, true, 94)); + expected_events.push(Event::NominationIncreased(4, 1, 8, true)); assert_eq!(events(), expected_events); let collator_state = Stake::collator_state2(1).unwrap(); // 17 + 18 + 19 + 20 + 20 = 94 (top 4 + self bond) @@ -3447,7 +3703,7 @@ fn only_top_collators_are_counted() { ); // bump bottom to the top assert_ok!(Stake::nominator_bond_more(Origin::signed(5), 1, 8)); - expected_events.push(Event::NominationIncreased(5, 1, 94, true, 98)); + expected_events.push(Event::NominationIncreased(5, 1, 8, true)); assert_eq!(events(), expected_events); let collator_state = Stake::collator_state2(1).unwrap(); // 18 + 19 + 20 + 21 + 20 = 98 (top 4 + self bond) @@ -3459,7 +3715,7 @@ fn only_top_collators_are_counted() { ); // bump bottom to the top assert_ok!(Stake::nominator_bond_more(Origin::signed(6), 1, 8)); - expected_events.push(Event::NominationIncreased(6, 1, 98, true, 102)); + expected_events.push(Event::NominationIncreased(6, 1, 8, true)); assert_eq!(events(), expected_events); let collator_state = Stake::collator_state2(1).unwrap(); // 19 + 20 + 21 + 22 + 20 = 102 (top 4 + self bond) @@ -3527,7 +3783,7 @@ fn nomination_events_convey_correct_position() { ); // 8 increases nomination to the top assert_ok!(Stake::nominator_bond_more(Origin::signed(8), 1, 3)); - expected_events.push(Event::NominationIncreased(8, 1, 74, true, 75)); + expected_events.push(Event::NominationIncreased(8, 1, 3, true)); assert_eq!(events(), expected_events); let collator1_state = Stake::collator_state2(1).unwrap(); // 13 + 13 + 14 + 15 + 20 = 75 (top 4 + self bond) @@ -3539,7 +3795,7 @@ fn nomination_events_convey_correct_position() { ); // 3 increases nomination but stays in bottom assert_ok!(Stake::nominator_bond_more(Origin::signed(3), 1, 1)); - expected_events.push(Event::NominationIncreased(3, 1, 75, false, 75)); + expected_events.push(Event::NominationIncreased(3, 1, 1, false)); assert_eq!(events(), expected_events); let collator1_state = Stake::collator_state2(1).unwrap(); // 13 + 13 + 14 + 15 + 20 = 75 (top 4 + self bond) @@ -3551,7 +3807,7 @@ fn nomination_events_convey_correct_position() { ); // 6 decreases nomination but stays in top assert_ok!(Stake::nominator_bond_less(Origin::signed(6), 1, 2)); - expected_events.push(Event::NominationDecreased(6, 1, 75, true, 73)); + expected_events.push(Event::NominationDecreased(6, 1, 2, true)); assert_eq!(events(), expected_events); let collator1_state = Stake::collator_state2(1).unwrap(); // 12 + 13 + 13 + 15 + 20 = 73 (top 4 + self bond) @@ -3563,7 +3819,7 @@ fn nomination_events_convey_correct_position() { ); // 6 decreases nomination and is bumped to bottom assert_ok!(Stake::nominator_bond_less(Origin::signed(6), 1, 1)); - expected_events.push(Event::NominationDecreased(6, 1, 73, false, 73)); + expected_events.push(Event::NominationDecreased(6, 1, 1, false)); assert_eq!(events(), expected_events); let collator1_state = Stake::collator_state2(1).unwrap(); // 12 + 13 + 13 + 15 + 20 = 73 (top 4 + self bond) @@ -3575,3 +3831,116 @@ fn nomination_events_convey_correct_position() { ); }); } + +#[test] +fn migration_corrects_storage_corrupted_by_bond_less_bug() { + ExtBuilder::default() + .with_balances(vec![ + (1, 100), + (2, 100), + (3, 100), + (4, 100), + (5, 100), + (6, 100), + ]) + .with_candidates(vec![(1, 20)]) + .with_nominations(vec![ + (2, 1, 19), + (3, 1, 20), + (4, 1, 21), + (5, 1, 22), + (6, 1, 23), + ]) + .build() + .execute_with(|| { + // start by corrupting collator state like the bug -- basically every `nominator_bond_less` + // call would bump the highest nomination in the bottom nominations without replacing it, + // thereby preventing proper exit for these nominators (unreserve) + let mut candidate_state = + >::get(&1).expect("set up 1 as candidate"); + candidate_state.bottom_nominators = Vec::new(); + // corrupt storage like the bug instance + >::insert(1, candidate_state); + // function called in executed nomination/nominator exits + assert_noop!( + Stake::nominator_leaves_collator(2, 1), + Error::::NominatorDNEinTopNorBottom + ); + assert_eq!(Stake::total(), 125); + let candidate_state = >::get(&1).expect("still exists"); + // was removed from bottom nominators and not the `nominators` set + assert_eq!( + candidate_state.nominators.0.len() - 1, + candidate_state.top_nominators.len() + candidate_state.bottom_nominators.len() + ); + // nominator state still has the nomination if the collator hasn't left and the nominator + // didn't exit entirely (this wasn't their last nomination) + assert!(Stake::is_nominator(&2)); + // make storage consistent at least + crate::correct_bond_less_removes_bottom_nomination_inconsistencies::(); + // check storage is consistent + let candidate_state = >::get(&1).expect("still exists"); + assert_eq!( + candidate_state.nominators.0.len(), + candidate_state.top_nominators.len() + candidate_state.bottom_nominators.len() + ); + // no longer a nominator + assert!(!Stake::is_nominator(&2)); + // check that the balance is not unreserved + assert_eq!(Balances::reserved_balance(&2), 19); + assert_eq!(Balances::free_balance(&2), 81); + assert_eq!(Stake::total(), 125); + // return due unreserved balance (NOTE: doesn't reset this storage item) + assert_ok!(Stake::hotfix_unreserve_nomination( + Origin::root(), + vec![(2, 19)] + )); + assert_eq!(Balances::reserved_balance(&2), 0); + assert_eq!(Balances::free_balance(&2), 100); + assert_eq!(Stake::total(), 106); + }); +} + +#[test] +fn migration_corrects_storage_corrupted_by_max_nominators_upgrade_bug() { + ExtBuilder::default() + .with_balances(vec![ + (1, 100), + (2, 100), + (3, 100), + (4, 100), + (5, 100), + (6, 100), + (7, 100), + (8, 100), + ]) + .with_candidates(vec![(1, 20)]) + .with_nominations(vec![ + (2, 1, 19), + (3, 1, 20), + (4, 1, 21), + (5, 1, 22), + (6, 1, 23), + (7, 1, 24), + (8, 1, 25), + ]) + .build() + .execute_with(|| { + // start by corrupting collator state like the bug -- have some in bottom with open + // slots in the top + let mut candidate_state = + >::get(&1).expect("set up 1 as candidate"); + // corrupt storage via unhandled pop + candidate_state.top_nominators.pop(); + candidate_state.top_nominators.pop(); + assert_eq!(candidate_state.top_nominators.len(), 2); // < MaxNominatorsPerCollator = 4 + assert_eq!(candidate_state.bottom_nominators.len(), 3); + >::insert(&1, candidate_state); + // full migration, first cleans nominator set and second cleans other items + crate::correct_bond_less_removes_bottom_nomination_inconsistencies::(); + let post_candidate_state = + >::get(&1).expect("set up 1 as candidate"); + assert_eq!(post_candidate_state.top_nominators.len(), 4); + assert_eq!(post_candidate_state.bottom_nominators.len(), 1); + }); +} diff --git a/precompiles/parachain-staking/src/tests.rs b/precompiles/parachain-staking/src/tests.rs index cad88c8d1c..f67dcea8c9 100644 --- a/precompiles/parachain-staking/src/tests.rs +++ b/precompiles/parachain-staking/src/tests.rs @@ -749,14 +749,9 @@ fn nominator_bond_more_works() { .dispatch(Origin::root())); // Check for the right events. - let expected_event: crate::mock::Event = StakingEvent::NominationIncreased( - TestAccount::Bob, - TestAccount::Alice, - 1_500, - true, - 2_000, - ) - .into(); + let expected_event: crate::mock::Event = + StakingEvent::NominationIncreased(TestAccount::Bob, TestAccount::Alice, 500, true) + .into(); assert!(events().contains(&expected_event)); }); @@ -790,14 +785,9 @@ fn nominator_bond_less_works() { .dispatch(Origin::root())); // Check for the right events. - let expected_event: crate::mock::Event = StakingEvent::NominationDecreased( - TestAccount::Bob, - TestAccount::Alice, - 2_500, - true, - 2_000, - ) - .into(); + let expected_event: crate::mock::Event = + StakingEvent::NominationDecreased(TestAccount::Bob, TestAccount::Alice, 500, true) + .into(); assert!(events().contains(&expected_event)); });