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

fix(sync-v2): Fix issues caused by concurrent syncing peers #872

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

msbrogli
Copy link
Member

@msbrogli msbrogli commented Nov 11, 2023

Motivation

Sync-v2 had some issues when syncing with multiple peers. It was causing some tests with three or more peers to be flaky. Notice that peers could still sync but it was taking longer because of the reconnections.

Acceptance Criteria

  1. Fix on_block_complete() to handle vertices that already exists. It might happen because other peer had added these vertices before the current peer.
  2. Fix BlockchainStreamingServer.send_next() which was sending one block more than the limit.
  3. Fix TransactionStreamingClient.process_tx() which was not correctly handling dependencies that already exists in the storage. This condition might happen because of a resume or because other peer had added these dependencies before the current peer.
  4. Always add vertex to _existing_deps if it already exists. This is necessary because the streaming server does not know we already have it so it will always send the whole sub-DAG of transactions.
  5. Improve logging.

@msbrogli msbrogli added the bug Something isn't working label Nov 11, 2023
@msbrogli msbrogli requested a review from glevco November 11, 2023 00:03
@msbrogli msbrogli self-assigned this Nov 11, 2023
@msbrogli msbrogli requested a review from jansegre as a code owner November 11, 2023 00:03
@msbrogli msbrogli force-pushed the fix/sync-v2-concurrency branch from c9dfc8d to 22b2093 Compare November 11, 2023 00:11
@msbrogli msbrogli changed the title fix(sync-v2): Fix issues caused by reorg during sync fix(sync-v2): Fix issues caused by concurrent syncing peers Nov 11, 2023
jansegre
jansegre previously approved these changes Nov 11, 2023
@msbrogli msbrogli force-pushed the fix/sync-v2-concurrency branch from 844be5a to 59b291c Compare November 11, 2023 00:24
Copy link

codecov bot commented Nov 11, 2023

Codecov Report

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

Comparison is base (090bd71) 85.42% compared to head (59b291c) 85.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #872      +/-   ##
==========================================
- Coverage   85.42%   85.37%   -0.06%     
==========================================
  Files         281      281              
  Lines       22353    22351       -2     
  Branches     3388     3387       -1     
==========================================
- Hits        19096    19083      -13     
- Misses       2582     2590       +8     
- Partials      675      678       +3     
Files Coverage Δ
hathor/p2p/sync_v2/agent.py 80.15% <100.00%> (-0.41%) ⬇️
hathor/p2p/sync_v2/streamers.py 86.59% <100.00%> (+0.30%) ⬆️
hathor/p2p/sync_v2/transaction_streaming_client.py 86.61% <88.88%> (-0.80%) ⬇️

... and 6 files with indirect coverage changes

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

@jansegre jansegre mentioned this pull request Nov 13, 2023
2 tasks
@msbrogli msbrogli merged commit 59b291c into master Nov 13, 2023
8 of 9 checks passed
@msbrogli msbrogli deleted the fix/sync-v2-concurrency branch November 13, 2023 19:29
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
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants