Cxx17 refac

src/coinjoin/client.h

@@ -9,6 +9,7 @@
 #include <coinjoin/coinjoin.h>
 
 #include <utility>
+#include <optional>
 
 class CDeterministicMN;
 using CDeterministicMNCPtr = std::shared_ptr<const CDeterministicMN>;
@@ -52,9 +53,9 @@ class CPendingDsaRequest
     {
     }
 
-    CService GetAddr() const { return addr; }
-    CCoinJoinAccept GetDSA() const { return dsa; }
-    bool IsExpired() const { return GetTime() - nTimeCreated > TIMEOUT; }
+    [[nodiscard]] CService GetAddr() const { return addr; }
+    [[nodiscard]] CCoinJoinAccept GetDSA() const { return dsa; }
+    [[nodiscard]] bool IsExpired() const { return GetTime() - nTimeCreated > TIMEOUT; }
 
     friend bool operator==(const CPendingDsaRequest& a, const CPendingDsaRequest& b)
     {
@@ -99,10 +100,15 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
     bool JoinExistingQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman);
     bool StartNewQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman);
 
-    /// step 0: select denominated inputs and txouts
-    bool SelectDenominate(std::string& strErrorRet, std::vector<CTxDSIn>& vecTxDSInRet);
+    /**
+     * step 0: select denominated inputs and txouts
+     * @return pair containing optional vector of selected inputs (CTxDSIn), and an error string which is only set
+     *         if returned vector is nullopt
+     */
+    std::pair<std::optional<std::vector<CTxDSIn>>, std::string> SelectDenominate();
     /// step 1: prepare denominated inputs and outputs
-    bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector<CTxDSIn>& vecTxDSIn, std::vector<std::pair<CTxDSIn, CTxOut> >& vecPSInOutPairsRet, bool fDryRun = false);
+    // TODO factor out strErrorRet
+    std::optional<std::vector<std::pair<CTxDSIn, CTxOut>>> PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector<CTxDSIn>& vecTxDSIn, bool fDryRun = false);
     /// step 2: send denominated inputs and outputs prepared in step 1
     bool SendDenominate(const std::vector<std::pair<CTxDSIn, CTxOut> >& vecPSInOutPairsIn, CConnman& connman);
 
@@ -141,7 +147,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
 
     std::string GetStatus(bool fWaitForBlock) const;
 
-    bool GetMixingMasternodeInfo(CDeterministicMNCPtr& ret) const;
+    std::optional<CDeterministicMNCPtr> GetMixingMasternodeInfo() const;
 
     /// Passively run mixing in the background according to the configuration in settings
     bool DoAutomaticDenominating(CConnman& connman, bool fDryRun = false);
@@ -215,7 +221,7 @@ class CCoinJoinClientManager
     std::string GetStatuses();
     std::string GetSessionDenoms();
 
-    bool GetMixingMasternodesInfo(std::vector<CDeterministicMNCPtr>& vecDmnsRet) const;
+    std::vector<CDeterministicMNCPtr> GetMixingMasternodesInfo() const;
 
     /// Passively run mixing in the background according to the configuration in settings
     bool DoAutomaticDenominating(CConnman& connman, bool fDryRun = false);

src/coinjoin/coinjoin.cpp

@@ -175,20 +175,19 @@ void CCoinJoinBaseManager::CheckQueue()
     }
 }
 
-bool CCoinJoinBaseManager::GetQueueItemAndTry(CCoinJoinQueue& dsqRet)
+std::optional<CCoinJoinQueue> CCoinJoinBaseManager::GetQueueItemAndTry()
 {
     TRY_LOCK(cs_vecqueue, lockDS);
-    if (!lockDS) return false; // it's ok to fail here, we run this quite frequently
+    if (!lockDS) return std::nullopt; // it's ok to fail here, we run this quite frequently
 
     for (auto& dsq : vecCoinJoinQueue) {
         // only try each queue once
         if (dsq.fTried || dsq.IsTimeOutOfBounds()) continue;
         dsq.fTried = true;
-        dsqRet = dsq;
-        return true;
+        return {dsq};
     }
 
-    return false;
+    return std::nullopt;
 }
 
 std::string CCoinJoinBaseSession::GetStateString() const
