fix: return removed code for m_dsq to relay Dsq queue by knst · Pull Request #7227 · dashpay/dash

knst · 2026-03-16T07:18:28Z

Issue being fixed or feature implemented

It has been removed too early in #7070, but that still used by CCoinJoinClientQueueManager::ProcessMessage

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

It has been removed too early in dashpay#7070, it's still used by CCoinJoinClientQueueManager::ProcessMessage

github-actions · 2026-03-16T07:19:25Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

github-actions · 2026-03-16T07:19:32Z

This pull request has conflicts, please rebase.

coderabbitai · 2026-03-16T07:21:57Z

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: 1676074d-37d3-4c35-ac44-912d647c6d3b

📥 Commits

Reviewing files that changed from the base of the PR and between 32fa1a8 and a910cb7.

📒 Files selected for processing (1)

src/net_processing.cpp

Walkthrough

The change adds DSQ (Dispute Settlement Queue) item relaying to the post-processing flow in PeerManagerImpl::PostProcessMessage. After the function processes transactions and inventory, it now iterates over result.m_dsq and invokes RelayDSQ(dsq) for each item, integrating DSQ handling into the existing message post-processing pipeline.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 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
Title check	✅ Passed	The title accurately describes the main change: restoring removed code for m_dsq to relay the DSQ queue, which aligns with the code modification.
Description check	✅ Passed	The description is directly related to the changeset, explaining the rationale (code was removed too early in `#7070`) and its continued use in CCoinJoinClientQueueManager::ProcessMessage.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📝 Coding Plan

Generate coding plan for human review comments

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.}

Tip

CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.

OpenGrep is compatible with Semgrep configurations. Add an opengrep.yml or semgrep.yml configuration file to your project to enable OpenGrep analysis.

thepastaclaw

Code Review

This is a correct and necessary 3-line bug fix. PR #7070 refactored the server side to call PeerRelayDSQ directly but also removed the m_dsq relay loop from PostProcessMessage, which the client side (CCoinJoinClientQueueManager::ProcessMessage) still depends on. Without this fix, DSQ messages received by non-masternode nodes are never relayed, breaking CoinJoin queue propagation. All findings are non-blocking suggestions.

Reviewed commit: a910cb7

