ĞDev: runtime-103

The runtime-103 has just been published :partying_face:

Below you will find a french explanation of the bugs and their fixes.

Release notes

Onchain proposal

Proposal hash: 0x0806476db0f7e844c63fa63047d2524cb1c2f159086f2bbb838dba9d53ebadd1

Proposal screen:

Verification of the proposal

  1. Checkout git tag runtime-103
  2. Build the runtime with srtool v0.9.20, see instructions here: docs/dev/verify-runtime-code.md · master · nodes / rust / Duniter v2S · GitLab
  3. Recompute the hash of the proposal, it’s exactly the hash oh the call that is proposed, so it’s: blake2_256(SCALE(upgradeOrigin.dispatchAsRoot(system.setCode(bytecode), 1)))
  4. Verify the changes in the source code

Changes

  • fix(account):if new account can’t pay NewAccountTax, it must be removed (!60)
  • fix(wot): Identity should be removed after the consumers of the identity & membership revocation should not trigger identity removal (!61)
  • fix(scheduler): impl PreimageProvider 3761fd38

Vote in favor or against this proposal

All (black)smith members are asked to vote, to do so, use the call smithsCollective.vote():

State inconsistency

These two bugs cause inconsistent state. After the runtime upgrade, a new proposal will be submitted containing a batch that will set the impacted storage entries.

2 Likes

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

@cgeek @HugoTrentesaux @tuxmain @poka @1000i100 @vit @kapis merci de voter pour ce nouveau runtime dès que vous pouvez.

J’ai essayé de détailler chacun des 2 bug et leur correctif, mais je n’ai malheureusement pas le temps de rentrer dans tous les détails à l’écrit, il y a beaucoup trop de choses à expliquer.

Je vous invite vraiment à relire mes commits pour comprendre chaque correctif, puis à demander ici ce qui vous ne comprenez pas :slight_smile:

Ces 2 bugs génèrent des états inconsistants, donc après le runtime upgrade je vais devoir lister toutes les données incorrectes et proposer l’exécution d’un batch qui corrigera le storage (donc il faudra revoter) :stuck_out_tongue:

4 Likes

Je regarde cela dans 1h.

2 Likes

Bon j’ai voté par confiance (et vu les faibles risques sur de la Ğdev), mais je n’ai pas pu contrôler correctement le vote car je n’ai pas (plus ?) accès à plusieurs calls de smithCollective, notamment : smithCollective.proposalOf et smithCollective.voting. Ces calls n’apparaissent plus dans Polkadotjs, je ne sais pas l’expliquer.

1 Like

Ce ne sont pas des call mais des storage item, il faut changer de “vue” (chain state au lieu de extrinsics) :slight_smile:

1 Like

Au fait je crois qu’on devra définir un quota plus faible pour les monnaies de test, surtout pour le réseau qui reçoit les nouveaux développements en premier, car c’est forcément celui qui reçoit plus de hotfix donc plus de runtime upgrades.

Je ne souhaite pas laisser trainer cet upgrade car plus il traîne plus il y aura de données inconsistantes dans le storage, ce qui va faire plus de boulot derrière pour rattraper les données. Je me permets donc exceptionnellement de voter à la place de @HugoTrentesaux et @1000i100 pour faire passer la proposition :slight_smile:

4 Likes

Le runtime upgrade s’est bien passé, j’ai testé un scénario qui produisait le bug1, et le bug ne se produit plus :partying_face:

J’ai versé 4 ĞD sur un compte, attendu 1 bloc qu’il soit détruit, puis versé 5ĞD sur le même compte, et les frais de création de compte ont bien été prélevés !

Reste à retester la révocation explicite, pour cela je viens de créer une nouvelle identité Elois3.

5 Likes