Fix issue 16

src/benchmarking.rs

@@ -543,7 +543,8 @@ mod benchmarks {
 		// Here we add the staker as candidate and immediately remove it so that the candidacy bond
 		// gets released and the corresponding weight accounted for.
 		CollatorStaking::<T>::do_register_as_candidate(&caller, bond).unwrap();
-		CollatorStaking::<T>::try_remove_candidate(&caller, true).unwrap();
+		CollatorStaking::<T>::try_remove_candidate(&caller, true, CandidacyBondReleaseReason::Idle)
+			.unwrap();
 
 		CollatorStaking::<T>::lock(
 			RawOrigin::Signed(caller.clone()).into(),

src/lib.rs

@@ -95,6 +95,7 @@ pub mod pallet {
 		BlockNumberFor<T>,
 	>;
 	pub type CandidateStakeInfoOf<T> = CandidateStakeInfo<BalanceOf<T>>;
+	pub type CandidacyBondReleaseOf<T> = CandidacyBondRelease<BalanceOf<T>, BlockNumberFor<T>>;
 
 	/// A convertor from collators id. Since this pallet does not have stash/controller, this is
 	/// just identity.
@@ -305,6 +306,28 @@ pub mod pallet {
 		pub candidates: AccountIdMap,
 	}
 
+	/// Reasons a candidady left the candidacy list for.
+	#[derive(
+		PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, scale_info::TypeInfo, MaxEncodedLen,
+	)]
+	pub enum CandidacyBondReleaseReason {
+		/// The candidacy did not produce at least one block for [`KickThreshold`] blocks.
+		Idle,
+		/// The candidate left by itself.
+		Left,
+		/// The candidate was replaced by another one with higher bond.
+		Replaced,
+	}
+
+	#[derive(
+		PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, scale_info::TypeInfo, MaxEncodedLen,
+	)]
+	pub struct CandidacyBondRelease<Balance, BlockNumber> {
+		pub bond: Balance,
+		pub block: BlockNumber,
+		pub reason: CandidacyBondReleaseReason,
+	}
+
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
 	pub struct Pallet<T>(_);
@@ -420,13 +443,8 @@ pub mod pallet {
 	/// If a candidate leaves the candidacy before its bond is released, the waiting period
 	/// will restart.
 	#[pallet::storage]
-	pub type CandidacyBondReleases<T: Config> = StorageMap<
-		_,
-		Blake2_128Concat,
-		T::AccountId,
-		(BalanceOf<T>, BlockNumberFor<T>),
-		OptionQuery,
-	>;
+	pub type CandidacyBondReleases<T: Config> =
+		StorageMap<_, Blake2_128Concat, T::AccountId, CandidacyBondReleaseOf<T>, OptionQuery>;
 
 	#[pallet::genesis_config]
 	#[derive(DefaultNoBound)]
@@ -774,7 +792,7 @@ pub mod pallet {
 				Error::<T>::TooFewEligibleCollators
 			);
 			// Do remove their last authored block.
-			Self::try_remove_candidate(&who, true)?;
+			Self::try_remove_candidate(&who, true, CandidacyBondReleaseReason::Left)?;
 
 			Ok(())
 		}
@@ -1326,6 +1344,31 @@ pub mod pallet {
 						stake.saturating_accrue(info.stake);
 						stakers.saturating_inc();
 					}
+					// Users are allowed to reuse the old candidacy bond as long as they were
+					// replaced by another candidate.
+					CandidacyBondReleases::<T>::try_mutate(
+						who,
+						|maybe_bond_release| -> DispatchResult {
+							if let Some(bond_release) = maybe_bond_release {
+								if bond_release.reason == CandidacyBondReleaseReason::Replaced
+									&& bond_release.bond >= bond
+								{
+									let remaining_lock =
+										Self::get_releasing_balance(who).saturating_sub(bond);
+									T::Currency::set_freeze(
+										&FreezeReason::Releasing.into(),
+										who,
+										remaining_lock,
+									)?;
+									bond_release.bond.saturating_reduce(bond);
+									if bond_release.bond.is_zero() {
+										*maybe_bond_release = None;
+									}
+								}
+							}
+							Ok(())
+						},
+					)?;
 					let info = CandidateInfo { stake, stakers };
 					*maybe_candidate_info = Some(info.clone());
 
