Merge #18923: wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off

fa73493930 refactor: Use C++11 range-based for loop (MarcoFalke)
fa7b164d62 wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off (MarcoFalke)
faf8401c19 wallet: Pass unused args to StartWallets (MarcoFalke)
fa6c186436 gui tests: Limit life-time of dummy testing setup (MarcoFalke)
fa28a61897 test: Add smoke test to check that wallets are flushed by default (MarcoFalke)

Pull request description:

  User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second.

ACKs for top commit:
  jnewbery:
    utACK fa73493930
  meshcollider:
    utACK fa73493930
  ryanofsky:
    Code review ACK fa73493930. Just rebased since last review

Tree-SHA512: 99e1fe1b2c22a3f4b19de3e566241d38693f4fd8d5a68ba1838d86740aa6c08e3325c11a072e30fd262a8861af4278bed52eb9374c85179b8f536477f528247c

src/interfaces/chain.h

@@ -15,6 +15,7 @@
 #include <string>
 #include <vector>
 
+class ArgsManager;
 class CBlock;
 class CFeeRate;
 class CRPCCommand;
@@ -322,7 +323,7 @@ std::unique_ptr<Chain> MakeChain(NodeContext& node);
 //! analysis, or fee estimation. These clients need to expose their own
 //! MakeXXXClient functions returning their implementations of the ChainClient
 //! interface.
-std::unique_ptr<ChainClient> MakeWalletClient(Chain& chain, std::vector<std::string> wallet_filenames);
+std::unique_ptr<ChainClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames);
 
 } // namespace interfaces
 

src/interfaces/wallet.cpp

@@ -483,10 +483,11 @@ public:
 class WalletClientImpl : public ChainClient
 {
 public:
-    WalletClientImpl(Chain& chain, std::vector<std::string> wallet_filenames)
+    WalletClientImpl(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)
         : m_wallet_filenames(std::move(wallet_filenames))
     {
         m_context.chain = &chain;
+        m_context.args = &args;
     }
     void registerRpcs() override
     {
@@ -499,7 +500,7 @@ public:
     }
     bool verify() override { return VerifyWallets(*m_context.chain, m_wallet_filenames); }
     bool load() override { return LoadWallets(*m_context.chain, m_wallet_filenames); }
-    void start(CScheduler& scheduler) override { return StartWallets(scheduler); }
+    void start(CScheduler& scheduler) override { return StartWallets(scheduler, *Assert(m_context.args)); }
     void flush() override { return FlushWallets(); }
     void stop() override { return StopWallets(); }
     void setMockTime(int64_t time) override { return SetMockTime(time); }
@@ -514,7 +515,7 @@ public:
     ~WalletClientImpl() override { UnloadWallets(); }
 
     WalletContext m_context;
-    std::vector<std::string> m_wallet_filenames;
+    const std::vector<std::string> m_wallet_filenames;
     std::vector<std::unique_ptr<Handler>> m_rpc_handlers;
     std::list<CRPCCommand> m_rpc_commands;
 };
@@ -523,9 +524,9 @@ public:
 
 std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? MakeUnique<WalletImpl>(wallet) : nullptr; }
 
-std::unique_ptr<ChainClient> MakeWalletClient(Chain& chain, std::vector<std::string> wallet_filenames)
+std::unique_ptr<ChainClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)
 {
-    return MakeUnique<WalletClientImpl>(chain, std::move(wallet_filenames));
+    return MakeUnique<WalletClientImpl>(chain, args, std::move(wallet_filenames));
 }
 
 } // namespace interfaces

src/qt/test/apptests.cpp

@@ -62,9 +62,10 @@ void AppTests::appTests()
     }
 #endif
 
-    BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to
-    ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
-    LogInstance().DisconnectTestLogger();
+    fs::create_directories([] {
+        BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to
+        return GetDataDir() / "blocks";
+    }());
 
     m_app.parameterSetup();
     m_app.createOptionsModel(true /* reset settings */);
@@ -80,6 +81,7 @@ void AppTests::appTests()
     m_app.exec();
 
     // Reset global state to avoid interfering with later tests.
+    LogInstance().DisconnectTestLogger();
     AbortShutdown();
     UnloadBlockIndex();
     WITH_LOCK(::cs_main, g_chainman.Reset());

src/qt/test/test_main.cpp

@@ -40,7 +40,7 @@ Q_IMPORT_PLUGIN(QCocoaIntegrationPlugin);
 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
 // This is all you need to run all the tests
