Skip to content

test: improve CoinJoin unit coverage#7380

Open
thepastaclaw wants to merge 1 commit into
dashpay:developfrom
thepastaclaw:test/coinjoin-unit-coverage
Open

test: improve CoinJoin unit coverage#7380
thepastaclaw wants to merge 1 commit into
dashpay:developfrom
thepastaclaw:test/coinjoin-unit-coverage

Conversation

@thepastaclaw

@thepastaclaw thepastaclaw commented Jun 26, 2026

Copy link
Copy Markdown

test: improve CoinJoin unit coverage

Issue being fixed or feature implemented

Improves Dash-specific CoinJoin unit test coverage with focused behavioral tests
for existing mixing support code. This follows up on the coverage review that
identified src/coinjoin as one of the weakest Dash-specific areas.

Tracker: https://github.com/thepastaclaw/tracker/issues/1592

What was done?

Added unit coverage for behavior used by CoinJoin server/client flows:

  • CCoinJoinEntry::AddScriptSig now covers matching by prevout and sequence,
    duplicate rejection without overwrite, wrong prevout/wrong sequence rejection,
    and multi-input isolation.
  • CoinJoinQueueManager now covers masternode queue lookup semantics, including
    readiness-aware lookups and duplicate detection for same masternode plus same
    readiness.
  • CDSTXManager now covers clearing confirmed height when a tracked DSTX
    re-enters the mempool and ignoring mempool events for unrelated transactions.

This deliberately avoids superficial enum/string/status tests and does not add
production hooks or fake network harnesses.

Coverage impact

Historical baseline from the published Dash Core coverage snapshot for
develop at a59ad9e8b:
https://thepastaclaw.github.io/dash-coverage/2026-06-25-develop-a59ad9e8b/.

Old CoinJoin coverage from that snapshot:

  • Unit: src/coinjoin was 464 / 2524 lines (18.4%) and 176 / 453 functions
    (38.9%).
  • Unit + functional: src/coinjoin was 695 / 2524 lines (27.5%) and 237 /
    453 functions (52.3%).

Measured targeted lcov comparison for this PR:

  • Base: 8c9f166a3da637ed4eb667731f5017a2cc8c33a6.
  • Head: 873b14e9558ac8d9c11d031504083853b5d30678.
  • Build: ./configure --enable-lcov --without-gui --disable-bench --with-daemon --with-cli --disable-stacktraces --without-libs.
  • Test filter:
    ./src/test/test_dash --run_test=coinjoin_inouts_tests,coinjoin_queue_tests,coinjoin_dstxmanager_tests.
  • Result: base ran 13 test cases with no errors; head ran 18 test cases with no
    errors.

Raw lcov totals from those targeted runs:

  • src/coinjoin/coinjoin.cpp: 105 / 302 lines (34.8%) and 23 / 41 functions
    (56.1%) on base; 133 / 302 lines (44.0%) and 28 / 41 functions (68.3%) on
    head.
  • Aggregate src/coinjoin: 214 / 2911 lines (7.4%) and 88 / 673 functions
    (13.1%) on base; 261 / 2930 lines (8.9%) and 105 / 685 functions (15.3%) on
    head.

The aggregate denominator differs from the published full-suite snapshot because
this measurement is a local targeted lcov run as emitted by llvm-cov gcov,
including the header records present in that trace. The coinjoin.cpp numbers
are the directly comparable source-file movement from the PR's focused tests.

How Has This Been Tested?

Ran locally on macOS arm64 after building src/test/test_dash in an isolated
worktree:

./src/test/test_dash --run_test=coinjoin_inouts_tests,coinjoin_queue_tests,coinjoin_dstxmanager_tests

Result: 18 test cases, no errors detected.

Pre-PR review gate:

code-review dashpay/dash upstream/develop test/coinjoin-unit-coverage

Result: ship.

Breaking Changes

None. Test-only change.

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)

…empool re-entry

Adds focused unit tests for behaviors the server/client mixing paths depend on
but which were not previously exercised:

- CCoinJoinEntry::AddScriptSig: matching prevout + sequence copies scriptSig
  and sets fHasSig; duplicate signature for the same input is rejected without
  overwriting; wrong prevout or wrong sequence is rejected; with multiple
  inputs only the matching one mutates.
- CoinJoinQueueManager: HasQueueFromMasternode honors fReady;
  TryHasQueueFromMasternode finds by outpoint only; TryCheckDuplicate flags
  exact duplicates and same-masternode/same-readiness re-broadcasts but
  allows different readiness or a different masternode.
- CDSTXManager: TransactionAddedToMempool clears the confirmed height set by
  BlockConnected (reorg/re-entry path); a mempool event for an unrelated tx
  does not mutate the stored DSTX.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions

Copy link
Copy Markdown

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PR adds unit tests for CoinJoin-related behavior. It covers DSTX entries when transactions re-enter the mempool after confirmation, AddScriptSig matching and rejection rules for CoinJoin inputs, and CoinJoinQueueManager lookup and duplicate-detection behavior based on masternode identity, readiness, and queue timing.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Suggested reviewers

  • PastaPastaPasta
  • UdjinM6
🚥 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title concisely summarizes the main change: improved CoinJoin unit test coverage.
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.
Description check ✅ Passed The pull request description clearly matches the test-only CoinJoin coverage changes in the diff.
✨ 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

Copy link
Copy Markdown
Author

CI note: all real Dash build/test jobs are green on 873b14e9558ac8d9c11d031504083853b5d30678. The only red check is check_merge; its log shows this PR fast-forwards cleanly onto develop, then fails at the existing develop -> master fast-forward step. predict_conflicts is green, and CodeRabbit had no actionable comments.

@thepastaclaw thepastaclaw marked this pull request as ready for review June 26, 2026 11:09
@thepastaclaw

thepastaclaw commented Jun 26, 2026

Copy link
Copy Markdown
Author

✅ Review complete (commit 873b14e)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Code Review

Test-only PR adding focused unit coverage for CCoinJoinEntry::AddScriptSig, CoinJoinQueueManager queue lookup/duplicate semantics, and CDSTXManager mempool re-entry. All assertions verified against the implementations in src/coinjoin/coinjoin.cpp. Both reviewers converged on no findings; spot-checks of AddScriptSig, TryHasQueueFromMasternode, TryCheckDuplicate, TransactionAddedToMempool, and BlockConnected confirm the tests assert real behavior. Clean single conventional commit.

Note: GitHub does not allow me to approve my own PR, so this clean review is posted as COMMENT.

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