🟡 2 suggestion(s) | 💬 1 nitpick(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/net_processing.cpp`:
- [SUGGESTION] lines 3646-3648: No test covers the client-side m_dsq relay path, risking repeat regression
  The DSQUEUE → `m_dsq.push_back()` → `PostProcessMessage` → `RelayDSQ` flow has no unit or functional test coverage. This exact path was silently broken by #7070 without any test catching it. A regression test exercising `CCoinJoinClientQueueManager::ProcessMessage` and verifying that `result.m_dsq` is non-empty (or that `RelayDSQ` is called) would prevent this from recurring. Neither `src/test/` nor `test/functional/` reference `m_dsq`, `DSQUEUE`, or `RelayDSQ`.

In `src/coinjoin/client.cpp`:
- [SUGGESTION] line 145: Client DSQ relay still uses indirect m_dsq path unlike the refactored server
  PR #7070 refactored the server side (`coinjoin/server.cpp`) to call `m_peer_manager->PeerRelayDSQ(dsq)` directly (lines 208, 533, 741), bypassing `MessageProcessingResult.m_dsq`. The client side still uses the old indirect path via `ret.m_dsq.push_back(dsq)`, which is the sole reason this `PostProcessMessage` loop needs to exist. As the PR description itself notes, the same refactoring should be done for `coinjoin/client` and `coinjoin/walletman`. This would eliminate the asymmetry and remove the fragile dependency on a relay loop that has already been accidentally deleted once.

thepastaclaw · 2026-03-16T08:49:49Z

src/net_processing.cpp

+    for (const auto& dsq : result.m_dsq) {
+        RelayDSQ(dsq);
+    }


🟡 Suggestion: No test covers the client-side m_dsq relay path, risking repeat regression

The DSQUEUE → m_dsq.push_back() → PostProcessMessage → RelayDSQ flow has no unit or functional test coverage. This exact path was silently broken by #7070 without any test catching it. A regression test exercising CCoinJoinClientQueueManager::ProcessMessage and verifying that result.m_dsq is non-empty (or that RelayDSQ is called) would prevent this from recurring. Neither src/test/ nor test/functional/ reference m_dsq, DSQUEUE, or RelayDSQ.

_{source: ['claude', 'codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed. In `src/net_processing.cpp`: - [SUGGESTION] lines 3646-3648: No test covers the client-side m_dsq relay path, risking repeat regression The DSQUEUE → `m_dsq.push_back()` → `PostProcessMessage` → `RelayDSQ` flow has no unit or functional test coverage. This exact path was silently broken by #7070 without any test catching it. A regression test exercising `CCoinJoinClientQueueManager::ProcessMessage` and verifying that `result.m_dsq` is non-empty (or that `RelayDSQ` is called) would prevent this from recurring. Neither `src/test/` nor `test/functional/` reference `m_dsq`, `DSQUEUE`, or `RelayDSQ`.

thepastaclaw · 2026-03-16T08:49:49Z

src/net_processing.cpp

+    for (const auto& dsq : result.m_dsq) {
+        RelayDSQ(dsq);
+    }


💬 Nitpick: A comment noting the sole caller would help prevent re-deletion

Since the server side now calls PeerRelayDSQ directly, only CCoinJoinClientQueueManager::ProcessMessage populates m_dsq. A brief comment here would help future developers understand why this loop exists and avoid accidentally removing it again.

Suggested change

for (const auto& dsq : result.m_dsq) {

RelayDSQ(dsq);

}

for (const auto& dsq : result.m_dsq) {

// Relays DSQ messages populated by CCoinJoinClientQueueManager::ProcessMessage (client-side)

RelayDSQ(dsq);

}

_{source: ['claude']}

UdjinM6 · 2026-03-16T09:23:03Z

pls see #7229

knst · 2026-03-16T09:29:19Z

Accordingly #7229 (comment) it is a dead code, not a fix.

…geProcessingResult d142fc8 refactor: remove dead m_dsq client-side relay from MessageProcessingResult (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Client-side DSQ relay was removed in #7070 but the `m_dsq` field and its population in `CCoinJoinClientQueueManager::ProcessMessage` were left behind as dead code. The network has been running without client-side DSQ relay with no issues since masternodes handle DSQ propagation through their own `CCoinJoinServer::ProcessDSQUEUE` path. Alternative to #7227 ## What was done? Remove the dead code and fix transitive include dependencies that were masked by msg_result.h pulling in coinjoin/coinjoin.h. This also resolves two circular dependency chains that went through that link. ## 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)_ ACKs for top commit: PastaPastaPasta: utACK d142fc8 Tree-SHA512: e8a8f0ac1b126021bbf1a9c61df4af7e062ce15e5541753232b967a4432965c650d3760df1e95fac9741e6fa87edc789683ada91e7e41d7269db473195c1601e

fix: return removed code for m_dsq to relay Dsq queue

a910cb7

It has been removed too early in dashpay#7070, it's still used by CCoinJoinClientQueueManager::ProcessMessage

github-actions bot added the needs rebase label Mar 16, 2026

github-actions bot removed the needs rebase label Mar 16, 2026

thepastaclaw reviewed Mar 16, 2026

View reviewed changes

UdjinM6 mentioned this pull request Mar 16, 2026

refactor: remove dead m_dsq client-side relay from MessageProcessingResult #7229

Merged

5 tasks

knst requested review from PastaPastaPasta and UdjinM6 March 16, 2026 09:21

knst added this to the 24 milestone Mar 16, 2026

knst added the backport-candidate-23.1.x label Mar 16, 2026

knst closed this Mar 16, 2026

UdjinM6 removed the backport-candidate-23.1.x label Mar 16, 2026

UdjinM6 removed this from the 24 milestone Mar 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: return removed code for m_dsq to relay Dsq queue#7227

fix: return removed code for m_dsq to relay Dsq queue#7227
knst wants to merge 1 commit intodashpay:developfrom
knst:fix-dsq-relay

knst commented Mar 16, 2026

Uh oh!

github-actions bot commented Mar 16, 2026

Uh oh!

github-actions bot commented Mar 16, 2026

Uh oh!

coderabbitai bot commented Mar 16, 2026

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Mar 16, 2026

Uh oh!

thepastaclaw Mar 16, 2026

Uh oh!

UdjinM6 commented Mar 16, 2026

Uh oh!

knst commented Mar 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

knst commented Mar 16, 2026

Issue being fixed or feature implemented

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Mar 16, 2026

✅ No Merge Conflicts Detected

Uh oh!

github-actions bot commented Mar 16, 2026

Uh oh!

coderabbitai bot commented Mar 16, 2026

Walkthrough

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Mar 16, 2026

Uh oh!

knst commented Mar 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants