refactor: decouple db hooks from CFlatDB-based C*Manager objects, mig…

…rate to *Store structs (#5555)

## Motivation

As highlighted in dashpay/dash-issues#52,
decoupling of `CFlatDB`-interacting components from managers of objects
like `CGovernanceManager` and `CSporkManager` is a key task for
achieving deglobalization of Dash-specific components.

The design of `CFlatDB` as a flat database agent relies on hooking into
the object's state its meant to load and store, using its
(de)serialization routines and other miscellaneous functions (notably,
without defining an interface) to achieve those ends. This approach was
taken predominantly for components that want a single-file cache.

Because of the method it uses to hook into the object (templates and the
use of temporary objects), it explicitly prevented passing arguments
into the object constructor, an explicit requirement for storing
references to other components during construction. This, in turn,
created an explicit dependency on those same components being available
in the global context, which would block the backport of bitcoin#21866,
a requirement for future backports meant to achieve parity in
`assumeutxo` support.

The design of these objects made no separation between persistent (i.e.
cached) and ephemeral (i.e. generated/fetched during initialization or
state transitions) data and the design of `CFlatDB` attempts to "clean"
the database by breaching this separation and attempting to access this
ephemeral data.

This might be acceptable if it is contained within the manager itself,
like `CSporkManager`'s `CheckAndRemove()` but is utterly unacceptable
when it relies on other managers (that, as a reminder, are only
accessible through the global state because of restrictions caused by
existing design), like `CGovernanceManager`'s `UpdateCachesAndClean()`.

This pull request aims to separate the `CFlatDB`-interacting portions of
these managers into a struct, with `CFlatDB` interacting only with this
struct, while the manager inherits the struct and manages
load/store/update of the database through the `CFlatDB` instance
initialized within its scope, though the instance only has knowledge of
what is exposed through the limited parent struct.

## Additional information

* As regards to existing behaviour, `CFlatDB` is written entirely as a
header as it relies on templates to specialize itself for the object it
hooks into. Attempting to split the logic and function definitions into
separate files will require you to explicitly define template
specializations, which is tedious.

* `m_db` is defined as a pointer as you cannot instantiate a
forward-declared template (see [this Stack Overflow
answer](https://stackoverflow.com/a/12797282) for more information),
which is done when defined as a member in the object scope.

* The conditional cache flush predicating on RPC _not_ being in the
warm-up state has been replaced with unconditional flushing of the
database on object destruction (@UdjinM6, is this acceptable?)

## TODOs

This is a list of things that aren't within the scope of this pull
request but should be addressed in subsequent pull requests

* [ ] Definition of an interface that `CFlatDB` stores are expected to
implement
* [ ] Lock annotations for all potential uses of members protected by
the `cs` mutex in each manager object and store
* [ ] Additional comments documenting what each function and member does
* [ ] Deglobalization of affected managers

---------

Co-authored-by: Kittywhiskers Van Gogh <[email protected]>

src/coinjoin/client.cpp

@@ -106,18 +106,18 @@ void CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, PeerManager&
                      dmn->pdmnState->addr.ToString());
             return;
         } else {
-            int64_t nLastDsq = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
-            int64_t nDsqThreshold = mmetaman.GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount());
+            int64_t nLastDsq = mmetaman->GetMetaInfo(dmn->proTxHash)->GetLastDsq();
+            int64_t nDsqThreshold = mmetaman->GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount());
             LogPrint(BCLog::COINJOIN, "DSQUEUE -- nLastDsq: %d  nDsqThreshold: %d  nDsqCount: %d\n", nLastDsq,
-                     nDsqThreshold, mmetaman.GetDsqCount());
+                     nDsqThreshold, mmetaman->GetDsqCount());
             // don't allow a few nodes to dominate the queuing process
-            if (nLastDsq != 0 && nDsqThreshold > mmetaman.GetDsqCount()) {
+            if (nLastDsq != 0 && nDsqThreshold > mmetaman->GetDsqCount()) {
                 LogPrint(BCLog::COINJOIN, "DSQUEUE -- Masternode %s is sending too many dsq messages\n",
                          dmn->proTxHash.ToString());
                 return;
             }
 
-            mmetaman.AllowMixing(dmn->proTxHash);
+            mmetaman->AllowMixing(dmn->proTxHash);
 
             LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue (%s) from masternode %s\n", dsq.ToString(),
                      dmn->pdmnState->addr.ToString());
@@ -1144,13 +1144,13 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon
             continue;
         }
 
-        int64_t nLastDsq = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
-        int64_t nDsqThreshold = mmetaman.GetDsqThreshold(dmn->proTxHash, nMnCount);
-        if (nLastDsq != 0 && nDsqThreshold > mmetaman.GetDsqCount()) {
+        int64_t nLastDsq = mmetaman->GetMetaInfo(dmn->proTxHash)->GetLastDsq();
+        int64_t nDsqThreshold = mmetaman->GetDsqThreshold(dmn->proTxHash, nMnCount);
+        if (nLastDsq != 0 && nDsqThreshold > mmetaman->GetDsqCount()) {
             WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- Too early to mix on this masternode!" /* Continued */
                       " masternode=%s  addr=%s  nLastDsq=%d  nDsqThreshold=%d  nDsqCount=%d\n",
                 dmn->proTxHash.ToString(), dmn->pdmnState->addr.ToString(), nLastDsq,
-                nDsqThreshold, mmetaman.GetDsqCount());
+                nDsqThreshold, mmetaman->GetDsqCount());
             nTries++;
             continue;
         }

src/coinjoin/server.cpp

@@ -78,9 +78,9 @@ void CCoinJoinServer::ProcessDSACCEPT(CNode& peer, CDataStream& vRecv)
             }
         }
 
-        int64_t nLastDsq = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
-        int64_t nDsqThreshold = mmetaman.GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount());
-        if (nLastDsq != 0 && nDsqThreshold > mmetaman.GetDsqCount()) {
+        int64_t nLastDsq = mmetaman->GetMetaInfo(dmn->proTxHash)->GetLastDsq();
+        int64_t nDsqThreshold = mmetaman->GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount());
+        if (nLastDsq != 0 && nDsqThreshold > mmetaman->GetDsqCount()) {
             if (fLogIPs) {
                 LogPrint(BCLog::COINJOIN, "DSACCEPT -- last dsq too recent, must wait: peer=%d, addr=%s\n", peer.GetId(), peer.addr.ToString());
             } else {
@@ -161,15 +161,15 @@ void CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, PeerManager& peerman, CD
     }
 
     if (!dsq.fReady) {
-        int64_t nLastDsq = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
-        int64_t nDsqThreshold = mmetaman.GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount());
-        LogPrint(BCLog::COINJOIN, "DSQUEUE -- nLastDsq: %d  nDsqThreshold: %d  nDsqCount: %d\n", nLastDsq, nDsqThreshold, mmetaman.GetDsqCount());
+        int64_t nLastDsq = mmetaman->GetMetaInfo(dmn->proTxHash)->GetLastDsq();
+        int64_t nDsqThreshold = mmetaman->GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount());
+        LogPrint(BCLog::COINJOIN, "DSQUEUE -- nLastDsq: %d  nDsqThreshold: %d  nDsqCount: %d\n", nLastDsq, nDsqThreshold, mmetaman->GetDsqCount());
         //don't allow a few nodes to dominate the queuing process
-        if (nLastDsq != 0 && nDsqThreshold > mmetaman.GetDsqCount()) {
+        if (nLastDsq != 0 && nDsqThreshold > mmetaman->GetDsqCount()) {
             LogPrint(BCLog::COINJOIN, "DSQUEUE -- Masternode %s is sending too many dsq messages\n", dmn->pdmnState->addr.ToString());
             return;
         }
-        mmetaman.AllowMixing(dmn->proTxHash);
+        mmetaman->AllowMixing(dmn->proTxHash);
 
         LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue (%s) from masternode %s\n", dsq.ToString(), dmn->pdmnState->addr.ToString());
 

src/evo/mnauth.cpp

@@ -123,7 +123,7 @@ void CMNAuth::ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connma
     }
 
     if (!peer.fInbound) {
-        mmetaman.GetMetaInfo(mnauth.proRegTxHash)->SetLastOutboundSuccess(GetAdjustedTime());
+        mmetaman->GetMetaInfo(mnauth.proRegTxHash)->SetLastOutboundSuccess(GetAdjustedTime());
         if (peer.m_masternode_probe_connection) {
             LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode probe successful for %s, disconnecting. peer=%d\n",
                      mnauth.proRegTxHash.ToString(), peer.GetId());

src/flat-database.h

@@ -6,6 +6,7 @@
 #define BITCOIN_FLAT_DATABASE_H
 
 #include <clientversion.h>
+#include <chainparams.h>
 #include <fs.h>
 #include <hash.h>
 #include <streams.h>
@@ -35,7 +36,7 @@ class CFlatDB
     std::string strFilename;
     std::string strMagicMessage;
 
-    bool Write(const T& objToSave)
+    bool CoreWrite(const T& objToSave)
     {
         // LOCK(objToSave.cs);
 
@@ -70,7 +71,7 @@ class CFlatDB
         return true;
     }
 
-    ReadResult Read(T& objToLoad, bool fDryRun = false)
+    ReadResult CoreRead(T& objToLoad)
     {
         //LOCK(objToLoad.cs);
 
@@ -151,28 +152,13 @@ class CFlatDB
 
         LogPrintf("Loaded info from %s  %dms\n", strFilename, GetTimeMillis() - nStart);
         LogPrintf("     %s\n", objToLoad.ToString());
-        if(!fDryRun) {
-            LogPrintf("%s: Cleaning....\n", __func__);
-            objToLoad.CheckAndRemove();
-            LogPrintf("     %s\n", objToLoad.ToString());
-        }
 
         return Ok;
     }
 
-
-public:
-    CFlatDB(std::string strFilenameIn, std::string strMagicMessageIn)
-    {
-        pathDB = GetDataDir() / strFilenameIn;
-        strFilename = strFilenameIn;
-        strMagicMessage = strMagicMessageIn;
-    }
-
-    bool Load(T& objToLoad)
+    bool Read(T& objToLoad)
     {
-        LogPrintf("Reading info from %s...\n", strFilename);
-        ReadResult readResult = Read(objToLoad);
+        ReadResult readResult = CoreRead(objToLoad);
         if (readResult == FileError)
             LogPrintf("Missing file %s, will try to recreate\n", strFilename);
         else if (readResult != Ok)
@@ -191,36 +177,34 @@ class CFlatDB
         return true;
     }
 
-    bool Dump(T& objToSave)
+public:
+    CFlatDB(std::string strFilenameIn, std::string strMagicMessageIn)
     {
-        int64_t nStart = GetTimeMillis();
+        pathDB = GetDataDir() / strFilenameIn;
+        strFilename = strFilenameIn;
+        strMagicMessage = strMagicMessageIn;
+    }
 
+    bool Load(T& objToLoad)
+    {
+        LogPrintf("Reading info from %s...\n", strFilename);
+        return Read(objToLoad);
+    }
+
+    bool Store(T& objToSave)
+    {
         LogPrintf("Verifying %s format...\n", strFilename);
         T tmpObjToLoad;
-        ReadResult readResult = Read(tmpObjToLoad, true);
+        if (!Read(tmpObjToLoad)) return false;
 
-        // there was an error and it was not an error on file opening => do not proceed
-        if (readResult == FileError)
-            LogPrintf("Missing file %s, will try to recreate\n", strFilename);
-        else if (readResult != Ok)
-        {
-            LogPrintf("Error reading %s: ", strFilename);
-            if(readResult == IncorrectFormat)
-                LogPrintf("%s: Magic is ok but data has invalid format, will try to recreate\n", __func__);
-            else
-            {
-                LogPrintf("%s: File format is unknown or invalid, please fix it manually\n", __func__);
-                return false;
-            }
-        }
+        int64_t nStart = GetTimeMillis();
 
         LogPrintf("Writing info to %s...\n", strFilename);
-        Write(objToSave);
+        CoreWrite(objToSave);
         LogPrintf("%s dump finished  %dms\n", strFilename, GetTimeMillis() - nStart);
 
         return true;
     }
-
 };
 
 

src/governance/governance.cpp

@@ -9,6 +9,7 @@
 #include <chainparams.h>
 #include <consensus/validation.h>
 #include <evo/deterministicmns.h>
+#include <flat-database.h>
 #include <governance/classes.h>
 #include <governance/validators.h>
 #include <masternode/meta.h>
@@ -26,25 +27,47 @@ std::unique_ptr<CGovernanceManager> governance;
 
 int nSubmittedFinalBudget;
 
-const std::string CGovernanceManager::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-16";
+const std::string GovernanceStore::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-16";
 const int CGovernanceManager::MAX_TIME_FUTURE_DEVIATION = 60 * 60;
 const int CGovernanceManager::RELIABLE_PROPAGATION_TIME = 60;
 
-CGovernanceManager::CGovernanceManager() :
-    nTimeLastDiff(0),
-    nCachedBlockHeight(0),
+GovernanceStore::GovernanceStore() :
+    cs(),
     mapObjects(),
     mapErasedGovernanceObjects(),
     cmapVoteToObject(MAX_CACHE_SIZE),
     cmapInvalidVotes(MAX_CACHE_SIZE),
     cmmapOrphanVotes(MAX_CACHE_SIZE),
     mapLastMasternodeObject(),
+    lastMNListForVotingKeys(std::make_shared<CDeterministicMNList>())
+{
+}
+
+CGovernanceManager::CGovernanceManager() :
+    m_db{std::make_unique<db_type>("governance.dat", "magicGovernanceCache")},
+    nTimeLastDiff(0),
+    nCachedBlockHeight(0),
     setRequestedObjects(),
     fRateChecksEnabled(true),
-    lastMNListForVotingKeys(std::make_shared<CDeterministicMNList>()),
-    votedFundingYesTriggerHash(std::nullopt),
-    cs()
+    votedFundingYesTriggerHash(std::nullopt)
+{
+}
+
+CGovernanceManager::~CGovernanceManager()
 {
+    if (!is_valid) return;
+    m_db->Store(*this);
+}
+
+bool CGovernanceManager::LoadCache(bool load_cache)
+{
+    assert(m_db != nullptr);
+    is_valid = load_cache ? m_db->Load(*this) : m_db->Store(*this);
+    if (is_valid && load_cache) {
+        CheckAndRemove();
+        InitOnLoad();
+    }
+    return is_valid;
 }
 
 // Accessors for thread-safe access to maps
@@ -318,7 +341,7 @@ void CGovernanceManager::UpdateCachesAndClean()
 
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdateCachesAndClean\n");
 
-    std::vector<uint256> vecDirtyHashes = mmetaman.GetAndClearDirtyGovernanceObjectHashes();
+    std::vector<uint256> vecDirtyHashes = mmetaman->GetAndClearDirtyGovernanceObjectHashes();
 
     LOCK2(cs_main, cs);
 
@@ -368,7 +391,7 @@ void CGovernanceManager::UpdateCachesAndClean()
         if ((pObj->IsSetCachedDelete() || pObj->IsSetExpired()) &&
             (nTimeSinceDeletion >= GOVERNANCE_DELETION_DELAY)) {
             LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdateCachesAndClean -- erase obj %s\n", (*it).first.ToString());
-            mmetaman.RemoveGovernanceObject(pObj->GetHash());
+            mmetaman->RemoveGovernanceObject(pObj->GetHash());
 
             // Remove vote references
             const object_ref_cm_t::list_t& listItems = cmapVoteToObject.GetItemList();
@@ -850,13 +873,13 @@ void CGovernanceManager::SyncObjects(CNode& peer, PeerManager& peerman, CConnman
     // do not provide any data until our node is synced
     if (!::masternodeSync->IsSynced()) return;
 
-    if (netfulfilledman.HasFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC)) {
+    if (netfulfilledman->HasFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC)) {
         // Asking for the whole list multiple times in a short period of time is no good
         LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- peer already asked me for the list\n", __func__);
         peerman.Misbehaving(peer.GetId(), 20);
         return;
     }
-    netfulfilledman.AddFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC);
+    netfulfilledman->AddFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC);
 
     int nObjCount = 0;
 
@@ -1323,7 +1346,7 @@ void CGovernanceManager::InitOnLoad()
     LogPrintf("     %s\n", ToString());
 }
 
-std::string CGovernanceManager::ToString() const
+std::string GovernanceStore::ToString() const
 {
     LOCK(cs);