Make GetEntriesForConflicts return std::optional

Avoids reusing err_string.

Author: glozow

src/policy/rbf.cpp

@@ -47,11 +47,10 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
     return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN;
 }
 
-bool GetEntriesForConflicts(const CTransaction& tx,
+std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
                             CTxMemPool& m_pool,
                             const CTxMemPool::setEntries& setIterConflicting,
-                            CTxMemPool::setEntries& allConflicting,
-                            std::string& err_string)
+                            CTxMemPool::setEntries& allConflicting)
 {
     AssertLockHeld(m_pool.cs);
     const uint256 hash = tx.GetHash();
@@ -62,18 +61,17 @@ bool GetEntriesForConflicts(const CTransaction& tx,
         // but we just want to be conservative to avoid doing too much
         // work.
         if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) {
-            err_string = strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
+            return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
                         hash.ToString(),
                         nConflictingCount,
                         MAX_BIP125_REPLACEMENT_CANDIDATES);
-            return false;
         }
     }
     // If not too many to replace, then calculate the set of
     // transactions that would have to be evicted
     for (CTxMemPool::txiter it : setIterConflicting) {
         m_pool.CalculateDescendants(it, allConflicting);
     }
-    return true;
+    return std::nullopt;
 }
 

src/policy/rbf.h

@@ -40,14 +40,13 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx);
  * mempool must not exceed a total of 100 transactions." Quit as early as possible. There cannot be
  * more than MAX_BIP125_REPLACEMENT_CANDIDATES potential entries.
  * @param[in]   setIterConflicting  The set of iterators to mempool entries.
- * @param[out]  err_string          Used to return errors, if any.
  * @param[out]  allConflicting      Populated with all the mempool entries that would be replaced,
  *                                  which includes descendants of setIterConflicting. Not cleared at
  *                                  the start; any existing mempool entries will remain in the set.
- * @returns false if Rule 5 is broken.
+ * @returns an error message if Rule #5 is broken, otherwise a std::nullopt.
  */
-bool GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& m_pool,
-                            const CTxMemPool::setEntries& setIterConflicting,
-                            CTxMemPool::setEntries& allConflicting,
-                            std::string& err_string) EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs);
+std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& m_pool,
+                                                  const CTxMemPool::setEntries& setIterConflicting,
+                                                  CTxMemPool::setEntries& allConflicting)
+                                                  EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs);
 #endif // BITCOIN_POLICY_RBF_H

src/validation.cpp

@@ -789,7 +789,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     fReplacementTransaction = setConflicts.size();
     if (fReplacementTransaction)
     {
-        std::string err_string;
         CFeeRate newFeeRate(nModifiedFees, nSize);
         for (const auto& mi : setIterConflicting) {
             // Don't allow the replacement to reduce the feerate of the
@@ -818,8 +817,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
         }
 
         // Calculate all conflicting entries and enforce Rule #5.
-        if (!GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting, err_string)) {
-            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", err_string);
+        if (const auto err_string{GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting)}) {
+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
+                                 "too many potential replacements", *err_string);
         }
 
         // Check if it's economically rational to mine this transaction rather