Skip to content

Commit

Permalink
Final vote requests/replies only (#4648)
Browse files Browse the repository at this point in the history 
* Remove cache from request aggregator

* Remove non final replies

* Trace vote source in vote processor

* Stats

* Disable local votes cache tests

* Constness

* Fix tests

* Since cached votes are no longer being used the requests_generated stat counters will increment for both requests.

* Fix tests

---------

Co-authored-by: Colin LeMahieu <clemahieu@gmail.com>
  • Loading branch information
@pwojcikdev @clemahieu
pwojcikdev and clemahieu authored Jul 11, 2024
1 parent 809becf commit a7a2c0b
Show file tree
Hide file tree
Showing 9 changed files with 132 additions and 173 deletions.
50 changes: 24 additions & 26 deletions nano/core_test/node.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,8 @@ TEST (node, fork_multi_flip)

auto election = nano::test::start_election (system, node2, send2->hash ());
ASSERT_NE (nullptr, election);
ASSERT_TIMELY (5s, election->contains (send1->hash ()));
nano::test::confirm (node1.ledger, send1);
ASSERT_TIMELY (5s, node2.block_or_pruned_exists (send1->hash ()));
ASSERT_TRUE (nano::test::block_or_pruned_none_exists (node2, { send2, send3 }));
auto winner = *election->tally ().begin ();
Expand All @@ -781,17 +783,15 @@ TEST (node, fork_multi_flip)
TEST (node, fork_bootstrap_flip)
{
nano::test::system system;
nano::test::system system0;
nano::test::system system1;
nano::node_config config0{ system.get_available_port () };
config0.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled;
nano::node_flags node_flags;
node_flags.disable_bootstrap_bulk_push_client = true;
node_flags.disable_lazy_bootstrap = true;
auto & node1 = *system0.add_node (config0, node_flags);
auto & node1 = *system.add_node (config0, node_flags);
system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
nano::node_config config1 (system.get_available_port ());
auto & node2 = *system1.add_node (config1, node_flags);
system0.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
auto & node2 = *system.make_disconnected_node (config1, node_flags);
nano::block_hash latest = node1.latest (nano::dev::genesis_key.pub);
nano::keypair key1;
nano::send_block_builder builder;
Expand All @@ -800,30 +800,27 @@ TEST (node, fork_bootstrap_flip)
.destination (key1.pub)
.balance (nano::dev::constants.genesis_amount - nano::Gxrb_ratio)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*system0.work.generate (latest))
.work (*system.work.generate (latest))
.build ();
nano::keypair key2;
auto send2 = builder.make_block ()
.previous (latest)
.destination (key2.pub)
.balance (nano::dev::constants.genesis_amount - nano::Gxrb_ratio)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*system0.work.generate (latest))
.work (*system.work.generate (latest))
.build ();
// Insert but don't rebroadcast, simulating settled blocks
ASSERT_EQ (nano::block_status::progress, node1.ledger.process (node1.ledger.tx_begin_write (), send1));
ASSERT_EQ (nano::block_status::progress, node2.ledger.process (node2.ledger.tx_begin_write (), send2));
ASSERT_TRUE (node2.ledger.any.block_exists (node2.ledger.tx_begin_read (), send2->hash ()));
node2.bootstrap_initiator.bootstrap (node1.network.endpoint ()); // Additionally add new peer to confirm & replace bootstrap block
auto again (true);
system0.deadline_set (50s);
system1.deadline_set (50s);
while (again)
{
ASSERT_NO_ERROR (system0.poll ());
ASSERT_NO_ERROR (system1.poll ());
again = !node2.ledger.any.block_exists (node2.ledger.tx_begin_read (), send1->hash ());
}
nano::test::confirm (node1.ledger, send1);
ASSERT_TIMELY (1s, node1.ledger.any.block_exists (node1.ledger.tx_begin_read (), send1->hash ()));
ASSERT_TIMELY (1s, node2.ledger.any.block_exists (node2.ledger.tx_begin_read (), send2->hash ()));

