Skip to content
Snippets Groups Projects

WIP: Resolve "Add block checking (checking all rules of the unit protocol)"

WIP: Resolve "Add block checking (checking all rules of the unit protocol)"
tuxmain/84-add-block-checking-checking-all-rules-of-the-unit-protocol into dev
3 unresolved threads
Closed Pascal Engélibert requested to merge tuxmain/84-add-block-checking-checking-all-rules-of-the-unit-protocol into dev
3 unresolved threads

Closes #84

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Éloïs
  • Éloïs
    Éloïs @librelois started a thread on an old version of the diff
    • lib/modules/blockchain/blockchain/src/dubp/check/mod.rs
    113 }
    114 if issuers.len() != block.issuers_count {
    115 return Err(invalid_rule_error(InvalidRuleError::WrongIssuersCount));
    116 }
    117
    118 //BR_G05 - issuers frame
    119 if block.issuers_frame
    120 != previous_block.issuers_frame
    121 + match previous_block.issuers_frame_var.cmp(&0) {
    122 Ordering::Less => -1,
    123 Ordering::Greater => 1,
    124 Ordering::Equal => 0,
    125 }
    126 {
    127 return Err(invalid_rule_error(InvalidRuleError::WrongIssuersFrame));
    128 }
  • Éloïs
    Éloïs @librelois started a thread on an old version of the diff
    • lib/modules/blockchain/blockchain/src/dubp/check/mod.rs
    115 return Err(invalid_rule_error(InvalidRuleError::WrongIssuersCount));
    116 }
    117
    118 //BR_G05 - issuers frame
    119 if block.issuers_frame
    120 != previous_block.issuers_frame
    121 + match previous_block.issuers_frame_var.cmp(&0) {
    122 Ordering::Less => -1,
    123 Ordering::Greater => 1,
    124 Ordering::Equal => 0,
    125 }
    126 {
    127 return Err(invalid_rule_error(InvalidRuleError::WrongIssuersFrame));
    128 }
    129 }
    130 // Only rules that do concern genesis block
    • Éloïs
      Éloïs @librelois
      Owner

      En terme d'organisation du code, je me dit que finalement c'est peut être mieux d'inverser et de traiter avant les cas du block genesis, voir même de créer une fonction a part pour les vérifications du bloc genesis, ça améliorerait la lisibilité :)

    • Pascal Engélibert
      Pascal Engélibert @tuxmain
      Author Developer

      Pour verify_genesis_block_validity, est-ce qu'il est vraiment nécessaire de mettre les DBs en argument, puisque logiquement tous les indexes sont vides avant le bloc 0 ? (c'est là que BR_G100 me semble bizarre : l'issuer du bloc 0 ne peut pas déjà être membre)

    • Éloïs
      Éloïs @librelois
      Owner

      @tuxmain en effet il n'y a pas besoin des DBs pour vérifier le bloc genesis.

      Concernant la BR_G100, l'issuer du bloc genesis doit faire parti des membres déclarés dans le bloc genesis ;)

    • Pascal Engélibert changed this line in version 2 of the diff

      changed this line in version 2 of the diff

    • Please register or sign in to reply
  • Pascal Engélibert added 14 commits

    added 14 commits

    Compare with previous version

  • Pascal Engélibert added 3 commits

    added 3 commits

    • f243dcb6 - [feat] blockchain: check rules BR_G99->04
    • fa749e06 - [ref] separate checks for block 0
    • a48dedcc - [tests] blockchain: test block verification

    Compare with previous version

  • Éloïs changed milestone to %v1.0 full member node

    changed milestone to %v1.0 full member node

  • Éloïs
  • Éloïs
  • Éloïs
  • Éloïs
    Éloïs @librelois started a thread on the diff
    • lib/modules/blockchain/blockchain/src/dubp/mod.rs
    98 &bc.wot_databases.certs_db,
    99 &bc.wot_index,
    100 &bc.wot_databases.wot_db,
    101 )?;
    95 if block_doc.number.0 == 0 {
    96 verify_genesis_block_validity(&block_doc)?;
    97 } else {
    98 verify_block_validity(
    99 &block_doc,
    100 &bc.blocks_databases.blockchain_db,
    101 &bc.wot_databases.identities_db,
    102 &bc.wot_databases.certs_db,
    103 &bc.wot_index,
    104 &bc.wot_databases.wot_db,
    105 )?;
    106 }
    • Éloïs
      Éloïs @librelois
      Owner

      La fonction verify_block_validity() commence a avoir trop de paramètres, tu peut en supprimer 2 en lui envoyent directement une référence vers bc.wot_databases, je te laisse modifier les signatures en conséquence ;)

    • Pascal Engélibert
      Pascal Engélibert @tuxmain
      Author Developer

      La fonction prend du coup l'argument wot_databases: &WotsV10DBs, mais maintenant j'ai cette erreur :

      error[E0283]: type annotations required: cannot resolve `_: durs_wot::data::WebOfTrust`
  --> lib/modules/blockchain/blockchain/src/dubp/mod.rs:98:13
   |
