Skip to content

Commit

Permalink
Removing tracking weight from rep_crawler class. (#4187)
Browse files Browse the repository at this point in the history 
This information is tracked accurately by nano::ledger and keeping the containers in sync is error prone.
  • Loading branch information
@clemahieu
clemahieu authored Mar 20, 2023
1 parent 7e6a310 commit e8ddd83
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 64 deletions.
4 changes: 2 additions & 2 deletions nano/core_test/active_transactions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ TEST (active_transactions, confirm_election_by_request)
// Add representative (node1) to disabled rep crawler of node2
{
nano::lock_guard<nano::mutex> guard (node2.rep_crawler.probable_reps_mutex);
node2.rep_crawler.probable_reps.emplace (nano::dev::genesis_key.pub, nano::dev::constants.genesis_amount, *peers.cbegin ());
node2.rep_crawler.probable_reps.emplace (nano::dev::genesis_key.pub, *peers.cbegin ());
}

// Expect a vote to come back
Expand Down Expand Up @@ -117,7 +117,7 @@ TEST (active_transactions, confirm_frontier)
ASSERT_FALSE (peers.empty ());
{
nano::lock_guard<nano::mutex> guard (node2.rep_crawler.probable_reps_mutex);
node2.rep_crawler.probable_reps.emplace (nano::dev::genesis_key.pub, nano::dev::constants.genesis_amount, *peers.begin ());
node2.rep_crawler.probable_reps.emplace (nano::dev::genesis_key.pub, *peers.begin ());
}

nano::state_block_builder builder;
Expand Down
6 changes: 3 additions & 3 deletions nano/core_test/confirmation_solicitor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ TEST (confirmation_solicitor, batches)
auto & node2 = *system.add_node (node_flags);
auto channel1 = nano::test::establish_tcp (system, node2, node1.network.endpoint ());
// Solicitor will only solicit from this representative
nano::representative representative (nano::dev::genesis_key.pub, nano::dev::constants.genesis_amount, channel1);
nano::representative representative (nano::dev::genesis_key.pub, channel1);
std::vector<nano::representative> representatives{ representative };
nano::confirmation_solicitor solicitor (node2.network, node2.config);
solicitor.prepare (representatives);
Expand Down Expand Up @@ -70,7 +70,7 @@ TEST (confirmation_solicitor, different_hash)
auto & node2 = *system.add_node (node_flags);
auto channel1 = nano::test::establish_tcp (system, node2, node1.network.endpoint ());
// Solicitor will only solicit from this representative
nano::representative representative (nano::dev::genesis_key.pub, nano::dev::constants.genesis_amount, channel1);
nano::representative representative (nano::dev::genesis_key.pub, channel1);
std::vector<nano::representative> representatives{ representative };
nano::confirmation_solicitor solicitor (node2.network, node2.config);
solicitor.prepare (representatives);
Expand Down Expand Up @@ -117,7 +117,7 @@ TEST (confirmation_solicitor, bypass_max_requests_cap)
{
// Make a temporary channel associated with node2
auto channel = std::make_shared<nano::transport::inproc::channel> (node2, node2);
nano::representative representative (nano::account (i), i, channel);
nano::representative representative{ nano::account (i), channel };
representatives.push_back (representative);
}
ASSERT_EQ (max_representatives + 1, representatives.size ());
Expand Down
10 changes: 5 additions & 5 deletions nano/core_test/node.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1679,10 +1679,10 @@ TEST (node, rep_list)
auto done (false);
while (!done)
{
auto reps (node1.rep_crawler.representatives (1));
auto reps = node1.rep_crawler.representatives (1);
if (!reps.empty ())
{
if (!reps[0].weight.is_zero ())
if (!node1.ledger.weight (reps[0].account).is_zero ())
{
done = true;
}
Expand Down Expand Up @@ -1771,9 +1771,9 @@ TEST (node, rep_weight)
node.rep_crawler.response (channel3, vote2);
ASSERT_TIMELY (5s, node.rep_crawler.representative_count () == 2);
// Make sure we get the rep with the most weight first
auto reps (node.rep_crawler.representatives (1));
auto reps = node.rep_crawler.representatives (1);
ASSERT_EQ (1, reps.size ());
ASSERT_EQ (node.balance (nano::dev::genesis_key.pub), reps[0].weight.number ());
ASSERT_EQ (node.balance (nano::dev::genesis_key.pub), node.ledger.weight (reps[0].account));
ASSERT_EQ (nano::dev::genesis_key.pub, reps[0].account);
ASSERT_EQ (*channel1, reps[0].channel_ref ());
ASSERT_TRUE (node.rep_crawler.is_pr (*channel1));
Expand Down Expand Up @@ -1856,7 +1856,7 @@ TEST (node, rep_remove)
ASSERT_TIMELY (5s, searching_node.rep_crawler.representative_count () == 1);
auto reps (searching_node.rep_crawler.representatives (1));
ASSERT_EQ (1, reps.size ());
ASSERT_EQ (searching_node.minimum_principal_weight () * 2, reps[0].weight.number ());
ASSERT_EQ (searching_node.minimum_principal_weight () * 2, searching_node.ledger.weight (reps[0].account));
ASSERT_EQ (keys_rep1.pub, reps[0].account);
ASSERT_EQ (*channel_rep1, reps[0].channel_ref ());

Expand Down
2 changes: 1 addition & 1 deletion nano/node/json_handler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2123,7 +2123,7 @@ void nano::json_handler::confirmation_quorum ()
boost::property_tree::ptree peer_node;
peer_node.put ("account", peer.account.to_account ());
peer_node.put ("ip", peer.channel->to_string ());
peer_node.put ("weight", peer.weight.to_string_dec ());
peer_node.put ("weight", nano::amount{ node.ledger.weight (peer.account) }.to_string_dec ());
peers.push_back (std::make_pair ("", peer_node));
}
response_l.add_child ("peers", peers);
Expand Down
56 changes: 14 additions & 42 deletions nano/node/repcrawler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,14 @@ void nano::rep_crawler::validate ()
{
debug_assert (info.account == vote->account);
updated = true;
info.weight = rep_weight;
prev_channel = info.channel;
info.channel = channel;
}
});
}
else
{
probable_reps.emplace (nano::representative (vote->account, rep_weight, channel));
probable_reps.emplace (nano::representative (vote->account, channel));
inserted = true;
}

