Fix off by one error in proportional slashing (#11782)

* Fix proportional slashing logic

* Update frame/nomination-pools/test-staking/src/lib.rs

Co-authored-by: David <dvdplm@gmail.com>

* Update frame/staking/src/lib.rs

Co-authored-by: David <dvdplm@gmail.com>

* Update frame/staking/src/lib.rs

Co-authored-by: David <dvdplm@gmail.com>

* Update frame/staking/src/lib.rs

Co-authored-by: David <dvdplm@gmail.com>

* fmt

* Update frame/nomination-pools/test-staking/src/lib.rs

* clean

* fix

* last fixes

* doc

Co-authored-by: David <dvdplm@gmail.com>

substrate/frame/nomination-pools/test-staking/src/lib.rs

@@ -22,7 +22,8 @@ mod mock;
 use frame_support::{assert_noop, assert_ok, bounded_btree_map, traits::Currency};
 use mock::*;
 use pallet_nomination_pools::{
-	Error as PoolsError, Event as PoolsEvent, LastPoolId, PoolMember, PoolMembers, PoolState,
+	BondedPools, Error as PoolsError, Event as PoolsEvent, LastPoolId, PoolMember, PoolMembers,
+	PoolState,
 };
 use pallet_staking::{CurrentEra, Event as StakingEvent, Payee, RewardDestination};
 
@@ -273,7 +274,7 @@ fn pool_slash_e2e() {
 			30,
 			&mut Default::default(),
 			&mut Default::default(),
-			1, // slash era 1, affects chunks at era 5 onwards.
+			2, // slash era 2, affects chunks at era 5 onwards.
 		);
 
 		assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Slashed(POOL1_BONDED, 30)]);
@@ -371,3 +372,254 @@ fn pool_slash_e2e() {
 		);
 	});
 }
