Ancien membre de retour avec seulement 4 certifications

En effet on peut simplifier :slight_smile:

Voici la version simplifiée :

ENTRY.enoughCerts = COUNT(REDUCE_BY(CONCAT(GLOBAL_CINDEX[receiver=ENTRY.pub,expired_on=null], LOCAL_CINDEX[receiver=ENTRY.pub,expired_on=null]), 'issuer')) >= sigQty

3 Likes

Ok, souhaite tu que je l’ajoute au draft de la RFC v12 ? (la version simplifiée après remarque de gerard94)

Oui, c’est bien d’ajouter ce correctif du protocole dans la v12.
Ça ne nécessitera pas de déclenchement par changement de version de bloc.
Juste un correctif du code qui peu être délivré à un moment différent afin de respecter cette version du protocole.

3 Likes

Une variante plus optimisée (sans REDUCE_BY) :

ENTRY.enoughCerts = COUNT(UNIQ(CONCAT(PICK(GLOBAL_CINDEX[receiver=ENTRY.pub,expired_on=null], 'issuer'), PICK(LOCAL_CINDEX[receiver=ENTRY.pub,expired_on=null]], 'issuer')))) >= sigQty

  1. On extrait le set des issuers des certifications inscrites
  2. On extrait le set des issuers des certifications pending
  3. On concatène les deux set d’issuers
  4. On supprime les doublons
  5. On compte
7 Likes

Oui ce serait bien. Avec la version que tu souhaites, les deux solutions proposées étant correctes a priori.

2 Likes

Pour info : je n’ai pas encore fini l’investigation, ce n’est pas si long mais j’ai peu de temps. En tout cas je développe de nouveaux outils de débogage qui seront bien utiles, je vous les partagerai bien entendu.

edit : bug identifié (en plus de celui du protocole, c’est le bug qui empêche la résolution de fork) dans la méthode findByReceiverAndExpiredOn(). Saurez-vous le trouver ?

3 Likes
--      return (await this.get(issuer)).issued.filter(e => e.receiver === pub && e.expired_on === 0)
++      return (await this.get(issuer)).issued.filter(e => e.receiver === pub && e.expired_on === expired_on)

?

1 Like
Ma réponse :

La méthode findByReceiverAndExpiredOn retourne toutes les lignes de l’index
CINDEX qui concernent une certification reçue par l’intéressé. Il manque un reduceBy([‘issuer’, ‘receiver’]).
D’ailleurs perso je supprimerai cette méthode et je remplacerai le seul appel par getValidLinksTo qui ferait parfaitement bien le job pour la BR_G27 :slight_smile:

@Moul non ce n’est pas ça car le seul appel a cette fonction ce fait avec la valeur zéro pour expired_on :stuck_out_tongue:

1 Like

It’s done :slight_smile:

J’ai créé un ticket lié sur le dépôt des RFC, je pense que c’est l’endroit idéal pour tracer les défauts de spec : BR_G27: do not count certifications being replayed twice (#11) · Issues · nodes / common / doc · GitLab

Et j’ai ajouté le correctif à la RFC v12 :

2 Likes

Alors oui, Moul, il y a ce bug mais voir la réponse d’Eloïs sur ce point. Mais je vais effectivement changer pour ta version.

Alors il manque effectivement la réduction, et oui c’est bien le cœur du problème mais pas exactement comme tu l’exprimes. En fait c’est l’élagage qui fait que cette méthode renvoie deux résultats différents selon l’état du CINDEX “élagué” ou “non élagué”.

Le protocole s’exprime dans un référentiel (index) non élagué, mais pour des raisons de performances Duniter élague ses index. Or dans le cas présent, la méthode findByReceiverAndExpiredOn buguée est sensible à l’élagage. Si le protocole avait été correct toutefois (s’il avait requis une réduction via la fonction REDUCE), elle ne l’aurait pas été et nous n’aurions pas eu de problème de résolution ce début d’année 2020.

6 Likes

Non là c’est faux (en fait, les 3 propositions le sont), il faut le REDUCE_BY et il faut fermer la fonction avant de filtrer sur expired_on=0, sinon ça ne sert à rien car cette condition est toujours vraie sans réduction : quand une certification est créée (op=CREATE), alors expired_on vaut 0 (et pas null d’ailleurs, c’est une erreur qui traîne dans le protocole) donc on retrouve systématiquement les certifications initiales du membres lors de son entrée dans la monnaie.

Du coup je propose :

BLOCKCHAIN = REDUCE_BY(GLOBAL_CINDEX[receiver=ENTRY.pub], 'issuer')[expired_on=0]
INCOMING = LOCAL_CINDEX[receiver=ENTRY.pub]
UNIQUE_ISSUERS = UNIQ(CONCAT(
    PICK(BLOCKCHAIN, 'issuer'),
    PICK(INCOMING, 'issuer')))
ENTRY.enoughCerts = COUNT(UNIQUE_ISSUERS) >= sigQty

À noter qu’il n’y a pas besoin de filtrer sur l’expiration pour les certifications entrantes, juste sur celles en blockchain.

1 Like

Je reformule pour voir si j’ai bien compris (et pour aider les lecteurs aussi) :

L’index GLOBAL_CINDEX comprend toutes les certifications passées et présentes, y compris celles qui ont déjà expirées. Or on ne veut que celles qui n’ont pas encore expirées.

Chaque ligne d’index correspond a un “évenement”, pour les certifications, il y a 3 evenements possibles :

  • création
  • renouvellement
  • expiration

Le champ expired_on indique le timestamp auquel la certification a expirée, la valeur 0 indique que la certification n’a pas expirée.
Lors de la création, expired_on vaut 0.
Lors de l’expiration, expired_on vaut la date d’expiration.
Lors du renouvellement, je suposse que expired_on vaut zéro (mais ce n’est pas spécifié explicitement et je n’ai pas vérifié dans le code).

Si on ne fait pas de REDUCE_BY, on vas compter toutes les lignes d’index pour lesquelles expired_on == 0, or certaines lignes d’index concernent le passé, donc sans REDUCE_BY on compte les certifications expirées.

Donc en effet, il faut un REDUCE_BY('issuer') dans tout les cas.


Erreur corrigée dans la RFC v12 :slight_smile:

3 Likes

Oui c’est bien cela. Je vais re-détailler le cas de GerardSiegle plus précisément plus tard, tout paraîtra évident.

Oui il vaut zéro, à cet endroit du protocole :

Certifications

Each certification produces 1 new entry:

CINDEX (
    op = 'CREATE'
    issuer = PUBKEY_FROM
    receiver = PUBKEY_TO
    created_on = BLOCK_ID
    written_on = BLOCKSTAMP
    sig = SIGNATURE
    expires_on = MedianTime + sigValidity
    chainable_on = MedianTime + sigPeriod
    replayable_on = MedianTime + sigReplay
    expired_on = 0
)

Qu’il s’agisse d’une nouvelle certification ou d’un renouvellement, dans tous les cas c’est “une certification” et donc l’entrée est identique.

Oui, et c’est ça le bug dans la méthode findByReceiverAndExpiredOn(). Plus précisément, le comportement alterne :

  • parfois les certifications expirées sont comptées (cas conforme à la v11 du protocole)
  • parfois les certifications expirées ne sont pas comptées à cause de l’élagage automatique (car l’élagage est une réduction !)

Pour résumer il y avait bien 2 bugs ici :

  • bug#1 : l’entrée de GerardSiegle avec 4 certifications, bug non bloquant pour la Ğ1 mais problème vis-à-vis de la licence
  • bug#2 : l’erreur de résolution de fork qui pour le coup est bloquante pour la Ğ1 et dont l’origine est double :
    • l’absence de réduction dans le protocole pour cette règle BR_G27 qui n’est clairement pas cohérente avec la philosophie générale des index
    • une réduction automatique par Duniter qui est en conflit avec l’absence de réduction de BR_G27
7 Likes

Eh bien en fait c’était plus subtil. La réduction automatique n’était pas en cause car au final elle ne supprimait rien dans le CINDEX.

Non, en fait c’est le dépilement de bloc (bug#1396) : lors d’une résolution de fork, Duniter dépile les blocs pour annuler les modifications introduites par eux.

Or, dans le cas de certifications renouvelées avant terme Duniter supprimait le lien existant entre l’émetteur et le receveur, alors que pourtant ce lien existe toujours puisque même si le renouvellement est supprimé, l’ancienne certification est bien entendu toujours valable !

De ce fait pour GerardSiegle si nous rejouons la situation : état au bloc #284604 :

duniter sync g1.cgeek.fr 284604
┌─────┬────────┬──────────────────┬────────────┬─────────────────┬────────────┬────────────┬──────────────┬───────────────┐
│ row │ op     │ issuer           │ created_on │ written_on      │ expires_on │ expired_on │ chainable_on │ replayable_on │
│ 0   │ CREATE │ CharlesAbecassis │ 96364      │ 96857-000000408 │ 1582171975 │ 0          │ 1519634701   │ 1524462301    │
│ 1   │ CREATE │ loanblanchard    │ 103654     │ 106456-000001A4 │ 1584427318 │ 0          │ 1522609561   │ 1527437161    │
└─────┴────────┴──────────────────┴────────────┴─────────────────┴────────────┴────────────┴──────────────┴───────────────┘

Puis, on applique le bloc qui contient les certifications dont 1 renouvellement de CharlesAbecassis :

duniter pull g1.cgeek.fr 284605
┌─────┬────────┬──────────────────┬────────────┬─────────────────┬────────────┬────────────┬──────────────┬───────────────┐
│ row │ op     │ issuer           │ created_on │ written_on      │ expires_on │ expired_on │ chainable_on │ replayable_on │
│ 0   │ CREATE │ CharlesAbecassis │ 96364      │ 96857-000000408 │ 1582171975 │ 0          │ 1519634701   │ 1524462301    │
│ 1   │ CREATE │ loanblanchard    │ 103654     │ 106456-000001A4 │ 1584427318 │ 0          │ 1522609561   │ 1527437161    │
│ 2   │ CREATE │ AlainLebrun      │ 284266     │ 284605-000001A7 │ 1640737001 │ 0          │ 1578165360   │ 1582992960    │
│ 3   │ CREATE │ CharlesAbecassis │ 278986     │ 284605-000001A7 │ 1639117359 │ 0          │ 1578165360   │ 1582992960    │
│ 4   │ CREATE │ AnneAmbles       │ 282287     │ 284605-000001A7 │ 1640145167 │ 0          │ 1578165360   │ 1582992960    │
└─────┴────────┴──────────────────┴────────────┴─────────────────┴────────────┴────────────┴──────────────┴───────────────┘

Jusque-là “tout va bien” : CharlesAbecassis a bien sa certification comptée en double par Duniter : c’est conforme au protocole v11 (mais pas à la licence).

Si je revert le bloc :

duniter revert 1

Alors au moment de l’exécution de la règle BR_G27, Duniter voit :

┌─────┬────────┬──────────────────┬────────────┬─────────────────┬────────────┬────────────┬──────────────┬───────────────┐
│ row │ op     │ issuer           │ created_on │ written_on      │ expires_on │ expired_on │ chainable_on │ replayable_on │
│ 0   │ CREATE │ loanblanchard    │ 103654     │ 106456-000001A4 │ 1584427318 │ 0          │ 1522609561   │ 1527437161    │
└─────┴────────┴──────────────────┴────────────┴─────────────────┴────────────┴────────────┴──────────────┴───────────────┘

En effet le revert a supprimé le lien existant entre GerardSiegle et CharlesAbecassis :confused:. Pourtant le CINDEX affiché est toujours bon, mais c’est en interne que la BDD est incohérente.

Conséquences

Comme indiqué dans le ticket du bug#1396 :

  • Conséquence 1 : BR_G27 : les forks qui dépilent des renouvellements anticipés de certification créent potentiellement une situation de rejet d’un bloc pourtant accepté précédemment dans lequel un utilisateur pourtant légitime à entrer (relativement au protocole v11). Ce rejet dépend de la situation du membre par rapport à la limite des 5 certifications.
  • Conséquence 2 : BR_G95 : les forks qui dépilent des renouvellements anticipés de certification peuvent exclure à tort des membres trop proches de la limite des 5 certifications.

Il n’est pas du tout exclu que cela se soit déjà produit.

À noter enfin que la règle de distance n’est pas affectée par cette anomalie, les liens sont toujours bien présents dans le module de calcul de WoT.

Correctif

Le correctif viendra cette semaine, avec le test automatisé de verrou. Il m’a déjà fallu un dimanche entier pour débusquer l’anomalie à cause d’une fausse piste, je suppose qu’il me faudra une autre journée pour commiter le correctif + test + commandes de débogage qui m’ont aidé (notamment celle qui vous affiche ces beaux tableaux d’INDEX).

8 Likes

Merci beaucoup pour le temps que tu continues de prendre pour Duniter en tout cas. Il était plutôt costaud celui ci ! Un dimanche complet passé a analyser un bug ça fait jamais plaisir :slight_smile: J’espère que ton analyse permettra a d’autres développeurs de tenter le correctif la prochaine fois !

4 Likes

La couche d’accès aux données c’est vraiment pas le plus facile. Qu’en penses-tu @elois, toi qui viens de refondre celle de Dunitrust ?

Perso je trouve que la difficulté n’est pas la couche d’accès aux données mais le revert d’un bloc.
C’est très délicat d’appliquer un bloc a l’envers et ça peut entraîner des anomalies très difficilement décelables, j’ai déjà eu une anomalie sur Dunitrust a cause d’un bout de code dans le revert d’un bloc.

Peut-être qu’il serait pertinent de spécifier dans le protocole l’algo de revert afin qu’on s’assure que la manière de revert respecte l’état des index du protocole :slight_smile:

2 Likes

Je suis pas certain d’avoir compris toutes les subtilités mais merci pour ces explications. :slight_smile:

Ben ça par contre c’est extrêmement simple : supprimer des index toutes les entrées dont le written_on = le blockstamp du bloc dépilé.

Sauf qu’aucune implémentation du protocole n’est faite directement dans son langage, mais optimise en utilisant sa propre logique. C’est cette partie qui est effectivement délicate, et pour Duniter c’est dans la couche d’accès aux données. D’où ma remarque :slight_smile:

1 Like

Ha ok, conceptuellement je fonctionne différemment. Je ne place pas d’intelligence métier dans la couche d’accès au donnée (ou vraiment très très peu et seulement des traitements très basiques), tout est commandé par les appelants :slight_smile: