Skip to content

fix: restore the intended cs_main scope in FinalizeNode - #7653

Open
UdjinM6 wants to merge 1 commit into
dashpay:developfrom
UdjinM6:fix-finalizenode-cs-main-scope
Open

fix: restore the intended cs_main scope in FinalizeNode#7653
UdjinM6 wants to merge 1 commit into
dashpay:developfrom
UdjinM6:fix-finalizenode-cs-main-scope

Conversation

@UdjinM6

@UdjinM6 UdjinM6 commented Sep 2, 2026

Copy link
Copy Markdown

Issue being fixed or feature implemented

40906b2 ("partial bitcoin#20228: Make addrman a top-level component", PR #5163) transposed two lines when backporting upstream 3fc06d3. Upstream opens a scope and takes cs_main inside it:

int misbehavior{0};
{
LOCK(cs_main);
{

while the backport landed the LOCK before the scope:

int misbehavior{0};
LOCK(cs_main);
{
{

The braces still balance, so this compiled and went unnoticed, but cs_main ended up held for the whole function instead of just that block. The } // cs_main marker has been wrong ever since.

It stayed harmless because the only code below the marker is CAddrMan::Connected() and a LogPrint, neither of which contends for anything meaningful. It stops being harmless as soon as someone adds work below the marker that takes a contended lock, since the comment says they are outside cs_main when they are not.

What was done?

Restore the upstream shape. No behavior change: nothing below the marker requires cs_main, and misbehavior and nodeid are declared above the scope. Discovered while reviewing #7563.

How Has This Been Tested?

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)

40906b2 ("partial bitcoin#20228: Make addrman a top-level component",
PR dashpay#5163) transposed two lines when backporting upstream 3fc06d3. Upstream
opens a scope and takes cs_main inside it:

    int misbehavior{0};
    {
    LOCK(cs_main);
    {

while the backport landed the LOCK before the scope:

    int misbehavior{0};
    LOCK(cs_main);
    {
    {

The braces still balance, so this compiled and went unnoticed, but cs_main
ended up held for the whole function instead of just that block. The
`} // cs_main` marker has been wrong ever since.

It stayed harmless because the only code below the marker is
CAddrMan::Connected() and a LogPrint, neither of which contends for anything
meaningful. It stops being harmless as soon as someone adds work below the
marker that takes a contended lock, since the comment says they are outside
cs_main when they are not.

Restore the upstream shape. No behavior change: nothing below the marker
requires cs_main, and misbehavior and nodeid are declared above the scope.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@UdjinM6 UdjinM6 added this to the 24 milestone Sep 2, 2026
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

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: Team

Run ID: 757af183-f2e6-4984-bafd-6a63fba13a69

📥 Commits

Reviewing files that changed from the base of the PR and between 6f3eacf and 202385c.

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

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

FinalizeNode now scopes cs_main to peer removal and node-state cleanup. The lock is released before addrman updates and logging.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 20238

This restores the intended cs_main lock scope in FinalizeNode without changing intended behavior; the localized change is merge-ready after normal checks and review, with no actionable merge-blocking risk remaining.

🚥 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the incorrect cs_main scope in FinalizeNode, the cause of the issue, and the intended fix.
Title check ✅ Passed The title clearly and concisely identifies the primary change: restoring the intended cs_main scope in FinalizeNode.
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.
  • Fix all pre-merge checks with AI
✨ 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.

@thepastaclaw

thepastaclaw commented Sep 2, 2026

Copy link
Copy Markdown

🕓 Ready for review — 27 ahead in queue (commit 202385c)
Queue position: 28/53 · 2 reviews active
ETA: start ~04:47 UTC · complete ~05:41 UTC (median 54m across 30 recent reviews; 2 slots)
Queued 1d 17h ago · Last checked: 2026-09-04 16:50 UTC

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants