From 6a021191172de7b2316029404f85021b0d06e451 Mon Sep 17 00:00:00 2001 From: clemahieu Date: Wed, 15 Jan 2020 08:05:52 -0600 Subject: [PATCH] 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 | 8 ++++-- nano/core_test/ledger.cpp | 38 +++++++++++++++++--------- nano/core_test/node.cpp | 2 -- nano/node/active_transactions.cpp | 16 +++-------- nano/node/active_transactions.hpp | 4 +-- nano/node/node.cpp | 2 +- nano/node/vote_processor.cpp | 6 +--- 7 files changed, 38 insertions(+), 38 deletions(-) diff --git a/nano/core_test/active_transactions.cpp b/nano/core_test/active_transactions.cpp index 6ceadbdf0b..c6e215f2ea 100644 --- a/nano/core_test/active_transactions.cpp +++ b/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 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 diff --git a/nano/core_test/ledger.cpp b/nano/core_test/ledger.cpp index 44df878d6e..92500e92d1 100644 --- a/nano/core_test/ledger.cpp +++ b/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 lock (node1.active.mutex); - auto votes1 (node1.active.roots.find (send1->qualified_root ())->election); - ASSERT_EQ (1, votes1->last_votes.size ()); + { + nano::lock_guard 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::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::test_genesis_key.pub, nano::test_genesis_key.prv, 2, send1)); - nano::lock_guard lock (node1.active.mutex); - auto votes1 (node1.active.roots.find (send1->qualified_root ())->election); + std::shared_ptr votes1; + { + nano::lock_guard lock (node1.active.mutex); + votes1 = node1.active.roots.find (send1->qualified_root ())->election; + } auto channel (std::make_shared (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::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 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 votes1; + std::shared_ptr votes2; + { + nano::unique_lock 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::test_genesis_key.pub, nano::test_genesis_key.prv, 2, send1)); auto channel (std::make_shared (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 lock (node1.active.mutex); - auto votes1 (node1.active.roots.find (send1->qualified_root ())->election); + std::shared_ptr votes1; + { + nano::unique_lock lock (node1.active.mutex); + votes1 = node1.active.roots.find (send1->qualified_root ())->election; + } auto vote1 (std::make_shared (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send1)); auto channel (std::make_shared (node1.network.udp_channels, node1.network.endpoint (), node1.network_params.protocol.protocol_version)); node1.vote_processor.vote_blocking (transaction, vote1, channel); diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index dabda43e97..535a4f4255 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2796,7 +2796,6 @@ TEST (node, fork_invalid_block_signature_vote_by_hash) auto vote (std::make_shared (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 0, vote_blocks)); { auto transaction (node1.store.tx_begin_read ()); - nano::unique_lock lock (node1.active.mutex); node1.vote_processor.vote_blocking (transaction, vote, std::make_shared (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::test_genesis_key.pub, nano::test_genesis_key.prv, 0, vote_blocks)); { auto transaction (node.store.tx_begin_read ()); - nano::unique_lock lock (node.active.mutex); node.vote_processor.vote_blocking (transaction, vote, std::make_shared (node.network.udp_channels, node.network.endpoint (), node.network_params.protocol.protocol_version)); } system.deadline_set (10s); diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index db04bae1f3..ffcded8510 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -530,7 +530,7 @@ bool nano::active_transactions::add (std::shared_ptr 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 vote_a, bool single_lock) +nano::vote_code nano::active_transactions::vote (std::shared_ptr 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 @@ -538,11 +538,7 @@ nano::vote_code nano::active_transactions::vote (std::shared_ptr vot bool replay (false); bool processed (false); { - nano::unique_lock lock; - if (!single_lock) - { - lock = nano::unique_lock (mutex); - } + nano::lock_guard 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> nano::active_transactions::list_blocks (bool single_lock) +std::deque> nano::active_transactions::list_blocks () { std::deque> result; - nano::unique_lock lock; - if (!single_lock) - { - lock = nano::unique_lock (mutex); - } + nano::lock_guard lock (mutex); for (auto & root : roots) { result.push_back (root.election->status.winner); diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index 5493458c56..2b82cb09e5 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -80,7 +80,7 @@ class active_transactions final bool start (std::shared_ptr, bool const = false, std::function)> const & = [](std::shared_ptr) {}); // clang-format on // Distinguishes replay votes, cannot be determined if the block is not in any election - nano::vote_code vote (std::shared_ptr, bool = false); + nano::vote_code vote (std::shared_ptr); // 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 &); uint64_t active_difficulty (); uint64_t limited_active_difficulty (); - std::deque> list_blocks (bool = false); + std::deque> list_blocks (); void erase (nano::block const &); bool empty (); size_t size (); diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 8c37ef8387..1519e803cc 100644 --- a/nano/node/node.cpp +++ b/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) diff --git a/nano/node/vote_processor.cpp b/nano/node/vote_processor.cpp index 280124589c..b0b6e8bbd9 100644 --- a/nano/node/vote_processor.cpp +++ b/nano/node/vote_processor.cpp @@ -71,7 +71,6 @@ void nano::vote_processor::process_loop () lock.unlock (); verify_votes (votes_l); { - nano::unique_lock 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 vote_a, std::shared_ptr 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