Fix account ref-counting in session (#8538)

* Fix account ref-counting in session.

* Avoid needless check

* fix compile

* put back in check and conversion

* Fix test to actually catch this error

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
This commit is contained in:
Gavin Wood
2021-04-06 17:57:37 +02:00
committed by GitHub
parent ca57860a13
commit 6a8c6b2b0a
2 changed files with 16 additions and 9 deletions
+8 -4
View File
@@ -750,11 +750,11 @@ impl<T: Config> Module<T> {
let who = T::ValidatorIdOf::convert(account.clone())
.ok_or(Error::<T>::NoAssociatedValidatorId)?;
frame_system::Pallet::<T>::inc_consumers(&account).map_err(|_| Error::<T>::NoAccount)?;
ensure!(frame_system::Pallet::<T>::can_inc_consumer(&account), Error::<T>::NoAccount);
let old_keys = Self::inner_set_keys(&who, keys)?;
if old_keys.is_some() {
let _ = frame_system::Pallet::<T>::dec_consumers(&account);
// ^^^ Defensive only; Consumers were incremented just before, so should never fail.
if old_keys.is_none() {
let assertion = frame_system::Pallet::<T>::inc_consumers(&account).is_ok();
debug_assert!(assertion, "can_inc_consumer() returned true; no change since; qed");
}
Ok(())
@@ -777,6 +777,10 @@ impl<T: Config> Module<T> {
Self::key_owner(*id, key).map_or(true, |owner| &owner == who),
Error::<T>::DuplicatedKey,
);
}
for id in T::Keys::key_ids() {
let key = keys.get_raw(*id);
if let Some(old) = old_keys.as_ref().map(|k| k.get_raw(*id)) {
if key == old {
+8 -5
View File
@@ -18,8 +18,9 @@
// Tests for the Session Pallet
use super::*;
use mock::Test;
use codec::Decode;
use frame_support::{traits::OnInitialize, assert_ok};
use frame_support::{traits::OnInitialize, assert_ok, assert_noop};
use sp_core::crypto::key_types::DUMMY;
use sp_runtime::testing::UintAuthorityId;
use mock::{
@@ -181,11 +182,14 @@ fn duplicates_are_not_allowed() {
new_test_ext().execute_with(|| {
System::set_block_number(1);
Session::on_initialize(1);
assert!(Session::set_keys(Origin::signed(4), UintAuthorityId(1).into(), vec![]).is_err());
assert!(Session::set_keys(Origin::signed(1), UintAuthorityId(10).into(), vec![]).is_ok());
assert_noop!(
Session::set_keys(Origin::signed(4), UintAuthorityId(1).into(), vec![]),
Error::<Test>::DuplicatedKey,
);
assert_ok!(Session::set_keys(Origin::signed(1), UintAuthorityId(10).into(), vec![]));
// is fine now that 1 has migrated off.
assert!(Session::set_keys(Origin::signed(4), UintAuthorityId(1).into(), vec![]).is_ok());
assert_ok!(Session::set_keys(Origin::signed(4), UintAuthorityId(1).into(), vec![]));
});
}
@@ -357,7 +361,6 @@ fn return_true_if_more_than_third_is_disabled() {
#[test]
fn upgrade_keys() {
use frame_support::storage;
use mock::Test;
use sp_core::crypto::key_types::DUMMY;
// This test assumes certain mocks.