// Additionally add new peer to confirm & replace bootstrap block
node2.network.merge_peer (node1.network.endpoint ());

ASSERT_TIMELY (10s, node2.ledger.any.block_exists (node2.ledger.tx_begin_read (), send1->hash ()));
}

TEST (node, fork_open)
Expand Down Expand Up @@ -1820,7 +1817,8 @@ TEST (node, confirm_quorum)
ASSERT_EQ (0, node1.balance (nano::dev::genesis_key.pub));
}

TEST (node, local_votes_cache)
// TODO: Local vote cache is no longer used when generating votes
TEST (node, DISABLED_local_votes_cache)
{
nano::test::system system;
nano::node_config node_config (system.get_available_port ());
Expand Down Expand Up @@ -1903,6 +1901,7 @@ TEST (node, local_votes_cache)
// Test disabled because it's failing intermittently.
// PR in which it got disabled: https://github.com/nanocurrency/nano-node/pull/3532
// Issue for investigating it: https://github.com/nanocurrency/nano-node/issues/3481
// TODO: Local vote cache is no longer used when generating votes
TEST (node, DISABLED_local_votes_cache_batch)
{
nano::test::system system;
Expand Down Expand Up @@ -1976,7 +1975,8 @@ TEST (node, DISABLED_local_votes_cache_batch)
* There is a cache for locally generated votes. This test checks that the node
* properly caches and uses those votes when replying to confirm_req requests.
*/
TEST (node, local_votes_cache_generate_new_vote)
// TODO: Local vote cache is no longer used when generating votes
TEST (node, DISABLED_local_votes_cache_generate_new_vote)
{
nano::test::system system;
nano::node_config node_config (system.get_available_port ());
Expand Down Expand Up @@ -2022,7 +2022,8 @@ TEST (node, local_votes_cache_generate_new_vote)
ASSERT_TIMELY_EQ (3s, 3, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
}

TEST (node, local_votes_cache_fork)
// TODO: Local vote cache is no longer used when generating votes
TEST (node, DISABLED_local_votes_cache_fork)
{
nano::test::system system;
nano::node_flags node_flags;
Expand Down Expand Up @@ -2959,13 +2960,10 @@ TEST (node, rollback_vote_self)
// Insert genesis key in the wallet
system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);

// Even without the rollback being finished, the aggregator must reply with a vote for the new winner, not the old one
ASSERT_TRUE (node.history.votes (send2->root (), send2->hash ()).empty ());
ASSERT_TRUE (node.history.votes (fork->root (), fork->hash ()).empty ());
// Without the rollback being finished, the aggregator should not reply with any vote
auto channel = std::make_shared<nano::transport::fake::channel> (node);
node.aggregator.request ({ { send2->hash (), send2->root () } }, channel);
ASSERT_TIMELY (5s, !node.history.votes (fork->root (), fork->hash ()).empty ());
ASSERT_TRUE (node.history.votes (send2->root (), send2->hash ()).empty ());
ASSERT_ALWAYS_EQ (1s, node.stats.count (nano::stat::type::request_aggregator_replies), 0);

// Going out of the scope allows the rollback to complete
}
Expand Down
78 changes: 40 additions & 38 deletions nano/core_test/request_aggregator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,27 +34,34 @@ TEST (request_aggregator, one)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*node.work_generate_blocking (nano::dev::genesis->hash ()))
.build ();
std::vector<std::pair<nano::block_hash, nano::root>> request;
request.emplace_back (send1->hash (), send1->root ());

std::vector<std::pair<nano::block_hash, nano::root>> request{ { send1->hash (), send1->root () } };

auto client = std::make_shared<nano::transport::tcp_socket> (node);
std::shared_ptr<nano::transport::channel> dummy_channel = std::make_shared<nano::transport::tcp_channel> (node, client);

// Not yet in the ledger
node.aggregator.request (request, dummy_channel);
ASSERT_TIMELY (3s, node.aggregator.empty ());
// Not yet in the ledger
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown));

// Process and confirm
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1));
node.aggregator.request (request, dummy_channel);
nano::test::confirm (node.ledger, send1);

