backport: bitcoin#24419, #25351, #25466, #25701, #25707, #25733, #25872, #31364, #31305, #31051, partial bitcoin#31306

ci/dash/lint-tidy.sh

@@ -44,6 +44,7 @@ iwyu_tool.py \
   "src/util/moneystr.cpp" \
   "src/util/serfloat.cpp" \
   "src/util/spanparsing.cpp" \
+  "src/util/string.cpp" \
   "src/util/strencodings.cpp" \
   "src/util/syserror.cpp" \
   "src/util/url.cpp" \

contrib/devtools/iwyu/bitcoin.core.imp

@@ -1,3 +1,3 @@
-# Fixups / upstreamed changes
+# Nothing for now.
 [
 ]

src/.clang-tidy

@@ -1,16 +1,28 @@
 Checks: '
 -*,
 bugprone-argument-comment,
+bugprone-use-after-move,
+misc-unused-using-decls,
 modernize-use-default-member-init,
 modernize-use-nullptr,
+performance-for-range-copy,
+performance-move-const-arg,
+performance-unnecessary-copy-initialization,
 readability-const-return-type,
 readability-redundant-declaration,
 readability-redundant-string-init,
 '
 WarningsAsErrors: '
 bugprone-argument-comment,
+bugprone-use-after-move,
+misc-unused-using-decls,
 modernize-use-default-member-init,
 modernize-use-nullptr,
+performance-move-const-arg,
+performance-unnecessary-copy-initialization,
 readability-redundant-declaration,
 readability-redundant-string-init,
 '
+CheckOptions:
+ - key: performance-move-const-arg.CheckTriviallyCopyableMove
+   value: false

src/bench/load_external.cpp

@@ -28,7 +28,7 @@ static void LoadExternalBlockFile(benchmark::Bench& bench)
     // block data) as a stream object.
     const fs::path blkfile{testing_setup.get()->m_path_root / "blk.dat"};
     CDataStream ss(SER_DISK, 0);
-    auto params{Params()};
+    const auto& params{Params()};
     ss << params.MessageStart();
     ss << static_cast<uint32_t>(benchmark::data::block813851.size());
     // We can't use the streaming serialization (ss << benchmark::data::block813851)

src/bench/prevector.cpp

