Merge #7303: refactor: drop dependency of spork manager on network code

4e6f844 refactor: collapse spork signer recovery to one ECDSA op, restore peer info in logs (UdjinM6)
6a4ee52 fix: replace assert to if-return. Less probability to get troubles in the future (Konstantin Akimov)
7460211 refactor: address review feedback on spork ProcessMessage refactor (UdjinM6)
7c899a2 refactor: drop dependency of spork manager on network code (Konstantin Akimov)
bd2bff1 refactor: move network implementation of spork to net_processing (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  It helps to break circular dependencies over spork <-> net.
  It helps to get spork manager ready kernel project.

  Apparantly, chainstate implementation uses sporks for validation of blockchain but network dependencies could be avoided there.

  This changes is PR-ready changes from #7234 for spork manager.

  ## What was done?
  Dropped dependency of spork manager on network code by moving network code to net_processing.

  There's no need dedicated NetHandler because imlementation is trivial, short and doesn't have any async worker or threads.

  ## How Has This Been Tested?
  Run unit & functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK 4e6f844

Tree-SHA512: 5b06e49c716cf66b66ffe9902fa0f987d0492236b87c45eb285d30def49a2bc66f8ef21b0ae4372465de4598b02f17605c5e127bab79bf2f35c83f0e819b7caf

Author: PastaPastaPasta

src/net_processing.cpp

@@ -5462,6 +5462,35 @@ void PeerManagerImpl::ProcessMessage(
         return;
     }
 
+    if (msg_type == NetMsgType::SPORK) {
+        CSporkMessage spork;
+        vRecv >> spork;
+
+        uint256 hash = spork.GetHash();
+        CInv spork_inv{MSG_SPORK, hash};
+        WITH_LOCK(::cs_main, EraseObjectRequest(pfrom.GetId(), spork_inv));
+        auto opt_signer = m_sporkman.GetValidSporkSigner(spork);
+        if (!opt_signer) {
+            Misbehaving(pfrom.GetId(), 100, strprintf("invalid spork received. peer=%d", pfrom.GetId()));
+            return;
+        }
+        if (m_sporkman.ProcessSpork(spork, *opt_signer, strprintf(" peer=%d", pfrom.GetId()))) {
+            RelayInv(spork_inv);
+        }
+        return;
+    }
+
+    if (msg_type == NetMsgType::GETSPORKS) {
+        // For 'getsporks', active sporks is sent to the requesting peer.
+        auto active_sporks = m_sporkman.ActiveSporks();
+        for (const auto& pair : active_sporks) {
+            for (const auto& signerSporkPair : pair.second) {
+                m_connman.PushMessage(&pfrom, CNetMsgMaker(pfrom.GetCommonVersion()).Make(NetMsgType::SPORK, signerSporkPair.second));
+            }
+        }
+        return;
+    }
+
     if (msg_type == NetMsgType::QUORUMROTATIONINFO) {
         // we have never requested this
         Misbehaving(pfrom.GetId(), 100, strprintf("received not-requested quorumrotationinfo. peer=%d", pfrom.GetId()));
@@ -5508,7 +5537,6 @@ void PeerManagerImpl::ProcessMessage(
             PostProcessMessage(m_cj_walletman->processMessage(pfrom, m_chainman.ActiveChainstate(), m_connman, m_mempool, msg_type, vRecv), pfrom.GetId());
         }
         PostProcessMessage(DKGSessionProcessMessage(pfrom, is_masternode, msg_type, vRecv), pfrom.GetId());
-        PostProcessMessage(m_sporkman.ProcessMessage(pfrom, m_connman, msg_type, vRecv), pfrom.GetId());
         PostProcessMessage(CMNAuth::ProcessMessage(pfrom, peer->m_their_services, m_connman, m_mn_metaman, (m_active_ctx ? m_active_ctx->nodeman.get() : nullptr), m_mn_sync, m_dmnman->GetListAtChainTip(), msg_type, vRecv), pfrom.GetId());
         PostProcessMessage(m_llmq_ctx->quorum_block_processor->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId());
         PostProcessMessage(ProcessPlatformBanMessage(pfrom.GetId(), msg_type, vRecv), pfrom.GetId());

src/spork.cpp

@@ -4,18 +4,15 @@
 
 #include <spork.h>
 
-#include <flat-database.h>
-#include <util/helpers.h>
-
 #include <chainparams.h>
+#include <flat-database.h>
 #include <key_io.h>
 #include <logging.h>
 #include <messagesigner.h>
-#include <net.h>
-#include <netmessagemaker.h>
 #include <protocol.h>
 #include <script/standard.h>
 #include <timedata.h>
+#include <util/helpers.h>
 #include <util/message.h> // for MESSAGE_MAGIC
 #include <util/string.h>
 
@@ -126,51 +123,35 @@ void CSporkManager::CheckAndRemove()
     }
 }
 
