Skip to content

fix: remove unnecessary lock in bad sources processing#6979

Merged
PastaPastaPasta merged 1 commit intodashpay:developfrom
PastaPastaPasta:fix/dumb-cs-main
Nov 18, 2025
Merged

fix: remove unnecessary lock in bad sources processing#6979
PastaPastaPasta merged 1 commit intodashpay:developfrom
PastaPastaPasta:fix/dumb-cs-main

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Nov 15, 2025

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • 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)

@PastaPastaPasta PastaPastaPasta added this to the 23.1 milestone Nov 15, 2025
@github-actions
Copy link

github-actions bot commented Nov 15, 2025 •
edited
Loading

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Nov 15, 2025 •
edited
Loading

Walkthrough

This change removes the acquisition of the global cs_main lock in an error handling code path within ProcessPendingInstantSendLocks. Previously, when processing bad sources from batch verification, the code would lock cs_main before iterating over batchVerifier.badSources to apply peer misbehavior penalties. The updated code performs this iteration and penalty application without acquiring the lock.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Areas requiring attention during review:
    • Verify that no data races are introduced by removing the cs_main lock acquisition on the bad sources iteration path
    • Confirm that PeerMisbehaving calls and operations on batchVerifier.badSources are safe to execute without cs_main protection
    • Check for consistency with other code paths that handle peer misbehavior to ensure uniform locking semantics

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description provided contains only checklist template placeholders without substantive content about the changes, testing approach, or implementation details related to the lock removal. Add meaningful content to the description sections explaining what issue is being fixed, what changes were made, how the changes were tested, and any relevant breaking changes or side effects.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: remove unnecessary lock in bad sources processing' is directly related to and accurately summarizes the main change in the changeset, which removes an unnecessary cs_main lock acquisition in the bad sources handling path.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb1dfa3bfa2ca03973c7bb70222f557d62146731 and 23fc177.

📒 Files selected for processing (1)
  • src/instantsend/net_instantsend.cpp (0 hunks)
💤 Files with no reviewable changes (1)
  • src/instantsend/net_instantsend.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: arm-linux-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: Lint / Run linters

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

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.

@github-actions github-actions bot mentioned this pull request Nov 15, 2025
Merged
5 tasks
knst
knst previously approved these changes Nov 15, 2025
View reviewed changes
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

LGTM cb1dfa3bfa2ca03973c7bb70222f557d62146731

consider #6959 to get merged first

@github-actions
Copy link

github-actions bot commented Nov 18, 2025

This pull request has conflicts, please rebase.

knst
knst approved these changes Nov 18, 2025
View reviewed changes
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK 23fc177

UdjinM6
UdjinM6 approved these changes Nov 18, 2025
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK 23fc177

@PastaPastaPasta PastaPastaPasta merged commit 97bd9fa into dashpay:develop Nov 18, 2025
28 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knst knst knst approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

23.1

Development

Successfully merging this pull request may close these issues.

3 participants

@PastaPastaPasta @knst @UdjinM6