@@ -209,52 +208,43 @@ std::string CCoinJoinBaseSession::GetStateString() const
     }
 }
 
-bool CCoinJoinBaseSession::IsValidInOuts(const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const
+std::tuple<bool /*success*/, bool /*eat_fee*/, PoolMessage> CCoinJoinBaseSession::IsValidInOuts(const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout) const
 {
     AssertLockHeld(cs_main);
 
     std::set<CScript> setScripPubKeys;
-    nMessageIDRet = MSG_NOERR;
-    if (fConsumeCollateralRet) *fConsumeCollateralRet = false;
 
     if (vin.size() != vout.size()) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: inputs vs outputs size mismatch! %d vs %d\n", __func__, vin.size(), vout.size());
-        nMessageIDRet = ERR_SIZE_MISMATCH;
-        if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
-        return false;
+        return {false, true, ERR_SIZE_MISMATCH};
     }
 
-    auto checkTxOut = [&](const CTxOut& txout) {
-        int nDenom = CCoinJoin::AmountToDenomination(txout.nValue);
-        if (nDenom != nSessionDenom) {
+    // Any checkTxOut failure will result in collateral consumption
+    auto checkTxOut = [&](const CTxOut& txout) -> std::pair<bool /*success*/, PoolMessage> {
+        if (int nDenom = CCoinJoin::AmountToDenomination(txout.nValue); nDenom != nSessionDenom) {
             LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- ERROR: incompatible denom %d (%s) != nSessionDenom %d (%s)\n",
                     nDenom, CCoinJoin::DenominationToString(nDenom), nSessionDenom, CCoinJoin::DenominationToString(nSessionDenom));
-            nMessageIDRet = ERR_DENOM;
-            if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
-            return false;
+            return {false, ERR_DENOM};
         }
         if (!txout.scriptPubKey.IsPayToPublicKeyHash()) {
             LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- ERROR: invalid script! scriptPubKey=%s\n", ScriptToAsmStr(txout.scriptPubKey));
-            nMessageIDRet = ERR_INVALID_SCRIPT;
-            if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
-            return false;
+            return {false, ERR_INVALID_SCRIPT};
         }
         if (!setScripPubKeys.insert(txout.scriptPubKey).second) {
             LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- ERROR: already have this script! scriptPubKey=%s\n", ScriptToAsmStr(txout.scriptPubKey));
-            nMessageIDRet = ERR_ALREADY_HAVE;
-            if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
-            return false;
+            return {false, ERR_ALREADY_HAVE};
         }
         // IsPayToPublicKeyHash() above already checks for scriptPubKey size,
         // no need to double-check, hence no usage of ERR_NON_STANDARD_PUBKEY
-        return true;
+        return {true, MSG_NOERR};
     };
 
     CAmount nFees{0};
 
     for (const auto& txout : vout) {
-        if (!checkTxOut(txout)) {
-            return false;
+        auto [success, poolMessage] = checkTxOut(txout);
+        if (!success) {
+            return {false, true, poolMessage};
         }
         nFees -= txout.nValue;
     }
@@ -266,21 +256,19 @@ bool CCoinJoinBaseSession::IsValidInOuts(const std::vector<CTxIn>& vin, const st
 
         if (txin.prevout.IsNull()) {
             LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: invalid input!\n", __func__);
-            nMessageIDRet = ERR_INVALID_INPUT;
-            if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
-            return false;
+            return {false, true, ERR_INVALID_INPUT};
         }
 
         Coin coin;
         if (!viewMemPool.GetCoin(txin.prevout, coin) || coin.IsSpent() ||
             (coin.nHeight == MEMPOOL_HEIGHT && !llmq::quorumInstantSendManager->IsLocked(txin.prevout.hash))) {
             LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: missing, spent or non-locked mempool input! txin=%s\n", __func__, txin.ToString());
-            nMessageIDRet = ERR_MISSING_TX;
-            return false;
+            return {false, false, ERR_MISSING_TX};
         }
 
-        if (!checkTxOut(coin.out)) {
-            return false;
+        auto [success, poolMessage] = checkTxOut(coin.out);
+        if (!success) {
+            return {false, true, poolMessage};
         }
 
         nFees += coin.out.nValue;
@@ -290,11 +278,10 @@ bool CCoinJoinBaseSession::IsValidInOuts(const std::vector<CTxIn>& vin, const st
     // no need to double-check. If not, we are doing something wrong, bail out.
     if (nFees != 0) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: non-zero fees! fees: %lld\n", __func__, nFees);
-        nMessageIDRet = ERR_FEES;
-        return false;
+        return {false, false, ERR_FEES};
     }
 
-    return true;
+    return {false, false, MSG_NOERR};
 }
 
 // Definitions for static data members
@@ -457,9 +444,7 @@ CAmount CCoinJoin::DenominationToAmount(int nDenom)
 */
 std::string CCoinJoin::DenominationToString(int nDenom)
 {
-    CAmount nDenomAmount = DenominationToAmount(nDenom);
-
-    switch (nDenomAmount) {
+    switch (CAmount nDenomAmount = DenominationToAmount(nDenom)) {
         case  0: return "N/A";
         case -1: return "out-of-bounds";
         case -2: return "non-denom";

src/coinjoin/coinjoin.h

@@ -240,7 +240,7 @@ class CCoinJoinQueue
         }
     }
 
-    uint256 GetSignatureHash() const;
+    [[nodiscard]] uint256 GetSignatureHash() const;
     /** Sign this mixing transaction
      *  return true if all conditions are met:
      *     1) we have an active Masternode,
@@ -250,14 +250,14 @@ class CCoinJoinQueue
      */
     bool Sign();
     /// Check if we have a valid Masternode address
-    bool CheckSignature(const CBLSPublicKey& blsPubKey) const;
+    [[nodiscard]] bool CheckSignature(const CBLSPublicKey& blsPubKey) const;
 
     bool Relay(CConnman& connman);
 
     /// Check if a queue is too old or too far into the future
-    bool IsTimeOutOfBounds() const;
+    [[nodiscard]] bool IsTimeOutOfBounds() const;
 
-    std::string ToString() const
+    [[nodiscard]] std::string ToString() const
     {
         return strprintf("nDenom=%d, nTime=%lld, fReady=%s, fTried=%s, masternode=%s",
             nDenom, nTime, fReady ? "true" : "false", fTried ? "true" : "false", masternodeOutpoint.ToStringShort());
@@ -324,14 +324,14 @@ class CCoinJoinBroadcastTx
         return *this != CCoinJoinBroadcastTx();
     }
 
-    uint256 GetSignatureHash() const;
+    [[nodiscard]] uint256 GetSignatureHash() const;
 
     bool Sign();
-    bool CheckSignature(const CBLSPublicKey& blsPubKey) const;
+    [[nodiscard]] bool CheckSignature(const CBLSPublicKey& blsPubKey) const;
 
     void SetConfirmedHeight(int nConfirmedHeightIn) { nConfirmedHeight = nConfirmedHeightIn; }
     bool IsExpired(const CBlockIndex* pindex) const;
-    bool IsValidStructure() const;
+    [[nodiscard]] bool IsValidStructure() const;
 };
 
 // base class
@@ -351,7 +351,7 @@ class CCoinJoinBaseSession
 
     void SetNull();
 
-    bool IsValidInOuts(const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    std::tuple<bool /*success*/, bool /*eat_fee*/, PoolMessage> IsValidInOuts(const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
 public:
     int nSessionDenom; // Users must submit a denom matching this
@@ -388,8 +388,8 @@ class CCoinJoinBaseManager
     CCoinJoinBaseManager() :
         vecCoinJoinQueue() {}
 
-    int GetQueueSize() const { LOCK(cs_vecqueue); return vecCoinJoinQueue.size(); }
-    bool GetQueueItemAndTry(CCoinJoinQueue& dsqRet);
+    uint64_t GetQueueSize() const { LOCK(cs_vecqueue); return vecCoinJoinQueue.size(); }
+    std::optional<CCoinJoinQueue> GetQueueItemAndTry();
 };
 
 // helper class