Remove unnecessary lock requirement in active_transactions::lock. (#2475

)

* Remove unnecessarry lock requirement in active_transactions::lock.
This also removes some code-smell around passing in a bool to determine if the lock should actually be acquired.

* Using a timeout counter in line with other test idioms.

nano/core_test/active_transactions.cpp

@@ -637,11 +637,13 @@ TEST (active_transactions, restart_dropped)
 		}
 		ASSERT_NO_ERROR (system.poll ());
 	}
-	// Verify the block was updated in the ledger
+	std::shared_ptr<nano::block> block;
+	while (block == nullptr)
 	{
-		auto block (node.store.block_get (node.store.tx_begin_read (), send1->hash ()));
-		ASSERT_EQ (work2, block->block_work ());
+		ASSERT_NO_ERROR (system.poll ());
+		block = node.store.block_get (node.store.tx_begin_read (), send1->hash ());
 	}
+	ASSERT_EQ (work2, block->block_work ());
 	// Drop election
 	node.active.erase (*send2);
 	// Try to restart election with the lower difficulty block, should not work since the block as lower work

nano/core_test/ledger.cpp

@@ -734,9 +734,11 @@ TEST (votes, check_signature)
 		ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
 	}
 	node1.active.start (send1);
-	nano::lock_guard<std::mutex> lock (node1.active.mutex);
-	auto votes1 (node1.active.roots.find (send1->qualified_root ())->election);
-	ASSERT_EQ (1, votes1->last_votes.size ());
+	{
+		nano::lock_guard<std::mutex> lock (node1.active.mutex);
+		auto votes1 (node1.active.roots.find (send1->qualified_root ())->election);
+		ASSERT_EQ (1, votes1->last_votes.size ());
+	}
 	auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send1));
 	vote1->signature.bytes[0] ^= 1;
 	auto transaction (node1.store.tx_begin_read ());
@@ -869,8 +871,11 @@ TEST (votes, add_old)
 	ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
 	node1.active.start (send1);
 	auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 2, send1));
-	nano::lock_guard<std::mutex> lock (node1.active.mutex);
-	auto votes1 (node1.active.roots.find (send1->qualified_root ())->election);
+	std::shared_ptr<nano::election> votes1;
+	{
+		nano::lock_guard<std::mutex> lock (node1.active.mutex);
+		votes1 = node1.active.roots.find (send1->qualified_root ())->election;
+	}
 	auto channel (std::make_shared<nano::transport::channel_udp> (node1.network.udp_channels, node1.network.endpoint (), node1.network_params.protocol.protocol_version));
 	node1.vote_processor.vote_blocking (transaction, vote1, channel);
 	nano::keypair key2;
@@ -879,7 +884,7 @@ TEST (votes, add_old)
 	auto vote2 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send2));
 	votes1->last_votes[nano::test_genesis_key.pub].time = std::chrono::steady_clock::now () - std::chrono::seconds (20);
 	node1.vote_processor.vote_blocking (transaction, vote2, channel);
-	ASSERT_EQ (2, votes1->last_votes.size ());
+	ASSERT_EQ (2, votes1->last_votes_size ());
 	ASSERT_NE (votes1->last_votes.end (), votes1->last_votes.find (nano::test_genesis_key.pub));
 	ASSERT_EQ (send1->hash (), votes1->last_votes[nano::test_genesis_key.pub].hash);
 	auto winner (*votes1->tally ().begin ());
@@ -902,11 +907,15 @@ TEST (votes, add_old_different_account)
 	ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send2).code);
 	node1.active.start (send1);
 	node1.active.start (send2);