Expand All @@ -111,7 +110,6 @@ void nano::rep_crawler::ongoing_crawl ()
auto now (std::chrono::steady_clock::now ());
auto total_weight_l (total_weight ());
cleanup_reps ();
update_weights ();
validate ();
query (get_crawl_targets (total_weight_l));
auto sufficient_weight (total_weight_l > node.online_reps.delta ());
Expand Down Expand Up @@ -225,7 +223,7 @@ bool nano::rep_crawler::is_pr (nano::transport::channel const & channel_a) const
bool result = false;
if (existing != probable_reps.get<tag_channel_ref> ().end ())
{
result = existing->weight > node.minimum_principal_weight ();
result = node.ledger.weight (existing->account) > node.minimum_principal_weight ();
}
return result;
}
Expand All @@ -250,19 +248,11 @@ nano::uint128_t nano::rep_crawler::total_weight () const
{
nano::lock_guard<nano::mutex> lock{ probable_reps_mutex };
nano::uint128_t result (0);
for (auto i (probable_reps.get<tag_weight> ().begin ()), n (probable_reps.get<tag_weight> ().end ()); i != n; ++i)
for (auto i (probable_reps.get<tag_account> ().begin ()), n (probable_reps.get<tag_account> ().end ()); i != n; ++i)
{
if (i->channel->alive ())
{
auto weight (i->weight.number ());
if (weight > 0)
{
result = result + weight;
}
else
{
break;
}
result += node.ledger.weight (i->account);
}
}
return result;
Expand Down Expand Up @@ -331,42 +321,24 @@ void nano::rep_crawler::cleanup_reps ()
}
}

