refactor: create InstantSendStorage to break circular dependency over instantsend/signing

[knst]

Issue being fixed or feature implemented

It resolves newly introduced circular dependency over instantsend/signing.
Same approach can be applied for https://github.com/dashpay/dash/pull/6761/files#r2219078456

What was done?

Created a new interface instantsend::InstantSendStorage that can be used all over codebase to break multiple circular dependency over instantsend. This PR is a minimal patch set to resolve instantsend/signing only. Further changes to be proposed in next PRs.

How Has This Been Tested?

Run test/lint/lint-circular-dependencies.py

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

[github-actions]

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

• refactor: move ChainLocks impl. to src/chainlock/ and split masternode logic out of manager source #6761 - refactor: move ChainLocks impl. to src/chainlocks/ and split masternode logic out of manager source

Please coordinate with the authors of these PRs to avoid merge conflicts.

[coderabbitai]

Walkthrough

The changes introduce a new abstract interface class, InstantSendStorage, in the instantsend/signing.h header. The CInstantSendManager class is updated to inherit from this interface, marking several of its methods as overrides. The InstantSendSigner class is refactored to depend on the InstantSendStorage interface instead of directly on CInstantSendManager, with corresponding changes to its constructor in both the header and implementation files. The inclusion of instantsend/instantsend.h is removed from signing.cpp. Additionally, an entry for a circular dependency between instantsend/instantsend and instantsend/signing is removed from the circular dependency linter test.

Estimated code review effort

3 (30–60 minutes)

---

📜 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 54c5f63 and 9cf98c6.

📒 Files selected for processing (4)

• src/instantsend/instantsend.h (4 hunks)
• src/instantsend/signing.cpp (1 hunks)
• src/instantsend/signing.h (2 hunks)
• test/lint/lint-circular-dependencies.py (0 hunks)

💤 Files with no reviewable changes (1)

• test/lint/lint-circular-dependencies.py

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit Inference Engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

• src/instantsend/signing.cpp
• src/instantsend/signing.h
• src/instantsend/instantsend.h

🧠 Learnings (4)

📓 Common learnings

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.

src/instantsend/signing.cpp (2)

Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.778Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety

src/instantsend/signing.h (1)

Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.

src/instantsend/instantsend.h (2)

Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.778Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety

⏰ 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: mac-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: arm-linux-build / Build source
• GitHub Check: linux64_nowallet-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: win64-build / Build source
• GitHub Check: Lint / Run linters

🔇 Additional comments (6)

src/instantsend/signing.cpp (1)

33-33: LGTM! Clean dependency inversion implementation.

The constructor parameter change from llmq::CInstantSendManager& to InstantSendStorage& successfully implements dependency inversion, allowing the InstantSendSigner to depend on an abstraction rather than a concrete implementation. This effectively breaks the circular dependency.

src/instantsend/instantsend.h (3)

17-17: Include added to make interface available.

The include of instantsend/signing.h is necessary to access the InstantSendStorage interface that CInstantSendManager now implements.

---

47-47: Excellent use of interface implementation with final specifier.

Making CInstantSendManager inherit from InstantSendStorage and marking it final is a clean implementation that:

• Provides the interface abstraction needed to break circular dependencies
• Prevents further inheritance, which is appropriate for this concrete implementation
• Maintains clear ownership of the InstantSend functionality

---

132-132: Proper override implementation matches interface contract.

All four methods correctly use the override specifier and match the interface definition in InstantSendStorage:

• IsLocked(const uint256& txHash) const
• GetConflictingLock(const CTransaction& tx) const
• TryEmplacePendingLock(const uint256& hash, const NodeId id, const InstantSendLockPtr& islock)
• IsInstantSendEnabled() const

This ensures compile-time verification that the interface contract is properly fulfilled.

Also applies to: 134-134, 156-156, 161-161

src/instantsend/signing.h (2)

28-38: Well-designed interface abstraction.

The InstantSendStorage interface is cleanly designed with:

• Virtual destructor for proper polymorphic cleanup
• Four focused pure virtual methods that encapsulate the essential InstantSend operations
• Method signatures that match the existing CInstantSendManager implementation perfectly
• Clear separation of concerns without over-abstraction

This interface effectively enables dependency inversion to break the circular dependency.

---

45-45: Clean dependency injection implementation.

The changes to InstantSendSigner properly implement dependency injection:

• Member variable m_isman now uses the InstantSendStorage& interface
• Constructor parameter updated to accept the interface reference
• This allows InstantSendSigner to work with any implementation of InstantSendStorage, not just CInstantSendManager

The dependency inversion principle is correctly applied, making the code more flexible and breaking the circular dependency.

Also applies to: 73-75

✨ Finishing Touches

• 📝 Generate Docstrings

---

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

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[kwvg]

As discussed in dash#6742 (comment), the circular dependency concern raised there, alongside other reviewer suggestions and comments will be addressed in a follow-up and have been (see dash#6770).

As for dash#6761, the review suggestion was noted and a ChainLocksStorage is currently being tested for inclusion in dash#6761

[knst]

kwvg commented 9 minutes ago

knst commented 6 minutes ago

Ooops, you have been 3 minutes faster. I opened this PR because I have seen https://github.com/dashpay/dash/pull/6761/files#r2219078456 and decided to take action!