fix: reject invalid-denomination DSQUEUEs on mixing clients by sharing the server validation

src/coinjoin/coinjoin.cpp

@@ -6,6 +6,7 @@
 
 #include <bls/bls.h>
 #include <chainlock/chainlock.h>
+#include <evo/deterministicmns.h>
 #include <instantsend/instantsend.h>
 #include <util/helpers.h>
 
@@ -767,3 +768,43 @@ bool CoinJoin::ValidateFinalTxComposition(size_t nTotalInputs, size_t nTotalOutp
     return nSessionInputs >= nLargerOutputs * size_t(PROMOTION_RATIO) &&
            nSessionOutputs >= nLargerInputs * size_t(PROMOTION_RATIO);
 }
+
+CoinJoin::DSQueueValidation CoinJoin::ValidateDSQueue(CCoinJoinQueue& dsq, const CDeterministicMNList& mn_list,
+                                                      const std::function<bool(const CCoinJoinQueue&)>& is_duplicate)
+{
+    if (!IsValidDenomination(dsq.nDenom)) {
+        LogPrint(BCLog::COINJOIN, "DSQUEUE -- invalid denomination %d\n", dsq.nDenom);
+        return {.misbehavior = 10};
+    }
+
+    if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
+        return {.misbehavior = 100};
+    }
+
+    if (dsq.masternodeOutpoint.IsNull()) {
+        if (auto dmn = mn_list.GetValidMN(dsq.m_protxHash)) {
+            dsq.masternodeOutpoint = dmn->collateralOutpoint;
+        } else {
+            return {.misbehavior = 10};
+        }
+    }
+
+    if (is_duplicate(dsq)) return {};
+
+    LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString());
+
+    if (dsq.IsTimeOutOfBounds()) return {};
+
+    auto dmn = mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint);
+    if (!dmn) return {};
+
+    if (dsq.m_protxHash.IsNull()) {
+        dsq.m_protxHash = dmn->proTxHash;
+    }
+
+    if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
+        return {.misbehavior = 10};
+    }
+
+    return {.dmn = std::move(dmn)};
+}

src/coinjoin/coinjoin.h

@@ -8,6 +8,7 @@
 #include <coinjoin/common.h>
 
 #include <core_io.h>
+#include <evo/types.h>
 #include <kernel/cs_main.h> // IWYU pragma: export
 #include <netaddress.h>
 #include <primitives/block.h>
@@ -20,6 +21,7 @@
 #include <version.h>
 
 #include <atomic>
+#include <functional>
 #include <map>
 #include <optional>
 #include <utility>
@@ -29,6 +31,7 @@
 class Chainstate;
 class CBLSPublicKey;
 class CBlockIndex;
+class CDeterministicMNList;
 class ChainstateManager;
 class CTxMemPool;
 
@@ -531,6 +534,24 @@ namespace CoinJoin
      */
     bool ValidateFinalTxComposition(size_t nTotalInputs, size_t nTotalOutputs,
                                     size_t nLargerInputs, size_t nLargerOutputs);
+
+    struct DSQueueValidation {
+        //! The masternode that signed the queue, null if the queue was rejected
+        CDeterministicMNCPtr dmn{};
+        //! Score to penalize the sending peer with, 0 if the queue was accepted or the rejection
+        //! is not the peer's fault
+        int misbehavior{0};
+    };
+
+    /**
+     * Runs the DSQUEUE checks common to mixing clients and servers and fills in the queue's
+     * masternodeOutpoint from the masternode list.
+     * @param is_duplicate Called once the outpoint is known and before the signature is verified,
+     *                     so duplicates are dropped without a BLS check. Returning true rejects
+     *                     the queue without a penalty.
+     */
+    DSQueueValidation ValidateDSQueue(CCoinJoinQueue& dsq, const CDeterministicMNList& mn_list,
+                                      const std::function<bool(const CCoinJoinQueue&)>& is_duplicate);
 }
 
 class CDSTXManager

src/coinjoin/server.cpp

@@ -134,52 +134,20 @@ void CCoinJoinServer::ProcessDSQUEUE(NodeId from, CDataStream& vRecv)
 
     WITH_LOCK(cs_main, m_peer_manager->PeerEraseObjectRequest(from, CInv{MSG_DSQ, dsq.GetHash()}));
 
-    // Validate denomination first
-    if (!CoinJoin::IsValidDenomination(dsq.nDenom)) {
-        LogPrint(BCLog::COINJOIN, "DSQUEUE -- invalid denomination %d from peer %d\n", dsq.nDenom, from);
-        m_peer_manager->PeerMisbehaving(from, 10);
-        return;
-    }
-
-    if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
-        m_peer_manager->PeerMisbehaving(from, 100);
-        return;
-    }
-
     const auto tip_mn_list = m_dmnman.GetListAtChainTip();
-    if (dsq.masternodeOutpoint.IsNull()) {
-        if (auto dmn = tip_mn_list.GetValidMN(dsq.m_protxHash)) {
-            dsq.masternodeOutpoint = dmn->collateralOutpoint;
-        } else {
-            m_peer_manager->PeerMisbehaving(from, 10);
-            return;
-        }
-    }
-
-    {
-        const auto isDup = m_queueman.TryCheckDuplicate(dsq);
-        if (!isDup.has_value()) return;
+    const auto [dmn, misbehavior] = CoinJoin::ValidateDSQueue(dsq, tip_mn_list, [this, from](const CCoinJoinQueue& q) {
+        const auto isDup = m_queueman.TryCheckDuplicate(q);
+        if (!isDup.has_value()) return true;
         if (*isDup) {
-            LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", from, dsq.masternodeOutpoint.ToStringShort());
-            return;
+            LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", from, q.masternodeOutpoint.ToStringShort());
         }
-    }
-
-    LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString());
-
-    if (dsq.IsTimeOutOfBounds()) return;
-
-    auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint);
-    if (!dmn) return;
-
-    if (dsq.m_protxHash.IsNull()) {
-        dsq.m_protxHash = dmn->proTxHash;
-    }
-
-    if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
-        m_peer_manager->PeerMisbehaving(from, 10);
+        return *isDup;
+    });
+    if (misbehavior > 0) {
+        m_peer_manager->PeerMisbehaving(from, misbehavior);
         return;
     }