@@ -112,6 +112,7 @@ static void PrevectorFillVectorDirect(benchmark::Bench& bench)
 {
     bench.run([&] {
         std::vector<prevector<28, T>> vec;
+        vec.reserve(260);
         for (size_t i = 0; i < 260; ++i) {
             vec.emplace_back();
         }
@@ -124,6 +125,7 @@ static void PrevectorFillVectorIndirect(benchmark::Bench& bench)
 {
     bench.run([&] {
         std::vector<prevector<28, T>> vec;
+        vec.reserve(260);
         for (size_t i = 0; i < 260; ++i) {
             // force allocation
             vec.emplace_back(29, T{});

src/bench/wallet_loading.cpp

@@ -19,8 +19,6 @@
 using wallet::CWallet;
 using wallet::DatabaseFormat;
 using wallet::DatabaseOptions;
-using wallet::ISMINE_SPENDABLE;
-using wallet::MakeWalletDatabase;
 using wallet::TxStateInactive;
 using wallet::WALLET_FLAG_DESCRIPTORS;
 using wallet::WalletContext;

src/bitcoin-cli.cpp

@@ -948,7 +948,7 @@ static void GetWalletBalances(UniValue& result)
 
     UniValue balances(UniValue::VOBJ);
     for (const UniValue& wallet : wallets.getValues()) {
-        const std::string wallet_name = wallet.get_str();
+        const std::string& wallet_name = wallet.get_str();
         const UniValue getbalances = ConnectAndCallRPC(&rh, "getbalances", /* args=*/{}, wallet_name);
         const UniValue& balance = getbalances.find_value("result")["mine"]["trusted"];
         balances.pushKV(wallet_name, balance);

src/bitcoin-tx.cpp

@@ -279,7 +279,7 @@ static void MutateTxAddOutAddr(CMutableTransaction& tx, const std::string& strIn
     CAmount value = ExtractAndValidateValue(vStrInputParts[0]);
 
     // extract and validate ADDRESS
-    std::string strAddr = vStrInputParts[1];
+    const std::string& strAddr = vStrInputParts[1];
     CTxDestination destination = DecodeDestination(strAddr);
     if (!IsValidDestination(destination)) {
         throw std::runtime_error("invalid TX output address");
@@ -311,7 +311,7 @@ static void MutateTxAddOutPubKey(CMutableTransaction& tx, const std::string& str
     // Extract and validate FLAGS
     bool bScriptHash = false;
     if (vStrInputParts.size() == 3) {
-        std::string flags = vStrInputParts[2];
+        const std::string& flags = vStrInputParts[2];
         bScriptHash = (flags.find('S') != std::string::npos);
     }
 
@@ -363,7 +363,7 @@ static void MutateTxAddOutMultiSig(CMutableTransaction& tx, const std::string& s
     // Extract FLAGS
     bool bScriptHash = false;
     if (vStrInputParts.size() == numkeys + 4) {
-        std::string flags = vStrInputParts.back();
+        const std::string& flags = vStrInputParts.back();
         bScriptHash = (flags.find('S') != std::string::npos);
     }
     else if (vStrInputParts.size() > numkeys + 4) {
@@ -428,13 +428,13 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
     CAmount value = ExtractAndValidateValue(vStrInputParts[0]);
 
     // extract and validate script
-    std::string strScript = vStrInputParts[1];
+    const std::string& strScript = vStrInputParts[1];
     CScript scriptPubKey = ParseScript(strScript);
 
     // Extract FLAGS
     bool bScriptHash = false;
     if (vStrInputParts.size() == 3) {
-        std::string flags = vStrInputParts.back();
+        const std::string& flags = vStrInputParts.back();
         bScriptHash = (flags.find('S') != std::string::npos);
     }
 
@@ -555,7 +555,7 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
     UniValue prevtxsObj = registers["prevtxs"];
     {
         for (unsigned int previdx = 0; previdx < prevtxsObj.size(); previdx++) {
-            UniValue prevOut = prevtxsObj[previdx];
+            const UniValue& prevOut = prevtxsObj[previdx];
             if (!prevOut.isObject())
                 throw std::runtime_error("expected prevtxs internal object");
 

src/blockfilter.cpp

@@ -170,7 +170,7 @@ const std::set<BlockFilterType>& AllBlockFilterTypes()
 
     static std::once_flag flag;
     std::call_once(flag, []() {
-            for (auto entry : g_filter_types) {
+            for (const auto& entry : g_filter_types) {
                 types.insert(entry.first);
             }
         });

src/bls/bls_worker.cpp

@@ -15,7 +15,7 @@ template <typename T>
 bool VerifyVectorHelper(Span<T> vec)
 {
     std::set<uint256> set;
-    for (auto item : vec) {
+    for (const auto& item : vec) {
         if (!item.IsValid())
             return false;
         // check duplicates

src/coins.cpp

@@ -356,7 +356,7 @@ bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) cons
     try {
         return CCoinsViewBacked::GetCoin(outpoint, coin);
     } catch(const std::runtime_error& e) {
-        for (auto f : m_err_callbacks) {
+        for (const auto& f : m_err_callbacks) {
             f();
         }
         LogPrintf("Error reading from database: %s\n", e.what());

src/core_read.cpp

@@ -39,7 +39,7 @@ class OpCodeParser
             }
             mapOpNames[strName] = static_cast<opcodetype>(op);
             // Convenience: OP_ADD and just ADD are both recognized:
-            if (strName.compare(0, 3, "OP_") == 0) { // strName starts with "OP_"
+            if (strName.starts_with("OP_")) {
                 mapOpNames[strName.substr(3)] = static_cast<opcodetype>(op);
             }
         }
@@ -189,7 +189,7 @@ int ParseSighashString(const UniValue& sighash)
             {std::string("SINGLE"), int(SIGHASH_SINGLE)},
             {std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)},
         };
-        std::string strHashType = sighash.get_str();
+        const std::string& strHashType = sighash.get_str();
         const auto& it = map_sighash_values.find(strHashType);
         if (it != map_sighash_values.end()) {
             hash_type = it->second;

src/evo/creditpool.cpp

@@ -91,7 +91,7 @@ static std::optional<CreditPoolDataPerBlock> GetCreditDataFromBlock(const gsl::n
         LogPrintf("%s: WARNING: No valid CbTx at height=%d\n", __func__, block_index->nHeight);
         return std::nullopt;
     }
-    for (CTransactionRef tx : block.vtx) {
+    for (const CTransactionRef& tx : block.vtx) {
         if (!tx->IsSpecialTxVersion() || tx->nType != TRANSACTION_ASSET_UNLOCK) continue;
 
         CAmount unlocked{0};

src/evo/deterministicmns.cpp

@@ -1836,7 +1836,7 @@ std::vector<std::pair<uint256, CDeterministicMNListDiff>> CDeterministicMNManage
             temp_diffs.emplace_back(pIndex->GetBlockHash(), recalc_diff);
 
             // Move forward
-            current_list = std::move(next_list);
+            current_list = next_list; // TODO: make CDeterministicMNList moveable
         }
 
         // Verify that applying all diffs results in the target snapshot

src/evo/mnauth.cpp

@@ -36,9 +36,7 @@ void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CActiveMasternode
     if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
         nOurNodeVersion = gArgs.GetIntArg("-pushversion", PROTOCOL_VERSION);
     }
-    auto pk = mn_activeman.GetPubKey();
-    const CBLSPublicKey pubKey(pk);
-    const uint256 signHash{::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion))};
+    const uint256 signHash{::SerializeHash(std::make_tuple(mn_activeman.GetPubKey(), receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion))};
 
     mnauth.proRegTxHash = mn_activeman.GetProTxHash();
 

src/evo/specialtxman.cpp

@@ -191,17 +191,16 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul
 
     int nHeight = pindexPrev->nHeight + 1;
 
-    CDeterministicMNList oldList = prevList;
-    CDeterministicMNList newList = oldList;
+    CDeterministicMNList newList = prevList;
     newList.SetBlockHash(uint256()); // we can't know the final block hash, so better not return a (invalid) block hash
     newList.SetHeight(nHeight);
 
-    auto payee = oldList.GetMNPayee(pindexPrev);
+    auto payee = prevList.GetMNPayee(pindexPrev);
 
-    // we iterate the oldList here and update the newList
+    // we iterate the prevList here and update the newList
     // this is only valid as long these have not diverged at this point, which is the case as long as we don't add
     // code above this loop that modifies newList
-    oldList.ForEachMN(false, [&pindexPrev, &newList, this](auto& dmn) {
+    prevList.ForEachMN(false, [&pindexPrev, &newList, this](auto& dmn) {
         if (!dmn.pdmnState->confirmedHash.IsNull()) {
             // already confirmed
             return;
@@ -503,7 +502,7 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul
         newList.UpdateMN(dmn.proTxHash, newState);
     });
 
-    mnListRet = std::move(newList);
+    mnListRet = newList;
 
     return true;
 }

src/governance/object.cpp

@@ -270,8 +270,8 @@ UniValue CGovernanceObject::GetJSONObject() const
     if (objResult.isObject()) {
         obj = objResult;
     } else {
-        std::vector<UniValue> arr1 = objResult.getValues();
-        std::vector<UniValue> arr2 = arr1.at(0).getValues();
+        const std::vector<UniValue>& arr1 = objResult.getValues();
+        const std::vector<UniValue>& arr2 = arr1.at(0).getValues();
         obj = arr2.at(1);
     }
 

src/init.cpp

@@ -1440,7 +1440,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 static bool LockDataDirectory(bool probeOnly)
 {
     // Make sure only a single Dash Core process is using the data directory.
-    fs::path datadir = gArgs.GetDataDirNet();
+    const fs::path& datadir = gArgs.GetDataDirNet();
     if (!DirIsWritable(datadir)) {
         return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir)));
     }

src/instantsend/instantsend.cpp

@@ -101,7 +101,7 @@ instantsend::PendingState CInstantSendManager::FetchPendingLocks()
     std::vector<uint256> removed;
     removed.reserve(std::min(maxCount, pendingInstantSendLocks.size()));
 
-    for (const auto& [islockHash, nodeid_islptr_pair] : pendingInstantSendLocks) {
+    for (auto& [islockHash, nodeid_islptr_pair] : pendingInstantSendLocks) {
         // Check if we've reached max count
         if (ret.m_pending_is.size() >= maxCount) {
             ret.m_pending_work = true;

src/instantsend/net_instantsend.cpp

@@ -237,11 +237,11 @@ void NetInstantSend::ProcessPendingISLocks(std::vector<std::pair<uint256, instan
         still_pending.reserve(bad_is_locks.size());
         for (auto& p : locks_to_process) {
             if (bad_is_locks.contains(p.first)) {
-                still_pending.push_back(std::move(p));
+                still_pending.emplace_back(std::move(p));
             }
         }
         // Now check against the previous active set and perform banning if this fails
-        ProcessPendingInstantSendLocks(llmq_params, dkgInterval, /*ban=*/true, std::move(still_pending));
+        ProcessPendingInstantSendLocks(llmq_params, dkgInterval, /*ban=*/true, still_pending);
     }
 }
 

src/llmq/blockprocessor.cpp

@@ -441,7 +441,7 @@ bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, gsl::no
 
     for (const auto& tx : block.vtx) {
         if (tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
-            const auto opt_qc = GetTxPayload<CFinalCommitmentTxPayload>(*tx);
+            auto opt_qc = GetTxPayload<CFinalCommitmentTxPayload>(*tx);
             if (!opt_qc) {
                 // should not happen as it was verified before processing the block
                 LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s height=%d GetTxPayload fails\n", __func__, pindex->nHeight);

src/llmq/signing_shares.cpp

@@ -1630,13 +1630,13 @@ void CSigSharesManager::SignPendingSigShares()
         auto opt_sigShare = CreateSigShare(*pQuorum, id, msgHash);
 
         if (opt_sigShare.has_value() && opt_sigShare->sigShare.Get().IsValid()) {
-            auto sigShare = *opt_sigShare;
+            auto& sigShare = *opt_sigShare;
             ProcessSigShare(sigShare, pQuorum);
 
             if (IsAllMembersConnectedEnabled(pQuorum->params.type, m_sporkman)) {
                 LOCK(cs);
                 auto& session = signedSessions[sigShare.GetSignHash()];
-                session.sigShare = sigShare;
+                session.sigShare = std::move(sigShare);
                 session.quorum = pQuorum;
                 session.nextAttemptTime = 0;
                 session.attempt = 0;

src/llmq/snapshot.cpp

@@ -209,17 +209,16 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
             response.quorumSnapshotAtHMinus4C = std::move(snapshotHMinus4C.value());
         }
 
-        CSimplifiedMNListDiff mn4c;
         if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
                                        GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus4CIndex,
                                                             use_legacy_construction),
-                                       pWorkBlockHMinus4CIndex->GetBlockHash(), mn4c, errorRet)) {
+                                       pWorkBlockHMinus4CIndex->GetBlockHash(), response.mnListDiffAtHMinus4C, errorRet)) {
+            response.mnListDiffAtHMinus4C = {};
             return false;
         }
         if (!use_legacy_construction) {
             baseBlockIndexes.push_back(pWorkBlockHMinus4CIndex);
         }
-        response.mnListDiffAtHMinus4C = std::move(mn4c);
     } else {
         response.extraShare = false;
     }

src/logging.cpp

@@ -462,7 +462,7 @@ void BCLog::Logger::LogPrintStr(const std::string& str, const std::string& loggi
     }
 
     if (m_log_threadnames && m_started_new_line) {
-        const auto threadname = util::ThreadGetInternalName();
+        const auto& threadname = util::ThreadGetInternalName();
         // 16 chars total, "dash-" is 5 of them and another 1 is a NUL terminator
         str_prefixed.insert(0, "[" + strprintf("%10s", (threadname.empty() ? "unknown" : threadname)) + "] ");
     }

src/masternode/meta.cpp

@@ -202,7 +202,7 @@ bool CMasternodeMetaMan::SetPlatformBan(const uint256& inv_hash, PlatformBanMess
 
     bool ret = GetMetaInfo(protx_hash).SetPlatformBan(true, ban_msg.m_requested_height);
     if (ret) {
-        m_seen_platform_bans.insert(inv_hash, std::move(ban_msg));
+        m_seen_platform_bans.emplace(inv_hash, std::move(ban_msg));
     }
     return ret;
 }

src/netbase.cpp

@@ -390,7 +390,7 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
         return error("Error sending to proxy");
     }
     uint8_t pchRet1[2];
-    if ((recvr = InterruptibleRecv(pchRet1, 2, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) {
+    if (InterruptibleRecv(pchRet1, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
         LogPrintf("Socks5() connect to %s:%d failed: InterruptibleRecv() timeout or other failure\n", strDest, port);
         return false;
     }
@@ -413,7 +413,7 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
         }
         LogPrint(BCLog::PROXY, "SOCKS5 sending proxy authentication %s:%s\n", auth->username, auth->password);
         uint8_t pchRetA[2];
-        if ((recvr = InterruptibleRecv(pchRetA, 2, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) {
+        if (InterruptibleRecv(pchRetA, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
             return error("Error reading proxy authentication response");
         }
         if (pchRetA[0] != 0x01 || pchRetA[1] != 0x00) {
@@ -479,7 +479,7 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
     if (recvr != IntrRecvError::OK) {
         return error("Error reading from proxy");
     }
-    if ((recvr = InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) {
+    if (InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
         return error("Error reading from proxy");
     }
     LogPrint(BCLog::NET, "SOCKS5 connected %s\n", strDest);

src/node/blockstorage.cpp

@@ -448,7 +448,7 @@ void CleanupBlockRevFiles()
     // Remove the rev files immediately and insert the blk file paths into an
     // ordered map keyed by block file index.
     LogPrintf("Removing unusable blk?????.dat and rev?????.dat files for -reindex with -prune\n");
-    fs::path blocksdir = gArgs.GetBlocksDirPath();
+    const fs::path& blocksdir = gArgs.GetBlocksDirPath();
     for (fs::directory_iterator it(blocksdir); it != fs::directory_iterator(); it++) {
         const std::string path = fs::PathToString(it->path().filename());
         if (fs::is_regular_file(*it) &&

src/qt/bitcoinunits.cpp

@@ -182,7 +182,7 @@ bool BitcoinUnits::parse(Unit unit, const QString& value, CAmount* val_out)
     {
         return false; // More than one dot
     }
-    QString whole = parts[0];
+    const QString& whole = parts[0];
     QString decimals;
 
     if(parts.size() > 1)