From 6b122a01dd2f05a69a8b2828f1739a5cfe2e8268 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Sun, 22 Mar 2020 11:30:42 +0000 Subject: [PATCH 1/5] Prevent getting stuck in block processor flush I noticed `node.block_processor_reject_state` was often freezing on windows, this is due to a the verification callback being called and notifying the block processor before transitioning into an inactive state, so the `block_processor::flush` ends up waiting for the condition forever. I've added a second callback to solve this. --- nano/node/blockprocessor.cpp | 3 +++ nano/node/state_block_signature_verification.cpp | 1 + nano/node/state_block_signature_verification.hpp | 1 + 3 files changed, 5 insertions(+) diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index a68b83ec15..48a95e9aa1 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -21,6 +21,9 @@ state_block_signature_verification (node.checker, node.ledger.network_params.led state_block_signature_verification.blocks_verified_callback = [this](std::deque & items, std::vector const & verifications, std::vector const & hashes, std::vector const & blocks_signatures) { this->process_verified_state_blocks (items, verifications, hashes, blocks_signatures); }; + state_block_signature_verification.transition_inactive_callback = [this]() { + this->condition.notify_all (); + }; } nano::block_processor::~block_processor () diff --git a/nano/node/state_block_signature_verification.cpp b/nano/node/state_block_signature_verification.cpp index 5683a0b5ed..1e1ed5b547 100644 --- a/nano/node/state_block_signature_verification.cpp +++ b/nano/node/state_block_signature_verification.cpp @@ -56,6 +56,7 @@ void nano::state_block_signature_verification::run (uint64_t state_block_signatu lk.lock (); } active = false; + transition_inactive_callback (); } else { diff --git a/nano/node/state_block_signature_verification.hpp b/nano/node/state_block_signature_verification.hpp index 7af89bfd8f..993026dade 100644 --- a/nano/node/state_block_signature_verification.hpp +++ b/nano/node/state_block_signature_verification.hpp @@ -25,6 +25,7 @@ class state_block_signature_verification bool is_active (); std::function &, std::vector const &, std::vector const &, std::vector const &)> blocks_verified_callback; + std::function transition_inactive_callback; private: nano::signature_checker & signature_checker; From cd7815172ff989e1ecc2ce0fc44430a4f068d36f Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Mon, 23 Mar 2020 10:48:32 +0000 Subject: [PATCH 2/5] Lock before notifying to prevent a race with condition.wait; check if flushing first --- nano/node/blockprocessor.cpp | 8 +++++++- nano/node/state_block_signature_verification.cpp | 2 ++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index 48a95e9aa1..e710cdc9c4 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -22,7 +22,13 @@ state_block_signature_verification (node.checker, node.ledger.network_params.led this->process_verified_state_blocks (items, verifications, hashes, blocks_signatures); }; state_block_signature_verification.transition_inactive_callback = [this]() { - this->condition.notify_all (); + if (this->flushing) + { + { + nano::lock_guard guard (this->mutex); + } + this->condition.notify_all (); + } }; } diff --git a/nano/node/state_block_signature_verification.cpp b/nano/node/state_block_signature_verification.cpp index 1e1ed5b547..9dc6700221 100644 --- a/nano/node/state_block_signature_verification.cpp +++ b/nano/node/state_block_signature_verification.cpp @@ -56,7 +56,9 @@ void nano::state_block_signature_verification::run (uint64_t state_block_signatu lk.lock (); } active = false; + lk.unlock (); transition_inactive_callback (); + lk.lock (); } else { From f4f71d3eef1962d78d021f493e338b6e1655e738 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Mon, 23 Mar 2020 10:49:05 +0000 Subject: [PATCH 3/5] Change the test to launch async and wait for future, to prevent freezing in the future but still fail --- nano/core_test/node.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index a4e34de2ce..79a8ef9bce 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -3193,6 +3193,7 @@ TEST (node, block_processor_signatures) /* * State blocks go through a different signature path, ensure invalidly signed state blocks are rejected + * This test can freeze if the wake conditions in block_processor::flush are off, for that reason this is done async here */ TEST (node, block_processor_reject_state) { @@ -3204,12 +3205,14 @@ TEST (node, block_processor_reject_state) send1->signature.bytes[0] ^= 1; ASSERT_FALSE (node.ledger.block_exists (send1->hash ())); node.process_active (send1); - node.block_processor.flush (); + auto flushed = std::async (std::launch::async, [&node] { node.block_processor.flush (); }); + ASSERT_NE (std::future_status::timeout, flushed.wait_for (5s)); ASSERT_FALSE (node.ledger.block_exists (send1->hash ())); auto send2 (std::make_shared (nano::test_genesis_key.pub, genesis.hash (), nano::test_genesis_key.pub, nano::genesis_amount - 2 * nano::Gxrb_ratio, nano::test_genesis_key.pub, nano::test_genesis_key.prv, nano::test_genesis_key.pub, 0)); node.work_generate_blocking (*send2); node.process_active (send2); - node.block_processor.flush (); + auto flushed2 = std::async (std::launch::async, [&node] { node.block_processor.flush (); }); + ASSERT_NE (std::future_status::timeout, flushed2.wait_for (5s)); ASSERT_TRUE (node.ledger.block_exists (send2->hash ())); } From b772bb753430c9c0dbf1175b1e64aee386c75f8b Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Mon, 23 Mar 2020 10:53:01 +0000 Subject: [PATCH 4/5] Comment formatting --- nano/core_test/node.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 79a8ef9bce..e7cec82992 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -3193,7 +3193,7 @@ TEST (node, block_processor_signatures) /* * State blocks go through a different signature path, ensure invalidly signed state blocks are rejected - * This test can freeze if the wake conditions in block_processor::flush are off, for that reason this is done async here + * This test can freeze if the wake conditions in block_processor::flush are off, for that reason this is done async here */ TEST (node, block_processor_reject_state) { From 51973ef5c7c0ad612ad271bc92ee7109f346b845 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Mon, 23 Mar 2020 11:03:27 +0000 Subject: [PATCH 5/5] Add comment on why lock before notifying (all Wesley review) --- nano/node/blockprocessor.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/nano/node/blockprocessor.cpp b/nano/node/blockprocessor.cpp index e710cdc9c4..c79e14efe0 100644 --- a/nano/node/blockprocessor.cpp +++ b/nano/node/blockprocessor.cpp @@ -25,6 +25,7 @@ state_block_signature_verification (node.checker, node.ledger.network_params.led if (this->flushing) { { + // Prevent a race with condition.wait in block_processor::flush nano::lock_guard guard (this->mutex); } this->condition.notify_all ();