Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sync): boilerplate for sync-v1.1 protocol #583

Merged
merged 1 commit into from
May 10, 2023

Conversation

jansegre
Copy link
Member

@jansegre jansegre commented May 8, 2023

This is a complimentary PR to #581

The reasoning is to create a barrier between peers using the new sync code. The protocol and implementation are backwards compatible, but a barrier helps ensure that peers that are only running sync-v1.1 (not the bridge peer running both versions) will almost certainly (unless someone is intentionally spoofing the sync version) connect to other peers running sync-v1.1, and that should be of great help to #581

Acceptance Criteria

  • add a new sync-v1.1 version marker that will use the same class as sync-v1.0 (which was just sync-v1 before)
  • disable sync-v1.0 by default, requiring the use of a cli param to enable it
  • enable sync-v1.1 whenever sync-v1.0 would be enabled before
  • builder will keep referencing a single sync-v1 version, which will only enable sync-v1.1 and not sync-v1.0, this is because the class used is exactly the same and there's no need to make the tests aware of this difference

@jansegre jansegre self-assigned this May 8, 2023
@jansegre jansegre requested a review from msbrogli as a code owner May 8, 2023 17:31
msbrogli
msbrogli previously approved these changes May 8, 2023
@jansegre jansegre force-pushed the feat/sync-v1.1-boilerplate branch from 365ba09 to fd8572d Compare May 8, 2023 17:55
@jansegre jansegre requested a review from pedroferreira1 May 8, 2023 17:55
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #583 (c1eaae7) into master (38bdb64) will decrease coverage by 0.01%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
- Coverage   83.47%   83.46%   -0.01%     
==========================================
  Files         221      222       +1     
  Lines       19144    19163      +19     
  Branches     2623     2625       +2     
==========================================
+ Hits        15981    15995      +14     
- Misses       2587     2589       +2     
- Partials      576      579       +3     
Impacted Files Coverage Δ
hathor/p2p/manager.py 67.54% <75.00%> (+0.32%) ⬆️
hathor/builder/builder.py 91.06% <100.00%> (ø)
hathor/builder/cli_builder.py 79.04% <100.00%> (+0.07%) ⬆️
hathor/manager.py 70.97% <100.00%> (ø)
hathor/p2p/sync_v1_1_factory.py 100.00% <100.00%> (ø)
hathor/p2p/sync_version.py 78.94% <100.00%> (-8.56%) ⬇️

... and 2 files with indirect coverage changes

@msbrogli msbrogli requested a review from glevco May 9, 2023 04:31
@msbrogli msbrogli force-pushed the feat/sync-v1.1-boilerplate branch from c1eaae7 to 85e83d2 Compare May 10, 2023 21:45
@msbrogli msbrogli merged commit 85e83d2 into master May 10, 2023
@msbrogli msbrogli deleted the feat/sync-v1.1-boilerplate branch May 10, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants