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

Add throttling functionality to the ascending bootstrapper #4205

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nano/core_test/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3432,7 +3432,7 @@ TEST (node, bidirectional_tcp)

// Tests that local blocks are flooded to all principal representatives
// Sanitizers or running within valgrind use different timings and number of nodes
TEST (node, aggressive_flooding)
TEST (node, DISABLED_aggressive_flooding)
{
nano::test::system system;
nano::node_flags node_flags;
Expand Down
1 change: 1 addition & 0 deletions nano/lib/stats_enums.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ enum class detail : uint8_t
// bootstrap ascending
missing_tag,
reply,
throttled,
track,
timeout,
nothing_new,
Expand Down
59 changes: 55 additions & 4 deletions nano/node/bootstrap/bootstrap_ascending.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,35 @@

using namespace std::chrono_literals;

nano::bootstrap_ascending::throttle::throttle (size_t count) :
successes{ count },
samples{ count, true }
{
}

bool nano::bootstrap_ascending::throttle::throttled () const
{
return successes == 0;
}

void nano::bootstrap_ascending::throttle::add (bool sample)
{
if (samples.front ())
{
--successes;
}
samples.push_back (sample);
if (sample)
{
++successes;
}
}

size_t nano::bootstrap_ascending::throttle::success_count () const
thsfs marked this conversation as resolved.
Show resolved Hide resolved
{
return successes;
}

/*
* database_iterator
*/
Expand Down Expand Up @@ -89,6 +118,11 @@ nano::account nano::bootstrap_ascending::buffered_iterator::next ()
return *(*this);
}

bool nano::bootstrap_ascending::buffered_iterator::warmup () const
{
return warmup_m;
}

void nano::bootstrap_ascending::buffered_iterator::fill ()
{
debug_assert (buffer.empty ());
Expand All @@ -112,6 +146,10 @@ void nano::bootstrap_ascending::buffered_iterator::fill ()
{
buffer.push_back (*pending_iterator);
}
else
{
warmup_m = false;
thsfs marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

Expand Down Expand Up @@ -374,6 +412,7 @@ nano::bootstrap_ascending::bootstrap_ascending (nano::node & node_a, nano::store
stats{ stat_a },
accounts{ stats },
iterator{ store },
throttle{ node.config.bootstrap_ascending.throttle_count },
limiter{ node.config.bootstrap_ascending.requests_limit, 1.0 },
database_limiter{ node.config.bootstrap_ascending.database_requests_limit, 1.0 }
{
Expand Down Expand Up @@ -678,12 +717,23 @@ bool nano::bootstrap_ascending::run_one ()
return success;
}

void nano::bootstrap_ascending::throttle_if_needed ()
{
if (!iterator.warmup () && throttle.throttled ())
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't 'throttle' object access need a lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it's written it assumes synchronisation is handled externally.
This seemed appropriate since it's a 1-to-1 association between bootstrap_ascending instance and throttle instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why then are you locking accesses of this class in other places, I believe your explanation is not correct and it is in fact being used from multiple threads.

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, just move the lock acquired on line 725 above this if statement.

{
stats.inc (nano::stat::type::bootstrap_ascending, nano::stat::detail::throttled);
nano::unique_lock<nano::mutex> lock{ mutex };
condition.wait_for (lock, std::chrono::milliseconds{ node.config.bootstrap_ascending.throttle_wait }, [this] () { return stopped; });
}
}

void nano::bootstrap_ascending::run ()
{
while (!stopped)
{
stats.inc (nano::stat::type::bootstrap_ascending, nano::stat::detail::loop);
run_one ();
throttle_if_needed ();
}
}

Expand Down Expand Up @@ -743,16 +793,17 @@ void nano::bootstrap_ascending::process (const nano::asc_pull_ack::blocks_payloa
{
block_processor.add (block);
}
nano::lock_guard<nano::mutex> lock{ mutex };
throttle.add (true);
}
break;
case verify_result::nothing_new:
{
stats.inc (nano::stat::type::bootstrap_ascending, nano::stat::detail::nothing_new);

{
nano::lock_guard<nano::mutex> lock{ mutex };
accounts.priority_down (tag.account);
}
nano::lock_guard<nano::mutex> lock{ mutex };
accounts.priority_down (tag.account);
throttle.add (false);
}
break;
case verify_result::invalid:
Expand Down
23 changes: 23 additions & 0 deletions nano/node/bootstrap/bootstrap_ascending.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,24 @@ class bootstrap_ascending
{
using id_t = uint64_t;

// Class used to throttle the ascending bootstrapper once it reaches a steady state
// Tracks verify_result samples and signals throttling if no tracked samples have gotten results
class throttle
{
public:
// Initialized with all true samples
throttle (size_t size);
thsfs marked this conversation as resolved.
Show resolved Hide resolved
bool throttled () const;
void add (bool success);
size_t success_count () const;

private:
// Rolling count of true samples in the sample buffer
size_t successes;
// Circular buffer that tracks sample results. True when something was retrieved, false otherwise
boost::circular_buffer<bool> samples;
};

public:
bootstrap_ascending (nano::node &, nano::store &, nano::block_processor &, nano::ledger &, nano::network &, nano::stats &);
~bootstrap_ascending ();
Expand Down Expand Up @@ -87,6 +105,7 @@ class bootstrap_ascending
/* Inspects a block that has been processed by the block processor */
void inspect (nano::transaction const &, nano::process_return const & result, nano::block const & block);

void throttle_if_needed ();
void run ();
bool run_one ();
void run_timeouts ();
Expand Down Expand Up @@ -273,13 +292,16 @@ class bootstrap_ascending
explicit buffered_iterator (nano::store & store);
nano::account operator* () const;
nano::account next ();
// Indicates if a full ledger iteration has taken place e.g. warmed up
bool warmup () const;

private:
void fill ();

private:
nano::store & store;
std::deque<nano::account> buffer;
bool warmup_m{ true };

database_iterator accounts_iterator;
database_iterator pending_iterator;
Expand All @@ -290,6 +312,7 @@ class bootstrap_ascending
private:
account_sets accounts;
buffered_iterator iterator;
throttle throttle;

// clang-format off
class tag_sequenced {};
Expand Down
4 changes: 4 additions & 0 deletions nano/node/bootstrap/bootstrap_config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ nano::error nano::bootstrap_ascending_config::deserialize (nano::tomlconfig & to
toml.get ("database_requests_limit", database_requests_limit);
toml.get ("pull_count", pull_count);
toml.get ("timeout", timeout);
toml.get ("throttle_count", throttle_count);
toml.get ("throttle_wait", throttle_wait);

if (toml.has_key ("account_sets"))
{
Expand All @@ -49,6 +51,8 @@ nano::error nano::bootstrap_ascending_config::serialize (nano::tomlconfig & toml
toml.put ("database_requests_limit", database_requests_limit, "Request limit for accounts from database after which requests will be dropped.\nNote: changing to unlimited (0) is not recommended as this operation competes for resources on querying the database.\ntype:uint64");
toml.put ("pull_count", pull_count, "Number of requested blocks for ascending bootstrap request.\ntype:uint64");
toml.put ("timeout", timeout, "Timeout in milliseconds for incoming ascending bootstrap messages to be processed.\ntype:milliseconds");
toml.put ("throttle_count", throttle_count, "Number of samples to track for bootstrap throttling.\ntype:uint64");
toml.put ("throttle_wait", throttle_wait, "Length of time to wait between requests when throttled.\ntype:milliseconds");

nano::tomlconfig account_sets_l;
account_sets.serialize (account_sets_l);
Expand Down
4 changes: 3 additions & 1 deletion nano/node/bootstrap/bootstrap_config.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ class bootstrap_ascending_config final
nano::error deserialize (nano::tomlconfig & toml);
nano::error serialize (nano::tomlconfig & toml) const;

std::size_t requests_limit{ 128 };
std::size_t requests_limit{ 1024 };
std::size_t database_requests_limit{ 1024 };
std::size_t pull_count{ nano::bootstrap_server::max_blocks };
nano::millis_t timeout{ 1000 * 3 };
std::size_t throttle_count{ 4 * 1024 };
nano::millis_t throttle_wait{ 100 };

nano::account_sets_config account_sets;
};
Expand Down