ĞDev: runtime-103

Ce runtime-103 corrige 2 bugs:

Bug1: bug: sometimes the account creation tax is not applied (#60) · Issues · nodes / rust / Duniter v2S · GitLab
Bug2: bug: identity explicitly revoked stay in the ud accounts storage (#57) · Issues · nodes / rust / Duniter v2S · GitLab

Chacun de ces bugs ont d’abord été reproduits par un test automatisé avant d’être corrigés :slight_smile:

Bug 1: Les frais de création de compte parfois non appliqués

C’était de loin le bug le plus difficile à identifier, il était causé par le fait que dans certain cas le compte était supprimé sans être vraiment supprimé.

La confusion vient du fait que les comptes sont stockés par la pallet system dans une StorageMap de type « ValueQuery ». Ce qui signifie qu’une valeur est toujours retournée (la valeur par défaut si l’entrée n’existe pas).
Lorsque de la monnaie est versée sur un nouveau compte, s’il n’a pas de quoi payer les frais de création de compte, il est détruit, sauf qu’au lieu de détruire le compte, je me contentais de le modifer à sa valeur par défaut, plus précisément je forçais le solde à zéro: pallets/duniter-account/src/lib.rs · runtime-102 · nodes / rust / Duniter v2S · GitLab.

Cela pose deux problèmes:

  1. La valeur reste dans le storage, ce qui ne sert à rien puisse-que c’est la valeur par défaut, ça pollue donc le storage pour rien.
  2. Lorsque de la monnaie est versée sur un compte, pour savoir s’il s’agit d’un nouveau compte, je vérifie son existence avec la méthode account_exists fournie par la pallet system: pallets/duniter-account/src/lib.rs · runtime-102 · nodes / rust / Duniter v2S · GitLab. Le problème, c’est que cette méthode n’est pas consistante avec le comportement de ValueQuery, dans le sens ou l’existence ou non du compte ne correspond pas à «ce compte a-t-il une valeur différente de la valeur par défaut» mais à «existe t’il réellement une entrée dans le storage»., ce qui n’est pas conforme à l’esprit de ValueQuery…

Bref, le fix est extrêmement simple, mais j’ai vraiment galéré pour identifier la cause, j’ai dû implémenter try-runtime en urgence dans notre codebase (bien plus tot que prévu), pour pouvoir rejouer localement l’un des blocs problématique, afin d’identifier pourquoi il ne se passait rien.

Le test: fix(account):if new account can't pay NewAccountTax, it must be removed (!60) · Merge requests · nodes / rust / Duniter v2S · GitLab

Le correctif: fix(account):if new account can't pay NewAccountTax, it must be removed (!60) · Merge requests · nodes / rust / Duniter v2S · GitLab

Bug 2: Les identités explicitement révoquées restent dans UdAccountStorage, et donc continuent de créer leur DU

Pour ce 2ème bug c’est l’inverse, j’ai facilement localisé la cause, sans avoir eu besoin de re-exécuter quoi que ce soi, mais une fois la cause identifiée je ne voyais pas comment coder un correctif sans faire un gros refactoring…

C’est ici dans le code que l’on supprime le compte du UdAccountStorage: runtime/common/src/handlers.rs · runtime-102 · nodes / rust / Duniter v2S · GitLab

Le problème, c’est que dans ce contexte on a que le IdtyIndex, on doit donc lire le storage pour trouver la clé publique associée, mais comme l’identité à justement été supprimée plus haut dans la pile des appels, on n’obtient pas de valeur et on ne rentre pas dans le if, donc le compte n’est pas supprimé du UdAccountStorage, et continu donc de co-créer son DU :sweat_smile:

Quand j’ai identifié ce bug hier, je me suis dit, il faut que je passe en plus la clé publique de l’identité à travers toutes les couches, donc gros refactaring, avec un générique en plus, en un mot l’enfer. Si j’avais choisi initialement de ne pas passer la clé publique, il y avait de bonnes raisons.
Hier toujours, j’ai pensé que implémenter la création manuelle du DU ferai disparaître le problème, car plus besoin de supprimer le compte du UdAccountStorage, cette pallet UdAccountStorage n’existera plus.

Mais ce matin j’ai compris 2 chose qui m’avais échappé hier:

  1. Même en passant à la création manuelle du DU, j’aurais quand même besoin de la clé publique de l’identité au moment de sa suppression, pour verser automatiquement les DU non-réclamés.
  2. La bonne solution à ce problème n’est pas de rajouter la clé publique en paramètre dans toutes les couches (gros refactoring), mais c’est de modifier le comportement de la pallet identity pour qu’elle ne supprime l’identité que après la suppression de tout ce qui « dépend » de cette identité (il s’avère que c’est simple à coder, car j’avais déjà défini un handler RemoveIdentityConsumers et il était déjà implémenté avec toute la plomberie nécessaire).

La misconception, c’est que je supprimais l’identité avant l’appel au handler RemoveIdentityConsumers, alors qu’il vaut évidemment la supprimée après.

Le test qui reproduit le bug: fix(wot): Identity should be removed after the consumers of the identity & membership revocation should not trigger identity removal (!61) · Merge requests · nodes / rust / Duniter v2S · GitLab

Le correctif: fix(wot): Identity should be removed after the consumers of the identity & membership revocation should not trigger identity removal (!61) · Merge requests · nodes / rust / Duniter v2S · GitLab

Ce correctif à fait échouer d’autres tests, qui réussissait auparavant, me révélant ainsi d’autre « misconceptions » dans mon code, qui était « caché » par le 1er problème. C’est pourquoi il y a plusieurs autres commit (dans la MR) pour les corriger :slight_smile:

5 Likes