-	nano::unique_lock<std::mutex> lock (node1.active.mutex);
-	auto votes1 (node1.active.roots.find (send1->qualified_root ())->election);
-	auto votes2 (node1.active.roots.find (send2->qualified_root ())->election);
-	ASSERT_EQ (1, votes1->last_votes.size ());
-	ASSERT_EQ (1, votes2->last_votes.size ());
+	std::shared_ptr<nano::election> votes1;
+	std::shared_ptr<nano::election> votes2;
+	{
+		nano::unique_lock<std::mutex> lock (node1.active.mutex);
+		votes1 = node1.active.roots.find (send1->qualified_root ())->election;
+		votes2 = node1.active.roots.find (send2->qualified_root ())->election;
+	}
+	ASSERT_EQ (1, votes1->last_votes_size ());
+	ASSERT_EQ (1, votes2->last_votes_size ());
 	auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 2, send1));
 	auto channel (std::make_shared<nano::transport::channel_udp> (node1.network.udp_channels, node1.network.endpoint (), node1.network_params.protocol.protocol_version));
 	auto vote_result1 (node1.vote_processor.vote_blocking (transaction, vote1, channel));
@@ -940,8 +949,11 @@ TEST (votes, add_cooldown)
 	auto transaction (node1.store.tx_begin_write ());
 	ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
 	node1.active.start (send1);
-	nano::unique_lock<std::mutex> lock (node1.active.mutex);
-	auto votes1 (node1.active.roots.find (send1->qualified_root ())->election);
+	std::shared_ptr<nano::election> votes1;
+	{
+		nano::unique_lock<std::mutex> lock (node1.active.mutex);
+		votes1 = node1.active.roots.find (send1->qualified_root ())->election;
+	}
 	auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send1));
 	auto channel (std::make_shared<nano::transport::channel_udp> (node1.network.udp_channels, node1.network.endpoint (), node1.network_params.protocol.protocol_version));
 	node1.vote_processor.vote_blocking (transaction, vote1, channel);

nano/core_test/node.cpp

@@ -2796,7 +2796,6 @@ TEST (node, fork_invalid_block_signature_vote_by_hash)
 	auto vote (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 0, vote_blocks));
 	{
 		auto transaction (node1.store.tx_begin_read ());
-		nano::unique_lock<std::mutex> lock (node1.active.mutex);
 		node1.vote_processor.vote_blocking (transaction, vote, std::make_shared<nano::transport::channel_udp> (node1.network.udp_channels, node1.network.endpoint (), node1.network_params.protocol.protocol_version));
 	}
 	while (node1.block (send1->hash ()))
@@ -2965,7 +2964,6 @@ TEST (node, confirm_back)
 	auto vote (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 0, vote_blocks));
 	{
 		auto transaction (node.store.tx_begin_read ());
-		nano::unique_lock<std::mutex> lock (node.active.mutex);
 		node.vote_processor.vote_blocking (transaction, vote, std::make_shared<nano::transport::channel_udp> (node.network.udp_channels, node.network.endpoint (), node.network_params.protocol.protocol_version));
 	}
 	system.deadline_set (10s);

nano/node/active_transactions.cpp

@@ -530,19 +530,15 @@ bool nano::active_transactions::add (std::shared_ptr<nano::block> block_a, bool
 }
 
 // Validate a vote and apply it to the current election if one exists
