Incorrect cemented count during conf height algo transition

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);

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 ())

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<container_info_component> collect_container_info (confirmation_height_processor &, const std::string &);

nano/node/confirmation_height_unbounded.hpp

@@ -100,6 +100,7 @@ class confirmation_height_unbounded final
 	std::function<void(nano::block_hash const &)> notify_block_already_cemented_observers_callback;
 	std::function<uint64_t ()> awaiting_processing_size_callback;
 
+	friend class confirmation_height_dynamic_algorithm_no_transition_while_pending_Test;
 	friend std::unique_ptr<nano::container_info_component> collect_container_info (confirmation_height_unbounded &, const std::string & name_a);
 };
 

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<std::shared_ptr<nano::state_block>> 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::state_block> (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)
 {