foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-03 09:56:38 -05:00
Code Issues Activity

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

Browse source 
8dec9c560b wallet, mempool: propagete `checkChainLimits` error message to wallet (ismaelsadeeq)

Pull request description:

  * Requested in [#28391 comment](https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382997719)

  * 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`](5800c558eb/src/txmempool.cpp (L199)) provide explicit information about the error message.
  * This PR updates [`CTxMempool::CheckPackageLimits`](5800c558eb/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`](5800c558eb/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 8dec9c560b
  Sjors:
    utACK 8dec9c560b
  TheCharlatan:
    Re-ACK 8dec9c560b

Tree-SHA512: ddeac18aeba6f8e3be0e3fe76bf3db655352e3b415169f1f83ea1e8976a2f3e3de021c8da6880eb8382ab52d545e418e3f4d57adcc68ecb4f390339710ee6f30
This commit is contained in:
glozow 2023-12-18 15:30:45 +00:00
commit dd391944dc
No known key found for this signature in database
GPG key ID: BA03F4DBE0C63FB4
7 changed files with 25 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/interfaces/chain.h
View file

@ -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 @@ 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<void> checkChainLimits(const CTransactionRef& tx) = 0;
//! Estimate smart fee.
virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc = nullptr) = 0;

8
src/node/interfaces.cpp
View file

@ -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 @@ public:
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
{

24
src/txmempool.cpp
View file

@ -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(

6
src/txmempool.h
View file

@ -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<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

7
src/validation.cpp
View file

@ -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;
}

5
src/wallet/spend.cpp
View file

@ -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)};
}
}

2
test/functional/wallet_basic.py
View file

@ -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)))