-nano::vote_code nano::active_transactions::vote (std::shared_ptr<nano::vote> vote_a, bool single_lock)
+nano::vote_code nano::active_transactions::vote (std::shared_ptr<nano::vote> vote_a)
 {
 	// If none of the hashes are active, it is unknown whether it's a replay
 	// In this case, votes are also not republished
 	bool at_least_one (false);
 	bool replay (false);
 	bool processed (false);
 	{
-		nano::unique_lock<std::mutex> lock;
-		if (!single_lock)
-		{
-			lock = nano::unique_lock<std::mutex> (mutex);
-		}
+		nano::lock_guard<std::mutex> lock (mutex);
 		for (auto vote_block : vote_a->blocks)
 		{
 			nano::election_vote_result result;
@@ -797,14 +793,10 @@ uint64_t nano::active_transactions::limited_active_difficulty ()
 }
 
 // List of active blocks in elections
-std::deque<std::shared_ptr<nano::block>> nano::active_transactions::list_blocks (bool single_lock)
+std::deque<std::shared_ptr<nano::block>> nano::active_transactions::list_blocks ()
 {
 	std::deque<std::shared_ptr<nano::block>> result;
-	nano::unique_lock<std::mutex> lock;
-	if (!single_lock)
-	{
-		lock = nano::unique_lock<std::mutex> (mutex);
-	}
+	nano::lock_guard<std::mutex> lock (mutex);
 	for (auto & root : roots)
 	{
 		result.push_back (root.election->status.winner);

nano/node/active_transactions.hpp

@@ -80,7 +80,7 @@ class active_transactions final
 	bool start (std::shared_ptr<nano::block>, bool const = false, std::function<void(std::shared_ptr<nano::block>)> const & = [](std::shared_ptr<nano::block>) {});
 	// clang-format on
 	// Distinguishes replay votes, cannot be determined if the block is not in any election
-	nano::vote_code vote (std::shared_ptr<nano::vote>, bool = false);
+	nano::vote_code vote (std::shared_ptr<nano::vote>);
 	// Is the root of this block in the roots container
 	bool active (nano::block const &);
 	bool active (nano::qualified_root const &);
@@ -89,7 +89,7 @@ class active_transactions final
 	void update_active_difficulty (nano::unique_lock<std::mutex> &);
 	uint64_t active_difficulty ();
 	uint64_t limited_active_difficulty ();
-	std::deque<std::shared_ptr<nano::block>> list_blocks (bool = false);
+	std::deque<std::shared_ptr<nano::block>> list_blocks ();
 	void erase (nano::block const &);
 	bool empty ();
 	size_t size ();

nano/node/node.cpp

@@ -331,7 +331,7 @@ startup_time (std::chrono::steady_clock::now ())
 						{
 							logger.try_log (boost::str (boost::format ("Found a representative at %1%") % channel_a->to_string ()));
 							// Rebroadcasting all active votes to new representative
-							auto blocks (this->active.list_blocks (true));
+							auto blocks (this->active.list_blocks ());
 							for (auto i (blocks.begin ()), n (blocks.end ()); i != n; ++i)
 							{
 								if (*i != nullptr)

nano/node/vote_processor.cpp

@@ -71,7 +71,6 @@ void nano::vote_processor::process_loop ()
 			lock.unlock ();
 			verify_votes (votes_l);
 			{
-				nano::unique_lock<std::mutex> active_single_lock (active.mutex);
 				auto transaction (store.tx_begin_read ());
 				uint64_t count (1);
 				for (auto & i : votes_l)
@@ -80,9 +79,7 @@ void nano::vote_processor::process_loop ()
 					// Free active_transactions mutex each 100 processed votes
 					if (count % 100 == 0)
 					{
-						active_single_lock.unlock ();
 						transaction.refresh ();
-						active_single_lock.lock ();
 					}
 					count++;
 				}
@@ -198,11 +195,10 @@ void nano::vote_processor::verify_votes (std::deque<std::pair<std::shared_ptr<na
 // node.active.mutex lock required
 nano::vote_code nano::vote_processor::vote_blocking (nano::transaction const & transaction_a, std::shared_ptr<nano::vote> vote_a, std::shared_ptr<nano::transport::channel> channel_a, bool validated)
 {
-	assert (!active.mutex.try_lock ());
 	auto result (nano::vote_code::invalid);
 	if (validated || !vote_a->validate ())
 	{
-		result = active.vote (vote_a, true);
+		result = active.vote (vote_a);
 		observers.vote.notify (vote_a, channel_a, result);
 		// This tries to assist rep nodes that have lost track of their highest sequence number by replaying our highest known vote back to them
 		// Only do this if the sequence number is significantly different to account for network reordering