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

src/coinjoin/server.cpp

@@ -284,7 +284,10 @@ void CCoinJoinServer::ProcessDSSIGNFINALTX(CNode& peer, CDataStream& vRecv)
             if (!AddScriptSig(txin)) {
                 LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- AddScriptSig() failed at %d/%d, session: %d\n", nTxInIndex,
                          nTxInsCount, session_id);
-                RelayStatus(STATUS_REJECTED);
+                // Only the sender is at fault, so only the sender is rejected. A session-wide
+                // rejection would let one participant abort the session for everyone, and those
+                // that obeyed it would be counted as non-signers at the timeout.
+                PushStatus(peer, STATUS_REJECTED, ERR_INVALID_INPUT);
                 return;
             }
             LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- AddScriptSig() %d/%d success\n", nTxInIndex, nTxInsCount);
@@ -540,56 +543,66 @@ CTransactionRef CCoinJoinServer::SelectCollateralToCharge()
     //we don't need to charge collateral for every offence.
     if (GetRand<int>(/*nMax=*/100) > 33) return {};
 
-    std::vector<CTransactionRef> vecOffendersCollaterals;
     const PoolState state{nState};
-    const size_t nSessionCollaterals{m_session_collaterals.size()};
+    auto offenders{FindSessionOffenders(state, m_session_collaterals.txs(), vecEntries)};
+    if (!offenders.collaterals.empty()) {
+        LogPrint(BCLog::COINJOIN, /* Continued */
+                 "CCoinJoinServer::SelectCollateralToCharge -- found %d of %d participants uncooperative (didn't %s)\n",
+                 offenders.collaterals.size(), offenders.participants,
+                 (state == POOL_STATE_SIGNING) ? "sign" : "send transaction");
+    }
+
+    switch (GetOffenderChargeRate(offenders)) {
+    case OffenderChargeRate::NONE:
+        return {};
+    case OffenderChargeRate::REDUCED:
+        if (GetRand<int>(/*nMax=*/100) > 33) return {};
+        break;
+    case OffenderChargeRate::NORMAL:
+        break;
+    }
+
+    // charge one of the offenders randomly
+    Shuffle(offenders.collaterals.begin(), offenders.collaterals.end(), FastRandomContext());
+
+    LogPrint(BCLog::COINJOIN, /* Continued */
+             "CCoinJoinServer::SelectCollateralToCharge -- found uncooperative node (didn't %s transaction), charging "
+             "fees: %s",
+             (state == POOL_STATE_SIGNING) ? "sign" : "send", offenders.collaterals[0]->ToString());
+    m_collaterals_being_charged.push_back(offenders.collaterals[0]);
+    return offenders.collaterals[0];
+}
 
+CoinJoinSessionOffenders FindSessionOffenders(PoolState state, const std::vector<CTransactionRef>& session_collaterals,
+                                              const std::vector<CCoinJoinEntry>& entries)
+{
+    CoinJoinSessionOffenders offenders;
     if (state == POOL_STATE_ACCEPTING_ENTRIES) {
-        for (const auto& txCollateral : m_session_collaterals.txs()) {
-            bool fFound = std::ranges::any_of(vecEntries, [&txCollateral](const auto& entry) {
+        offenders.participants = session_collaterals.size();
+        for (const auto& txCollateral : session_collaterals) {
+            // This queue entry didn't send us the promised transaction
+            const bool fFound = std::ranges::any_of(entries, [&txCollateral](const auto& entry) {
                 return *entry.txCollateral == *txCollateral;
             });
-
-            // This queue entry didn't send us the promised transaction
-            if (!fFound) {
-                LogPrint(BCLog::COINJOIN, /* Continued */
-                         "CCoinJoinServer::SelectCollateralToCharge -- found uncooperative node (didn't send "
-                         "transaction), found offence\n");
-                vecOffendersCollaterals.push_back(txCollateral);
-            }
+            if (!fFound) offenders.collaterals.push_back(txCollateral);
         }
     } else if (state == POOL_STATE_SIGNING) {
-        // who didn't sign?
-        for (const auto& entry : vecEntries) {
-            for (const auto& txdsin : entry.vecTxDSIn) {
-                if (!txdsin.fHasSig) {
-                    LogPrint(BCLog::COINJOIN, /* Continued */
-                             "CCoinJoinServer::SelectCollateralToCharge -- found uncooperative node (didn't sign), "
-                             "found offence\n");
-                    vecOffendersCollaterals.push_back(entry.txCollateral);
-                }
+        offenders.participants = entries.size();
+        for (const auto& entry : entries) {
+            if (std::ranges::any_of(entry.vecTxDSIn, [](const auto& txdsin) { return !txdsin.fHasSig; })) {
+                offenders.collaterals.push_back(entry.txCollateral);
             }
         }
     }
+    return offenders;
+}
 
-    // no offences found
-    if (vecOffendersCollaterals.empty()) return {};
-
-    //mostly offending? Charge sometimes
-    if (vecOffendersCollaterals.size() >= nSessionCollaterals - 1 && GetRand<int>(/*nMax=*/100) > 33) return {};
-
-    //everyone is an offender? That's not right
-    if (vecOffendersCollaterals.size() >= nSessionCollaterals) return {};
-
-    //charge one of the offenders randomly
-    Shuffle(vecOffendersCollaterals.begin(), vecOffendersCollaterals.end(), FastRandomContext());
-
-    LogPrint(BCLog::COINJOIN, /* Continued */
-             "CCoinJoinServer::SelectCollateralToCharge -- found uncooperative node (didn't %s transaction), charging "
-             "fees: %s",
-             (state == POOL_STATE_SIGNING) ? "sign" : "send", vecOffendersCollaterals[0]->ToString());
-    m_collaterals_being_charged.push_back(vecOffendersCollaterals[0]);
-    return vecOffendersCollaterals[0];
+OffenderChargeRate GetOffenderChargeRate(const CoinJoinSessionOffenders& offenders)
+{
+    const size_t failed{offenders.collaterals.size()};
+    if (failed == 0 || failed >= offenders.participants) return OffenderChargeRate::NONE;
+    if (failed == offenders.participants - 1) return OffenderChargeRate::REDUCED;
+    return OffenderChargeRate::NORMAL;
 }
 
 /*
@@ -1308,17 +1321,16 @@ void CCoinJoinServer::RelayFinalTransaction(const CTransaction& txFinal)
         __func__, nSessionID, nSessionDenom, CoinJoin::DenominationToString(nSessionDenom));
 
     // final mixing tx with empty signatures should be relayed to mixing participants only
-    for (const auto& entry : vecEntries) {
-        bool fOk = connman.ForNode(entry.addr, [&txFinal, this](CNode* pnode) {
+    const bool all_reached = std::ranges::all_of(vecEntries, [&txFinal, this](const auto& entry) {
+        return connman.ForNode(entry.addr, [&txFinal, this](CNode* pnode) {
             CNetMsgMaker msgMaker(pnode->GetCommonVersion());
             connman.PushMessage(pnode, msgMaker.Make(NetMsgType::DSFINALTX, nSessionID.load(), txFinal));
             return true;
         });
-        if (!fOk) {
-            // no such node? maybe this client disconnected or our own connection went down
-            RelayStatus(STATUS_REJECTED);
-            break;
-        }
+    });
+    if (!all_reached) {
+        // no such node? maybe this client disconnected or our own connection went down
+        RelayStatus(STATUS_REJECTED);
     }
 }
 
@@ -1358,11 +1370,11 @@ void CCoinJoinServer::RelayStatus(PoolStatusUpdate nStatusUpdate, PoolMessage nM
         });
     }
 
-    if (nDisconnected == vecEntries.size()) {
-        // all clients disconnected, there is probably some issues with our own connection
-        // do not charge any fees, just reset the pool
-        SetNull();
-    }
+    // The participants told to terminate release their inputs and stop, so the session can no
+    // longer complete. End it now without charging anyone: at the timeout they would look like
+    // participants that never sent or signed, and a peer we cannot reach is no evidence of
+    // misbehavior - our own connection may be the one that failed.
+    SetNull();
 }
 
 void CCoinJoinServer::RelayCompletedTransaction(int session_id, const std::vector<CService>& participants,

src/coinjoin/server.h

@@ -49,6 +49,34 @@ struct CoinJoinSessionProgress {
  */
 bool ShouldResetTimedOutSession(const CoinJoinSessionProgress& session);
 
+/** The participants of a session that did not do their part at its current stage */
+struct CoinJoinSessionOffenders {
+    //! The collateral of each participant that failed, once per participant
+    std::vector<CTransactionRef> collaterals;
+    //! How many participants were expected to act at this stage
+    size_t participants{0};
+};
+
+/**
+ * Who failed to do their part at this stage of a session? While entries are being accepted, every
+ * admitted participant had to send one. While signing, only the participants whose entries made it
+ * into the final transaction were asked to sign: a session finalized on timeout still holds the
+ * collaterals of those that never sent one. A participant counts once however many of its inputs
+ * are unsigned.
+ */
+CoinJoinSessionOffenders FindSessionOffenders(PoolState state, const std::vector<CTransactionRef>& session_collaterals,
+                                              const std::vector<CCoinJoinEntry>& entries);
+
+/** How readily one of a session's offenders may be charged, on top of the random sampling every charge goes through */
+enum class OffenderChargeRate {
+    //! No offence, or every participant failed, which points at the masternode rather than at any one of them
+    NONE,
+    //! All but one participant failed: charge only some of the time
+    REDUCED,
+    NORMAL,
+};
+OffenderChargeRate GetOffenderChargeRate(const CoinJoinSessionOffenders& offenders);
+
 /** Used to keep track of current status of mixing pool
  */
 class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler

src/test/coinjoin_inouts_tests.cpp

@@ -17,6 +17,7 @@
 #include <protocol.h>
 #include <script/script.h>
 #include <streams.h>
+#include <test/util/net.h>
 #include <test/util/setup_common.h>
 #include <tinyformat.h>
 #include <uint256.h>
@@ -29,6 +30,7 @@
 #include <algorithm>
 #include <array>
 #include <cstdint>
+#include <iterator>
 #include <memory>
 #include <vector>
 
@@ -517,6 +519,184 @@ BOOST_AUTO_TEST_CASE(server_timeout_resets_only_sessions_that_cannot_advance)
     }
 }
 
+BOOST_AUTO_TEST_CASE(server_invalid_signature_rejects_only_its_sender)
+{
+    BOOST_REQUIRE(m_node.mn_sync);
+    m_node.mn_sync->SwitchToNextAsset();
+    BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced());
+
+    CActiveMasternodeManager mn_activeman(*Assert(m_node.connman), *Assert(m_node.dmnman), MakeSecretKey());
+    TestableCoinJoinServer server(m_node.peerman.get(), *Assert(m_node.chainman), *Assert(m_node.connman),
+                                  *Assert(m_node.dmnman), *Assert(m_node.dstxman), *Assert(m_node.mn_metaman),
+                                  *Assert(m_node.mempool), mn_activeman, *Assert(m_node.mn_sync),
+                                  *Assert(m_node.isman));
+    auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
+
+    CNode& sender = *MakePeer(/*id=*/7, /*ipv4=*/0x0a000001).release();
+    CNode& other = *MakePeer(/*id=*/8, /*ipv4=*/0x0a000002).release();
+    for (CNode* node : {&sender, &other}) {
+        node->fSuccessfullyConnected = true;
+        connman.AddTestNode(*node);
+        server.SeedParticipant(node->addr);
+    }
+    server.EnterSigningState();
+
+    std::vector<CService> status_recipients;
+    const auto capture_orig{CaptureMessage};
+    m_node.args->ForceSetArg("-capturemessages", "1");
+    CaptureMessage = [&status_recipients](const CAddress& addr, const std::string& msg_type,
+                                          Span<const unsigned char>, bool is_incoming) {
+        if (!is_incoming && msg_type == NetMsgType::DSSTATUSUPDATE) status_recipients.push_back(addr);
+    };
+
+    // A signature that fails validation is the sender's own doing. Only the sender is rejected:
+    // the other participant is not told to abandon a session it can still complete.
+    CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
+    stream << std::vector<CTxIn>{CTxIn{COutPoint{uint256::ONE, 0}}};
+    BOOST_CHECK_NO_THROW(server.ProcessMessage(sender, NetMsgType::DSSIGNFINALTX, stream));
+
+    CaptureMessage = capture_orig;
+    m_node.args->ForceSetArg("-capturemessages", "0");
+
+    BOOST_CHECK(std::ranges::count(status_recipients, sender.addr) == 1);
+    BOOST_CHECK(std::ranges::count(status_recipients, other.addr) == 0);
+    BOOST_CHECK_EQUAL(server.GetState(), int{POOL_STATE_SIGNING});
+    BOOST_CHECK_EQUAL(server.GetEntriesCount(), 2);
+
+    connman.ClearTestNodes();
+}
+
+BOOST_AUTO_TEST_CASE(server_unreachable_participant_ends_session)
+{
+    CActiveMasternodeManager mn_activeman(*Assert(m_node.connman), *Assert(m_node.dmnman), MakeSecretKey());
+    auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
+
+    // Every participant has submitted an entry, so the final transaction is relayed to them. If
+    // one of them can no longer be reached, the others are told to abandon the session; they
+    // cannot be charged at its timeout for obeying, so it must end right away. If everyone can be
+    // reached, it moves on to signing.
+    for (const bool all_reachable : {true, false}) {
+        BOOST_TEST_CONTEXT("all_reachable=" << all_reachable)
+        {
+            TestableCoinJoinServer server(m_node.peerman.get(), *Assert(m_node.chainman), *Assert(m_node.connman),
+                                          *Assert(m_node.dmnman), *Assert(m_node.dstxman),
+                                          *Assert(m_node.mn_metaman), *Assert(m_node.mempool), mn_activeman,
+                                          *Assert(m_node.mn_sync), *Assert(m_node.isman));
+            for (int i = 0; i < CoinJoin::GetMinPoolParticipants(); ++i) {
+                auto peer = MakePeer(/*id=*/i, /*ipv4=*/0x0a000001 + i);
+                CMutableTransaction collateral;
+                collateral.vin.emplace_back(COutPoint{uint256::ONE, static_cast<uint32_t>(i)});
+                server.SeedSessionCollateral(collateral, CoinJoin::MixShape::STANDARD);
+                CCoinJoinEntry entry{{CTxDSIn{CTxIn{COutPoint{uint256::TWO, static_cast<uint32_t>(i)}}, P2PKHScript(), 0}},
+                                     {CTxOut{CoinJoin::GetSmallestDenomination(), P2PKHScript()}},
+                                     CTransaction{collateral}};
+                entry.addr = peer->addr;
+                server.SeedEntry(std::move(entry));
+                if (all_reachable || i == 0) {
+                    peer->fSuccessfullyConnected = true;
+                    connman.AddTestNode(*peer.release());
+                }
+            }
+            server.EnterAcceptingEntriesState();
+
+            server.CreateFinalTransaction(/*session_id=*/1, /*charge_fees=*/false);
+            BOOST_CHECK_EQUAL(server.GetState(), all_reachable ? int{POOL_STATE_SIGNING} : int{POOL_STATE_IDLE});
+
+            connman.ClearTestNodes();
+        }
+    }
+}
+
+BOOST_AUTO_TEST_CASE(server_charges_only_attributable_offenders)
+{
+    struct Case {
+        std::string name;
+        PoolState state;
+        int collaterals;
+        //! The submitted entries, entry i belonging to collateral i: {signed inputs, inputs}
+        std::vector<std::pair<int, int>> entries;
+        //! The collaterals expected to be found offending
+        std::vector<int> offenders;
+        size_t participants;
+        OffenderChargeRate rate;
+    };
+    const std::pair<int, int> sent{0, 1}, signed_entry{1, 1}, unsigned_entry{0, 1};
+    const std::vector<Case> cases{
+        {"accepting, two entries missing", POOL_STATE_ACCEPTING_ENTRIES, 5, {sent, sent, sent}, {3, 4}, 5, OffenderChargeRate::NORMAL},
+        {"accepting, every entry in", POOL_STATE_ACCEPTING_ENTRIES, 3, {sent, sent, sent}, {}, 3, OffenderChargeRate::NONE},
+        {"accepting, all entries but one missing", POOL_STATE_ACCEPTING_ENTRIES, 3, {sent}, {1, 2}, 3, OffenderChargeRate::REDUCED},
+        {"accepting, no entry at all", POOL_STATE_ACCEPTING_ENTRIES, 3, {}, {0, 1, 2}, 3, OffenderChargeRate::NONE},
+        // A single non-signer however many inputs it holds. Counted once per input it looked like every
+        // participant had failed, which exempts the session from charging.
+        {"signing, one participant leaves all of its inputs unsigned",
+         POOL_STATE_SIGNING,
+         4,
+         {{0, 4}, signed_entry, signed_entry, signed_entry},
+         {0},
+         4,
+         OffenderChargeRate::NORMAL},
+        {"signing, one participant signs only some of its inputs",
+         POOL_STATE_SIGNING,
+         4,
+         {{2, 3}, signed_entry, signed_entry, signed_entry},
+         {0},
+         4,
+         OffenderChargeRate::NORMAL},
+        {"signing, everyone signed", POOL_STATE_SIGNING, 3, {signed_entry, signed_entry, signed_entry}, {}, 3, OffenderChargeRate::NONE},
+        // Finalized on timeout without the last two participants' entries, so only the first two were asked to
+        // sign. Every one of them failing points at the masternode rather than at any one of them.
+        {"signing after a timeout, every participant asked to sign fails",
+         POOL_STATE_SIGNING,
+         4,
+         {unsigned_entry, unsigned_entry},
+         {0, 1},
+         2,
+         OffenderChargeRate::NONE},
+        {"signing after a timeout, all signers but one fail",
+         POOL_STATE_SIGNING,
+         5,
+         {unsigned_entry, unsigned_entry, signed_entry},
+         {0, 1},
+         3,
+         OffenderChargeRate::REDUCED},
+        {"queue", POOL_STATE_QUEUE, 3, {}, {}, 0, OffenderChargeRate::NONE},
+    };
+
+    for (const auto& [name, state, n_collaterals, entry_shapes, expected_offenders, participants, rate] : cases) {
+        BOOST_TEST_CONTEXT(name)
+        {
+            std::vector<CTransactionRef> collaterals;
+            for (int i = 0; i < n_collaterals; ++i) {
+                CMutableTransaction collateral;
+                collateral.vin.emplace_back(COutPoint{uint256::ONE, static_cast<uint32_t>(i)});
+                collaterals.push_back(MakeTransactionRef(collateral));
+            }
+            std::vector<CCoinJoinEntry> entries;
+            for (size_t i = 0; i < entry_shapes.size(); ++i) {
+                const auto [n_signed, n_inputs] = entry_shapes[i];
+                std::vector<CTxDSIn> inputs;
+                for (int j = 0; j < n_inputs; ++j) {
+                    inputs.emplace_back(CTxIn{COutPoint{uint256::TWO, static_cast<uint32_t>(i * 16 + j)}},
+                                        P2PKHScript(), 0);
+                    inputs.back().fHasSig = j < n_signed;
+                }
+                entries.emplace_back(inputs, std::vector<CTxOut>{}, *collaterals[i]);
+            }
+
+            const auto offenders{FindSessionOffenders(state, collaterals, entries)};
+            // An entry carries its own copy of the collateral, so compare transactions, not pointers
+            std::vector<uint256> found, expected;
+            std::ranges::transform(offenders.collaterals, std::back_inserter(found),
+                                   [](const auto& collateral) { return collateral->GetHash(); });
+            std::ranges::transform(expected_offenders, std::back_inserter(expected),
+                                   [&collaterals](int i) { return collaterals[i]->GetHash(); });
+            BOOST_CHECK_EQUAL_COLLECTIONS(found.begin(), found.end(), expected.begin(), expected.end());
+            BOOST_CHECK_EQUAL(offenders.participants, participants);
+            BOOST_CHECK(GetOffenderChargeRate(offenders) == rate);
+        }
+    }
+}
+
 BOOST_AUTO_TEST_CASE(entry_deserializes_vectors_through_wire_cap)
 {
     const size_t wire_cap{CoinJoin::GetMaxPoolInputOutputCount()};