@@ -1571,6 +1614,7 @@ pub mod pallet {
 		pub fn try_remove_candidate(
 			who: &T::AccountId,
 			remove_last_authored: bool,
+			reason: CandidacyBondReleaseReason,
 		) -> Result<CandidateInfoOf<T>, DispatchError> {
 			Candidates::<T>::try_mutate_exists(
 				who,
@@ -1579,7 +1623,7 @@ pub mod pallet {
 					if remove_last_authored {
 						LastAuthoredBlock::<T>::remove(who.clone())
 					}
-					Self::release_candidacy_bond(who)?;
+					Self::release_candidacy_bond(who, reason)?;
 
 					// Store removed candidate in SessionRemovedCandidates to properly reward
 					// the candidate and its stakers at the end of the session.
@@ -1620,7 +1664,10 @@ pub mod pallet {
 		}
 
 		/// Prepares the candidacy bond to be released.
-		fn release_candidacy_bond(account: &T::AccountId) -> DispatchResult {
+		fn release_candidacy_bond(
+			account: &T::AccountId,
+			reason: CandidacyBondReleaseReason,
+		) -> DispatchResult {
 			let bond = Self::get_bond(account);
 			if !bond.is_zero() {
 				// We firstly release the candidacy bond.
@@ -1644,7 +1691,11 @@ pub mod pallet {
 					Self::current_block_number().saturating_add(T::BondUnlockDelay::get());
 				CandidacyBondReleases::<T>::mutate(account, |maybe_bond_release| {
 					let mut final_amount = bond;
-					if let Some((previous_bond_amount, previous_bond_deadline)) = maybe_bond_release
+					if let Some(CandidacyBondRelease {
+						bond: previous_bond_amount,
+						block: previous_bond_deadline,
+						..
+					}) = maybe_bond_release
 					{
 						// Since we are on it, we auto-claim the previous candidacy bond and only
 						// add this one. But if the previous candidacy bond's delay was not
@@ -1653,7 +1704,11 @@ pub mod pallet {
 							final_amount.saturating_accrue(*previous_bond_amount);
 						}
 					}
-					*maybe_bond_release = Some((final_amount, release_block));
+					*maybe_bond_release = Some(CandidacyBondRelease {
+						bond: final_amount,
+						block: release_block,
+						reason,
+					});
 				});
 			}
 			Ok(())
@@ -1662,7 +1717,9 @@ pub mod pallet {
 		/// Claims the candidacy bond, provided sufficient time has passed.
 		fn do_claim_candidacy_bond(account: &T::AccountId) -> DispatchResult {
 			CandidacyBondReleases::<T>::try_mutate(account, |maybe_bond_release| {
-				if let Some((bond, bond_release)) = maybe_bond_release {
+				if let Some(CandidacyBondRelease { bond, block: bond_release, .. }) =
+					maybe_bond_release
+				{
 					if *bond_release > Self::current_block_number() {
 						let new_release =
 							Self::get_releasing_balance(account).saturating_sub(*bond);
@@ -1887,7 +1944,7 @@ pub mod pallet {
                         Some(info)
                     } else {
                         // This collator has not produced a block recently enough. Bye bye.
-                        match Self::try_remove_candidate(&who, true) {
+                        match Self::try_remove_candidate(&who, true, CandidacyBondReleaseReason::Idle) {
 							Ok(_) => None,
 							Err(error) => {
 								log::warn!("Could not remove candidate {:?}: {:?}", info, error);
@@ -1917,8 +1974,7 @@ pub mod pallet {
 		) -> Result<T::AccountId, DispatchError> {
 			let (candidate, worst_bond) = Self::get_worst_candidate()?;
 			ensure!(bond > worst_bond, Error::<T>::InvalidCandidacyBond);
-			Self::try_remove_candidate(&candidate, false)?;
-
+			Self::try_remove_candidate(&candidate, false, CandidacyBondReleaseReason::Replaced)?;
 			Ok(candidate)
 		}
 

src/tests.rs

@@ -15,13 +15,13 @@
 
 use crate as collator_staking;
 use crate::{
-	mock::*, AutoCompound, BalanceOf, CandidacyBondReleases, CandidateInfo, CandidateStakeInfo,
-	Candidates, ClaimableRewards, CollatorRewardPercentage, Config, CurrentSession,
-	DesiredCandidates, Error, Event, ExtraReward, FreezeReason, Invulnerables, LastAuthoredBlock,
-	MinCandidacyBond, MinStake, PerSessionRewards, ProducedBlocks, ReleaseQueues,
-	SessionRemovedCandidates, StakeTarget, TotalBlocks, UserStake, UserStakeInfo,
+	mock::*, AutoCompound, BalanceOf, CandidacyBondRelease, CandidacyBondReleaseReason,
+	CandidacyBondReleases, CandidateInfo, CandidateStake, CandidateStakeInfo, Candidates,
+	ClaimableRewards, CollatorRewardPercentage, Config, CurrentSession, DesiredCandidates, Error,
+	Event, ExtraReward, FreezeReason, Invulnerables, LastAuthoredBlock, MinCandidacyBond, MinStake,
+	PerSessionRewards, ProducedBlocks, ReleaseQueues, ReleaseRequest, SessionRemovedCandidates,
+	StakeTarget, TotalBlocks, UserStake, UserStakeInfo,
 };
-use crate::{CandidateStake, ReleaseRequest};
 use frame_support::pallet_prelude::TypedGet;
 use frame_support::traits::fungible::InspectFreeze;
 use frame_support::traits::tokens::Preservation::Preserve;
@@ -847,6 +847,95 @@ mod register_as_candidate {
 			assert_eq!(Candidates::<Test>::get(3), Some(CandidateInfo { stake: 60, stakers: 1 }));
 		});
 	}
+
+	#[test]
+	fn register_as_candidate_reuses_old_bond_if_replaced() {
+		new_test_ext().execute_with(|| {
+			initialize_to_block(1);
+
+			// given
+			assert_eq!(DesiredCandidates::<Test>::get(), 2);
+			assert_eq!(MinCandidacyBond::<Test>::get(), 10);
+			assert_eq!(Candidates::<Test>::count(), 0);
+			assert_eq!(Invulnerables::<Test>::get(), vec![1, 2]);
+
+			// register the first time
+			assert_eq!(Balances::balance(&3), 100);
+			assert_eq!(
+				CandidateStake::<Test>::get(3, 3),
+				CandidateStakeInfo { stake: 0, session: 0 }
+			);
+			register_candidates(3..=3);
+			assert_eq!(Balances::balance_frozen(&FreezeReason::CandidacyBond.into(), &3), 10);
+			assert_eq!(
+				CandidateStake::<Test>::get(3, 3),
+				CandidateStakeInfo { stake: 0, session: 0 }
+			);
+			assert_eq!(Candidates::<Test>::count(), 1);
+			assert_eq!(Candidates::<Test>::get(3), Some(CandidateInfo { stake: 0, stakers: 0 }));
+			assert_eq!(CollatorStaking::get_bond(&3), 10);
+
+			// the candidate is replaced (artificially)
+			assert_ok!(CollatorStaking::leave_intent(RuntimeOrigin::signed(3)));
+			CandidacyBondReleases::<Test>::mutate(3, |maybe_bond_release| {
+				let bond_release = maybe_bond_release.as_mut().unwrap();
+				bond_release.reason = CandidacyBondReleaseReason::Replaced;
+			});
+			assert_eq!(CollatorStaking::get_releasing_balance(&3), 10);
+			assert_eq!(CollatorStaking::get_bond(&3), 0);
+
+			// and finally rejoins using the old candidacy bond
+			assert_ok!(CollatorStaking::register_as_candidate(
+				RuntimeOrigin::signed(3),
+				MinCandidacyBond::<Test>::get()
+			));
+			assert_eq!(CollatorStaking::get_releasing_balance(&3), 0);
+			assert_eq!(CollatorStaking::get_bond(&3), 10);
+		});
+	}
+
+	#[test]
+	fn register_as_candidate_does_not_reuse_old_bond_if_wrong_reason() {
+		new_test_ext().execute_with(|| {
+			initialize_to_block(1);
+
+			// given
+			assert_eq!(DesiredCandidates::<Test>::get(), 2);
+			assert_eq!(MinCandidacyBond::<Test>::get(), 10);
+			assert_eq!(Candidates::<Test>::count(), 0);
+			assert_eq!(Invulnerables::<Test>::get(), vec![1, 2]);
+
+			// register the first time
+			assert_eq!(Balances::balance(&3), 100);
+			assert_eq!(
+				CandidateStake::<Test>::get(3, 3),
+				CandidateStakeInfo { stake: 0, session: 0 }
+			);
+			register_candidates(3..=3);
+			assert_eq!(Balances::balance_frozen(&FreezeReason::CandidacyBond.into(), &3), 10);
+			assert_eq!(
+				CandidateStake::<Test>::get(3, 3),
+				CandidateStakeInfo { stake: 0, session: 0 }
+			);
+			assert_eq!(Candidates::<Test>::count(), 1);
+			assert_eq!(Candidates::<Test>::get(3), Some(CandidateInfo { stake: 0, stakers: 0 }));
+			assert_eq!(CollatorStaking::get_bond(&3), 10);
+
+			// the candidate removes itself
+			assert_ok!(CollatorStaking::leave_intent(RuntimeOrigin::signed(3)));
+			assert_eq!(CollatorStaking::get_releasing_balance(&3), 10);
+			assert_eq!(CollatorStaking::get_bond(&3), 0);
+
+			// and finally rejoins using the old candidacy bond
+			assert_ok!(CollatorStaking::register_as_candidate(
+				RuntimeOrigin::signed(3),
+				MinCandidacyBond::<Test>::get()
+			));
+			// the old locked candidacy bond should remain
+			assert_eq!(CollatorStaking::get_releasing_balance(&3), 10);
+			assert_eq!(CollatorStaking::get_bond(&3), 10);
+		});
+	}
 }
 
 mod leave_intent {
@@ -924,7 +1013,14 @@ mod leave_intent {
 
 			assert_eq!(CandidacyBondReleases::<Test>::get(3), None);
 			assert_ok!(CollatorStaking::leave_intent(RuntimeOrigin::signed(3)));
-			assert_eq!(CandidacyBondReleases::<Test>::get(3), Some((10, 6)));
+			assert_eq!(
+				CandidacyBondReleases::<Test>::get(3),
+				Some(CandidacyBondRelease {
+					bond: 10,
+					block: 6,
+					reason: CandidacyBondReleaseReason::Left
+				})
+			);
 
 			assert_eq!(Balances::balance_frozen(&FreezeReason::CandidacyBond.into(), &3), 0);
 			assert_eq!(Balances::balance_frozen(&FreezeReason::Releasing.into(), &3), 10);
@@ -3393,7 +3489,14 @@ mod session_management {
 			// kicked collator gets funds back after a delay
 			assert_eq!(Balances::balance_frozen(&FreezeReason::CandidacyBond.into(), &3), 0);
 			assert_eq!(Balances::balance_frozen(&FreezeReason::Releasing.into(), &3), 10);
-			assert_eq!(CandidacyBondReleases::<Test>::get(3), Some((10, 25)));
+			assert_eq!(
+				CandidacyBondReleases::<Test>::get(3),
+				Some(CandidacyBondRelease {
+					bond: 10,
+					block: 25,
+					reason: CandidacyBondReleaseReason::Idle
+				})
+			);
 		});
 	}
 
@@ -3440,7 +3543,14 @@ mod session_management {
 			assert_eq!(SessionHandlerCollators::get(), vec![3]);
 			// kicked collator gets funds back after a delay
 			assert_eq!(Balances::balance_frozen(&FreezeReason::Releasing.into(), &5), 10);
-			assert_eq!(CandidacyBondReleases::<Test>::get(5), Some((10, 25)));
+			assert_eq!(
+				CandidacyBondReleases::<Test>::get(5),
+				Some(CandidacyBondRelease {
+					bond: 10,
+					block: 25,
+					reason: CandidacyBondReleaseReason::Idle
+				})
+			);
 		});
 	}
 }