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-v2): Wait for sync internal methods to finish before initiating next syncing cycle #845

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

msbrogli
Copy link
Member

@msbrogli msbrogli commented Oct 31, 2023

Motivation

Previously, the _run_sync() method checked for ongoing mempool syncs or open streamings to prevent concurrent cycle runs. Despite the presence of the _is_running: bool flag, it was prematurely marked as false due to mempool syncs and streamings returning at the start, not the end, of their execution. This led to complexity and potential errors in _run_sync(). This PR streamlines this process for better clarity and reliability.

Acceptance Criteria

  1. Create the deferred _deferred_blockchain_streaming when we request the peer to open a block streaming.
  2. Resolve the _deferred_blockchain_streaming when the streaming ends.
  3. Create the deferred _deferred_transactions_streaming when we request the peer to open a transactions streaming.
  4. Resolve the _deferred_transactions_streaming when the streaming ends.
  5. Change run_sync_transactions() to wait for its streaming to end.
  6. Change run_sync_blocks()to wait for its streaming to end.
  7. Change mempool_manager.run() to return a deferred that is resolved when the execution is complete.

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 requested a review from glevco October 31, 2023 20:15
@msbrogli msbrogli self-assigned this Oct 31, 2023
@msbrogli msbrogli requested a review from jansegre as a code owner October 31, 2023 20:15
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements-2 branch 2 times, most recently from 681e60d to 30cc9af Compare October 31, 2023 20:45
hathor/p2p/sync_v2/agent.py Show resolved Hide resolved
hathor/p2p/sync_v2/mempool.py Outdated Show resolved Hide resolved
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements branch from 438a81a to 67d5ce4 Compare November 3, 2023 03:22
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements-2 branch 3 times, most recently from b547f76 to 708b5a8 Compare November 3, 2023 05:24
glevco
glevco previously approved these changes Nov 3, 2023
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements branch from 67d5ce4 to 98121c4 Compare November 3, 2023 15:48
Base automatically changed from feat/sync-v2-improvements to master November 3, 2023 16:22
@msbrogli msbrogli dismissed glevco’s stale review November 3, 2023 16:22

The base branch was changed.

@msbrogli msbrogli force-pushed the feat/sync-v2-improvements-2 branch from 708b5a8 to 38158b6 Compare November 3, 2023 16:24
glevco
glevco previously approved these changes Nov 3, 2023
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements-2 branch from 38158b6 to 92bde24 Compare November 3, 2023 17:51
jansegre
jansegre previously approved these changes Nov 3, 2023
hathor/p2p/sync_v2/mempool.py Outdated Show resolved Hide resolved
hathor/p2p/sync_v2/mempool.py Show resolved Hide resolved
@msbrogli msbrogli dismissed stale reviews from jansegre and glevco via 4b04c9c November 3, 2023 18:07
@msbrogli msbrogli force-pushed the feat/sync-v2-improvements-2 branch from 92bde24 to 4b04c9c Compare November 3, 2023 18:07
@msbrogli msbrogli merged commit 4b04c9c into master Nov 3, 2023
@msbrogli msbrogli deleted the feat/sync-v2-improvements-2 branch November 3, 2023 19:11
@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