Perform wallet representative action without holding any mutex (#2759)

* Perform wallet representative action without holding any mutex

 This relaxes restrictions on the action and avoids potential deadlocks through lock-order-inversion

* Relax debug_assert when adding to votes_cache as the representative counts can change even during a foreach_representative action

* Windows requires explicitly unlocking the mutex once locked

nano/core_test/wallet.cpp

@@ -1556,3 +1556,22 @@ TEST (wallet, epoch_2_receive_unopened)
 	}
 	ASSERT_LT (tries, max_tries);
 }
+
+TEST (wallet, foreach_representative_deadlock)
+{
+	nano::system system (1);
+	auto & node (*system.nodes[0]);
+	system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv);
+	node.wallets.compute_reps ();
+	ASSERT_EQ (1, node.wallets.rep_counts ().voting);
+	node.wallets.foreach_representative ([&node](nano::public_key const & pub, nano::raw_key const & prv) {
+		if (node.wallets.mutex.try_lock ())
+		{
+			node.wallets.mutex.unlock ();
+		}
+		else
+		{
+			ASSERT_FALSE (true);
+		}
+	});
+}

nano/node/voting.cpp

@@ -137,9 +137,12 @@ wallets (wallets_a)
 
 void nano::votes_cache::add (std::shared_ptr<nano::vote> const & vote_a)
 {
-	nano::lock_guard<std::mutex> lock (cache_mutex);
 	auto voting (wallets.rep_counts ().voting);
-	debug_assert (voting > 0);
+	if (voting == 0)
+	{
+		return;
+	}
+	nano::lock_guard<std::mutex> lock (cache_mutex);
 	auto const max_cache_size (network_params.voting.max_cache / std::max (voting, static_cast<decltype (voting)> (1)));
 	for (auto & block : vote_a->blocks)
 	{

nano/node/wallet.cpp

@@ -1750,44 +1750,51 @@ void nano::wallets::foreach_representative (std::function<void(nano::public_key
 {
 	if (node.config.enable_voting)
 	{
-		nano::lock_guard<std::mutex> lock (mutex);
+		std::vector<std::pair<nano::public_key const, nano::raw_key const>> action_accounts_l;
-		auto transaction_l (tx_begin_read ());
-		for (auto i (items.begin ()), n (items.end ()); i != n; ++i)
 		{
-			auto & wallet (*i->second);
+			auto transaction_l (tx_begin_read ());
-			nano::lock_guard<std::recursive_mutex> store_lock (wallet.store.mutex);
+			nano::lock_guard<std::mutex> lock (mutex);
-			decltype (wallet.representatives) representatives_l;
+			for (auto i (items.begin ()), n (items.end ()); i != n; ++i)
 			{
-				nano::lock_guard<std::mutex> representatives_lock (wallet.representatives_mutex);
+				auto & wallet (*i->second);
-				representatives_l = wallet.representatives;
+				nano::lock_guard<std::recursive_mutex> store_lock (wallet.store.mutex);
-			}
+				decltype (wallet.representatives) representatives_l;
-			for (auto const & account : representatives_l)
-			{
-				if (wallet.store.exists (transaction_l, account))
 				{
-					if (!node.ledger.weight (account).is_zero ())
+					nano::lock_guard<std::mutex> representatives_lock (wallet.representatives_mutex);
+					representatives_l = wallet.representatives;
+				}
+				for (auto const & account : representatives_l)
+				{
+					if (wallet.store.exists (transaction_l, account))
 					{
-						if (wallet.store.valid_password (transaction_l))
+						if (!node.ledger.weight (account).is_zero ())
 						{
-							nano::raw_key prv;
+							if (wallet.store.valid_password (transaction_l))
-							auto error (wallet.store.fetch (transaction_l, account, prv));
-							(void)error;
-							debug_assert (!error);
-							action_a (account, prv);
-						}
-						else
-						{
-							static auto last_log = std::chrono::steady_clock::time_point ();
-							if (last_log < std::chrono::steady_clock::now () - std::chrono::seconds (60))
 							{
-								last_log = std::chrono::steady_clock::now ();
+								nano::raw_key prv;
-								node.logger.always_log (boost::str (boost::format ("Representative locked inside wallet %1%") % i->first.to_string ()));
+								auto error (wallet.store.fetch (transaction_l, account, prv));
+								(void)error;
+								debug_assert (!error);
+								action_accounts_l.emplace_back (account, prv);
+							}
+							else
+							{
+								static auto last_log = std::chrono::steady_clock::time_point ();
+								if (last_log < std::chrono::steady_clock::now () - std::chrono::seconds (60))
+								{
+									last_log = std::chrono::steady_clock::now ();
+									node.logger.always_log (boost::str (boost::format ("Representative locked inside wallet %1%") % i->first.to_string ()));
+								}
 							}
 						}
 					}
 				}
 			}
 		}
+		for (auto const & representative : action_accounts_l)
+		{
+			action_a (representative.first, representative.second);
+		}
 	}
 }