refactor: drop dependency of spork manager on network code#7303
refactor: drop dependency of spork manager on network code#7303knst wants to merge 4 commits intodashpay:developfrom
Conversation
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.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 6a4ee52) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81e15cd42f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR moves spork handling into explicit SPORK and GETSPORKS branches in PeerManagerImpl::ProcessMessage(). SPORK messages are deserialized to CSporkMessage, the peer's object-download request cleared, and the spork validated via CSporkManager::IsSporkValid() (misbehavior recorded for invalid). Valid sporks are passed to CSporkManager::ProcessSpork(const CSporkMessage&) and, only on success, the SPORK inv is relayed. GETSPORKS requests are answered by iterating CSporkManager::ActiveSporks() and pushing each active SPORK to the peer. The generic post-process routing for sporks was removed. The lint expected circular-dependencies list dropped one entry. Sequence Diagram(s)sequenceDiagram
participant Peer as Remote Peer
participant PM as PeerManagerImpl
participant SM as SporkManager
participant Inv as Inventory/Relay
rect rgba(100, 150, 200, 0.5)
Note over Peer,Inv: SPORK Message Flow (New)
Peer->>PM: Send SPORK
PM->>PM: Deserialize to CSporkMessage
PM->>SM: IsSporkValid(spork)
SM-->>PM: bool (valid/invalid)
alt Validation passes
PM->>SM: ProcessSpork(spork)
SM-->>PM: bool (processed)
alt Process succeeds
PM->>Inv: Relay SPORK inv
end
else Invalid
PM->>Peer: Record misbehavior / drop
end
end
rect rgba(150, 100, 200, 0.5)
Note over Peer,PM: GETSPORKS Message Flow (New)
Peer->>PM: Send GETSPORKS
PM->>SM: ActiveSporks()
SM-->>PM: Map of active sporks
PM->>Peer: Push SPORK messages for each active spork
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/spork.h (1)
262-264: ⚡ Quick winPrefer a message-oriented return type for
ActiveSporks().Returning
std::unordered_map<SporkId, std::map<CKeyID, CSporkMessage>>leaksCSporkManager’s storage layout into the new public API. The network-facing caller only needs the activeCSporkMessages, so a flat collection or visitor would keep this boundary aligned with the decoupling goal of the refactor.♻️ Suggested API shape
- std::unordered_map<SporkId, std::map<CKeyID, CSporkMessage>> ActiveSporks() const EXCLUSIVE_LOCKS_REQUIRED(!cs); + std::vector<CSporkMessage> ActiveSporks() const EXCLUSIVE_LOCKS_REQUIRED(!cs);-std::unordered_map<SporkId, std::map<CKeyID, CSporkMessage>> CSporkManager::ActiveSporks() const +std::vector<CSporkMessage> CSporkManager::ActiveSporks() const { LOCK(cs); // make sure to not lock this together with cs_main - return mapSporksActive; + std::vector<CSporkMessage> out; + for (const auto& [_, by_signer] : mapSporksActive) { + for (const auto& [_, spork] : by_signer) { + out.push_back(spork); + } + } + return out; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spork.h` around lines 262 - 264, ActiveSporks() currently exposes the internal nested container type (std::unordered_map<SporkId, std::map<CKeyID, CSporkMessage>>) — change it to return a message-oriented collection like std::map<SporkId, CSporkMessage> or std::vector<CSporkMessage> (or implement a visitor callback signature) so callers only receive the active CSporkMessage objects; update the declaration of ActiveSporks in src/spork.h and its implementation in CSporkManager to build and return the flattened collection (or invoke the visitor) from the existing internal storage, and adjust all callers to use the new return type (or visitor) rather than relying on CKeyID or the nested storage layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/net_processing.cpp`:
- Around line 5482-5489: The GETSPORKS handler currently streams every entry
from m_sporkman.ActiveSporks() back to the requester via m_connman.PushMessage
which can produce an unbounded large response; modify the GETSPORKS branch to
enforce a strict response budget (either a max message count and/or max byte
count) before calling m_connman.PushMessage for each signerSporkPair: iterate
ActiveSporks() and accumulate a counter and estimated byte size for messages
created with CNetMsgMaker(pfrom.GetCommonVersion()).Make(NetMsgType::SPORK,
...), stop sending when the budget is reached, and if there are remaining
entries fall back to sending an inventory-style response (e.g., advertise
available spork IDs or a single compact summary) rather than streaming the rest;
reference GETSPORKS, m_sporkman.ActiveSporks(), signerSporkPair,
m_connman.PushMessage and pfrom when making the changes.
In `@src/spork.cpp`:
- Around line 142-149: ProcessSpork currently unconditionally dereferences the
optional signer (assert(spork.GetSignerKeyID().has_value()) / value()) which can
abort on malformed input; change CSporkManager::ProcessSpork to validate the
spork signer before use by calling IsSporkValid() or checking
GetSignerKeyID().has_value() and returning false (or rejecting the message) if
missing/invalid, and only then extract keyIDSigner via GetSignerKeyID().value();
update any logging to reflect rejection reason and avoid using assert() for
input validation.
---
Nitpick comments:
In `@src/spork.h`:
- Around line 262-264: ActiveSporks() currently exposes the internal nested
container type (std::unordered_map<SporkId, std::map<CKeyID, CSporkMessage>>) —
change it to return a message-oriented collection like std::map<SporkId,
CSporkMessage> or std::vector<CSporkMessage> (or implement a visitor callback
signature) so callers only receive the active CSporkMessage objects; update the
declaration of ActiveSporks in src/spork.h and its implementation in
CSporkManager to build and return the flattened collection (or invoke the
visitor) from the existing internal storage, and adjust all callers to use the
new return type (or visitor) rather than relying on CKeyID or the nested storage
layout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4da6e749-a439-4668-bc4d-d3f07d77b34c
📒 Files selected for processing (4)
src/net_processing.cppsrc/spork.cppsrc/spork.htest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (1)
- test/lint/lint-circular-dependencies.py
knst
force-pushed
the
refactor-spork-net-processing-v2
branch
from
81e15cd to
7c899a2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/spork.cpp (1)
142-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
ProcessSpork()self-validating.This entry point still assumes prior validation and dereferences signer data via
assert()/.value(). If any non-network caller skipsIsSporkValid(), malformed input can still abort the node or get stored here. Please hardenProcessSpork()itself before extractingkeyIDSigner.Suggested hardening
bool CSporkManager::ProcessSpork(const CSporkMessage& spork) { + if (!IsSporkValid(spork)) return false; + uint256 hash = spork.GetHash(); std::string strLogMsg{strprintf("SPORK -- hash: %s id: %d value: %10d", hash.ToString(), spork.nSporkID, spork.nValue)}; - assert(spork.GetSignerKeyID().has_value()); - auto keyIDSigner = spork.GetSignerKeyID().value(); + const auto opt_keyIDSigner = spork.GetSignerKeyID(); + assert(opt_keyIDSigner.has_value()); + const CKeyID& keyIDSigner = *opt_keyIDSigner;Based on learnings, this repository keeps
assert()enabled in all build types, so this assertion is still production-impacting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spork.cpp` around lines 142 - 149, ProcessSpork currently unconditionally dereferences the optional returned by CSporkMessage::GetSignerKeyID() via assert()/.value(), which can abort the node if a caller skips IsSporkValid(); make ProcessSpork self-validating by checking the optional before use: call spork.GetSignerKeyID() and if it has no value, log an error and return false (or otherwise reject the spork) instead of asserting, and only assign keyIDSigner when the optional is present; also ensure any subsequent code that uses keyIDSigner (e.g., comparisons, map lookups, or calls that assume a valid signer) is guarded by that same presence check so ProcessSpork never dereferences a missing signer even if IsSporkValid() was skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/spork.cpp`:
- Around line 142-149: ProcessSpork currently unconditionally dereferences the
optional returned by CSporkMessage::GetSignerKeyID() via assert()/.value(),
which can abort the node if a caller skips IsSporkValid(); make ProcessSpork
self-validating by checking the optional before use: call spork.GetSignerKeyID()
and if it has no value, log an error and return false (or otherwise reject the
spork) instead of asserting, and only assign keyIDSigner when the optional is
present; also ensure any subsequent code that uses keyIDSigner (e.g.,
comparisons, map lookups, or calls that assume a valid signer) is guarded by
that same presence check so ProcessSpork never dereferences a missing signer
even if IsSporkValid() was skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 02c45efc-2758-49c4-b9c5-1811d627cf4f
📒 Files selected for processing (4)
src/net_processing.cppsrc/spork.cppsrc/spork.htest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (1)
- test/lint/lint-circular-dependencies.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spork.h
|
Followed up on this CodeRabbit finding by opening a small PR against the source branch: It makes Validation:
|
|
Prepared the requested follow-up branch to disable legacy P2P spork handling on mainnet conservatively (no punishment; empty GETSPORKS response; no mainnet MSG_SPORK request/response/relay paths): https://github.com/thepastaclaw/dash/tree/tracker-1145-disable-mainnet-p2p-sporks\n\nCommit: thepastaclaw@3633209d38eaacf0ca42e72ffde5dc201e050ad8\n\nVerification run: |
|
Opened a draft follow-up PR for the mainnet spork P2P review feedback: knst#3 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Refactor cleanly extracts spork P2P handling from CSporkManager into net_processing. One real behavioral regression: already-seen sporks are now relayed where previously they were not. Codex's quorum getdata finding is invalid — this PR does not touch any quorum/RPC code.
Reviewed commit: 81e15cd
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments 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.cpp`:
- [SUGGESTION] lines 154-156: Already-seen sporks are now relayed; previously they were not
Before this refactor, the old `CSporkManager::ProcessSpork` only populated `ret.m_inventory` after the `mapSporksByHash[hash] = spork` write, so the 'seen' branch (`mapSporksActive[id][signer].nTimeSigned >= spork.nTimeSigned`) returned with empty inventory and the caller did not relay anything. The new `ProcessSpork` returns `true` on that same seen path (src/spork.cpp:156), and the new dispatch in `PeerManagerImpl::ProcessMessage` calls `RelayInv(spork_inv)` whenever `ProcessSpork` returns true (src/net_processing.cpp:5476-5478). Net effect: every duplicate spork received from a peer is now re-announced via INV, which previously was treated as a no-op. Inventory tracking on the receiving side limits the practical fallout, but this is an unintended propagation change. Either return `false` for the seen branch or split the API into 'stored vs. should-relay' so the prior semantics are preserved.
| 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)}; | ||
| assert(spork.GetSignerKeyID().has_value()); | ||
| auto keyIDSigner = spork.GetSignerKeyID().value(); |
There was a problem hiding this comment.
💬 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']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-reviewed commit 7c899a2. The duplicate-spork relay regression from the prior review is fixed: the already-seen path now returns false again, so duplicate sporks are no longer re-announced. I did not find any new blocking issues; the remaining ProcessSpork/IsSporkValid API precondition is the same minor thread already left on the previous review, so I am not restating it as a duplicate inline comment here.
Reviewed commit: 7c899a2
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
No blockers from me on the current head; remaining comments are advisory suggestions/nits.
|
pls see be5c18a |
- Use __func__ in IsSporkValid log prefixes (was stale "ProcessSpork") - Mark IsSporkValid as const - Pass reason string to Misbehaving on invalid spork Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Reviewed the current head (7460211; same patch as be5c18a). The new changes look good to me: the stale log prefix is fixed via func, IsSporkValid() can safely be const with the existing mutable lock, and passing a concrete Misbehaving reason preserves the existing rejection path. No new blockers from me.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Refactor cleanly extracts SPORK/GETSPORKS dispatch into PeerManagerImpl, removing spork->net coupling. Behavior is preserved across the validation/erase/relay paths and duplicate relay is correctly suppressed. One suggestion remains around the new split API: ProcessSpork() now relies on a caller-side precondition (IsSporkValid first) that the public interface and header comment do not communicate.
Reviewed commit: 7460211
🟡 1 suggestion(s)
🤖 Prompt for all review comments 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.
| /** | ||
| * 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); |
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69f09d1a20
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/spork.cpp`:
- Around line 148-150: The early-return has the wrong polarity and calls value()
on an empty optional; change the guard in spork.cpp so it returns false when
GetSignerKeyID() is not present (if (!spork.GetSignerKeyID().has_value()) return
false;) and then safely obtain the signer ID (auto keyIDSigner =
spork.GetSignerKeyID().value();) — this restores the original defensive check
around spork.GetSignerKeyID() and prevents std::bad_optional_access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Confirmed the new Codex/CodeRabbit signer-guard report: current head I pushed a minimal one-line fix branch here (no PR opened from this worker): https://github.com/thepastaclaw/dash/tree/tracker-1145-fix-spork-signer-guard Commit: thepastaclaw@182a8d6 Validation: |
|
Opened a draft fix PR for the signer guard issue here: knst#4 |
knst
force-pushed
the
refactor-spork-net-processing-v2
branch
from
69f09d1 to
6a4ee52
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Refactor cleanly extracts spork P2P handling out of CSporkManager into PeerManagerImpl. Two minor issues remain: the ProcessSpork() Doxygen comment is stale (it no longer validates) and ProcessSpork() recomputes GetSignerKeyID() unnecessarily. No blocking issues.
Reviewed commit: 6a4ee52
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments 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.
| /** | ||
| * 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); |
There was a problem hiding this comment.
🟡 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
| /** | |
| * 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.
| if (!spork.GetSignerKeyID().has_value()) return false; | ||
|
|
||
| auto keyIDSigner = spork.GetSignerKeyID().value(); |
There was a problem hiding this comment.
💬 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
| 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']
3 participants
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: