refactor: move CCoinsViewErrorCatcher out of init.cpp #16355

This is only used in the cpp file, no?

Yup - vestige from previous version where function defs were inlined to header. Thanks.

Any reason for not inlining this trivial typdef?

Nope; took your patch.

Looks like modifying the vector of functions is not thread safe? Should be fine with the current code, but maybe remove this method, and make the function member const, so that it has to be provided via the constructor. Which should be thread-safe?

E.g. something like this:

diff --git a/src/coins.cpp b/src/coins.cpp
index 976e058c49..ee0f1ac419 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -5,6 +5,7 @@
 #include <coins.h>
 
 #include <consensus/consensus.h>
+#include <logging.h>
 #include <random.h>
 #include <version.h>
 
@@ -263,9 +264,7 @@ bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) cons
     try {
         return CCoinsViewBacked::GetCoin(outpoint, coin);
     } catch(const std::runtime_error& e) {
-        for (auto f : m_err_callbacks) {
-            f();
-        }
+        m_err_callback();
         LogPrintf("Error reading from database: %s\n", e.what());
         // Starting the shutdown sequence and returning false to the caller would be
         // interpreted as 'entry not found' (as opposed to unable to read data), and
diff --git a/src/coins.h b/src/coins.h
index 2ce788cebf..37bbd7c12b 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -10,7 +10,6 @@
 #include <compressor.h>
 #include <core_memusage.h>
 #include <crypto/siphash.h>
-#include <logging.h>
 #include <memusage.h>
 #include <serialize.h>
 #include <uint256.h>
@@ -327,18 +326,17 @@ const Coin& AccessByTxid(const CCoinsViewCache& cache, const uint256& txid);
 class CCoinsViewErrorCatcher final : public CCoinsViewBacked
 {
 public:
-    typedef std::function<void()> Function;
-
-    explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}
-
-    void AddReadErrCallback(const Function f) { m_err_callbacks.emplace_back(std::move(f)); }
+    CCoinsViewErrorCatcher(CCoinsView* view, const std::function<void()> err_callback)
+        : CCoinsViewBacked{view},
+          m_err_callback{std::move(err_callback)}
+    {
+    }
 
     bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
 
 private:
-    /** A list of callbacks to execute upon leveldb read error. */
-    std::vector<CCoinsViewErrorCatcher::Function> m_err_callbacks;
-
+    /** A callback to execute upon leveldb read error. */
+    const std::function<void()> m_err_callback;
 };
 
 #endif // BITCOIN_COINS_H
diff --git a/src/init.cpp b/src/init.cpp
index 3e4ce2ff02..ca06d92ac0 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1517,12 +1517,12 @@ bool AppInitMain(InitInterfaces& interfaces)
                 // block tree into mapBlockIndex!
 
                 pcoinsdbview.reset(new CCoinsViewDB(nCoinDBCache, false, fReset || fReindexChainState));
-                pcoinscatcher.reset(new CCoinsViewErrorCatcher(pcoinsdbview.get()));
-                pcoinscatcher->AddReadErrCallback([]() {
-                    uiInterface.ThreadSafeMessageBox(
-                        _("Error reading from database, shutting down."),
-                        "", CClientUIInterface::MSG_ERROR);
-                });
+                pcoinscatcher.reset(new CCoinsViewErrorCatcher(pcoinsdbview.get(),
+                    []() {
+                        uiInterface.ThreadSafeMessageBox(
+                            _("Error reading from database, shutting down."),
+                            "", CClientUIInterface::MSG_ERROR);
+                    }));
 
                 // If necessary, upgrade from older database format.
                 // This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate

Sounds great, thanks.

Ah oops, just realizing that we can't do this. In a later version, the coinscatcher is constructed in a place where uiInterface (and qt) isn't known, so we actually need to specify the callback after construction.

Should drop const, otherwise std::move below can't really have any effect.

Fixed, thanks!