fix: ignore repeated getsporks requests - #7343
Conversation
|
@coderabbitai review |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
✅ Action performedReview finished.
|
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
this seems reasonable to me; utACK 7c799761c80509e2293af700a6cf864cf6d5d263
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 59ac8d351908710f4e96213e53b0f375bd596021 and 60efd84. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughPer-peer GETSPORKS state is tracked and GETSPORKS responses are suppressed when the currently-active spork-hash set equals the last response sent to that connection. The change adds an include, two Peer fields (m_getsporks_recvd and m_getsporks_last_response), and updates the GETSPORKS handler to compute a sorted active-spork-hash list, compare it to the peer's cached list, early-return when identical, and refresh the cache when sending. A functional test sends GETSPORKS twice on one connection and asserts only one spork response was delivered; changing a spork causes a refreshed response. Sequence Diagram(s)sequenceDiagram
participant PeerConn as Peer
participant Handler as GETSPORKS_Handler
participant SporkSet as ActiveSporkSet
participant SporkSender as Spork_Response
PeerConn->>Handler: GETSPORKS
Handler->>SporkSet: Build sorted active spork hashes
Handler->>PeerConn: Read m_getsporks_recvd & m_getsporks_last_response
alt active set == last_response && m_getsporks_recvd == true
Handler-->>PeerConn: Log + early return (no spork messages)
else
Handler->>PeerConn: Update m_getsporks_recvd = true and m_getsporks_last_response
Handler->>SporkSender: Send active spork messages
SporkSender-->>PeerConn: spork messages
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
7c79976 to
06de524
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
06de524 to
b965f90
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
b965f90 to
59ac8d3
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/functional/feature_sporks.py`:
- Around line 91-96: The test records peer.message_count["spork"] too early
because wait_until only waits for the first spork; call peer.sync_with_ping()
immediately after wait_until (before reading spork_responses) to ensure all
spork messages from the first msg_getsporks() have been delivered; update the
block around peer.wait_until(...), peer.sync_with_ping(), and spork_responses so
spork_responses = peer.message_count["spork"] happens only after the
sync_with_ping() completes.
🪄 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: 5a4f030e-5dfd-401c-9e91-49f0b73dbdc8
📥 Commits
Reviewing files that changed from the base of the PR and between b965f90834ad71a7f76583819b3b1b38a529e004 and 59ac8d351908710f4e96213e53b0f375bd596021.
📒 Files selected for processing (2)
src/net_processing.cpptest/functional/feature_sporks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/net_processing.cpp
|
✅ Review complete (commit 60efd84) |
|
Addressed CodeRabbit race concern in the functional test by syncing after the initial getsporks response before recording the baseline count. Validation: |
|
@coderabbitai review |
59ac8d3 to
60efd84
Compare
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the per-peer getsporks ignore logic in src/net_processing.cpp:5454-5481 and the functional test in test/functional/feature_sporks.py. The implementation correctly suppresses duplicate responses while still re-answering when the active spork hash set changes, and the test exercises both paths. Claude's three findings are all low-confidence nitpicks/meta observations that don't merit public review noise; Codex correctly found nothing to flag.
Note: GitHub does not allow the review bot account to approve its own PR, so this clean verification is posted as a comment review instead of an approval.
|
@thepastaclaw I'm not fully sure this is worth it. Please review to confirm this makes sense to do holistically. provide justification which ever way you decide |
|
Reviewed this holistically and I still think it is worth keeping, with the caveat that it is a small hardening/cleanup rather than an urgent bug fix. Why I think it makes sense:
Why it is not a high-priority fix:
My recommendation is to keep/merge it as a modest DoS-surface cleanup because it aligns with existing peer-request hygiene without breaking legitimate refreshed |
Backport of dashpay#7438 (upstream merge 4537f37, cherry-picked with -m1). v23.1.x adaptations: (1) on this branch spork messages are deserialized in CSporkManager::ProcessSpork (src/spork.cpp), not in net_processing's SPORK handler as on develop - the same try/catch is applied there, reporting the failure as MisbehavingError{100, ...} through the existing MessageProcessingResult path (equivalent to develop's Misbehaving(*peer, 100, ...)). (2) The functional test's msg_spork_raw/SporkP2PInterface helpers come from dashpay#7343 (commit 60efd84), which is not on this branch; they are included verbatim, and the new test registers MESSAGEMAP[b"spork"] itself since dashpay#7343's test method (which did the registration on develop) is absent here. The spork.h LIMITED_VECTOR bound is unchanged from upstream. (cherry picked from commit 4537f37d51d5b62be3aca4bfc9e40404db76ffc9)
Backport of dashpay#7438 (upstream merge 4537f37, cherry-picked with -m1). v23.1.x adaptations: (1) on this branch spork messages are deserialized in CSporkManager::ProcessSpork (src/spork.cpp), not in net_processing's SPORK handler as on develop - the same try/catch is applied there, reporting the failure as MisbehavingError{100, ...} through the existing MessageProcessingResult path (equivalent to develop's Misbehaving(*peer, 100, ...)). (2) The functional test's msg_spork_raw/SporkP2PInterface helpers come from dashpay#7343 (commit 60efd84), which is not on this branch; they are included verbatim, and the new test registers MESSAGEMAP[b"spork"] itself since dashpay#7343's test method (which did the registration on develop) is absent here. The spork.h LIMITED_VECTOR bound is unchanged from upstream. (cherry picked from commit 4537f37d51d5b62be3aca4bfc9e40404db76ffc9)
Backport of dashpay#7438 (upstream merge 4537f37, cherry-picked with -m1). v23.1.x adaptations: (1) on this branch spork messages are deserialized in CSporkManager::ProcessSpork (src/spork.cpp), not in net_processing's SPORK handler as on develop - the same try/catch is applied there, reporting the failure as MisbehavingError{100, ...} through the existing MessageProcessingResult path (equivalent to develop's Misbehaving(*peer, 100, ...)). (2) The functional test's msg_spork_raw/SporkP2PInterface helpers come from dashpay#7343 (commit 60efd84), which is not on this branch; they are included verbatim, and the new test registers MESSAGEMAP[b"spork"] itself since dashpay#7343's test method (which did the registration on develop) is absent here. The spork.h LIMITED_VECTOR bound is unchanged from upstream. (cherry picked from commit 4537f37d51d5b62be3aca4bfc9e40404db76ffc9)
3 participants
Pull request
Issue being fixed or feature implemented
getsporkscurrently responds to every request from a connected peer bysnapshotting active sporks and sending each stored spork message back. The active
spork set is small, so this is low impact, but repeated requests are unnecessary
resource use and keep being flagged by security-audit tooling.
This PR makes
getsporksfollow the existinggetaddrpattern: answer once perconnection and ignore repeats.
What was done?
m_getsporks_recvdstate.getsporksrequests on the same connection after the firstresponse.
feature_sporks.pythat sendsgetsporkstwice and verifies only the first request returns spork messages.How Has This Been Tested?
Tested on macOS arm64 using the existing depends prefix.
git diff --checkpython3 -m py_compile test/functional/feature_sporks.pymake -j8 src/dashdtest/functional/feature_sporks.py --tmpdir=/Users/claw/.openclaw/workspace/tmp-dash-test-feature-sporks-rerun-79105The focused functional test passed successfully.
Breaking Changes
None.
Checklist