refactor: Get rid of more redundant chain methods #19425

nit, should be ::ChainActive?

Anyway, maybe add argument CChain& chain to FillBlock to avoid multiple calls to ChainActive()?

Also, new code should probably use node->chainman.activeChain

re: #19425 (review)

Also, new code should probably use node->chainman.activeChain

Removed ChainActive calls in new commit.

Unrelated, but why return true if ReadBlockfromDisk fails?

re: #19425 (review)

Unrelated, but why return true if ReadBlockfromDisk fails?

findBlock and related methods return true if the block is found, false if it is not found. Whether block data is available is a different question. In practice ReadBlockFromDisk fails when a block is pruned, and callers that want to see if data is pruned can check data.IsNull. We could also add more accessors to return pruning information if there's a need.

            const CChain& active = Assert(m_node.chainman)->ActiveChain();

style-nit, no strong opinion, though.

re: #19425 (review)

style-nit, no strong opinion, though.

Thanks, switched throughout

It seems fragile to require the caller to have to pass inActiveChain here as well and initialize the bool reference that was passed to false.

At the very least the dev docs should mention the footgun.

Curious: Why does the passed bool reference have to be initialized to false? Wouldn't it be okay for it to be initialized to true as well?

That would imply if the block isn't found it is assumed to be in the active chain? Seems a footgun just as dangerous

re: #19425 (review)

That would imply if the block isn't found it is assumed to be in the active chain? Seems a footgun just as dangerous

Information about a block isn't returned if the block doesn't exist. Maybe this is a footgun but it is also pre-existing behavior and how every other accessor works here. I don't think this PR would be improved by initializing this one property while not initializing the others. Inconsistent initialization seems like a bigger footgun, worse than uniformly requiring callers to initialize values that they read.

Maybe in the future another PR could decide on special default values to return for each block property when a block doesn't exist. Maybe this would make calling code safer in case it forgets to initialize a variable. But I could also imagine it leading to other bugs if a caller choses its own different default value and then checks against that wrong value. I could imagine it making calling code harder to read, because you may need to look up default values to understand it instead of being able to rely on initial values being unchanged. All of this is arguable and I don't have strong opinions, but I do think changing this would go beyond scope of current PR and that it would probably be better to look into separately.

Can you explain what this is doing and why the redundant call is needed?

diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp
index 98eac80055..aa436ee3ea 100644
--- a/src/bench/wallet_balance.cpp
+++ b/src/bench/wallet_balance.cpp
@@ -24,7 +24,6 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b
 
     const auto& ADDRESS_WATCHONLY = ADDRESS_BCRT1_UNSPENDABLE;
 
