Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
knst wants to merge 4 commits into
base: develop
Choose a base branch
from
+59 −61
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd2bff1
refactor: move network implementation of spork to net_processing
knst May 2, 2026
7c899a2
refactor: drop dependency of spork manager on network code
knst May 2, 2026
7460211
refactor: address review feedback on spork ProcessMessage refactor
UdjinM6 May 3, 2026
6a4ee52
fix: replace assert to if-return. Less probability to get troubles in…
knst May 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5462,6 +5462,34 @@ 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));
if (!m_sporkman.IsSporkValid(spork)) {
Misbehaving(pfrom.GetId(), 100, strprintf("invalid spork received. peer=%d", pfrom.GetId()));
return;
}
if (m_sporkman.ProcessSpork(spork)) {
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));
}
}
Comment thread
knst marked this conversation as resolved.
return;
}

if (msg_type == NetMsgType::QUORUMROTATIONINFO) {
// we have never requested this
Misbehaving(pfrom.GetId(), 100, strprintf("received not-requested quorumrotationinfo. peer=%d", pfrom.GetId()));
Expand Down Expand Up @@ -5508,7 +5536,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());
Expand Down
65 changes: 22 additions & 43 deletions src/spork.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -126,51 +123,39 @@ void CSporkManager::CheckAndRemove()
}
}

MessageProcessingResult CSporkManager::ProcessMessage(CNode& peer, CConnman& connman, std::string_view msg_type, CDataStream& vRecv)
bool CSporkManager::IsSporkValid(const CSporkMessage& spork) const
{
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)
{
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 false;
}

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 false;
}
return true;
}

bool CSporkManager::ProcessSpork(const CSporkMessage& spork)
{
uint256 hash = spork.GetHash();
std::string strLogMsg{strprintf("SPORK -- hash: %s id: %d value: %10d", hash.ToString(), spork.nSporkID,
spork.nValue)};

std::string strLogMsg{strprintf("SPORK -- hash: %s id: %d value: %10d peer=%d", hash.ToString(), spork.nSporkID,
spork.nValue, from)};
auto keyIDSigner = *opt_keyIDSigner;
if (!spork.GetSignerKeyID().has_value()) return false;

auto keyIDSigner = spork.GetSignerKeyID().value();
Comment on lines +142 to +150
Copy link
Copy Markdown

@thepastaclaw thepastaclaw May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Undocumented precondition that IsSporkValid was called first

ProcessSpork asserts spork.GetSignerKeyID().has_value() and immediately dereferences the optional. The only thing guaranteeing the optional is engaged is that callers must run IsSporkValid first (it rejects empty signers). This precondition is not stated in the spork.h doc-comment for ProcessSpork, which makes the API easy to misuse from future call sites (e.g. RPC paths or tests). Either document the requirement on the header declaration or fold the validity check into ProcessSpork itself.

source: ['claude']

Comment on lines +148 to +150
Copy link
Copy Markdown

@thepastaclaw thepastaclaw May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: ProcessSpork() invokes GetSignerKeyID() twice

CSporkMessage::GetSignerKeyID() performs a CPubKey::RecoverCompact on the signature each call and is not cached on the message. ProcessSpork() now calls it twice in succession (line 148 for the has_value() check, line 150 for value()), and the prior IsSporkValid() in the caller has already done it once — three recoveries per SPORK message on the happy path. Sporks are infrequent so the practical impact is negligible, but caching the optional locally avoids the redundant ECDSA recovery.

💡 Suggested change
Suggested change
if (!spork.GetSignerKeyID().has_value()) return false;
auto keyIDSigner = spork.GetSignerKeyID().value();
auto opt_keyIDSigner = spork.GetSignerKeyID();
if (!opt_keyIDSigner.has_value()) return false;
auto keyIDSigner = *opt_keyIDSigner;

source: ['claude']


Comment thread
coderabbitai[bot] marked this conversation as resolved.
{
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);
}
Expand All @@ -191,21 +176,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());
Expand Down
25 changes: 9 additions & 16 deletions src/spork.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -249,26 +244,24 @@ class CSporkManager : public SporkStore
void CheckAndRemove() EXCLUSIVE_LOCKS_REQUIRED(!cs);

/**
* ProcessMessage is used to call ProcessSpork and ProcessGetSporks. See below
* IsSporkValid validate signed time and pubkey
* If these values mismatch function returns false [spork is invalid]
* If spork validation failed, peer should be punished
*/
[[nodiscard]] MessageProcessingResult ProcessMessage(CNode& peer, CConnman& connman, std::string_view msg_type,
CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache);

[[nodiscard]] bool IsSporkValid(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.
*/
[[nodiscard]] MessageProcessingResult ProcessSpork(NodeId from, CDataStream& vRecv)
[[nodiscard]] bool ProcessSpork(const CSporkMessage& spork)
EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache);
Comment on lines 252 to 259
Copy link
Copy Markdown

