Merge #6820: refactor: migrate ProcessMessage definitions to MessageProcessingResult, drop PeerMsgRet

491ae50 revert: new util class `expected` for return errors by more convenient way (Kittywhiskers Van Gogh)
2020757 trivial: remove `PeerMsgRet` handling logic (Kittywhiskers Van Gogh)
5d1222e chore: drop govobj/govobjvote counts, use `ret.m_inventory.size()` (UdjinM6)
4822b93 refactor: allow submitting multiple `CInv`s to `MessageProcessingResult` (Kittywhiskers Van Gogh)
0517aff refactor: mark `RelayInv{,Filtered}`'s `CInv` argument as const (Kittywhiskers Van Gogh)
9084530 refactor: migrate `CGovernanceManager::ProcessMessage()` and friends (Kittywhiskers Van Gogh)
8b9bf7c refactor: migrate `CCoinJoinServer::ProcessMessage()` (Kittywhiskers Van Gogh)
1d50a1a refactor: migrate `CCoinJoinClientQueueManager::ProcessMessage()` (Kittywhiskers Van Gogh)
6352baf refactor: migrate `CMNAuth::ProcessMessage()` (Kittywhiskers Van Gogh)
9f85bd0 refactor: migrate `CInstantSendManager::ProcessMessage()` (Kittywhiskers Van Gogh)
ef4a0bb refactor: migrate `CDKGSessionManager::ProcessMessage()` and friends (Kittywhiskers Van Gogh)
93d68b8 refactor: migrate `CQuorumManager::ProcessMessage()` (Kittywhiskers Van Gogh)
1bbcb95 refactor: migrate `CSporkManager::ProcessMessage()` and friends (Kittywhiskers Van Gogh)
e7b23a2 refactor: migrate `CSigningManager::ProcessMessage()` (Kittywhiskers Van Gogh)
f3b98ce chore: add redefinition of `NodeId` to avoid repetitive redefinition (Kittywhiskers Van Gogh)
f341ef5 chore: add `nodiscard` attrib to `MessageProcessingResult` ret functions (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #6821

  * To avoid having to redeclare `NodeId` repeatedly to avoid circular dependencies from including `net.h` ([source](https://github.com/dashpay/dash/blob/fb87a5a937dd232d4f2ae60b5b8c5c29d49cb92f/src/net.h#L121)), a redeclaration is made in the relatively lightweight `net_types.h` that as of `develop` (fb87a5a), doesn't have includes outside the standard library ([source](https://github.com/dashpay/dash/blob/fb87a5a937dd232d4f2ae60b5b8c5c29d49cb92f/src/net_types.h#L8-L10)).

    Redeclarations were flagged during review (see [comment](#6820 (comment)), [comment](#6820 (comment))).

  * As `tl::expected` was introduced alongside `PeerMsgRet` in [dash#5782](#5782) and has not been used elsewhere, we can safely remove it as we've finished migrating all usage to `MessageProcessingResult`.

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK 491ae50
  UdjinM6:
    utACK 491ae50

Tree-SHA512: c37fe6370555f1c33877fa5a335356e2febf25c2d0d954fa44623f49994647ea2a10aec5ceb9e788bb6fa9315dc00b3e54ca8e0da96c7899fb31c30b888eb732

Author: PastaPastaPasta

contrib/devtools/copyright_header.py

@@ -25,7 +25,6 @@
     'src/ctpl_stl.h',
     'src/test/fuzz/FuzzedDataProvider.h',
     'src/tinyformat.h',
-    'src/util/expected.h',
     'src/wallet/bip39.cpp',
     'src/wallet/bip39.h',
     'src/wallet/bip39_english.h',

src/Makefile.am

@@ -357,7 +357,6 @@ BITCOIN_CORE_H = \
   util/enumerate.h \
   util/epochguard.h \
   util/error.h \
-  util/expected.h \
   util/fastrange.h \
   util/fees.h \
   util/golombrice.h \

src/chainlock/chainlock.h

@@ -7,6 +7,7 @@
 
 #include <chainlock/clsig.h>
 
+#include <net_types.h>
 #include <primitives/transaction.h>
 #include <protocol.h>
 #include <saltedhasher.h>
@@ -28,8 +29,6 @@ class CScheduler;
 class CSporkManager;
 class CTxMemPool;
 
-using NodeId = int64_t;
-
 namespace llmq {
 class CInstantSendManager;
 class CQuorumManager;

src/chainlock/signing.h

@@ -30,7 +30,7 @@ class ChainLockSignerParent
     virtual bool HasConflictingChainLock(int nHeight, const uint256& blockHash) const = 0;
     virtual bool IsEnabled() const = 0;
     virtual bool IsTxSafeForMining(const uint256& txid) const = 0;
-    virtual MessageProcessingResult ProcessNewChainLock(NodeId from, const ChainLockSig& clsig, const uint256& hash) = 0;
+    [[nodiscard]] virtual MessageProcessingResult ProcessNewChainLock(NodeId from, const ChainLockSig& clsig, const uint256& hash) = 0;
     virtual void UpdateTxFirstSeenMap(const std::unordered_set<uint256, StaticSaltedHasher>& tx, const int64_t& time) = 0;
 };
 

src/coinjoin/client.cpp

@@ -38,41 +38,36 @@ using wallet::CoinType;
 using wallet::CWallet;
 using wallet::ReserveDestination;
 
-PeerMsgRet CCoinJoinClientQueueManager::ProcessMessage(const CNode& peer, CConnman& connman, PeerManager& peerman,
-                                                       std::string_view msg_type, CDataStream& vRecv)
+MessageProcessingResult CCoinJoinClientQueueManager::ProcessMessage(NodeId from, CConnman& connman, PeerManager& peerman,
+                                                                    std::string_view msg_type, CDataStream& vRecv)
 {
+    if (msg_type != NetMsgType::DSQUEUE) {
+        return {};
+    }
+
     if (m_is_masternode) return {};
     if (!m_mn_sync.IsBlockchainSynced()) return {};
 
-    if (msg_type == NetMsgType::DSQUEUE) {
-        return CCoinJoinClientQueueManager::ProcessDSQueue(peer, connman, peerman, vRecv);
-    }
-    return {};
-}
-
-PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CConnman& connman, PeerManager& peerman,
-                                                       CDataStream& vRecv)
-{
     assert(m_mn_metaman.IsValid());
 
     CCoinJoinQueue dsq;
     vRecv >> dsq;
 
-    {
-        LOCK(::cs_main);
-        peerman.EraseObjectRequest(peer.GetId(), CInv(MSG_DSQ, dsq.GetHash()));
-    }
+    MessageProcessingResult ret{};
+    ret.m_to_erase = CInv{MSG_DSQ, dsq.GetHash()};
 
     if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
-        return tl::unexpected{100};
+        ret.m_error = MisbehavingError{100};
+        return ret;
     }
 
     const auto tip_mn_list = m_dmnman.GetListAtChainTip();
     if (dsq.masternodeOutpoint.IsNull()) {
         if (auto dmn = tip_mn_list.GetValidMN(dsq.m_protxHash)) {
             dsq.masternodeOutpoint = dmn->collateralOutpoint;
         } else {
-            return tl::unexpected{10};
+            ret.m_error = MisbehavingError{10};
+            return ret;
         }
     }
 
@@ -84,31 +79,32 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CConnm
             // process every dsq only once
             for (const auto &q: vecCoinJoinQueue) {
                 if (q == dsq) {
-                    return {};
+                    return ret;
                 }
                 if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) {
                     // no way the same mn can send another dsq with the same readiness this soon
                     LogPrint(BCLog::COINJOIN, /* Continued */
-                             "DSQUEUE -- Peer %s is sending WAY too many dsq messages for a masternode with collateral %s\n",
-                             peer.GetLogString(), dsq.masternodeOutpoint.ToStringShort());
-                    return {};
+                             "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n",
+                             from, dsq.masternodeOutpoint.ToStringShort());
+                    return ret;
                 }
             }
         } // cs_vecqueue
 
         LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString());
 
-        if (dsq.IsTimeOutOfBounds()) return {};
+        if (dsq.IsTimeOutOfBounds()) return ret;
 
         auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint);
-        if (!dmn) return {};
+        if (!dmn) return ret;
 
         if (dsq.m_protxHash.IsNull()) {
             dsq.m_protxHash = dmn->proTxHash;
         }
 
         if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
-            return tl::unexpected{10};
+            ret.m_error = MisbehavingError{10};
+            return ret;
         }
 
         // if the queue is ready, submit if we can
@@ -117,7 +113,7 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CConnm
                 return clientman->TrySubmitDenominate(dmn->proTxHash, connman);
             })) {
             LogPrint(BCLog::COINJOIN, "DSQUEUE -- CoinJoin queue is ready, masternode=%s, queue=%s\n", dmn->proTxHash.ToString(), dsq.ToString());
-            return {};
+            return ret;
         } else {
             int64_t nLastDsq = m_mn_metaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq();
             int64_t nDsqThreshold = m_mn_metaman.GetDsqThreshold(dmn->proTxHash, tip_mn_list.GetValidMNsCount());
@@ -127,7 +123,7 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CConnm
             if (nLastDsq != 0 && nDsqThreshold > m_mn_metaman.GetDsqCount()) {
                 LogPrint(BCLog::COINJOIN, "DSQUEUE -- Masternode %s is sending too many dsq messages\n",
                          dmn->proTxHash.ToString());
-                return {};
+                return ret;
             }
 
             m_mn_metaman.AllowMixing(dmn->proTxHash);
@@ -142,7 +138,7 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CConnm
         }
     } // cs_ProcessDSQueue
     peerman.RelayDSQ(dsq);
-    return {};
+    return ret;
 }
 
 void CCoinJoinClientManager::ProcessMessage(CNode& peer, CChainState& active_chainstate, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv)

src/coinjoin/client.h

@@ -8,6 +8,7 @@
 #include <coinjoin/util.h>
 #include <coinjoin/coinjoin.h>
 
+#include <net_types.h>
 #include <protocol.h>
 #include <util/ranges.h>
 #include <util/translation.h>
@@ -252,9 +253,9 @@ class CCoinJoinClientQueueManager : public CCoinJoinBaseManager
         m_mn_sync(mn_sync),
         m_is_masternode{is_masternode} {};
 
-    PeerMsgRet ProcessMessage(const CNode& peer, CConnman& connman, PeerManager& peerman, std::string_view msg_type,
-                              CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);
-    PeerMsgRet ProcessDSQueue(const CNode& peer, CConnman& connman, PeerManager& peerman, CDataStream& vRecv);
+    [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, CConnman& connman, PeerManager& peerman, std::string_view msg_type,
+                                                         CDataStream& vRecv)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);
     void DoMaintenance();
 };
 

src/coinjoin/server.cpp

@@ -23,15 +23,15 @@
 
 #include <univalue.h>
 
-PeerMsgRet CCoinJoinServer::ProcessMessage(CNode& peer, std::string_view msg_type, CDataStream& vRecv)
+MessageProcessingResult CCoinJoinServer::ProcessMessage(CNode& peer, std::string_view msg_type, CDataStream& vRecv)
 {
     if (!m_mn_activeman) return {};
     if (!m_mn_sync.IsBlockchainSynced()) return {};
 
     if (msg_type == NetMsgType::DSACCEPT) {
         ProcessDSACCEPT(peer, vRecv);
     } else if (msg_type == NetMsgType::DSQUEUE) {
-        return ProcessDSQUEUE(peer, vRecv);
+        return ProcessDSQUEUE(peer.GetId(), vRecv);
     } else if (msg_type == NetMsgType::DSVIN) {
         ProcessDSVIN(peer, vRecv);
     } else if (msg_type == NetMsgType::DSSIGNFINALTX) {
@@ -109,61 +109,62 @@ void CCoinJoinServer::ProcessDSACCEPT(CNode& peer, CDataStream& vRecv)
     }
 }
 
