Skip to content

fix: remove cs_main from MaybePunishNodeForTx, missing changes from #19607#7332

Open
knst wants to merge 1 commit into
dashpay:developfrom
knst:fix-19607
Open

fix: remove cs_main from MaybePunishNodeForTx, missing changes from #19607#7332
knst wants to merge 1 commit into
dashpay:developfrom
knst:fix-19607

Conversation

@knst
Copy link
Copy Markdown
Collaborator

@knst knst commented May 20, 2026

Issue being fixed or feature implemented

Helper MaybePunishNodeForTx has been introduced when bitcoin#19607 is done, it caused missing changes.

What was done?

Removed cs_main from MaybePunishNodeForTx

How Has This Been Tested?

N/A

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
Copy link
Copy Markdown

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 20, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8485e8e9-9f14-4166-8fd2-3a90237be035

📥 Commits

Reviewing files that changed from the base of the PR and between beca567 and eb769ab.

📒 Files selected for processing (1)
  • src/net_processing.cpp

Walkthrough

This pull request removes a scoped LOCK(cs_main) block from the TX_CONSENSUS error handling path in PeerManagerImpl::MaybePunishNodeForTx. The change simplifies the code by eliminating the explicit lock scope around the Misbehaving(nodeid, 100) call, allowing the function to call and return directly. This suggests either the lock is no longer necessary at this location or is handled internally by the callee.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • UdjinM6
  • PastaPastaPasta
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removing cs_main from MaybePunishNodeForTx as missing changes from #19607, which aligns with the code modifications shown in the raw summary.
Description check ✅ Passed The description is directly related to the changeset, explaining the issue (missing changes from bitcoin#19607), what was done (removed cs_main), and providing context about the helper function.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Small, correct cleanup completing the bitcoin#19607 backport. Removes a stale LOCK(cs_main) around Misbehaving(nodeid, 100) in MaybePunishNodeForTx. Misbehaving() is internally guarded by peer->m_misbehavior_mutex and does not require cs_main; the sibling MaybePunishNodeForBlock already calls it the same way. Both reviewers (Claude and Codex, plus backport specialists) independently reached the same conclusion with no findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants