Skip to content

Commit

Permalink
Fix reported by TSAN issues (#1739)
Browse files Browse the repository at this point in the history 
*  prevent reported possible deadlocks & segfaults
* improve mutexes in core_test & slow_test
  • Loading branch information
@SergiySW
SergiySW authored Feb 18, 2019
1 parent 13f5ab9 commit ba597ff
Show file tree
Hide file tree
Showing 23 changed files with 301 additions and 202 deletions.
2 changes: 1 addition & 1 deletion crypto/ed25519-donna/ed25519-donna-batchverify.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ ge25519_is_neutral_vartime(const ge25519 *p) {
curve25519_contract(point_buffer[0], p->x);
curve25519_contract(point_buffer[1], p->y);
curve25519_contract(point_buffer[2], p->z);
memcpy(batch_point_buffer[1], point_buffer[1], 32);
// memcpy(batch_point_buffer[1], point_buffer[1], 32); // remove used in testing batch_point_buffer to fix tsan warnings
return (memcmp(point_buffer[0], zero, 32) == 0) && (memcmp(point_buffer[1], point_buffer[2], 32) == 0);
}

Expand Down
59 changes: 36 additions & 23 deletions nano/core_test/conflicts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,18 @@ TEST (conflicts, start_stop)
auto send1 (std::make_shared<nano::send_block> (genesis.hash (), key1.pub, 0, nano::test_genesis_key.prv, nano::test_genesis_key.pub, 0));
node1.work_generate_blocking (*send1);
ASSERT_EQ (nano::process_result::progress, node1.process (*send1).code);
ASSERT_EQ (0, node1.active.roots.size ());
ASSERT_EQ (0, node1.active.size ());
node1.active.start (send1);
ASSERT_EQ (1, node1.active.roots.size ());
ASSERT_EQ (1, node1.active.size ());
auto root1 (send1->root ());
auto existing1 (node1.active.roots.find (nano::uint512_union (send1->previous (), root1)));
ASSERT_NE (node1.active.roots.end (), existing1);
auto votes1 (existing1->election);
ASSERT_NE (nullptr, votes1);
ASSERT_EQ (1, votes1->last_votes.size ());
{
std::lock_guard<std::mutex> guard (node1.active.mutex);
auto existing1 (node1.active.roots.find (nano::uint512_union (send1->previous (), root1)));
ASSERT_NE (node1.active.roots.end (), existing1);
auto votes1 (existing1->election);
ASSERT_NE (nullptr, votes1);
ASSERT_EQ (1, votes1->last_votes.size ());
}
}

TEST (conflicts, add_existing)
Expand All @@ -34,14 +37,17 @@ TEST (conflicts, add_existing)
nano::keypair key2;
auto send2 (std::make_shared<nano::send_block> (genesis.hash (), key2.pub, 0, nano::test_genesis_key.prv, nano::test_genesis_key.pub, 0));
node1.active.start (send2);
ASSERT_EQ (1, node1.active.roots.size ());
ASSERT_EQ (1, node1.active.size ());
auto vote1 (std::make_shared<nano::vote> (key2.pub, key2.prv, 0, send2));
node1.active.vote (vote1);
ASSERT_EQ (1, node1.active.roots.size ());
auto votes1 (node1.active.roots.find (nano::uint512_union (send2->previous (), send2->root ()))->election);
ASSERT_NE (nullptr, votes1);
ASSERT_EQ (2, votes1->last_votes.size ());
ASSERT_NE (votes1->last_votes.end (), votes1->last_votes.find (key2.pub));
ASSERT_EQ (1, node1.active.size ());
{
std::lock_guard<std::mutex> guard (node1.active.mutex);
auto votes1 (node1.active.roots.find (nano::uint512_union (send2->previous (), send2->root ()))->election);
ASSERT_NE (nullptr, votes1);
ASSERT_EQ (2, votes1->last_votes.size ());
ASSERT_NE (votes1->last_votes.end (), votes1->last_votes.find (key2.pub));
}
}