void nano::rep_crawler::update_weights ()
{
nano::lock_guard<nano::mutex> lock{ probable_reps_mutex };
for (auto i (probable_reps.get<tag_last_request> ().begin ()), n (probable_reps.get<tag_last_request> ().end ()); i != n;)
{
auto weight (node.ledger.weight (i->account));
if (weight > 0)
{
if (i->weight.number () != weight)
{
probable_reps.get<tag_last_request> ().modify (i, [weight] (nano::representative & info) {
info.weight = weight;
});
}
++i;
}
else
{
// Erase non representatives
i = probable_reps.get<tag_last_request> ().erase (i);
}
}
}

std::vector<nano::representative> nano::rep_crawler::representatives (std::size_t count_a, nano::uint128_t const weight_a, boost::optional<decltype (nano::network_constants::protocol_version)> const & opt_version_min_a)
{
auto version_min (opt_version_min_a.value_or (node.network_params.network.protocol_version_min));
std::vector<representative> result;
std::multimap<nano::amount, representative, std::greater<nano::amount>> ordered;
nano::lock_guard<nano::mutex> lock{ probable_reps_mutex };
for (auto i (probable_reps.get<tag_weight> ().begin ()), n (probable_reps.get<tag_weight> ().end ()); i != n && result.size () < count_a; ++i)
for (auto i (probable_reps.get<tag_account> ().begin ()), n (probable_reps.get<tag_account> ().end ()); i != n; ++i)
{
if (i->weight > weight_a && i->channel->get_network_version () >= version_min)
auto weight = node.ledger.weight (i->account);
if (weight > weight_a && i->channel->get_network_version () >= version_min)
{
result.push_back (*i);
ordered.insert ({ nano::amount{ weight }, *i });
}
}
std::vector<nano::representative> result;
for (auto i = ordered.begin (), n = ordered.end (); i != n && result.size () < count_a; ++i)
{
result.push_back (i->second);
}
return result;
}

Expand Down
16 changes: 5 additions & 11 deletions nano/node/repcrawler.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ class representative
{
public:
representative () = default;
representative (nano::account account_a, nano::amount weight_a, std::shared_ptr<nano::transport::channel> const & channel_a) :
account (account_a), weight (weight_a), channel (channel_a)
representative (nano::account account_a, std::shared_ptr<nano::transport::channel> const & channel_a) :
account (account_a), channel (channel_a)
{
debug_assert (channel != nullptr);
}
Expand All @@ -41,7 +41,6 @@ class representative
return account == other_a.account;
}
nano::account account{};
nano::amount weight{ 0 };
std::shared_ptr<nano::transport::channel> channel;
std::chrono::steady_clock::time_point last_request{ std::chrono::steady_clock::time_point () };
std::chrono::steady_clock::time_point last_response{ std::chrono::steady_clock::time_point () };
Expand All @@ -59,16 +58,14 @@ class rep_crawler final
class tag_account {};
class tag_channel_ref {};
class tag_last_request {};
class tag_weight {};
class tag_sequenced {};

using probably_rep_t = boost::multi_index_container<representative,
mi::indexed_by<
mi::hashed_unique<mi::member<representative, nano::account, &representative::account>>,
mi::random_access<>,
mi::hashed_unique<mi::tag<tag_account>, mi::member<representative, nano::account, &representative::account>>,
mi::sequenced<mi::tag<tag_sequenced>>,
mi::ordered_non_unique<mi::tag<tag_last_request>,
mi::member<representative, std::chrono::steady_clock::time_point, &representative::last_request>>,
mi::ordered_non_unique<mi::tag<tag_weight>,
mi::member<representative, nano::amount, &representative::weight>, std::greater<nano::amount>>,
mi::hashed_non_unique<mi::tag<tag_channel_ref>,
mi::const_mem_fun<representative, std::reference_wrapper<nano::transport::channel const>, &representative::channel_ref>>>>;
// clang-format on
Expand Down Expand Up @@ -141,9 +138,6 @@ class rep_crawler final
/** Clean representatives with inactive channels */
void cleanup_reps ();

/** Update representatives weights from ledger */
void update_weights ();

/** Protects the probable_reps container */
mutable nano::mutex probable_reps_mutex;

Expand Down

0 comments on commit e8ddd83

Please sign in to comment.