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

Avoid voting until caught up with network #262

Merged
merged 6 commits into from
Jun 11, 2024
Merged

Conversation

heifner
Copy link
Member

@heifner heifner commented Jun 10, 2024

  • Do not consider voting until within 30 seconds of head block timestamp
  • Do not broadcast votes if syncing from peer

Resolves #259

@heifner heifner added the OCI Work exclusive to OCI team label Jun 10, 2024
@heifner heifner requested a review from linh2931 June 10, 2024 17:14
Comment on lines 104 to 108
if (!enable_voting) { // Avoid extra processing while syncing. Once caught up, consider voting
if (bsp->timestamp() < fc::time_point::now() - fc::seconds(30))
return;
enable_voting = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move before line 97 to avoid extra memory allocation+free when syncing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enable_voting is protected by the mutex.

  • Could make it atomic
  • Could move the .reserve() call after and inside the mutex lock
  • Could leave it as is

Not clear to me which is better. Maybe atomic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think atomic may be best. Even without atomic, it seems that the worse that could happen is for a couple extra votes to go though.

Copy link
Contributor

@greg7mdp greg7mdp Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or move the reserve within the mutex protection, even simpler imo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even better would be to do this check on the main thread before we call consider_voting(bsp, use_thread_pool_t::yes);, so we don't post unneeded tasks on the thread pool.

libraries/testing/tester.cpp Show resolved Hide resolved
@heifner heifner merged commit 6b5687e into main Jun 11, 2024
36 checks passed
@heifner heifner deleted the GH-259-avoid-voting branch June 11, 2024 12:10
heifner added a commit that referenced this pull request Jun 11, 2024
heifner added a commit that referenced this pull request Jun 11, 2024
Do not post to thread pool while syncing/replaying
@ericpassmore
Copy link
Contributor

Note:start
group: STABILITY
category: INTERNALS
summary: Avoid spamming peers with unnecessary votes when nodeos is first syncing old blocks.
Note:end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On startup, avoid voting until caught up with network
4 participants