WIP: sync print 1 block apply when no blocks applied
Merge request reports
Activity
@librelois doit-on ajouter le Cargo.lock au git, ou au contraire l'ajouter dans le .gitignore ?
@librelois quand aucun bloc n'était appliqué, la synchronisation affichait quand même "1 bloc appliqué". Je me suis permis de corriger ça, est-ce que ça te semble juste ?
209 209 let sync_start_time = SystemTime::now(); 210 210 211 211 // Count number of blocks and chunks 212 let count_blocks = target_blockstamp.id.0 + 1 - current_blockstamp.id.0; 213 let count_chunks = if count_blocks % 250 > 0 { 214 (count_blocks / 250) + 1 215 } else { 216 count_blocks / 250 217 }; 212 let count_blocks = target_blockstamp.id.0 - current_blockstamp.id.0; -
Est-ce que tu es sûr qu'on se retrouve avec une panique ? En Julia, le comportement d'overflow aboutit à des nombres très grands : https://docs.julialang.org/en/v1/manual/integers-and-floating-point-numbers/#Overflow-behavior-1
Si je te le dit c'est que j'en suis certain oui, j'en ai déjà fait les frais, le Rust n'est pas le Julia ;) D'ailleurs c'est cohérent avec la logique de Rust => interdire les comportement "bizarres" => donc au lieu d'aboutir a des nombres très grands qui pourraient provoquer des comportements imprévisibles et difficiles a débuger, la ça panic et au moins on sait tout de suite ou est la cause.
209 209 let sync_start_time = SystemTime::now(); 210 210 211 211 // Count number of blocks and chunks 212 let count_blocks = target_blockstamp.id.0 + 1 - current_blockstamp.id.0; 213 let count_chunks = if count_blocks % 250 > 0 { 214 (count_blocks / 250) + 1 215 } else { 216 count_blocks / 250 217 }; 212 let count_blocks = target_blockstamp.id.0 - current_blockstamp.id.0; 213 let count_chunks = count_blocks / 250 + 1; -
Faux : un chunk pour les blocs de 0 à 249, et un seul bloc dans le chunk n°2
Non le bloc n°0 compte, donc si j'ai 250 blocs a sync, ça va du n°0 au n°249 = un seul chunk. En fait j'avais pensé la formule pour une sync initiale sans blockchain. Et du coup
current_blockstamp.idvaut zéro alors qu'on a zéro block, il devrait donc valoir -1 ou None mais du coup on ne pourrait plus utiliser un u32. Il faut remettre le +1 dans les cas ou le nœud n'a aucun block (cad quandcurrent_blockstamp == Blockstamp::default()).En fait, c'est même pire que ça, ça dépend de la valeur de
current_blockstamp. Par exemple si je suis au bloc 200 et que le target est 550 j'ai 3 chunks a récupérer : les chunks 0, 1 et 2 ! En fait il faudrait étudier comment est utilisé la variablecount_chunkspour voir ce qui est exactement attendu actuellement. Mais en tout les cas a terme un récupérera des chunks fixes donc faudra changer la formule de calcul.Je regarderai ça dés que possible, mais probablement que lundi car je suis en déplacement ce WE :)
-
D'accord, dans ce cas, il faut juste regarder sur combien de chunks on est. Est-ce qu'il ne vaudrait pas mieux faire une fonction qui retourne un itérable qui permet de parcourir les blocs sans se soucier des chunks ? La fonction s'occuperait elle même de lire les chunks nécessaire pour fournir l'itérateur demandé? Et cet itérable pourrait être produit sois par une fonction de synchronisation locale sois par une fonction de synchronisation distante.
Je regarderai ça dés que possible, mais probablement que lundi car je suis en déplacement ce WE :)
Idem, ce weekend, je pars avec Lucie
-
D'accord, dans ce cas, il faut juste regarder sur combien de chunks on est.
Il faut surtout étudier comment est utilisé la variable
count_chunksactuellement pour s'assurer de ne pas produire de régression.Est-ce qu'il ne vaudrait pas mieux faire une fonction qui retourne un itérable qui permet de parcourir les blocs sans se soucier des chunks ? La fonction s'occuperait elle même de lire les chunks nécessaire pour fournir l'itérateur demandé?
On ne pourra pas faire ça car dans le cas d'une sync via le réseau on recevra les chunks en asynchrone et pas forcément dans l'ordre. C'est la fonction de récupération des chunks qui gèrera son cache pour fournir ensuite les blocs dans l'ordre a traver le channel Rust. La fonction de sync écoutera sur un channel rust pour recevoir les blocks, c'est déjà le comportement actuel dans une sous-partie de local_sync() donc c'est le choix de refactoring le plus pertinent par rapport a l'existant ;) En fait ce code que tu modifie sera déplacé dans la fonction qui récupère les blocs car cela dépens du target. Il faudra 2 channel entre les 2 (1 dans chaque sens). C'est le starter qui donnera les bon receiver et sender a chacun, c'est un refactoring un peu compliqué et sensible je préfèrerai le faire moi. Du coup pour cette MR reste iso avec l'existant stp :)
Edited by Éloïs
-
@HugoTrentesaux review:
Le Cargo.lock doit etre dans le dépot git, si je ne l'ai jamais mi dans le gitignore depuis le temps ce n'est pas pour rien ;)
Il permet de savoir exactement la version de chaque dépendance, si une mise a jours mineure d'une dépendance provoque une régression on aura besoin des diff du Cargo.lock pour savoir quel upgrade a causer la régréssion :)
-
Par contre ici la formule deviens fausse, il faut garder l'ancienne formule. Si j'ai 250 blocs j'ai bien 1 seul chunk et pas 2 !
Faux : un chunk pour les blocs de 0 à 249, et un seul bloc dans le chunk n°2
Est-ce que tu es sûr qu'on se retrouve avec une panique ? En Julia, le comportement d'overflow aboutit à des nombres très grands : https://docs.julialang.org/en/v1/manual/integers-and-floating-point-numbers/#Overflow-behavior-1
Le Cargo.lock doit etre dans le dépot git
Dans ce cas, mon commit ffe85d18 est aussi utile, non ?
-
@HugoTrentesaux merci de répondre sous les commentaires de code :) Gitlab offre une fonctionalité de "résolution de commentaires" que j'utilise pour ne pas avoir besoin de tout recheker a chaque fois, ça me fait perdre un temps précieux si tu répond aux commentaires dans la discussion générale.
En cas de question tu peut créer toi même un commentaire sous une ligne de ton propre code :)
La discution générale sert a dire des choses plus "générales" sur toute la MR ou a tagger le reviewer quand tu veut qu'il repasse ;)
Edited by Éloïs 1 # This file is automatically @generated by Cargo. 2 # It is not intended for manual editing. -
Dans ce cas, mon commit ffe85d18 est aussi utile, non ?
Il faudrait voir si cargo supporte ces commentaires en début de fichier et qu'il ne les suppriment pas. Pour ça tu peut upgrader une dépendance pour tester puis recompiler et voir ;)
-
Ce n'est pas une question d'avis, ma remarque consistait juste a dire "il faut faire le test pour vérifier". A tu testé en modifiant une dépendance dans un cargo.toml puis en recompilant ?
EDIT: De préférence un sujet par MR. Ca va pour cette fois mais a l'avenir si tes modifs impliquent plusieurs sujets qui n'ont rien a voir il faudra faire une MR par sujet ;)
Edited by Éloïs