Merge #2935: [Cleanup] Properly terminate printed log lines

dade23d lint: Add LogPrint(f) termination linter (Fuzzbawls)
7459120 Cleanup: Properly terminate printed log lines. (Fuzzbawls)

Pull request description:

  Ensure that every Logprint(f) call is properly terminated with a newline (`\n`). Not doing this leads to conjoined lines in the log output, which should not happen. There are few instances where an unterminated call is intentional (ie, inside a for loop, with a final call at the end of the loop)

  Multi-line calls and daisy-chained calls can use a descriptor comment at the end of the code line (`/* Continued */`) to denote the log line is continued afterwards.

  To prevent future instances of un-terminated log lines, add a linter specifically to check for this.

ACKs for top commit: dade23d
  panleone:
    utACK dade23d
  Liquid369:
    tACK dade23d
  Duddino:
    ACK dade23d

Tree-SHA512: 8ed1ce5ee36ed3d1851f97e33afb1d5105dd1c98a53bcf77bd957515b031ce0efe79ed5714abc73ad756acfe1ec21cbc0b9edcf28bdabfcd283f7fc1a46864aa

src/activemasternode.cpp

@@ -336,7 +336,7 @@ void CActiveMasternode::ManageStatus()
     if (status == ACTIVE_MASTERNODE_INITIAL || (pmn && status == ACTIVE_MASTERNODE_NOT_CAPABLE)) {
         if (pmn) {
             if (pmn->protocolVersion != PROTOCOL_VERSION) {
-                LogPrintf("%s: ERROR Trying to start a masternode running an old protocol version, "
+                LogPrintf("%s: ERROR Trying to start a masternode running an old protocol version, " /* Continued */
                           "the controller and masternode wallets need to be running the latest release version.\n", __func__);
                 return;
             }

src/budget/budgetmanager.cpp

@@ -126,7 +126,7 @@ uint256 CBudgetManager::SubmitFinalBudget()
 
     if (nBlockStart - nCurrentHeight > finalizationWindow) {
         LogPrint(BCLog::MNBUDGET,"%s: Too early for finalization. Current block is %ld, next Superblock is %ld.\n", __func__, nCurrentHeight, nBlockStart);
-        LogPrint(BCLog::MNBUDGET,"%s: First possible block for finalization: %ld. Last possible block for finalization: %ld. "
+        LogPrint(BCLog::MNBUDGET,"%s: First possible block for finalization: %ld. Last possible block for finalization: %ld. " /* Continued */
                 "You have to wait for %ld block(s) until Budget finalization will be possible\n", __func__, nFinalizationStart, nBlockStart, nOffsetToStart);
         return UINT256_ZERO;
     }

src/init.cpp

@@ -694,7 +694,7 @@ void ThreadImport(const std::vector<fs::path>& vImportFiles)
     // scan for better chains in the block chain database, that are not yet connected in the active best chain
     CValidationState state;
     if (!ActivateBestChain(state)) {
-        LogPrintf("Failed to connect best block");
+        LogPrintf("Failed to connect best block\n");
         StartShutdown();
     }
 
@@ -1201,9 +1201,9 @@ bool AppInitMain()
 
     // Warn about relative -datadir path.
     if (gArgs.IsArgSet("-datadir") && !fs::path(gArgs.GetArg("-datadir", "")).is_absolute()) {
-        LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the "
-                  "current working directory '%s'. This is fragile because if PIVX is started in the future "
-                  "from a different location. It will be unable to locate the current data files. There could "
+        LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the " /* Continued */
+                  "current working directory '%s'. This is fragile because if PIVX is started in the future " /* Continued */
+                  "from a different location. It will be unable to locate the current data files. There could " /* Continued */
                   "also be data loss if PIVX is started while in a temporary directory.\n",
             gArgs.GetArg("-datadir", ""), fs::current_path().string());
     }

src/kernel.cpp

@@ -67,14 +67,14 @@ bool CStakeKernel::CheckKernelHash(bool fSkipLog) const
     const bool res = hashProofOfStake < bnTarget;
 
     if (!fSkipLog || res) {
-        LogPrint(BCLog::STAKING, "%s : Proof Of Stake:"
-                            "\nstakeModifier=%s"
-                            "\nnTimeBlockFrom=%d"
-                            "\nssUniqueID=%s"
-                            "\nnTimeTx=%d"
-                            "\nhashProofOfStake=%s"
-                            "\nnBits=%d"
-                            "\nweight=%d"
+        LogPrint(BCLog::STAKING, "%s : Proof Of Stake:" /* Continued */
+                            "\nstakeModifier=%s" /* Continued */
+                            "\nnTimeBlockFrom=%d" /* Continued */
+                            "\nssUniqueID=%s" /* Continued */
+                            "\nnTimeTx=%d" /* Continued */
+                            "\nhashProofOfStake=%s" /* Continued */
+                            "\nnBits=%d" /* Continued */
+                            "\nweight=%d" /* Continued */
                             "\nbnTarget=%s (res: %d)\n\n",
             __func__, HexStr(stakeModifier), nTimeBlockFrom, HexStr(stakeUniqueness), nTime, hashProofOfStake.GetHex(),
             nBits, stakeValue, bnTarget.GetHex(), res);

src/llmq/quorums.cpp

@@ -323,7 +323,7 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint25
         pindexQuorum = LookupBlockIndex(quorumHash);
 
         if (!pindexQuorum) {
-            LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- block %s not found", __func__, quorumHash.ToString());
+            LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- block %s not found.\n", __func__, quorumHash.ToString());
             return nullptr;
         }
     }

src/llmq/quorums_signing_shares.cpp

@@ -491,7 +491,7 @@ bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
             if (!pubKeyShare.IsValid()) {
                 // this should really not happen (we already ensured we have the quorum vvec,
                 // so we should also be able to create all pubkey shares)
-                LogPrintf("CSigSharesManager::%s -- pubKeyShare is invalid, which should not be possible here", __func__);
+                LogPrintf("CSigSharesManager::%s -- pubKeyShare is invalid, which should not be possible here.\n", __func__);
                 assert(false);
             }
 

src/masternode-payments.cpp

@@ -657,7 +657,7 @@ bool CMasternodePayments::IsTransactionValid(const CTransaction& txNew, const CB
                 CTxDestination mnDest;
                 const std::string& payee = ExtractDestination(o.scriptPubKey, mnDest) ? EncodeDestination(mnDest)
                                                                                       : HexStr(o.scriptPubKey);
-                LogPrint(BCLog::MASTERNODE, "%s: Failed to find expected payee %s in block at height %d (tx %s)",
+                LogPrint(BCLog::MASTERNODE, "%s: Failed to find expected payee %s in block at height %d (tx %s)\n",
                                             __func__, payee, pindexPrev->nHeight + 1, txNew.GetHash().ToString());
                 return false;
             }
@@ -743,7 +743,7 @@ void CMasternodePayments::ProcessBlock(int nBlockHeight)
 
     // check winner height
     if (nBlockHeight - 100 > mnodeman.GetBestHeight() + 1) {
-        LogPrintf("%s: mnw - invalid height %d > %d", __func__, nBlockHeight - 100, mnodeman.GetBestHeight() + 1);
+        LogPrintf("%s: mnw - invalid height %d > %d\n", __func__, nBlockHeight - 100, mnodeman.GetBestHeight() + 1);
         return;
     }
 

src/miner.cpp

@@ -274,9 +274,9 @@ void static ThreadBitcoinMiner(void* parg)
         BitcoinMiner(pwallet, false);
         boost::this_thread::interruption_point();
     } catch (const std::exception& e) {
-        LogPrintf("PIVXMiner exception");
+        LogPrintf("PIVXMiner exception\n");
     } catch (...) {
-        LogPrintf("PIVXMiner exception");
+        LogPrintf("PIVXMiner exception\n");
     }
 
     LogPrintf("PIVXMiner exiting\n");

src/net.cpp

@@ -370,7 +370,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char* pszDest, bool fCo
         if (Lookup(pszDest, resolved,  default_port, fNameLookup && !HaveNameProxy(), 256) && !resolved.empty()) {
             addrConnect = CAddress(resolved[GetRand(resolved.size())], NODE_NONE);
             if (!addrConnect.IsValid()) {
-                LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s", addrConnect.ToString(), pszDest);
+                LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToString(), pszDest);
                 return nullptr;
             }
         }

src/primitives/block.cpp

@@ -52,5 +52,5 @@ std::string CBlock::ToString() const
 
 void CBlock::print() const
 {
-    LogPrintf("%s", ToString());
+    LogPrintf("%s\n", ToString());
 }

src/qt/pivx/governancemodel.cpp

@@ -311,7 +311,7 @@ void GovernanceModel::pollGovernanceChanged()
 
         // Try to add it
         if (!g_budgetman.AddProposal(*it)) {
-            LogPrint(BCLog::QT, "Cannot broadcast budget proposal - %s", it->IsInvalidReason());
+            LogPrint(BCLog::QT, "Cannot broadcast budget proposal - %s\n", it->IsInvalidReason());
             // Remove proposals which due a reorg lost their fee tx
             if (it->IsInvalidReason().find("Can't find collateral tx") != std::string::npos) {
                 // future: notify the user about it.

src/sapling/crypter_sapling.cpp

@@ -93,7 +93,7 @@ bool CCryptoKeyStore::UnlockSaplingKeys(const CKeyingMaterial& vMasterKeyIn, boo
     }
 
     if (keyPass && keyFail) {
-        LogPrintf("Sapling wallet is probably corrupted: Some keys decrypt but not all.");
+        LogPrintf("Sapling wallet is probably corrupted: Some keys decrypt but not all.\n");
         throw std::runtime_error("Error unlocking sapling wallet: some keys decrypt but not all. Your wallet file may be corrupt.");
     }
     if (keyFail || !keyPass)

src/sapling/saplingscriptpubkeyman.cpp

@@ -831,7 +831,7 @@ Optional<std::pair<
         try {
             ovks.emplace(getCommonOVK());
         } catch (...) {
-            LogPrintf("WARNING: No CommonOVK found. Some notes might not be correctly recovered. "
+            LogPrintf("WARNING: No CommonOVK found. Some notes might not be correctly recovered. " /* Continued */
                       "Unlock the wallet and call 'viewshieldtransaction %s' to fix.\n", txId.ToString());
         }
     } else {

src/tiertwo/init.cpp

@@ -178,12 +178,12 @@ bool LoadTierTwo(int chain_active_height, bool load_cache_files)
     CFlatDB<CNetFulfilledRequestManager> netRequestsDb(NET_REQUESTS_CACHE_FILENAME, NET_REQUESTS_CACHE_FILE_ID);
     if (load_cache_files) {
         if (!netRequestsDb.Load(g_netfulfilledman)) {
-            LogPrintf("Failed to load network requests cache from %s", netRequestsDb.GetDbPath().string());
+            LogPrintf("Failed to load network requests cache from %s\n", netRequestsDb.GetDbPath().string());
         }
     } else {
         CNetFulfilledRequestManager netfulfilledmanTmp(0);
         if (!netRequestsDb.Dump(netfulfilledmanTmp)) {
-            LogPrintf("Failed to clear network requests cache at %s", netRequestsDb.GetDbPath().string());
+            LogPrintf("Failed to clear network requests cache at %s\n", netRequestsDb.GetDbPath().string());
         }
     }
 

src/timedata.cpp

@@ -88,8 +88,8 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample, int nOffsetLimit)
         }
         if (!gArgs.GetBoolArg("-shrinkdebugfile", g_logger->DefaultShrinkDebugFile())) {
             for (int64_t n : vSorted)
-                LogPrintf("%+d  ", n);
-            LogPrintf("|  ");
+                LogPrintf("%+d  ", n); /* Continued */
+            LogPrintf("|  "); /* Continued */
         }
         LogPrintf("nTimeOffset = %+d\n", nTimeOffset);
     }

src/txmempool.cpp

@@ -793,7 +793,7 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx)
         case CTransaction::TxType::PROREG: {
             ProRegPL pl;
             if (!GetTxPayload(tx, pl)) {
-                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString());
+                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s\n", __func__, tx.ToString());
                 return;
             }
             if (mapProTxAddresses.count(pl.addr)) {
@@ -813,7 +813,7 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx)
         case CTransaction::TxType::PROUPSERV: {
             ProUpServPL pl;
             if (!GetTxPayload(tx, pl)) {
-                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString());
+                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s\n", __func__, tx.ToString());
                 return;
             }
             if (mapProTxAddresses.count(pl.addr)) {
@@ -828,7 +828,7 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx)
         case CTransaction::TxType::PROUPREG: {
             ProUpRegPL pl;
             if (!GetTxPayload(tx, pl)) {
-                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString());
+                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s\n", __func__, tx.ToString());
                 return;
             }
             removeProTxPubKeyConflicts(tx, pl.pubKeyOperator);
@@ -1153,7 +1153,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const
         case CTransaction::TxType::PROREG: {
             ProRegPL pl;
             if (!GetTxPayload(tx, pl)) {
-                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString());
+                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s\n", __func__, tx.ToString());
                 return true; // i.e. can't decode payload == conflict
             }
             if (mapProTxAddresses.count(pl.addr) || mapProTxPubKeyIDs.count(pl.keyIDOwner) ||
@@ -1176,7 +1176,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const
         case CTransaction::TxType::PROUPSERV: {
             ProUpServPL pl;
             if (!GetTxPayload(tx, pl)) {
-                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString());
+                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s\n", __func__, tx.ToString());
                 return true; // i.e. can't decode payload == conflict
             }
             auto it = mapProTxAddresses.find(pl.addr);
@@ -1186,7 +1186,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const
         case CTransaction::TxType::PROUPREG: {
             ProUpRegPL pl;
             if (!GetTxPayload(tx, pl)) {
-                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString());
+                LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s\n", __func__, tx.ToString());
                 return true; // i.e. can't decode payload == conflict
             }
             auto it = mapProTxBlsPubKeyHashes.find(pl.pubKeyOperator.GetHash());
@@ -1217,7 +1217,7 @@ bool CTxMemPool::WriteFeeEstimates(CAutoFile& fileout) const
         fileout << CLIENT_VERSION;  // version that wrote the file
         minerPolicyEstimator->Write(fileout);
     } catch (const std::exception&) {
-        LogPrintf("CTxMemPool::WriteFeeEstimates() : unable to write policy estimator data (non-fatal)");
+        LogPrintf("CTxMemPool::WriteFeeEstimates() : unable to write policy estimator data (non-fatal)\n");
         return false;
     }
     return true;
@@ -1229,12 +1229,12 @@ bool CTxMemPool::ReadFeeEstimates(CAutoFile& filein)
         int nVersionRequired, nVersionThatWrote;
         filein >> nVersionRequired >> nVersionThatWrote;
         if (nVersionRequired > CLIENT_VERSION)
-            return error("CTxMemPool::ReadFeeEstimates() : up-version (%d) fee estimate file", nVersionRequired);
+            return error("CTxMemPool::ReadFeeEstimates() : up-version (%d) fee estimate file\n", nVersionRequired);
 
         LOCK(cs);
         minerPolicyEstimator->Read(filein, nVersionThatWrote);
     } catch (const std::exception&) {
-        LogPrintf("CTxMemPool::ReadFeeEstimates() : unable to read policy estimator data (non-fatal)");
+        LogPrintf("CTxMemPool::ReadFeeEstimates() : unable to read policy estimator data (non-fatal)\n");
         return false;
     }
     return true;

src/validation.cpp

@@ -3290,7 +3290,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockInde
     if (pindex->nStatus & BLOCK_HAVE_DATA) {
         // TODO: deal better with duplicate blocks.
         // return state.DoS(20, error("AcceptBlock() : already have block %d %s", pindex->nHeight, pindex->GetBlockHash().ToString()), REJECT_DUPLICATE, "duplicate");
-        LogPrintf("%s : already have block %d %s", __func__, pindex->nHeight, pindex->GetBlockHash().ToString());
+        LogPrintf("%s : already have block %d %s\n", __func__, pindex->nHeight, pindex->GetBlockHash().ToString());
         return true;
     }
 
@@ -4059,7 +4059,7 @@ bool LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp)
                     }
                 }
             } catch (const std::exception& e) {
-                LogPrintf("%s : Deserialize or I/O error - %s", __func__, e.what());
+                LogPrintf("%s : Deserialize or I/O error - %s\n", __func__, e.what());
             }
         }
     } catch (const std::runtime_error& e) {

src/wallet/wallet.cpp

@@ -3528,7 +3528,7 @@ CWallet::CommitResult CWallet::CommitTransaction(CTransactionRef tx, CReserveKey
 
     {
         LOCK2(cs_main, cs_wallet);
-        LogPrintf("%s:\n%s", __func__, wtxNew.tx->ToString());
+        LogPrintf("%s: %s\n", __func__, wtxNew.tx->ToString());
         {
             // Take key pair from key pool so it won't be used again
             if (opReservekey) opReservekey->KeepKey();

src/zpiv/zpos.cpp

@@ -31,7 +31,7 @@ static const CBlockIndex* FindIndexFrom(uint32_t nChecksum, libzerocoin::CoinDen
 
     // Not found. This should never happen (during IBD we save the accumulator checksums
     // in the cache as they are updated, and persist the cache to DB) but better to have a fallback.
-    LogPrintf("WARNING: accumulatorCache corrupt - missing (%d-%d), height=%d",
+    LogPrintf("WARNING: accumulatorCache corrupt - missing (%d-%d), height=%d\n",
               nChecksum, libzerocoin::ZerocoinDenominationToInt(denom), cpHeight);
 
     // Start at the current checkpoint and go backwards
@@ -59,22 +59,22 @@ CLegacyZPivStake* CLegacyZPivStake::NewZPivStake(const CTxIn& txin, int nHeight)
 {
     // Construct the stakeinput object
     if (!txin.IsZerocoinSpend()) {
-        LogPrintf("%s: unable to initialize CLegacyZPivStake from non zc-spend", __func__);
+        LogPrintf("%s: unable to initialize CLegacyZPivStake from non zc-spend\n", __func__);
         return nullptr;
     }
 
     // Return immediately if zPOS not enforced
     const Consensus::Params& consensus = Params().GetConsensus();
     if (!consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_ZC_V2) ||
             nHeight >= consensus.height_last_ZC_AccumCheckpoint) {
-        LogPrint(BCLog::LEGACYZC, "%s : zPIV stake block: height %d outside range", __func__, nHeight);
+        LogPrint(BCLog::LEGACYZC, "%s : zPIV stake block: height %d outside range\n", __func__, nHeight);
         return nullptr;
     }
 
     // Check spend type
     libzerocoin::CoinSpend spend = ZPIVModule::TxInToZerocoinSpend(txin);
     if (spend.getSpendType() != libzerocoin::SpendType::STAKE) {
-        LogPrintf("%s : spend is using the wrong SpendType (%d)", __func__, (int)spend.getSpendType());
+        LogPrintf("%s : spend is using the wrong SpendType (%d)\n", __func__, (int)spend.getSpendType());
         return nullptr;
     }
 
@@ -86,13 +86,13 @@ CLegacyZPivStake* CLegacyZPivStake::NewZPivStake(const CTxIn& txin, int nHeight)
     // The checkpoint needs to be from 200 blocks ago
     const int cpHeight = nHeight - 1 - consensus.ZC_MinStakeDepth;
     if (ParseAccChecksum(chainActive[cpHeight]->nAccumulatorCheckpoint, _denom) != _nChecksum) {
-        LogPrint(BCLog::LEGACYZC, "%s : accum. checksum at height %d is wrong.", __func__, nHeight);
+        LogPrint(BCLog::LEGACYZC, "%s : accum. checksum at height %d is wrong.\n", __func__, nHeight);
     }
 
     // Find the pindex of the first block with the accumulator checksum
     const CBlockIndex* _pindexFrom = FindIndexFrom(_nChecksum, _denom, cpHeight);
     if (_pindexFrom == nullptr) {
-        LogPrintf("%s : Failed to find the block index for zpiv stake origin", __func__);
+        LogPrintf("%s : Failed to find the block index for zpiv stake origin\n", __func__);
         return nullptr;
     }