From 35fc9f68603be3e25ed6a6487cd1cd55da0fc068 Mon Sep 17 00:00:00 2001 From: clemahieu Date: Tue, 30 Mar 2021 11:32:15 +0200 Subject: [PATCH] Putting block announcing in the active election state and removing the rebroadcasting state entirely. (#3173) This was depending on a certain count of successful confirmation requests to move to the rebroadcasting state. This could leave elections hanging if confirmation reqs can't be published to another PR for whatever reason. This additionally only publishes blocks when an election has started which is the new desired behavior. --- nano/core_test/active_transactions.cpp | 15 ++-- nano/core_test/node.cpp | 103 +++++++++++-------------- nano/node/blockprocessor.cpp | 4 - nano/node/election.cpp | 19 ----- nano/node/election.hpp | 1 - 5 files changed, 55 insertions(+), 87 deletions(-) diff --git a/nano/core_test/active_transactions.cpp b/nano/core_test/active_transactions.cpp index 46fa06c1..a5c4f953 100644 --- a/nano/core_test/active_transactions.cpp +++ b/nano/core_test/active_transactions.cpp @@ -740,11 +740,11 @@ TEST (active_transactions, dropped_cleanup) TEST (active_transactions, republish_winner) { nano::system system; - nano::node_config node_config (nano::get_available_port (), system.logging); + 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)); + auto & node1 = *system.add_node (node_config); node_config.peering_port = nano::get_available_port (); - auto & node2 (*system.add_node (node_config)); + auto & node2 = *system.add_node (node_config); nano::genesis genesis; nano::keypair key; @@ -794,12 +794,15 @@ TEST (active_transactions, republish_winner) node1.process_active (fork); node1.block_processor.flush (); - auto vote (std::make_shared (nano::dev_genesis_key.pub, nano::dev_genesis_key.prv, 0, std::vector{ fork->hash () })); + auto election = node1.active.election (fork->qualified_root ()); + ASSERT_NE (nullptr, election); + auto vote = std::make_shared (nano::dev_genesis_key.pub, nano::dev_genesis_key.prv, 0, std::vector{ fork->hash () }); node1.vote_processor.vote (vote, std::make_shared (node1)); node1.vote_processor.flush (); node1.block_processor.flush (); - - ASSERT_TIMELY (3s, node2.stats.count (nano::stat::type::message, nano::stat::detail::publish, nano::stat::dir::in) == 2); + ASSERT_TIMELY (3s, election->confirmed ()); + ASSERT_EQ (fork->hash (), election->status.winner->hash ()); + ASSERT_TIMELY (3s, node2.block_confirmed (fork->hash ())); } TEST (active_transactions, fork_filter_cleanup) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 4197569f..3e48e591 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -3016,53 +3016,42 @@ TEST (node, vote_by_hash_bundle) TEST (node, vote_by_hash_republish) { - std::vector types{ nano::transport::transport_type::tcp, nano::transport::transport_type::udp }; - for (auto & type : types) - { - nano::node_flags node_flags; - if (type == nano::transport::transport_type::udp) - { - node_flags.disable_tcp_realtime = true; - node_flags.disable_bootstrap_listener = true; - node_flags.disable_udp = false; - } - nano::system system (2, type, node_flags); - auto & node1 (*system.nodes[0]); - auto & node2 (*system.nodes[1]); - nano::keypair key2; - system.wallet (1)->insert_adhoc (key2.prv); - nano::genesis genesis; - nano::send_block_builder builder; - auto send1 = builder.make_block () - .previous (genesis.hash ()) - .destination (key2.pub) - .balance (std::numeric_limits::max () - node1.config.receive_minimum.number ()) - .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) - .work (*system.work.generate (genesis.hash ())) - .build_shared (); - auto send2 = builder.make_block () - .previous (genesis.hash ()) - .destination (key2.pub) - .balance (std::numeric_limits::max () - node1.config.receive_minimum.number () * 2) - .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) - .work (*system.work.generate (genesis.hash ())) - .build_shared (); - node1.process_active (send1); - ASSERT_TIMELY (5s, node2.active.active (*send1)); - node1.active.publish (send2); - std::vector vote_blocks; - vote_blocks.push_back (send2->hash ()); - auto vote (std::make_shared (nano::dev_genesis_key.pub, nano::dev_genesis_key.prv, std::numeric_limits::max (), vote_blocks)); - ASSERT_TRUE (node1.active.active (*send1)); - ASSERT_TRUE (node2.active.active (*send1)); - node1.vote_processor.vote (vote, std::make_shared (node1)); - ASSERT_TIMELY (10s, node1.block (send2->hash ())); - ASSERT_TIMELY (10s, node2.block (send2->hash ())); - ASSERT_FALSE (node1.block (send1->hash ())); - ASSERT_FALSE (node2.block (send1->hash ())); - ASSERT_TIMELY (5s, node2.balance (key2.pub) == node1.config.receive_minimum.number () * 2); - ASSERT_TIMELY (10s, node1.balance (key2.pub) == node1.config.receive_minimum.number () * 2); - } + nano::system system{ 2 }; + auto & node1 = *system.nodes[0]; + auto & node2 = *system.nodes[1]; + nano::keypair key2; + system.wallet (1)->insert_adhoc (key2.prv); + nano::genesis genesis; + nano::send_block_builder builder; + auto send1 = builder.make_block () + .previous (genesis.hash ()) + .destination (key2.pub) + .balance (std::numeric_limits::max () - node1.config.receive_minimum.number ()) + .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) + .work (*system.work.generate (genesis.hash ())) + .build_shared (); + auto send2 = builder.make_block () + .previous (genesis.hash ()) + .destination (key2.pub) + .balance (std::numeric_limits::max () - node1.config.receive_minimum.number () * 2) + .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) + .work (*system.work.generate (genesis.hash ())) + .build_shared (); + node1.process_active (send1); + ASSERT_TIMELY (5s, node2.active.active (*send1)); + node1.process_active (send2); + std::vector vote_blocks; + vote_blocks.push_back (send2->hash ()); + auto vote = std::make_shared (nano::dev_genesis_key.pub, nano::dev_genesis_key.prv, std::numeric_limits::max (), vote_blocks); + ASSERT_TRUE (node1.active.active (*send1)); + ASSERT_TRUE (node2.active.active (*send1)); + node1.vote_processor.vote (vote, std::make_shared (node1)); + ASSERT_TIMELY (10s, node1.block (send2->hash ())); + ASSERT_TIMELY (10s, node2.block (send2->hash ())); + ASSERT_FALSE (node1.block (send1->hash ())); + ASSERT_FALSE (node2.block (send1->hash ())); + ASSERT_TIMELY (5s, node2.balance (key2.pub) == node1.config.receive_minimum.number () * 2); + ASSERT_TIMELY (10s, node1.balance (key2.pub) == node1.config.receive_minimum.number () * 2); } TEST (node, vote_by_hash_epoch_block_republish) @@ -4503,14 +4492,14 @@ TEST (node, deferred_dependent_elections) .representative (nano::dev_genesis_key.pub) .sign (key.prv, key.pub) .build_shared (); - node.process_active (send1); + node.process_local (send1); node.block_processor.flush (); auto election_send1 = node.active.election (send1->qualified_root ()); ASSERT_NE (nullptr, election_send1); // Should process and republish but not start an election for any dependent blocks - node.process_active (open); - node.process_active (send2); + node.process_local (open, false); + node.process_local (send2, false); node.block_processor.flush (); ASSERT_TRUE (node.block (open->hash ())); ASSERT_TRUE (node.block (send2->hash ())); @@ -4521,7 +4510,7 @@ TEST (node, deferred_dependent_elections) // Re-processing older blocks with updated work also does not start an election node.work_generate_blocking (*open, open->difficulty () + 1); - node.process_active (open); + node.process_local (open, false); node.block_processor.flush (); ASSERT_FALSE (node.active.active (open->qualified_root ())); /// However, work is still updated @@ -4536,7 +4525,7 @@ TEST (node, deferred_dependent_elections) /// The election was dropped but it's still not possible to restart it node.work_generate_blocking (*open, open->difficulty () + 1); ASSERT_FALSE (node.active.active (open->qualified_root ())); - node.process_active (open); + node.process_local (open, false); node.block_processor.flush (); ASSERT_FALSE (node.active.active (open->qualified_root ())); /// However, work is still updated @@ -4544,7 +4533,7 @@ TEST (node, deferred_dependent_elections) // Frontier confirmation also starts elections ASSERT_NO_ERROR (system.poll_until_true (5s, [&node, &send2] { - nano::unique_lock lock (node.active.mutex); + nano::unique_lock lock{ node.active.mutex }; node.active.frontiers_confirmation (lock); lock.unlock (); return node.active.election (send2->qualified_root ()) != nullptr; @@ -4575,14 +4564,14 @@ TEST (node, deferred_dependent_elections) ASSERT_FALSE (node.active.active (receive->qualified_root ())); ASSERT_FALSE (node.ledger.rollback (node.store.tx_begin_write (), receive->hash ())); ASSERT_FALSE (node.block (receive->hash ())); - node.process_active (receive); + node.process_local (receive, false); node.block_processor.flush (); ASSERT_TRUE (node.block (receive->hash ())); ASSERT_FALSE (node.active.active (receive->qualified_root ())); // Processing a fork will also not start an election ASSERT_EQ (nano::process_result::fork, node.process (*fork).code); - node.process_active (fork); + node.process_local (fork, false); node.block_processor.flush (); ASSERT_FALSE (node.active.active (receive->qualified_root ())); @@ -4592,7 +4581,7 @@ TEST (node, deferred_dependent_elections) ASSERT_TIMELY (2s, node.active.active (receive->qualified_root ())); node.active.erase (*receive); ASSERT_FALSE (node.active.active (receive->qualified_root ())); - node.process_active (fork); + node.process_local (fork, false); node.block_processor.flush (); ASSERT_TRUE (node.active.active (receive->qualified_root ())); @@ -4600,7 +4589,7 @@ TEST (node, deferred_dependent_elections) node.active.erase (*fork); ASSERT_FALSE (node.active.active (fork->qualified_root ())); node.work_generate_blocking (*receive, receive->difficulty () + 1); - node.process_active (receive); + node.process_local (receive, false); node.block_processor.flush (); ASSERT_TRUE (node.active.active (receive->qualified_root ())); } diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index 894eaac5..26d27c0b 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -360,10 +360,6 @@ void nano::block_processor::process_live (nano::transaction const & transaction_ { node.network.flood_block_initial (block_a); } - else if (!node.flags.disable_block_processor_republishing) - { - node.network.flood_block (block_a, nano::buffer_drop_policy::no_limiter_drop); - } if (node.websocket_server && node.websocket_server->any_subscriber (nano::websocket::topic::new_unconfirmed_block)) { diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 70ed03e3..4257d753 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -83,18 +83,6 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele } break; case nano::election::state_t::active: - switch (desired_a) - { - case nano::election::state_t::broadcasting: - case nano::election::state_t::confirmed: - case nano::election::state_t::expired_unconfirmed: - result = true; - break; - default: - break; - } - break; - case nano::election::state_t::broadcasting: switch (desired_a) { case nano::election::state_t::confirmed: @@ -188,13 +176,6 @@ bool nano::election::transition_time (nano::confirmation_solicitor & solicitor_a } break; case nano::election::state_t::active: - send_confirm_req (solicitor_a); - if (confirmation_request_count > active_request_count_min) - { - state_change (nano::election::state_t::active, nano::election::state_t::broadcasting); - } - break; - case nano::election::state_t::broadcasting: broadcast_block (solicitor_a); send_confirm_req (solicitor_a); break; diff --git a/nano/node/election.hpp b/nano/node/election.hpp index 677eb3d4..a215448b 100644 --- a/nano/node/election.hpp +++ b/nano/node/election.hpp @@ -70,7 +70,6 @@ private: // State management { passive, // only listening for incoming votes active, // actively request confirmations - broadcasting, // request confirmations and broadcast the winner confirmed, // confirmed but still listening for votes expired_confirmed, expired_unconfirmed