From fac04cb6ba1d032587bd02eab2247fd655a548cd Mon Sep 17 00:00:00 2001 From: MacroFake Date: Fri, 6 May 2022 16:58:53 +0200 Subject: refactor: Add lock annotations to Active* methods This is a refactor, putting the burden to think about thread safety to the caller. Otherwise, there is a risk that the caller will assume thread safety where none exists, as is evident in the previous two commits. --- src/test/interfaces_tests.cpp | 6 +++++- src/test/validation_block_tests.cpp | 5 +++-- src/test/validation_chainstatemanager_tests.cpp | 16 ++++++++-------- 3 files changed, 16 insertions(+), 11 deletions(-) (limited to 'src/test') diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp index 49b7d2003b..cb901b2259 100644 --- a/src/test/interfaces_tests.cpp +++ b/src/test/interfaces_tests.cpp @@ -17,6 +17,7 @@ BOOST_FIXTURE_TEST_SUITE(interfaces_tests, TestChain100Setup) BOOST_AUTO_TEST_CASE(findBlock) { + LOCK(Assert(m_node.chainman)->GetMutex()); auto& chain = m_node.chain; const CChain& active = Assert(m_node.chainman)->ActiveChain(); @@ -61,6 +62,7 @@ BOOST_AUTO_TEST_CASE(findBlock) BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight) { + LOCK(Assert(m_node.chainman)->GetMutex()); auto& chain = m_node.chain; const CChain& active = Assert(m_node.chainman)->ActiveChain(); uint256 hash; @@ -73,6 +75,7 @@ BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight) BOOST_AUTO_TEST_CASE(findAncestorByHeight) { + LOCK(Assert(m_node.chainman)->GetMutex()); auto& chain = m_node.chain; const CChain& active = Assert(m_node.chainman)->ActiveChain(); uint256 hash; @@ -83,6 +86,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHeight) BOOST_AUTO_TEST_CASE(findAncestorByHash) { + LOCK(Assert(m_node.chainman)->GetMutex()); auto& chain = m_node.chain; const CChain& active = Assert(m_node.chainman)->ActiveChain(); int height = -1; @@ -94,7 +98,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHash) BOOST_AUTO_TEST_CASE(findCommonAncestor) { auto& chain = m_node.chain; - const CChain& active = Assert(m_node.chainman)->ActiveChain(); + const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain()); auto* orig_tip = active.Tip(); for (int i = 0; i < 10; ++i) { BlockValidationState state; diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 7ade4d8195..7a82e1a736 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -234,7 +234,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg) // Run the test multiple times for (int test_runs = 3; test_runs > 0; --test_runs) { - BOOST_CHECK_EQUAL(last_mined->GetHash(), m_node.chainman->ActiveChain().Tip()->GetBlockHash()); + BOOST_CHECK_EQUAL(last_mined->GetHash(), WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip()->GetBlockHash())); // Later on split from here const uint256 split_hash{last_mined->hashPrevBlock}; @@ -316,7 +316,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg) ProcessBlock(b); } // Check that the reorg was eventually successful - BOOST_CHECK_EQUAL(last_mined->GetHash(), m_node.chainman->ActiveChain().Tip()->GetBlockHash()); + BOOST_CHECK_EQUAL(last_mined->GetHash(), WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip()->GetBlockHash())); // We can join the other thread, which returns when the reorg was successful rpc_thread.join(); @@ -325,6 +325,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg) BOOST_AUTO_TEST_CASE(witness_commitment_index) { + LOCK(Assert(m_node.chainman)->GetMutex()); CScript pubKey; pubKey << 1 << OP_TRUE; auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get()}.CreateNewBlock(pubKey); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 14de96ff41..7b7bd05b5c 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -49,12 +49,12 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) auto all = manager.GetAll(); BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end()); - auto& active_chain = manager.ActiveChain(); + auto& active_chain = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain()); BOOST_CHECK_EQUAL(&active_chain, &c1.m_chain); - BOOST_CHECK_EQUAL(manager.ActiveHeight(), -1); + BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), -1); - auto active_tip = manager.ActiveTip(); + auto active_tip = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip()); auto exp_tip = c1.m_chain.Tip(); BOOST_CHECK_EQUAL(active_tip, exp_tip); @@ -84,12 +84,12 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) auto all2 = manager.GetAll(); BOOST_CHECK_EQUAL_COLLECTIONS(all2.begin(), all2.end(), chainstates.begin(), chainstates.end()); - auto& active_chain2 = manager.ActiveChain(); + auto& active_chain2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain()); BOOST_CHECK_EQUAL(&active_chain2, &c2.m_chain); - BOOST_CHECK_EQUAL(manager.ActiveHeight(), 0); + BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), 0); - auto active_tip2 = manager.ActiveTip(); + auto active_tip2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip()); auto exp_tip2 = c2.m_chain.Tip(); BOOST_CHECK_EQUAL(active_tip2, exp_tip2); @@ -236,7 +236,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup) BOOST_CHECK(WITH_LOCK(::cs_main, return !chainman.ActiveChain().Genesis()->IsAssumedValid())); const AssumeutxoData& au_data = *ExpectedAssumeutxo(snapshot_height, ::Params()); - const CBlockIndex* tip = chainman.ActiveTip(); + const CBlockIndex* tip = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()); BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx); @@ -335,7 +335,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) const int assumed_valid_start_idx = last_assumed_valid_idx - expected_assumed_valid; CBlockIndex* validated_tip{nullptr}; - CBlockIndex* assumed_tip{chainman.ActiveChain().Tip()}; + CBlockIndex* assumed_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip())}; auto reload_all_block_indexes = [&]() { for (CChainState* cs : chainman.GetAll()) { -- cgit v1.2.3