fix: don't charge CoinJoin participants for failures the masternode can't attribute to them

[PastaPastaPasta]

Issue being fixed or feature implemented

A CoinJoin masternode charges a participant's collateral when the participant does not cooperate: it reserved a slot but never sent its entry, or it never signed the final transaction. The masternode cannot tell a participant that withholds a message from one whose message was lost, so that charge is deliberately probabilistic (about one session in three). Nobody is charged when every participant failed, because that points at the masternode itself.

Three cases broke that balance. In each one, a participant could be charged for a failure that was not its own:

1. One participant's bad signature aborted the session for everyone. If a DSSIGNFINALTX input failed validation, the masternode relayed STATUS_REJECTED to every participant. Each client that receives it enters POOL_STATE_ERROR, releases its inputs and stops signing. The server kept the session until the signing timeout, where the clients that obeyed were the non-signers and one of them could be charged. Meanwhile the sender could already have signed everything. Resending an already accepted signature was enough to make an honest peer pay, every session.
2. One unreachable participant got the others charged. When the masternode could not deliver the final transaction or a status update to one participant, it told the others the session was terminated, but only reset it if every participant was unreachable. At the timeout, the participants that had obeyed counted as non-cooperating. So one disconnected peer, or the masternode's own connection dropping to part of the session, could cost an honest participant its collateral.
3. Signing offenders were counted per input, not per participant. A participant that left as many inputs unsigned as there are collaterals looked like "everyone failed" and could abort the signing phase without ever risking its collateral. A participant with several inputs was also more likely to be picked than one with a single input.

An earlier version of this PR made the timeout charge one collateral for every aborted session. As pointed out in review, that would have charged participants with certainty for failures that can come from the masternode's own connectivity. This version drops that policy. The probabilistic charge for a participant that does not cooperate in a running session is unchanged.

What was done?

This PR is built on top of #7537, and its six commits are included. Review the last three:

• fix(coinjoin): reject only the sender of an invalid signature: when an input of a DSSIGNFINALTX fails AddScriptSig(), only the sender gets STATUS_REJECTED. The session goes on for everyone else. If the sender's signature is still missing at the timeout, the sender is the non-signer there. A duplicate of an already accepted signature no longer affects the session at all.
• fix(coinjoin): end a session once a participant is unreachable: once RelayStatus() has told the participants to terminate because one of them is unreachable, it resets the session right away. The reset itself charges nobody. The one exception is the timeout-finalize path: a participant that never sent its entry is picked for charging before the final transaction is relayed, and is still charged if the relay then ends the session, as on develop. Before, it did this only when every participant was unreachable. RelayFinalTransaction() now finishes iterating the entries before it calls RelayStatus(), which can now clear them.
• fix(coinjoin): count each non-signing participant once: SelectCollateralToCharge() adds a participant once if any of its inputs is unsigned. The probabilistic policy and the everyone-failed exemption are unchanged.

What the masternode still charges for, unchanged from develop: a participant that reserved a slot and never sent its entry, or never signed, while the session was running. It charges at most about a third of the time, and never when every participant failed. A message lost on a connection that never visibly drops cannot be told apart from one that was withheld, so that case still falls under the probabilistic policy. This PR does not make it worse. It removes the cases where the masternode knows the failure was on its side or another participant's.

