Merge bitcoin/bitcoin#28863: wallet, mempool: propagete checkChainLimits error message to wallet

8dec9c5 wallet, mempool: propagete `checkChainLimits` error message to wallet (ismaelsadeeq)

Pull request description:

  * Requested in [#28391 comment](bitcoin/bitcoin#28391 (comment))

  * The error message is static when a new transaction is created and package limit is reached.
  `Transaction has too long of a mempool chain`
  While the [`CTxMempool::CheckPackageLimits`](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/txmempool.cpp#L199) provide explicit information about the error message.
  * This PR updates [`CTxMempool::CheckPackageLimits`](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/txmempool.cpp#L199) return type to `util::Result<void>`, `CheckPackageLimits` now returns void when package limit is not hit, and returns the error string whenever package limit is hit instead of using out parameter `errString`.
  * The PR updates [`checkChainLimits`](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/node/interfaces.cpp#L703) return type to `util::Result<void>`.

  * Now the wallet `CreateTransactionInternal` will have access to the package limit error string whenever its hit.
  * Also Updated functional test to reflect the error message from `CTxMempool::CheckPackageLimits` output.

ACKs for top commit:
  glozow:
    utACK 8dec9c5
  Sjors:
    utACK 8dec9c5
  TheCharlatan:
    Re-ACK 8dec9c5

Tree-SHA512: ddeac18aeba6f8e3be0e3fe76bf3db655352e3b415169f1f83ea1e8976a2f3e3de021c8da6880eb8382ab52d545e418e3f4d57adcc68ecb4f390339710ee6f30

Author: glozow

src/interfaces/chain.h

@@ -8,6 +8,7 @@
 #include <blockfilter.h>
 #include <common/settings.h>
 #include <primitives/transaction.h> // For CTransactionRef
+#include <util/result.h>
 
 #include <functional>
 #include <memory>
@@ -260,7 +261,7 @@ class Chain
     virtual void getPackageLimits(unsigned int& limit_ancestor_count, unsigned int& limit_descendant_count) = 0;
 
     //! Check if transaction will pass the mempool's chain limits.
-    virtual bool checkChainLimits(const CTransactionRef& tx) = 0;
+    virtual util::Result<void> checkChainLimits(const CTransactionRef& tx) = 0;
 
     //! Estimate smart fee.
     virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc = nullptr) = 0;

src/node/interfaces.cpp

@@ -45,6 +45,7 @@
 #include <uint256.h>
 #include <univalue.h>
 #include <util/check.h>
+#include <util/result.h>
 #include <util/signalinterrupt.h>
 #include <util/translation.h>
 #include <validation.h>
@@ -702,14 +703,13 @@ class ChainImpl : public Chain
         limit_ancestor_count = limits.ancestor_count;
         limit_descendant_count = limits.descendant_count;
     }
-    bool checkChainLimits(const CTransactionRef& tx) override
+    util::Result<void> checkChainLimits(const CTransactionRef& tx) override
     {
-        if (!m_node.mempool) return true;
+        if (!m_node.mempool) return {};
         LockPoints lp;
         CTxMemPoolEntry entry(tx, 0, 0, 0, 0, false, 0, lp);
         LOCK(m_node.mempool->cs);
-        std::string err_string;
-        return m_node.mempool->CheckPackageLimits({tx}, entry.GetTxSize(), err_string);
+        return m_node.mempool->CheckPackageLimits({tx}, entry.GetTxSize());
     }
     CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override
     {

src/txmempool.cpp

@@ -196,25 +196,20 @@ util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimit
     return ancestors;
 }
 
-bool CTxMemPool::CheckPackageLimits(const Package& package,
-                                    const int64_t total_vsize,
-                                    std::string &errString) const
+util::Result<void> CTxMemPool::CheckPackageLimits(const Package& package,
+                                                  const int64_t total_vsize) const
 {
     size_t pack_count = package.size();
 
     // Package itself is busting mempool limits; should be rejected even if no staged_ancestors exist
     if (pack_count > static_cast<uint64_t>(m_limits.ancestor_count)) {
-        errString = strprintf("package count %u exceeds ancestor count limit [limit: %u]", pack_count, m_limits.ancestor_count);
-        return false;
+        return util::Error{Untranslated(strprintf("package count %u exceeds ancestor count limit [limit: %u]", pack_count, m_limits.ancestor_count))};
     } else if (pack_count > static_cast<uint64_t>(m_limits.descendant_count)) {
-        errString = strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_count, m_limits.descendant_count);
-        return false;
+        return util::Error{Untranslated(strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_count, m_limits.descendant_count))};
     } else if (total_vsize > m_limits.ancestor_size_vbytes) {
-        errString = strprintf("package size %u exceeds ancestor size limit [limit: %u]", total_vsize, m_limits.ancestor_size_vbytes);
-        return false;
+        return util::Error{Untranslated(strprintf("package size %u exceeds ancestor size limit [limit: %u]", total_vsize, m_limits.ancestor_size_vbytes))};
     } else if (total_vsize > m_limits.descendant_size_vbytes) {
-        errString = strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_limits.descendant_size_vbytes);
-        return false;
+        return util::Error{Untranslated(strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_limits.descendant_size_vbytes))};
     }
 
     CTxMemPoolEntry::Parents staged_ancestors;
