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

Merged
PastaPastaPasta merged 5 commits into from
May 12, 2026
+63 −65
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 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
4e6f844
refactor: collapse spork signer recovery to one ECDSA op, restore pee…
UdjinM6 May 9, 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
30 changes: 29 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,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));
}
}
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 +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());
Expand Down
61 changes: 18 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,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);
}
Expand All @@ -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());
Expand Down
36 changes: 16 additions & 20 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,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);
Comment on lines 253 to 261
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 253 to 261
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 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
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