Compared with the previous version of this PR, these are all gone: the guaranteed abort charge, the in-flight message marks, the pending-charge reservations (#7537 now has its own, narrower reservation that only keeps a collateral being charged from being readmitted), the admission epoch, the immediate saboteur charge, the merge of #7815 and the release notes. This PR no longer depends on #7815.

How Has This Been Tested?

macOS arm64, depends build, clang with -Wthread-safety -Werror, on top of develop c8bfd7a.

New unit tests in coinjoin_inouts_tests. Each one was checked to fail with only its own commit's server.cpp change reverted:

• server_invalid_signature_rejects_only_its_sender: a participant's DSSIGNFINALTX fails validation through ProcessMessage(). With -capturemessages, the test checks that only the sender gets a DSSTATUSUPDATE, and that the session stays in SIGNING with both entries.
• server_unreachable_participant_ends_session: the final transaction is relayed by CreateFinalTransaction(). When every participant is reachable, the session moves to SIGNING. When one is not, the session ends right away.
• server_signing_timeout_counts_each_non_signer_once (uses TestChain100Setup): a participant with a real, funded collateral leaves all of its inputs unsigned while the others sign. The test checks that CheckTimeout() → ConsumeCollateral() puts that collateral in the mempool. Charging is randomized at about 0.34 per round, so the test allows up to 100 rounds; the chance of a false failure is about 1e-18. Without the fix the collateral is never charged. The test passed 30 out of 30 repeated runs.

Also run:

• Each of the three commits builds test_dash and passes coinjoin_inouts_tests on its own.
• At the tip, the unit suites coinjoin_inouts_tests, coinjoin_tests, coinjoin_queue_tests and coinjoin_dstxmanager_tests pass, and the functional tests rpc_coinjoin.py and p2p_dstx.py pass.
• lint-whitespace.py, lint-includes.py, lint-circular-dependencies.py, lint-logs.py and git diff --check pass.

Breaking Changes

None. No protocol, serialization or RPC change. Two server-side behaviors change. A participant whose signature is rejected now gets the only STATUS_REJECTED; the other participants no longer receive one. A session in which a participant has become unreachable now ends right away instead of at its timeout, and its participants have already been told to abandon it.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

🤖 Generated with Claude Code

[thepastaclaw]

🕓 Review not started yet because this PR is a draft.

• Request normal review — click when the PR is ready for review.
• Request priority review — click to move this review to the front of the queue.

Commit e328cd7. Normal review starts when eligible; priority review starts as soon as a slot is available.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2bc5dbd248

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dd393908-9203-402f-a9c3-15b66f6e9c1e

📥 Commits

Reviewing files that changed from the base of the PR and between a67fc15 and 118d995.

📒 Files selected for processing (3)

• src/coinjoin/server.cpp
• src/coinjoin/server.h
• src/test/coinjoin_inouts_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (2)

• src/coinjoin/server.h
• src/coinjoin/server.cpp

Walkthrough

CoinJoin session denomination is now atomic, and validation uses captured denomination values. Server collateral state is consolidated in a lock-protected container. Pool checks, message handling, entry admission, finalization, and timeout processing use session snapshots and lock coordination. Fee selection supports probabilistic and guaranteed-abort policies. Tests cover collateral selection, timeout ordering, in-flight messages, pool locking, session recovery, and concurrent denomination validation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CoinJoinClient
  participant CCoinJoinServer
  participant cs_coinjoin
  participant SessionCollaterals
  participant Mempool
  CoinJoinClient->>CCoinJoinServer: submit entry or signing message
  CCoinJoinServer->>cs_coinjoin: capture and validate session state
  CCoinJoinServer->>SessionCollaterals: check or store collateral
  CCoinJoinServer->>cs_coinjoin: finalize session or update state
  CCoinJoinServer->>Mempool: process final transaction

Loading

Possibly related PRs

• dashpay/dash#7537: Implements related CoinJoin race-condition fixes across the same session state and server paths.
• dashpay/dash#7566: Introduces related FeePolicy, collateral selection, and fee-charging changes.
• dashpay/dash#7567: Covers related collateral fee-policy and abort-handling changes.

Suggested reviewers: udjinm6, knst

Mergeability Score: ⚪ Minimal · up to 118d9

The PR adds collateral charging for non-cooperative aborted sessions while preserving free queue timeouts and expanding test coverage. No actionable merge-blocking risk remains after normal checks and review.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		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
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly and concisely describes the main change: preventing CoinJoin participants from being charged for failures that the masternode cannot attribute to them.
Description check		The description is directly related to the CoinJoin collateral-handling changes and explains the affected failure cases, implementation, testing, and behavior changes.

• Fix all pre-merge checks with AI

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/coinjoin/coinjoin.h (1)

345-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Correct the logging explanation. tinyformat formats std::atomic<int> through its implicit conversion to int. LogPrint() accepts arguments by const reference, which avoids copying the non-copyable atomic. WalletCJLogPrint() forwards to CWallet::WalletLogPrintf, whose parameters are passed by value, so .load() is required to pass an int.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/coinjoin/coinjoin.h` around lines 345 - 350, Update the comments above
nSessionDenom to accurately explain that tinyformat uses the atomic’s implicit
int conversion, LogPrint() accepts it by const reference without copying, and
WalletCJLogPrint() forwards to CWallet::WalletLogPrintf with by-value
parameters, requiring an explicit .load().

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/coinjoin/coinjoin.h`:
- Around line 345-350: Update the comments above nSessionDenom to accurately
explain that tinyformat uses the atomic’s implicit int conversion, LogPrint()
accepts it by const reference without copying, and WalletCJLogPrint() forwards
to CWallet::WalletLogPrintf with by-value parameters, requiring an explicit
.load().

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c98ea00-f5ed-46f1-b28d-035fcce2d4d4

📥 Commits

Reviewing files that changed from the base of the PR and between 1fbf489 and 2bc5dbd.

📒 Files selected for processing (5)

• src/coinjoin/client.cpp
• src/coinjoin/coinjoin.h
• src/coinjoin/server.cpp
• src/coinjoin/server.h
• src/test/coinjoin_inouts_tests.cpp

[thepastaclaw]

Preliminary review — Codex only

The guaranteed abort-fee policy can still consume collateral from honest submissions already being processed, and a separate TRY_LOCK interleaving can reset sessions that remain recoverable. The commit stack also contains four syntactically unbuildable intermediate commits, while the offender-deduplication behavior change is obscured by a refactor-only commit subject.
Source: reviewer backends: gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history); final verifier backend: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 3 blocking | 🟡 1 suggestion(s)

🤖 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/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:625-628: Exclude in-flight submissions from guaranteed timeout penalties
  `SelectCollateralToCharge()` only recognizes entries already committed to `vecEntries`, but `AddEntry()` releases `cs_coinjoin` while running `IsCollateralValid()` and `IsValidInOuts()` at lines 759-794. A valid `DSVIN` whose processing began before the deadline can therefore still be validating when the scheduler observes the timeout. With three reservations, two committed entries, and the third entry in flight, the third participant is classified as the only missing submitter, selected with certainty, and charged after `SetNull()` makes its final session revalidation fail. Signing has the same gap while `DSSIGNFINALTX` is being decoded or between its per-input `AddScriptSig()` calls. Track in-flight messages for the current session or serialize the timeout cutoff with their complete processing before applying a guaranteed collateral penalty.
- [BLOCKING] src/coinjoin/server.cpp:610-628: Preserve recoverable finalization when the preceding pool check is skipped
  `CheckTimeout()` assumes the immediately preceding `CheckPool()` handled every recoverable accepting-entry timeout, but `CheckPool()` uses a non-blocking `TRY_LOCK`. A message-handling thread can hold `cs_check_pool`, sample `HasTimedOut()` as false immediately before the deadline, and then release the mutex after the scheduler's `CheckPool()` has skipped it but before the scheduler calls `CheckTimeout()`. `CheckTimeout()` then acquires the mutex after the deadline and unconditionally resets the session. If the session has at least `GetMinPoolParticipants()` committed entries but fewer entries than reservations, it should enter `ChargeAndFinalize` and retain the probabilistic policy; this interleaving instead aborts it and applies `GUARANTEED_ON_ABORT`. Re-evaluate the full accepting-entry action after acquiring `cs_check_pool` rather than relying on a preceding check that may have been skipped or sampled an earlier time.
- [BLOCKING] src/coinjoin/server.cpp:634: Fold the stray-brace correction into its introducing commit
  Commit `67c9647ed03` introduces an extra closing brace immediately after `CCoinJoinServer::CheckTimeout()`. The unmatched brace remains in `264ba3fdfa7`, `76e366e1ceb`, and `9e199087637`, and is only removed by the final commit `2bc5dbd248b`. Those four intermediate commits are syntactically unbuildable and unusable as `git bisect` points. Amend `67c9647ed03` to omit the extra brace, remove the corrective deletion from the final commit, and rebase the intervening commits so every permanent-history state builds independently.
- [SUGGESTION] src/coinjoin/server.cpp:503-509: Make offender deduplication explicit in the commit history
  Commit `9e199087637` is titled `refactor: separate CoinJoin offender selection policy`, but it also changes observable behavior: signing offenders are added once per entry instead of once per unsigned input. This removes the extra random-selection weight previously assigned to participants with multiple unsigned inputs. Move the deduplication hunk into the behavioral fix commit or reword `9e199087637` and add a commit body explaining the weighting change so `git log` and `git blame` expose the functional change.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3d5cf6b6e0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thepastaclaw]

Preliminary review — Codex only

All four prior verified findings are resolved at the current head, but two signing-timeout paths still block approval: guaranteed abort charging can penalize honest participants after the server tells them to stop, and a stale pool-check interleaving can discard a fully signed transaction. The stack also has two non-blocking history issues: the lint-only follow-up should be folded into its introducing commits, and the subtle timeout fixes should retain their rationale in commit bodies.
Source: reviewer backends: gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history); final verifier backend: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking | 🟡 2 suggestion(s)

2 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/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:681-684: Do not guarantee a fee after the coordinator aborts signing
  The guaranteed timeout policy applies even after the server has instructed connected clients to stop signing. `RelayFinalTransaction()` calls `RelayStatus(STATUS_REJECTED)` when any entry is disconnected, and `ProcessDSSIGNFINALTX()` does the same after any `AddScriptSig()` failure. Connected clients process that rejection by entering `POOL_STATE_ERROR` and releasing their session resources, but the server remains in `POOL_STATE_SIGNING` unless every entry is disconnected. A malicious participant can exploit this by submitting its valid signature and then resubmitting it: the duplicate fails, every honest peer is told to abort, and the malicious participant is excluded from the unsigned-offender set. At timeout, this block then guarantees that one honest participant is charged. A failed `DSFINALTX` relay can similarly charge a connected entry that never received the transaction. Coordinator-originated signing aborts must reset without the guaranteed fee, or eligibility must be limited to participants that received the final transaction and were not subsequently instructed to abort.
- [BLOCKING] src/coinjoin/server.cpp:669-684: Commit complete signatures when rechecking a timeout
  `CheckTimeout()` re-evaluates recoverable accepting-entry sessions but does not re-evaluate `IsSignaturesComplete()` for signing sessions. A scheduler `CheckPool()` can acquire `cs_check_pool`, observe the signatures as incomplete, and release `cs_coinjoin`. The final on-time `DSSIGNFINALTX` can then add its signature, skip its own `CheckPool()` because the scheduler still owns `cs_check_pool`, and clear its in-flight guard. When the scheduler subsequently enters `CheckTimeout()` after the deadline, there are no unsigned offenders, but this block still calls `SetNull()` and discards the fully signed transaction. Re-evaluate the signing action under `cs_coinjoin`, then call `CommitFinalTransaction()` after releasing that lock, just as the accepting-entry action is re-evaluated and finalized.

In `<commit:aaa6d04>`:
- [SUGGESTION] <commit:aaa6d04>:1: Fold the lint-only follow-up into its introducing commits
  Commit `aaa6d0464a4` only adds `/* Continued */` markers to four `LogPrint` calls introduced earlier in this stack: one in `96cf3768cab` and three in `7443d022e09`. The linter rejects those unmarked calls, so retaining the separate correction leaves the introducing revisions as lint-failing bisect points and adds review-fix noise to permanent history. Fold each marker into the commit that introduced its call and drop `aaa6d0464a4`.

In `<commit:bb056c6>`:
- [SUGGESTION] <commit:bb056c6>:1: Preserve the rationale for the non-obvious CoinJoin fixes
  Commits `bb056c6d397`, `7da61132813`, and `3d5cf6b6e0f` have empty bodies despite changing subtle concurrency and fee-policy invariants. In particular, they serialize timeout reset with finalization, bind entry validation to a session denomination snapshot, and combine guaranteed abort charging with in-flight-message deferral and recoverable-timeout re-evaluation. Add concise bodies explaining the race or policy invariant each commit preserves so ordinary `git log`, blame, and bisect retain the reasoning currently available only from source comments and PR discussion.

[coderabbitai]

🧹 Nitpick comments (4)

src/test/coinjoin_inouts_tests.cpp (4)

750-781: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Narrow the size_t values passed to MakeCollateral and MakePeer.

MakeCollateral(uint32_t) receives the size_t loop variable i at Line 762, and MakePeer(NodeId, uint32_t) receives 0x0a000001 + i at Line 766. Both conversions are implicit narrowing. Add explicit casts so the intent is clear and no compiler in CI reports a conversion warning.

♻️ Proposed change

-        const auto collateral = MakeCollateral(i);
+        const auto collateral = MakeCollateral(static_cast<uint32_t>(i));
@@
-        auto peer = MakePeer(i, 0x0a000001 + i);
+        auto peer = MakePeer(static_cast<NodeId>(i), static_cast<uint32_t>(0x0a000001 + i));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/coinjoin_inouts_tests.cpp` around lines 750 - 781, In
server_timeout_rechecks_recoverable_session, explicitly narrow the size_t loop
variable when passing it to MakeCollateral(uint32_t), and explicitly cast the
computed address value passed to MakePeer(NodeId, uint32_t). Preserve the
existing loop behavior while eliminating implicit conversion warnings.

---

188-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add negative lock annotations to the new helpers that take cs_coinjoin.

SeedParticipant at Line 246 declares EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin), and RelayAbortForTest at Line 240 follows that pattern. The other new helpers (ResetForTest, SetTimedOutForTest, AddCollateralForTest, AddEntryForTest, SelectForTest, EnterSigningState, SetFinalTransactionForTest, SeedTimedOutSession, MarkMessageInFlightForTest) also acquire cs_coinjoin but declare no negative capability. Clang thread-safety analysis then cannot detect a caller that already holds cs_coinjoin.

♻️ Example annotation for the affected helpers

-    void ResetForTest(PoolState state)
+    void ResetForTest(PoolState state) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin)
     {
         LOCK(cs_coinjoin);

Also applies to: 253-275

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/coinjoin_inouts_tests.cpp` around lines 188 - 244, Add
EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin) annotations to ResetForTest,
SetTimedOutForTest, AddCollateralForTest, AddEntryForTest, SelectForTest,
EnterSigningState, SetFinalTransactionForTest, SeedTimedOutSession, and
MarkMessageInFlightForTest, matching the existing annotations on SeedParticipant
and RelayAbortForTest. Keep each helper’s internal cs_coinjoin locking
unchanged.

---

321-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider splitting this test case into separate scenarios.

server_abort_fee_selects_unique_offenders verifies nine distinct behaviours in one test case: queue timeout, all-missing submitters, one submitter, partial submitters, all submitters, a lone non-signer, weighting across multiple non-signers, the probabilistic gate, and ChargeRandomFees. A failure in an early scenario stops the later ones, and the failure output does not name the scenario. Separate BOOST_AUTO_TEST_CASE blocks per policy scenario would isolate failures. The server construction can move into a small helper to avoid repetition.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/coinjoin_inouts_tests.cpp` around lines 321 - 424, Split
server_abort_fee_selects_unique_offenders into separate BOOST_AUTO_TEST_CASE
blocks covering each policy scenario, including timeout handling, submission
coverage, non-signer selection and weighting, the probabilistic gate, and
ChargeRandomFees. Move shared server setup into a small helper to avoid
repetition, and retain each scenario’s existing assertions and setup behavior.

---

609-624: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer an exact integer constant for the expected mempool delta.

static_cast<CAmount>(0.1 * COIN) uses floating point for a consensus-adjacent amount comparison. Use COIN / 10 to keep the expectation exact and independent of double rounding.

♻️ Proposed change

-    BOOST_CHECK_EQUAL(delta, static_cast<CAmount>(0.1 * COIN));
+    BOOST_CHECK_EQUAL(delta, COIN / 10);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/coinjoin_inouts_tests.cpp` around lines 609 - 624, Replace the
floating-point expected amount in the delta assertion after ApplyDelta with the
exact integer expression COIN / 10, while preserving the existing CAmount
comparison and test behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 750-781: In server_timeout_rechecks_recoverable_session,
explicitly narrow the size_t loop variable when passing it to
MakeCollateral(uint32_t), and explicitly cast the computed address value passed
to MakePeer(NodeId, uint32_t). Preserve the existing loop behavior while
eliminating implicit conversion warnings.
- Around line 188-244: Add EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin) annotations to
ResetForTest, SetTimedOutForTest, AddCollateralForTest, AddEntryForTest,
SelectForTest, EnterSigningState, SetFinalTransactionForTest,
SeedTimedOutSession, and MarkMessageInFlightForTest, matching the existing
annotations on SeedParticipant and RelayAbortForTest. Keep each helper’s
internal cs_coinjoin locking unchanged.
- Around line 321-424: Split server_abort_fee_selects_unique_offenders into
separate BOOST_AUTO_TEST_CASE blocks covering each policy scenario, including
timeout handling, submission coverage, non-signer selection and weighting, the
probabilistic gate, and ChargeRandomFees. Move shared server setup into a small
helper to avoid repetition, and retain each scenario’s existing assertions and
setup behavior.
- Around line 609-624: Replace the floating-point expected amount in the delta
assertion after ApplyDelta with the exact integer expression COIN / 10, while
preserving the existing CAmount comparison and test behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ec59951d-1248-4c0c-a6ba-d5f91c7b66eb

📥 Commits

Reviewing files that changed from the base of the PR and between 3d5cf6b and 9777136.

📒 Files selected for processing (3)

• src/coinjoin/server.cpp
• src/coinjoin/server.h
• src/test/coinjoin_inouts_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (2)

• src/coinjoin/server.h
• src/coinjoin/server.cpp

[thephez]

@PastaPastaPasta this could result in CoinJoin participants being penalized for masternode connectivity issues, right?

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The exact-head Codex revalidation found no actionable defects: the two signing-timeout blockers are fixed, and the rewritten commit stack resolves both history suggestions. All four prior verified findings are resolved at 9777136, so this preliminary gate approves the PR for the next review phase.
Source: reviewer backends: gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history); final verifier backend: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
• Secondary pass: disabled (temporary_phase2_sonnet_disable)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4618c53e4b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (5)

src/coinjoin/server.cpp (1)

536-612: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a unit test for the guaranteed-abort path after a relayed abort.

CheckTimeout() skips collateral selection when m_relayed_abort is set. RelayStatus(STATUS_REJECTED) and the disconnect path both set that flag. The test helpers shown in src/test/coinjoin_inouts_tests.cpp seed timed-out sessions and relay rejections, so a case that relays a rejection and then asserts that no collateral is consumed is cheap to add.

As per coding guidelines: "Add small tests proving invariants when changing consensus, serialization, masternode, LLMQ, InstantSend, ChainLocks, governance, EvoDB, credit-pool, Platform, BLS, quorum prediction, or timing/shutdown behavior."

Also applies to: 666-708

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/coinjoin/server.cpp` around lines 536 - 612, Add a unit test in the
existing coinjoin input/output test helpers covering a timed-out session that
receives RelayStatus(STATUS_REJECTED), then verify CheckTimeout() takes the
guaranteed-abort path without consuming or selecting collateral. Reuse the
established session-seeding and relay-rejection setup, and assert the collateral
state remains unchanged after the timeout check.

Source: Coding guidelines

src/test/coinjoin_inouts_tests.cpp (4)

180-199: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Confirm that consumed_collaterals is never written from two threads.

consumed_collaterals is a plain mutable std::vector with no synchronization. In server_timeout_does_not_reset_during_pool_check, a second thread runs HoldPoolCheck, which only takes cs_check_pool and does not consume collateral, so the current tests appear safe. If a future test lets a helper thread reach ConsumeCollateral, the vector will race. Guarding it with cs_coinjoin or a dedicated mutex would make the invariant explicit.

Also applies to: 220-251, 260-345, 819-841

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/coinjoin_inouts_tests.cpp` around lines 180 - 199, The test-only
TestableCoinJoinServer::consumed_collaterals vector is unsynchronized and could
race if ConsumeCollateral runs on another thread. Protect accesses in
ConsumeCollateral, ResetForTest, and all relevant test assertions with
cs_coinjoin or a dedicated mutex, preserving the existing lifecycle-test
behavior.

---

355-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Clarify the unsigned_inputs semantics in MakeEntry.

unsigned_inputs == 0 produces one signed input, and unsigned_inputs == N produces N unsigned inputs. The parameter therefore controls both the input count and the signature flag. A short comment or a distinct parameter name would make the call sites at Lines 429, 438, and 539 easier to read.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/coinjoin_inouts_tests.cpp` around lines 355 - 364, Clarify the dual
semantics of unsigned_inputs in MakeEntry: zero creates one signed input, while
a positive value creates that many unsigned inputs. Add a concise comment or
rename the parameter to explicitly communicate this behavior, keeping the
existing input-count and fHasSig logic unchanged.

---

366-486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated server construction into a fixture helper.

Eight test cases repeat the same CActiveMasternodeManager plus TestableCoinJoinServer construction with ten identical arguments. A helper that returns the configured server would remove the duplication and keep future constructor changes to one site.

Also applies to: 488-524, 526-555, 654-677, 679-718, 720-775, 777-817, 819-841, 843-863, 865-885

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/coinjoin_inouts_tests.cpp` around lines 366 - 486, Extract the
repeated CActiveMasternodeManager and TestableCoinJoinServer setup from the
affected test cases into a shared fixture helper that returns a configured
TestableCoinJoinServer. Update each listed test, including
server_abort_fee_selects_unique_offenders, to use the helper while preserving
the existing node dependencies and test behavior; keep constructor arguments
centralized in that helper.

