-
Notifications
You must be signed in to change notification settings - Fork 790
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
Bootstrap tuning #4806
Bootstrap tuning #4806
Conversation
Test Results for Commit 6924d54Pull Request 4806: Results Test Case Results
Last updated: 2024-12-10 13:42:21 UTC |
// Do not throttle accounts that are probably fully synced | ||
if (sent && fails == 0) | ||
{ | ||
nano::lock_guard<nano::mutex> lock{ mutex }; | ||
accounts.timestamp_set (account); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By setting the timestamp to current value ( accounts.timestamp_set (account);
) a cooldown period in next_priority ()
is applied to this account. Is that the desired behavior ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work as intended. It might be counterintuitive because we want to throttle only accounts with potentially more blocks; it's to avoid requesting blocks from the same frontier over and over before the current batch is processed by block processor.
The flow is something like:
if (likely to have more blocks)
{
set cooldown timestamp
}
// ... later in the block processing logic
if (last received block processed)
{
reset cooldown timestamp
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that makes sense.
I got confused by the comment
// Do not throttle accounts that are probably fully synced
So if I understand correctly,the goal is to rapidly deprioritise fully synced accounts by requesting again without throttle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I updated the comment to make the intention clear
@@ -204,8 +206,24 @@ bool nano::bootstrap_service::send (std::shared_ptr<nano::transport::channel> co | |||
|
|||
// TODO: There is no feedback mechanism if bandwidth limiter starts dropping our requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this now handled by the added callback ? Or is the TODO about something else ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This todo is now out of date
fefadf2
to
6924d54
Compare
This fixes a few more problems with bootstrap that I noticed when working on traffic shaping.