diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index aeb3797392..328399c4ad 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -8,6 +8,7 @@ #include #include #include // For CTransactionRef +#include #include #include @@ -260,7 +261,7 @@ public: 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 checkChainLimits(const CTransactionRef& tx) = 0; //! Estimate smart fee. virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc = nullptr) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 2e547e8e2c..6963e928fe 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -45,6 +45,7 @@ #include #include #include +#include #include #include #include @@ -702,14 +703,13 @@ public: limit_ancestor_count = limits.ancestor_count; limit_descendant_count = limits.descendant_count; } - bool checkChainLimits(const CTransactionRef& tx) override + util::Result 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 { diff --git a/src/txmempool.cpp b/src/txmempool.cpp index b783181bb8..acee56fe78 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -196,25 +196,20 @@ util::Result CTxMemPool::CalculateAncestorsAndCheckLimit return ancestors; } -bool CTxMemPool::CheckPackageLimits(const Package& package, - const int64_t total_vsize, - std::string &errString) const +util::Result 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(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(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(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::CalculateMemPoolAncestors( diff --git a/src/txmempool.h b/src/txmempool.h index bac7a445d6..60640acdb0 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -605,11 +605,9 @@ public: * 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 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 diff --git a/src/validation.cpp b/src/validation.cpp index 0501499004..0f3d5d1454 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -51,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -1024,10 +1025,10 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& 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; } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e97e658f38..b51cd6332f 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1296,8 +1296,9 @@ static util::Result 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)}; } } diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 78bfa97212..f798eee365 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -639,7 +639,7 @@ class WalletTest(BitcoinTestFramework): 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)))