diff --git a/nano/core_test/confirmation_height.cpp b/nano/core_test/confirmation_height.cpp index 02b6ca642c..7f6b073a15 100644 --- a/nano/core_test/confirmation_height.cpp +++ b/nano/core_test/confirmation_height.cpp @@ -72,6 +72,7 @@ TEST (confirmation_height, single) ASSERT_EQ (1, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (1, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); ASSERT_EQ (1, node->stats.count (nano::stat::type::http_callback, nano::stat::detail::http_callback, nano::stat::dir::out)); + ASSERT_EQ (2, node->ledger.cache.cemented_count); ASSERT_EQ (0, node->active.election_winner_details_size ()); } @@ -215,6 +216,7 @@ TEST (confirmation_height, multiple_accounts) ASSERT_EQ (10, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (10, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); ASSERT_EQ (10, node->stats.count (nano::stat::type::http_callback, nano::stat::detail::http_callback, nano::stat::dir::out)); + ASSERT_EQ (11, node->ledger.cache.cemented_count); ASSERT_EQ (0, node->active.election_winner_details_size ()); }; @@ -292,6 +294,7 @@ TEST (confirmation_height, gap_bootstrap) ASSERT_EQ (0, node1.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (0, node1.stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); ASSERT_EQ (0, node1.stats.count (nano::stat::type::http_callback, nano::stat::detail::http_callback, nano::stat::dir::out)); + ASSERT_EQ (1, node1.ledger.cache.cemented_count); ASSERT_EQ (0, node1.active.election_winner_details_size ()); }; @@ -378,6 +381,7 @@ TEST (confirmation_height, gap_live) ASSERT_EQ (6, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (6, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); ASSERT_EQ (6, node->stats.count (nano::stat::type::http_callback, nano::stat::detail::http_callback, nano::stat::dir::out)); + ASSERT_EQ (7, node->ledger.cache.cemented_count); ASSERT_EQ (0, node->active.election_winner_details_size ()); }; @@ -753,6 +757,7 @@ TEST (confirmation_height, observers) ASSERT_EQ (1, node1->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (1, node1->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); ASSERT_EQ (1, node1->stats.count (nano::stat::type::http_callback, nano::stat::detail::http_callback, nano::stat::dir::out)); + ASSERT_EQ (2, node1->ledger.cache.cemented_count); ASSERT_EQ (0, node1->active.election_winner_details_size ()); }; @@ -818,12 +823,14 @@ TEST (confirmation_height, modified_chain) ASSERT_EQ (1, confirmation_height_info.height); ASSERT_EQ (nano::genesis_hash, confirmation_height_info.frontier); ASSERT_EQ (1, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::invalid_block, nano::stat::dir::in)); + ASSERT_EQ (1, node->ledger.cache.cemented_count); } else { // A non-existent block is cemented, expected given these conditions but is of course incorrect. ASSERT_EQ (2, confirmation_height_info.height); ASSERT_EQ (send->hash (), confirmation_height_info.frontier); + ASSERT_EQ (2, node->ledger.cache.cemented_count); } ASSERT_EQ (0, node->active.election_winner_details_size ()); @@ -871,6 +878,7 @@ TEST (confirmation_height, pending_observer_callbacks) ASSERT_EQ (2, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (2, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); + ASSERT_EQ (3, node->ledger.cache.cemented_count); ASSERT_EQ (0, node->active.election_winner_details_size ()); }; @@ -1172,7 +1180,7 @@ TEST (confirmation_height, callback_confirmed_history) ASSERT_EQ (1, node->stats.count (nano::stat::type::observer, nano::stat::detail::observer_confirmation_inactive, nano::stat::dir::out)); ASSERT_EQ (2, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (2, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); - + ASSERT_EQ (3, node->ledger.cache.cemented_count); ASSERT_EQ (0, node->active.election_winner_details_size ()); }; @@ -1233,6 +1241,7 @@ TEST (confirmation_height, dependent_election) ASSERT_EQ (1, node->stats.count (nano::stat::type::observer, nano::stat::detail::observer_confirmation_inactive, nano::stat::dir::out)); ASSERT_EQ (3, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (3, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); + ASSERT_EQ (4, node->ledger.cache.cemented_count); ASSERT_EQ (0, node->active.election_winner_details_size ()); }; @@ -1314,6 +1323,7 @@ TEST (confirmation_height, cemented_gap_below_receive) ASSERT_EQ (9, node->stats.count (nano::stat::type::observer, nano::stat::detail::observer_confirmation_inactive, nano::stat::dir::out)); ASSERT_EQ (10, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (10, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); + ASSERT_EQ (11, node->ledger.cache.cemented_count); ASSERT_EQ (0, node->active.election_winner_details_size ()); // Check that the order of callbacks is correct @@ -1406,6 +1416,7 @@ TEST (confirmation_height, cemented_gap_below_no_cache) ASSERT_EQ (5, node->stats.count (nano::stat::type::observer, nano::stat::detail::observer_confirmation_inactive, nano::stat::dir::out)); ASSERT_EQ (6, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (6, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); + ASSERT_EQ (7, node->ledger.cache.cemented_count); }; test_mode (nano::confirmation_height_mode::bounded); @@ -1486,6 +1497,7 @@ TEST (confirmation_height, election_winner_details_clearing) ASSERT_EQ (2, node->stats.count (nano::stat::type::observer, nano::stat::detail::observer_confirmation_active_quorum, nano::stat::dir::out)); ASSERT_EQ (3, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (3, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); + ASSERT_EQ (4, node->ledger.cache.cemented_count); }; test_mode (nano::confirmation_height_mode::bounded); diff --git a/nano/node/confirmation_height_processor.cpp b/nano/node/confirmation_height_processor.cpp index cad2739572..8b9366fc4d 100644 --- a/nano/node/confirmation_height_processor.cpp +++ b/nano/node/confirmation_height_processor.cpp @@ -66,7 +66,9 @@ void nano::confirmation_height_processor::run (confirmation_height_mode mode_a) auto blocks_within_automatic_unbounded_selection = (ledger.cache.block_count < num_blocks_to_use_unbounded || ledger.cache.block_count - num_blocks_to_use_unbounded < ledger.cache.cemented_count); // Don't want to mix up pending writes across different processors - if (mode_a == confirmation_height_mode::unbounded || (mode_a == confirmation_height_mode::automatic && blocks_within_automatic_unbounded_selection && confirmation_height_bounded_processor.pending_empty ())) + auto valid_unbounded = (mode_a == confirmation_height_mode::automatic && blocks_within_automatic_unbounded_selection && confirmation_height_bounded_processor.pending_empty ()); + auto force_unbounded = (!confirmation_height_unbounded_processor.pending_empty () || mode_a == confirmation_height_mode::unbounded); + if (force_unbounded || valid_unbounded) { debug_assert (confirmation_height_bounded_processor.pending_empty ()); if (confirmation_height_unbounded_processor.pending_empty ()) diff --git a/nano/node/confirmation_height_processor.hpp b/nano/node/confirmation_height_processor.hpp index 1763b0fc43..f7fa090e73 100644 --- a/nano/node/confirmation_height_processor.hpp +++ b/nano/node/confirmation_height_processor.hpp @@ -69,6 +69,7 @@ class confirmation_height_processor final friend class confirmation_height_pending_observer_callbacks_Test; friend class confirmation_height_dependent_election_Test; friend class confirmation_height_dependent_election_after_already_cemented_Test; + friend class confirmation_height_dynamic_algorithm_no_transition_while_pending_Test; }; std::unique_ptr collect_container_info (confirmation_height_processor &, const std::string &); diff --git a/nano/node/confirmation_height_unbounded.hpp b/nano/node/confirmation_height_unbounded.hpp index c95059bb3d..a233d1716f 100644 --- a/nano/node/confirmation_height_unbounded.hpp +++ b/nano/node/confirmation_height_unbounded.hpp @@ -100,6 +100,7 @@ class confirmation_height_unbounded final std::function notify_block_already_cemented_observers_callback; std::function awaiting_processing_size_callback; + friend class confirmation_height_dynamic_algorithm_no_transition_while_pending_Test; friend std::unique_ptr collect_container_info (confirmation_height_unbounded &, const std::string & name_a); }; diff --git a/nano/slow_test/node.cpp b/nano/slow_test/node.cpp index 1532306a35..face20e4ad 100644 --- a/nano/slow_test/node.cpp +++ b/nano/slow_test/node.cpp @@ -744,6 +744,97 @@ TEST (confirmation_height, dynamic_algorithm) namespace nano { +/* + * This tests an issue of incorrect cemented block counts during the transition of conf height algorithms + * The scenario was as follows: + * - There is at least 1 pending write entry in the unbounded conf height processor + * - 0 blocks currently awaiting processing in the main conf height processor class + * - A block was confirmed when hit the chain in the pending write above but was not a block higher than it. + * - It must be in `confirmation_height_processor::pause ()` function so that `pause` is set (and the difference between the number + * of blocks uncemented is > unbounded_cutoff so that it hits the bounded processor), the main `run` loop on the conf height processor is iterated. + * + * This cause unbounded pending entries not to be written, and then the bounded processor would write them, causing some inconsistencies. +*/ +TEST (confirmation_height, dynamic_algorithm_no_transition_while_pending) +{ + // Repeat in case of intermittent issues not replicating the issue talked about above. + for (auto _ = 0; _ < 3; ++_) + { + 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::keypair key; + system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv); + + auto latest_genesis = node->latest (nano::test_genesis_key.pub); + std::vector> state_blocks; + auto const num_blocks = nano::confirmation_height::unbounded_cutoff - 2; + + auto add_block_to_genesis_chain = [&](nano::write_transaction & transaction) { + static int num = 0; + auto send (std::make_shared (nano::test_genesis_key.pub, latest_genesis, nano::test_genesis_key.pub, nano::genesis_amount - num - 1, key.pub, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *system.work.generate (latest_genesis))); + latest_genesis = send->hash (); + state_blocks.push_back (send); + ASSERT_EQ (nano::process_result::progress, node->ledger.process (transaction, *send).code); + ++num; + }; + + for (auto i = 0; i < num_blocks; ++i) + { + auto transaction = node->store.tx_begin_write (); + add_block_to_genesis_chain (transaction); + } + + { + auto write_guard = node->write_database_queue.wait (nano::writer::testing); + // To limit any data races we are not calling node.block_confirm, + // the relevant code is replicated here (implementation detail): + node->confirmation_height_processor.pause (); + node->confirmation_height_processor.add (state_blocks.back ()->hash ()); + node->confirmation_height_processor.unpause (); + + nano::timer<> timer; + timer.start (); + while (node->confirmation_height_processor.current ().is_zero ()) + { + ASSERT_LT (timer.since_start (), 2s); + } + + // Pausing prevents any writes in the outer while loop in the confirmaiton height processor (implementation detail) + node->confirmation_height_processor.pause (); + + timer.restart (); + while (node->confirmation_height_processor.confirmation_height_unbounded_processor.pending_writes_size == 0) + { + ASSERT_NO_ERROR (system.poll ()); + } + + { + // Make it so that the number of blocks exceed the unbounded cutoff would go into the bounded processor (but shouldn't due to unbounded pending writes) + auto transaction = node->store.tx_begin_write (); + add_block_to_genesis_chain (transaction); + add_block_to_genesis_chain (transaction); + } + // Make sure this is at a height lower than the block in the add () call above + node->confirmation_height_processor.add (state_blocks.front ()->hash ()); + node->confirmation_height_processor.unpause (); + } + + system.deadline_set (10s); + while (node->confirmation_height_processor.awaiting_processing_size () != 0 || !node->confirmation_height_processor.current ().is_zero ()) + { + ASSERT_NO_ERROR (system.poll ()); + } + + ASSERT_EQ (node->ledger.cache.cemented_count, num_blocks + 1); + ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in), num_blocks); + ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_bounded, nano::stat::dir::in), 0); + ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_unbounded, nano::stat::dir::in), num_blocks); + ASSERT_EQ (node->active.election_winner_details_size (), 0); + } +} + // Can take up to 1 hour TEST (confirmation_height, prioritize_frontiers_overwrite) {