Skip to content

Commit

Permalink
Ensure the DisconnectedBlockTransactions pool has enough space
Browse files Browse the repository at this point in the history
Summary
---

This commit closes issue Bitcoin-ABC#296. In particular, on ScaleNet, the
statically configured maximum bytes for the disconnect pool is 640MB.
This is bytes of *dynamic* usage.  ScaleNet, with its block size of
256MB serialized size -- may exceed this dynamic memory limit (the
dynamic size multiplier for a tx in the disconnectpool may be ~2.89
or more of its serializd size).  As such, for ScaleNet, it is unsafe to
use a disconnectpool that has a max dynamic size of 640MB (see issue Bitcoin-ABC#296
for an explanation as to why).

This commit basically makes the disconnectpool max dynamic size no
longer be a static constant, but calculated at runtime by checking with
the global config object (via GetConfig()).

It ensures the size is max(640MB, MaxMempoolSize).  With default settings,
this change right now only affects scalenet -- for every other network the
old value of 640MB will continue to be used by default. **Unless**, of course,
the  user configures a larger mempool size via `-maxmempool=`.

For scalenet, the new value will be the current maxMempoolSize used on scalenet
(which is 2.5GB by default).

Implementation Details
---

A `static` private member was added to `DisconnectedBlockTransactions`
that calculates the size at runtime, based on current config.

- It just grabs the active config via `GetConfig()`.
  - The alternative would have been to pass down the config object from the
    caller, but I tried that in a branch and the diff was huge.
  - It seemed like overkill as a code change that is only here to fix a minor
    corner case bug discussed in Bitcoin-ABC#296.
  - This codebase anyway uses `GetConfig()`, still, all over the place.
- This function returns `uint64_t` and not `size_t` because it is a
  value that relies on the `GetMaxMempoolSize`, which itself is
  `uint64_t`.

Test Plan
---

- `ninja all check-all`

Sadly, there is no test now that tries to reproduce the bug discussed in Bitcoin-ABC#296,
but careful study of the code should indicate that the issue is real and
not imagined.
  • Loading branch information
cculianu committed Apr 24, 2021
1 parent 5a770b8 commit 6d17407
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 4 deletions.
11 changes: 7 additions & 4 deletions src/txmempool.cpp
Expand Up @@ -1241,8 +1241,9 @@ void CTxMemPool::SetIsLoaded(bool loaded) {
m_is_loaded = loaded;
}

/** Maximum bytes for transactions to store for processing during reorg */
static const size_t MAX_DISCONNECTED_TX_POOL_SIZE = 20 * DEFAULT_EXCESSIVE_BLOCK_SIZE;
/* static */ uint64_t DisconnectedBlockTransactions::maxDynamicUsage() {
return std::max(20 * DEFAULT_EXCESSIVE_BLOCK_SIZE, GetConfig().GetMaxMemPoolSize());
}

void DisconnectedBlockTransactions::addForBlock(const std::vector<CTransactionRef> &vtx) {
for (const auto &tx : reverse_iterate(vtx)) {
Expand Down Expand Up @@ -1291,7 +1292,8 @@ void DisconnectedBlockTransactions::addForBlock(const std::vector<CTransactionRe
}

// Keep the size under control.
while (DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE) {
const auto maxUsage = maxDynamicUsage();
while (DynamicMemoryUsage() > maxUsage) {
// Drop the earliest entry, and remove its children from the
// mempool.
auto it = queuedTx.get<insertion_order>().begin();
Expand Down Expand Up @@ -1337,7 +1339,8 @@ void DisconnectedBlockTransactions::importMempool(CTxMemPool &pool) {

// We limit memory usage because we can't know if more blocks will be
// disconnected
while (DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE) {
const auto maxUsage = maxDynamicUsage();
while (DynamicMemoryUsage() > maxUsage) {
// Drop the earliest entry which, by definition, has no children
removeEntry(queuedTx.get<insertion_order>().begin());
}
Expand Down
5 changes: 5 additions & 0 deletions src/txmempool.h
Expand Up @@ -846,6 +846,11 @@ class DisconnectedBlockTransactions {
/// Note that the returned pointer is only valid for as long as its underlying map node is valid.
const TxInfo *getTxInfo(const CTransactionRef &tx) const;

/// @returns the maximum number of bytes that this instance will use for DynamicMemoryUsage()
/// before txs are culled from this instance. Right now this is max(640MB, maxMempoolSize) and
/// it relies on the global Config object being valid and correctly configured.
static uint64_t maxDynamicUsage();

public:
// It's almost certainly a logic bug if we don't clear out queuedTx before
// destruction, as we add to it while disconnecting blocks, and then we
Expand Down

0 comments on commit 6d17407

Please sign in to comment.