+    if (!dmn) return;
 
     if (!dsq.fReady) {
         //don't allow a few nodes to dominate the queuing process

src/coinjoin/walletman.cpp

@@ -258,50 +258,29 @@ MessageProcessingResult CJWalletManagerImpl::ProcessDSQueue(NodeId from, CConnma
     MessageProcessingResult ret{};
     ret.m_to_erase = CInv{MSG_DSQ, dsq.GetHash()};
 
-    if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
-        ret.m_error = MisbehavingError{100};
-        return ret;
-    }
-
     const auto tip_mn_list = m_dmnman.GetListAtChainTip();
-    if (dsq.masternodeOutpoint.IsNull()) {
-        if (auto dmn = tip_mn_list.GetValidMN(dsq.m_protxHash)) {
-            dsq.masternodeOutpoint = dmn->collateralOutpoint;
-        } else {
-            ret.m_error = MisbehavingError{10};
-            return ret;
-        }
-    }
 
     {
         LOCK(cs_ProcessDSQueue);
 
-        if (m_queueman->HasQueue(dsq.GetHash())) return ret;
-
-        if (m_queueman->HasQueueFromMasternode(dsq.masternodeOutpoint, dsq.fReady)) {
-            // no way the same mn can send another dsq with the same readiness this soon
-            LogPrint(BCLog::COINJOIN, /* Continued */
-                     "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n",
-                     from, dsq.masternodeOutpoint.ToStringShort());
+        const auto validation = CoinJoin::ValidateDSQueue(dsq, tip_mn_list, [this, from](const CCoinJoinQueue& q) {
+            if (m_queueman->HasQueue(q.GetHash())) return true;
+            if (m_queueman->HasQueueFromMasternode(q.masternodeOutpoint, q.fReady)) {
+                // no way the same mn can send another dsq with the same readiness this soon
+                LogPrint(BCLog::COINJOIN, /* Continued */
+                         "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n",
+                         from, q.masternodeOutpoint.ToStringShort());
+                return true;
+            }
+            return false;
+        });
+        if (validation.misbehavior > 0) {
+            ret.m_error = MisbehavingError{validation.misbehavior};
             return ret;
         }
-
-        LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString());
-
-        if (dsq.IsTimeOutOfBounds()) return ret;
-
-        auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint);
+        const auto& dmn = validation.dmn;
         if (!dmn) return ret;
 
-        if (dsq.m_protxHash.IsNull()) {
-            dsq.m_protxHash = dmn->proTxHash;
-        }
-
-        if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
-            ret.m_error = MisbehavingError{10};
-            return ret;
-        }
-
         // if the queue is ready, submit if we can
         if (dsq.fReady && ForAnyCJClientMan([&connman, &dmn](CCoinJoinClientManager& clientman) {
                 return clientman.TrySubmitDenominate(dmn->proTxHash, connman);

src/test/coinjoin_queue_tests.cpp

@@ -2,21 +2,107 @@
 // Distributed under the MIT/X11 software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <test/util/masternode.h>
 #include <test/util/setup_common.h>
 
 #include <active/masternode.h>
 #include <bls/bls.h>
 #include <coinjoin/coinjoin.h>
 #include <coinjoin/common.h>
+#include <coinjoin/walletman.h>
 #include <consensus/amount.h>
+#include <evo/deterministicmns.h>
+#include <masternode/meta.h>
+#include <masternode/sync.h>
+#include <net.h>
+#include <protocol.h>
+#include <script/standard.h>
+#include <streams.h>
+#include <validation.h>
 
 #include <uint256.h>
 
 #include <climits>
 #include <cstdint>
+#include <memory>
 
 #include <boost/test/unit_test.hpp>
 
+#ifdef ENABLE_WALLET
+namespace {
+// TestChainSetup only accepts checkpointed chain lengths, so 107 blocks plus -dip3params=109:500
+// is the shortest chain a ProRegTx can be mined on. Same setup as TestChainDIP3Setup in
+// evo_deterministicmns_tests.cpp.
+constexpr int DIP3_ACTIVATION_HEIGHT{109};
+
+//! A synced chain with one registered masternode whose operator key is known to the test, so
+//! DSQUEUE messages can be signed for real and pass every check that does not depend on their
+//! content.
+struct RegisteredMasternodeSetup : public TestChainSetup {
+    CBLSSecretKey operator_key;
+    uint256 protx_hash;
+
+    RegisteredMasternodeSetup() :
+        TestChainSetup(DIP3_ACTIVATION_HEIGHT - 2, CBaseChainParams::REGTEST, {"-dip3params=109:500"})
+    {
+        auto utxos = BuildSimpleUtxoMap(m_coinbase_txns);
+        // Activate DIP3, then register the masternode
+        MineBlock({});
+        CKey owner_key;
+        const auto protx = CreateProRegTx(*m_node.chainman, utxos, /*port=*/1,
+                                          GetScriptForDestination(PKHash(coinbaseKey.GetPubKey())), coinbaseKey,
+                                          owner_key, operator_key);
+        protx_hash = protx.GetHash();
+        MineBlock({protx});
+        BOOST_REQUIRE(m_node.dmnman->GetListAtChainTip().GetValidMN(protx_hash));
+
+        BOOST_REQUIRE(m_node.mn_metaman->LoadCache(/*load_cache=*/false));
+        // The bound is just a loop guard
+        for (int i = 0; i < 5 && !m_node.mn_sync->IsBlockchainSynced(); ++i) {
+            m_node.mn_sync->SwitchToNextAsset();
+        }
+        BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced());
+    }
+
+    void MineBlock(const std::vector<CMutableTransaction>& txns)
+    {
+        CreateAndProcessBlock(txns, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
+        m_node.dmnman->UpdatedBlockTip(WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip()));
+    }
+
+    CCoinJoinQueue MakeQueue(int denom) const
+    {
+        return CCoinJoinQueue{denom, COutPoint{}, protx_hash, GetAdjustedTime(), /*fReady=*/false};
+    }
+
+    static void Sign(CCoinJoinQueue& dsq, const CBLSSecretKey& key)
+    {
+        dsq.vchSig = key.Sign(dsq.GetSignatureHash(), /*specificLegacyScheme=*/false)
+                         .ToByteVector(/*specificLegacyScheme=*/false);
+    }
+};
+
+std::unique_ptr<CNode> MakePeer(NodeId id)
+{
+    in_addr peer_in_addr{};
+    peer_in_addr.s_addr = htonl(0x0a000001 + static_cast<uint32_t>(id));
+    auto peer{std::make_unique<CNode>(id,
+                                      /*sock=*/nullptr,
+                                      /*addrIn=*/CAddress{CService{peer_in_addr, 9999}, NODE_NETWORK},
+                                      /*nKeyedNetGroupIn=*/0,
+                                      /*nLocalHostNonceIn=*/0,
+                                      /*addrBindIn=*/CAddress{},
+                                      /*addrNameIn=*/std::string{},
+                                      /*conn_type_in=*/ConnectionType::INBOUND,
+                                      /*inbound_onion=*/false)};
+    peer->nVersion = PROTOCOL_VERSION;
+    peer->SetCommonVersion(PROTOCOL_VERSION);
+    peer->fSuccessfullyConnected = true;
+    return peer;
+}
+} // namespace
+#endif // ENABLE_WALLET
+
 BOOST_FIXTURE_TEST_SUITE(coinjoin_queue_tests, TestingSetup)
 
 static CBLSSecretKey MakeSecretKey()
@@ -260,4 +346,51 @@ BOOST_AUTO_TEST_CASE(queuemanager_try_check_duplicate)
     BOOST_CHECK(!is_dup(MakeQueue(denom, now, /*fReady=*/false, other_mn)));
 }
 
+#ifdef ENABLE_WALLET
+BOOST_FIXTURE_TEST_CASE(client_rejects_invalid_queues, RegisteredMasternodeSetup)
+{
+    auto& cj_walletman = *Assert(m_node.cj_walletman);
+    const auto peer{MakePeer(/*id=*/0)};
+    auto process = [&](const CCoinJoinQueue& dsq) {
+        CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
+        stream << dsq;
+        return cj_walletman.processMessage(*peer, m_node.chainman->ActiveChainstate(), *m_node.connman,
+                                           *m_node.mempool, NetMsgType::DSQUEUE, stream);
+    };
+    auto check_rejected = [&](CCoinJoinQueue dsq, const CBLSSecretKey& key, int score) {
+        Sign(dsq, key);
+        const auto result{process(dsq)};
+        BOOST_REQUIRE(result.m_error.has_value());
+        BOOST_CHECK_EQUAL(result.m_error->score, score);
+        BOOST_CHECK(!cj_walletman.hasQueue(dsq.GetHash()));
+    };
+
+    const int denom{CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination())};
+
+    BOOST_TEST_CONTEXT("invalid denomination") check_rejected(MakeQueue(/*denom=*/999), operator_key, 10);
+
+    auto ready_invalid{MakeQueue(/*denom=*/999)};
+    ready_invalid.fReady = true;
+    BOOST_TEST_CONTEXT("invalid denomination, ready") check_rejected(ready_invalid, operator_key, 10);
+
+    auto no_protx{MakeQueue(denom)};
+    no_protx.m_protxHash.SetNull();
+    BOOST_TEST_CONTEXT("no masternode") check_rejected(no_protx, operator_key, 100);
+
+    auto unknown_protx{MakeQueue(denom)};
+    unknown_protx.m_protxHash = uint256::ONE;
+    BOOST_TEST_CONTEXT("unknown masternode") check_rejected(unknown_protx, operator_key, 10);
+
+    CBLSSecretKey other_key;
+    other_key.MakeNewKey();
+    BOOST_TEST_CONTEXT("wrong signer") check_rejected(MakeQueue(denom), other_key, 10);
+
+    // A valid queue from the same masternode is accepted
+    auto valid{MakeQueue(denom)};
+    Sign(valid, operator_key);
+    BOOST_CHECK(!process(valid).m_error.has_value());
+    BOOST_CHECK(cj_walletman.hasQueue(valid.GetHash()));
+}
+#endif // ENABLE_WALLET
+
 BOOST_AUTO_TEST_SUITE_END()