Refactor: minor misc coinjoin refactoring (#5053)

* refactor: add more [[nodiscard]]

* refactor: drop unneeded .get()

* refactor: use functional style int cast

* refactor: use std::numeric_limits::max()

* refactor: use GetRand instead of GetRandInt

src/coinjoin/client.cpp

@@ -945,7 +945,7 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(CConnman& connman, bool fDr
     bool fResult = true;
     AssertLockNotHeld(cs_deqsessions);
     LOCK(cs_deqsessions);
-    if ((int)deqSessions.size() < CCoinJoinClientOptions::GetSessions()) {
+    if (int(deqSessions.size()) < CCoinJoinClientOptions::GetSessions()) {
         deqSessions.emplace_back(mixingWallet);
     }
     for (auto& session : deqSessions) {
@@ -1497,7 +1497,7 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx
         return false;
     }
 
-    const auto& output = vCoins.at(GetRandInt(vCoins.size()));
+    const auto& output = vCoins.at(GetRand(vCoins.size()));
     const CTxOut txout = output.tx->tx->vout[output.i];
 
     txCollateral.vin.clear();

src/coinjoin/client.h

@@ -49,9 +49,9 @@ public:
     {
     }
 
-    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)
     {

src/coinjoin/coinjoin.h

@@ -218,7 +218,7 @@ public:
         }
     }
 
-    uint256 GetSignatureHash() const;
+    [[nodiscard]] uint256 GetSignatureHash() const;
     /** Sign this mixing transaction
      *  return true if all conditions are met:
      *     1) we have an active Masternode,
@@ -228,14 +228,14 @@ public:
      */
     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(int64_t current_time = GetAdjustedTime()) const;
+    [[nodiscard]] bool IsTimeOutOfBounds(int64_t current_time = GetAdjustedTime()) 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());
@@ -296,14 +296,14 @@ public:
         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 llmq::CChainLocksHandler& clhandler) const;
-    bool IsValidStructure() const;
+    [[nodiscard]] bool IsValidStructure() const;
 };
 
 // base class

src/coinjoin/util.cpp

@@ -113,9 +113,9 @@ CTransactionBuilder::CTransactionBuilder(std::shared_ptr<CWallet> pwalletIn, con
     tallyItem(tallyItemIn)
 {
     // Generate a feerate which will be used to consider if the remainder is dust and will go into fees or not
-    coinControl.m_discard_feerate = ::GetDiscardRate(*pwallet.get());
+    coinControl.m_discard_feerate = ::GetDiscardRate(*pwallet);
     // Generate a feerate which will be used by calculations of this class and also by CWallet::CreateTransaction
-    coinControl.m_feerate = std::max(::feeEstimator.estimateSmartFee((int)pwallet->m_confirm_target, nullptr, true), pwallet->m_pay_tx_fee);
+    coinControl.m_feerate = std::max(::feeEstimator.estimateSmartFee(int(pwallet->m_confirm_target), nullptr, true), pwallet->m_pay_tx_fee);
     // Change always goes back to origin
     coinControl.destChange = tallyItemIn.txdest;
     // Only allow tallyItems inputs for tx creation
@@ -186,8 +186,8 @@ bool CTransactionBuilder::CouldAddOutput(CAmount nAmountOutput) const
 bool CTransactionBuilder::CouldAddOutputs(const std::vector<CAmount>& vecOutputAmounts) const
 {
     CAmount nAmountAdditional{0};
-    assert(vecOutputAmounts.size() < INT_MAX);
-    int nBytesAdditional = nBytesOutput * (int)vecOutputAmounts.size();
+    assert(vecOutputAmounts.size() < std::numeric_limits<int>::max());
+    int nBytesAdditional = nBytesOutput * int(vecOutputAmounts.size());
     for (const auto nAmountOutput : vecOutputAmounts) {
         if (nAmountOutput < 0) {
             return false;
@@ -232,7 +232,7 @@ CAmount CTransactionBuilder::GetAmountUsed() const
 CAmount CTransactionBuilder::GetFee(unsigned int nBytes) const
 {
     CAmount nFeeCalc = coinControl.m_feerate->GetFee(nBytes);
-    CAmount nRequiredFee = GetRequiredFee(*pwallet.get(), nBytes);
+    CAmount nRequiredFee = GetRequiredFee(*pwallet, nBytes);
     if (nRequiredFee > nFeeCalc) {
         nFeeCalc = nRequiredFee;
     }
@@ -246,8 +246,8 @@ int CTransactionBuilder::GetSizeOfCompactSizeDiff(size_t nAdd) const
 {
     size_t nSize = WITH_LOCK(cs_outputs, return vecOutputs.size());
     unsigned int ret = ::GetSizeOfCompactSizeDiff(nSize, nSize + nAdd);
-    assert(ret <= INT_MAX);
-    return (int)ret;
+    assert(ret <= std::numeric_limits<int>::max());
+    return int(ret);
 }
 
 bool CTransactionBuilder::IsDust(CAmount nAmount) const

src/coinjoin/util.h

@@ -23,7 +23,7 @@ public:
     void KeepKey();
     void ReturnKey();
 
-    CScript GetScriptForDestination() const;
+    [[nodiscard]] CScript GetScriptForDestination() const;
 };
 
 class CKeyHolderStorage
@@ -58,9 +58,9 @@ public:
     CTransactionBuilderOutput(CTransactionBuilderOutput&&) = delete;
     CTransactionBuilderOutput& operator=(CTransactionBuilderOutput&&) = delete;
     /// Get the scriptPubKey of this output
-    CScript GetScript() const { return script; }
+    [[nodiscard]] CScript GetScript() const { return script; }
     /// Get the amount of this output
-    CAmount GetAmount() const { return nAmount; }
+    [[nodiscard]] CAmount GetAmount() const { return nAmount; }
     /// Try update the amount of this output. Returns true if it was successful and false if not (e.g. insufficient amount left).
     bool UpdateAmount(CAmount nAmount);
     /// Tell the wallet to remove the key used by this output from the keypool