// In the ledger but no vote generated yet
ASSERT_TIMELY (3s, 0 < node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
ASSERT_TIMELY (3s, node.aggregator.empty ());
node.aggregator.request (request, dummy_channel);
ASSERT_TIMELY (3s, node.aggregator.empty ());
ASSERT_TIMELY (3s, 0 < node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));

// Already cached
// TODO: This is outdated, aggregator should not be using cache
node.aggregator.request (request, dummy_channel);
ASSERT_TIMELY (3s, node.aggregator.empty ());
ASSERT_TIMELY_EQ (3s, 3, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_accepted));
ASSERT_TIMELY_EQ (3s, 0, node.stats.count (nano::stat::type::aggregator, nano::stat::detail::aggregator_dropped));
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown));
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_votes));
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
ASSERT_TIMELY_EQ (3s, 0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cannot_vote));
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
}
Expand All @@ -78,8 +85,7 @@ TEST (request_aggregator, one_update)
.work (*node.work_generate_blocking (nano::dev::genesis->hash ()))
.build ();
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1));
node.confirming_set.add (send1->hash ());
ASSERT_TIMELY (5s, node.ledger.confirmed.block_exists_or_pruned (node.ledger.tx_begin_read (), send1->hash ()));
nano::test::confirm (node.ledger, send1);
auto send2 = nano::state_block_builder ()
.account (nano::dev::genesis_key.pub)
.previous (send1->hash ())
Expand All @@ -90,6 +96,7 @@ TEST (request_aggregator, one_update)
.work (*node.work_generate_blocking (send1->hash ()))
.build ();
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send2));
nano::test::confirm (node.ledger, send2);
auto receive1 = nano::state_block_builder ()
.account (key1.pub)
.previous (0)
Expand All @@ -100,6 +107,7 @@ TEST (request_aggregator, one_update)
.work (*node.work_generate_blocking (key1.pub))
.build ();
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), receive1));
nano::test::confirm (node.ledger, receive1);

auto dummy_channel = nano::test::fake_channel (node);

Expand Down Expand Up @@ -143,8 +151,7 @@ TEST (request_aggregator, two)
.work (*node.work_generate_blocking (nano::dev::genesis->hash ()))
.build ();
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1));
node.confirming_set.add (send1->hash ());
ASSERT_TIMELY (5s, node.ledger.confirmed.block_exists_or_pruned (node.ledger.tx_begin_read (), send1->hash ()));
nano::test::confirm (node.ledger, send1);
auto send2 = builder.make_block ()
.account (nano::dev::genesis_key.pub)
.previous (send1->hash ())
Expand All @@ -164,7 +171,10 @@ TEST (request_aggregator, two)
.work (*node.work_generate_blocking (key1.pub))
.build ();
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send2));
nano::test::confirm (node.ledger, send2);
ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), receive1));
nano::test::confirm (node.ledger, receive1);

std::vector<std::pair<nano::block_hash, nano::root>> request;
request.emplace_back (send2->hash (), send2->root ());
request.emplace_back (receive1->hash (), receive1->root ());
Expand All @@ -181,10 +191,8 @@ 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_TIMELY_EQ (3s, 0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown));
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes));
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_hashes));
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_votes));
ASSERT_TIMELY_EQ (3s, 4, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes));
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
ASSERT_TIMELY_EQ (3s, 0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cannot_vote));
ASSERT_TIMELY_EQ (3s, 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
Expand Down Expand Up @@ -217,13 +225,15 @@ TEST (request_aggregator, two_endpoints)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*node1.work_generate_blocking (nano::dev::genesis->hash ()))
.build ();
std::vector<std::pair<nano::block_hash, nano::root>> request;
request.emplace_back (send1->hash (), send1->root ());
ASSERT_EQ (nano::block_status::progress, node1.ledger.process (node1.ledger.tx_begin_write (), send1));
nano::test::confirm (node1.ledger, send1);

auto dummy_channel1 = std::make_shared<nano::transport::inproc::channel> (node1, node1);
auto dummy_channel2 = std::make_shared<nano::transport::inproc::channel> (node2, node2);
ASSERT_NE (nano::transport::map_endpoint_to_v6 (dummy_channel1->get_endpoint ()), nano::transport::map_endpoint_to_v6 (dummy_channel2->get_endpoint ()));

std::vector<std::pair<nano::block_hash, nano::root>> request{ { send1->hash (), send1->root () } };

// For the first request, aggregator should generate a new vote
node1.aggregator.request (request, dummy_channel1);
ASSERT_TIMELY (5s, node1.aggregator.empty ());
Expand All @@ -234,22 +244,19 @@ TEST (request_aggregator, two_endpoints)
ASSERT_TIMELY_EQ (5s, 0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown));
ASSERT_TIMELY_EQ (5s, 1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes));
ASSERT_TIMELY_EQ (5s, 1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
ASSERT_TIMELY_EQ (3s, 0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_hashes));
ASSERT_TIMELY_EQ (3s, 0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_votes));
ASSERT_TIMELY_EQ (3s, 0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cannot_vote));

// For the second request, aggregator should use the cache
// TODO: This is outdated, aggregator should not be using cache
node1.aggregator.request (request, dummy_channel1);
ASSERT_TIMELY (5s, node1.aggregator.empty ());

ASSERT_TIMELY_EQ (5s, 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_TIMELY_EQ (5s, 0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown));
ASSERT_TIMELY_EQ (5s, 1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes));
ASSERT_TIMELY_EQ (5s, 1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
ASSERT_TIMELY_EQ (3s, 1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_hashes));
ASSERT_TIMELY_EQ (3s, 1, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cached_votes));
ASSERT_TIMELY_EQ (5s, 2, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes));
ASSERT_TIMELY_EQ (5s, 2, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
ASSERT_TIMELY_EQ (3s, 0, node1.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cannot_vote));
}

Expand Down Expand Up @@ -365,8 +372,6 @@ TEST (request_aggregator, DISABLED_unique)
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
}

namespace nano
{
TEST (request_aggregator, cannot_vote)
{
nano::test::system system;
Expand Down Expand Up @@ -400,44 +405,41 @@ TEST (request_aggregator, cannot_vote)
request.emplace_back (send2->hash (), send2->root ());
// Incorrect hash, correct root
request.emplace_back (1, send2->root ());

auto client = std::make_shared<nano::transport::tcp_socket> (node);
std::shared_ptr<nano::transport::channel> dummy_channel = std::make_shared<nano::transport::tcp_channel> (node, client);
node.aggregator.request (request, dummy_channel);
ASSERT_TIMELY (3s, node.aggregator.empty ());
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_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cannot_vote));
ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_non_final));
ASSERT_EQ (0, 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_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::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));

// With an ongoing election
node.start_election (send2);
ASSERT_TIMELY (5s, node.active.election (send2->qualified_root ()));

node.aggregator.request (request, dummy_channel);
ASSERT_TIMELY (3s, 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_TIMELY_EQ (3s, 4, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cannot_vote));
ASSERT_TIMELY_EQ (3s, 4, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_non_final));
ASSERT_EQ (0, 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_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::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));

// Confirm send1
node.start_election (send1);
std::shared_ptr<nano::election> election;
ASSERT_TIMELY (5s, election = node.active.election (send1->qualified_root ()));
election->force_confirm ();
ASSERT_TIMELY (3s, node.ledger.dependents_confirmed (node.ledger.tx_begin_read (), *send2));
// Confirm send1 and send2
nano::test::confirm (node.ledger, { send1, send2 });

node.aggregator.request (request, dummy_channel);
ASSERT_TIMELY (3s, node.aggregator.empty ());
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 (4, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_cannot_vote));
ASSERT_EQ (4, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_non_final));
ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_hashes));
ASSERT_TIMELY_EQ (3s, 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_unknown));
ASSERT_TIMELY (3s, 1 <= node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
}
}
Loading

0 comments on commit a7a2c0b

Please sign in to comment.