---

709-716: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Replace the duplicated prioritisation amount with the production constant.

The test hardcodes static_cast<CAmount>(0.1 * COIN). The production code in CommitFinalTransaction applies the same value. If the production value changes, this assertion silently encodes the old policy. Reference the shared constant instead.

🔍 Verification script

#!/bin/bash
# Locate the prioritisation delta used by CommitFinalTransaction.
rg -nP -C4 'PrioritiseTransaction|0\.1\s*\*\s*COIN' src/coinjoin

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/coinjoin_inouts_tests.cpp` around lines 709 - 716, Update the delta
assertion in the coinjoin test to use the shared production prioritisation
constant applied by CommitFinalTransaction instead of hardcoding
static_cast<CAmount>(0.1 * COIN). Keep the existing delta calculation and
validation unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/coinjoin/server.cpp`:
- Around line 348-404: Update CCoinJoinServer::CheckPool’s locked snapshot so
the timeout-driven ChargeAndFinalize action is selected only when
m_inflight_session does not equal the current nSessionID. Preserve normal
finalization when all entries are present, but defer charging/finalizing while
the current session has an in-flight submission, matching the guard used by
CheckTimeout().
- Around line 578-587: Update the probabilistic collateral check using
nSessionCollaterals and vecOffendersCollaterals so it cannot evaluate
nSessionCollaterals - 1 when the size is zero. Add an explicit nonzero guard or
use a signed comparison, while preserving the intended “mostly offending”
behavior when entries exist without collaterals.

---

Nitpick comments:
In `@src/coinjoin/server.cpp`:
- Around line 536-612: Add a unit test in the existing coinjoin input/output
test helpers covering a timed-out session that receives
RelayStatus(STATUS_REJECTED), then verify CheckTimeout() takes the
guaranteed-abort path without consuming or selecting collateral. Reuse the
established session-seeding and relay-rejection setup, and assert the collateral
state remains unchanged after the timeout check.

In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 180-199: The test-only
TestableCoinJoinServer::consumed_collaterals vector is unsynchronized and could
race if ConsumeCollateral runs on another thread. Protect accesses in
ConsumeCollateral, ResetForTest, and all relevant test assertions with
cs_coinjoin or a dedicated mutex, preserving the existing lifecycle-test
behavior.
- Around line 355-364: Clarify the dual semantics of unsigned_inputs in
MakeEntry: zero creates one signed input, while a positive value creates that
many unsigned inputs. Add a concise comment or rename the parameter to
explicitly communicate this behavior, keeping the existing input-count and
fHasSig logic unchanged.
- Around line 366-486: Extract the repeated CActiveMasternodeManager and
TestableCoinJoinServer setup from the affected test cases into a shared fixture
helper that returns a configured TestableCoinJoinServer. Update each listed
test, including server_abort_fee_selects_unique_offenders, to use the helper
while preserving the existing node dependencies and test behavior; keep
constructor arguments centralized in that helper.
- Around line 709-716: Update the delta assertion in the coinjoin test to use
the shared production prioritisation constant applied by CommitFinalTransaction
instead of hardcoding static_cast<CAmount>(0.1 * COIN). Keep the existing delta
calculation and validation unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c9e11abb-3443-427e-ac0d-4a4598969547

📥 Commits

Reviewing files that changed from the base of the PR and between 9777136 and a67fc15.

📒 Files selected for processing (5)

• src/coinjoin/coinjoin.cpp
• src/coinjoin/coinjoin.h
• src/coinjoin/server.cpp
• src/coinjoin/server.h
• src/test/coinjoin_inouts_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (2)

• src/coinjoin/coinjoin.h
• src/coinjoin/server.h

[thepastaclaw]

Preliminary review — Codex only

The timeout handling still has two in-scope concurrency defects: an on-time in-flight entry can be excluded and probabilistically charged during recoverable finalization, and a selected timeout collateral can be admitted into a replacement session before the penalty transaction consumes it. The unsigned-underflow report is not actionable because the locked production invariants ensure a nonempty offender set always belongs to a session with at least one collateral.
Source: reviewer backend model gpt-5.6-sol (general); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking

🤖 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/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:382-385: Defer timeout finalization while an entry is in flight
  `ProcessDSVIN()` marks an on-time entry in flight before deserialization and validation, but this timeout-driven `ChargeAndFinalize` branch does not inspect that marker. If the existing committed entries already meet the minimum, the scheduler can select the still-validating participant as a missing submitter, potentially charge its collateral, and move the session to signing. The pending `AddEntry()` then fails its session-state revalidation, excluding an entry that arrived before the deadline. Require `m_inflight_session != nSessionID` for this timeout branch, matching `CheckTimeout()`; normal finalization when every reservation already has an entry should remain unchanged.
