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

P2P sync improvements #1215

Merged
merged 18 commits into from
May 31, 2023
Merged

P2P sync improvements #1215

merged 18 commits into from
May 31, 2023

Conversation

heifner
Copy link
Member

@heifner heifner commented May 26, 2023

  • Add new option sync-peer-limit to limit the number of peers to sync from. Default 3.
  • Track latency of peers and only sync from sync-peer-limit with lowest latency.
  • Change default sync-fetch-span from 100 to 1000.
  • Add new network protocol version to include start/end range in notice_message. Only attempt to sync from peers with range of required blocks.
  • Fix: do not save incoming trxs into unapplied queue while syncing.

Resolves #1072

@heifner heifner added documentation Improvements or additions to documentation OCI Work exclusive to OCI team labels May 26, 2023
@heifner heifner requested review from greg7mdp and linh2931 May 26, 2023 21:25
@greg7mdp
Copy link
Contributor

greg7mdp commented May 28, 2023

I was wondering why we use these 3 states:

      std::atomic<bool>       connecting{true};
      std::atomic<bool>       syncing{false};
      std::atomic<bool>       closing{false};

instead of having a single state such as:

enum class connection_state { connecting, connected, syncing, synced, closing, closed  };

Can we be in multiple states at once?

@heifner
Copy link
Member Author

heifner commented May 30, 2023

I was wondering why we use these 3 states:

      std::atomic<bool>       connecting{true};
      std::atomic<bool>       syncing{false};
      std::atomic<bool>       closing{false};

instead of having a single state such as:

enum class connection_state { connecting, connected, syncing, synced, closing, closed  };

Can we be in multiple states at once?

The net_plugin could do for a refactor to use a state machine. This looks like low-hanging fruit to me. I'll go ahead and clean that up.

plugins/net_plugin/net_plugin.cpp Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Show resolved Hide resolved
@BenjaminGormanPMP BenjaminGormanPMP requested a review from vladtr May 30, 2023 21:06
…ool variables. Rename syncing to peer_syncing_from_us. Rename syncing_with_peer() to syncing_from_peer().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2P Improve sync behavior
3 participants