From d6706d9d82251c431b0213da3bf80770cd3c9d51 Mon Sep 17 00:00:00 2001
From: Hugo Trentesaux <hugo@trentesaux.fr>
Date: Wed, 8 Jan 2025 14:57:48 +0100
Subject: [PATCH 1/5] deduplicate authorities events
---
pallets/authority-members/src/lib.rs | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/pallets/authority-members/src/lib.rs b/pallets/authority-members/src/lib.rs
index 587e82aa3..cb02e5b92 100644
--- a/pallets/authority-members/src/lib.rs
+++ b/pallets/authority-members/src/lib.rs
@@ -575,18 +575,25 @@ impl<T: Config> pallet_session::SessionManager<T::ValidatorId> for Pallet<T> {
return None;
}
+ // -- handle incoming members
+ // callback when smith is incoming
for member_id in members_ids_to_add.iter() {
T::OnIncomingMember::on_incoming_member(*member_id);
- Self::deposit_event(Event::IncomingAuthorities {
- members: members_ids_to_add.clone(),
- });
}
+ // a single event with all authorities
+ Self::deposit_event(Event::IncomingAuthorities {
+ members: members_ids_to_add.clone(),
+ });
+
+ // -- handle outgoing members
+ // callback when smith is outgoing
for member_id in members_ids_to_del.iter() {
T::OnOutgoingMember::on_outgoing_member(*member_id);
- Self::deposit_event(Event::OutgoingAuthorities {
- members: members_ids_to_del.clone(),
- });
}
+ // a single event with all authorities
+ Self::deposit_event(Event::OutgoingAuthorities {
+ members: members_ids_to_del.clone(),
+ });
// updates the list of OnlineAuthorities and returns the list of their key
Some(
--
GitLab
From f36c580f7583d387f4874abb82650e075bfe90af Mon Sep 17 00:00:00 2001
From: Hugo Trentesaux <hugo@trentesaux.fr>
Date: Wed, 8 Jan 2025 16:00:33 +0100
Subject: [PATCH 2/5] add std for sp keyring
---
runtime/gdev/Cargo.toml | 1 +
1 file changed, 1 insertion(+)
diff --git a/runtime/gdev/Cargo.toml b/runtime/gdev/Cargo.toml
index 3299b812d..08fc4435a 100644
--- a/runtime/gdev/Cargo.toml
+++ b/runtime/gdev/Cargo.toml
@@ -112,6 +112,7 @@ std = [
"sp-genesis-builder/std",
"sp-inherents/std",
"sp-io/std",
+ "sp-keyring/std",
"sp-membership/std",
"sp-offchain/std",
"sp-runtime/std",
--
GitLab
From a46e1971af667e77ac6d346d55ab86c8375a2bc9 Mon Sep 17 00:00:00 2001
From: Hugo Trentesaux <hugo@trentesaux.fr>
Date: Wed, 8 Jan 2025 16:55:34 +0100
Subject: [PATCH 3/5] no need for event when no changes to authority
---
pallets/authority-members/src/lib.rs | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/pallets/authority-members/src/lib.rs b/pallets/authority-members/src/lib.rs
index cb02e5b92..f4118ee3a 100644
--- a/pallets/authority-members/src/lib.rs
+++ b/pallets/authority-members/src/lib.rs
@@ -580,20 +580,24 @@ impl<T: Config> pallet_session::SessionManager<T::ValidatorId> for Pallet<T> {
for member_id in members_ids_to_add.iter() {
T::OnIncomingMember::on_incoming_member(*member_id);
}
- // a single event with all authorities
- Self::deposit_event(Event::IncomingAuthorities {
- members: members_ids_to_add.clone(),
- });
+ // a single event with all authorities if some
+ if !members_ids_to_add.is_empty() {
+ Self::deposit_event(Event::IncomingAuthorities {
+ members: members_ids_to_add.clone(),
+ });
+ }
// -- handle outgoing members
// callback when smith is outgoing
for member_id in members_ids_to_del.iter() {
T::OnOutgoingMember::on_outgoing_member(*member_id);
}
- // a single event with all authorities
- Self::deposit_event(Event::OutgoingAuthorities {
- members: members_ids_to_del.clone(),
- });
+ // a single event with all authorities if some
+ if !members_ids_to_del.is_empty() {
+ Self::deposit_event(Event::OutgoingAuthorities {
+ members: members_ids_to_del.clone(),
+ });
+ }
// updates the list of OnlineAuthorities and returns the list of their key
Some(
--
GitLab
From 2aa18ec55ef5128194c9340371675089ee3244ad Mon Sep 17 00:00:00 2001
From: Hugo Trentesaux <hugo@trentesaux.fr>
Date: Wed, 8 Jan 2025 17:00:22 +0100
Subject: [PATCH 4/5] reveal that expires_on is non null
issue #243
---
runtime/gdev/tests/integration_tests.rs | 90 +++++++++++++++++++++++++
1 file changed, 90 insertions(+)
diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs
index 5ca299b43..af4094518 100644
--- a/runtime/gdev/tests/integration_tests.rs
+++ b/runtime/gdev/tests/integration_tests.rs
@@ -25,6 +25,7 @@ use frame_support::{
use gdev_runtime::*;
use pallet_identity::{RevocationPayload, REVOCATION_PAYLOAD_PREFIX};
use pallet_membership::MembershipRemovalReason;
+use pallet_session::historical::SessionManager;
use pallet_smith_members::{SmithMeta, SmithStatus};
use scale_info::prelude::num::NonZeroU16;
use sp_core::{Encode, Pair};
@@ -1020,6 +1021,95 @@ fn test_smith_process() {
})
}
+// reveal bug from #243
+#[test]
+fn test_expired_smith_with_null_expires_on() {
+ // initial_authorities_len = 2 → Alice and Bob are online
+ // initial_smiths_len = 3 → Charlie is offline Smith
+ // initial_identities_len = 4 → Dave is member but not smith
+ ExtBuilder::new(2, 3, 4).build().execute_with(|| {
+ run_to_block(1);
+
+ // Bob is smith
+ assert_eq!(
+ SmithMembers::smiths(2),
+ Some(pallet_smith_members::SmithMeta {
+ status: SmithStatus::Smith,
+ expires_on: None, // because online
+ issued_certs: vec![1, 3],
+ received_certs: vec![1, 3],
+ })
+ );
+
+ // force Bob to leave by expiring his main WoT membership
+ Membership::do_remove_membership(2, MembershipRemovalReason::System);
+
+ // check events
+ // membership removal
+ System::assert_has_event(RuntimeEvent::Membership(
+ pallet_membership::Event::MembershipRemoved {
+ member: 2,
+ reason: MembershipRemovalReason::System,
+ },
+ ));
+ // smith membership removal
+ System::assert_has_event(RuntimeEvent::SmithMembers(
+ pallet_smith_members::Event::SmithMembershipRemoved { idty_index: 2 },
+ ));
+ System::assert_has_event(RuntimeEvent::AuthorityMembers(
+ pallet_authority_members::Event::MemberRemoved { member: 2 },
+ ));
+ // also events for certifications
+
+ // check state
+ // Bob is not Smith anymore
+ assert_eq!(
+ SmithMembers::smiths(2),
+ Some(pallet_smith_members::SmithMeta {
+ status: SmithStatus::Excluded, // automatically excluded
+ expires_on: None, // because excluded, no expiry is scheduled
+ issued_certs: vec![1, 3],
+ received_certs: vec![], // received certs are deleted
+ })
+ );
+ // Alice smith cert to Bob has been deleted
+ assert_eq!(
+ SmithMembers::smiths(1),
+ Some(pallet_smith_members::SmithMeta {
+ status: SmithStatus::Smith,
+ expires_on: None, // because online
+ issued_certs: vec![3], // cert to Bob has been deleted
+ received_certs: vec![2, 3],
+ })
+ );
+
+ // run to next block
+ run_to_block(2);
+
+ // simulate new session
+ AuthorityMembers::new_session(2);
+ // check event
+ System::assert_has_event(RuntimeEvent::AuthorityMembers(
+ pallet_authority_members::Event::OutgoingAuthorities { members: vec![2] },
+ ));
+
+ // show that expires_on is non null
+ // this is issue #243
+ // Bob is not Smith anymore
+ assert_eq!(
+ SmithMembers::smiths(2),
+ Some(pallet_smith_members::SmithMeta {
+ status: SmithStatus::Excluded, // still excluded
+ expires_on: Some(48), // should be still None !!
+ issued_certs: vec![1, 3],
+ received_certs: vec![],
+ })
+ );
+
+ // println!("{:#?}", System::events()); // with -- --nocapture
+ })
+}
+
/// test new account creation
#[test]
fn test_create_new_account() {
--
GitLab
From c8247b0625291d1b3b6c7c32bb0641b50242fa9c Mon Sep 17 00:00:00 2001
From: Hugo Trentesaux <hugo@trentesaux.fr>
Date: Wed, 8 Jan 2025 17:19:35 +0100
Subject: [PATCH 5/5] fix #243
---
pallets/smith-members/src/lib.rs | 8 +++++---
runtime/gdev/tests/integration_tests.rs | 8 +++-----
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/pallets/smith-members/src/lib.rs b/pallets/smith-members/src/lib.rs
index 35918f55a..1ed34c47b 100644
--- a/pallets/smith-members/src/lib.rs
+++ b/pallets/smith-members/src/lib.rs
@@ -582,7 +582,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
+ // FIXME: unschedule old expiry (#182)
}
});
}
@@ -592,10 +592,12 @@ impl<T: Config> Pallet<T> {
/// 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) {
- if smith_meta.expires_on.is_none() {
+ // 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 {
- // As long as the smith is online, it cannot expire
+ // schedule expiry
let new_expires_on =
CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get();
smith_meta.expires_on = Some(new_expires_on);
diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs
index af4094518..a8b3cb47b 100644
--- a/runtime/gdev/tests/integration_tests.rs
+++ b/runtime/gdev/tests/integration_tests.rs
@@ -1023,7 +1023,7 @@ fn test_smith_process() {
// reveal bug from #243
#[test]
-fn test_expired_smith_with_null_expires_on() {
+fn test_expired_smith_has_null_expires_on() {
// initial_authorities_len = 2 → Alice and Bob are online
// initial_smiths_len = 3 → Charlie is offline Smith
// initial_identities_len = 4 → Dave is member but not smith
@@ -1093,14 +1093,12 @@ fn test_expired_smith_with_null_expires_on() {
pallet_authority_members::Event::OutgoingAuthorities { members: vec![2] },
));
- // show that expires_on is non null
- // this is issue #243
- // Bob is not Smith anymore
+ // control state is still ok
assert_eq!(
SmithMembers::smiths(2),
Some(pallet_smith_members::SmithMeta {
status: SmithStatus::Excluded, // still excluded
- expires_on: Some(48), // should be still None !!
+ expires_on: None, // should be still None
issued_certs: vec![1, 3],
received_certs: vec![],
})
--
GitLab