From 4d7fc6cb0193d0173a3a3a64f3b9f389456a3c47 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Wed, 22 Jan 2020 14:26:42 +0000 Subject: [PATCH 1/4] Adjust request aggregator stats And ensure unique hashes before generating votes --- nano/core_test/request_aggregator.cpp | 73 +++++++++++++++------------ nano/lib/stats.cpp | 20 +++++--- nano/lib/stats.hpp | 8 ++- nano/node/request_aggregator.cpp | 23 ++++++--- nano/node/request_aggregator.hpp | 2 +- 5 files changed, 75 insertions(+), 51 deletions(-) diff --git a/nano/core_test/request_aggregator.cpp b/nano/core_test/request_aggregator.cpp index 81714cb675..24bc0b4563 100644 --- a/nano/core_test/request_aggregator.cpp +++ b/nano/core_test/request_aggregator.cpp @@ -26,8 +26,8 @@ TEST (request_aggregator, one) { ASSERT_NO_ERROR (system.poll ()); } - // Not yet in the ledger, should be ignored - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_ignored)); + // Not yet in the ledger + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); ASSERT_EQ (nano::process_result::progress, node.ledger.process (node.store.tx_begin_write (), *send1).code); node.aggregator.add (channel, request); ASSERT_EQ (1, node.aggregator.size ()); @@ -47,11 +47,12 @@ TEST (request_aggregator, one) { ASSERT_NO_ERROR (system.poll ()); } + ASSERT_EQ (3, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); ASSERT_EQ (3, node.stats.count (nano::stat::type::requests, nano::stat::detail::all)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_ignored)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated)); ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_dropped)); ASSERT_EQ (2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); } @@ -79,16 +80,19 @@ TEST (request_aggregator, one_update) // In the ledger but no vote generated yet // Generated votes are created after the pool is removed from the aggregator, so a simple check on empty () is not enough system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated) == 0) + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out) == 0) { ASSERT_NO_ERROR (system.poll ()); } ASSERT_TRUE (node.aggregator.empty ()); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::all)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_ignored)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::in)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::out)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_dropped)); ASSERT_EQ (1, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); } @@ -119,7 +123,7 @@ TEST (request_aggregator, two) ASSERT_NO_ERROR (system.poll ()); } ASSERT_TRUE (node.aggregator.empty ()); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); // The same request should now send the cached vote node.aggregator.add (channel, request); ASSERT_EQ (1, node.aggregator.size ()); @@ -128,11 +132,15 @@ TEST (request_aggregator, two) { ASSERT_NO_ERROR (system.poll ()); } - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::all)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_ignored)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_dropped)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); + ASSERT_EQ (4, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::in)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::out)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached, nano::stat::dir::in)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached, nano::stat::dir::out)); ASSERT_EQ (2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); // Make sure the cached vote is for both hashes auto vote1 (node.votes_cache.find (send1->hash ())); @@ -169,11 +177,15 @@ TEST (request_aggregator, two_endpoints) { ASSERT_NO_ERROR (system.poll ()); } - ASSERT_EQ (2, node1.stats.count (nano::stat::type::requests, nano::stat::detail::all)); - ASSERT_EQ (0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_ignored)); - ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated)); - ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached)); - ASSERT_EQ (0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_dropped)); + ASSERT_EQ (2, node1.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); + ASSERT_EQ (0, node1.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); + ASSERT_EQ (2, node1.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::in)); + ASSERT_EQ (2, node1.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::out)); + ASSERT_EQ (0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); + ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); + ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); + ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached, nano::stat::dir::in)); + ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached, nano::stat::dir::out)); } TEST (request_aggregator, split) @@ -218,20 +230,15 @@ TEST (request_aggregator, split) } ASSERT_TRUE (node.aggregator.empty ()); // Two votes were sent, the first one for 12 hashes and the second one for 1 hash - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::all)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_ignored)); - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); + ASSERT_EQ (13, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::in)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::out)); + ASSERT_EQ (13, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_dropped)); ASSERT_EQ (2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); - auto transaction (node.store.tx_begin_read ()); - auto pre_last_hash (node.store.block_get (transaction, previous)->previous ()); - auto vote1 (node.votes_cache.find (pre_last_hash)); - ASSERT_EQ (1, vote1.size ()); - ASSERT_EQ (max_vbh, vote1[0]->blocks.size ()); - auto vote2 (node.votes_cache.find (previous)); - ASSERT_EQ (1, vote2.size ()); - ASSERT_EQ (1, vote2[0]->blocks.size ()); } TEST (request_aggregator, channel_lifetime) @@ -308,7 +315,7 @@ TEST (request_aggregator, channel_max_queue) node.aggregator.add (channel, request); node.aggregator.add (channel, request); system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_dropped) < 1) + while (node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped) < 1) { ASSERT_NO_ERROR (system.poll ()); } diff --git a/nano/lib/stats.cpp b/nano/lib/stats.cpp index 07b2077f4a..d9b6c4357d 100644 --- a/nano/lib/stats.cpp +++ b/nano/lib/stats.cpp @@ -438,6 +438,9 @@ std::string nano::stat::type_to_string (uint32_t key) case nano::stat::type::drop: res = "drop"; break; + case nano::stat::type::aggregator: + res = "aggregator"; + break; case nano::stat::type::requests: res = "requests"; break; @@ -643,17 +646,20 @@ std::string nano::stat::detail_to_string (uint32_t key) case nano::stat::detail::blocks_confirmed: res = "blocks_confirmed"; break; + case nano::stat::detail::aggregator_accepted: + res = "aggregator_accepted"; + break; + case nano::stat::detail::aggregator_dropped: + res = "aggregator_dropped"; + break; case nano::stat::detail::requests_cached: - res = "requests_votes_cached"; + res = "requests_cached"; break; case nano::stat::detail::requests_generated: - res = "requests_votes_generated"; - break; - case nano::stat::detail::requests_ignored: - res = "requests_votes_ignored"; + res = "requests_generated"; break; - case nano::stat::detail::requests_dropped: - res = "requests_dropped"; + case nano::stat::detail::requests_unknown: + res = "requests_unknown"; break; } return res; diff --git a/nano/lib/stats.hpp b/nano/lib/stats.hpp index 304b2adf15..9d39b5b6ca 100644 --- a/nano/lib/stats.hpp +++ b/nano/lib/stats.hpp @@ -198,6 +198,7 @@ class stat final observer, confirmation_height, drop, + aggregator, requests }; @@ -295,11 +296,14 @@ class stat final blocks_confirmed, invalid_block, + // [request] aggregator + aggregator_accepted, + aggregator_dropped, + // requests requests_cached, requests_generated, - requests_ignored, - requests_dropped + requests_unknown, }; /** Direction of the stat. If the direction is irrelevant, use in */ diff --git a/nano/node/request_aggregator.cpp b/nano/node/request_aggregator.cpp index 6cba4f23c0..de39f689f7 100644 --- a/nano/node/request_aggregator.cpp +++ b/nano/node/request_aggregator.cpp @@ -56,10 +56,7 @@ void nano::request_aggregator::add (std::shared_ptr & condition.notify_all (); } } - if (error) - { - stats.inc (nano::stat::type::requests, nano::stat::detail::requests_dropped); - } + stats.inc (nano::stat::type::aggregator, !error ? nano::stat::detail::aggregator_accepted : nano::stat::detail::aggregator_dropped); } void nano::request_aggregator::run () @@ -133,6 +130,7 @@ bool nano::request_aggregator::empty () std::vector nano::request_aggregator::aggregate (nano::transaction const & transaction_a, channel_pool & pool_a) const { + size_t cached_hashes = 0; std::vector to_generate; std::vector> cached_votes; for (auto const & hash_root : pool_a.hashes_roots) @@ -140,6 +138,7 @@ std::vector nano::request_aggregator::aggregate (nano::transac auto find_votes (votes_cache.find (hash_root.first)); if (!find_votes.empty ()) { + ++cached_hashes; cached_votes.insert (cached_votes.end (), find_votes.begin (), find_votes.end ()); } else if (!hash_root.first.is_zero () && store.block_exists (transaction_a, hash_root.first)) @@ -178,7 +177,7 @@ std::vector nano::request_aggregator::aggregate (nano::transac } else { - stats.inc (nano::stat::type::requests, nano::stat::detail::requests_ignored); + stats.inc (nano::stat::type::requests, nano::stat::detail::requests_unknown, stat::dir::in); } } } @@ -190,12 +189,18 @@ std::vector nano::request_aggregator::aggregate (nano::transac nano::confirm_ack confirm (vote); pool_a.channel->send (confirm); } - stats.add (nano::stat::type::requests, nano::stat::detail::requests_cached, stat::dir::in, cached_votes.size ()); + // #hashes in, #votes out + stats.add (nano::stat::type::requests, nano::stat::detail::requests_cached, stat::dir::in, cached_hashes); + stats.add (nano::stat::type::requests, nano::stat::detail::requests_cached, stat::dir::out, cached_votes.size ()); return to_generate; } -void nano::request_aggregator::generate (nano::transaction const & transaction_a, std::vector const hashes_a, std::shared_ptr & channel_a) const +void nano::request_aggregator::generate (nano::transaction const & transaction_a, std::vector hashes_a, std::shared_ptr & channel_a) const { + // Unique hashes + std::sort (hashes_a.begin (), hashes_a.end ()); + hashes_a.erase (std::unique (hashes_a.begin (), hashes_a.end ()), hashes_a.end ()); + size_t generated_l = 0; auto i (hashes_a.begin ()); auto n (hashes_a.end ()); @@ -214,7 +219,9 @@ void nano::request_aggregator::generate (nano::transaction const & transaction_a this->votes_cache.add (vote); }); } - stats.add (nano::stat::type::requests, nano::stat::detail::requests_generated, stat::dir::in, generated_l); + // #hashes in, #votes out + stats.add (nano::stat::type::requests, nano::stat::detail::requests_generated, stat::dir::in, hashes_a.size ()); + stats.add (nano::stat::type::requests, nano::stat::detail::requests_generated, stat::dir::out, generated_l); } std::unique_ptr nano::collect_container_info (nano::request_aggregator & aggregator, const std::string & name) diff --git a/nano/node/request_aggregator.hpp b/nano/node/request_aggregator.hpp index 471355fee3..0d71806a6c 100644 --- a/nano/node/request_aggregator.hpp +++ b/nano/node/request_aggregator.hpp @@ -76,7 +76,7 @@ class request_aggregator final /** Aggregate and send cached votes for \p pool_a, returning the leftovers that were not found in cached votes **/ std::vector aggregate (nano::transaction const &, channel_pool & pool_a) const; /** Generate and send votes from \p hashes_a to \p channel_a, does not need a lock on the mutex **/ - void generate (nano::transaction const &, std::vector const hashes_a, std::shared_ptr & channel_a) const; + void generate (nano::transaction const &, std::vector hashes_a, std::shared_ptr & channel_a) const; nano::stat & stats; nano::votes_cache & votes_cache; From 506063fc8534c347e89170ddc5b2333385081cab Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Wed, 22 Jan 2020 16:00:43 +0000 Subject: [PATCH 2/4] Move hash duplicate removal to before aggregation, and add a test --- nano/core_test/request_aggregator.cpp | 25 +++++++++++++++++++++++++ nano/node/request_aggregator.cpp | 14 ++++++++++---- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/nano/core_test/request_aggregator.cpp b/nano/core_test/request_aggregator.cpp index 24bc0b4563..624a6cef66 100644 --- a/nano/core_test/request_aggregator.cpp +++ b/nano/core_test/request_aggregator.cpp @@ -319,4 +319,29 @@ TEST (request_aggregator, channel_max_queue) { ASSERT_NO_ERROR (system.poll ()); } +} + +TEST (request_aggregator, unique) +{ + nano::system system; + nano::node_config node_config (nano::get_available_port (), system.logging); + node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled; + auto & node (*system.add_node (node_config)); + nano::genesis genesis; + system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv); + auto send1 (std::make_shared (nano::test_genesis_key.pub, genesis.hash (), nano::test_genesis_key.pub, nano::genesis_amount - nano::Gxrb_ratio, nano::test_genesis_key.pub, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *node.work_generate_blocking (genesis.hash ()))); + ASSERT_EQ (nano::process_result::progress, node.ledger.process (node.store.tx_begin_write (), *send1).code); + std::vector> request; + request.emplace_back (send1->hash (), send1->root ()); + auto channel (node.network.udp_channels.create (node.network.endpoint ())); + node.aggregator.add (channel, request); + node.aggregator.add (channel, request); + node.aggregator.add (channel, request); + node.aggregator.add (channel, request); + system.deadline_set (3s); + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out) < 1) + { + ASSERT_NO_ERROR (system.poll ()); + } + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); } \ No newline at end of file diff --git a/nano/node/request_aggregator.cpp b/nano/node/request_aggregator.cpp index de39f689f7..731e96dabe 100644 --- a/nano/node/request_aggregator.cpp +++ b/nano/node/request_aggregator.cpp @@ -130,6 +130,16 @@ bool nano::request_aggregator::empty () std::vector nano::request_aggregator::aggregate (nano::transaction const & transaction_a, channel_pool & pool_a) const { + // Unique hashes + using pair = decltype (pool_a.hashes_roots)::value_type; + std::sort (pool_a.hashes_roots.begin (), pool_a.hashes_roots.end (), [](pair const & pair1, pair const & pair2) { + return pair1.first < pair2.first; + }); + pool_a.hashes_roots.erase (std::unique (pool_a.hashes_roots.begin (), pool_a.hashes_roots.end (), [](pair const & pair1, pair const & pair2) { + return pair1.first == pair2.first; + }), + pool_a.hashes_roots.end ()); + size_t cached_hashes = 0; std::vector to_generate; std::vector> cached_votes; @@ -197,10 +207,6 @@ std::vector nano::request_aggregator::aggregate (nano::transac void nano::request_aggregator::generate (nano::transaction const & transaction_a, std::vector hashes_a, std::shared_ptr & channel_a) const { - // Unique hashes - std::sort (hashes_a.begin (), hashes_a.end ()); - hashes_a.erase (std::unique (hashes_a.begin (), hashes_a.end ()), hashes_a.end ()); - size_t generated_l = 0; auto i (hashes_a.begin ()); auto n (hashes_a.end ()); From 7a58ecd52ab20dad4e8a9f98233011efc9bfa268 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Fri, 24 Jan 2020 09:59:40 +0000 Subject: [PATCH 3/4] Change stats to be more obvious without reading documentation --- nano/core_test/request_aggregator.cpp | 61 ++++++++++++--------------- nano/lib/stats.cpp | 14 ++++-- nano/lib/stats.hpp | 8 ++-- nano/node/request_aggregator.cpp | 10 ++--- 4 files changed, 45 insertions(+), 48 deletions(-) diff --git a/nano/core_test/request_aggregator.cpp b/nano/core_test/request_aggregator.cpp index 624a6cef66..48105382db 100644 --- a/nano/core_test/request_aggregator.cpp +++ b/nano/core_test/request_aggregator.cpp @@ -34,7 +34,7 @@ TEST (request_aggregator, one) // In the ledger but no vote generated yet // Generated votes are created after the pool is removed from the aggregator, so a simple check on empty () is not enough system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated) == 0) + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes) == 0) { ASSERT_NO_ERROR (system.poll ()); } @@ -49,10 +49,9 @@ TEST (request_aggregator, one) } ASSERT_EQ (3, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); ASSERT_EQ (0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); - ASSERT_EQ (3, node.stats.count (nano::stat::type::requests, nano::stat::detail::all)); ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_votes)); ASSERT_EQ (2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); } @@ -80,19 +79,18 @@ TEST (request_aggregator, one_update) // In the ledger but no vote generated yet // Generated votes are created after the pool is removed from the aggregator, so a simple check on empty () is not enough system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out) == 0) + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes) == 0) { ASSERT_NO_ERROR (system.poll ()); } ASSERT_TRUE (node.aggregator.empty ()); ASSERT_EQ (2, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); ASSERT_EQ (0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::in)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::out)); ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_hashes)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_votes)); ASSERT_EQ (1, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); } @@ -118,12 +116,11 @@ TEST (request_aggregator, two) // One vote should be generated for both blocks // Generated votes are created after the pool is removed from the aggregator, so a simple check on empty () is not enough system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated) == 0) + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes) == 0) { ASSERT_NO_ERROR (system.poll ()); } ASSERT_TRUE (node.aggregator.empty ()); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); // The same request should now send the cached vote node.aggregator.add (channel, request); ASSERT_EQ (1, node.aggregator.size ()); @@ -134,13 +131,11 @@ TEST (request_aggregator, two) } ASSERT_EQ (2, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); ASSERT_EQ (0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); - ASSERT_EQ (4, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::in)); - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::out)); ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached, nano::stat::dir::in)); - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached, nano::stat::dir::out)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_hashes)); + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_votes)); ASSERT_EQ (2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); // Make sure the cached vote is for both hashes auto vote1 (node.votes_cache.find (send1->hash ())); @@ -179,13 +174,11 @@ TEST (request_aggregator, two_endpoints) } ASSERT_EQ (2, node1.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); ASSERT_EQ (0, node1.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); - ASSERT_EQ (2, node1.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::in)); - ASSERT_EQ (2, node1.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::out)); ASSERT_EQ (0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); - ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); - ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); - ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached, nano::stat::dir::in)); - ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached, nano::stat::dir::out)); + ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes)); + ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes)); + ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_hashes)); + ASSERT_EQ (1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_votes)); } TEST (request_aggregator, split) @@ -224,7 +217,7 @@ TEST (request_aggregator, split) // In the ledger but no vote generated yet // Generated votes are created after the pool is removed from the aggregator, so a simple check on empty () is not enough system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated) < 2) + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes) < 2) { ASSERT_NO_ERROR (system.poll ()); } @@ -232,12 +225,10 @@ TEST (request_aggregator, split) // Two votes were sent, the first one for 12 hashes and the second one for 1 hash ASSERT_EQ (1, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted)); ASSERT_EQ (0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped)); - ASSERT_EQ (13, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::in)); - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::all, nano::stat::dir::out)); - ASSERT_EQ (13, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); - ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out)); + ASSERT_EQ (13, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes)); + ASSERT_EQ (2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes)); ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown)); - ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached)); + ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_hashes)); ASSERT_EQ (2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out)); } @@ -260,7 +251,7 @@ TEST (request_aggregator, channel_lifetime) } ASSERT_EQ (1, node.aggregator.size ()); system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated) == 0) + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes) == 0) { ASSERT_NO_ERROR (system.poll ()); } @@ -292,7 +283,7 @@ TEST (request_aggregator, channel_update) // channel1 is not being held anymore ASSERT_EQ (nullptr, channel1_w.lock ()); system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated) == 0) + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes) == 0) { ASSERT_NO_ERROR (system.poll ()); } @@ -339,9 +330,9 @@ TEST (request_aggregator, unique) node.aggregator.add (channel, request); node.aggregator.add (channel, request); system.deadline_set (3s); - while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::out) < 1) + while (node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes) < 1) { ASSERT_NO_ERROR (system.poll ()); } - ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated, nano::stat::dir::in)); -} \ No newline at end of file + ASSERT_EQ (1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes)); +} diff --git a/nano/lib/stats.cpp b/nano/lib/stats.cpp index d9b6c4357d..54a6eaa16a 100644 --- a/nano/lib/stats.cpp +++ b/nano/lib/stats.cpp @@ -652,11 +652,17 @@ std::string nano::stat::detail_to_string (uint32_t key) case nano::stat::detail::aggregator_dropped: res = "aggregator_dropped"; break; - case nano::stat::detail::requests_cached: - res = "requests_cached"; + case nano::stat::detail::requests_cached_hashes: + res = "requests_cached_hashes"; break; - case nano::stat::detail::requests_generated: - res = "requests_generated"; + case nano::stat::detail::requests_generated_hashes: + res = "requests_generated_hashes"; + break; + case nano::stat::detail::requests_cached_votes: + res = "requests_cached_votes"; + break; + case nano::stat::detail::requests_generated_votes: + res = "requests_generated_votes"; break; case nano::stat::detail::requests_unknown: res = "requests_unknown"; diff --git a/nano/lib/stats.hpp b/nano/lib/stats.hpp index 9d39b5b6ca..544ffa4869 100644 --- a/nano/lib/stats.hpp +++ b/nano/lib/stats.hpp @@ -301,9 +301,11 @@ class stat final aggregator_dropped, // requests - requests_cached, - requests_generated, - requests_unknown, + requests_cached_hashes, + requests_generated_hashes, + requests_cached_votes, + requests_generated_votes, + requests_unknown }; /** Direction of the stat. If the direction is irrelevant, use in */ diff --git a/nano/node/request_aggregator.cpp b/nano/node/request_aggregator.cpp index 731e96dabe..c77be74285 100644 --- a/nano/node/request_aggregator.cpp +++ b/nano/node/request_aggregator.cpp @@ -199,9 +199,8 @@ std::vector nano::request_aggregator::aggregate (nano::transac nano::confirm_ack confirm (vote); pool_a.channel->send (confirm); } - // #hashes in, #votes out - stats.add (nano::stat::type::requests, nano::stat::detail::requests_cached, stat::dir::in, cached_hashes); - stats.add (nano::stat::type::requests, nano::stat::detail::requests_cached, stat::dir::out, cached_votes.size ()); + stats.add (nano::stat::type::requests, nano::stat::detail::requests_cached_hashes, stat::dir::in, cached_hashes); + stats.add (nano::stat::type::requests, nano::stat::detail::requests_cached_votes, stat::dir::in, cached_votes.size ()); return to_generate; } @@ -225,9 +224,8 @@ void nano::request_aggregator::generate (nano::transaction const & transaction_a this->votes_cache.add (vote); }); } - // #hashes in, #votes out - stats.add (nano::stat::type::requests, nano::stat::detail::requests_generated, stat::dir::in, hashes_a.size ()); - stats.add (nano::stat::type::requests, nano::stat::detail::requests_generated, stat::dir::out, generated_l); + stats.add (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes, stat::dir::in, hashes_a.size ()); + stats.add (nano::stat::type::requests, nano::stat::detail::requests_generated_votes, stat::dir::in, generated_l); } std::unique_ptr nano::collect_container_info (nano::request_aggregator & aggregator, const std::string & name) From 27ca0e96c60ed197618e7991409a0533ef24be00 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Fri, 24 Jan 2020 10:04:58 +0000 Subject: [PATCH 4/4] Fix a rare failure in a test due to rep crawler sending an extra request --- nano/core_test/request_aggregator.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/nano/core_test/request_aggregator.cpp b/nano/core_test/request_aggregator.cpp index 48105382db..819dd04354 100644 --- a/nano/core_test/request_aggregator.cpp +++ b/nano/core_test/request_aggregator.cpp @@ -150,9 +150,11 @@ TEST (request_aggregator, two_endpoints) nano::system system; nano::node_config node_config (nano::get_available_port (), system.logging); node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled; - auto & node1 (*system.add_node (node_config)); + nano::node_flags node_flags; + node_flags.disable_rep_crawler = true; + auto & node1 (*system.add_node (node_config, node_flags)); node_config.peering_port = nano::get_available_port (); - auto & node2 (*system.add_node (node_config)); + auto & node2 (*system.add_node (node_config, node_flags)); nano::genesis genesis; system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv); auto send1 (std::make_shared (nano::test_genesis_key.pub, genesis.hash (), nano::test_genesis_key.pub, nano::genesis_amount - nano::Gxrb_ratio, nano::test_genesis_key.pub, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *node1.work_generate_blocking (genesis.hash ())));