Skip to content

Ensure the DisconnectedBlockTransactions pool has enough space

Calin Culianu requested to merge cculianu/bitcoin-cash-node:fix_issue_296 into master

Summary

This commit closes issue #296 (closed). 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 #296 (closed) 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 #296 (closed).
    • 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 #296 (closed), but careful study of the code should indicate that the issue is real and not imagined.

Edited by Calin Culianu

Merge request reports