mempool: use util::Result for CalculateAncestorsAndCheckLimits

Avoid using setAncestors outparameter, simplify function signatures
and avoid creating unused dummy strings.

src/txmempool.cpp

@@ -17,8 +17,10 @@
 #include <util/check.h>
 #include <util/moneystr.h>
 #include <util/overflow.h>
+#include <util/result.h>
 #include <util/system.h>
 #include <util/time.h>
+#include <util/translation.h>
 #include <validationinterface.h>
 
 #include <cmath>
@@ -147,32 +149,29 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256>& vHashes
     }
 }
 
-bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
+util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimits(
-                                                  size_t entry_count,
+    size_t entry_size,
-                                                  setEntries& setAncestors,
+    size_t entry_count,
-                                                  CTxMemPoolEntry::Parents& staged_ancestors,
+    CTxMemPoolEntry::Parents& staged_ancestors,
-                                                  const Limits& limits,
+    const Limits& limits) const
-                                                  std::string &errString) const
 {
     size_t totalSizeWithAncestors = entry_size;
+    setEntries ancestors;
 
     while (!staged_ancestors.empty()) {
         const CTxMemPoolEntry& stage = staged_ancestors.begin()->get();
         txiter stageit = mapTx.iterator_to(stage);
 
-        setAncestors.insert(stageit);
+        ancestors.insert(stageit);
         staged_ancestors.erase(stage);
         totalSizeWithAncestors += stageit->GetTxSize();
 
         if (stageit->GetSizeWithDescendants() + entry_size > static_cast<uint64_t>(limits.descendant_size_vbytes)) {
-            errString = strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_size_vbytes);
+            return util::Error{Untranslated(strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_size_vbytes))};
-            return false;
         } else if (stageit->GetCountWithDescendants() + entry_count > static_cast<uint64_t>(limits.descendant_count)) {
-            errString = strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_count);
+            return util::Error{Untranslated(strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_count))};
-            return false;
         } else if (totalSizeWithAncestors > static_cast<uint64_t>(limits.ancestor_size_vbytes)) {
-            errString = strprintf("exceeds ancestor size limit [limit: %u]", limits.ancestor_size_vbytes);
+            return util::Error{Untranslated(strprintf("exceeds ancestor size limit [limit: %u]", limits.ancestor_size_vbytes))};
-            return false;
         }
 
         const CTxMemPoolEntry::Parents& parents = stageit->GetMemPoolParentsConst();
@@ -180,17 +179,16 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
             txiter parent_it = mapTx.iterator_to(parent);
 
             // If this is a new ancestor, add it.
-            if (setAncestors.count(parent_it) == 0) {
+            if (ancestors.count(parent_it) == 0) {
                 staged_ancestors.insert(parent);
             }
-            if (staged_ancestors.size() + setAncestors.size() + entry_count > static_cast<uint64_t>(limits.ancestor_count)) {
+            if (staged_ancestors.size() + ancestors.size() + entry_count > static_cast<uint64_t>(limits.ancestor_count)) {
-                errString = strprintf("too many unconfirmed ancestors [limit: %u]", limits.ancestor_count);
+                return util::Error{Untranslated(strprintf("too many unconfirmed ancestors [limit: %u]", limits.ancestor_count))};
-                return false;
             }
         }
     }
 
-    return true;
+    return ancestors;
 }
 
 bool CTxMemPool::CheckPackageLimits(const Package& package,
@@ -215,13 +213,11 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
     // When multiple transactions are passed in, the ancestors and descendants of all transactions
     // considered together must be within limits even if they are not interdependent. This may be
     // stricter than the limits for each individual transaction.
-    setEntries setAncestors;
+    const auto ancestors{CalculateAncestorsAndCheckLimits(total_size, package.size(),
-    const auto ret = CalculateAncestorsAndCheckLimits(total_size, package.size(),
+                                                          staged_ancestors, limits)};
-                                                      setAncestors, staged_ancestors,
-                                                      limits, errString);
     // It's possible to overestimate the ancestor/descendant totals.
-    if (!ret) errString.insert(0, "possibly ");
+    if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original;
-    return ret;
+    return ancestors.has_value();
 }
 
 bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
@@ -254,9 +250,14 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
         staged_ancestors = it->GetMemPoolParentsConst();
     }
 
-    return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1,
+    const auto calculated_ancestors{CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1,
-                                            setAncestors, staged_ancestors,
+                                                       staged_ancestors, limits)};
-                                            limits, errString);
+    if (!calculated_ancestors.has_value()) {
+        errString = util::ErrorString(calculated_ancestors).original;
+        return false;
+    }
+    setAncestors = *calculated_ancestors;
+    return true;
 }
 
 void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors)

src/txmempool.h

@@ -28,6 +28,7 @@
 #include <txmempool_entry.h>
 #include <util/epochguard.h>
 #include <util/hasher.h>
+#include <util/result.h>
 
 #include <boost/multi_index/hashed_index.hpp>
 #include <boost/multi_index/ordered_index.hpp>
@@ -428,24 +429,20 @@ private:
 
     /**
      * Helper function to calculate all in-mempool ancestors of staged_ancestors and apply ancestor
-     * and descendant limits (including staged_ancestors thsemselves, entry_size and entry_count).
+     * and descendant limits (including staged_ancestors themselves, entry_size and entry_count).
      *
      * @param[in]   entry_size          Virtual size to include in the limits.
      * @param[in]   entry_count         How many entries to include in the limits.
-     * @param[out]  setAncestors        Will be populated with all mempool ancestors.
      * @param[in]   staged_ancestors    Should contain entries in the mempool.
      * @param[in]   limits              Maximum number and size of ancestors and descendants
-     * @param[out]  errString           Populated with error reason if any limits are hit
      *
-     * @return true if no limits were hit and all in-mempool ancestors were calculated, false
+     * @return all in-mempool ancestors, or an error if any ancestor or descendant limits were hit
-     * otherwise
      */
-    bool CalculateAncestorsAndCheckLimits(size_t entry_size,
+    util::Result<setEntries> CalculateAncestorsAndCheckLimits(size_t entry_size,
-                                          size_t entry_count,
+                                                              size_t entry_count,
-                                          setEntries& setAncestors,
+                                                              CTxMemPoolEntry::Parents &staged_ancestors,
-                                          CTxMemPoolEntry::Parents &staged_ancestors,
+                                                              const Limits& limits
-                                          const Limits& limits,
+                                                              ) const EXCLUSIVE_LOCKS_REQUIRED(cs);
-                                          std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
 public:
     indirectmap<COutPoint, const CTransaction*> mapNextTx GUARDED_BY(cs);