Skip to content

feat(sync-v2): Improve sync-v2 reliability#843

Merged
msbrogli merged 1 commit intomasterfrom
feat/sync-v2-improvements
Nov 3, 2023
Merged

feat(sync-v2): Improve sync-v2 reliability#843
msbrogli merged 1 commit intomasterfrom
feat/sync-v2-improvements

Conversation

@msbrogli
Copy link
Member

@msbrogli msbrogli commented Oct 27, 2023

Motivation

The previous organization of responsibilities within sync-v2's internal methods was ambiguous. This Pull Request enhances the clarity by:

  1. Distinctly delineating each method's responsibilities.
  2. Standardizing the point at which we determine if blocks are synchronized.
  3. Designating a consistent time to ask the peer to transmit vertices in real-time.

This refactoring aims to streamline the process, reduce potential errors, and make the codebase more maintainable.

Acceptance Criteria

  1. Change find_best_common_block() to just find the best common block. So it will not set sync state and update relay anymore.
  2. Simplify logic to decide whether we have something to sync or not.
  3. Move the logic to set sync state and to enable/disable real-time relay to the run_sync_blocks() method.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@msbrogli msbrogli self-assigned this Oct 27, 2023
@msbrogli msbrogli requested a review from jansegre as a code owner October 27, 2023 22:45
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements branch from aa8f709 to 3c04ac0 Compare October 27, 2023 22:46
@codecov
Copy link

codecov bot commented Oct 28, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c47f06e) 85.01% compared to head (67d5ce4) 85.04%.
Report is 1 commits behind head on master.

❗ Current head 67d5ce4 differs from pull request most recent head 98121c4. Consider uploading reports for the commit 98121c4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #843      +/-   ##
==========================================
+ Coverage   85.01%   85.04%   +0.02%     
==========================================
  Files         271      271              
  Lines       22368    22368              
  Branches     3420     3418       -2     
==========================================
+ Hits        19017    19022       +5     
+ Misses       2675     2671       -4     
+ Partials      676      675       -1     
Files Coverage Δ
hathor/p2p/sync_v2/agent.py 75.64% <92.59%> (+0.43%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msbrogli msbrogli force-pushed the fix/sync-v2-n-ary-search branch 4 times, most recently from 5c9fed6 to f5d6e6b Compare October 30, 2023 18:36
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements branch 2 times, most recently from 9092163 to 438a81a Compare October 30, 2023 18:51
@msbrogli msbrogli requested a review from glevco October 30, 2023 18:55
glevco
glevco previously approved these changes Nov 1, 2023
jansegre
jansegre previously approved these changes Nov 1, 2023
@msbrogli msbrogli force-pushed the fix/sync-v2-n-ary-search branch 2 times, most recently from dcb0e77 to c47f06e Compare November 3, 2023 03:20
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements branch from 438a81a to 67d5ce4 Compare November 3, 2023 03:22
Base automatically changed from fix/sync-v2-n-ary-search to master November 3, 2023 15:46
@msbrogli msbrogli dismissed stale reviews from jansegre and glevco November 3, 2023 15:46

The base branch was changed.

@msbrogli msbrogli force-pushed the feat/sync-v2-improvements branch from 67d5ce4 to 98121c4 Compare November 3, 2023 15:48
@msbrogli msbrogli merged commit 98121c4 into master Nov 3, 2023
@msbrogli msbrogli deleted the feat/sync-v2-improvements branch November 3, 2023 16:22
@jansegre jansegre mentioned this pull request Nov 13, 2023
2 tasks
This was referenced Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants