Skip to content

pallet-balances: Clean up empty reserves & do not send the same event twice #12318

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.

Sign up for GitHub

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

Open
bkchr wants to merge 2 commits into
base: master
Choose a base branch
from
+167 −75
Open

pallet-balances: Clean up empty reserves & do not send the same event twice #12318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
711a731
pallet-balances: Clean up empty reserves & do not send the same event…
bkchr Jun 9, 2026
8d51898
Update from github-actions[bot] running command 'prdoc --audience run…
github-actions[bot] Jun 9, 2026
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
8 changes: 8 additions & 0 deletions prdoc/pr_12318.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
title: 'pallet-balances: Clean up empty reserves & do not send the same event twice'
doc:
- audience: Runtime Dev
description: |-
When reserves are cleared, the corresponding storage should be cleaned up as well. So, we do not run into `MaxReserves` with only empty reserves. This pull request also fixes the double sending of the same event when slashing a reserved amount.
crates:
- name: pallet-balances
bump: patch
180 changes: 105 additions & 75 deletions substrate/frame/balances/src/impl_currency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,26 +838,41 @@ where
return (NegativeImbalance::zero(), Zero::zero());
}

Reserves::<T, I>::mutate(who, |reserves| -> (Self::NegativeImbalance, Self::Balance) {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let to_change = cmp::min(reserves[index].amount, value);
Reserves::<T, I>::mutate_exists(
who,
|maybe_reserves| -> (Self::NegativeImbalance, Self::Balance) {
let Some(reserves) = maybe_reserves.as_mut() else {
return (NegativeImbalance::zero(), value);
};
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let to_change = cmp::min(reserves[index].amount, value);

let (imb, remain) =
<Self as ReservableCurrency<_>>::slash_reserved(who, to_change);
let (imb, remain) =
<Self as ReservableCurrency<_>>::slash_reserved(who, to_change);

// remain should always be zero but just to be defensive here.
let actual = to_change.defensive_saturating_sub(remain);
// remain should always be zero but just to be defensive here.
let actual = to_change.defensive_saturating_sub(remain);

// `actual <= to_change` and `to_change <= amount`; qed;
reserves[index].amount -= actual;
// `actual <= to_change` and `to_change <= amount`; qed;
reserves[index].amount -= actual;

Self::deposit_event(Event::Slashed { who: who.clone(), amount: actual });
(imb, value - actual)
},
Err(_) => (NegativeImbalance::zero(), value),
}
})
if reserves[index].amount.is_zero() {
if reserves.len() == 1 {
// no more named reserves
*maybe_reserves = None;
} else {
// remove this named reserve
reserves.remove(index);
}
}

(imb, value - actual)
},
Err(_) => (NegativeImbalance::zero(), value),
}
},
)
}

/// Move the reserved balance of one account into the balance of another, according to `status`.
Expand Down Expand Up @@ -886,81 +901,96 @@ where
};
}

Reserves::<T, I>::try_mutate(slashed, |reserves| -> Result<Self::Balance, DispatchError> {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let to_change = cmp::min(reserves[index].amount, value);

let actual = if status == Status::Reserved {
// make it the reserved under same identifier
Reserves::<T, I>::try_mutate(
beneficiary,
|reserves| -> Result<T::Balance, DispatchError> {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let remain =
Reserves::<T, I>::try_mutate_exists(
slashed,
|maybe_reserves| -> Result<Self::Balance, DispatchError> {
let Some(reserves) = maybe_reserves.as_mut() else { return Ok(value) };
match reserves.binary_search_by_key(id, |data| data.id) {

@ggwpez ggwpez Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could be another let else since the Err(_) match is trivial.

Ok(index) => {
let to_change = cmp::min(reserves[index].amount, value);

let actual = if status == Status::Reserved {

@ggwpez ggwpez Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: could be a match to hard-error if a variant is ever added to Status.

// make it the reserved under same identifier
Reserves::<T, I>::try_mutate(
beneficiary,
|reserves| -> Result<T::Balance, DispatchError> {
match reserves.binary_search_by_key(id, |data| data.id) {
Ok(index) => {
let remain =
<Self as ReservableCurrency<_>>::repatriate_reserved(
slashed,
beneficiary,
to_change,
status,
)?;

// remain should always be zero but just to be defensive
// here.
let actual = to_change.defensive_saturating_sub(remain);
// remain should always be zero but just to be defensive
// here.
let actual = to_change.defensive_saturating_sub(remain);

// this add can't overflow but just to be defensive.
reserves[index].amount =
reserves[index].amount.defensive_saturating_add(actual);
// this add can't overflow but just to be defensive.
reserves[index].amount = reserves[index]
.amount
.defensive_saturating_add(actual);

Ok(actual)
},
Err(index) => {
let remain =
Ok(actual)
},
Err(index) => {
let remain =
<Self as ReservableCurrency<_>>::repatriate_reserved(
slashed,
beneficiary,
to_change,
status,
)?;

// remain should always be zero but just to be defensive
// here
let actual = to_change.defensive_saturating_sub(remain);

reserves
.try_insert(
index,
ReserveData { id: *id, amount: actual },
)
.map_err(|_| Error::<T, I>::TooManyReserves)?;

Ok(actual)
},
}
},
)?
} else {
let remain = <Self as ReservableCurrency<_>>::repatriate_reserved(
slashed,
beneficiary,
to_change,
status,
)?;

// remain should always be zero but just to be defensive here
to_change.defensive_saturating_sub(remain)
};

// `actual <= to_change` and `to_change <= amount`; qed;
reserves[index].amount -= actual;

Ok(value - actual)
},
Err(_) => Ok(value),
}
})
// remain should always be zero but just to be defensive
// here
let actual = to_change.defensive_saturating_sub(remain);

reserves
.try_insert(
index,
ReserveData { id: *id, amount: actual },
)
.map_err(|_| Error::<T, I>::TooManyReserves)?;
Comment on lines +951 to +956

@dhirajs0 dhirajs0 Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small asymmetry: the beneficiary try_insert isn't guarded, so when repatriate_reserved moves nothing (actual == 0 under Polite, e.g. reserved funds frozen) we insert a zero-amount entry on the receiver — the same leak this PR fixes on the slashed side, just mirrored. One-line guard (if !actual.is_zero()) keeps it symmetric. Low priority since the only in-tree caller uses Status::Free. Intentional?

Failing repro (red on this branch, green with the guard) 
#[test]
fn repatriate_reserved_named_reserved_status_does_not_leak_empty_beneficiary_entry() {
	ExtBuilder::default().build_and_execute_with(|| {
		let _ = Balances::deposit_creating(&1, 111);
		let _ = Balances::deposit_creating(&2, 100);

		// Account 1 holds a named reserve `Foo = 10` (free 101, reserved 10).
		assert_ok!(Balances::reserve_named(&TestId::Foo, &1, 10));
		assert_eq!(Balances::reserved_balance_named(&TestId::Foo, &1), 10);

		// Freeze the whole balance so the reserved funds are unmovable 
		Balances::set_lock(ID_1, &1, 111, WithdrawReasons::all());

		// Beneficiary 2 has no entry under `Foo`, so the insert path is taken.
		assert!(Balances::reserves(&2).is_empty());

		// Nothing can move, so `actual == 0` and the call returns the full value.
		assert_eq!(
			Balances::repatriate_reserved_named(&TestId::Foo, &1, &2, 10, Reserved).unwrap(),
			10,
		);
		assert_eq!(Balances::reserved_balance_named(&TestId::Foo, &2), 0);

		// Since nothing moved, the beneficiary must NOT gain a named-reserve entry.
		assert!(
			Balances::reserves(&2).is_empty(),
			"no balance moved, so the beneficiary must not gain a named-reserve entry",
		);

		// No `MaxReserves` (= 2) slot was leaked, so two real reserves still fit.
		assert_ok!(Balances::reserve_named(&TestId::Bar, &2, 5));
		assert_ok!(Balances::reserve_named(&TestId::Baz, &2, 5));
	});
}

I have pushed this as a branch (repatriate-reserved-named-empty-entry-repro) if you'd rather run it directly.


Ok(actual)
},
}
},
)?
} else {
let remain = <Self as ReservableCurrency<_>>::repatriate_reserved(
slashed,
beneficiary,
to_change,
status,
)?;

// remain should always be zero but just to be defensive here
to_change.defensive_saturating_sub(remain)
};