+
+#[test]
+fn pool_slash_proportional() {
+	// a typical example where 3 pool members unbond in era 99, 100, and 101, and a slash that
+	// happened in era 100 should only affect the latter two.
+	new_test_ext().execute_with(|| {
+		ExistentialDeposit::set(1);
+		BondingDuration::set(28);
+		assert_eq!(Balances::minimum_balance(), 1);
+		assert_eq!(Staking::current_era(), None);
+
+		// create the pool, we know this has id 1.
+		assert_ok!(Pools::create(Origin::signed(10), 40, 10, 10, 10));
+		assert_eq!(LastPoolId::<T>::get(), 1);
+
+		assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Bonded(POOL1_BONDED, 40)]);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![
+				PoolsEvent::Created { depositor: 10, pool_id: 1 },
+				PoolsEvent::Bonded { member: 10, pool_id: 1, bonded: 40, joined: true },
+			]
+		);
+
+		// have two members join
+		let bond = 20;
+		assert_ok!(Pools::join(Origin::signed(20), bond, 1));
+		assert_ok!(Pools::join(Origin::signed(21), bond, 1));
+		assert_ok!(Pools::join(Origin::signed(22), bond, 1));
+
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![
+				StakingEvent::Bonded(POOL1_BONDED, bond),
+				StakingEvent::Bonded(POOL1_BONDED, bond),
+				StakingEvent::Bonded(POOL1_BONDED, bond),
+			]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![
+				PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: bond, joined: true },
+				PoolsEvent::Bonded { member: 21, pool_id: 1, bonded: bond, joined: true },
+				PoolsEvent::Bonded { member: 22, pool_id: 1, bonded: bond, joined: true },
+			]
+		);
+
+		// now let's progress a lot.
+		CurrentEra::<T>::set(Some(99));
+
+		// and unbond
+		assert_ok!(Pools::unbond(Origin::signed(20), 20, bond));
+
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Unbonded(POOL1_BONDED, bond),]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::Unbonded { member: 20, pool_id: 1, balance: bond, points: bond }]
+		);
+
+		CurrentEra::<T>::set(Some(100));
+		assert_ok!(Pools::unbond(Origin::signed(21), 21, bond));
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Unbonded(POOL1_BONDED, bond),]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::Unbonded { member: 21, pool_id: 1, balance: bond, points: bond }]
+		);
+
+		CurrentEra::<T>::set(Some(101));
+		assert_ok!(Pools::unbond(Origin::signed(22), 22, bond));
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Unbonded(POOL1_BONDED, bond),]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::Unbonded { member: 22, pool_id: 1, balance: bond, points: bond }]
+		);
+
+		// Apply a slash that happened in era 100. This is typically applied with a delay.
+		// Of the total 100, 50 is slashed.
+		assert_eq!(BondedPools::<T>::get(1).unwrap().points, 40);
+		pallet_staking::slashing::do_slash::<Runtime>(
+			&POOL1_BONDED,
+			50,
+			&mut Default::default(),
+			&mut Default::default(),
+			100,
+		);
+
+		assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Slashed(POOL1_BONDED, 50)]);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![
+				// This last pool got slashed only the leftover dust. Otherwise in principle, this
+				// chunk/pool should have not been affected.
+				PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 127, balance: 19 },
+				// This pool got slashed 12.5, which rounded down to 12.
+				PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 128, balance: 8 },
+				// This pool got slashed 12.5, which rounded down to 12.
+				PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 129, balance: 8 },
+				// Bonded pool got slashed for 25, remaining 15 in it.
+				PoolsEvent::PoolSlashed { pool_id: 1, balance: 15 }
+			]
+		);
+	});
+}
+
+#[test]
+fn pool_slash_non_proportional_only_bonded_pool() {
+	// A typical example where a pool member unbonds in era 99, and he can get away with a slash
+	// that happened in era 100, as long as the pool has enough active bond to cover the slash. If
+	// everything else in the slashing/staking system works, this should always be the case.
+	// Nonetheless, `ledger.slash` has been written such that it will slash greedily from any chunk
+	// if it runs out of chunks that it thinks should be affected by the slash.
+	new_test_ext().execute_with(|| {
+		ExistentialDeposit::set(1);
+		BondingDuration::set(28);
+		assert_eq!(Balances::minimum_balance(), 1);
+		assert_eq!(Staking::current_era(), None);
+
+		// create the pool, we know this has id 1.
+		assert_ok!(Pools::create(Origin::signed(10), 40, 10, 10, 10));
+		assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Bonded(POOL1_BONDED, 40)]);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![
+				PoolsEvent::Created { depositor: 10, pool_id: 1 },
+				PoolsEvent::Bonded { member: 10, pool_id: 1, bonded: 40, joined: true },
+			]
+		);
+
+		// have two members join
+		let bond = 20;
+		assert_ok!(Pools::join(Origin::signed(20), bond, 1));
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Bonded(POOL1_BONDED, bond)]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: bond, joined: true }]
+		);
+
+		// progress and unbond.
+		CurrentEra::<T>::set(Some(99));
+		assert_ok!(Pools::unbond(Origin::signed(20), 20, bond));
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Unbonded(POOL1_BONDED, bond)]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::Unbonded { member: 20, pool_id: 1, balance: bond, points: bond }]
+		);
+
+		// slash for 30. This will be deducted only from the bonded pool.
+		CurrentEra::<T>::set(Some(100));
+		assert_eq!(BondedPools::<T>::get(1).unwrap().points, 40);
+		pallet_staking::slashing::do_slash::<Runtime>(
+			&POOL1_BONDED,
+			30,
+			&mut Default::default(),
+			&mut Default::default(),
+			100,
+		);
+
+		assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Slashed(POOL1_BONDED, 30)]);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::PoolSlashed { pool_id: 1, balance: 10 }]
+		);
+	});
+}
+
+#[test]
+fn pool_slash_non_proportional_bonded_pool_and_chunks() {
+	// An uncommon example where even though some funds are unlocked such that they should not be
+	// affected by a slash, we still slash out of them. This should not happen at all. If a
+	// nomination has unbonded, from the next era onwards, their exposure will drop, so if an era
+	// happens in that era, then their share of that slash should naturally be less, such that only
+	// their active ledger stake is enough to compensate it.
+	new_test_ext().execute_with(|| {
+		ExistentialDeposit::set(1);
+		BondingDuration::set(28);
+		assert_eq!(Balances::minimum_balance(), 1);
+		assert_eq!(Staking::current_era(), None);
+
+		// create the pool, we know this has id 1.
+		assert_ok!(Pools::create(Origin::signed(10), 40, 10, 10, 10));
+		assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Bonded(POOL1_BONDED, 40)]);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![
+				PoolsEvent::Created { depositor: 10, pool_id: 1 },
+				PoolsEvent::Bonded { member: 10, pool_id: 1, bonded: 40, joined: true },
+			]
+		);
+
+		// have two members join
+		let bond = 20;
+		assert_ok!(Pools::join(Origin::signed(20), bond, 1));
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Bonded(POOL1_BONDED, bond)]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: bond, joined: true }]
+		);
+
+		// progress and unbond.
+		CurrentEra::<T>::set(Some(99));
+		assert_ok!(Pools::unbond(Origin::signed(20), 20, bond));
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Unbonded(POOL1_BONDED, bond)]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::Unbonded { member: 20, pool_id: 1, balance: bond, points: bond }]
+		);
+
+		// slash 50. This will be deducted only from the bonded pool and one of the unbonding pools.
+		CurrentEra::<T>::set(Some(100));
+		assert_eq!(BondedPools::<T>::get(1).unwrap().points, 40);
+		pallet_staking::slashing::do_slash::<Runtime>(
+			&POOL1_BONDED,
+			50,
+			&mut Default::default(),
+			&mut Default::default(),
+			100,
+		);
+
+		assert_eq!(staking_events_since_last_call(), vec![StakingEvent::Slashed(POOL1_BONDED, 50)]);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![
+				// out of 20, 10 was taken.
+				PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 127, balance: 10 },
+				// out of 40, all was taken.
+				PoolsEvent::PoolSlashed { pool_id: 1, balance: 0 }
+			]
+		);
+	});
+}

