En effet on peut simplifier
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
En effet on peut simplifier
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
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.
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
Oui ce serait bien. Avec la version que tu souhaites, les deux solutions proposées étant correctes a priori.
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 ?
-- 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)
?
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
@Moul non ce n’est pas ça car le seul appel a cette fonction ce fait avec la valeur zéro pour expired_on
It’s done
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 :
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.
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.
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 :
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
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 :
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 :
Pour résumer il y avait bien 2 bugs ici :
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 . Pourtant le CINDEX affiché est toujours bon, mais c’est en interne que la BDD est incohérente.
Comme indiqué dans le ticket du bug#1396 :
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.
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).
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 J’espère que ton analyse permettra a d’autres développeurs de tenter le correctif la prochaine fois !
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
Je suis pas certain d’avoir compris toutes les subtilités mais merci pour ces explications.
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
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