pool: Add and use MemPoolOptions, ApplyArgsManOptions
ClosedPublic
Actions

Authored by PiRK on Jan 26 2024, 16:06.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCba26a00e7ace: pool: Add and use MemPoolOptions, ApplyArgsManOptions

Summary

Note that CTxMemPool now requires a non-defaulted
CTxMemPool::Options for its constructor. Meaning that there's no need to
worry about a stray CTxMemPool constructor somewhere defaulting to
something incorrect. All instances of CTxMemPool construction are
addressed here in this commit.

We set options for CTxMemPool and construct it in many different ways. A
good example can be seen in how we determine CTxMemPool's check_ratio in
AppInitMain(...).

We first set the default based on chainparams's DefaultConsistencyChecks()
Then, we apply the ArgsManager option on top of that default
Finally, we clamp the result of that between 0 and 1 Million

With this patch, most CTxMemPool construction are along the lines of:

MemPoolOptions mempool_opts{...default overrides...};
ApplyArgsManOptions(argsman, mempool_opts);
...hard overrides...
CTxMemPool pool{mempool_opts};

This "compositional" style of building options means that we can omit
unnecessary/irrelevant steps wherever we want but also maintain full
customizability.

For example:

For users of libbitcoinkernel, where we eventually want to remove ArgsManager, they simply won't call (or even know about) ApplyArgsManOptions.

See src/init.cpp to see how the check_ratio CTxMemPool option works after this change.

A MemPoolOptionsForTest helper was also added.

[META] As this patchset progresses, we will move more and more CTxMemPool-relevant options into MemPoolOptions and add their ArgsMan-related logic to ApplyArgsManOptions.

This is a partial backport of core#25290
https://github.com/bitcoin/bitcoin/pull/25290/commits/f1941e8bfd2eecc478c7660434b1ebf6a64095a0

Depends on D15298

Test Plan

ninja all check-all bitcoin-bench

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 26713
Build 52994: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-clang-tidy · build-debug · build-clang · build-bench
Build 52993: arc lint + arc unit

Event Timeline

PiRK created this revision.Jan 26 2024, 16:06

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 26 2024, 16:06

PiRK requested review of this revision.Jan 26 2024, 16:06

teamcity edited the summary of this revision. (Show Details)Jan 26 2024, 16:06

PiRK edited the summary of this revision. (Show Details)Jan 26 2024, 16:07

PiRK edited the test plan for this revision. (Show Details)

PiRK edited the summary of this revision. (Show Details)

PiRK added a parent revision: D15298: scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit.

Harbormaster completed remote builds in B26688: Diff 44637.Jan 26 2024, 16:34

Build Bitcoin ABC Diffs / Diff Testing (build-bench) failed.

Tail of the build log:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              818.42 |        1,221,866.52 |    0.3% |      0.01 | `CashAddrDecode`

|             ns/byte |              byte/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|               15.19 |       65,831,272.74 |    0.1% |      0.01 | `CashAddrEncode`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          116,494.62 |            8,584.09 |    0.2% |      0.01 | `CoinSelection`
|       21,988,275.00 |               45.48 |    0.4% |      0.24 | `ComplexMemPool`

|             ns/elem |              elem/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              122.06 |        8,193,021.02 |    0.4% |      0.01 | `ConstructGCSFilter`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       10,004,982.00 |               99.95 |    0.3% |      0.11 | `DeserializeAndCheckBlockTest`
|        3,910,612.00 |              255.71 |    0.4% |      0.04 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|        4,297,317.00 |              232.70 |    0.5% |      0.05 | `DuplicateInputs`
|    1,786,406,039.50 |                0.56 |    0.4% |      3.57 | `EvictChained50Tx`
|    1,804,581,361.00 |                0.55 |    0.2% |      3.61 | `EvictChained50TxRev`
|           11,000.20 |           90,907.42 |    0.2% |      0.01 | `EvictionProtection0Networks250Candidates`
|           15,346.76 |           65,160.31 |    0.1% |      0.01 | `EvictionProtection1Networks250Candidates`
|           20,456.65 |           48,883.86 |    0.3% |      0.01 | `EvictionProtection2Networks250Candidates`
|            3,584.11 |          279,009.24 |    0.2% |      0.01 | `EvictionProtection3Networks050Candidates`
|            9,290.55 |          107,636.20 |    0.1% |      0.01 | `EvictionProtection3Networks100Candidates`
|           22,029.62 |           45,393.42 |    0.1% |      0.01 | `EvictionProtection3Networks250Candidates`
|                1.80 |      554,888,467.96 |    1.1% |      0.01 | `FastRandom_1bit`
|                7.18 |      139,275,771.40 |    0.0% |      0.01 | `FastRandom_32bit`
|        7,503,978.00 |              133.26 |    0.4% |      0.08 | `GenerateBlock500ChainedTxs`
|          794,541.00 |            1,258.59 |    0.7% |      0.01 | `GenerateBlock50ChainedTxs`

|             ns/byte |              byte/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|                2.22 |      449,636,050.61 |    0.1% |      0.03 | `HASH_1MB`
|                3.58 |      279,245,583.82 |    0.0% |      0.01 | `HASH_256BYTES`
|                7.66 |      130,492,442.34 |    0.0% |      0.01 | `HASH_64BYTES`

|             ns/elem |              elem/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           18,944.78 |           52,784.98 |    0.6% |      0.01 | `MatchGCSFilter`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       15,356,355.00 |               65.12 |    0.3% |      0.17 | `MempoolAcceptance500ChainedTxs`
|        1,558,919.00 |              641.47 |    0.9% |      0.02 | `MempoolAcceptance50ChainedTxs`
|       16,319,467.00 |               61.28 |    0.4% |      0.18 | `MempoolAcceptance511TxTree`
|        1,973,992.00 |              506.59 |    0.8% |      0.02 | `MempoolAcceptance63TxTree`
bitcoin-bench: ../../src/txmempool.cpp:442: void CTxMemPool::check(const CCoinsViewCache&, int64_t) const: Assertion `mempoolDuplicate.HaveCoin(txin.prevout)' failed.
Child aborted
FAILED: src/bench/CMakeFiles/bench-bitcoin 
cd /work/abc-ci-builds/build-bench/src/bench && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-bench/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-bench/test/log && /usr/bin/cmake -E env /work/abc-ci-builds/build-bench/src/bench/bitcoin-bench -output_csv=BitcoinABC_Bench.csv -output_json=BitcoinABC_Bench.json
ninja: build stopped: cannot make progress due to previous errors.
Build build-bench failed with exit code 1