From 4ed38683373e890eb26a47f3ab5f5c8cf31cbae9 Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Tue, 18 Feb 2025 20:42:17 +0100
Subject: [PATCH 1/8] remove unnecessary ownership
---
pallets/authority-members/src/lib.rs | 52 ++++++++++++++--------------
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/pallets/authority-members/src/lib.rs b/pallets/authority-members/src/lib.rs
index b69c0d72..99babcc9 100644
--- a/pallets/authority-members/src/lib.rs
+++ b/pallets/authority-members/src/lib.rs
@@ -258,17 +258,17 @@ pub mod pallet {
if !Members::<T>::contains_key(member_id) {
return Err(Error::<T>::MemberNotFound.into());
}
- if Self::is_outgoing(member_id) {
+ if Self::is_outgoing(&member_id) {
return Err(Error::<T>::AlreadyOutgoing.into());
}
- let is_incoming = Self::is_incoming(member_id);
- if !is_incoming && !Self::is_online(member_id) {
+ let is_incoming = Self::is_incoming(&member_id);
+ if !is_incoming && !Self::is_online(&member_id) {
return Err(Error::<T>::NotOnlineNorIncoming.into());
}
// Apply phase //
if is_incoming {
- Self::remove_in(member_id);
+ Self::remove_in(&member_id);
} else {
Self::insert_out(member_id);
}
@@ -284,7 +284,7 @@ pub mod pallet {
let who = ensure_signed(origin)?;
let member_id = Self::verify_ownership_and_membership(&who)?;
- if Self::is_blacklisted(member_id) {
+ if Self::is_blacklisted(&member_id) {
return Err(Error::<T>::MemberBlacklisted.into());
}
if !Members::<T>::contains_key(member_id) {
@@ -296,11 +296,11 @@ pub mod pallet {
return Err(Error::<T>::SessionKeysNotProvided.into());
}
- if Self::is_incoming(member_id) {
+ if Self::is_incoming(&member_id) {
return Err(Error::<T>::AlreadyIncoming.into());
}
- let is_outgoing = Self::is_outgoing(member_id);
- if Self::is_online(member_id) && !is_outgoing {
+ let is_outgoing = Self::is_outgoing(&member_id);
+ if Self::is_online(&member_id) && !is_outgoing {
return Err(Error::<T>::AlreadyOnline.into());
}
if Self::authorities_counter() >= T::MaxAuthorities::get() {
@@ -309,7 +309,7 @@ pub mod pallet {
// Apply phase //
if is_outgoing {
- Self::remove_out(member_id);
+ Self::remove_out(&member_id);
} else {
Self::insert_in(member_id);
}
@@ -427,14 +427,14 @@ pub mod pallet {
impl<T: Config> Pallet<T> {
/// Perform authority member removal.
fn do_remove_member(member_id: T::MemberId, owner_key: T::AccountId) {
- if Self::is_online(member_id) {
+ if Self::is_online(&member_id) {
// Trigger the member deletion for next session
Self::insert_out(member_id);
}
// remove all member data
- Self::remove_in(member_id);
- Self::remove_online(member_id);
+ Self::remove_in(&member_id);
+ Self::remove_online(&member_id);
Members::<T>::remove(member_id);
// Purge session keys
@@ -485,53 +485,53 @@ pub mod pallet {
}
/// Check if member is incoming.
- fn is_incoming(member_id: T::MemberId) -> bool {
+ fn is_incoming(member_id: &T::MemberId) -> bool {
IncomingAuthorities::<T>::get()
- .binary_search(&member_id)
+ .binary_search(member_id)
.is_ok()
}
/// C&heck if member is online.
- fn is_online(member_id: T::MemberId) -> bool {
+ fn is_online(member_id: &T::MemberId) -> bool {
OnlineAuthorities::<T>::get()
- .binary_search(&member_id)
+ .binary_search(member_id)
.is_ok()
}
/// Check if member is outgoing.
- fn is_outgoing(member_id: T::MemberId) -> bool {
+ fn is_outgoing(member_id: &T::MemberId) -> bool {
OutgoingAuthorities::<T>::get()
- .binary_search(&member_id)
+ .binary_search(member_id)
.is_ok()
}
/// Check if member is blacklisted.
- fn is_blacklisted(member_id: T::MemberId) -> bool {
- Blacklist::<T>::get().contains(&member_id)
+ fn is_blacklisted(member_id: &T::MemberId) -> bool {
+ Blacklist::<T>::get().contains(member_id)
}
/// Perform removal from incoming authorities.
- fn remove_in(member_id: T::MemberId) {
+ fn remove_in(member_id: &T::MemberId) {
IncomingAuthorities::<T>::mutate(|members_ids| {
- if let Ok(index) = members_ids.binary_search(&member_id) {
+ if let Ok(index) = members_ids.binary_search(member_id) {
members_ids.remove(index);
}
})
}
/// Perform removal from online authorities.
- fn remove_online(member_id: T::MemberId) {
+ fn remove_online(member_id: &T::MemberId) {
OnlineAuthorities::<T>::mutate(|members_ids| {
- if let Ok(index) = members_ids.binary_search(&member_id) {
+ if let Ok(index) = members_ids.binary_search(member_id) {
members_ids.remove(index);
}
});
}
/// Perform removal from outgoing authorities.
- fn remove_out(member_id: T::MemberId) {
+ fn remove_out(member_id: &T::MemberId) {
OutgoingAuthorities::<T>::mutate(|members_ids| {
- if let Ok(index) = members_ids.binary_search(&member_id) {
+ if let Ok(index) = members_ids.binary_search(member_id) {
members_ids.remove(index);
}
});
--
GitLab
From 3d40828c1145ee7b548dee61c4ee445986fbbbab Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Tue, 18 Feb 2025 21:09:53 +0100
Subject: [PATCH 2/8] reproduce #291
---
runtime/gdev/tests/integration_tests.rs | 30 +++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs
index 34d3150d..82c1b846 100644
--- a/runtime/gdev/tests/integration_tests.rs
+++ b/runtime/gdev/tests/integration_tests.rs
@@ -1516,6 +1516,36 @@ fn test_link_account() {
})
}
+/// test change owner key
+#[test]
+fn test_change_owner_key_validator_keys_set() {
+ ExtBuilder::new(1, 3, 4).build().execute_with(|| {
+ let genesis_hash = System::block_hash(0);
+ let bob = AccountKeyring::Bob.to_account_id();
+ let ferdie = AccountKeyring::Ferdie.to_account_id();
+ let payload = (b"icok", genesis_hash, 2u32, bob.clone()).encode();
+ let signature = AccountKeyring::Bob.sign(&payload);
+
+ // Bob is not an online validator, but his identity/owner_key is already associated.
+ // He should not be able to change its owner_key.
+ assert_ok!(AuthorityMembers::set_session_keys(
+ RuntimeOrigin::signed(AccountKeyring::Bob.to_account_id()),
+ create_dummy_session_keys()
+ ));
+ assert!(!pallet_authority_members::OnlineAuthorities::<Runtime>::get().contains(&2));
+ assert!(pallet_authority_members::Members::<Runtime>::get(2).is_some());
+
+ assert_noop!(
+ Identity::change_owner_key(
+ RuntimeOrigin::signed(bob.clone()),
+ ferdie.clone(),
+ signature.into()
+ ),
+ pallet_identity::Error::<gdev_runtime::Runtime>::OwnerKeyUsedAsValidator
+ );
+ })
+}
+
/// test change owner key
#[test]
fn test_change_owner_key_validator_online() {
--
GitLab
From 87568a28fbce2cd0c8265fd916d32ef5068e1e05 Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Tue, 18 Feb 2025 21:27:10 +0100
Subject: [PATCH 3/8] fix bad signature shadowed error
---
runtime/gdev/tests/integration_tests.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs
index 82c1b846..8b859ce7 100644
--- a/runtime/gdev/tests/integration_tests.rs
+++ b/runtime/gdev/tests/integration_tests.rs
@@ -1524,7 +1524,7 @@ fn test_change_owner_key_validator_keys_set() {
let bob = AccountKeyring::Bob.to_account_id();
let ferdie = AccountKeyring::Ferdie.to_account_id();
let payload = (b"icok", genesis_hash, 2u32, bob.clone()).encode();
- let signature = AccountKeyring::Bob.sign(&payload);
+ let signature = AccountKeyring::Ferdie.sign(&payload);
// Bob is not an online validator, but his identity/owner_key is already associated.
// He should not be able to change its owner_key.
@@ -1554,7 +1554,7 @@ fn test_change_owner_key_validator_online() {
let alice = AccountKeyring::Alice.to_account_id();
let ferdie = AccountKeyring::Ferdie.to_account_id();
let payload = (b"icok", genesis_hash, 1u32, alice.clone()).encode();
- let signature = AccountKeyring::Alice.sign(&payload);
+ let signature = AccountKeyring::Ferdie.sign(&payload);
// Alice is an online validator
assert!(pallet_authority_members::OnlineAuthorities::<Runtime>::get().contains(&1));
--
GitLab
From 0f23b7d023372632f481afb1c46a1e0b6853cf65 Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Tue, 18 Feb 2025 21:27:47 +0100
Subject: [PATCH 4/8] fix #291
---
pallets/authority-members/src/lib.rs | 24 +--
pallets/distance/src/mock.rs | 2 +-
pallets/duniter-wot/src/mock.rs | 2 +-
pallets/identity/src/lib.rs | 13 +-
pallets/identity/src/mock.rs | 2 +-
pallets/identity/src/traits.rs | 14 +-
pallets/quota/src/mock.rs | 2 +-
runtime/common/src/handlers.rs | 16 +-
runtime/common/src/pallets_config.rs | 2 +-
runtime/gdev/tests/integration_tests.rs | 241 +++++++++++-------------
10 files changed, 150 insertions(+), 168 deletions(-)
diff --git a/pallets/authority-members/src/lib.rs b/pallets/authority-members/src/lib.rs
index 99babcc9..b6c1bd13 100644
--- a/pallets/authority-members/src/lib.rs
+++ b/pallets/authority-members/src/lib.rs
@@ -378,25 +378,25 @@ pub mod pallet {
// Execute the annotated function in a new storage transaction.
// The return type of the annotated function must be `Result`. All changes to storage performed
// by the annotated function are discarded if it returns `Err`, or committed if `Ok`.
- #[frame_support::transactional]
/// Change the owner key of an authority member.
+ #[frame_support::transactional]
pub fn change_owner_key(
member_id: T::MemberId,
new_owner_key: T::AccountId,
) -> DispatchResultWithPostInfo {
- let old_owner_key = Members::<T>::mutate_exists(member_id, |maybe_member_data| {
- if let Some(ref mut member_data) = maybe_member_data {
- Ok(core::mem::replace(
- &mut member_data.owner_key,
- new_owner_key.clone(),
- ))
- } else {
- Err(Error::<T>::MemberNotFound)
- }
+ let old_owner_key = Members::<T>::try_mutate_exists(member_id, |maybe_member_data| {
+ let member_data = maybe_member_data
+ .as_mut()
+ .ok_or(Error::<T>::MemberNotFound)?;
+ Ok::<T::AccountId, Error<T>>(core::mem::replace(
+ &mut member_data.owner_key,
+ new_owner_key.clone(),
+ ))
})?;
let validator_id = T::ValidatorIdOf::convert(old_owner_key.clone())
.ok_or(pallet_session::Error::<T>::NoAssociatedValidatorId)?;
+
let session_keys = pallet_session::NextKeys::<T>::get(validator_id)
.ok_or(Error::<T>::SessionKeysNotProvided)?;
@@ -405,7 +405,7 @@ pub mod pallet {
.dispatch_bypass_filter(frame_system::RawOrigin::Signed(old_owner_key).into())?;
// Set session keys
- let _post_info = pallet_session::Call::<T>::set_keys {
+ pallet_session::Call::<T>::set_keys {
keys: session_keys,
proof: vec![],
}
@@ -491,7 +491,7 @@ pub mod pallet {
.is_ok()
}
- /// C&heck if member is online.
+ /// Check if member is online.
fn is_online(member_id: &T::MemberId) -> bool {
OnlineAuthorities::<T>::get()
.binary_search(member_id)
diff --git a/pallets/distance/src/mock.rs b/pallets/distance/src/mock.rs
index 06b89976..23f86521 100644
--- a/pallets/distance/src/mock.rs
+++ b/pallets/distance/src/mock.rs
@@ -262,9 +262,9 @@ impl pallet_identity::Config for Test {
type IdtyData = ();
type IdtyIndex = u32;
type IdtyNameValidator = IdtyNameValidatorTestImpl;
+ type OnKeyChange = ();
type OnNewIdty = ();
type OnRemoveIdty = ();
- type OwnerKeyChangePermission = ();
type RuntimeEvent = RuntimeEvent;
type Signature = TestSignature;
type Signer = UintAuthorityId;
diff --git a/pallets/duniter-wot/src/mock.rs b/pallets/duniter-wot/src/mock.rs
index e246133a..0685fc5d 100644
--- a/pallets/duniter-wot/src/mock.rs
+++ b/pallets/duniter-wot/src/mock.rs
@@ -148,9 +148,9 @@ impl pallet_identity::Config for Test {
type IdtyData = ();
type IdtyIndex = IdtyIndex;
type IdtyNameValidator = IdtyNameValidatorTestImpl;
+ type OnKeyChange = ();
type OnNewIdty = DuniterWot;
type OnRemoveIdty = DuniterWot;
- type OwnerKeyChangePermission = ();
type RuntimeEvent = RuntimeEvent;
type Signature = TestSignature;
type Signer = UintAuthorityId;
diff --git a/pallets/identity/src/lib.rs b/pallets/identity/src/lib.rs
index 0e00c7e7..45244e4a 100644
--- a/pallets/identity/src/lib.rs
+++ b/pallets/identity/src/lib.rs
@@ -137,8 +137,8 @@ pub mod pallet {
/// The type used to check account worthiness.
type CheckAccountWorthiness: CheckAccountWorthiness<Self>;
- /// Handler that checks the necessary permissions for an identity's owner key change.
- type OwnerKeyChangePermission: CheckKeyChangeAllowed<Self>;
+ /// A handler called when an identity's owner key change.
+ type OnKeyChange: KeyChange<Self>;
/// Custom data to store in each identity.
type IdtyData: Clone
@@ -461,12 +461,6 @@ pub mod pallet {
Error::<T>::OwnerKeyAlreadyUsed
);
- // Ensure that the key is not currently as a validator
- ensure!(
- T::OwnerKeyChangePermission::check_allowed(&idty_index),
- Error::<T>::OwnerKeyUsedAsValidator
- );
-
let block_number = frame_system::Pallet::<T>::block_number();
let maybe_old_old_owner_key =
if let Some((old_owner_key, last_change)) = idty_value.old_owner_key {
@@ -508,6 +502,7 @@ pub mod pallet {
IdentityIndexOf::<T>::insert(&idty_value.owner_key, idty_index);
Identities::<T>::insert(idty_index, idty_value);
T::AccountLinker::link_identity(&new_key, idty_index)?;
+ T::OnKeyChange::on_changed(idty_index, new_key.clone())?;
Self::deposit_event(Event::IdtyChangedOwnerKey {
idty_index,
new_owner_key: new_key,
@@ -815,8 +810,6 @@ pub mod pallet {
AccountNotExist,
/// Insufficient balance to create an identity.
InsufficientBalance,
- /// Owner key currently used as validator.
- OwnerKeyUsedAsValidator,
/// Legacy revocation document format is invalid
InvalidLegacyRevocationFormat,
}
diff --git a/pallets/identity/src/mock.rs b/pallets/identity/src/mock.rs
index de48f0a2..f38afd09 100644
--- a/pallets/identity/src/mock.rs
+++ b/pallets/identity/src/mock.rs
@@ -101,9 +101,9 @@ impl pallet_identity::Config for Test {
type IdtyData = ();
type IdtyIndex = u64;
type IdtyNameValidator = IdtyNameValidatorTestImpl;
+ type OnKeyChange = ();
type OnNewIdty = ();
type OnRemoveIdty = ();
- type OwnerKeyChangePermission = ();
type RuntimeEvent = RuntimeEvent;
type Signature = Signature;
type Signer = AccountPublic;
diff --git a/pallets/identity/src/traits.rs b/pallets/identity/src/traits.rs
index 8366d1cf..9aca1dc6 100644
--- a/pallets/identity/src/traits.rs
+++ b/pallets/identity/src/traits.rs
@@ -95,14 +95,14 @@ impl<AccountId, IdtyIndex> LinkIdty<AccountId, IdtyIndex> for () {
}
}
-/// Trait for checking whether a key change is allowed for a given identity.
-pub trait CheckKeyChangeAllowed<T: Config> {
- /// Determines if a key change is allowed for the given identity.
- fn check_allowed(account_id: &T::IdtyIndex) -> bool;
+/// A trait for handling identity owner key changes.
+pub trait KeyChange<T: Config> {
+ fn on_changed(id: T::IdtyIndex, account_id: T::AccountId) -> Result<(), DispatchError>;
}
-impl<T: Config> CheckKeyChangeAllowed<T> for () {
- fn check_allowed(_: &T::IdtyIndex) -> bool {
- true
+impl<T: Config> KeyChange<T> for () {
+ /// Called when an identity's owner key has changed.
+ fn on_changed(_id: T::IdtyIndex, _account_id: T::AccountId) -> Result<(), DispatchError> {
+ Ok(())
}
}
diff --git a/pallets/quota/src/mock.rs b/pallets/quota/src/mock.rs
index dcfd30f7..029192f0 100644
--- a/pallets/quota/src/mock.rs
+++ b/pallets/quota/src/mock.rs
@@ -145,9 +145,9 @@ impl pallet_identity::Config for Test {
type IdtyData = ();
type IdtyIndex = u64;
type IdtyNameValidator = IdtyNameValidatorTestImpl;
+ type OnKeyChange = ();
type OnNewIdty = ();
type OnRemoveIdty = ();
- type OwnerKeyChangePermission = ();
type RuntimeEvent = RuntimeEvent;
type Signature = Signature;
type Signer = AccountPublic;
diff --git a/runtime/common/src/handlers.rs b/runtime/common/src/handlers.rs
index 3fdd7b2d..ba0d0f29 100644
--- a/runtime/common/src/handlers.rs
+++ b/runtime/common/src/handlers.rs
@@ -171,16 +171,20 @@ where
}
/// Runtime handler OwnerKeyChangePermission.
-pub struct OwnerKeyChangePermissionHandler<Runtime>(core::marker::PhantomData<Runtime>);
+pub struct KeyChangeHandler<Runtime>(core::marker::PhantomData<Runtime>);
impl<
- Runtime: frame_system::Config
+ Runtime: frame_system::Config<AccountId = AccountId>
+ pallet_identity::Config<IdtyIndex = IdtyIndex>
+ pallet_authority_members::Config<MemberId = IdtyIndex>,
- > pallet_identity::traits::CheckKeyChangeAllowed<Runtime>
- for OwnerKeyChangePermissionHandler<Runtime>
+ > pallet_identity::traits::KeyChange<Runtime> for KeyChangeHandler<Runtime>
{
- fn check_allowed(idty_index: &IdtyIndex) -> bool {
- !pallet_authority_members::Pallet::<Runtime>::online().contains(idty_index)
+ fn on_changed(
+ idty_index: IdtyIndex,
+ account_id: AccountId,
+ ) -> Result<(), sp_runtime::DispatchError> {
+ pallet_authority_members::Pallet::<Runtime>::change_owner_key(idty_index, account_id)
+ .map_err(|e| e.error)?;
+ Ok(())
}
}
diff --git a/runtime/common/src/pallets_config.rs b/runtime/common/src/pallets_config.rs
index c3b6e728..af8bdf59 100644
--- a/runtime/common/src/pallets_config.rs
+++ b/runtime/common/src/pallets_config.rs
@@ -444,9 +444,9 @@ macro_rules! pallets_config {
type IdtyData = IdtyData;
type IdtyIndex = IdtyIndex;
type IdtyNameValidator = IdtyNameValidatorImpl;
+ type OnKeyChange = KeyChangeHandler<Runtime>;
type OnNewIdty = OnNewIdtyHandler<Runtime>;
type OnRemoveIdty = OnRemoveIdtyHandler<Runtime>;
- type OwnerKeyChangePermission = OwnerKeyChangePermissionHandler<Runtime>;
type RuntimeEvent = RuntimeEvent;
type Signature = Signature;
type Signer = <Signature as sp_runtime::traits::Verify>::Signer;
diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs
index 8b859ce7..32dd489f 100644
--- a/runtime/gdev/tests/integration_tests.rs
+++ b/runtime/gdev/tests/integration_tests.rs
@@ -1516,154 +1516,139 @@ fn test_link_account() {
})
}
-/// test change owner key
-#[test]
-fn test_change_owner_key_validator_keys_set() {
- ExtBuilder::new(1, 3, 4).build().execute_with(|| {
- let genesis_hash = System::block_hash(0);
- let bob = AccountKeyring::Bob.to_account_id();
- let ferdie = AccountKeyring::Ferdie.to_account_id();
- let payload = (b"icok", genesis_hash, 2u32, bob.clone()).encode();
- let signature = AccountKeyring::Ferdie.sign(&payload);
-
- // Bob is not an online validator, but his identity/owner_key is already associated.
- // He should not be able to change its owner_key.
- assert_ok!(AuthorityMembers::set_session_keys(
- RuntimeOrigin::signed(AccountKeyring::Bob.to_account_id()),
- create_dummy_session_keys()
- ));
- assert!(!pallet_authority_members::OnlineAuthorities::<Runtime>::get().contains(&2));
- assert!(pallet_authority_members::Members::<Runtime>::get(2).is_some());
-
- assert_noop!(
- Identity::change_owner_key(
- RuntimeOrigin::signed(bob.clone()),
- ferdie.clone(),
- signature.into()
- ),
- pallet_identity::Error::<gdev_runtime::Runtime>::OwnerKeyUsedAsValidator
- );
- })
-}
-
/// test change owner key
#[test]
fn test_change_owner_key_validator_online() {
- ExtBuilder::new(1, 3, 4).build().execute_with(|| {
- let genesis_hash = System::block_hash(0);
- let alice = AccountKeyring::Alice.to_account_id();
- let ferdie = AccountKeyring::Ferdie.to_account_id();
- let payload = (b"icok", genesis_hash, 1u32, alice.clone()).encode();
- let signature = AccountKeyring::Ferdie.sign(&payload);
+ ExtBuilder::new(1, 3, 4)
+ .with_initial_balances(vec![(AccountKeyring::Ferdie.to_account_id(), 8888)])
+ .build()
+ .execute_with(|| {
+ let genesis_hash = System::block_hash(0);
+ let alice = AccountKeyring::Alice.to_account_id();
+ let ferdie = AccountKeyring::Ferdie.to_account_id();
+ let payload = (b"icok", genesis_hash, 1u32, alice.clone()).encode();
+ let signature = AccountKeyring::Ferdie.sign(&payload);
- // Alice is an online validator
- assert!(pallet_authority_members::OnlineAuthorities::<Runtime>::get().contains(&1));
+ // Alice is an online validator
+ assert!(pallet_authority_members::OnlineAuthorities::<Runtime>::get().contains(&1));
+ assert_eq!(
+ pallet_authority_members::Members::<Runtime>::get(1)
+ .unwrap()
+ .owner_key,
+ alice
+ );
- // As an online validator she cannot change key
- assert_noop!(
- Identity::change_owner_key(
+ assert_ok!(Identity::change_owner_key(
RuntimeOrigin::signed(alice.clone()),
ferdie.clone(),
signature.into()
- ),
- pallet_identity::Error::<gdev_runtime::Runtime>::OwnerKeyUsedAsValidator
- );
- })
+ ),);
+
+ assert_eq!(
+ pallet_authority_members::Members::<Runtime>::get(1)
+ .unwrap()
+ .owner_key,
+ ferdie
+ );
+ })
}
/// test change owner key
#[test]
fn test_change_owner_key() {
- ExtBuilder::new(1, 3, 4).build().execute_with(|| {
- let genesis_hash = System::block_hash(0);
- let charlie = AccountKeyring::Charlie.to_account_id();
- let ferdie = AccountKeyring::Ferdie.to_account_id();
- let payload = (b"icok", genesis_hash, 3u32, charlie.clone()).encode();
- let signature = AccountKeyring::Ferdie.sign(&payload);
-
- SmithMembers::on_smith_goes_offline(3);
- // Charlie is now offline smith
- assert_eq!(
- SmithMembers::smiths(3),
- Some(SmithMeta {
- status: SmithStatus::Smith,
- expires_on: Some(48),
- issued_certs: vec![1, 2],
- received_certs: vec![1, 2]
- })
- );
+ ExtBuilder::new(1, 3, 4)
+ .with_initial_balances(vec![(AccountKeyring::Ferdie.to_account_id(), 8888)])
+ .build()
+ .execute_with(|| {
+ let genesis_hash = System::block_hash(0);
+ let charlie = AccountKeyring::Charlie.to_account_id();
+ let ferdie = AccountKeyring::Ferdie.to_account_id();
+ let payload = (b"icok", genesis_hash, 3u32, charlie.clone()).encode();
+ let signature = AccountKeyring::Ferdie.sign(&payload);
- assert_eq!(
- frame_system::Pallet::<Runtime>::get(&charlie).linked_idty,
- Some(3)
- );
- assert_eq!(
- frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty,
- None
- );
- // Dave can change his owner key to Ferdie's
- assert_ok!(Identity::change_owner_key(
- RuntimeOrigin::signed(charlie.clone()),
- ferdie.clone(),
- signature.into()
- ));
- assert_eq!(
- frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty,
- Some(3)
- );
+ SmithMembers::on_smith_goes_offline(3);
+ // Charlie is now offline smith
+ assert_eq!(
+ SmithMembers::smiths(3),
+ Some(SmithMeta {
+ status: SmithStatus::Smith,
+ expires_on: Some(48),
+ issued_certs: vec![1, 2],
+ received_certs: vec![1, 2]
+ })
+ );
- // Charlie is still an offline smith
- assert_eq!(
- SmithMembers::smiths(3),
- Some(SmithMeta {
- status: SmithStatus::Smith,
- expires_on: Some(48),
- issued_certs: vec![1, 2],
- received_certs: vec![1, 2]
- })
- );
+ assert_eq!(
+ frame_system::Pallet::<Runtime>::get(&charlie).linked_idty,
+ Some(3)
+ );
+ assert_eq!(
+ frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty,
+ None
+ );
+ // Dave can change his owner key to Ferdie's
+ assert_ok!(Identity::change_owner_key(
+ RuntimeOrigin::signed(charlie.clone()),
+ ferdie.clone(),
+ signature.into()
+ ));
+ assert_eq!(
+ frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty,
+ Some(3)
+ );
- // Ferdie can set its session_keys and go online
- frame_system::Pallet::<Runtime>::inc_providers(&ferdie);
- assert_ok!(AuthorityMembers::set_session_keys(
- RuntimeOrigin::signed(AccountKeyring::Ferdie.to_account_id()),
- create_dummy_session_keys()
- ));
- assert_ok!(AuthorityMembers::go_online(RuntimeOrigin::signed(
- AccountKeyring::Ferdie.to_account_id()
- )));
+ // Charlie is still an offline smith
+ assert_eq!(
+ SmithMembers::smiths(3),
+ Some(SmithMeta {
+ status: SmithStatus::Smith,
+ expires_on: Some(48),
+ issued_certs: vec![1, 2],
+ received_certs: vec![1, 2]
+ })
+ );
- // Charlie is still an offline smith
- assert_eq!(
- SmithMembers::smiths(3),
- Some(SmithMeta {
- status: SmithStatus::Smith,
- expires_on: Some(48),
- issued_certs: vec![1, 2],
- received_certs: vec![1, 2]
- })
- );
+ // Ferdie can set its session_keys and go online
+ frame_system::Pallet::<Runtime>::inc_providers(&ferdie);
+ assert_ok!(AuthorityMembers::set_session_keys(
+ RuntimeOrigin::signed(AccountKeyring::Ferdie.to_account_id()),
+ create_dummy_session_keys()
+ ));
+ assert_ok!(AuthorityMembers::go_online(RuntimeOrigin::signed(
+ AccountKeyring::Ferdie.to_account_id()
+ )));
- run_to_block(25);
+ // Charlie is still an offline smith
+ assert_eq!(
+ SmithMembers::smiths(3),
+ Some(SmithMeta {
+ status: SmithStatus::Smith,
+ expires_on: Some(48),
+ issued_certs: vec![1, 2],
+ received_certs: vec![1, 2]
+ })
+ );
- System::assert_has_event(RuntimeEvent::AuthorityMembers(
- pallet_authority_members::Event::IncomingAuthorities { members: vec![3] },
- ));
- System::assert_has_event(RuntimeEvent::AuthorityMembers(
- pallet_authority_members::Event::OutgoingAuthorities { members: vec![1] },
- ));
+ run_to_block(25);
- // "Charlie" (idty 3) is now online because its identity is mapped to Ferdies's key
- assert_eq!(
- SmithMembers::smiths(3),
- Some(SmithMeta {
- status: SmithStatus::Smith,
- expires_on: None,
- issued_certs: vec![1, 2],
- received_certs: vec![1, 2]
- })
- );
- })
+ System::assert_has_event(RuntimeEvent::AuthorityMembers(
+ pallet_authority_members::Event::IncomingAuthorities { members: vec![3] },
+ ));
+ System::assert_has_event(RuntimeEvent::AuthorityMembers(
+ pallet_authority_members::Event::OutgoingAuthorities { members: vec![1] },
+ ));
+
+ // "Charlie" (idty 3) is now online because its identity is mapped to Ferdies's key
+ assert_eq!(
+ SmithMembers::smiths(3),
+ Some(SmithMeta {
+ status: SmithStatus::Smith,
+ expires_on: None,
+ issued_certs: vec![1, 2],
+ received_certs: vec![1, 2]
+ })
+ );
+ })
}
/// members of the smith subwot can revoke their identity
--
GitLab
From 093a7258b24a3d59639f951a8042256915a79ad3 Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Fri, 21 Feb 2025 16:43:46 +0100
Subject: [PATCH 5/8] enforce bound period
---
pallets/identity/src/lib.rs | 4 +
pallets/smith-members/src/lib.rs | 19 ++-
pallets/smith-members/src/tests.rs | 53 +++++++--
pallets/smith-members/src/types.rs | 7 +-
runtime/common/src/handlers.rs | 40 ++++++-
runtime/common/src/pallets_config.rs | 2 +-
runtime/g1/src/parameters.rs | 2 +-
runtime/gdev/src/parameters.rs | 3 +-
runtime/gdev/tests/integration_tests.rs | 150 +++++++++++++++++++++---
runtime/gtest/src/parameters.rs | 2 +-
10 files changed, 236 insertions(+), 46 deletions(-)
diff --git a/pallets/identity/src/lib.rs b/pallets/identity/src/lib.rs
index 45244e4a..4a2c8087 100644
--- a/pallets/identity/src/lib.rs
+++ b/pallets/identity/src/lib.rs
@@ -788,6 +788,10 @@ pub mod pallet {
IdtyNotFound,
/// Invalid payload signature.
InvalidSignature,
+ /// Key used as validator.
+ OwnerKeyUsedAsValidator,
+ /// Key in bound period.
+ OwnerKeyInBound,
/// Invalid revocation key.
InvalidRevocationKey,
/// Issuer is not member and can not perform this action.
diff --git a/pallets/smith-members/src/lib.rs b/pallets/smith-members/src/lib.rs
index d9246b0c..66cab0d7 100644
--- a/pallets/smith-members/src/lib.rs
+++ b/pallets/smith-members/src/lib.rs
@@ -59,7 +59,10 @@ use frame_support::{
ensure,
pallet_prelude::{Get, RuntimeDebug, Weight},
};
-use frame_system::{ensure_signed, pallet_prelude::OriginFor};
+use frame_system::{
+ ensure_signed,
+ pallet_prelude::{BlockNumberFor, OriginFor},
+};
use scale_info::{
prelude::{collections::BTreeMap, fmt::Debug, vec, vec::Vec},
TypeInfo,
@@ -239,6 +242,7 @@ pub mod pallet {
},
issued_certs: vec![],
received_certs: issuers_,
+ last_online: None,
},
);
// if smith is offline, schedule expire
@@ -264,8 +268,13 @@ pub mod pallet {
/// The Smith metadata for each identity.
#[pallet::storage]
#[pallet::getter(fn smiths)]
- pub type Smiths<T: Config> =
- StorageMap<_, Twox64Concat, T::IdtyIndex, SmithMeta<T::IdtyIndex>, OptionQuery>;
+ pub type Smiths<T: Config> = StorageMap<
+ _,
+ Twox64Concat,
+ T::IdtyIndex,
+ SmithMeta<T::IdtyIndex, BlockNumberFor<T>>,
+ OptionQuery,
+ >;
/// The indexes of Smith to remove at a given session.
#[pallet::storage]
@@ -586,7 +595,7 @@ impl<T: Config> Pallet<T> {
if let Some(smith_meta) = maybe_smith_meta {
// As long as the smith is online, it cannot expire
smith_meta.expires_on = None;
- // FIXME: unschedule old expiry (#182)
+ smith_meta.last_online = None;
}
});
}
@@ -605,6 +614,8 @@ impl<T: Config> Pallet<T> {
let new_expires_on =
CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get();
smith_meta.expires_on = Some(new_expires_on);
+ let block_number = frame_system::pallet::Pallet::<T>::block_number();
+ smith_meta.last_online = Some(block_number);
ExpiresOn::<T>::append(new_expires_on, idty_index);
}
});
diff --git a/pallets/smith-members/src/tests.rs b/pallets/smith-members/src/tests.rs
index c92bffb6..068821a4 100644
--- a/pallets/smith-members/src/tests.rs
+++ b/pallets/smith-members/src/tests.rs
@@ -62,6 +62,7 @@ fn process_to_become_a_smith_and_lose_it() {
expires_on: Some(5),
issued_certs: vec![],
received_certs: vec![],
+ last_online: None,
}
);
// Then certification 1/2
@@ -80,6 +81,7 @@ fn process_to_become_a_smith_and_lose_it() {
expires_on: Some(5),
issued_certs: vec![],
received_certs: vec![1],
+ last_online: None,
}
);
// Then certification 2/2
@@ -101,6 +103,7 @@ fn process_to_become_a_smith_and_lose_it() {
expires_on: Some(5),
issued_certs: vec![],
received_certs: vec![1, 2],
+ last_online: None,
}
);
// Go online to be able to invite+certify
@@ -161,7 +164,8 @@ fn process_to_become_a_smith_and_lose_it() {
status: SmithStatus::Excluded,
expires_on: None,
issued_certs: vec![],
- received_certs: vec![]
+ received_certs: vec![],
+ last_online: Some(1),
})
);
assert_eq!(
@@ -170,7 +174,8 @@ fn process_to_become_a_smith_and_lose_it() {
status: SmithStatus::Excluded,
expires_on: None,
issued_certs: vec![],
- received_certs: vec![]
+ received_certs: vec![],
+ last_online: Some(1),
})
);
assert_eq!(
@@ -179,7 +184,8 @@ fn process_to_become_a_smith_and_lose_it() {
status: SmithStatus::Excluded,
expires_on: None,
issued_certs: vec![],
- received_certs: vec![]
+ received_certs: vec![],
+ last_online: None,
})
);
});
@@ -252,6 +258,7 @@ fn should_have_checks_on_certify() {
expires_on: None,
issued_certs: vec![4],
received_certs: vec![2, 3, 4],
+ last_online: None,
}
);
assert_eq!(
@@ -261,6 +268,7 @@ fn should_have_checks_on_certify() {
expires_on: Some(5),
issued_certs: vec![1, 4],
received_certs: vec![3, 4],
+ last_online: None,
}
);
assert_eq!(
@@ -270,6 +278,7 @@ fn should_have_checks_on_certify() {
expires_on: Some(5),
issued_certs: vec![1, 2],
received_certs: vec![4],
+ last_online: None,
}
);
assert_eq!(
@@ -279,6 +288,7 @@ fn should_have_checks_on_certify() {
expires_on: Some(5),
issued_certs: vec![1, 2, 3],
received_certs: vec![1, 2],
+ last_online: None,
}
);
@@ -312,6 +322,7 @@ fn should_have_checks_on_certify() {
expires_on: Some(5),
issued_certs: vec![1, 2],
received_certs: vec![4],
+ last_online: None,
}
);
// Try to certify #3
@@ -327,6 +338,7 @@ fn should_have_checks_on_certify() {
expires_on: Some(5),
issued_certs: vec![1, 2],
received_certs: vec![1, 4],
+ last_online: None,
}
);
});
@@ -359,7 +371,8 @@ fn smith_activity_postpones_expiration() {
status: SmithStatus::Excluded,
expires_on: None,
issued_certs: vec![],
- received_certs: vec![]
+ received_certs: vec![],
+ last_online: None,
})
);
// issued_certs is empty because #1 was excluded
@@ -370,6 +383,7 @@ fn smith_activity_postpones_expiration() {
expires_on: None,
issued_certs: vec![],
received_certs: vec![3, 4],
+ last_online: None,
})
);
@@ -383,6 +397,7 @@ fn smith_activity_postpones_expiration() {
expires_on: Some(11),
issued_certs: vec![],
received_certs: vec![3, 4],
+ last_online: Some(0),
})
);
// Still not expired on session 10
@@ -394,6 +409,7 @@ fn smith_activity_postpones_expiration() {
expires_on: Some(11),
issued_certs: vec![],
received_certs: vec![3, 4],
+ last_online: Some(0),
})
);
// But expired on session 11
@@ -404,7 +420,8 @@ fn smith_activity_postpones_expiration() {
status: SmithStatus::Excluded,
expires_on: None,
issued_certs: vec![],
- received_certs: vec![]
+ received_certs: vec![],
+ last_online: None,
})
);
assert_eq!(
@@ -413,7 +430,8 @@ fn smith_activity_postpones_expiration() {
status: SmithStatus::Excluded,
expires_on: None,
issued_certs: vec![],
- received_certs: vec![]
+ received_certs: vec![],
+ last_online: Some(0),
})
);
});
@@ -443,7 +461,8 @@ fn smith_coming_back_recovers_its_issued_certs() {
status: Excluded,
expires_on: None,
issued_certs: vec![1],
- received_certs: vec![]
+ received_certs: vec![],
+ last_online: None,
})
);
// Smith #2 comes back
@@ -466,7 +485,8 @@ fn smith_coming_back_recovers_its_issued_certs() {
status: Smith,
expires_on: Some(10),
issued_certs: vec![1],
- received_certs: vec![1, 3]
+ received_certs: vec![1, 3],
+ last_online: None,
})
);
Pallet::<Runtime>::on_smith_goes_online(2);
@@ -590,7 +610,8 @@ fn certifying_an_online_smith() {
status: Smith,
expires_on: Some(8),
issued_certs: vec![],
- received_certs: vec![1, 2]
+ received_certs: vec![1, 2],
+ last_online: None,
})
);
assert_eq!(ExpiresOn::<Runtime>::get(7), Some(vec![5]));
@@ -604,7 +625,8 @@ fn certifying_an_online_smith() {
status: Smith,
expires_on: None,
issued_certs: vec![],
- received_certs: vec![1, 2]
+ received_certs: vec![1, 2],
+ last_online: None,
})
);
// ExpiresOn is not unscheduled, but as expires_on has switched to None it's not a problem
@@ -622,7 +644,8 @@ fn certifying_an_online_smith() {
status: Smith,
expires_on: None,
issued_certs: vec![],
- received_certs: vec![1, 2, 3]
+ received_certs: vec![1, 2, 3],
+ last_online: None,
})
);
});
@@ -648,7 +671,8 @@ fn expires_on_cleans_up() {
status: Smith,
expires_on: Some(5),
issued_certs: vec![2, 3],
- received_certs: vec![2, 3]
+ received_certs: vec![2, 3],
+ last_online: Some(0),
})
);
// It is also present in ExpiresOn schedule
@@ -662,7 +686,8 @@ fn expires_on_cleans_up() {
status: Excluded,
expires_on: None,
issued_certs: vec![2, 3],
- received_certs: vec![]
+ received_certs: vec![],
+ last_online: Some(0),
})
);
// ExpiresOn is clean
@@ -713,6 +738,7 @@ fn losing_wot_membership_cascades_to_smith_members() {
expires_on: Some(5),
issued_certs: vec![3],
received_certs: vec![2, 3, 4],
+ last_online: None,
})
);
assert_eq!(
@@ -742,6 +768,7 @@ fn losing_wot_membership_cascades_to_smith_members() {
expires_on: None,
issued_certs: vec![3],
received_certs: vec![],
+ last_online: None,
})
);
// Issued certifications updated for certifiers of 1
diff --git a/pallets/smith-members/src/types.rs b/pallets/smith-members/src/types.rs
index 83992265..396e0334 100644
--- a/pallets/smith-members/src/types.rs
+++ b/pallets/smith-members/src/types.rs
@@ -24,7 +24,7 @@ use sp_staking::SessionIndex;
/// Represents a certification metadata attached to a Smith identity.
#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)]
-pub struct SmithMeta<IdtyIndex> {
+pub struct SmithMeta<IdtyIndex, BlockNumber> {
/// Current status of the Smith.
pub status: SmithStatus,
/// The session at which the Smith will expire (for lack of validation activity).
@@ -33,16 +33,19 @@ pub struct SmithMeta<IdtyIndex> {
pub issued_certs: Vec<IdtyIndex>,
/// Certifications received from other Smiths.
pub received_certs: Vec<IdtyIndex>,
+ /// Last online time.
+ pub last_online: Option<BlockNumber>,
}
/// By default, a smith has the least possible privileges
-impl<IdtyIndex> Default for SmithMeta<IdtyIndex> {
+impl<IdtyIndex, BlockNumber> Default for SmithMeta<IdtyIndex, BlockNumber> {
fn default() -> Self {
Self {
status: SmithStatus::Excluded,
expires_on: None,
issued_certs: Vec::<IdtyIndex>::new(),
received_certs: Vec::<IdtyIndex>::new(),
+ last_online: Default::default(),
}
}
}
diff --git a/runtime/common/src/handlers.rs b/runtime/common/src/handlers.rs
index ba0d0f29..66ac960d 100644
--- a/runtime/common/src/handlers.rs
+++ b/runtime/common/src/handlers.rs
@@ -19,6 +19,7 @@ use frame_support::{
pallet_prelude::Weight,
traits::{Imbalance, UnfilteredDispatchable},
};
+use frame_system::pallet_prelude::BlockNumberFor;
use pallet_smith_members::SmithRemovalReason;
use sp_core::Get;
@@ -171,19 +172,48 @@ where
}
/// Runtime handler OwnerKeyChangePermission.
-pub struct KeyChangeHandler<Runtime>(core::marker::PhantomData<Runtime>);
+pub struct KeyChangeHandler<Runtime, ReportLongevity>(
+ core::marker::PhantomData<Runtime>,
+ core::marker::PhantomData<ReportLongevity>,
+);
impl<
Runtime: frame_system::Config<AccountId = AccountId>
+ pallet_identity::Config<IdtyIndex = IdtyIndex>
- + pallet_authority_members::Config<MemberId = IdtyIndex>,
- > pallet_identity::traits::KeyChange<Runtime> for KeyChangeHandler<Runtime>
+ + pallet_authority_members::Config<MemberId = IdtyIndex>
+ + pallet_smith_members::Config<IdtyIndex = IdtyIndex>,
+ ReportLongevity: Get<BlockNumberFor<Runtime>>,
+ > pallet_identity::traits::KeyChange<Runtime> for KeyChangeHandler<Runtime, ReportLongevity>
{
+ /// Handles the event when an identity's owner key is changed.
+ ///
+ /// # Errors
+ /// * Returns `OwnerKeyInBound` if the smith was a validator and the bond period is not finished, meaning it can still be punished for past actions.
+ /// * Returns `OwnerKeyUsedAsValidator` if the owner key is currently used as a validator.
+ ///
+ /// # Behavior
+ /// * If the smith is online, the operation is rejected.
+ /// * If the smith was a validator and is still within the bond period, the operation is rejected. It means they can still be punished for past actions.
+ /// * If the smith is neither online nor within the bond period, the owner key is changed successfully and the change is reflected in the validator member data if available.
fn on_changed(
idty_index: IdtyIndex,
account_id: AccountId,
) -> Result<(), sp_runtime::DispatchError> {
- pallet_authority_members::Pallet::<Runtime>::change_owner_key(idty_index, account_id)
- .map_err(|e| e.error)?;
+ if let Some(smith) = pallet_smith_members::Pallet::<Runtime>::smiths(&idty_index) {
+ if let Some(last_online) = smith.last_online {
+ if last_online + ReportLongevity::get()
+ > frame_system::pallet::Pallet::<Runtime>::block_number()
+ {
+ return Err(pallet_identity::Error::<Runtime>::OwnerKeyInBound.into());
+ } else {
+ pallet_authority_members::Pallet::<Runtime>::change_owner_key(
+ idty_index, account_id,
+ )
+ .map_err(|e| e.error)?;
+ }
+ } else {
+ return Err(pallet_identity::Error::<Runtime>::OwnerKeyUsedAsValidator.into());
+ }
+ }
Ok(())
}
}
diff --git a/runtime/common/src/pallets_config.rs b/runtime/common/src/pallets_config.rs
index af8bdf59..8cc6b927 100644
--- a/runtime/common/src/pallets_config.rs
+++ b/runtime/common/src/pallets_config.rs
@@ -444,7 +444,7 @@ macro_rules! pallets_config {
type IdtyData = IdtyData;
type IdtyIndex = IdtyIndex;
type IdtyNameValidator = IdtyNameValidatorImpl;
- type OnKeyChange = KeyChangeHandler<Runtime>;
+ type OnKeyChange = KeyChangeHandler<Runtime, ReportLongevity>;
type OnNewIdty = OnNewIdtyHandler<Runtime>;
type OnRemoveIdty = OnRemoveIdtyHandler<Runtime>;
type RuntimeEvent = RuntimeEvent;
diff --git a/runtime/g1/src/parameters.rs b/runtime/g1/src/parameters.rs
index d787eb5b..5fe8b963 100644
--- a/runtime/g1/src/parameters.rs
+++ b/runtime/g1/src/parameters.rs
@@ -58,7 +58,7 @@ pub const EPOCH_DURATION_IN_SLOTS: BlockNumber = 4 * HOURS;
parameter_types! {
pub const EpochDuration: u64 = EPOCH_DURATION_IN_SLOTS as u64;
pub const ExpectedBlockTime: u64 = MILLISECS_PER_BLOCK;
- pub const ReportLongevity: u64 = 168 * EpochDuration::get();
+ pub const ReportLongevity: BlockNumber = 168 * EPOCH_DURATION_IN_SLOTS;
}
// ImOnline
diff --git a/runtime/gdev/src/parameters.rs b/runtime/gdev/src/parameters.rs
index bcca8e7d..7fc7e9ba 100644
--- a/runtime/gdev/src/parameters.rs
+++ b/runtime/gdev/src/parameters.rs
@@ -58,9 +58,8 @@ parameter_types! {
// Babe
parameter_types! {
pub const ExpectedBlockTime: u64 = MILLISECS_PER_BLOCK;
- pub const ReportLongevity: u64 = 168 * HOURS as u64;
+ pub const ReportLongevity: BlockNumber = 168 * HOURS;
}
-
// ImOnline
parameter_types! {
pub const ImOnlineUnsignedPriority: TransactionPriority = TransactionPriority::MAX;
diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs
index 32dd489f..bc3ccd87 100644
--- a/runtime/gdev/tests/integration_tests.rs
+++ b/runtime/gdev/tests/integration_tests.rs
@@ -1020,6 +1020,7 @@ fn test_smith_process() {
expires_on: Some(48),
issued_certs: vec![],
received_certs: vec![1, 2, 3],
+ last_online: None,
})
);
@@ -1053,6 +1054,7 @@ fn test_expired_smith_has_null_expires_on() {
expires_on: None, // because online
issued_certs: vec![1, 3],
received_certs: vec![1, 3],
+ last_online: None,
})
);
@@ -1085,6 +1087,7 @@ fn test_expired_smith_has_null_expires_on() {
expires_on: None, // because excluded, no expiry is scheduled
issued_certs: vec![1, 3],
received_certs: vec![], // received certs are deleted
+ last_online: None,
})
);
// Alice smith cert to Bob has been deleted
@@ -1095,6 +1098,7 @@ fn test_expired_smith_has_null_expires_on() {
expires_on: None, // because online
issued_certs: vec![3], // cert to Bob has been deleted
received_certs: vec![2, 3],
+ last_online: None,
})
);
@@ -1116,6 +1120,7 @@ fn test_expired_smith_has_null_expires_on() {
expires_on: None, // should be still None
issued_certs: vec![1, 3],
received_certs: vec![],
+ last_online: None,
})
);
@@ -1516,7 +1521,7 @@ fn test_link_account() {
})
}
-/// test change owner key
+/// test change owner key was validator is online
#[test]
fn test_change_owner_key_validator_online() {
ExtBuilder::new(1, 3, 4)
@@ -1538,23 +1543,104 @@ fn test_change_owner_key_validator_online() {
alice
);
+ // As an online validator she cannot change key
+ assert_noop!(
+ Identity::change_owner_key(
+ RuntimeOrigin::signed(alice.clone()),
+ ferdie.clone(),
+ signature.into()
+ ),
+ pallet_identity::Error::<gdev_runtime::Runtime>::OwnerKeyUsedAsValidator
+ );
+ })
+}
+
+/// test change owner key between set_key and go online
+#[test]
+#[ignore = "long to go to ReportLongevity"]
+fn test_change_owner_key_offline() {
+ ExtBuilder::new(1, 3, 4)
+ .with_initial_balances(vec![(AccountKeyring::Ferdie.to_account_id(), 8888)])
+ .build()
+ .execute_with(|| {
+ let genesis_hash = System::block_hash(0);
+ let charlie = AccountKeyring::Charlie.to_account_id();
+ let ferdie = AccountKeyring::Ferdie.to_account_id();
+ let payload = (b"icok", genesis_hash, 3u32, charlie.clone()).encode();
+ let signature = AccountKeyring::Ferdie.sign(&payload);
+
+ // Charlie is an offline smith
+ SmithMembers::on_smith_goes_offline(3);
+ assert_eq!(
+ SmithMembers::smiths(3),
+ Some(SmithMeta {
+ status: SmithStatus::Smith,
+ expires_on: Some(48),
+ issued_certs: vec![1, 2],
+ received_certs: vec![1, 2],
+ last_online: Some(1),
+ })
+ );
+ assert_eq!(
+ frame_system::Pallet::<Runtime>::get(&charlie).linked_idty,
+ Some(3)
+ );
+ assert_eq!(
+ frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty,
+ None
+ );
+
+ // We run after the bound period
+ // Keeping members intact
+ for i in 1..4 {
+ let expiration = pallet_membership::Membership::<Runtime>::get(i)
+ .unwrap()
+ .expire_on;
+ pallet_membership::MembershipsExpireOn::<Runtime>::take(expiration);
+ pallet_smith_members::Smiths::<Runtime>::mutate(i, |data| {
+ if let Some(ref mut data) = data {
+ data.expires_on = None;
+ }
+ });
+ }
+ run_to_block(ReportLongevity::get() + 1);
+
+ // Charlie can set its session_keys
+ assert_ok!(AuthorityMembers::set_session_keys(
+ RuntimeOrigin::signed(AccountKeyring::Charlie.to_account_id()),
+ create_dummy_session_keys()
+ ));
+ assert_eq!(
+ pallet_authority_members::Members::<Runtime>::get(3)
+ .unwrap()
+ .owner_key,
+ charlie.clone()
+ );
+
+ // Charlie can change his owner key to Ferdie's a valid account
+ // with providers and balance
assert_ok!(Identity::change_owner_key(
- RuntimeOrigin::signed(alice.clone()),
+ RuntimeOrigin::signed(charlie.clone()),
ferdie.clone(),
signature.into()
- ),);
-
+ ));
+ // The change is reflected on the authority member data
assert_eq!(
- pallet_authority_members::Members::<Runtime>::get(1)
+ pallet_authority_members::Members::<Runtime>::get(3)
.unwrap()
.owner_key,
- ferdie
+ ferdie.clone()
+ );
+ assert_eq!(
+ frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty,
+ Some(3)
);
})
}
/// test change owner key
#[test]
+#[ignore = "long to go to ReportLongevity"]
fn test_change_owner_key() {
ExtBuilder::new(1, 3, 4)
.with_initial_balances(vec![(AccountKeyring::Ferdie.to_account_id(), 8888)])
@@ -1574,7 +1660,8 @@ fn test_change_owner_key() {
status: SmithStatus::Smith,
expires_on: Some(48),
issued_certs: vec![1, 2],
- received_certs: vec![1, 2]
+ received_certs: vec![1, 2],
+ last_online: Some(1),
})
);
@@ -1586,7 +1673,36 @@ fn test_change_owner_key() {
frame_system::Pallet::<Runtime>::get(&ferdie).linked_idty,
None
);
- // Dave can change his owner key to Ferdie's
+
+ run_to_block(5);
+
+ // Charlie cannot change his owner key because he is in bound period
+ // and can be punished for past actions
+ assert_noop!(
+ Identity::change_owner_key(
+ RuntimeOrigin::signed(charlie.clone()),
+ ferdie.clone(),
+ signature.into()
+ ),
+ pallet_identity::Error::<gdev_runtime::Runtime>::OwnerKeyInBound
+ );
+
+ // We run after the bound period
+ // Keeping members intact
+ for i in 1..4 {
+ let expiration = pallet_membership::Membership::<Runtime>::get(i)
+ .unwrap()
+ .expire_on;
+ pallet_membership::MembershipsExpireOn::<Runtime>::take(expiration);
+ pallet_smith_members::Smiths::<Runtime>::mutate(i, |data| {
+ if let Some(ref mut data) = data {
+ data.expires_on = Some(ReportLongevity::get() * 2);
+ }
+ });
+ }
+ run_to_block(ReportLongevity::get() + 1);
+
+ // Charlie can change his owner key to Ferdie's
assert_ok!(Identity::change_owner_key(
RuntimeOrigin::signed(charlie.clone()),
ferdie.clone(),
@@ -1602,9 +1718,10 @@ fn test_change_owner_key() {
SmithMembers::smiths(3),
Some(SmithMeta {
status: SmithStatus::Smith,
- expires_on: Some(48),
+ expires_on: Some(ReportLongevity::get() * 2),
issued_certs: vec![1, 2],
- received_certs: vec![1, 2]
+ received_certs: vec![1, 2],
+ last_online: Some(1),
})
);
@@ -1623,20 +1740,18 @@ fn test_change_owner_key() {
SmithMembers::smiths(3),
Some(SmithMeta {
status: SmithStatus::Smith,
- expires_on: Some(48),
+ expires_on: Some(ReportLongevity::get() * 2),
issued_certs: vec![1, 2],
- received_certs: vec![1, 2]
+ received_certs: vec![1, 2],
+ last_online: Some(1),
})
);
- run_to_block(25);
+ run_to_block(((ReportLongevity::get() + 1 + 24) / 25) * 25);
System::assert_has_event(RuntimeEvent::AuthorityMembers(
pallet_authority_members::Event::IncomingAuthorities { members: vec![3] },
));
- System::assert_has_event(RuntimeEvent::AuthorityMembers(
- pallet_authority_members::Event::OutgoingAuthorities { members: vec![1] },
- ));
// "Charlie" (idty 3) is now online because its identity is mapped to Ferdies's key
assert_eq!(
@@ -1645,7 +1760,8 @@ fn test_change_owner_key() {
status: SmithStatus::Smith,
expires_on: None,
issued_certs: vec![1, 2],
- received_certs: vec![1, 2]
+ received_certs: vec![1, 2],
+ last_online: None,
})
);
})
diff --git a/runtime/gtest/src/parameters.rs b/runtime/gtest/src/parameters.rs
index 307f59b1..8ac8aeb0 100644
--- a/runtime/gtest/src/parameters.rs
+++ b/runtime/gtest/src/parameters.rs
@@ -58,7 +58,7 @@ pub const EPOCH_DURATION_IN_SLOTS: BlockNumber = HOURS;
parameter_types! {
pub const EpochDuration: u64 = EPOCH_DURATION_IN_SLOTS as u64;
pub const ExpectedBlockTime: u64 = MILLISECS_PER_BLOCK;
- pub const ReportLongevity: u64 = 168 * EpochDuration::get();
+ pub const ReportLongevity: BlockNumber = 168 * EPOCH_DURATION_IN_SLOTS;
}
// ImOnline
--
GitLab
From f1c6e12bc37a4a5f4562fbea32d125f80f9b5f1a Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Mon, 24 Feb 2025 15:59:01 +0100
Subject: [PATCH 6/8] refactore double queries
---
pallets/smith-members/src/lib.rs | 53 ++++++++++++++------------------
1 file changed, 23 insertions(+), 30 deletions(-)
diff --git a/pallets/smith-members/src/lib.rs b/pallets/smith-members/src/lib.rs
index 66cab0d7..25e44c05 100644
--- a/pallets/smith-members/src/lib.rs
+++ b/pallets/smith-members/src/lib.rs
@@ -589,46 +589,39 @@ impl<T: Config> Pallet<T> {
/// Handle the event when a Smith goes online.
pub fn on_smith_goes_online(idty_index: T::IdtyIndex) {
- if let Some(smith_meta) = Smiths::<T>::get(idty_index) {
- if smith_meta.expires_on.is_some() {
- Smiths::<T>::mutate(idty_index, |maybe_smith_meta| {
- if let Some(smith_meta) = maybe_smith_meta {
- // As long as the smith is online, it cannot expire
- smith_meta.expires_on = None;
- smith_meta.last_online = None;
- }
- });
+ Smiths::<T>::mutate(idty_index, |maybe_smith_meta| {
+ if let Some(smith_meta) = maybe_smith_meta {
+ if smith_meta.expires_on.is_some() {
+ // As long as the smith is online, it cannot expire
+ smith_meta.expires_on = None;
+ smith_meta.last_online = None;
+ }
}
- }
+ });
}
/// Handle the event when a Smith goes offline.
pub fn on_smith_goes_offline(idty_index: T::IdtyIndex) {
- if let Some(smith_meta) = Smiths::<T>::get(idty_index) {
- // Smith can go offline after main membership expiry
- // in this case, there is no scheduled expiry since it is already excluded
- if smith_meta.status != SmithStatus::Excluded {
- Smiths::<T>::mutate(idty_index, |maybe_smith_meta| {
- if let Some(smith_meta) = maybe_smith_meta {
- // schedule expiry
- let new_expires_on =
- CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get();
- smith_meta.expires_on = Some(new_expires_on);
- let block_number = frame_system::pallet::Pallet::<T>::block_number();
- smith_meta.last_online = Some(block_number);
- ExpiresOn::<T>::append(new_expires_on, idty_index);
- }
- });
+ Smiths::<T>::mutate(idty_index, |maybe_smith_meta| {
+ if let Some(smith_meta) = maybe_smith_meta {
+ // Smith can go offline after main membership expiry
+ // in this case, there is no scheduled expiry since it is already excluded
+ if smith_meta.status != SmithStatus::Excluded {
+ // schedule expiry
+ let new_expires_on =
+ CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get();
+ smith_meta.expires_on = Some(new_expires_on);
+ let block_number = frame_system::pallet::Pallet::<T>::block_number();
+ smith_meta.last_online = Some(block_number);
+ ExpiresOn::<T>::append(new_expires_on, idty_index);
+ }
}
- }
+ });
}
/// Provide whether the given identity index is a Smith.
fn provide_is_member(idty_id: &T::IdtyIndex) -> bool {
- let Some(smith) = Smiths::<T>::get(idty_id) else {
- return false;
- };
- smith.status == SmithStatus::Smith
+ Smiths::<T>::get(idty_id).map_or(false, |smith| smith.status == SmithStatus::Smith)
}
}
--
GitLab
From a1085864d637dc7e642da16bae446691eecc8310 Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Tue, 25 Feb 2025 09:59:14 +0100
Subject: [PATCH 7/8] regenerate metadata
---
resources/metadata.scale | Bin 152736 -> 152810 bytes
1 file changed, 0 insertions(+), 0 deletions(-)
diff --git a/resources/metadata.scale b/resources/metadata.scale
index 430bae89f08fbcf6bcf656bfc6f26516d767119b..566b876a228174b3294280f96249b9b01095bd10 100644
GIT binary patch
delta 316
zcmZ3mlJnI{&W0_FUNN#9DxN7Nm7aMisTGU@I!-zH$=QCTxk;%-i~<ZI(<5UT>y#LE
zauSP6;`8%zGV@Yp7=8Q;N;32FY>?D$zZ%21OO7#Q`-EynH^%xH|MI-lBJb47(Bjk-
z$KtTWoXnKOlKdhDc9saBs6r`FSRt`kp$sOWX9LsWndg*WnwP@B!4d#cnVF}M1mY_c
zq!wl7r|1QE=7BW@rIzI<CxV;-RK>u_5(5=iC<01M|6Ri<!^kyVqLwj~k$ZYxEu#-3
z&-C53j8=@i)4$a+nltiE*Q;Z+VC0{kUdO1-C@}qQ9izeY!*z_3jDjGxBcsrC`Fchh
ZM&ao(^^CgHPuDZbGm30~UC+4n3IIcCYO4SM
delta 222
zcmaF0l5@dI&W0_FUNQVEDxN7Nm7aMisTGU@3<A^BV;JjL7?v_~Oizeq6y2^A%Q#<-
zF<|?lYDPE4=}EPWVvOw5D{C3G7&)ddsbvgh<edJimeGfiYr1<KqZK3f^rkvSb4H%&
zyXqJ%7<s3Eu4B|@<eM&E&*;g>526gF&#Y$@WE7abx}MR7QE>W;dPZHx82|FT)FSWH
z%FyD}6vyJQ#GK5O#FG3X1|gOiU@3*{)Jlcq(xRf&ypo(sg;Jn0g~VcoGMK{YA`OfR
NjKbSZ8yL4<0RWDAM#2C9
--
GitLab
From 0b78960c5040d2c41efb6668fc8756de2346cfa2 Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Fri, 21 Mar 2025 16:27:07 +0100
Subject: [PATCH 8/8] add reviews
---
runtime/common/src/handlers.rs | 6 ++----
runtime/gdev/tests/integration_tests.rs | 12 +++++++++---
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/runtime/common/src/handlers.rs b/runtime/common/src/handlers.rs
index 66ac960d..0029cb4e 100644
--- a/runtime/common/src/handlers.rs
+++ b/runtime/common/src/handlers.rs
@@ -173,8 +173,7 @@ where
/// Runtime handler OwnerKeyChangePermission.
pub struct KeyChangeHandler<Runtime, ReportLongevity>(
- core::marker::PhantomData<Runtime>,
- core::marker::PhantomData<ReportLongevity>,
+ core::marker::PhantomData<(Runtime, ReportLongevity)>,
);
impl<
Runtime: frame_system::Config<AccountId = AccountId>
@@ -220,8 +219,7 @@ impl<
/// Runtime handler for managing fee handling by transferring unbalanced amounts to a treasury account.
pub struct HandleFees<TreasuryAccount, Balances>(
- frame_support::pallet_prelude::PhantomData<TreasuryAccount>,
- frame_support::pallet_prelude::PhantomData<Balances>,
+ frame_support::pallet_prelude::PhantomData<(TreasuryAccount, Balances)>,
);
type CreditOf<Balances> = frame_support::traits::tokens::fungible::Credit<AccountId, Balances>;
impl<TreasuryAccount, Balances> frame_support::traits::OnUnbalanced<CreditOf<Balances>>
diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs
index bc3ccd87..4031f377 100644
--- a/runtime/gdev/tests/integration_tests.rs
+++ b/runtime/gdev/tests/integration_tests.rs
@@ -1603,6 +1603,7 @@ fn test_change_owner_key_offline() {
}
});
}
+ pallet_certification::CertsRemovableOn::<Runtime>::take(ValidityPeriod::get());
run_to_block(ReportLongevity::get() + 1);
// Charlie can set its session_keys
@@ -1689,6 +1690,7 @@ fn test_change_owner_key() {
// We run after the bound period
// Keeping members intact
+ let smith_expire_on = ReportLongevity::get() * 2;
for i in 1..4 {
let expiration = pallet_membership::Membership::<Runtime>::get(i)
.unwrap()
@@ -1696,10 +1698,14 @@ fn test_change_owner_key() {
pallet_membership::MembershipsExpireOn::<Runtime>::take(expiration);
pallet_smith_members::Smiths::<Runtime>::mutate(i, |data| {
if let Some(ref mut data) = data {
- data.expires_on = Some(ReportLongevity::get() * 2);
+ data.expires_on = Some(smith_expire_on);
}
});
}
+ pallet_certification::CertsRemovableOn::<Runtime>::take(ValidityPeriod::get());
+ pallet_smith_members::ExpiresOn::<Runtime>::take(SmithInactivityMaxDuration::get() + 1);
+ run_to_block(SmithInactivityMaxDuration::get() * 25);
+ pallet_smith_members::ExpiresOn::<Runtime>::take(SmithInactivityMaxDuration::get() + 2);
run_to_block(ReportLongevity::get() + 1);
// Charlie can change his owner key to Ferdie's
@@ -1718,7 +1724,7 @@ fn test_change_owner_key() {
SmithMembers::smiths(3),
Some(SmithMeta {
status: SmithStatus::Smith,
- expires_on: Some(ReportLongevity::get() * 2),
+ expires_on: Some(smith_expire_on),
issued_certs: vec![1, 2],
received_certs: vec![1, 2],
last_online: Some(1),
@@ -1740,7 +1746,7 @@ fn test_change_owner_key() {
SmithMembers::smiths(3),
Some(SmithMeta {
status: SmithStatus::Smith,
- expires_on: Some(ReportLongevity::get() * 2),
+ expires_on: Some(smith_expire_on),
issued_certs: vec![1, 2],
received_certs: vec![1, 2],
last_online: Some(1),
--
GitLab