// `actual <= to_change` and `to_change <= amount`; qed;
reserves[index].amount -= actual;

if reserves[index].amount.is_zero() {
if reserves.len() == 1 {
// no more named reserves
*maybe_reserves = None;
} else {
// remove this named reserve
reserves.remove(index);
}
}

Ok(value - actual)
},
Err(_) => Ok(value),
}
},
)
}
}

Expand Down
54 changes: 54 additions & 0 deletions substrate/frame/balances/src/tests/currency_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1313,6 +1313,60 @@ fn named_reserve_should_work() {
});
}

#[test]
fn slash_reserved_named_removes_empty_reserve_entry() {
ExtBuilder::default().build_and_execute_with(|| {
let _ = Balances::deposit_creating(&1, 111);

assert_ok!(Balances::reserve_named(&TestId::Foo, &1, 10));

assert_eq!(Balances::slash_reserved_named(&TestId::Foo, &1, 10).1, 0);
assert_eq!(Balances::reserved_balance_named(&TestId::Foo, &1), 0);

// The zeroed entry must be removed, as `unreserve_named` does.
assert!(Balances::reserves(&1).is_empty());

// With `MaxReserves = 2`, two other ids must still fit. A leaked zero-amount
// entry would make the second `reserve_named` fail with `TooManyReserves`.
assert_ok!(Balances::reserve_named(&TestId::Bar, &1, 10));
assert_ok!(Balances::reserve_named(&TestId::Baz, &1, 10));
});
}

#[test]
fn repatriate_reserved_named_removes_empty_reserve_entry() {
ExtBuilder::default().build_and_execute_with(|| {
let _ = Balances::deposit_creating(&1, 111);
let _ = Balances::deposit_creating(&2, 100);

assert_ok!(Balances::reserve_named(&TestId::Foo, &1, 10));

assert_eq!(Balances::repatriate_reserved_named(&TestId::Foo, &1, &2, 10, Free).unwrap(), 0);
assert_eq!(Balances::reserved_balance_named(&TestId::Foo, &1), 0);

// The zeroed entry must be removed, as `unreserve_named` does.
assert!(Balances::reserves(&1).is_empty());
});
}

#[test]
fn slash_reserved_named_emits_single_slashed_event() {
ExtBuilder::default().build_and_execute_with(|| {
let _ = Balances::deposit_creating(&1, 111);

assert_ok!(Balances::reserve_named(&TestId::Foo, &1, 10));
let _ = events();

assert_eq!(Balances::slash_reserved_named(&TestId::Foo, &1, 10).1, 0);

let slashed_count = events()
.into_iter()
.filter(|e| matches!(e, RuntimeEvent::Balances(Event::Slashed { .. })))
.count();
assert_eq!(slashed_count, 1, "`Slashed` must be emitted exactly once");
});
}

#[test]
fn reserve_must_succeed_if_can_reserve_does() {
ExtBuilder::default().build_and_execute_with(|| {
Expand Down
Loading