Skip to content

fix: instantsend processing in background thread#293

Open
HashEngineering wants to merge 2 commits intomasterfrom
fix/instantsend-processing
Open

fix: instantsend processing in background thread#293
HashEngineering wants to merge 2 commits intomasterfrom
fix/instantsend-processing

Conversation

@HashEngineering
Copy link
Collaborator

@HashEngineering HashEngineering commented Mar 6, 2026

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

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

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • Refactor

    • Streamlined transaction lock processing by removing legacy thread-based execution in favor of a configurable threading model.
    • Improved exception handling for blockchain store operations with enhanced error recovery.
  • Chores

    • Updated test configuration references for build consistency.

@HashEngineering HashEngineering self-assigned this Mar 6, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 6, 2026

📝 Walkthrough

Walkthrough

This change introduces a non-threaded execution mode for InstantSendManager by adding a runWithoutThread boolean flag, removes the deprecated thread-based lifecycle methods, improves null-safety in transaction confidence handling, refines exception handling between RuntimeException and BlockStoreException, and updates constructor signatures accordingly.

Changes

Cohort / File(s) Summary
Test Configuration
.run/Tests in 'dashj-master-three.core.test'.run.xml
Updates Gradle test task reference from :core:test to :dashj-core:test and adds RunAsTest configuration element.
InstantSendManager Constructor Update
core/src/main/java/org/bitcoinj/manager/DashSystem.java
Updates InstantSendManager instantiation to pass new boolean parameter (false) to constructor.
InstantSendManager Core Refactoring
core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java
Introduces runWithoutThread flag to replace thread-based execution model. Adds masternodeListManager dependency injection. Makes wallets field final. Removes worker thread lifecycle methods (start/stop/interrupt). Improves null-safety for transaction confidence. Refines exception handling and pending lock processing logic using removeIf for efficiency.
LLMQBackgroundThread Thread Handling
core/src/main/java/org/bitcoinj/quorums/LLMQBackgroundThread.java
Adds explicit thread naming. Updates exception handling to catch RuntimeException and conditionally log BlockStoreException vs general errors. Adds explicit thread re-interrupt on InterruptedException.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 A thread unwinds, a flag takes flight,
No more workers toil through the night,
Safer null-checks guard the way,
Exception handling's here to stay!
Dependencies dance, constructors align,
Non-threaded dreams in code now shine! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 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 (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: refactoring InstantSend processing to work in a background thread, with support for running without threads when needed.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/instantsend-processing

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java (1)

434-445: ⚠️ Potential issue | 🟠 Major

Logic error: && short-circuits despite comment saying "Don't short circuit".

On line 441, the comment explicitly states "Don't short circuit. Try to process deterministic and not deterministic islocks", but using && will short-circuit if the first call returns false, preventing the second call from executing.

🐛 Proposed fix to prevent short-circuit evaluation
         } else {
             // Don't short circuit. Try to process deterministic and not deterministic islocks
-            return processPendingInstantSendLocks(false) && processPendingInstantSendLocks(true);
+            boolean nonDeterministic = processPendingInstantSendLocks(false);
+            boolean deterministic = processPendingInstantSendLocks(true);
+            return nonDeterministic || deterministic;
         }

Alternatively, use bitwise OR | which doesn't short-circuit: return processPendingInstantSendLocks(false) | processPendingInstantSendLocks(true);

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java` around lines
434 - 445, The method InstantSendManager.processPendingInstantSendLocks()
currently uses logical AND which short-circuits and can prevent the second call
to processPendingInstantSendLocks(true) from running despite the comment; change
the control flow so both calls always execute (e.g., call
processPendingInstantSendLocks(false) and processPendingInstantSendLocks(true)
unconditionally and combine their results non-short-circuitingly) so
deterministic and non-deterministic islocks are both processed; update the
return to reflect the combined boolean result while keeping the existing
isInitialized() and DIP0024Active check logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.run/Tests in 'dashj-master-three.core.test'.run.xml:
- Line 13: The Gradle task path is using the project's display name
':dashj-core:test' which Gradle won't resolve; update the option value to use
the actual project path ':core:test' (as defined by project(':core').name =
'dashj-core' in settings.gradle) so the task reference points to the correct
subproject task; locate the option element with value=":dashj-core:test" and
replace it with the project path ':core:test'.
- Line 21: The test run configuration has RunAsTest set to false which disables
IDE test runner integration for the "Tests in..." configuration; change the
RunAsTest value to true so IntelliJ recognizes test results, shows the test
tree, and reports pass/fail status (update the RunAsTest element in the "Tests
in..." run configuration XML accordingly).

---

Outside diff comments:
In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java`:
- Around line 434-445: The method
InstantSendManager.processPendingInstantSendLocks() currently uses logical AND
which short-circuits and can prevent the second call to
processPendingInstantSendLocks(true) from running despite the comment; change
the control flow so both calls always execute (e.g., call
processPendingInstantSendLocks(false) and processPendingInstantSendLocks(true)
unconditionally and combine their results non-short-circuitingly) so
deterministic and non-deterministic islocks are both processed; update the
return to reflect the combined boolean result while keeping the existing
isInitialized() and DIP0024Active check logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 547524d8-813d-437c-aad8-89c70afa76ff

📥 Commits

Reviewing files that changed from the base of the PR and between 3777b7c and 98de6c4.

📒 Files selected for processing (4)
  • .run/Tests in 'dashj-master-three.core.test'.run.xml
  • core/src/main/java/org/bitcoinj/manager/DashSystem.java
  • core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java
  • core/src/main/java/org/bitcoinj/quorums/LLMQBackgroundThread.java

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.

1 participant