-PeerMsgRet CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv)
+MessageProcessingResult CCoinJoinServer::ProcessDSQUEUE(NodeId from, CDataStream& vRecv)
 {
     assert(m_mn_metaman.IsValid());
 
     CCoinJoinQueue dsq;
     vRecv >> dsq;
 
-    {
-        LOCK(::cs_main);
-        Assert(m_peerman)->EraseObjectRequest(peer.GetId(), CInv(MSG_DSQ, dsq.GetHash()));
-    }
+    MessageProcessingResult ret{};
+    ret.m_to_erase = CInv{MSG_DSQ, dsq.GetHash()};
 
     if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
-        return tl::unexpected{100};
+        ret.m_error = MisbehavingError{100};
+        return ret;
     }
 
     const auto tip_mn_list = m_dmnman.GetListAtChainTip();
     if (dsq.masternodeOutpoint.IsNull()) {
         if (auto dmn = tip_mn_list.GetValidMN(dsq.m_protxHash)) {
             dsq.masternodeOutpoint = dmn->collateralOutpoint;
         } else {
-            return tl::unexpected{10};
+            ret.m_error = MisbehavingError{10};
+            return ret;
         }
     }
 
     {
         TRY_LOCK(cs_vecqueue, lockRecv);
-        if (!lockRecv) return {};
+        if (!lockRecv) return ret;
 
         // process every dsq only once
         for (const auto& q : vecCoinJoinQueue) {
             if (q == dsq) {
-                return {};
+                return ret;
             }
             if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) {
                 // no way the same mn can send another dsq with the same readiness this soon
-                LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %s is sending WAY too many dsq messages for a masternode with collateral %s\n", peer.GetLogString(), dsq.masternodeOutpoint.ToStringShort());
-                return {};
+                LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", from, dsq.masternodeOutpoint.ToStringShort());
+                return ret;
             }
         }
     } // cs_vecqueue
 
     LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString());
 
-    if (dsq.IsTimeOutOfBounds()) return {};
+    if (dsq.IsTimeOutOfBounds()) return ret;
 
     auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint);
-    if (!dmn) return {};
+    if (!dmn) return ret;
 
     if (dsq.m_protxHash.IsNull()) {
         dsq.m_protxHash = dmn->proTxHash;
     }
 
     if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
-        return tl::unexpected{10};
+        ret.m_error = MisbehavingError{10};
+        return ret;
     }
 
     if (!dsq.fReady) {
@@ -173,18 +174,18 @@ PeerMsgRet CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv
         //don't allow a few nodes to dominate the queuing process
         if (nLastDsq != 0 && nDsqThreshold > m_mn_metaman.GetDsqCount()) {
             LogPrint(BCLog::COINJOIN, "DSQUEUE -- node sending too many dsq messages, masternode=%s\n", dmn->proTxHash.ToString());
-            return {};
+            return ret;
         }
         m_mn_metaman.AllowMixing(dmn->proTxHash);
 
         LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue, masternode=%s, queue=%s\n", dmn->proTxHash.ToString(), dsq.ToString());
 
         TRY_LOCK(cs_vecqueue, lockRecv);
-        if (!lockRecv) return {};
+        if (!lockRecv) return ret;
         vecCoinJoinQueue.push_back(dsq);
         m_peerman->RelayDSQ(dsq);
     }
-    return {};
+    return ret;
 }
 
 void CCoinJoinServer::ProcessDSVIN(CNode& peer, CDataStream& vRecv)

src/coinjoin/server.h

@@ -7,6 +7,7 @@
 
 #include <coinjoin/coinjoin.h>
 
+#include <net_types.h>
 #include <protocol.h>
 
 class CActiveMasternodeManager;
@@ -85,7 +86,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager
     void RelayCompletedTransaction(PoolMessage nMessageID) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
 
     void ProcessDSACCEPT(CNode& peer, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);