TEST (conflicts, add_two)
Expand All @@ -59,7 +65,7 @@ TEST (conflicts, add_two)
node1.work_generate_blocking (*send2);
ASSERT_EQ (nano::process_result::progress, node1.process (*send2).code);
node1.active.start (send2);
ASSERT_EQ (2, node1.active.roots.size ());
ASSERT_EQ (2, node1.active.size ());
}

TEST (vote_uniquer, null)
Expand Down Expand Up @@ -159,17 +165,24 @@ TEST (conflicts, reprioritize)
node1.work_generate_blocking (*send1);
uint64_t difficulty1;
nano::work_validate (*send1, &difficulty1);
nano::send_block send1_copy (*send1);
node1.process_active (send1);
node1.block_processor.flush ();
auto existing1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ())));
ASSERT_NE (node1.active.roots.end (), existing1);
ASSERT_EQ (difficulty1, existing1->difficulty);
node1.work_generate_blocking (*send1, difficulty1);
{
std::lock_guard<std::mutex> guard (node1.active.mutex);
auto existing1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ())));
ASSERT_NE (node1.active.roots.end (), existing1);
ASSERT_EQ (difficulty1, existing1->difficulty);
}
node1.work_generate_blocking (send1_copy, difficulty1);
uint64_t difficulty2;
nano::work_validate (*send1, &difficulty2);
node1.process_active (send1);
nano::work_validate (send1_copy, &difficulty2);
node1.process_active (std::make_shared<nano::send_block> (send1_copy));
node1.block_processor.flush ();
auto existing2 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ())));
ASSERT_NE (node1.active.roots.end (), existing2);
ASSERT_EQ (difficulty2, existing2->difficulty);
{
std::lock_guard<std::mutex> guard (node1.active.mutex);
auto existing2 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ())));
ASSERT_NE (node1.active.roots.end (), existing2);
ASSERT_EQ (difficulty2, existing2->difficulty);
}
}
28 changes: 20 additions & 8 deletions nano/core_test/gap_cache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,19 @@ TEST (gap_cache, add_existing)
auto block1 (std::make_shared<nano::send_block> (0, 1, 2, nano::keypair ().prv, 4, 5));
auto transaction (system.nodes[0]->store.tx_begin (true));
cache.add (transaction, block1->hash ());
std::unique_lock<std::mutex> lock (cache.mutex);
auto existing1 (cache.blocks.get<1> ().find (block1->hash ()));
ASSERT_NE (cache.blocks.get<1> ().end (), existing1);
auto arrival (existing1->arrival);
lock.unlock ();
system.deadline_set (20s);
while (arrival == std::chrono::steady_clock::now ())
;
{
ASSERT_NO_ERROR (system.poll ());
}
cache.add (transaction, block1->hash ());
ASSERT_EQ (1, cache.blocks.size ());
ASSERT_EQ (1, cache.size ());
lock.lock ();
auto existing2 (cache.blocks.get<1> ().find (block1->hash ()));
ASSERT_NE (cache.blocks.get<1> ().end (), existing2);
ASSERT_GT (existing2->arrival, arrival);
Expand All @@ -39,14 +45,20 @@ TEST (gap_cache, comparison)
auto block1 (std::make_shared<nano::send_block> (1, 0, 2, nano::keypair ().prv, 4, 5));
auto transaction (system.nodes[0]->store.tx_begin (true));
cache.add (transaction, block1->hash ());
std::unique_lock<std::mutex> lock (cache.mutex);
auto existing1 (cache.blocks.get<1> ().find (block1->hash ()));
ASSERT_NE (cache.blocks.get<1> ().end (), existing1);
auto arrival (existing1->arrival);
lock.unlock ();
system.deadline_set (20s);
while (std::chrono::steady_clock::now () == arrival)
;
{
ASSERT_NO_ERROR (system.poll ());
}
auto block3 (std::make_shared<nano::send_block> (0, 42, 1, nano::keypair ().prv, 3, 4));
cache.add (transaction, block3->hash ());
ASSERT_EQ (2, cache.blocks.size ());
ASSERT_EQ (2, cache.size ());
lock.lock ();
auto existing2 (cache.blocks.get<1> ().find (block3->hash ()));
ASSERT_NE (cache.blocks.get<1> ().end (), existing2);
ASSERT_GT (existing2->arrival, arrival);
Expand Down Expand Up @@ -92,16 +104,16 @@ TEST (gap_cache, two_dependencies)
auto send1 (std::make_shared<nano::send_block> (genesis.hash (), key.pub, 1, nano::test_genesis_key.prv, nano::test_genesis_key.pub, system.work.generate (genesis.hash ())));
auto send2 (std::make_shared<nano::send_block> (send1->hash (), key.pub, 0, nano::test_genesis_key.prv, nano::test_genesis_key.pub, system.work.generate (send1->hash ())));
auto open (std::make_shared<nano::open_block> (send1->hash (), key.pub, key.pub, key.prv, key.pub, system.work.generate (key.pub)));
ASSERT_EQ (0, system.nodes[0]->gap_cache.blocks.size ());
ASSERT_EQ (0, system.nodes[0]->gap_cache.size ());
system.nodes[0]->block_processor.add (send2, nano::seconds_since_epoch ());
system.nodes[0]->block_processor.flush ();
ASSERT_EQ (1, system.nodes[0]->gap_cache.blocks.size ());
ASSERT_EQ (1, system.nodes[0]->gap_cache.size ());
system.nodes[0]->block_processor.add (open, nano::seconds_since_epoch ());
system.nodes[0]->block_processor.flush ();
ASSERT_EQ (2, system.nodes[0]->gap_cache.blocks.size ());
ASSERT_EQ (2, system.nodes[0]->gap_cache.size ());
system.nodes[0]->block_processor.add (send1, nano::seconds_since_epoch ());
system.nodes[0]->block_processor.flush ();
ASSERT_EQ (0, system.nodes[0]->gap_cache.blocks.size ());
ASSERT_EQ (0, system.nodes[0]->gap_cache.size ());
auto transaction (system.nodes[0]->store.tx_begin ());
ASSERT_TRUE (system.nodes[0]->store.block_exists (transaction, send1->hash ()));
ASSERT_TRUE (system.nodes[0]->store.block_exists (transaction, send2->hash ()));
Expand Down
50 changes: 26 additions & 24 deletions nano/core_test/ledger.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -717,9 +717,9 @@ TEST (votes, check_signature)
ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
auto node_l (system.nodes[0]);
node1.active.start (send1);
std::lock_guard<std::mutex> lock (node1.active.mutex);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
ASSERT_EQ (1, votes1->last_votes.size ());
std::unique_lock<std::mutex> lock (node1.active.mutex);
auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send1));
vote1->signature.bytes[0] ^= 1;
ASSERT_EQ (nano::vote_code::invalid, node1.vote_processor.vote_blocking (transaction, vote1, nano::endpoint (boost::asio::ip::address_v6 (), 0)));
Expand All @@ -739,12 +739,15 @@ TEST (votes, add_one)
auto transaction (node1.store.tx_begin (true));
ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
node1.active.start (send1);
std::unique_lock<std::mutex> lock (node1.active.mutex);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
ASSERT_EQ (1, votes1->last_votes.size ());
lock.unlock ();
auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send1));
ASSERT_FALSE (node1.active.vote (vote1));
auto vote2 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 2, send1));
ASSERT_FALSE (node1.active.vote (vote2));
lock.lock ();
ASSERT_EQ (2, votes1->last_votes.size ());
auto existing1 (votes1->last_votes.find (nano::test_genesis_key.pub));
ASSERT_NE (votes1->last_votes.end (), existing1);
Expand All @@ -765,20 +768,23 @@ TEST (votes, add_two)
auto transaction (node1.store.tx_begin (true));
ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
node1.active.start (send1);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send1));
ASSERT_FALSE (node1.active.vote (vote1));
nano::keypair key2;
auto send2 (std::make_shared<nano::send_block> (genesis.hash (), key2.pub, 0, nano::test_genesis_key.prv, nano::test_genesis_key.pub, 0));
auto vote2 (std::make_shared<nano::vote> (key2.pub, key2.prv, 1, send2));
ASSERT_FALSE (node1.active.vote (vote2));
ASSERT_EQ (3, 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);
ASSERT_NE (votes1->last_votes.end (), votes1->last_votes.find (key2.pub));
ASSERT_EQ (send2->hash (), votes1->last_votes[key2.pub].hash);
auto winner (*votes1->tally (transaction).begin ());
ASSERT_EQ (*send1, *winner.second);
{
std::lock_guard<std::mutex> lock (node1.active.mutex);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
ASSERT_EQ (3, 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);
ASSERT_NE (votes1->last_votes.end (), votes1->last_votes.find (key2.pub));
ASSERT_EQ (send2->hash (), votes1->last_votes[key2.pub].hash);
auto winner (*votes1->tally (transaction).begin ());
ASSERT_EQ (*send1, *winner.second);
}
}