-    test_setup.m_node.chain = interfaces::MakeChain(test_setup.m_node);
     CWallet wallet{test_setup.m_node.chain.get(), "", CreateMockWalletDatabase()};
     {
         wallet.SetupLegacyScriptPubKeyMan();
diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp
index b8c4bc8605..73463b071e 100644
--- a/src/test/interfaces_tests.cpp
+++ b/src/test/interfaces_tests.cpp
@@ -17,7 +17,7 @@ BOOST_FIXTURE_TEST_SUITE(interfaces_tests, TestChain100Setup)
 
 BOOST_AUTO_TEST_CASE(findBlock)
 {
-    auto chain = interfaces::MakeChain(m_node);
+    auto& chain = m_node.chain;
     const CChain& active = Assert(m_node.chainman)->ActiveChain();
 
     uint256 hash;
@@ -61,7 +61,7 @@ BOOST_AUTO_TEST_CASE(findBlock)
 
 BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight)
 {
-    auto chain = interfaces::MakeChain(m_node);
+    auto& chain = m_node.chain;
     const CChain& active = Assert(m_node.chainman)->ActiveChain();
     uint256 hash;
     int height;
@@ -73,7 +73,7 @@ BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight)
 
 BOOST_AUTO_TEST_CASE(findAncestorByHeight)
 {
-    auto chain = interfaces::MakeChain(m_node);
+    auto& chain = m_node.chain;
     const CChain& active = Assert(m_node.chainman)->ActiveChain();
     uint256 hash;
     BOOST_CHECK(chain->findAncestorByHeight(active[20]->GetBlockHash(), 10, FoundBlock().hash(hash)));
@@ -83,7 +83,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHeight)
 
 BOOST_AUTO_TEST_CASE(findAncestorByHash)
 {
-    auto chain = interfaces::MakeChain(m_node);
+    auto& chain = m_node.chain;
     const CChain& active = Assert(m_node.chainman)->ActiveChain();
     int height = -1;
     BOOST_CHECK(chain->findAncestorByHash(active[20]->GetBlockHash(), active[10]->GetBlockHash(), FoundBlock().height(height)));
@@ -93,7 +93,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHash)
 
 BOOST_AUTO_TEST_CASE(findCommonAncestor)
 {
-    auto chain = interfaces::MakeChain(m_node);
+    auto& chain = m_node.chain;
     const CChain& active = Assert(m_node.chainman)->ActiveChain();
     auto* orig_tip = active.Tip();
     for (int i = 0; i < 10; ++i) {
@@ -123,7 +123,7 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor)
 
 BOOST_AUTO_TEST_CASE(hasBlocks)
 {
-    auto chain = interfaces::MakeChain(m_node);
+    auto& chain = m_node.chain;
     const CChain& active = Assert(m_node.chainman)->ActiveChain();
 
     // Test ranges
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index f38ccba384..4127cd45f8 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -283,7 +283,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
     // Make sure that can use BnB when there are preset inputs
     empty_wallet();
     {
-        std::unique_ptr<CWallet> wallet = MakeUnique<CWallet>(m_chain.get(), "", CreateMockWalletDatabase());
+        std::unique_ptr<CWallet> wallet = MakeUnique<CWallet>(m_node.chain.get(), "", CreateMockWalletDatabase());
         bool firstRun;
         wallet->LoadWallet(firstRun);
         wallet->SetupLegacyScriptPubKeyMan();
diff --git a/src/wallet/test/init_test_fixture.cpp b/src/wallet/test/init_test_fixture.cpp
index c80310045a..334e4ae0d8 100644
--- a/src/wallet/test/init_test_fixture.cpp
+++ b/src/wallet/test/init_test_fixture.cpp
@@ -10,7 +10,7 @@
 
 InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)
 {
-    m_wallet_client = MakeWalletClient(*m_chain, *Assert(m_node.args));
+    m_wallet_client = MakeWalletClient(*m_node.chain, *Assert(m_node.args));
 
     std::string sep;
     sep += fs::path::preferred_separator;
diff --git a/src/wallet/test/init_test_fixture.h b/src/wallet/test/init_test_fixture.h
index f5bade77df..f666c45a34 100644
--- a/src/wallet/test/init_test_fixture.h
+++ b/src/wallet/test/init_test_fixture.h
@@ -19,7 +19,6 @@ struct InitWalletDirTestingSetup: public BasicTestingSetup {
     fs::path m_datadir;
     fs::path m_cwd;
     std::map<std::string, fs::path> m_walletdir_path_cases;
-    std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain(m_node);
     std::unique_ptr<interfaces::WalletClient> m_wallet_client;
 };
 
diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp
index d5aed99d99..0ef8b9c4bf 100644
--- a/src/wallet/test/ismine_tests.cpp
+++ b/src/wallet/test/ismine_tests.cpp
@@ -27,8 +27,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard)
     CKey uncompressedKey;
     uncompressedKey.MakeNewKey(false);
     CPubKey uncompressedPubkey = uncompressedKey.GetPubKey();
-    NodeContext node;
-    std::unique_ptr<interfaces::Chain> chain = interfaces::MakeChain(node);
+    std::unique_ptr<interfaces::Chain>& chain = m_node.chain;
 
     CScript scriptPubKey;
     isminetype result;
diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp
index f7c1337b0d..347a436429 100644
--- a/src/wallet/test/scriptpubkeyman_tests.cpp
+++ b/src/wallet/test/scriptpubkeyman_tests.cpp
@@ -17,9 +17,7 @@ BOOST_FIXTURE_TEST_SUITE(scriptpubkeyman_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(CanProvide)
 {
     // Set up wallet and keyman variables.
-    NodeContext node;
-    std::unique_ptr<interfaces::Chain> chain = interfaces::MakeChain(node);
-    CWallet wallet(chain.get(), "", CreateDummyWalletDatabase());
+    CWallet wallet(m_node.chain.get(), "", CreateDummyWalletDatabase());
     LegacyScriptPubKeyMan& keyman = *wallet.GetOrCreateLegacyScriptPubKeyMan();
 
     // Make a 1 of 2 multisig script
diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp
index 4d6f427618..badf2eb459 100644
--- a/src/wallet/test/wallet_test_fixture.cpp
+++ b/src/wallet/test/wallet_test_fixture.cpp
@@ -6,10 +6,10 @@
 
 WalletTestingSetup::WalletTestingSetup(const std::string& chainName)
     : TestingSetup(chainName),
-      m_wallet(m_chain.get(), "", CreateMockWalletDatabase())
+      m_wallet(m_node.chain.get(), "", CreateMockWalletDatabase())
 {
     bool fFirstRun;
     m_wallet.LoadWallet(fFirstRun);
-    m_chain_notifications_handler = m_chain->handleNotifications({ &m_wallet, [](CWallet*) {} });
+    m_chain_notifications_handler = m_node.chain->handleNotifications({ &m_wallet, [](CWallet*) {} });
     m_wallet_client->registerRpcs();
 }
diff --git a/src/wallet/test/wallet_test_fixture.h b/src/wallet/test/wallet_test_fixture.h
index ba8a5ff1f3..ab7fb8c42b 100644
--- a/src/wallet/test/wallet_test_fixture.h
+++ b/src/wallet/test/wallet_test_fixture.h
@@ -20,8 +20,7 @@
 struct WalletTestingSetup : public TestingSetup {
     explicit WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
 
-    std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain(m_node);
-    std::unique_ptr<interfaces::WalletClient> m_wallet_client = interfaces::MakeWalletClient(*m_chain, *Assert(m_node.args));
+    std::unique_ptr<interfaces::WalletClient> m_wallet_client = interfaces::MakeWalletClient(*m_node.chain, *Assert(m_node.args));
     CWallet m_wallet;
     std::unique_ptr<interfaces::Handler> m_chain_notifications_handler;
 };
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 156be053d0..eb0bbb6ccc 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -83,8 +83,6 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
     CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
     CBlockIndex* newTip = ::ChainActive().Tip();
 
-    m_node.chain = interfaces::MakeChain(m_node);
-
     // Verify ScanForWalletTransactions fails to read an unknown start block.
     {
         CWallet wallet(m_node.chain.get(), "", CreateDummyWalletDatabase());
@@ -182,8 +180,6 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
     CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
     CBlockIndex* newTip = ::ChainActive().Tip();
 
-    m_node.chain = interfaces::MakeChain(m_node);
-
     // Prune the older block file.
     {
         LOCK(cs_main);
@@ -253,8 +249,6 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
     SetMockTime(KEY_TIME);
     m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
 
-    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.
@@ -489,7 +483,6 @@ public:
     ListCoinsTestingSetup()
     {
         CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
-        m_node.chain = interfaces::MakeChain(m_node);
         wallet = MakeUnique<CWallet>(m_node.chain.get(), "", CreateMockWalletDatabase());
         {
             LOCK2(wallet->cs_wallet, ::cs_main);
@@ -701,7 +694,6 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup)
 BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
 {
     // Create new wallet with known key and unload it.
-    m_node.chain = interfaces::MakeChain(m_node);
     auto wallet = TestLoadWallet(*m_node.chain);
     CKey key;
     key.MakeNewKey(true);
@@ -794,8 +786,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
 
 BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup)
 {
-    auto chain = interfaces::MakeChain(m_node);
-    auto wallet = TestLoadWallet(*chain);
+    auto wallet = TestLoadWallet(*m_node.chain);
     CKey key;
     key.MakeNewKey(true);
     AddKey(*wallet, key);

Right, looks like anything that has a fixture which inherits from BasicTestingSetup will have m_node.chain properly initialized: https://github.com/bitcoin/bitcoin/blob/a0489f3472f3799dc1ece32a59556fd239c4c14b/src/test/util/setup_common.cpp#L109

re: #19425 (review)

Right, looks like anything that has a fixture which inherits from BasicTestingSetup will have m_node.chain properly initialized:

https://github.com/bitcoin/bitcoin/blob/a0489f3472f3799dc1ece32a59556fd239c4c14b/src/test/util/setup_common.cpp#L109

Thanks, removed this and included Marco's patch in new commit 5baa88fd38c8efa0e361636bb2c60af89d27b5d5. This code in commit 6965f1352d2d7086d308750ce83a84f191a17755 was written July and had to call MakeChain because the change which started setting m_node.chain (#19098) was only merged in August.