-MessageProcessingResult CSporkManager::ProcessMessage(CNode& peer, CConnman& connman, std::string_view msg_type, CDataStream& vRecv)
-{
-    if (msg_type == NetMsgType::SPORK) {
-        return ProcessSpork(peer.GetId(), vRecv);
-    } else if (msg_type == NetMsgType::GETSPORKS) {
-        ProcessGetSporks(peer, connman);
-    }
-    return {};
-}
-
-MessageProcessingResult CSporkManager::ProcessSpork(NodeId from, CDataStream& vRecv)
+std::optional<CKeyID> CSporkManager::GetValidSporkSigner(const CSporkMessage& spork) const
 {
-    CSporkMessage spork;
-    vRecv >> spork;
-
-    uint256 hash = spork.GetHash();
-
-    MessageProcessingResult ret{};
-    ret.m_to_erase = CInv{MSG_SPORK, hash};
-
     if (spork.nTimeSigned > GetAdjustedTime() + 2 * 60 * 60) {
-        LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: too far into the future\n");
-        ret.m_error = MisbehavingError{100};
-        return ret;
+        LogPrint(BCLog::SPORK, "CSporkManager::%s -- ERROR: too far into the future\n", __func__);
+        return std::nullopt;
     }
 
     auto opt_keyIDSigner = spork.GetSignerKeyID();
 
     if (opt_keyIDSigner == std::nullopt || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(*opt_keyIDSigner))) {
-        LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n");
-        ret.m_error = MisbehavingError{100};
-        return ret;
+        LogPrint(BCLog::SPORK, "CSporkManager::%s -- ERROR: invalid signature\n", __func__);
+        return std::nullopt;
     }
+    return opt_keyIDSigner;
+}
 