- [BLOCKING] src/coinjoin/server.cpp:703-707: Keep new-session admission closed until the penalty is consumed
  `SetNull()` clears the timed-out session and makes the server idle before `ConsumeCollateral()` acquires `cs_main`. During that interval, the message-handling thread can test-accept the selected collateral in `IsAcceptableDSA()` and record it in a replacement session through `CreateNewSession()`. The timeout thread can then consume and relay that collateral, leaving the replacement session with a reservation whose inputs are spent; its participant cannot submit a valid entry, and the new mix can stall or abort. Keep admission closed until consumption finishes, or maintain a pending-penalty prevout set that both new- and existing-session admission reject until the penalty attempt completes.

[thepastaclaw]

Preliminary review — Codex only

The in-flight timeout-finalization defect is fixed at the current head: CheckPool() now defers ChargeAndFinalize while the current session has a message in flight. One blocking race remains because CheckTimeout() exposes the idle state before consuming the selected penalty collateral, allowing that collateral to be reserved by a replacement session before its inputs are spent.
Source: reviewer backend model gpt-5.6-sol (general); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:706-710: Keep new-session admission closed until the penalty is consumed
  (existing thread: https://github.com/dashpay/dash/pull/7568#discussion_r3770096427)
  `SetNull()` clears the timed-out session and exposes `POOL_STATE_IDLE` before `ConsumeCollateral()` acquires `cs_main`. A concurrent `DSACCEPT` can therefore pass `IsAcceptableDSA()` while the selected collateral is still unspent and commit it to a replacement session through `CreateNewSession()`; the timeout thread can then consume that collateral, leaving the replacement session with a reservation whose inputs are already spent. The participant cannot subsequently submit a valid entry, so the replacement mix can stall or abort. Keep admission closed until the consumption attempt completes without introducing a `cs_coinjoin` to `cs_main` lock inversion, or retain the selected collateral's prevouts in a pending-penalty set that both new- and existing-session admission reject until consumption finishes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 118d995b42

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thepastaclaw]

Preliminary review — Codex only

One blocking concurrency defect remains in the signature failure path: a stale DSSIGNFINALTX can relay an abort after the completed session has reset, causing the following session to inherit m_relayed_abort and skip its guaranteed abort charge. The prior admission race is fixed by reserving selected collateral prevouts in m_pending_charges until collateral consumption settles.
Source: reviewer backend model gpt-5.6-sol (general); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

🤖 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/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:294-310: Revalidate the session before relaying a signature abort
  The failure path does not verify that the session captured in `session_id` is still the current signing session before charging collateral or calling `RelayStatus(STATUS_REJECTED)`. After an extra or duplicate signature passes the initial state check, `CheckPool()` can observe that all required signatures are complete and commit the session without consulting `m_inflight_session`. If the commit resets the session before this message reaches the failure block, `AddScriptSig()` fails against the cleared entries, the collateral lookup finds nothing, and the unconditional `RelayStatus()` sets `m_relayed_abort` while the server is idle. `CreateNewSession()` does not clear that flag, so the next session inherits it and `CheckTimeout()` suppresses its guaranteed abort charge. Revalidate `nSessionID == *session_id` and `nState == POOL_STATE_SIGNING` under `cs_coinjoin` before mutating abort state.

[PastaPastaPasta]

I rebuilt this PR on the rebased #7537 (develop 8819507). Head is now a56563bb77: #7537's six commits plus this PR's four (c7d51f07e0, c52f378494, 23759f5319, a56563bb77). The PR description is updated to match and still notes the dependency on #7537. All inline threads have replies.

Changes beyond the plain rebase

• MarkMessageInFlight() now uses Assume() instead of assert(). A leftover mark could only delay a timeout, so it should not take the node down.
• fix: data races and check-then-act races in the CoinJoin server #7537 now applies a DSSIGNFINALTX batch atomically against a revalidated session. So the saboteur charge and the rejection relay sit inside that critical section, which replaces the separate stale-failure revalidation. AddScriptSig() no longer needs to be virtual. I dropped server_stale_signature_after_commit_does_not_poison_next_session, because the interleaving it simulated (a reset while AddScriptSig() runs) can no longer happen with cs_coinjoin held across the batch.
• The in-flight deferral for timeout finalization moved from CheckPool()'s snapshot into CreateFinalTransaction(), under the lock where the charge is selected. A new case, server_timeout_finalization_waits_for_in_flight_entry, covers it.
• The tests use develop's protected-seam pattern instead of a friend. The test seams that put a session into a state now also stamp nTimeLastSuccessfulStep, as SetState() does, so they do not seed already-timed-out sessions now that late messages are rejected at the cutoff.

CodeRabbit nitpicks from the review bodies