-    PeerMsgRet ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);
+    [[nodiscard]] MessageProcessingResult ProcessDSQUEUE(NodeId from, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);
     void ProcessDSVIN(CNode& peer, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
     void ProcessDSSIGNFINALTX(CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
 
@@ -110,7 +111,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager
         fUnitTest(false)
     {}
 
-    PeerMsgRet ProcessMessage(CNode& pfrom, std::string_view msg_type, CDataStream& vRecv);
+    [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& pfrom, std::string_view msg_type, CDataStream& vRecv);
 
     bool HasTimedOut() const;
     void CheckTimeout();

src/evo/mnauth.cpp

@@ -55,9 +55,9 @@ void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CActiveMasternode
     connman.PushMessage(&peer, CNetMsgMaker(peer.GetCommonVersion()).Make(NetMsgType::MNAUTH, mnauth));
 }
 
-PeerMsgRet CMNAuth::ProcessMessage(CNode& peer, ServiceFlags node_services, CConnman& connman, CMasternodeMetaMan& mn_metaman, const CActiveMasternodeManager* const mn_activeman,
-                                   const CMasternodeSync& mn_sync, const CDeterministicMNList& tip_mn_list,
-                                   std::string_view msg_type, CDataStream& vRecv)
+MessageProcessingResult CMNAuth::ProcessMessage(CNode& peer, ServiceFlags node_services, CConnman& connman, CMasternodeMetaMan& mn_metaman,
+                                                const CActiveMasternodeManager* const mn_activeman, const CMasternodeSync& mn_sync,
+                                                const CDeterministicMNList& tip_mn_list, std::string_view msg_type, CDataStream& vRecv)
 {
     assert(mn_metaman.IsValid());
 
@@ -71,30 +71,30 @@ PeerMsgRet CMNAuth::ProcessMessage(CNode& peer, ServiceFlags node_services, CCon
 
     // only one MNAUTH allowed
     if (!peer.GetVerifiedProRegTxHash().IsNull()) {
-        return tl::unexpected{MisbehavingError{100, "duplicate mnauth"}};
+        return MisbehavingError{100, "duplicate mnauth"};
     }
 
     if ((~node_services) & (NODE_NETWORK | NODE_BLOOM)) {
         // either NODE_NETWORK or NODE_BLOOM bit is missing in node's services
-        return tl::unexpected{MisbehavingError{100, "mnauth from a node with invalid services"}};
+        return MisbehavingError{100, "mnauth from a node with invalid services"};
     }
 
     if (mnauth.proRegTxHash.IsNull()) {
-        return tl::unexpected{MisbehavingError{100, "empty mnauth proRegTxHash"}};
+        return MisbehavingError{100, "empty mnauth proRegTxHash"};
     }
 
     if (!mnauth.sig.IsValid()) {
         LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- invalid mnauth for protx=%s with sig=%s\n",
                  mnauth.proRegTxHash.ToString(), mnauth.sig.ToString(false));
-        return tl::unexpected{MisbehavingError{100, "invalid mnauth signature"}};
+        return MisbehavingError{100, "invalid mnauth signature"};
     }
 
     const auto dmn = tip_mn_list.GetMN(mnauth.proRegTxHash);
     if (!dmn) {
         // in case node was unlucky and not up to date, just let it be connected as a regular node, which gives it
         // a chance to get up-to-date and thus realize that it's not a MN anymore. We still give it a
         // low DoS score.
-        return tl::unexpected{MisbehavingError{10, "missing mnauth masternode"}};
+        return MisbehavingError{10, "missing mnauth masternode"};
     }
 
     uint256 signHash;
@@ -114,7 +114,7 @@ PeerMsgRet CMNAuth::ProcessMessage(CNode& peer, ServiceFlags node_services, CCon
     if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator.Get(), signHash, false)) {
         // Same as above, MN seems to not know its fate yet, so give it a chance to update. If this is a
         // malicious node (DoSing us), it'll get banned soon.
-        return tl::unexpected{MisbehavingError{10, "mnauth signature verification failed"}};
+        return MisbehavingError{10, "mnauth signature verification failed"};
     }
 
     if (!peer.IsInboundConn()) {

src/evo/mnauth.h

@@ -58,9 +58,9 @@ class CMNAuth
      * @pre CMasternodeMetaMan's database must be successfully loaded before
      *      attempting to call this function regardless of sync state
      */
-    static PeerMsgRet ProcessMessage(CNode& peer, ServiceFlags node_services, CConnman& connman, CMasternodeMetaMan& mn_metaman, const CActiveMasternodeManager* const mn_activeman,
-                                     const CMasternodeSync& mn_sync, const CDeterministicMNList& tip_mn_list,
-                                     std::string_view msg_type, CDataStream& vRecv);
+    [[nodiscard]] static MessageProcessingResult ProcessMessage(CNode& peer, ServiceFlags node_services, CConnman& connman, CMasternodeMetaMan& mn_metaman,
+                                                                const CActiveMasternodeManager* const mn_activeman, const CMasternodeSync& mn_sync,
+                                                                const CDeterministicMNList& tip_mn_list, std::string_view msg_type, CDataStream& vRecv);
     static void NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& oldMNList, const CDeterministicMNListDiff& diff, CConnman& connman);
 };