diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 3255bc20af..cbe70598a9 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -750,11 +750,11 @@ impl Module { let who = T::ValidatorIdOf::convert(account.clone()) .ok_or(Error::::NoAssociatedValidatorId)?; - frame_system::Pallet::::inc_consumers(&account).map_err(|_| Error::::NoAccount)?; + ensure!(frame_system::Pallet::::can_inc_consumer(&account), Error::::NoAccount); let old_keys = Self::inner_set_keys(&who, keys)?; - if old_keys.is_some() { - let _ = frame_system::Pallet::::dec_consumers(&account); - // ^^^ Defensive only; Consumers were incremented just before, so should never fail. + if old_keys.is_none() { + let assertion = frame_system::Pallet::::inc_consumers(&account).is_ok(); + debug_assert!(assertion, "can_inc_consumer() returned true; no change since; qed"); } Ok(()) @@ -777,6 +777,10 @@ impl Module { Self::key_owner(*id, key).map_or(true, |owner| &owner == who), Error::::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 { diff --git a/substrate/frame/session/src/tests.rs b/substrate/frame/session/src/tests.rs index a528b3293d..f48388b5a0 100644 --- a/substrate/frame/session/src/tests.rs +++ b/substrate/frame/session/src/tests.rs @@ -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::::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.