@thepastaclaw thepastaclaw May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: ProcessSpork() has an undocumented validity precondition after the split

The refactor split spork handling into IsSporkValid() and ProcessSpork(), but ProcessSpork() now asserts spork.GetSignerKeyID().has_value() at src/spork.cpp:147 and unconditionally dereferences the optional on the next line. That precondition is only satisfied if IsSporkValid() ran first and returned true. The single current caller in net_processing.cpp does this correctly, but two things make the new API a footgun for future use: (1) the Doxygen comment at src/spork.h:255 still claims "it validates the spork and adds it to the internal spork storage", which is no longer true; (2) since the stated goal of the refactor is to make spork management reusable outside the networking layer, a future caller is likely to copy this pattern incorrectly and either crash in debug builds or throw std::bad_optional_access in release. Either fold the validity check back into ProcessSpork(), update the comment to state the precondition explicitly, or change the signature to take a validated wrapper type so the unsafe call is unrepresentable.

source: ['codex']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/spork.h`:
- [SUGGESTION] lines 252-259: ProcessSpork() has an undocumented validity precondition after the split
  The refactor split spork handling into IsSporkValid() and ProcessSpork(), but ProcessSpork() now asserts spork.GetSignerKeyID().has_value() at src/spork.cpp:147 and unconditionally dereferences the optional on the next line. That precondition is only satisfied if IsSporkValid() ran first and returned true. The single current caller in net_processing.cpp does this correctly, but two things make the new API a footgun for future use: (1) the Doxygen comment at src/spork.h:255 still claims "it validates the spork and adds it to the internal spork storage", which is no longer true; (2) since the stated goal of the refactor is to make spork management reusable outside the networking layer, a future caller is likely to copy this pattern incorrectly and either crash in debug builds or throw std::bad_optional_access in release. Either fold the validity check back into ProcessSpork(), update the comment to state the precondition explicitly, or change the signature to take a validated wrapper type so the unsafe call is unrepresentable.

Comment on lines 252 to 259
Copy link
Copy Markdown

@thepastaclaw thepastaclaw May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: ProcessSpork() doc comment is stale and hides an unsafe precondition

The Doxygen comment claims ProcessSpork() "validates the spork and adds it to the internal spork storage," but after the refactor validation (timestamp window + signer-pubkey membership in setSporkPubKeyIDs) lives entirely in IsSporkValid(). ProcessSpork() now only checks GetSignerKeyID().has_value() (src/spork.cpp:148) before storing. The single current caller in net_processing.cpp:5472 invokes IsSporkValid() first, but since the stated purpose of this PR is to make CSporkManager reusable outside the networking layer, leaving this implicit precondition undocumented turns the new public API into a footgun: a future caller following the doc could store a spork with a future nTimeSigned or one signed by a non-spork key. Either fold the IsSporkValid() check back into ProcessSpork(), or update the comment to state explicitly that callers must call IsSporkValid() first.

💡 Suggested change
Suggested change
/**
* 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.
*/
[[nodiscard]] MessageProcessingResult ProcessSpork(NodeId from, CDataStream& vRecv)
[[nodiscard]] bool ProcessSpork(const CSporkMessage& spork)
EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache);
/**
* ProcessSpork stores a spork that has already been validated by IsSporkValid().
*
* Callers MUST invoke IsSporkValid(spork) first; ProcessSpork() trusts its
* input apart from a defensive check that the signer key can be recovered.
* Returns false for duplicate/older sporks and for messages whose signer
* cannot be recovered.
*/
[[nodiscard]] bool ProcessSpork(const CSporkMessage& spork)
EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache);

source: ['claude', 'codex']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/spork.h`:
- [SUGGESTION] lines 252-259: ProcessSpork() doc comment is stale and hides an unsafe precondition
  The Doxygen comment claims ProcessSpork() "validates the spork and adds it to the internal spork storage," but after the refactor validation (timestamp window + signer-pubkey membership in setSporkPubKeyIDs) lives entirely in IsSporkValid(). ProcessSpork() now only checks `GetSignerKeyID().has_value()` (src/spork.cpp:148) before storing. The single current caller in net_processing.cpp:5472 invokes IsSporkValid() first, but since the stated purpose of this PR is to make CSporkManager reusable outside the networking layer, leaving this implicit precondition undocumented turns the new public API into a footgun: a future caller following the doc could store a spork with a future nTimeSigned or one signed by a non-spork key. Either fold the IsSporkValid() check back into ProcessSpork(), or update the comment to state explicitly that callers must call IsSporkValid() first.


/**
* ProcessGetSporks is used to handle the 'getsporks' p2p message.
*
* For 'getsporks', it sends active sporks to the requesting peer.
* ActiveSporks is used to handle 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
Expand Down
1 change: 0 additions & 1 deletion test/lint/lint-circular-dependencies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading