From 6e10c2b8832d24cc010fd39b23e23543e00012e5 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 20 Jul 2020 09:49:10 -0400 Subject: [PATCH] refactor: Replace uses ChainActive() in interfaces/chain.cpp Suggested https://github.com/bitcoin/bitcoin/pull/19425#discussion_r456236407 --- src/bench/wallet_balance.cpp | 7 ++--- src/interfaces/chain.cpp | 41 +++++++++++++++++---------- src/test/interfaces_tests.cpp | 12 ++++---- src/wallet/test/wallet_tests.cpp | 48 +++++++++++++------------------- 4 files changed, 55 insertions(+), 53 deletions(-) diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index e16182b48ec9f..b251060071d52 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -24,15 +24,14 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b const auto& ADDRESS_WATCHONLY = ADDRESS_BCRT1_UNSPENDABLE; - NodeContext node; - std::unique_ptr chain = interfaces::MakeChain(node); - CWallet wallet{chain.get(), WalletLocation(), CreateMockWalletDatabase()}; + test_setup.m_node.chain = interfaces::MakeChain(test_setup.m_node); + CWallet wallet{test_setup.m_node.chain.get(), WalletLocation(), CreateMockWalletDatabase()}; { wallet.SetupLegacyScriptPubKeyMan(); bool first_run; if (wallet.LoadWallet(first_run) != DBErrors::LOAD_OK) assert(false); } - auto handler = chain->handleNotifications({&wallet, [](CWallet*) {}}); + auto handler = test_setup.m_node.chain->handleNotifications({&wallet, [](CWallet*) {}}); const Optional address_mine{add_mine ? Optional{getnewaddress(wallet)} : nullopt}; if (add_watchonly) importaddress(wallet, ADDRESS_WATCHONLY); diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 38b8db3ab5b58..d1790fdfa5043 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -38,7 +38,7 @@ namespace interfaces { namespace { -bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock& lock) +bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock& lock, const CChain& active) { if (!index) return false; if (block.m_hash) *block.m_hash = index->GetBlockHash(); @@ -46,8 +46,8 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLockGetBlockTime(); if (block.m_max_time) *block.m_max_time = index->GetBlockTimeMax(); if (block.m_mtp_time) *block.m_mtp_time = index->GetMedianTimePast(); - if (block.m_in_active_chain) *block.m_in_active_chain = ChainActive()[index->nHeight] == index; - if (block.m_next_block) FillBlock(ChainActive()[index->nHeight] == index ? ChainActive()[index->nHeight + 1] : nullptr, *block.m_next_block, lock); + if (block.m_in_active_chain) *block.m_in_active_chain = active[index->nHeight] == index; + if (block.m_next_block) FillBlock(active[index->nHeight] == index ? active[index->nHeight + 1] : nullptr, *block.m_next_block, lock, active); if (block.m_data) { REVERSE_LOCK(lock); if (!ReadBlockFromDisk(*block.m_data, index, Params().GetConsensus())) block.m_data->SetNull(); @@ -150,7 +150,8 @@ class ChainImpl : public Chain Optional getHeight() override { LOCK(::cs_main); - int height = ::ChainActive().Height(); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); + int height = active.Height(); if (height >= 0) { return height; } @@ -159,20 +160,23 @@ class ChainImpl : public Chain uint256 getBlockHash(int height) override { LOCK(::cs_main); - CBlockIndex* block = ::ChainActive()[height]; + const CChain& active = Assert(m_node.chainman)->ActiveChain(); + CBlockIndex* block = active[height]; assert(block); return block->GetBlockHash(); } bool haveBlockOnDisk(int height) override { LOCK(cs_main); - CBlockIndex* block = ::ChainActive()[height]; + const CChain& active = Assert(m_node.chainman)->ActiveChain(); + CBlockIndex* block = active[height]; return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0; } CBlockLocator getTipLocator() override { LOCK(cs_main); - return ::ChainActive().GetLocator(); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); + return active.GetLocator(); } bool checkFinalTx(const CTransaction& tx) override { @@ -182,7 +186,8 @@ class ChainImpl : public Chain Optional findLocatorFork(const CBlockLocator& locator) override { LOCK(cs_main); - if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) { + const CChain& active = Assert(m_node.chainman)->ActiveChain(); + if (CBlockIndex* fork = FindForkInGlobalIndex(active, locator)) { return fork->nHeight; } return nullopt; @@ -190,40 +195,45 @@ class ChainImpl : public Chain bool findBlock(const uint256& hash, const FoundBlock& block) override { WAIT_LOCK(cs_main, lock); - return FillBlock(LookupBlockIndex(hash), block, lock); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); + return FillBlock(LookupBlockIndex(hash), block, lock, active); } bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block) override { WAIT_LOCK(cs_main, lock); - return FillBlock(ChainActive().FindEarliestAtLeast(min_time, min_height), block, lock); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); + return FillBlock(active.FindEarliestAtLeast(min_time, min_height), block, lock, active); } bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out) override { WAIT_LOCK(cs_main, lock); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); if (const CBlockIndex* block = LookupBlockIndex(block_hash)) { if (const CBlockIndex* ancestor = block->GetAncestor(ancestor_height)) { - return FillBlock(ancestor, ancestor_out, lock); + return FillBlock(ancestor, ancestor_out, lock, active); } } - return FillBlock(nullptr, ancestor_out, lock); + return FillBlock(nullptr, ancestor_out, lock, active); } bool findAncestorByHash(const uint256& block_hash, const uint256& ancestor_hash, const FoundBlock& ancestor_out) override { WAIT_LOCK(cs_main, lock); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); const CBlockIndex* block = LookupBlockIndex(block_hash); const CBlockIndex* ancestor = LookupBlockIndex(ancestor_hash); if (block && ancestor && block->GetAncestor(ancestor->nHeight) != ancestor) ancestor = nullptr; - return FillBlock(ancestor, ancestor_out, lock); + return FillBlock(ancestor, ancestor_out, lock, active); } bool findCommonAncestor(const uint256& block_hash1, const uint256& block_hash2, const FoundBlock& ancestor_out, const FoundBlock& block1_out, const FoundBlock& block2_out) override { WAIT_LOCK(cs_main, lock); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); const CBlockIndex* block1 = LookupBlockIndex(block_hash1); const CBlockIndex* block2 = LookupBlockIndex(block_hash2); const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr; // Using & instead of && below to avoid short circuiting and leaving // output uninitialized. - return FillBlock(ancestor, ancestor_out, lock) & FillBlock(block1, block1_out, lock) & FillBlock(block2, block2_out, lock); + return FillBlock(ancestor, ancestor_out, lock, active) & FillBlock(block1, block1_out, lock, active) & FillBlock(block2, block2_out, lock, active); } void findCoins(std::map& coins) override { return FindCoins(m_node, coins); } double guessVerificationProgress(const uint256& block_hash) override @@ -334,7 +344,8 @@ class ChainImpl : public Chain { if (!old_tip.IsNull()) { LOCK(::cs_main); - if (old_tip == ::ChainActive().Tip()->GetBlockHash()) return; + const CChain& active = Assert(m_node.chainman)->ActiveChain(); + if (old_tip == active.Tip()->GetBlockHash()) return; } SyncWithValidationInterfaceQueue(); } diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp index 2e8729736cb7b..b8c4bc8605502 100644 --- a/src/test/interfaces_tests.cpp +++ b/src/test/interfaces_tests.cpp @@ -18,7 +18,7 @@ BOOST_FIXTURE_TEST_SUITE(interfaces_tests, TestChain100Setup) BOOST_AUTO_TEST_CASE(findBlock) { auto chain = interfaces::MakeChain(m_node); - auto& active = ChainActive(); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); uint256 hash; BOOST_CHECK(chain->findBlock(active[10]->GetBlockHash(), FoundBlock().hash(hash))); @@ -62,7 +62,7 @@ BOOST_AUTO_TEST_CASE(findBlock) BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight) { auto chain = interfaces::MakeChain(m_node); - auto& active = ChainActive(); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); uint256 hash; int height; BOOST_CHECK(chain->findFirstBlockWithTimeAndHeight(/* min_time= */ 0, /* min_height= */ 5, FoundBlock().hash(hash).height(height))); @@ -74,7 +74,7 @@ BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight) BOOST_AUTO_TEST_CASE(findAncestorByHeight) { auto chain = interfaces::MakeChain(m_node); - auto& active = ChainActive(); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); uint256 hash; BOOST_CHECK(chain->findAncestorByHeight(active[20]->GetBlockHash(), 10, FoundBlock().hash(hash))); BOOST_CHECK_EQUAL(hash, active[10]->GetBlockHash()); @@ -84,7 +84,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHeight) BOOST_AUTO_TEST_CASE(findAncestorByHash) { auto chain = interfaces::MakeChain(m_node); - auto& active = ChainActive(); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); int height = -1; BOOST_CHECK(chain->findAncestorByHash(active[20]->GetBlockHash(), active[10]->GetBlockHash(), FoundBlock().height(height))); BOOST_CHECK_EQUAL(height, 10); @@ -94,7 +94,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHash) BOOST_AUTO_TEST_CASE(findCommonAncestor) { auto chain = interfaces::MakeChain(m_node); - auto& active = ChainActive(); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); auto* orig_tip = active.Tip(); for (int i = 0; i < 10; ++i) { BlockValidationState state; @@ -124,7 +124,7 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor) BOOST_AUTO_TEST_CASE(hasBlocks) { auto chain = interfaces::MakeChain(m_node); - auto& active = ChainActive(); + const CChain& active = Assert(m_node.chainman)->ActiveChain(); // Test ranges BOOST_CHECK(chain->hasBlocks(active.Tip()->GetBlockHash(), 10, 90)); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 7ef06663b55bb..6b564fab77da2 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -80,12 +80,11 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); CBlockIndex* newTip = ::ChainActive().Tip(); - NodeContext node; - auto chain = interfaces::MakeChain(node); + m_node.chain = interfaces::MakeChain(m_node); // Verify ScanForWalletTransactions fails to read an unknown start block. { - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -104,7 +103,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions picks up transactions in both the old // and new block files. { - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -130,7 +129,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions only picks transactions in the new block // file. { - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -155,7 +154,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions scans no blocks. { - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -180,8 +179,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); CBlockIndex* newTip = ::ChainActive().Tip(); - NodeContext node; - auto chain = interfaces::MakeChain(node); + m_node.chain = interfaces::MakeChain(m_node); // Prune the older block file. { @@ -194,7 +192,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) // before the missing block, and success for a key whose creation time is // after. { - std::shared_ptr wallet = std::make_shared(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + std::shared_ptr wallet = std::make_shared(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); WITH_LOCK(wallet->cs_wallet, wallet->SetLastBlockProcessed(newTip->nHeight, newTip->GetBlockHash())); AddWallet(wallet); @@ -252,14 +250,13 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) SetMockTime(KEY_TIME); m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); - NodeContext node; - auto chain = interfaces::MakeChain(node); + m_node.chain = interfaces::MakeChain(m_node); std::string backup_file = (GetDataDir() / "wallet.backup").string(); // Import key into wallet and call dumpwallet to create backup file. { - std::shared_ptr wallet = std::make_shared(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + std::shared_ptr wallet = std::make_shared(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); { auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan(); LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore); @@ -281,7 +278,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // Call importwallet RPC and verify all blocks with timestamps >= BLOCK_TIME // were scanned, and no prior blocks were scanned. { - std::shared_ptr wallet = std::make_shared(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + std::shared_ptr wallet = std::make_shared(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); LOCK(wallet->cs_wallet); wallet->SetupLegacyScriptPubKeyMan(); @@ -314,10 +311,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // debit functions. BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) { - NodeContext node; - auto chain = interfaces::MakeChain(node); - - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); auto spk_man = wallet.GetOrCreateLegacyScriptPubKeyMan(); CWalletTx wtx(&wallet, m_coinbase_txns.back()); @@ -492,7 +486,8 @@ class ListCoinsTestingSetup : public TestChain100Setup ListCoinsTestingSetup() { CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - wallet = MakeUnique(m_chain.get(), WalletLocation(), CreateMockWalletDatabase()); + m_node.chain = interfaces::MakeChain(m_node); + wallet = MakeUnique(m_node.chain.get(), WalletLocation(), CreateMockWalletDatabase()); { LOCK2(wallet->cs_wallet, ::cs_main); wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -541,7 +536,6 @@ class ListCoinsTestingSetup : public TestChain100Setup return it->second; } - std::unique_ptr m_chain = interfaces::MakeChain(m_node); std::unique_ptr wallet; }; @@ -608,9 +602,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) { - NodeContext node; - auto chain = interfaces::MakeChain(node); - std::shared_ptr wallet = std::make_shared(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + std::shared_ptr wallet = std::make_shared(m_node.chain.get(), WalletLocation(), CreateDummyWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); wallet->SetMinVersion(FEATURE_LATEST); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); @@ -705,8 +697,8 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup) BOOST_FIXTURE_TEST_CASE(CreateWalletFromFile, TestChain100Setup) { // Create new wallet with known key and unload it. - auto chain = interfaces::MakeChain(m_node); - auto wallet = TestLoadWallet(*chain); + m_node.chain = interfaces::MakeChain(m_node); + auto wallet = TestLoadWallet(*m_node.chain); CKey key; key.MakeNewKey(true); AddKey(*wallet, key); @@ -741,12 +733,12 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletFromFile, TestChain100Setup) auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); - BOOST_CHECK(chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); + BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); // Reload wallet and make sure new transactions are detected despite events // being blocked - wallet = TestLoadWallet(*chain); + wallet = TestLoadWallet(*m_node.chain); BOOST_CHECK(rescan_completed); BOOST_CHECK_EQUAL(addtx_count, 2); { @@ -779,12 +771,12 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletFromFile, TestChain100Setup) block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); - BOOST_CHECK(chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); + BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); LEAVE_CRITICAL_SECTION(wallet->wallet()->cs_wallet); SyncWithValidationInterfaceQueue(); ENTER_CRITICAL_SECTION(wallet->wallet()->cs_wallet); }); - wallet = TestLoadWallet(*chain); + wallet = TestLoadWallet(*m_node.chain); BOOST_CHECK_EQUAL(addtx_count, 4); { LOCK(wallet->cs_wallet);