Repository navigation
fix(net): announce sporks once per new peer - #7918
Conversation
dashpay#7893 reached develop twice: f99212b merged an earlier revision of its commit, which announces sporks inside the CanRelay() block, and 72a5997 merged the final revision, which announces them after that block so masternode connections are covered too. Both loops survived, so on test chains every relay-capable peer had each active spork inv queued twice at VERACK. Drop the earlier copy. The handler now matches the final revision (45acdf3). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe test-chain active-spork announcement now runs after the Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The description clearly explains the duplicate spork announcements, the eight-line deletion, the affected test-chain behavior, and the reported validation. |
| Title check | |
The title clearly and concisely identifies the main change: announcing sporks once per new peer. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files.
✨ Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
✅ Final review complete — no blockers (commit ac1bc5d) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified ac1bc5d: the eight-line deletion removes redundant spork queueing while preserving test-chain gating, masternode coverage, and repeated-VERACK protection; no in-scope defects were found. The existing inventory-send path filters duplicates, so the PR description overstates the wire-level effect, but the cleanup remains correct. Validation was static only: the supplied CI snapshot reports an ASan build failure and several pending jobs, and the failed-log request confirmed logs remain unavailable while the workflow is running.
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff deletes one duplicate spork announcement loop while preserving the existing announcement path, making this a small, contained behavior fix whose correctness is easy to confirm. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
thepastaclaw
removed
the
pastaclaw:approved
Issue being fixed or feature implemented
developthrough two merge commits: f99212b carried an earlier revision of the PR's commit (ce24c40) and 72a5997 later carried the final revision (45acdf3). Git merged both without conflict, so the VERACK handler insrc/net_processing.cppnow announces sporks twice: once inside theif (pfrom.CanRelay())block (earlier revision) and once right after that block (final revision, which also covers masternode connections that only become relay-capable after MNAUTH).vInventoryOtherToSendper connection.SendMessageschecks and updates the known-inventory filter for each queued inv, so the second copy is dropped before it is sent. The wire output is unchanged, and the cost is one extra queue entry and filter lookup per spork. This is a cleanup of dead duplicate code with no correctness or consensus effect. Mainnet is untouched because both copies are gated onIsTestChain().What was done?
Removed the earlier copy of the spork announcement loop (and its comment) from inside the
if (pfrom.CanRelay())block, so that block again ends right after theSENDCMPCTpush. The copy after the block is kept. The VERACK handler now matches the final revision of #7893 (45acdf3).How Has This Been Tested?
The diff is a pure deletion (8 lines) that restores the final revision of #7893. I compared the VERACK handler region (from the SENDHEADERS comment through
if (!RejectIncomingTxs(pfrom))) againstgit show 45acdf30f3:src/net_processing.cppand it is identical.Built on macOS (Apple silicon,
--enable-debug --enable-werror --disable-hardening --enable-suppress-external-warnings --disable-bench) and ran:test/functional/test_runner.py feature_sporks.py p2p_instantsend.py: both passed../src/test/test_dash --run_test=governance_vote_sync_tests: 6 test cases, no errors detected.No new test was added: the duplicate queue entry is filtered before sending, so it has no observable effect on the wire or on peers.
Breaking Changes
None.
Checklist:
🤖 Generated with Claude Code