Merge #6635: refactor: reduce ostringstream usage, set UniValue variable to object during construction, reduce GetHash() usage

f292483 refactor: use empty() instead of comparing against empty string literal (Kittywhiskers Van Gogh)
d30b9ae refactor: reduce ostringstream usage when constructing strings (Kittywhiskers Van Gogh)
7200505 refactor: stop using ostringstream to construct filter strings (Kittywhiskers Van Gogh)
1c4f441 util: implement string padding function (Kittywhiskers Van Gogh)
5bfc4bc refactor: use `UniValue::VOBJ` instead of `setObject()` when possible (Kittywhiskers Van Gogh)
8b09188 refactor: store `tx_hash` instead of calling `GetHash()` repeatedly (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #6627

  * We use `std::ostringstream` in `masternodelist` to pad values using `std::setw` ([source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/rpc/masternode.cpp#L574-L575)) for consistency, this was fine because we didn't plan on removing anything from the stream after it was added.

    But in [dash#6627](#6627), we intend to print multiple addresses and need to remove excess padding after the last entry, which can be trivially done with a `pop_back()` ([source](https://github.com/dashpay/dash/blob/bba9c6dd8e2edbbb83a26763a1d64209409b5b82/src/rpc/masternode.cpp#L569-L572)) but that cannot be done with a `std::ostringstream`. The best that can be done is rewinding the cursor but that doesn't change its contents and at most lets you overwrite it ([source](https://stackoverflow.com/a/26492431)), which is fine when you do have something to overwrite it with but this isn't always the case.

    To get around this, `PadString()` has been implemented and `std::ostringstream` usage has been replaced with constructing a string with `strprintf`.

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK f292483
  PastaPastaPasta:
    utACK f292483

Tree-SHA512: a3ca9dffc4d13bae80469f669af64d924a9bc6fd02d04832dc1bd07a7016b64973ce41c4dc54dc9c6f580ae92e00edb1cafeeb3b2ee29ae93f2f565ec7f18008

Author: PastaPastaPasta

src/evo/deterministicmns.cpp

@@ -44,9 +44,7 @@ std::string CDeterministicMN::ToString() const
 
 UniValue CDeterministicMN::ToJson() const
 {
-    UniValue obj;
-    obj.setObject();
-
+    UniValue obj(UniValue::VOBJ);
     obj.pushKV("type", std::string(GetMnType(nType).description));
     obj.pushKV("proTxHash", proTxHash.ToString());
     obj.pushKV("collateralHash", collateralOutpoint.hash.ToString());

src/evo/dmnstate.cpp

@@ -37,8 +37,7 @@ std::string CDeterministicMNState::ToString() const
 
 UniValue CDeterministicMNState::ToJson(MnType nType) const
 {
-    UniValue obj;
-    obj.setObject();
+    UniValue obj(UniValue::VOBJ);
     obj.pushKV("version", nVersion);
     obj.pushKV("service", addr.ToStringAddrPort());
     obj.pushKV("registeredHeight", nRegisteredHeight);
@@ -69,8 +68,7 @@ UniValue CDeterministicMNState::ToJson(MnType nType) const
 
 UniValue CDeterministicMNStateDiff::ToJson(MnType nType) const
 {
-    UniValue obj;
-    obj.setObject();
+    UniValue obj(UniValue::VOBJ);
     if (fields & Field_nVersion) {
         obj.pushKV("version", state.nVersion);
     }

src/evo/mnhftx.h

@@ -49,9 +49,7 @@ class MNHFTx
 
     [[nodiscard]] UniValue ToJson() const
     {
-        UniValue obj;
-        obj.clear();
-        obj.setObject();
+        UniValue obj(UniValue::VOBJ);
         obj.pushKV("versionBit", (int)versionBit);
         obj.pushKV("quorumHash", quorumHash.ToString());
         obj.pushKV("sig", sig.ToString());

src/evo/simplifiedmns.cpp

@@ -72,8 +72,7 @@ std::string CSimplifiedMNListEntry::ToString() const
 
 UniValue CSimplifiedMNListEntry::ToJson(bool extended) const
 {
-    UniValue obj;
-    obj.setObject();
+    UniValue obj(UniValue::VOBJ);
     obj.pushKV("nVersion", nVersion);
     obj.pushKV("nType", ToUnderlying(nType));
     obj.pushKV("proRegTxHash", proRegTxHash.ToString());
@@ -239,8 +238,7 @@ bool CSimplifiedMNListDiff::BuildQuorumChainlockInfo(const llmq::CQuorumManager&
 
 UniValue CSimplifiedMNListDiff::ToJson(bool extended) const
 {
-    UniValue obj;
-    obj.setObject();
+    UniValue obj(UniValue::VOBJ);
 
     obj.pushKV("nVersion", nVersion);
     obj.pushKV("baseBlockHash", baseBlockHash.ToString());

src/governance/classes.cpp

@@ -24,55 +24,41 @@ CAmount ParsePaymentAmount(const std::string& strAmount)
 {
     CAmount nAmount = 0;
     if (strAmount.empty()) {
-        std::ostringstream ostr;
-        ostr << "ParsePaymentAmount: Amount is empty";
-        throw std::runtime_error(ostr.str());
+        throw std::runtime_error(strprintf("%s -- Amount is empty", __func__));
     }
     if (strAmount.size() > 20) {
         // String is much too long, the functions below impose stricter
         // requirements
-        std::ostringstream ostr;
-        ostr << "ParsePaymentAmount: Amount string too long";
-        throw std::runtime_error(ostr.str());
+        throw std::runtime_error(strprintf("%s -- Amount string too long", __func__));
     }
     // Make sure the string makes sense as an amount
     // Note: No spaces allowed
     // Also note: No scientific notation
     size_t pos = strAmount.find_first_not_of("0123456789.");
     if (pos != std::string::npos) {
-        std::ostringstream ostr;
-        ostr << "ParsePaymentAmount: Amount string contains invalid character";
-        throw std::runtime_error(ostr.str());
+        throw std::runtime_error(strprintf("%s -- Amount string contains invalid character", __func__));
     }
 
     pos = strAmount.find('.');
     if (pos == 0) {
         // JSON doesn't allow values to start with a decimal point
-        std::ostringstream ostr;
-        ostr << "ParsePaymentAmount: Invalid amount string, leading decimal point not allowed";
-        throw std::runtime_error(ostr.str());
+        throw std::runtime_error(strprintf("%s -- Invalid amount string, leading decimal point not allowed", __func__));
     }
 
     // Make sure there's no more than 1 decimal point
     if ((pos != std::string::npos) && (strAmount.find('.', pos + 1) != std::string::npos)) {
-        std::ostringstream ostr;
-        ostr << "ParsePaymentAmount: Invalid amount string, too many decimal points";
-        throw std::runtime_error(ostr.str());
+        throw std::runtime_error(strprintf("%s -- Invalid amount string, too many decimal points", __func__));
     }
 
     // Note this code is taken from AmountFromValue in rpcserver.cpp
     // which is used for parsing the amounts in createrawtransaction.
     if (!ParseFixedPoint(strAmount, 8, &nAmount)) {
         nAmount = 0;
-        std::ostringstream ostr;
-        ostr << "ParsePaymentAmount: ParseFixedPoint failed for string: " << strAmount;
-        throw std::runtime_error(ostr.str());
+        throw std::runtime_error(strprintf("%s -- ParseFixedPoint failed for string \"%s\"", __func__, strAmount));
     }
     if (!MoneyRange(nAmount)) {
         nAmount = 0;
-        std::ostringstream ostr;
-        ostr << "ParsePaymentAmount: Invalid amount string, value outside of valid money range";
-        throw std::runtime_error(ostr.str());
+        throw std::runtime_error(strprintf("%s -- Invalid amount string, value outside of valid money range", __func__));
     }
 
     return nAmount;
@@ -188,17 +174,15 @@ void CSuperblock::ParsePaymentSchedule(const std::string& strPaymentAddresses, c
     // IF THESE DON'T MATCH, SOMETHING IS WRONG
 
     if (vecPaymentAddresses.size() != vecPaymentAmounts.size() || vecPaymentAddresses.size() != vecProposalHashes.size()) {
-        std::ostringstream ostr;
-        ostr << "CSuperblock::ParsePaymentSchedule -- Mismatched payments, amounts and proposalHashes";
-        LogPrintf("%s\n", ostr.str());
-        throw std::runtime_error(ostr.str());
+        std::string msg{strprintf("CSuperblock::%s -- Mismatched payments, amounts and proposalHashes", __func__)};
+        LogPrintf("%s\n", msg);
+        throw std::runtime_error(msg);
     }
 
     if (vecPaymentAddresses.empty()) {
-        std::ostringstream ostr;
-        ostr << "CSuperblock::ParsePaymentSchedule -- Error no payments";
-        LogPrintf("%s\n", ostr.str());
-        throw std::runtime_error(ostr.str());
+        std::string msg{strprintf("CSuperblock::%s -- Error no payments", __func__)};
+        LogPrintf("%s\n", msg);
+        throw std::runtime_error(msg);
     }
 
     // LOOP THROUGH THE ADDRESSES/AMOUNTS AND CREATE PAYMENTS
@@ -210,36 +194,33 @@ void CSuperblock::ParsePaymentSchedule(const std::string& strPaymentAddresses, c
     for (int i = 0; i < (int)vecPaymentAddresses.size(); i++) {
         CTxDestination dest = DecodeDestination(vecPaymentAddresses[i]);
         if (!IsValidDestination(dest)) {
-            std::ostringstream ostr;
-            ostr << "CSuperblock::ParsePaymentSchedule -- Invalid Dash Address : " << vecPaymentAddresses[i];
-            LogPrintf("%s\n", ostr.str());
-            throw std::runtime_error(ostr.str());
+            std::string msg{strprintf("CSuperblock::%s -- Invalid Dash Address: %s", __func__, vecPaymentAddresses[i])};
+            LogPrintf("%s\n", msg);
+            throw std::runtime_error(msg);
         }
 
         CAmount nAmount = ParsePaymentAmount(vecPaymentAmounts[i]);
 
         uint256 proposalHash;
         if (!ParseHashStr(vecProposalHashes[i], proposalHash)) {
-            std::ostringstream ostr;
-            ostr << "CSuperblock::ParsePaymentSchedule -- Invalid proposal hash : " << vecProposalHashes[i];
-            LogPrintf("%s\n", ostr.str());
-            throw std::runtime_error(ostr.str());
+            std::string msg{strprintf("CSuperblock::%s -- Invalid proposal hash: %s", __func__, vecProposalHashes[i])};
+            LogPrintf("%s\n", msg);
+            throw std::runtime_error(msg);
         }
 
         LogPrint(BCLog::GOBJECT, /* Continued */
-                 "CSuperblock::ParsePaymentSchedule -- i = %d, amount string = %s, nAmount = %lld, proposalHash = %s\n",
+                 "CSuperblock::%s -- i = %d, amount string = %s, nAmount = %lld, proposalHash = %s\n", __func__,
                  i, vecPaymentAmounts[i], nAmount, proposalHash.ToString());
 
         CGovernancePayment payment(dest, nAmount, proposalHash);
         if (payment.IsValid()) {
             vecPayments.push_back(payment);
         } else {
             vecPayments.clear();
-            std::ostringstream ostr;
-            ostr << "CSuperblock::ParsePaymentSchedule -- Invalid payment found: address = " << EncodeDestination(dest)
-                 << ", amount = " << nAmount;
-            LogPrintf("%s\n", ostr.str());
-            throw std::runtime_error(ostr.str());
+            std::string msg{strprintf("CSuperblock::%s -- Invalid payment found: address = %s, amount = %d", __func__,
+                EncodeDestination(dest), nAmount)};
+            LogPrintf("%s\n", msg);
+            throw std::runtime_error(msg);
         }
     }
 }

src/governance/exceptions.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <governance/exceptions.h>
+#include <tinyformat.h>
 
 #include <iostream>
 #include <sstream>
@@ -32,11 +33,8 @@ std::ostream& operator<<(std::ostream& os, governance_exception_type_enum_t eTyp
 CGovernanceException::CGovernanceException(const std::string& strMessageIn,
                                            governance_exception_type_enum_t eTypeIn,
                                            int nNodePenaltyIn) :
-    strMessage(),
-    eType(eTypeIn),
-    nNodePenalty(nNodePenaltyIn)
+    strMessage{strprintf("%s:%s", eTypeIn, strMessageIn)},
+    eType{eTypeIn},
+    nNodePenalty{nNodePenaltyIn}
 {
-    std::ostringstream ostr;
-    ostr << eType << ":" << strMessageIn;
-    strMessage = ostr.str();
 }

src/governance/governance.cpp

@@ -1131,44 +1131,43 @@ bool CGovernanceManager::ProcessVote(CNode* pfrom, const CGovernanceVote& vote,
     uint256 nHashGovobj = vote.GetParentHash();
 
     if (cmapVoteToObject.HasKey(nHashVote)) {
-        LogPrint(BCLog::GOBJECT, "CGovernanceObject::ProcessVote -- skipping known valid vote %s for object %s\n", nHashVote.ToString(), nHashGovobj.ToString());
+        LogPrint(BCLog::GOBJECT, "CGovernanceObject::%s -- skipping known valid vote %s for object %s\n", __func__,
+            nHashVote.ToString(), nHashGovobj.ToString());
         LEAVE_CRITICAL_SECTION(cs);
         return false;
     }
 
     if (cmapInvalidVotes.HasKey(nHashVote)) {
-        std::ostringstream ostr;
-        ostr << "CGovernanceManager::ProcessVote -- Old invalid vote "
-             << ", MN outpoint = " << vote.GetMasternodeOutpoint().ToStringShort()
-             << ", governance object hash = " << nHashGovobj.ToString();
-        LogPrint(BCLog::GOBJECT, "%s\n", ostr.str());
-        exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_PERMANENT_ERROR, 20);
+        std::string msg{strprintf("CGovernanceManager::%s -- Old invalid vote, MN outpoint = %s, governance object hash = %s",
+            __func__, vote.GetMasternodeOutpoint().ToStringShort(), nHashGovobj.ToString())};
+        LogPrint(BCLog::GOBJECT, "%s\n", msg);
+        exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_PERMANENT_ERROR, 20);
         LEAVE_CRITICAL_SECTION(cs);
         return false;
     }
 
     auto it = mapObjects.find(nHashGovobj);
     if (it == mapObjects.end()) {
-        std::ostringstream ostr;
-        ostr << "CGovernanceManager::ProcessVote -- Unknown parent object " << nHashGovobj.ToString()
-             << ", MN outpoint = " << vote.GetMasternodeOutpoint().ToStringShort();
-        exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_WARNING);
+        std::string msg{strprintf("CGovernanceManager::%s -- Unknown parent object %s, MN outpoint = %s", __func__,
+            nHashGovobj.ToString(), vote.GetMasternodeOutpoint().ToStringShort())};
+        exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_WARNING);
         if (cmmapOrphanVotes.Insert(nHashGovobj, vote_time_pair_t(vote, GetTime<std::chrono::seconds>().count() + GOVERNANCE_ORPHAN_EXPIRATION_TIME))) {
             LEAVE_CRITICAL_SECTION(cs);
             RequestGovernanceObject(pfrom, nHashGovobj, connman);
-            LogPrint(BCLog::GOBJECT, "%s\n", ostr.str());
+            LogPrint(BCLog::GOBJECT, "%s\n", msg);
             return false;
         }
 
-        LogPrint(BCLog::GOBJECT, "%s\n", ostr.str());
+        LogPrint(BCLog::GOBJECT, "%s\n", msg);
         LEAVE_CRITICAL_SECTION(cs);
         return false;
     }
 
     CGovernanceObject& govobj = it->second;
 
     if (govobj.IsSetCachedDelete() || govobj.IsSetExpired()) {
-        LogPrint(BCLog::GOBJECT, "CGovernanceObject::ProcessVote -- ignoring vote for expired or deleted object, hash = %s\n", nHashGovobj.ToString());
+        LogPrint(BCLog::GOBJECT, "CGovernanceObject::%s -- ignoring vote for expired or deleted object, hash = %s\n",
+            __func__, nHashGovobj.ToString());
         LEAVE_CRITICAL_SECTION(cs);
         return false;
     }