Repository: bitcoin/bitcoin
File: src/txmempool.cpp (C++)

Code before:
    for (txiter removeIt : entriesToRemove) {
        UpdateChildrenForRemoval(removeIt);
    }
}

void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount)
{
    nSizeWithDescendants += modifySize;
    assert(nSizeWithDescendants > 0);
    nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee);
    m_count_with_descendants += modifyCount;
    assert(m_count_with_descendants > 0);
}

void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps)
{
    nSizeWithAncestors += modifySize;
    assert(nSizeWithAncestors > 0);
    nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, modifyFee);
    m_count_with_ancestors += modifyCount;
    assert(m_count_with_ancestors > 0);
    nSigOpCostWithAncestors += modifySigOps;
    assert(int(nSigOpCostWithAncestors) >= 0);
}

static CTxMemPool::Options SetOptionLimits(CTxMemPool::Options&& opts)
{
    opts.check_ratio = std::clamp<int>(opts.check_ratio, 0, 1'000'000);
    return opts;
}

CTxMemPool::CTxMemPool(Options opts, bilingual_str& error)
    : m_opts{SetOptionLimits(std::move(opts))}
{
    int64_t descendant_limit_bytes = m_opts.limits.descendant_size_vbytes * 40;
    if (m_opts.max_size_bytes < 0 || m_opts.max_size_bytes < descendant_limit_bytes) {
        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
    }
}

bool CTxMemPool::isSpent(const COutPoint& outpoint) const
{
    LOCK(cs);
    return mapNextTx.count(outpoint);
}

unsigned int CTxMemPool::GetTransactionsUpdated() const
{
    return nTransactionsUpdated;
}


Review comment (performance):
In commit "[[refactor]] Check CTxMemPool options in constructor" (27af3910e5fa9915d4913b82443db216d1b1d61b)

I think it would be clearer to avoid using && and std::move, since using these doesn't actually make things more efficient. In 27af3910e5fa9915d4913b82443db216d1b1d61b, one copy and one move of the option struct will happen. The copy happens when calling the CTxMemPool constructor (since the parameter type is a value parameter), the move happens when returning from SetOptionLimits.

But you could get the same result just using plain references and values:

```diff
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -397,19 +397,20 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
     assert(int(nSigOpCostWithAncestors) >= 0);
 }
 
-static CTxMemPool::Options SetOptionLimits(CTxMemPool::Options&& opts)
+//! Clamp option values and return error if options are not valid.
+static CTxMemPool::Options NormalizeOptions(CTxMemPool::Options opts, bilingual_str& error)
 {
     opts.check_ratio = std::clamp<int>(opts.check_ratio, 0, 1'000'000);
+    int64_t descendant_limit_bytes = opts.limits.descendant_size_vbytes * 40;
+    if (opts.max_size_bytes < 0 || opts.max_size_bytes < descendant_limit_bytes) {
+        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
+    }
     return opts;
 }
 
-CTxMemPool::CTxMemPool(Options opts, bilingual_str& error)
-    : m_opts{SetOptionLimits(std::move(opts))}
+CTxMemPool::CTxMemPool(const Options& opts, bilingual_str& error)
+    : m_opts{NormalizeOptions(opts, error)}
 {
-    int64_t descendant_limit_bytes = m_opts.limits.descendant_size_vbytes * 40;
-    if (m_opts.max_size_bytes < 0 || m_opts.max_size_bytes < descendant_limit_bytes) {
-        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
-    }
 }
 
 bool CTxMemPool::isSpent(const COutPoint& outpoint) const
```

This avoids need for std::move and &&, keeps the `CTxMemPool` constructor parameter unchanged, still does one copy and one move when calling and returning from NormalizeOptions. This also keeps error checking and clamping in the same function, which could be helpful if more options are added, because if clamping happens in one function and error checking happens in another function it could make the checking less flexible or require duplicate code.

Code after:
    for (txiter removeIt : entriesToRemove) {
        UpdateChildrenForRemoval(removeIt);
    }
}

void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount)
{
    nSizeWithDescendants += modifySize;
    assert(nSizeWithDescendants > 0);
    nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee);
    m_count_with_descendants += modifyCount;
    assert(m_count_with_descendants > 0);
}

void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps)
{
    nSizeWithAncestors += modifySize;
    assert(nSizeWithAncestors > 0);
    nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, modifyFee);
    m_count_with_ancestors += modifyCount;
    assert(m_count_with_ancestors > 0);
    nSigOpCostWithAncestors += modifySigOps;
    assert(int(nSigOpCostWithAncestors) >= 0);
}

//! Clamp option values and populate the error if options are not valid.
static CTxMemPool::Options&& Flatten(CTxMemPool::Options&& opts, bilingual_str& error)
{
    opts.check_ratio = std::clamp<int>(opts.check_ratio, 0, 1'000'000);
    int64_t descendant_limit_bytes = opts.limits.descendant_size_vbytes * 40;
    if (opts.max_size_bytes < 0 || opts.max_size_bytes < descendant_limit_bytes) {
        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
    }
    return std::move(opts);
}

CTxMemPool::CTxMemPool(Options opts, bilingual_str& error)
    : m_opts{Flatten(std::move(opts), error)}
{
}

bool CTxMemPool::isSpent(const COutPoint& outpoint) const
{
    LOCK(cs);
    return mapNextTx.count(outpoint);
}

unsigned int CTxMemPool::GetTransactionsUpdated() const
{
    return nTransactionsUpdated;
}