// Higher sequence numbers change the vote
Expand All @@ -793,22 +799,27 @@ TEST (votes, add_existing)
auto transaction (node1.store.tx_begin (true));
ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
node1.active.start (send1);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send1));
ASSERT_FALSE (node1.active.vote (vote1));
ASSERT_FALSE (node1.active.publish (send1));
std::unique_lock<std::mutex> lock (node1.active.mutex);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
ASSERT_EQ (1, votes1->last_votes[nano::test_genesis_key.pub].sequence);
nano::keypair key2;
auto send2 (std::make_shared<nano::send_block> (genesis.hash (), key2.pub, nano::genesis_amount - nano::Gxrb_ratio, nano::test_genesis_key.prv, nano::test_genesis_key.pub, 0));
auto vote2 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 2, send2));
// Pretend we've waited the timeout
votes1->last_votes[nano::test_genesis_key.pub].time = std::chrono::steady_clock::now () - std::chrono::seconds (20);
lock.unlock ();
ASSERT_FALSE (node1.active.vote (vote2));
ASSERT_FALSE (node1.active.publish (send2));
lock.lock ();
ASSERT_EQ (2, votes1->last_votes[nano::test_genesis_key.pub].sequence);
// Also resend the old vote, and see if we respect the sequence number
votes1->last_votes[nano::test_genesis_key.pub].time = std::chrono::steady_clock::now () - std::chrono::seconds (20);
lock.unlock ();
ASSERT_TRUE (node1.active.vote (vote1));
lock.lock ();
ASSERT_EQ (2, votes1->last_votes[nano::test_genesis_key.pub].sequence);
ASSERT_EQ (2, votes1->last_votes.size ());
ASSERT_NE (votes1->last_votes.end (), votes1->last_votes.find (nano::test_genesis_key.pub));
Expand All @@ -829,18 +840,15 @@ TEST (votes, add_old)
auto transaction (node1.store.tx_begin (true));
ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
node1.active.start (send1);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 2, send1));
std::unique_lock<std::mutex> lock (node1.active.mutex);
std::lock_guard<std::mutex> lock (node1.active.mutex);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
node1.vote_processor.vote_blocking (transaction, vote1, node1.network.endpoint ());
lock.unlock ();
nano::keypair key2;
auto send2 (std::make_shared<nano::send_block> (genesis.hash (), key2.pub, 0, nano::test_genesis_key.prv, nano::test_genesis_key.pub, 0));
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);
lock.lock ();
node1.vote_processor.vote_blocking (transaction, vote2, node1.network.endpoint ());
lock.unlock ();
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);
Expand All @@ -864,21 +872,18 @@ 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);
std::unique_lock<std::mutex> lock (node1.active.mutex);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
auto votes2 (node1.active.roots.find (nano::uint512_union (send2->previous (), send2->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));
std::unique_lock<std::mutex> lock (node1.active.mutex);
auto vote_result1 (node1.vote_processor.vote_blocking (transaction, vote1, node1.network.endpoint ()));
lock.unlock ();
ASSERT_EQ (nano::vote_code::vote, vote_result1);
ASSERT_EQ (2, votes1->last_votes.size ());
ASSERT_EQ (1, votes2->last_votes.size ());
lock.lock ();
auto vote2 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send2));
auto vote_result2 (node1.vote_processor.vote_blocking (transaction, vote2, node1.network.endpoint ()));
lock.unlock ();
ASSERT_EQ (nano::vote_code::vote, vote_result2);
ASSERT_EQ (2, votes1->last_votes.size ());
ASSERT_EQ (2, votes2->last_votes.size ());
Expand All @@ -904,18 +909,15 @@ TEST (votes, add_cooldown)
auto transaction (node1.store.tx_begin (true));
ASSERT_EQ (nano::process_result::progress, node1.ledger.process (transaction, *send1).code);
node1.active.start (send1);
std::unique_lock<std::mutex> lock (node1.active.mutex);
auto votes1 (node1.active.roots.find (nano::uint512_union (send1->previous (), send1->root ()))->election);
auto vote1 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 1, send1));
std::unique_lock<std::mutex> lock (node1.active.mutex);
node1.vote_processor.vote_blocking (transaction, vote1, node1.network.endpoint ());
lock.unlock ();
nano::keypair key2;
auto send2 (std::make_shared<nano::send_block> (genesis.hash (), key2.pub, 0, nano::test_genesis_key.prv, nano::test_genesis_key.pub, 0));
node1.work_generate_blocking (*send2);
auto vote2 (std::make_shared<nano::vote> (nano::test_genesis_key.pub, nano::test_genesis_key.prv, 2, send2));
lock.lock ();
node1.vote_processor.vote_blocking (transaction, vote2, node1.network.endpoint ());
lock.unlock ();
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);
Expand Down Expand Up @@ -2495,7 +2497,7 @@ TEST (ledger, unchecked_epoch_invalid)
auto transaction (node1.store.tx_begin ());
ASSERT_FALSE (node1.store.block_exists (transaction, epoch1->hash ()));
ASSERT_TRUE (node1.store.block_exists (transaction, epoch2->hash ()));
ASSERT_TRUE (node1.active.roots.empty ());
ASSERT_TRUE (node1.active.empty ());
auto unchecked_count (node1.store.unchecked_count (transaction));
ASSERT_EQ (unchecked_count, 0);
nano::account_info info;
Expand Down
8 changes: 4 additions & 4 deletions nano/core_test/network.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ TEST (network, self_discard)
nano::udp_data data;
data.endpoint = system.nodes[0]->network.endpoint ();
ASSERT_EQ (0, system.nodes[0]->stats.count (nano::stat::type::error, nano::stat::detail::bad_sender));
system.nodes[0]->network.receive_action (&data);
system.nodes[0]->network.receive_action (&data, system.nodes[0]->network.endpoint ());
ASSERT_EQ (1, system.nodes[0]->stats.count (nano::stat::type::error, nano::stat::detail::bad_sender));
}

Expand Down Expand Up @@ -612,7 +612,7 @@ TEST (bootstrap_processor, process_one)
{
ASSERT_NO_ERROR (system.poll ());
}
ASSERT_EQ (0, node1->active.roots.size ());
ASSERT_EQ (0, node1->active.size ());
node1->stop ();
}

Expand Down Expand Up @@ -665,7 +665,7 @@ TEST (bootstrap_processor, process_state)
{
ASSERT_NO_ERROR (system.poll ());
}
ASSERT_EQ (0, node1->active.roots.size ());
ASSERT_EQ (0, node1->active.size ());
node1->stop ();
}

Expand Down Expand Up @@ -1062,7 +1062,7 @@ TEST (bulk, offline_send)
ASSERT_NE (std::numeric_limits<nano::uint256_t>::max (), system.nodes[0]->balance (nano::test_genesis_key.pub));
// Wait to finish election background tasks
system.deadline_set (10s);
while (!system.nodes[0]->active.roots.empty ())
while (!system.nodes[0]->active.empty ())
{
ASSERT_NO_ERROR (system.poll ());
}
Expand Down
Loading

0 comments on commit ba597ff

Please sign in to comment.