Pour moi ça c’est la responsabilité du renouvellement d’adhésion, qui est évidemment à conserver. Donc :
membership.request_membership() → pas utile, pour la demande d’évaluation de la distance ce sera distance.request_distance_evaluation()
membership.renew_membership() → c’est utile, on conserve
membership.claim_membership() → c’est doublon avec identity.validate_identity(), je préfère garder le call membership et retirer le call identity
membership.revoke_membership() → quel est le scénario dans lequel on souhaite révoquer son adhésion membre mais pas son identité ? redondant avec identite.revoke_identity()
Ce n’est pas si évident : si le concept même d’adhésion ne sert plus qu’à dire “je suis en vie” et “je respecte toujours la distance”, il y a possiblement d’autres façons de le faire plutôt qu’une matérialisation par une pallet.
Par exemple, ajouter dans l’identité un champ distance_valid_until (n° de bloc maximum jusqu’auquel la distance est valide) qui porte à la fois le côté “vivant” de l’identité et qui permet de contrôler la distance, accessible par distance.request_distance_evaluation().
Supprimer la notion d’adhésion implique de recoder la gestion de la toile forgeron. Cela peut se faire sur une pallet dédiée aux Smiths, simplifiée, et qui repose uniquement sur la pallet identity (pour être forgeron il faut être membre, pour rester forgeron il faut rester membre). Pourquoi pas authority-members comme tu indiques @HugoTrentesaux que la pallet est redondante avec la pallet membership.
Il faudrait également gérer l’expiration d’une identité (se produisant à T+distance_valid_until), possiblement par un traitement IDLE, et passer l’identité au statut Disabled qui est actuellement commenté dans le code.
En option, il faudrait quand même gérer un état supplémentaire Revoked qu’on n’a pas aujourd’hui et qui avait une raison d’exister dans Duniter v1 : pouvoir empêcher une personne de nuire à un compte dont les identifiants ont été volés. Aujourd’hui, si une identité est supprimée dans V2S, on peut se retrouver face à une usurpation d’identité.
A part ces quelques détails, je ne vois que des avantages à retirer cette notion d’adhésion.
en gros, ok pour Membership et MembershipsExpireOn mais pas ok pour PendingMembership et PendingMembershipsExpireOn. Je suis pas prêt à faire de gros changements d’architecture, ça prendrait trop de temps, mais un petit peu d’élagage pour que le développement de l’écosystème soit plus simple.
Ça c’est déjà pris en charge par le renouvellement d’adhésion qui nécessite une évaluation positive de la distance.
Ce statut n’a jamais existé, je l’ai ajouté dans le code en commentaire au moment où j’ai permis à une adhésion d’expirer sans que l’identité soit supprimée, mais je me suis aussi rendu compte que le statut d’identité faisait doublon avec la présence d’une adhésion (Demande d'adhésion et statut d'identité).
Non, à la révocation d’identité, l’identité est supprimée, donc plus personne ne peut s’en servir. Et je posais la question sur l’existence d’un scénario dans lequel on souhaiterait révoquer l’adhésion sans révoquer l’identité :
Ce ne sont pas des détails, c’est la raison d’être de la pallet membership, et je la trouve utile. C’est juste “pending membership” qui me pose problème.
Les clés ont été dérobées : il suffit d’un complice pour recréer l’identité avec exactement la même clé publique. Et même si le pseudo restait réservé, il suffit d’une petite variation pour facilement tromper autrui. La révocation de Duniter V1 ne souffrait pas de ce problème.
Je n’ai pas lu d’argument d’utilité dans ton message, mais des arguments de convenance : la pallet est déjà là donc on ne va pas investir plus d’énergie.
Mais d’une part je ne suis pas certain que le coût de refacto soit très élevé (l’élagage en général c’est plutôt facile), d’autre part je pense que tu sous-estimes les coûts liés aux bugs. Nous en identifions déjà fréquemment sur des scénarios pourtant standards. Nous allons rapidement payer le prix de la complexité, et au final nous finirons par accepter l’idée qu’il faut simplifier.
Si tu n’as pas l’énergie pour le faire tant pis, moi ça ne me dérange pas de le faire. D’ailleurs je ne vais pas demander l’autorisation, je le ferai dans mon coin puis je vous montrerai le résultat.
Plus tôt nous acceptons de nous séparer du superflux, moins ça fera mal.
Oui, en effet, dans Duniter-v2 une paire de clés n’identifie pas une identité de manière unique. On pourrait ajouter un mécanisme qui permet de bannir des clés. Et on pourrait bannir les anciennes clés lorsque l’on appelle change_owner_key. Mais c’est un nouveau mécanisme à ajouter côté Duniter alors que pour moi ça pourrait se régler côté client. Il faudrait détailler ce scénario d’attaque dans un post dédié pour voir si c’est pertinent d’y apporter une solution côté Duniter.
Oui, la SOC c’est de la convenance. On pourrait avoir un architecture monolithique, mais elle serait plus compliquée à comprendre et faire évoluer. Alors que là c’est très clair : pallet identity pour gérer l’identité, pallet membership pour gérer l’adhésion. Une identité peut avoir un historique d’obtention et expiration d’adhésion.
Attention, je vais avoir des réticences à approuver des modifications qui diminuent la modularité du logiciel. C’était le cas pour ta branche qui intégrait le calcul de la distance directement dans la pallet certification, j’étais réticent à cette idée.
J’ai ouvert ce fil pour réfléchir à retirer les pending membership (demandes d’adhésions), pas les membership (adhésions), que je souhaite conserver.
Un exemple d’utilisation de l’historique d’adhésion : afficher le DU dans l’historique des transactions. C’est d’ailleurs en réfléchissant à implémenter ça côté indexeur que je me suis rendu compte que seuls les événements MembershipAcquired et MembershipExpired étaient utiles, pas Pending*. J’ai donc fait une pause dans l’implémentation de duniter-squid pour aborder ce sujet ici, éventuellement aboutir au retrait des pending_machinchose (donc simplifier), puis reprendre l’implémentation côté indexeur pour arriver à fournir simplement l’historique des DU perçus.
Si je résume ce que j’ai écrit plus haut en un tableau :
ce que j'ai écrit plus haut
membership call
useful in membership instance
useful in smith membership instance
request
no
no
renew
yes
maybe
claim
yes
maybe
revoke
no
no
Et donc ma proposition serait :
supprimer le call request_membership
supprimer le call revoke_membership
Si on a besoin d’une durée de validité de l’adhésion forgeron indépendante de la durée de validité de l’adhésion membre, ou si on souhaite ajouter une étape forgeron claim_membership plutôt qu’ajouter add_member dans authority-members, on peut conserver l’instance smith de la pallet membership.
Mais dans le cas contraire, je propose en plus de :
retirer l’instance smith de la pallet membership
Voilà, je suis d’accord avec moi-même
[edit] plus sérieusement j’ai créé le ticket #156 et l’ai mis en “S-needdiscussion”
Pour clarifier : le besoin c’est d’empêcher l’usurpation d’identité, pas d’empêcher totalement des clés de fonctionner (laisse la possibilité de transférer des fonds par ex.). Le statut Revoked me semble la chose la plus simple à gérer.
Mais pour quoi faire ?
MembershipAcquired et MembershipExpired n’ont pas besoin d’une pallet dédiée pour être émis, ça peut très bien être fait par la palette identité puisque c’est celle-ci que ça concerne.
Avec ta proposition, que reste-t-il dans cette palette qui vaudrait le coup de la conserver ? Dans quel but ?
Pour moi, la notion d’identité et la notion d’adhésion sont différentes, donc mieux vaut les séparer dans des pallets testables indépendamment. Il n’y a pas besoin d’une pallet dédiée, on pourrait tout mettre dans une seule pallet, mais ça me semble moins clair, c’est tout.
On pourrait déplacer les calls claim_membership et renew_membership dans la pallet identity, déplacer le storage Membership et MembershipExpireOn dans la pallet identity, ou même supprimer le storage Membership pour le mettre dans un champ de IdtyValue… Tout ça est possible oui, c’est juste un choix d’organisation. Je trouve ça plus clair quand c’est organisé dans des petits modules, tu trouve ça plus clair quand c’est organisé dans moins de modules (donc plus gros). Je propose un refactoring qui élague, tu proposes un refactoring qui élague et regroupe des pallets.
Je ne pense pas que ce soit très productif qu’on travaille en parallèle sur les deux solutions pour n’en retenir qu’une à la fin, donc je vais laisser ça de côté, y revenir à tête reposée, éventuellement en parler avec d’autres (comme @poka, @tuxmain, @bgallois, @vit, @kimamila), qui auront peut-être également un avis, et voir ce qui plait le plus.
Soit à peu près 300 lignes de code. lib.rs de la pallet identity fait 800 lignes de code. Réunir les deux nous fait passer au dessus des 1000 lignes de code. On pourrait aussi séparer en différents fichiers, mais pour la facilité de compréhension et de gestion du code, je préfère toujours avec deux pallets. Peut être qu’en revenant plus tard j’aurai changé d’avis, mais pour l’instant c’est ce que je préfère. J’aime bien la séparation en pallets :
identity
membership
wot
certification
J’aime moins la séparation en pallet sans membership :
identity
wot
certification
C’est pas argumenté, c’est juste une histoire de préférence d’organisation.
Voilà. Et encore, si tu détailles les éléments du storage : Membership ne fait que stocker le bloc d’expiration. Quant à MembershipsExpireOn, c’est la transposée.
Bref, une seule donnée à stocker par identité. On garde une pallet pour ça ? On explique aux clients qu’il faut un concept supplémentaire juste pour une date d’expiration ? Pour le coup on va dupliquer des clés (idty_id) dans le storage “par principe de séparation des problèmes”. Pour moi ce n’est pas la raison qui parle, c’est du dogme (ou de le résistance au changement, je n’en sais rien).
Cela dit, je ne vais pas insister. Je trouve déjà que les suppressions que tu proposes sont une bonne avancée.
D’un point de vue du protocole (et des tests), ce sera déjà plus simple.
Le révocation de forgeron peut servir à quelqu’un qui sait qu’il ne fera plus goOnline et ne veut plus être soumis à un risque aussi élevé en cas de fuite de clé privée. Sans ça, l’ex forgeron est obligé de se conformer à la licence forgeron, même s’il n’a plus les moyens de la respecter, jusqu’à expiration.
Pas besoin de dupliquer des clés si on n’a qu’une seule StorageMap pour les identités, contenant dans un champ une struct membership. Ça alourdit la config avec des providers mais c’est du code creux.
J’ai pas très bien dormi cette nuit, mais je me suis quand même réveillé avec une idée en plus :
lorsque le nombre de certifications reçues passe en dessous du seuil, il faut supprimer l’adhésion, que ce soit pour la toile principale, ou la toile foreron
Actuellement c’est fait avec le handler OnRemovedCert dont l’implémentation par la pallet wot est la suivante :
// remove cert handler
impl<T: Config<I>, I: 'static> pallet_certification::traits::OnRemovedCert<IdtyIndex>
for Pallet<T, I>
{
fn on_removed_cert(
_issuer: IdtyIndex,
_issuer_issued_count: u32,
receiver: IdtyIndex,
receiver_received_count: u32,
_expiration: bool,
) {
if receiver_received_count < T::MinCertForMembership::get()
&& pallet_membership::Pallet::<T, I>::is_member(&receiver)
{
// expire receiver membership
// it gives them a bit of time to get back enough certs
if let Err(e) = <pallet_membership::Pallet<T, I>>::force_expire_membership(receiver) {
sp_std::if_std! {
println!("fail to expire membership: {:?}", e)
}
}
}
}
}
Si on retire la pallet membership, il faudra déplacer cette implémentation dans la pallet identity pour les adhésions normales et dans la pallet authority members pour les adhésions forgeron. Cette nuit cette idée ne me plaisait pas, mais en l’écrivant, ça ne me gène pas plus que ça. L’idée fait son chemin
Effectivement, dit comme ça, ça donne envie de faire le ménage !!
Plutôt de la résistance au changement, mais pour ça, la nuit peut aider (alors que pour le dogme ça n’y fait pas grand chose).
Pas besoin d’insister, juste amener des arguments et des idées, et laisser un peu de temps à ma petite tête pour digérer tout ça. D’où le :
Actuellement, la révocation d’adhésion forgeron n’empêche pas quelqu’un qui récupère la clé de refaire request_membership et claim_membership, les certifications sont toujours valides. Si on veut obtenir cet effet de révocation longue durée, il faut soit bannir les clés soit pouvoir annuler des certifications forgeron reçues.
Euh, ces palettes ne connaissent pas les certifs, tu veux parler de duniter-wot ?
Il faut en effet supprimer les certifs forgeron et choisir un délai après révocation (même mécanisme que pour la révocation principale). Mais le délai devrait être plus court ici.
Par contre si le forgeron revient il faut s’assurer qu’il a changé ses clés de session. Avec une table de clés bannies ? (et donc une expiration des bannissements au bout de N années…)
En fait non, je me suis mal exprimé, on garde l’implémentation dans la pallet wot, mais la fonction qu’elle appelle sera dans une autre pallet.
// cette partie reste dans la pallet wot
if receiver_received_count < T::MinCertForMembership::get()
// cette partie disparaît, pas besoin de vérifier si l'identité est membre pour transmettre
// le message qu'elle est passée en dessous du seuil de certifications
&& pallet_membership::Pallet::<T, I>::is_member(&receiver)
// cette partie est déplacée dans les pallets qui vont gérer les adhésions
if let Err(e) = <pallet_membership::Pallet<T, I>>::force_expire_membership(receiver) {
Donc en gros, ça fait
if receiver_received_count < MinCertForMembership
try_remove_membership()
Pour l’instance wot, ce sera la pallet identity qui essaiera de retirer l’adhésion (mais si elle n’est déjà plus là il n’y a rien à faire), pour l’instance smith_wot, ce sera la pallet authoriy_members.
Les idty_id sont des u32 (32 bits), pas des clés publiques (32 octets). Donc l’impact de la duplication est moindre. Mais effectivement, ça fait de la duplication peu utile.
Dans un premier temps ça peut être fait comme ça, et dans un 2ème temps : on peut carrément faire sauter la pallet duniter-wot.
Ce n’est pas que je veux tout faire sauter ! Mais ça permet de découpler la partie WoT de la partie autorités, vu que cette idée de Smith WoT ne te satisfait pas vraiment et que cet avis est partagé par d’autres (moi le 1er). La solution d’Eloïs avait le mérite de fonctionner tout de suite, mais si l’on peut faire plus adapté (et surtout moins complexe), ça peut valoir le coup.
A mon avis, authority_members pourrait être auto-suffisante pour gérer la problématique d’entrée/sortie des Smith, avec des règles plus spécifiques que celles de la WoT classique. Il y aura sûrement un lien avec la pallet identity, mais pas beaucoup plus.
Actuellement je ne suis pas serein sur les tests, je trouve que la combinatoire est trop grande avec tous les liens qui existent au niveau des pallet de WoT, qui en plus est dupliquée avec les Smith. Je pense que stratégiquement il vaudrait mieux séparer tout ça.
Actuellement il y a des palettes très génériques (membership, identity, cert) et une palette “colle”, duniter-wot. Qui ferait son travail ?
Idée pour éviter les problèmes de combinaisons imprévues : implémenter une machine à états dans duniter-wot, qui décrit entièrement tous les états possibles d’une identité, et toutes les transitions entre états (aucune transition partielle). Mais ça risque d’être une usine à gaz monolithique.