substrate/frame/nomination-pools/test-staking/src/mock.rs

@@ -24,6 +24,8 @@ type AccountIndex = u32;
 type BlockNumber = u64;
 type Balance = u128;
 
+pub(crate) type T = Runtime;
+
 pub(crate) const POOL1_BONDED: AccountId = 20318131474730217858575332831085u128;
 pub(crate) const POOL1_REWARD: AccountId = 20397359637244482196168876781421u128;
 
@@ -194,13 +196,14 @@ frame_support::construct_runtime!(
 );
 
 pub fn new_test_ext() -> sp_io::TestExternalities {
+	sp_tracing::try_init_simple();
 	let mut storage = frame_system::GenesisConfig::default().build_storage::<Runtime>().unwrap();
 	let _ = pallet_nomination_pools::GenesisConfig::<Runtime> {
 		min_join_bond: 2,
 		min_create_bond: 2,
 		max_pools: Some(3),
-		max_members_per_pool: Some(3),
+		max_members_per_pool: Some(5),
-		max_members: Some(3 * 3),
+		max_members: Some(3 * 5),
 	}
 	.assimilate_storage(&mut storage)
 	.unwrap();

substrate/frame/staking/src/lib.rs

@@ -529,14 +529,27 @@ impl<T: Config> StakingLedger<T> {
 		(self, unlocking_balance)
 	}
 
-	/// Slash the staker for a given amount of balance. This can grow the value of the slash in the
+	/// Slash the staker for a given amount of balance.
-	/// case that either the active bonded or some unlocking chunks become dust after slashing.
+	///
-	/// Returns the amount of funds actually slashed.
+	/// This implements a proportional slashing system, whereby we set our preference to slash as
+	/// such:
+	///
+	/// - If any unlocking chunks exist that are scheduled to be unlocked at `slash_era +
+	///   bonding_duration` and onwards, the slash is divided equally between the active ledger and
+	///   the unlocking chunks.
+	/// - If no such chunks exist, then only the active balance is slashed.
+	///
+	/// Note that the above is only a *preference*. If for any reason the active ledger, with or
+	/// without some portion of the unlocking chunks that are more justified to be slashed are not
+	/// enough, then the slashing will continue and will consume as much of the active and unlocking
+	/// chunks as needed.
+	///
+	/// This will never slash more than the given amount. If any of the chunks become dusted, the
+	/// last chunk is slashed slightly less to compensate. Returns the amount of funds actually
+	/// slashed.
 	///
 	/// `slash_era` is the era in which the slash (which is being enacted now) actually happened.
 	///
-	/// # Note
-	///
 	/// This calls `Config::OnStakerSlash::on_slash` with information as to how the slash was
 	/// applied.
 	fn slash(
@@ -545,54 +558,81 @@ impl<T: Config> StakingLedger<T> {
 		minimum_balance: BalanceOf<T>,
 		slash_era: EraIndex,
 	) -> BalanceOf<T> {
-		use sp_staking::OnStakerSlash as _;
-
 		if slash_amount.is_zero() {
 			return Zero::zero()
 		}
 
+		use sp_staking::OnStakerSlash as _;
 		let mut remaining_slash = slash_amount;
 		let pre_slash_total = self.total;
 
-		let era_after_slash = slash_era + 1;
+		// for a `slash_era = x`, any chunk that is scheduled to be unlocked at era `x + 28`
-		let chunk_unlock_era_after_slash = era_after_slash + T::BondingDuration::get();
+		// (assuming 28 is the bonding duration) onwards should be slashed.
+		let slashable_chunks_start = slash_era + T::BondingDuration::get();
 
-		// Calculate the total balance of active funds and unlocking funds in the affected range.
+		// `Some(ratio)` if this is proportional, with `ratio`, `None` otherwise. In both cases, we
-		let (affected_balance, slash_chunks_priority): (_, Box<dyn Iterator<Item = usize>>) = {
+		// slash first the active chunk, and then `slash_chunks_priority`.
-			if let Some(start_index) =
+		let (maybe_proportional, slash_chunks_priority) = {
-				self.unlocking.iter().position(|c| c.era >= chunk_unlock_era_after_slash)
+			if let Some(first_slashable_index) =
+				self.unlocking.iter().position(|c| c.era >= slashable_chunks_start)
 			{
+				// If there exists a chunk who's after the first_slashable_start, then this is a
+				// proportional slash, because we want to slash active and these chunks
+				// proportionally.
+
 				// The indices of the first chunk after the slash up through the most recent chunk.
 				// (The most recent chunk is at greatest from this era)
-				let affected_indices = start_index..self.unlocking.len();
+				let affected_indices = first_slashable_index..self.unlocking.len();
 				let unbonding_affected_balance =
 					affected_indices.clone().fold(BalanceOf::<T>::zero(), |sum, i| {
-						if let Some(chunk) = self.unlocking.get_mut(i).defensive() {
+						if let Some(chunk) = self.unlocking.get(i).defensive() {
 							sum.saturating_add(chunk.value)
 						} else {
 							sum
 						}
 					});
+				let affected_balance = self.active.saturating_add(unbonding_affected_balance);
+				let ratio = Perquintill::from_rational(slash_amount, affected_balance);
 				(
-					self.active.saturating_add(unbonding_affected_balance),
+					Some(ratio),
-					Box::new(affected_indices.chain((0..start_index).rev())),
+					affected_indices.chain((0..first_slashable_index).rev()).collect::<Vec<_>>(),
 				)
 			} else {
-				(self.active, Box::new((0..self.unlocking.len()).rev()))
+				// We just slash from the last chunk to the most recent one, if need be.
+				(None, (0..self.unlocking.len()).rev().collect::<Vec<_>>())
 			}
 		};
 
 		// Helper to update `target` and the ledgers total after accounting for slashing `target`.
-		let ratio = Perquintill::from_rational(slash_amount, affected_balance);
+		log!(
+			debug,
+			"slashing {:?} for era {:?} out of {:?}, priority: {:?}, proportional = {:?}",
+			slash_amount,
+			slash_era,
+			self,
+			slash_chunks_priority,
+			maybe_proportional,
+		);
+
 		let mut slash_out_of = |target: &mut BalanceOf<T>, slash_remaining: &mut BalanceOf<T>| {
-			let mut slash_from_target =
+			let mut slash_from_target = if let Some(ratio) = maybe_proportional {
-				if slash_amount < affected_balance { ratio * (*target) } else { *slash_remaining }
+				ratio * (*target)
-					.min(*target);
+			} else {
+				*slash_remaining
+			}
+			// this is the total that that the slash target has. We can't slash more than
+			// this anyhow!
+			.min(*target)
+			// this is the total amount that we would have wanted to slash
+			// non-proportionally, a proportional slash should never exceed this either!
+			.min(*slash_remaining);
 
 			// slash out from *target exactly `slash_from_target`.
 			*target = *target - slash_from_target;
 			if *target < minimum_balance {
-				// Slash the rest of the target if its dust
+				// Slash the rest of the target if it's dust. This might cause the last chunk to be
+				// slightly under-slashed, by at most `MaxUnlockingChunks * ED`, which is not a big
+				// deal.
 				slash_from_target =
 					sp_std::mem::replace(target, Zero::zero()).saturating_add(slash_from_target)
 			}
@@ -606,10 +646,11 @@ impl<T: Config> StakingLedger<T> {
 
 		let mut slashed_unlocking = BTreeMap::<_, _>::new();
 		for i in slash_chunks_priority {
+			if remaining_slash.is_zero() {
+				break
+			}
+
 			if let Some(chunk) = self.unlocking.get_mut(i).defensive() {
-				if remaining_slash.is_zero() {
-					break
-				}
 				slash_out_of(&mut chunk.value, &mut remaining_slash);
 				// write the new slashed value of this chunk to the map.
 				slashed_unlocking.insert(chunk.era, chunk.value);
@@ -618,7 +659,9 @@ impl<T: Config> StakingLedger<T> {
 			}
 		}
 
+		// clean unlocking chunks that are set to zero.
 		self.unlocking.retain(|c| !c.value.is_zero());
+
 		T::OnStakerSlash::on_slash(&self.stash, self.active, &slashed_unlocking);
 		pre_slash_total.saturating_sub(self.total)
 	}

substrate/frame/staking/src/tests.rs

@@ -2081,8 +2081,7 @@ fn reward_validator_slashing_validator_does_not_overflow() {
 		let _ = Balances::make_free_balance_be(&11, stake);
 		let _ = Balances::make_free_balance_be(&2, stake);
 
-		// only slashes out of bonded stake are applied. without this line,
+		// only slashes out of bonded stake are applied. without this line, it is 0.
-		// it is 0.
 		Staking::bond(Origin::signed(2), 20000, stake - 1, RewardDestination::default()).unwrap();
 		// Override exposure of 11
 		ErasStakers::<Test>::insert(
@@ -2104,7 +2103,7 @@ fn reward_validator_slashing_validator_does_not_overflow() {
 			&[Perbill::from_percent(100)],
 		);
 
-		assert_eq!(Balances::total_balance(&11), stake);
+		assert_eq!(Balances::total_balance(&11), stake - 1);
 		assert_eq!(Balances::total_balance(&2), 1);
 	})
 }
@@ -4960,7 +4959,6 @@ fn proportional_ledger_slash_works() {
 		unlocking: bounded_vec![],
 		claimed_rewards: vec![],
 	};
-
 	assert_eq!(BondingDuration::get(), 3);
 
 	// When we slash a ledger with no unlocking chunks
@@ -4997,7 +4995,7 @@ fn proportional_ledger_slash_works() {
 	ledger.total = 4 * 100;
 	ledger.active = 0;
 	// When the first 2 chunks don't overlap with the affected range of unlock eras.
-	assert_eq!(ledger.slash(140, 0, 2), 140);
+	assert_eq!(ledger.slash(140, 0, 3), 140);
 	// Then
 	assert_eq!(ledger.unlocking, vec![c(4, 100), c(5, 100), c(6, 30), c(7, 30)]);
 	assert_eq!(ledger.total, 4 * 100 - 140);
@@ -5039,7 +5037,7 @@ fn proportional_ledger_slash_works() {
 	ledger.active = 500;
 	ledger.total = 40 + 10 + 100 + 250 + 500; // 900
 	assert_eq!(ledger.total, 900);
-	// When  we have a higher min balance
+	// When we have a higher min balance
 	assert_eq!(
 		ledger.slash(
 			900 / 2,
@@ -5047,16 +5045,17 @@ fn proportional_ledger_slash_works() {
 			     * get swept */
 			0
 		),
-		475
+		450
 	);
-	let dust = (10 / 2) + (40 / 2);
 	assert_eq!(ledger.active, 500 / 2);
-	assert_eq!(ledger.unlocking, vec![c(5, 100 / 2), c(7, 250 / 2)]);
+	// the last chunk was not slashed 50% like all the rest, because some other earlier chunks got
-	assert_eq!(ledger.total, 900 / 2 - dust);
+	// dusted.
+	assert_eq!(ledger.unlocking, vec![c(5, 100 / 2), c(7, 150)]);
+	assert_eq!(ledger.total, 900 / 2);
 	assert_eq!(LedgerSlashPerEra::get().0, 500 / 2);
 	assert_eq!(
 		LedgerSlashPerEra::get().1,
-		BTreeMap::from([(4, 0), (5, 100 / 2), (6, 0), (7, 250 / 2)])
+		BTreeMap::from([(4, 0), (5, 100 / 2), (6, 0), (7, 150)])
 	);
 
 	// Given
@@ -5068,7 +5067,7 @@ fn proportional_ledger_slash_works() {
 		ledger.slash(
 			500 + 10 + 250 + 100 / 2, // active + era 6 + era 7 + era 5 / 2
 			0,
-			2 /* slash era 2+4 first, so the affected parts are era 2+4, era 3+4 and
+			3 /* slash era 6 first, so the affected parts are era 6, era 7 and
 			   * ledge.active. This will cause the affected to go to zero, and then we will
 			   * start slashing older chunks */
 		),
@@ -5091,7 +5090,7 @@ fn proportional_ledger_slash_works() {
 		ledger.slash(
 			351, // active + era 6 + era 7 + era 5 / 2 + 1
 			50,  // min balance - everything slashed below 50 will get dusted
-			2    /* slash era 2+4 first, so the affected parts are era 2+4, era 3+4 and
+			3    /* slash era 3+3 first, so the affected parts are era 6, era 7 and
 			      * ledge.active. This will cause the affected to go to zero, and then we will
 			      * start slashing older chunks */
 		),
@@ -5108,9 +5107,8 @@ fn proportional_ledger_slash_works() {
 
 	// Given
 	let slash = u64::MAX as Balance * 2;
-	let value = slash
+	// The value of the other parts of ledger that will get slashed
-		- (9 * 4) // The value of the other parts of ledger that will get slashed
+	let value = slash - (10 * 4);
-		+ 1;
 
 	ledger.active = 10;
 	ledger.unlocking = bounded_vec![c(4, 10), c(5, 10), c(6, 10), c(7, value)];