refactor: drop dependency of spork on net_processing

[knst]

Issue being fixed or feature implemented

Dependency of Spork on PeerManager (net_processing) is a blocker for kernel / chainstate project.

What was done?

Removed dependency of Spork on PeerManager by moving network code to rpc/node

How Has This Been Tested?

Run test/lint/lint-circular-dependencies.py

Breaking Changes

N/A

Checklist:

• 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
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

• src/rpc/node.cpp: Adds include net_processing.h. Calls CSporkManager::UpdateSpork(SporkId, int64_t) which now returns std::optional; if an inv is returned, obtains the PeerManager and calls RelayInv to broadcast; otherwise no relay and RPC returns null.
• src/spork.cpp: Changes CSporkManager::UpdateSpork to return std::optional and remove the PeerManager parameter; returns a CInv when a spork message should be relayed or std::nullopt otherwise. Removes CSporkMessage::Relay(PeerManager&) and adjusts locking/return paths.
• src/spork.h: Removes PeerManager forward declaration and CSporkMessage::Relay declaration; updates CSporkManager::UpdateSpork signature and comments to return std::optional.
• test/lint/lint-circular-dependencies.py: Removes the expected circular dependency entry "net_processing -> spork -> net_processing".

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly and accurately summarizes the primary change of removing the Spork component's dependency on net_processing, clearly reflecting the main refactoring objective without unnecessary detail.
Description Check	✅ Passed	The description clearly outlines the issue being addressed, the refactoring performed, the testing approach, and the checklist status, all of which directly relate to the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f170aed and 95de8a0734ce45d8e9860059900b67e63976637b.

📒 Files selected for processing (4)

• src/rpc/node.cpp (2 hunks)
• src/spork.cpp (1 hunks)
• src/spork.h (1 hunks)
• test/lint/lint-circular-dependencies.py (0 hunks)

💤 Files with no reviewable changes (1)

• test/lint/lint-circular-dependencies.py

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

• src/spork.cpp
• src/spork.h
• src/rpc/node.cpp

🔇 Additional comments (3)

src/rpc/node.cpp (2)

22-22: LGTM! Include necessary for refactored relay pattern.

The addition of net_processing.h is required to access PeerManager and EnsurePeerman for the updated relay logic in sporkupdate.

---

229-236: LGTM! Relay logic correctly moved to RPC layer.

The updated logic properly handles the new API:

1. Calls UpdateSpork which returns std::optional<CInv>
2. Retrieves PeerManager only when relay is needed
3. Returns "success" on successful relay, NullUniValue on failure

This successfully decouples Spork from PeerManager by moving network relay concerns to the RPC layer.

src/spork.cpp (1)

208-238: LGTM! Refactor successfully removes PeerManager dependency.

The implementation correctly:

1. Removes PeerManager parameter from signature
2. Returns std::optional<CInv> for external relay instead of bool
3. Returns std::nullopt on signing failure or invalid keyID
4. Maintains proper locking with cs acquired first, then cs_cache
5. Creates and returns inventory message for the caller to relay

The nested locking pattern is safe as indicated by the EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache) annotation.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/spork.h (1)

274-278: Documentation typo persists.

The grammatical error flagged in a previous review remains: "It return nullopt" should be "It returns nullopt" or "Returns std::nullopt".

Apply this diff to fix the typo:

 /**
  * UpdateSpork is used by the spork RPC command to set a new spork value, sign
  * and return the spork message, ready for network relay.
- * It returns nullopt if nothing to relay
+ * It returns std::nullopt if nothing to relay
  */
 std::optional<CInv> UpdateSpork(SporkId nSporkID, SporkValue nValue) EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache);

🧹 Nitpick comments (1)

src/spork.cpp (1)

231-233: Consider using nested scope for cache lock.

The LOCK(cs_cache) on line 231 is held until the function returns. While technically correct due to RAII, this pattern is inconsistent with the rest of the codebase. Compare to ProcessSpork (lines 186-190), which uses an explicit scope block.

Apply this diff for consistency:

 mapSporksActive[nSporkID][*opt_keyIDSigner] = spork;
 // Clear cached values on new spork being processed

-LOCK(cs_cache);
-mapSporksCachedActive.erase(spork.nSporkID);
-mapSporksCachedValues.erase(spork.nSporkID);
-
+{
+    LOCK(cs_cache);
+    mapSporksCachedActive.erase(spork.nSporkID);
+    mapSporksCachedValues.erase(spork.nSporkID);
+}

 CInv inv(MSG_SPORK, spork.GetHash());
 return inv;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95de8a0734ce45d8e9860059900b67e63976637b and f0a2cec.

📒 Files selected for processing (4)

• src/rpc/node.cpp (2 hunks)
• src/spork.cpp (1 hunks)
• src/spork.h (1 hunks)
• test/lint/lint-circular-dependencies.py (0 hunks)

💤 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/rpc/node.cpp

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

• src/spork.h
• src/spork.cpp

🔇 Additional comments (1)

src/spork.cpp (1)

208-238: LGTM! Successful removal of PeerManager dependency.

The refactoring correctly removes the PeerManager dependency by:

• Eliminating the peerman parameter
• Returning std::optional<CInv> instead of bool
• Delegating relay responsibility to the caller

The logic is sound with proper early returns on failures and correct spork creation/storage.

[UdjinM6]

utACK f0a2cec

[kwvg]

utACK f0a2cec

[PastaPastaPasta]

utACK f0a2cec