Fix unit test confirmation_height.conflict_rollback_cemented (#3745)

* Added lots of comments and made some improvements to conflict_rollback_cemented

* Introduce `ledger.rollback_failed` stat counter (#3746)

* Use ledger.rollback_failed in confirmation_height.conflict_rollback_cemented

Co-authored-by: Dimitrios Siganos <[email protected]>

nano/core_test/confirmation_height.cpp

@@ -640,66 +640,98 @@ TEST (confirmation_height, all_block_types)
 	test_mode (nano::confirmation_height_mode::unbounded);
 }
 
-/* Bulk of the this test was taken from the node.fork_flip test */
-// Test disabled because it's failing intermittently.
-// PR in which it got disabled: https://github.com/nanocurrency/nano-node/pull/3629
-// Issue for investigating it: https://github.com/nanocurrency/nano-node/issues/3633
-TEST (confirmation_height, DISABLED_conflict_rollback_cemented)
+// this test cements a block on one node and another block on another node
+// it therefore tests that once a block is confirmed it cannot be rolled back
+// and if both nodes have different branches of the fork cemented then it is a permanent fork
+TEST (confirmation_height, conflict_rollback_cemented)
 {
+	// functor to perform the conflict_rollback_cemented test using a certain mode
 	auto test_mode = [] (nano::confirmation_height_mode mode_a) {
-		boost::iostreams::stream_buffer<nano::stringstream_mt_sink> sb;
-		sb.open (nano::stringstream_mt_sink{});
-		nano::boost_log_cerr_redirect redirect_cerr (&sb);
-		nano::system system;
-		nano::node_flags node_flags;
+		nano::state_block_builder builder{};
+		auto const genesis_hash = nano::dev::genesis->hash ();
+
+		nano::system system{};
+		nano::node_flags node_flags{};
 		node_flags.confirmation_height_processor_mode = mode_a;
+
+		// create node 1 and account key1 (no voting key yet)
 		auto node1 = system.add_node (node_flags);
-		auto node2 = system.add_node (node_flags);
-		ASSERT_EQ (1, node1->network.size ());
-		nano::keypair key1;
-		auto send1 (std::make_shared<nano::send_block> (nano::dev::genesis->hash (), key1.pub, nano::dev::constants.genesis_amount - 100, nano::dev::genesis_key.prv, nano::dev::genesis_key.pub, *system.work.generate (nano::dev::genesis->hash ())));
-		nano::publish publish1{ nano::dev::network_params.network, send1 };
-		nano::keypair key2;
-		auto send2 (std::make_shared<nano::send_block> (nano::dev::genesis->hash (), key2.pub, nano::dev::constants.genesis_amount - 100, nano::dev::genesis_key.prv, nano::dev::genesis_key.pub, *system.work.generate (nano::dev::genesis->hash ())));
-		nano::publish publish2{ nano::dev::network_params.network, send2 };
-		auto channel1 (node1->network.udp_channels.create (node1->network.endpoint ()));
-		node1->network.inbound (publish1, channel1);
-		node1->block_processor.flush ();
-		node1->scheduler.flush ();
-		auto channel2 (node2->network.udp_channels.create (node1->network.endpoint ()));
-		node2->network.inbound (publish2, channel2);
-		node2->block_processor.flush ();
-		node2->scheduler.flush ();
-		ASSERT_EQ (1, node1->active.size ());
-		ASSERT_EQ (1, node2->active.size ());
-		system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
-		node1->network.inbound (publish2, channel1);
-		node1->block_processor.flush ();
-		node2->network.inbound (publish1, channel2);
-		node2->block_processor.flush ();
-		auto election (node2->active.election (nano::qualified_root (nano::dev::genesis->hash (), nano::dev::genesis->hash ())));
-		ASSERT_NE (nullptr, election);
-		ASSERT_EQ (1, election->votes ().size ());
-		// Force blocks to be cemented on both nodes
-		{
-			auto transaction (node1->store.tx_begin_write ());
-			ASSERT_TRUE (node1->store.block.exists (transaction, publish1.block->hash ()));
-			node1->store.confirmation_height.put (transaction, nano::dev::genesis->account (), nano::confirmation_height_info{ 2, send2->hash () });
-		}
-		{
-			auto transaction (node2->store.tx_begin_write ());
-			ASSERT_TRUE (node2->store.block.exists (transaction, publish2.block->hash ()));
-			node2->store.confirmation_height.put (transaction, nano::dev::genesis->account (), nano::confirmation_height_info{ 2, send2->hash () });
-		}
+		nano::keypair key1{};
+
+		// create one side of a forked transaction on node1
+		auto send1 = builder.make_block ()
+					 .previous (genesis_hash)
+					 .account (nano::dev::genesis_key.pub)
+					 .representative (nano::dev::genesis_key.pub)
+					 .link (key1.pub)
+					 .balance (nano::dev::constants.genesis_amount - 100)
+					 .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, node1->active.election (send1->qualified_root ()) != nullptr);
 
-		auto rollback_log_entry = boost::str (boost::format ("Failed to roll back %1%") % send2->hash ().to_string ());
-		ASSERT_TIMELY (20s, sb.component ()->str ().find (rollback_log_entry) != std::string::npos);
-		auto winner (*election->tally ().begin ());
-		ASSERT_EQ (*publish1.block, *winner.second);
-		ASSERT_EQ (nano::dev::constants.genesis_amount - 100, winner.first);
-		ASSERT_TRUE (node1->ledger.block_or_pruned_exists (publish1.block->hash ()));
-		ASSERT_TRUE (node2->ledger.block_or_pruned_exists (publish2.block->hash ()));
-		ASSERT_FALSE (node2->ledger.block_or_pruned_exists (publish1.block->hash ()));
+		// create the other side of the fork on node2
+		nano::keypair key2;
+		auto send2 = builder.make_block ()
+					 .previous (genesis_hash)
+					 .account (nano::dev::genesis_key.pub)
+					 .representative (nano::dev::genesis_key.pub)
+					 .link (key2.pub)
+					 .balance (nano::dev::constants.genesis_amount - 100)
+					 .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
+					 .work (*system.work.generate (genesis_hash))
+					 .build_shared ();
+
+		// create node2, with send2 pre-initialised in the ledger so that block send1 cannot possibly get in the ledger first
+		system.initialization_blocks.push_back (send2);
+		auto node2 = system.add_node (node_flags);
+		system.initialization_blocks.clear ();
+		auto wallet1 = system.wallet (0);
+		node2->process_active (send2);
+		ASSERT_TIMELY (5s, node2->active.election (send2->qualified_root ()) != nullptr);
+
+		// force confirm send2 on node2
+		ASSERT_TIMELY (5s, node2->ledger.store.block.get (node2->ledger.store.tx_begin_read (), send2->hash ()));
+		node2->process_confirmed (nano::election_status{ send2 });
+		ASSERT_TIMELY (5s, node2->block_confirmed (send2->hash ()));
+
+		// make node1 a voting node (it has all the voting weight)
+		// from now on, node1 can vote for send1 at any time
+		wallet1->insert_adhoc (nano::dev::genesis_key.prv);
+
+		// we expect node1 to vote for one side of the fork only, whichever side
+		std::shared_ptr<nano::election> election_send1_node1{};
+		ASSERT_EQ (send1->qualified_root (), send2->qualified_root ());
+		ASSERT_TIMELY (5s, (election_send1_node1 = node1->active.election (send1->qualified_root ())) != nullptr);
+		ASSERT_TIMELY (5s, 2 == election_send1_node1->votes ().size ());
+
+		// check that the send1 on node1 won the election and got confirmed
+		// this happens because send1 is seen first by node1, and therefore it already winning and it cannot replaced by send2
+		ASSERT_TIMELY (5s, election_send1_node1->confirmed ());
+		auto const winner = election_send1_node1->winner ();
+		ASSERT_NE (nullptr, winner);
+		ASSERT_EQ (*winner, *send1);
+
+		// node2 already has send2 forced confirmed whilst node1 should have confirmed send1 and therefore we have a cemented fork on node2
+		// and node2 should print an error message on the log that it cannot rollback send2 because it is already cemented
+		ASSERT_TIMELY (5s, 1 == node2->stats.count (nano::stat::type::ledger, nano::stat::detail::rollback_failed));
+
+		// get the tally for election the election on node1
+		// we expect the winner to be send1 and we expect send1 to have "genesis balance" vote weight
+		auto const tally = election_send1_node1->tally ();
+		ASSERT_FALSE (tally.empty ());
+		auto const & [amount, winner_alias] = *tally.begin ();
+		ASSERT_EQ (*winner_alias, *send1);
+		ASSERT_EQ (amount, nano::dev::constants.genesis_amount - 100);
+
+		// we expect send1 to exist on node1, is that because send2 is rolled back?
+		ASSERT_TRUE (node1->ledger.block_or_pruned_exists (send1->hash ()));
+		ASSERT_FALSE (node1->ledger.block_or_pruned_exists (send2->hash ()));
+
+		// we expect only send2  to be existing on node2
+		ASSERT_TRUE (node2->ledger.block_or_pruned_exists (send2->hash ()));
+		ASSERT_FALSE (node2->ledger.block_or_pruned_exists (send1->hash ()));
 	};
 
 	test_mode (nano::confirmation_height_mode::bounded);

nano/lib/stats.cpp

@@ -612,6 +612,9 @@ std::string nano::stat::detail_to_string (stat::detail detail)
 		case nano::stat::detail::gap_source:
 			res = "gap_source";
 			break;
+		case nano::stat::detail::rollback_failed:
+			res = "rollback_failed";
+			break;
 		case nano::stat::detail::frontier_confirmation_failed:
 			res = "frontier_confirmation_failed";
 			break;

nano/lib/stats.hpp

@@ -272,6 +272,7 @@ class stat final
 		old,
 		gap_previous,
 		gap_source,
+		rollback_failed,
 
 		// message specific
 		keepalive,

nano/node/blockprocessor.cpp

@@ -280,6 +280,7 @@ void nano::block_processor::process_batch (nano::unique_lock<nano::mutex> & lock
 				std::vector<std::shared_ptr<nano::block>> rollback_list;
 				if (node.ledger.rollback (transaction, successor->hash (), rollback_list))
 				{
+					node.stats.inc (nano::stat::type::ledger, nano::stat::detail::rollback_failed);
 					node.logger.always_log (nano::severity_level::error, boost::str (boost::format ("Failed to roll back %1% because it or a successor was confirmed") % successor->hash ().to_string ()));
 				}
 				else if (node.config.logging.ledger_rollback_logging ())

nano/secure/ledger.hpp

@@ -12,6 +12,7 @@ class store;
 class stat;
 class write_transaction;
 
+// map of vote weight per block, ordered greater first
 using tally_t = std::map<nano::uint128_t, std::shared_ptr<nano::block>, std::greater<nano::uint128_t>>;
 
 class uncemented_info

nano/test_common/system.hpp

@@ -55,8 +55,6 @@ class system final
 	std::chrono::time_point<std::chrono::steady_clock, std::chrono::duration<double>> deadline{ std::chrono::steady_clock::time_point::max () };
 	double deadline_scaling_factor{ 1.0 };
 	unsigned node_sequence{ 0 };
-
-private:
 	std::vector<std::shared_ptr<nano::block>> initialization_blocks;
 };
 std::unique_ptr<nano::state_block> upgrade_epoch (nano::work_pool &, nano::ledger &, nano::epoch);