@@ -224,8 +219,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
             if (piter) {
                 staged_ancestors.insert(**piter);
                 if (staged_ancestors.size() + package.size() > static_cast<uint64_t>(m_limits.ancestor_count)) {
-                    errString = strprintf("too many unconfirmed parents [limit: %u]", m_limits.ancestor_count);
-                    return false;
+                    return util::Error{Untranslated(strprintf("too many unconfirmed parents [limit: %u]", m_limits.ancestor_count))};
                 }
             }
         }
@@ -236,8 +230,8 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
     const auto ancestors{CalculateAncestorsAndCheckLimits(total_vsize, package.size(),
                                                           staged_ancestors, m_limits)};
     // It's possible to overestimate the ancestor/descendant totals.
-    if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original;
-    return ancestors.has_value();
+    if (!ancestors.has_value()) return util::Error{Untranslated("possibly " + util::ErrorString(ancestors).original)};
+    return {};
 }
 
 util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateMemPoolAncestors(

src/txmempool.h

@@ -605,11 +605,9 @@ class CTxMemPool
      *                                          to mempool. The transactions need not be direct
      *                                          ancestors/descendants of each other.
      * @param[in]       total_vsize             Sum of virtual sizes for all transactions in package.
-     * @param[out]      errString               Populated with error reason if a limit is hit.
      */
-    bool CheckPackageLimits(const Package& package,
-                            int64_t total_vsize,
-                            std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    util::Result<void> CheckPackageLimits(const Package& package,
+                                          int64_t total_vsize) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /** Populate setDescendants with all in-mempool descendants of hash.
      *  Assumes that setDescendants includes all in-mempool descendants of anything

src/validation.cpp

@@ -51,6 +51,7 @@
 #include <util/hasher.h>
 #include <util/moneystr.h>
 #include <util/rbf.h>
+#include <util/result.h>
 #include <util/signalinterrupt.h>
 #include <util/strencodings.h>
 #include <util/time.h>
@@ -1024,10 +1025,10 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
     assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx)
                        { return !m_pool.exists(GenTxid::Txid(tx->GetHash()));}));
 
-    std::string err_string;
-    if (!m_pool.CheckPackageLimits(txns, total_vsize, err_string)) {
+    auto result = m_pool.CheckPackageLimits(txns, total_vsize);
+    if (!result) {
         // This is a package-wide error, separate from an individual transaction error.
-        return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string);
+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", util::ErrorString(result).original);
     }
    return true;
 }

src/wallet/spend.cpp

@@ -1296,8 +1296,9 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
 
     if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
         // Lastly, ensure this tx will pass the mempool's chain limits
-        if (!wallet.chain().checkChainLimits(tx)) {
-            return util::Error{_("Transaction has too long of a mempool chain")};
+        auto result = wallet.chain().checkChainLimits(tx);
+        if (!result) {
+            return util::Error{util::ErrorString(result)};
         }
     }
 

test/functional/wallet_basic.py

@@ -639,7 +639,7 @@ def run_test(self):
 
         node0_balance = self.nodes[0].getbalance()
         # With walletrejectlongchains we will not create the tx and store it in our wallet.
-        assert_raises_rpc_error(-6, "Transaction has too long of a mempool chain", self.nodes[0].sendtoaddress, sending_addr, node0_balance - Decimal('0.01'))
+        assert_raises_rpc_error(-6, f"too many unconfirmed ancestors [limit: {chainlimit * 2}]", self.nodes[0].sendtoaddress, sending_addr, node0_balance - Decimal('0.01'))
 
         # Verify nothing new in wallet
         assert_equal(total_txs, len(self.nodes[0].listtransactions("*", 99999)))