-int main(int argc, char *argv[])
+int main(int argc, char* argv[])
 {
     // Initialize persistent globals with the testing setup state for sanity.
     // E.g. -datadir in gArgs is set to a temp directory dummy value (instead
@@ -70,6 +70,8 @@ int main(int argc, char *argv[])
     BitcoinApplication app(*node);
     app.setApplicationName("Bitcoin-Qt-test");
 
+    node->setupServerArgs();            // Make gArgs available in the NodeContext
+    node->context()->args->ClearArgs(); // Clear added args again
     AppTests app_tests(app);
     if (QTest::qExec(&app_tests) != 0) {
         fInvalid = true;

src/wallet/bdb.cpp

@@ -623,8 +623,8 @@ bool BerkeleyDatabase::PeriodicFlush()
     if (!lockDb) return false;
 
     // Don't flush if any databases are in use
-    for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) {
-        if ((*it).second > 0) return false;
+    for (const auto& use_count : env->mapFileUseCount) {
+        if (use_count.second > 0) return false;
     }
 
     // Don't flush if there haven't been any batch writes for this database.

src/wallet/context.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_WALLET_CONTEXT_H
 #define BITCOIN_WALLET_CONTEXT_H
 
+class ArgsManager;
 namespace interfaces {
 class Chain;
 } // namespace interfaces
@@ -21,6 +22,7 @@ class Chain;
 //! behavior.
 struct WalletContext {
     interfaces::Chain* chain{nullptr};
+    ArgsManager* args{nullptr};
 
     //! Declare default constructor and destructor that are not inline, so code
     //! instantiating the WalletContext struct doesn't need to #include class

src/wallet/init.cpp

@@ -9,6 +9,7 @@
 #include <node/context.h>
 #include <node/ui_interface.h>
 #include <outputtype.h>
+#include <util/check.h>
 #include <util/moneystr.h>
 #include <util/system.h>
 #include <util/translation.h>
@@ -16,9 +17,9 @@
 #include <wallet/wallet.h>
 #include <walletinitinterface.h>
 
-class WalletInit : public WalletInitInterface {
+class WalletInit : public WalletInitInterface
+{
 public:
-
     //! Was the wallet component compiled in.
     bool HasWalletSupport() const override {return true;}
 
@@ -112,10 +113,11 @@ bool WalletInit::ParameterInteraction() const
 
 void WalletInit::Construct(NodeContext& node) const
 {
-    if (gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) {
+    ArgsManager& args = *Assert(node.args);
+    if (args.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) {
         LogPrintf("Wallet disabled!\n");
         return;
     }
-    gArgs.SoftSetArg("-wallet", "");
-    node.chain_clients.emplace_back(interfaces::MakeWalletClient(*node.chain, gArgs.GetArgs("-wallet")));
+    args.SoftSetArg("-wallet", "");
+    node.chain_clients.emplace_back(interfaces::MakeWalletClient(*node.chain, args, args.GetArgs("-wallet")));
 }

src/wallet/load.cpp

@@ -11,6 +11,7 @@
 #include <util/system.h>
 #include <util/translation.h>
 #include <wallet/wallet.h>
+#include <wallet/walletdb.h>
 
 bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
 {
@@ -82,14 +83,16 @@ bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& walle
     }
 }
 
-void StartWallets(CScheduler& scheduler)
+void StartWallets(CScheduler& scheduler, const ArgsManager& args)
 {
     for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
         pwallet->postInitProcess();
     }
 
     // Schedule periodic wallet flushes and tx rebroadcasts
-    scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
+    if (args.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
+        scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
+    }
     scheduler.scheduleEvery(MaybeResendWalletTxs, std::chrono::milliseconds{1000});
 }
 

src/wallet/load.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+class ArgsManager;
 class CScheduler;
 
 namespace interfaces {
@@ -22,7 +23,7 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
 bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files);
 
 //! Complete startup of wallets.
-void StartWallets(CScheduler& scheduler);
+void StartWallets(CScheduler& scheduler, const ArgsManager& args);
 
 //! Flush all wallets in preparation for shutdown.
 void FlushWallets();

src/wallet/test/init_test_fixture.cpp

@@ -3,13 +3,14 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <fs.h>
+#include <util/check.h>
 #include <util/system.h>
 
 #include <wallet/test/init_test_fixture.h>
 
-InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainName): BasicTestingSetup(chainName)
+InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)
 {
-    m_chain_client = MakeWalletClient(*m_chain, {});
+    m_chain_client = MakeWalletClient(*m_chain, *Assert(m_node.args), {});
 
     std::string sep;
     sep += fs::path::preferred_separator;

src/wallet/test/wallet_test_fixture.h

@@ -10,17 +10,18 @@
 #include <interfaces/chain.h>
 #include <interfaces/wallet.h>
 #include <node/context.h>
+#include <util/check.h>
 #include <wallet/wallet.h>
 
 #include <memory>
 
 /** Testing setup and teardown for wallet.
  */
-struct WalletTestingSetup: public TestingSetup {
+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::ChainClient> m_chain_client = interfaces::MakeWalletClient(*m_chain, {});
+    std::unique_ptr<interfaces::ChainClient> m_chain_client = interfaces::MakeWalletClient(*m_chain, *Assert(m_node.args), {});
     CWallet m_wallet;
     std::unique_ptr<interfaces::Handler> m_chain_notifications_handler;
 };

src/wallet/walletdb.cpp

@@ -952,9 +952,6 @@ void MaybeCompactWalletDB()
     if (fOneThread.exchange(true)) {
         return;
     }
-    if (!gArgs.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
-        return;
-    }
 
     for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
         WalletDatabase& dbh = pwallet->GetDBHandle();

test/functional/wallet_dump.py

@@ -202,5 +202,10 @@ class WalletDumpTest(BitcoinTestFramework):
         result = self.nodes[0].getaddressinfo(multisig_addr)
         assert result['ismine']
 
+        self.log.info('Check that wallet is flushed')
+        with self.nodes[0].assert_debug_log(['Flushing wallet.dat'], timeout=20):
+            self.nodes[0].getnewaddress()
+
+
 if __name__ == '__main__':
     WalletDumpTest().main()