-    std::string strLogMsg{strprintf("SPORK -- hash: %s id: %d value: %10d peer=%d", hash.ToString(), spork.nSporkID,
-                                    spork.nValue, from)};
-    auto keyIDSigner = *opt_keyIDSigner;
+bool CSporkManager::ProcessSpork(const CSporkMessage& spork, const CKeyID& keyIDSigner, std::string_view peer_log_suffix)
+{
+    uint256 hash = spork.GetHash();
+    std::string strLogMsg{strprintf("SPORK -- hash: %s id: %d value: %10d%s", hash.ToString(), spork.nSporkID,
+                                    spork.nValue, peer_log_suffix)};
 
     {
         LOCK(cs); // make sure to not lock this together with cs_main
         if (mapSporksActive.count(spork.nSporkID)) {
             if (mapSporksActive[spork.nSporkID].count(keyIDSigner)) {
                 if (mapSporksActive[spork.nSporkID][keyIDSigner].nTimeSigned >= spork.nTimeSigned) {
                     LogPrint(BCLog::SPORK, "%s seen\n", strLogMsg);
-                    return ret;
+                    return false;
                 } else {
                     LogPrintf("%s updated\n", strLogMsg);
                 }
@@ -191,21 +172,15 @@ MessageProcessingResult CSporkManager::ProcessSpork(NodeId from, CDataStream& vR
         }
     }
 
-    ret.m_inventory.emplace_back(MSG_SPORK, hash);
-    return ret;
+    return true;
 }
 
-void CSporkManager::ProcessGetSporks(CNode& peer, CConnman& connman)
+std::unordered_map<SporkId, std::map<CKeyID, CSporkMessage>> CSporkManager::ActiveSporks() const
 {
     LOCK(cs); // make sure to not lock this together with cs_main
-    for (const auto& pair : mapSporksActive) {
-        for (const auto& signerSporkPair : pair.second) {
-            connman.PushMessage(&peer, CNetMsgMaker(peer.GetCommonVersion()).Make(NetMsgType::SPORK, signerSporkPair.second));
-        }
-    }
+    return mapSporksActive;
 }
 
-
 std::optional<CInv> CSporkManager::UpdateSpork(SporkId nSporkID, SporkValue nValue)
 {
     CSporkMessage spork(nSporkID, nValue, GetAdjustedTime());

src/spork.h

@@ -5,28 +5,23 @@
 #ifndef BITCOIN_SPORK_H
 #define BITCOIN_SPORK_H
 
-#include <msg_result.h>
-
 #include <hash.h>
 #include <key.h>
-#include <net.h>
-#include <net_types.h>
 #include <pubkey.h>
 #include <saltedhasher.h>
 #include <sync.h>
-#include <uint256.h>
 
 #include <array>
 #include <optional>
 #include <string_view>
 #include <unordered_map>
 #include <vector>
 
-class CConnman;
 template<typename T>
 class CFlatDB;
-class CNode;
 class CDataStream;
+class uint256;
+class CInv;
 
 class CSporkMessage;
 class CSporkManager;
@@ -249,26 +244,27 @@ class CSporkManager : public SporkStore
     void CheckAndRemove() EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     /**
-     * ProcessMessage is used to call ProcessSpork and ProcessGetSporks. See below
+     * GetValidSporkSigner validates signed time and recovers the signer pubkey.
+     * Returns the signer's CKeyID on success, or std::nullopt if the spork is invalid
+     * (peer should be punished in that case).
      */
-    [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& peer, CConnman& connman, std::string_view msg_type,
-                                                         CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache);
-
+    [[nodiscard]] std::optional<CKeyID> GetValidSporkSigner(const CSporkMessage& spork) const
+        EXCLUSIVE_LOCKS_REQUIRED(!cs);
     /**
-     * ProcessSpork is used to handle the 'spork' p2p message.
-     *
-     * For 'spork', it validates the spork and adds it to the internal spork storage and
-     * performs any necessary processing.
+     * ProcessSpork adds the spork to local state. Returns true if the spork was new or
+     * updated and should be relayed. `keyIDSigner` must be the signer key previously
+     * recovered via GetValidSporkSigner. `peer_log_suffix` is appended to log lines for
+     * cross-referencing with the source peer (e.g. " peer=42").
      */
-    [[nodiscard]] MessageProcessingResult ProcessSpork(NodeId from, CDataStream& vRecv)
+    [[nodiscard]] bool ProcessSpork(const CSporkMessage& spork, const CKeyID& keyIDSigner,
+                                    std::string_view peer_log_suffix = {})
         EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache);
 
     /**
-     * ProcessGetSporks is used to handle the 'getsporks' p2p message.
-     *
-     * For 'getsporks', it sends active sporks to the requesting peer.
+     * ActiveSporks returns a snapshot of currently active sporks indexed by SporkId then
+     * signer CKeyID. Used by net_processing to answer the 'getsporks' p2p message.
      */
-    void ProcessGetSporks(CNode& peer, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    std::unordered_map<SporkId, std::map<CKeyID, CSporkMessage>> ActiveSporks() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     /**
      * UpdateSpork is used by the spork RPC command to set a new spork value, sign

test/lint/lint-circular-dependencies.py

@@ -23,7 +23,6 @@
     # Dash
     "active/context -> active/dkgsessionhandler -> llmq/dkgsessionhandler -> net_processing -> active/context",
     "banman -> common/bloom -> evo/assetlocktx -> llmq/quorumsman -> llmq/blockprocessor -> net -> banman",
-    "chainlock/chainlock -> spork -> net -> evo/deterministicmns -> evo/providertx -> validation -> chainlock/chainlock",
     "coinjoin/client -> coinjoin/util -> wallet/wallet -> psbt -> node/transaction -> net_processing -> coinjoin/walletman -> coinjoin/client",
     "common/bloom -> evo/assetlocktx -> llmq/commitment -> evo/deterministicmns -> evo/simplifiedmns -> merkleblock -> common/bloom",
     "common/bloom -> evo/assetlocktx -> llmq/quorumsman -> llmq/blockprocessor -> net -> common/bloom",