98 |             verify_block_validity(
   |             ^^^^^^^^^^^^^^^^^^^^^
   |
note: required by `dubp::check::verify_block_validity`
  --> lib/modules/blockchain/blockchain/src/dubp/check/mod.rs:77:1
   |
77 | / pub fn verify_block_validity<W: WebOfTrust>(
78 | |     block: &BlockDocument,
79 | |     blockchain_db: &BinDB<LocalBlockchainV10Datas>,
80 | |     wot_databases: &WotsV10DBs,
...  |
142| |     Ok(())
143| | }
   | |_^

      Si je comprends bien, c'est que la fonction verify_block_validity ne respecte pas le trait WebOfTrust. Ce que je ne comprends pas c'est pourquoi l'erreur pointe l'appel de la fonction et pas sa déclaration, et pourquoi la fonction avait ce trait. D'ailleurs si j'enlève <W: WebOfTrust> ça passe.

    • Éloïs
      Éloïs @librelois
      Owner

      @tuxmain c'est parce que le compilateur rust n'arrive pas a inférer le type de W.

      Ce type générique sert pour l'argument wot_db qui est de type &BinDB<W>.

      Maintenant que tu passe la wot_db a l'intérieur de l'objet WotsV10DBs, cet objet n'est pas générique, il impose une implémentation spécifique du trait WebOfTrust, laquelle a t'on avis ?

      Du coup c'est tout a fait normal que ça marche en supprimant la généricité sur ta fonction, ce que je veut c'est que tu comprenne le pourquoi du comment :)

    • Please register or sign in to reply
  • Éloïs
    Éloïs @librelois
    Owner

    @tuxmain dans l'ensemble c'est plutôt bien, je vois que tu a progréssé en rust ça fait très plaisir :)

    Concernant les tests il faudrait en réalité 1 test par règle. Pour éviter d'avoir un fichier avec des dizaines de milliers de lignes voir plus, il faudrait 1 fichier par règle. Chaque règle sera évaluée dans une fonction inline afin de ne pas alourdir la stack a l'éxécution (sauf pour les quelques règles les plus complexes a évaluer).

    Et en effet, comme tu me l'a dit en privé, je te confirme qu'il faut aussi tester le cas d'un bloc invalide (et ceux pour chaque règle).

    Ce que je te dit la n'est pas valable pour le block genesis, c'est un bloc spécial, ça peut être pertinent de garder un seul fichier pour les vérifications de ce block la.

  • Éloïs added 19 commits

    added 19 commits

    Compare with previous version

    Toggle commit list
  • Éloïs
    Éloïs @librelois
    Owner

    @tuxmain j'ai mis a jours ta branche sur dev, je me suis contenté de résoudre les conflits en modifiant le moins possible ton code :)

    Ainsi les commentaires non résolues restent d'actualité ;) (Si je résout mes remarques a ta place tu n'apprendra pas).

    Aussi j'ai commencé a coder un moteur de règle pour que l'application du protocole soit plus propre et plus faible, quand je l'aurais fini il faudra que tu réfactor ton code pour utiliser le moteur de règle.

    EDIT: tien en rebasant j'ai oublié de changer tes BlockId par BlockNumber (le nom du type a changé sur dev). Bon c'est que dal je réglerai ça lundi ;)

    Edited by Éloïs
  • Éloïs added 1 commit

    added 1 commit

    • ab078bac - [tests] blockchain: test block verification

    Compare with previous version

  • Éloïs added 5 commits

    added 5 commits

    Compare with previous version

    Toggle commit list
  • Éloïs added 7 commits

    added 7 commits

    Compare with previous version

    Toggle commit list
  • Éloïs added 4 commits

    added 4 commits

    • 85ef562c - 1 commit from branch dev
    • d431c173 - [feat] blockchain: check rules BR_G99->04
    • b626334c - [ref] separate checks for block 0
    • 63a372d9 - [tests] blockchain: test block verification

    Compare with previous version

    Toggle commit list
  • Éloïs
    Éloïs @librelois
    Owner

    @tuxmain voila j'ai fini le moteur de règle, il est sur dev, et je t'ai mis a jours ta branche comme ça tu n'aura pas de conflits a gérer :)

    J'aimerais que tu réfactor entièrement ta contribution en utilisant le rules engine, aussi sépare la vérification en 2 parties : local et globale.

    je t'ai donné un exemple de ce que j'attends sur la branche elois/use-rule-engine-example :)

    Edited by Éloïs
  • Pascal Engélibert added 6 commits

    added 6 commits

    Compare with previous version

    Toggle commit list
  • Éloïs closed

    closed

    • Please register or sign in to reply