• Split server_abort_fee_selects_unique_offenders into one case per scenario, with a shared ServerHarness fixture: done.
• EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin) on the test helpers: done.
• COIN / 10 instead of 0.1 * COIN in the prioritisation check: done. There is no named production constant to reuse; CommitFinalTransaction() uses the literal.
• Clarify MakeEntry's unsigned_inputs semantics: done, with a doc comment.
• Explicit narrowing in server_timeout_rechecks_recoverable_session: that test no longer exists. Its coverage now lives in fix: data races and check-then-act races in the CoinJoin server #7537's server_timeout_does_not_reset_actionable_session, which uses explicit casts.
• Test that RelayStatus(STATUS_REJECTED) then CheckTimeout() charges nobody: covered by server_relayed_abort_forgoes_guaranteed_timeout_charge.
• Synchronize consumed_collaterals: declined. ChargeRecordingServer is only ever driven from the test thread; ConsumeCollateral() is called synchronously from CheckTimeout() / ProcessDSSIGNFINALTX() on that thread, so there is no concurrent access to guard.
• Unsigned underflow in the probabilistic comparison: already resolved. The expression is offenders.size() + 1 >= nSessionCollaterals.

On the question of whether participants could be penalized for masternode connectivity issues: partly, yes. Here is what the code does now:

• If the masternode notices it cannot reach a participant (relaying DSFINALTX or a status update fails), it relays a session-wide abort, and that session's timeout charges nobody.
• If every participant is unreachable, the session is reset without any charge.
• If a participant's DSVIN/DSSIGNFINALTX was received before the deadline, it is never counted as missing.
• What remains: if a message from a participant never reaches the masternode, and the masternode does not see a disconnect (for example, an inbound path problem it cannot detect), that participant looks exactly like a no-show. Previously that cost ~33% × (sometimes) and nothing if everyone failed. With this PR, exactly one non-cooperating participant per aborted session is charged.

That trade-off is the policy this PR proposes, and I've left it for pasta to confirm rather than changing it here.

Validation: clang -Wthread-safety -Werror build. Each of the four commits builds and passes coinjoin_inouts_tests. At the tip, coinjoin_inouts_tests, coinjoin_tests, coinjoin_queue_tests, coinjoin_dstxmanager_tests, rpc_coinjoin.py, p2p_dstx.py and the whitespace/includes/circular-dependency lints pass.

---

🤖 Posted autonomously by Codex on behalf of pasta.

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[PastaPastaPasta]

this could result in CoinJoin participants being penalized for masternode connectivity issues, right?

Yes, with the version you reviewed. It charged one collateral for every session that timed out, so a participant whose message never reached the masternode, or who never got DSFINALTX from it, paid with certainty, even when the masternode's own connection was at fault. Develop itself already had a weaker form of this. When the masternode can't reach one participant, it tells all the others to abandon the session. It only reset the session if every participant was unreachable, though, so at the timeout the participants that had obeyed counted as non-signers and could be charged (RelayStatus() / RelayFinalTransaction() in src/coinjoin/server.cpp).

I've redesigned the PR around that:

• No guaranteed charge any more. For a participant that doesn't send or sign while the session is running, charging stays as on develop: roughly one session in three, and never when every participant failed, since that points at the masternode itself.
• Unreachable participant (83cd00f). Once the masternode has told the participants to terminate because one of them is unreachable, it now ends the session right away and charges nobody. A peer we can't reach is no evidence against anyone; our own connection may be what broke.
• Invalid signature (a5e6caf). A signature that fails validation no longer aborts the session for everyone. Only its sender is rejected, so honest participants are never told to stop and then charged for stopping.

What's left is inherent and unchanged from develop: if a participant's message is lost on a connection that never visibly drops, the masternode can't tell that from withholding. That case still falls under the probabilistic charge and the everyone-failed exemption. The charge is the collateral fee (0.0001 DASH), never the inputs being mixed. This PR no longer makes that case worse; it only removes the cases where the masternode knows the failure wasn't the participant's.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[PastaPastaPasta]

Rebuilt this PR on the updated #7537 (head 1d59788, on develop c8bfd7a). It is now #7537's six commits plus three commits of its own:

• a5e6caf fix(coinjoin): reject only the sender of an invalid signature
• 83cd00f fix(coinjoin): end a session once a participant is unreachable
• 95b378a fix(coinjoin): count each non-signing participant once

The previous design charged one collateral for every aborted session. That penalized participants for failures the masternode can't attribute (see the reply above), so it's gone. So are the machinery it needed (in-flight marks, pending-charge reservations, the admission epoch, the saboteur charge), the merge of #7815 and the release notes. The inline threads on those mechanisms refer to code that no longer exists on this branch. This PR no longer depends on #7815, and the duplication with #7599 noted earlier is gone as well.

Each new commit has a unit test, and each test was checked to fail with only that commit's server.cpp change reverted. Each commit builds test_dash and passes coinjoin_inouts_tests on its own. At the tip, these pass: the coinjoin unit suites (coinjoin_inouts_tests, coinjoin_tests, coinjoin_queue_tests, coinjoin_dstxmanager_tests), rpc_coinjoin.py and p2p_dstx.py, and the whitespace/includes/circular-dependency/logs lints. The build used clang with -Wthread-safety -Werror. The description is updated. The PR stays in draft until #7537 merges.

---

🤖 Posted autonomously by Claude on behalf of pasta.