À quoi servent les pending membership?

Protocols Ğ1v2 protocol
1

Je me posais la question de l’utilité des pending membership (≈ demandes d’adhésion). Ce qui est rigolo c’est qu’en cherchant sur le forum, le premier résultat est un bug : First ĞDev bug: Pending membership expiration remove membership even if it is no longer pending.

En gros dans Duniter v1 :

  • on fait une demande d’adhésion
  • on reçoit des certifications
  • si la demande d’adhésion n’est pas validée à temps (règle de distance inclue), le dossier est perdu, il faut re-obtenir les certifications (mais les certificateurs n’ont pas subi le délai de certification ni le coût en stock), le pseudo déclaré peut être réutilisé

Et en Duniter v2 actuellement :

  • on fait une demande d’adhésion (automatiquement à la création de l’identité)
  • on reçoit des certifications
  • si la demande d’adhésion n’est pas validée à temps (règle de distance inclue), l’identité est perdue, il faut re-obtenir les certifications et les certificateurs ont quand même subi le délai de certification et le coût en stock (les certifications ne sont supprimées que pour une identité au statut “validée” ??), le pseudo déclaré ne peut pas être réutilisé.

Et en plus, en Duniter v2 on a l’adhésion forgeron :

  • on fait une demande d’adhésion forgeron
  • si la demande forgeron n’est pas validée à temps, le comportement reste à tester :
    • peut-être que ça supprime l’identité (cf ticket #155)
    • sinon ça n’a pas d’incidence, il suffit de redemander l’adhésion forgeron et les certifications sont toujours là
  • en plus on peut recevoir des certifications forgeron même sans avoir fait de demande d’adhésion forgeron

Je suis de plus en plus convaincu qu’avoir fait de wot une pallet instanciable qui a des comportements différents en fonction de IsSubWot introduit une complexité difficile à appréhender. (cf réflexion Pallets instanciables et vérifications d'adhésion).

Et je me demande également à quoi servent les pending membership. Répondent-elles vraiment à un besoin ? Dans les deux cas (adhésion normale et forgeron) ?

À priori, toute identité créée et non révoquée devrait souhaiter être membre, y a-t-il besoin de différencier celles qui souhaitent devenir membre (pending membership) et celles qui ne le souhaitent pas ?

Par contre ce n’est pas le cas pour la toile forgeron. On a noté que ce serait bien de limiter les certifications forgeron aux candidats pour l’être. Mais actuellement c’est un bug, les certifications peuvent être émises qu’il y ait une demande en cours ou pas. Et même quand on révoque l’adhésion forgeron, les certifications sont toujours là.

J’ai l’intuition (mais pas encore vérifié) qu’en éliminant les pending membership et l’instance membership forgeron, on retirerait pas mal de bugs et simplifierait le fonctionnement.

Il faut que je prenne le temps de proposer un fonctionnement alternatif détaillé. Voilà les critères à remplir (ou pas ?) :

  • après une trop longue période pendant laquelle une identité n’est pas/plus membre de la toile principale, cette identité est supprimée (mais pas les certifications), (le pseudo est-il à nouveau disponible ?)
  • toute identité peut recevoir des certifications normales à tout moment, qu’elle soit membre ou pas
  • la “demande d’adhésion” n’a pas besoin d’être formulée pour la toile principale (la seule raison de l’existence d’une identité est d’être membre)
  • la “demande d’adhésion forgeron” est une condition nécessaire à la réception de certifications forgeron
  • un forgeron peut changer sa clé forgeron pour plus de sécurité
  • l’identité peut toujours révoquer son adhésion forgeron
2 Likes
2

Ce point central dans le protocole DUBPv2 nous montre qu’il y a encore potentiellement beaucoup de choses de fonds à revoir avant une mise en prod.

Dans l’hypothèse où l’on garderai ce système de pending membership, ne serait-il pas plus logique de faire cette demande d’adhésion non pas à la création de l’identité (qui peut être fait par n’importe quel membre vers n’importe quel wallet) mais plutôt lors de la première demande d’évaluation de la règle de distance ? Ou bien à la demande de validation de l’identité, je ne sais pas en fait…

C’est vrai que plus j’y pense et moins et comprends l’utilité de cette demande d’adhésion, d’où ma confusion à ce sujet récemment avec la suppression de l’identité avec l’expiration du statut de membre.

Même pour les smiths, pourquoi l’adhésion est-elle nécessaire ? La condition pour recevoir une certification smith pourrait simplement être d’être membre validé de la toile principale.

Globalement, tout ce qui va dans le sens de simplifier implémentation du protocole me va, quitte à dupliquer cette palette wot en fonction des toiles, mais là dessus je vous laisse gérer ça je n’ai pas assez de détails sur l’implémentation.

3

Comment ? Les certifications reçues par une identité qui n’existe plus n’ont pas de sens, et seule l’identité fait le lien entre les certifications et le compte, donc le compte ne pourra pas récupérer ces certifs sur une nouvelle identité.

4

Oui, c’est assez perturbant de passer sans cesse de l’état “ah c’est bon, on a presque fini” à l’état “ah merde, il nous reste toute cette catégorie de bugs”. Il faudra qu’on discute dans quand est-ce qu’on va geler l’implémentation v2 pour programmer la migration. Ce sera plus de boulot de faire des runtime upgrade sur une Ğ1 live, mais en même temps ce sera immédiatement utile à la communauté.
Le ticket critique pour savoir quand est-ce qu’on pourra geler les développements est le #141.

Mon hypothèse actuelle c’est qu’on peut s’en passer, et juste implémenter ça côté pallet identity pour avoir le délai de suppression d’une identité si elle ne devient pas membre suffisamment vite. Effectivement, on peut choisir à partir de quel événement on compte ce délai.

C’est l’éternel sujet Proposition de supprimer la notion d'identité désactivée mais non révoquée et la notion d'adhésion qui m’a causé tant de maux de tête ><

Effectivement, ce serait plus simple. Je partais de ces commentaires :

mais je reste sur ma position :

En fait pour l’instant il y a deux instances de la pallet membership plus une de la pallet authority members. Et je trouve que la pallet membership forgeron est redondante avec la pallet authority members. Si on regarde call par call :

  • smithMembership.request_membership() → pas besoin de demander à être forgeron on chain, il suffit de parler hors chaîne à un autre forgeron qui nous certifiera selon la licence
  • smithMembership.renew_membership() → y a-t-il vraiment besoin de l’expiration des adhésions forgeron ? sachant qu’on a en plus un critère max_offline_session dans authority_members ? doit-on garder les deux, un seul (lequel ?), aucun ?
  • smithMembership.claim_membership() → c’est pour être ajouté aux authority members, mais est-ce que le critère sur go_online ne suffit pas ?
  • smithMembership.revoke_membership() → quelles sont les raisons pour appeler ce call ? si on ne veut plus être online, il suffit d’appeler go_offline, si on s’est fait voler ses clés membre, il faut révoquer son identité

Là c’est un peu brouillon, mais je pense qu’il faudrait lister les scénarios qu’on souhaite envisager et pour chaque scénario voir quelles solutions humaines ou techniques on souhaite proposer.

Dans Duniter-v1, il n’y a pas de mécanisme pour supprimer une certification autrement que par expiration de la durée de 2 ans après le dernier renouvellement. Ça se matérialise dans les interfaces par des certifications vers des identité ayant perdu leur adhésion ou même révoquées :

image
image956×793 122 KB

image

Ces certifications sont toujours dans leur délai de validité, mais peuvent ne plus jamais “servir” (ici dans le cas de pyg).

Ces certifications font toujours partie du total de certification émises. Supprimer ces certifications prématurément aurait pour effet de diminuer la tension sur le quota de certifications des personnes ayant certifié beaucoup d’identités révoquées. Donc je préfère ne pas changer le protocole pour l’instant, quitte à avoir cette discussion plus tard avec la communauté.

2 Likes
5

La demande d’adhésion a répondu à trois fonctions :

  1. Maîtriser la création finale du compte dans la blockchain : créer l’identité était une chose, recevoir des certifications aussi, mais je voulais laisser la liberté à l’utilisateur de confirmer que cette identité était bien celle qu’il voulait confirmer dans la blockchain.
  2. Manifester que le détenteur du compte est toujours en vie
  3. Conséquence de 2° : c’est le support idéal pour évaluer la règle de distance

On pourrait considérer que 1° est de l’excès de zèle, et que 2° et 3° peuvent être gérés autrement. Si c’est le cas, je ne vois plus de justification pour maintenir cette étape.

Oui, mais il reste le besoin de s’assurer des point 2° et 3°.

Alors si, justement : il y a une inertie volontaire dans les certifications pour éviter un écroulement de la distance.

Que les priorités soient claires : autant il est important de migrer la Ğ1 sans tarder sur une solution pérenne comme Duniter V2S sur Substrate, autant aller en Prod avec un système bancale est pire que rester sur Duniter V1 (qui au-delà de ses modestes performances, est stable).

5 Likes
6

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()
7

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().

Il n’y a plus de notion d’adhésion dans ce cas.

1 Like
8

Je vais dérouler ma pensée.

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.

2 Likes
9

Je n’étais pas sur une position aussi radicale, je voulais juste retirer l’élément du storage “pending”.

image
image1163×731 118 KB

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.

10

En parallèle, @bgallois a ajouté de la documentation dans le README de la pallet membership. Le fonctionnement actuel y est décrit, lecture conseillée : pallets/membership/README.md · 2e6fa77dc1b2400112487ca9b3e9ceb8ea940f9a · Benjamin Gallois / Duniter v2S · GitLab

11

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.

3 Likes
12

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.

13

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 :joy:

[edit] plus sérieusement j’ai créé le ticket #156 et l’ai mis en “S-needdiscussion”

14

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 ?

15

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.

16

Tu bottes en touche à chaque fois.

Je répète : quand tu enlèves les call que tu comptes enlever, que reste-t-il ? As-tu fait l’inventaire avant de me répondre ?

On peut aussi créer une pallet pour chaque micro-donnée, ça va être bien modularisé, vraiment ?

17

Il restera :

  • deux calls
    • claim_membership
    • renew_membership
  • pas mal de fonctions internes
    • force_expire_membership
    • try_claim_membership
    • unschedule_membership_expiry
    • insert_membership_and_schedule_expiry
    • check_allowed_to_claim
    • do_claim_membership
    • do_expire_membership
    • expire_memberships
  • deux éléments de storage
    • Membership
    • MembershipsExpireOn
  • trois événements
    • MembershipAcquired
    • MembershipExpired
    • MembershipRenewed
  • quatre erreurs
    • IdtyIdNotFound
    • MembershipAlreadyAcquired
    • MembershipNotFound

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.

18

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. :+1:

D’un point de vue du protocole (et des tests), ce sera déjà plus simple.

Bref je vote pour tes propositions. :white_check_mark:

1 Like
19

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.

20

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 :slight_smile:

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.