pallet-balances: Clean up empty reserves & do not send the same event twice#12318
pallet-balances: Clean up empty reserves & do not send the same event twice#12318bkchr wants to merge 2 commits into
Conversation
… twice 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.
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
| reserves | ||
| .try_insert( | ||
| index, | ||
| ReserveData { id: *id, amount: actual }, | ||
| ) | ||
| .map_err(|_| Error::<T, I>::TooManyReserves)?; |
There was a problem hiding this comment.
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.
| 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) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
nit: could be a match to hard-error if a variant is ever added to Status.
When reserves are cleared, the corresponding storage should be cleaned up as well. So, we do not run into
MaxReserveswith only empty reserves. This pull request also fixes the double sending of the same event when slashing a reserved amount.