[refactor] Check CTxMemPool options in ctor #28830

TheCharlatan · 2023-11-09T11:24:01Z

The tests should run the same checks on the mempool options that the init code also applies. The downside to this patch is that the log line may now be printed more than once in the for loop.

This was originally noticed here #25290 (comment).

DrahtBot · 2023-11-09T11:24:04Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, ryanofsky, achow101
Stale ACK	glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30058 (Encapsulate warnings in generalized node::Warnings and remove globals by stickies-v)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#28687 (C++20 std::views::reverse by stickies-v)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

stickies-v

Concept ACK

src/test/fuzz/package_eval.cpp

src/txmempool.cpp

TheCharlatan · 2023-12-05T22:38:50Z

Rebased 29818f5 -> 1d18cc6 (mempoolArgs_0 -> mempoolArgs_1, compare)

Updated 1d18cc6 -> 3aa41a3 (mempoolArgs_1 -> mempoolArgs_2, compare)

Addressed @stickies-v's comment, using the Result type and a factory method instead of error string parameters.

stickies-v

LGTM 3aa41a3 when fuzzer is fixed

src/test/fuzz/package_eval.cpp

src/init.cpp

src/test/fuzz/mini_miner.cpp

src/txmempool.h

src/test/fuzz/rbf.cpp

stickies-v · 2023-12-12T14:39:43Z

src/test/fuzz/package_eval.cpp

@@ -125,7 +125,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
    mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();

    // ...and construct a CTxMemPool from it
-    return CTxMemPool{mempool_opts};
+    return std::move(Assert(CTxMemPool::Make(mempool_opts)).value());


Wdyt about this approach to fix the fuzzer (this impl requires rebase to use c++20 for std::string::starts_with)? This should be efficient without relying too much (but still somewhat) on the implementation of CTxMemPool::Make?

After #25665 is merged, could have a Make return an enum failure value and avoid the errorstring checking.

git diff on 3aa41a3

diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index ec708a5c53..7f8aecb1ae 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -111,13 +111,15 @@ std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider // Take the default options for tests... CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)}; - // ...override specific options for this specific fuzz suite mempool_opts.limits.ancestor_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 50); mempool_opts.limits.ancestor_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000; mempool_opts.limits.descendant_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 50); - mempool_opts.limits.descendant_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000; - mempool_opts.max_size_bytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200) * 1'000'000; + auto set_descendants_size = [&]() { + mempool_opts.limits.descendant_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000; + mempool_opts.max_size_bytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200) * 1'000'000; + }; + set_descendants_size(); mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 999)}; nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(1, 999); @@ -125,7 +127,15 @@ std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); // ...and construct a CTxMemPool from it - return std::move(Assert(CTxMemPool::Make(mempool_opts)).value()); + while (true) { + if (auto res{CTxMemPool::Make(mempool_opts)}) { + return std::move(res.value()); + } else { + // Ensure this harness is updated when additional mempool construction failure paths are introduced + assert(util::ErrorString(res).original.starts_with("-maxmempool must be at least")); + set_descendants_size(); + } + } } FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)

TheCharlatan · 2023-12-15T09:32:13Z

Rebased 3aa41a3 -> 571fa4f (mempoolArgs_2 -> mempoolArgs_3, compare)

Updated 571fa4f -> 364456f (mempoolArgs_3 -> mempoolArgs_4, compare)

Addressed @stickies-v's comment, using get() to access underlying pointer
Addressed @stickies-v's comment, limiting scope of mempool var in init.
Addressed @stickies-v's comment, removed superfluous assertions.
Addressed @stickies-v's comment, removed left over newline.
Addressed @stickies-v's comment, renamed Make to MakeUnique.
Addressed @stickies-v's comment, adjusted ownership holder variable naming to existing pattern.
Applied @stickies-v's patch in a separate commit for fixing the fuzz test.

I'm still not quite happy with the fuzz test, which is why I left it in a separate commit. Maybe a better approach would be to let the fuzzer do its job and just return early in the fuzz test if MakeUnique fails?

stickies-v · 2023-12-15T14:27:13Z

src/test/fuzz/tx_pool.cpp

+    auto tx_pool_{MakeMempool(fuzzed_data_provider, node)};
+    while (true) {
+        if (tx_pool_) {
+            break;
+        } else {
+            // Ensure this harness is updated when additional mempool construction failure paths are introduced
+            assert(util::ErrorString(tx_pool_).original.starts_with("-maxmempool must be at least"));
+            SetMempoolConstraints(*node.args, fuzzed_data_provider);
+            tx_pool_ =  MakeMempool(fuzzed_data_provider, node);
+        }
+    }
+
+    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.value().get());


Since we're resetting all constraints on every loop, is this not overly verbose? (+below)

git diff on 364456f

diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 1400fadea0..7ddc45c4b4 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -200,17 +200,10 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) // The sum of the values of all spendable outpoints constexpr CAmount SUPPLY_TOTAL{COINBASE_MATURITY * 50 * COIN}; - SetMempoolConstraints(*node.args, fuzzed_data_provider); - auto tx_pool_{MakeMempool(fuzzed_data_provider, node)}; - while (true) { - if (tx_pool_) { - break; - } else { - // Ensure this harness is updated when additional mempool construction failure paths are introduced - assert(util::ErrorString(tx_pool_).original.starts_with("-maxmempool must be at least")); - SetMempoolConstraints(*node.args, fuzzed_data_provider); - tx_pool_ = MakeMempool(fuzzed_data_provider, node); - } + util::Result<std::unique_ptr<CTxMemPool>> tx_pool_; + while (!tx_pool_) { + SetMempoolConstraints(*node.args, fuzzed_data_provider); + tx_pool_ = MakeMempool(fuzzed_data_provider, node); } MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.value().get());

stickies-v · 2024-05-15T12:59:45Z

re-ACK 27af391

ryanofsky

Code review ACK 27af391

src/txmempool.cpp

TheCharlatan · 2024-05-15T14:09:47Z

Thanks for the reviews, going to take @ryanofsky's suggestion, hope it is not much work to review again

Updated 27af391 -> 33f2a70 (mempoolArgs_8 -> mempoolArgs_9, compare)

Addressed @ryanofsky's comment, slightly changing the mechanics of the mempool constructor for better clarity.

ryanofsky

Code review ACK 33f2a70. I actually didn't notice this push when I made my last two comments, so feel free to ignore the comments if you want to keep the current code.

TheCharlatan · 2024-05-15T18:02:15Z

Sorry for the many pushes, but I think it's important to get this consistent.

Updated 33f2a70 -> 856c856 (mempoolArgs_9 -> mempoolArgs_10, compare)

Changed the constructor to be more like the ChainstateManager.

ryanofsky

Code review ACK 856c856, just tweaked since last review to be more consistent with validation.cpp Flatten code

Sorry for the many pushes, but I think it's important to get this consistent.

Thanks for the update! I agree it's much nicer to be consistent. And sorry for the detour I caused by not fully understanding the original approach.

stickies-v

ACK 856c856

stickies-v · 2024-05-16T14:50:05Z

src/txmempool.cpp

@@ -395,8 +397,19 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
    assert(int(nSigOpCostWithAncestors) >= 0);
 }

-CTxMemPool::CTxMemPool(const Options& opts)
-    : m_opts{opts}
+//! Clamp option values and return error if options are not valid.


nit: slightly confusing docstring: the Options ref is always returned, error is non-empty if options are not valid. Since Flatten is internal, this is probably something that mostly should be documented in the CTxMemPool ctor anyway though (highlighting that callers should check that error.empty())

achow101 · 2024-05-17T19:57:16Z

ACK 856c856

src/test/fuzz/tx_pool.cpp

TheCharlatan · 2024-05-17T20:26:29Z

856c856 -> 09ef322 (mempoolArgs_10 -> mempoolArgs_11, compare)

Addressed @achow101's comment, fixing unreachable Assert.
Slightly adjusted wording in docstring as per @stickies-v's comment.

This ensures that the tests run the same checks on the mempool options that the init code also applies.

stickies-v · 2024-05-18T09:18:18Z

re-ACK 09ef322 . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the tx_pool fuzz test, which makes sense when looking at how the mempool options are constructed in SetMempoolConstraints.

ryanofsky

Code review ACK 09ef322. Just fuzz test error checking fix and updated comment since last review

ryanofsky · 2024-06-10T12:59:14Z

src/test/fuzz/package_eval.cpp

@@ -107,7 +109,7 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains
    SetMockTime(time);
 }

-CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
+std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)


In commit "[[refactor]] Check CTxMemPool options in constructor" (09ef322)

It's a shame this has to return a pointer now. It would have been nice to avoid the unreachable code with something like scope_exit, but I guess that is still experimental and hasn't been standardized.

bilingual_str error; auto error_check{std::experimental::scope_exit([&] { Assert(error.empty()); })}; return CTxMemPool{mempool_opts, error};

ryanofsky

[deleted comment accidentally posted to wrong pr]

achow101 · 2024-06-11T19:04:14Z

ACK 09ef322

maflcko · 2024-07-05T13:11:16Z

pm-lgtm

e9d60af refactor: Replace init retry for loop with if statement (TheCharlatan) c1d8870 refactor: Move most of init retry for loop to a function (TheCharlatan) 781c01f init: Check mempool arguments in AppInitParameterInteractions (TheCharlatan) Pull request description: The for loop around the chain loading logic in `init.cpp` allows users of the GUI to retry once on failure with reindexing without having to manually set the reindex flag on startup. However this current mechanism has problems: * It is badly documented and has led to confusion among developers and bugs making it into master. Examples: * https://github.com/bitcoin/bitcoin/pull/28830/files#r1598392660 * #30132 (comment) * It can only ever iterate once, making the choice of a for loop questionable. * With its large scope it is easy for re-entry bugs to sneak in. Example: * #28830 (comment) Attempt to fix this by moving the bulk of the logic into a separate function and replacing the for loop with a simpler `if` statement. The diff's in this pull request are best reviewed with `--color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`. The error behaviour can be tested by either manually making `LoadChainstate` return a failure, or deleting some of the block index database files. ACKs for top commit: maflcko: review ACK e9d60af 🚸 josibake: crACK e9d60af achow101: ACK e9d60af ryanofsky: Code review ACK e9d60af. Nice change to make AppInitMain shorter and more understandable. Tree-SHA512: 5e5c0a5fd1b32225346450f8482f0ae8792e1557cdab1518112c1a3ec3a4400b64f5796692245cc5bf2f9010bb97b3a9558f07626a285ccd6ae525dd671ead13

TheCharlatan changed the title ~~[refactor] Check CTxMemPool options in ctor~~ [refactor] Improve CTxMemPool options handling Nov 9, 2023

TheCharlatan changed the title ~~[refactor] Improve CTxMemPool options handling~~ [refactor] Check CTxMemPool options in ctor Nov 9, 2023

This was referenced Nov 9, 2023

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Merged

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

TheCharlatan marked this pull request as ready for review November 10, 2023 06:52

DrahtBot mentioned this pull request Nov 28, 2023

kernel: Remove dependency on CScheduler #28960

Merged

DrahtBot added the Needs rebase label Dec 1, 2023

TheCharlatan force-pushed the mempoolArgs branch from 9f635cf to 29818f5 Compare December 1, 2023 22:01

DrahtBot removed the Needs rebase label Dec 1, 2023

stickies-v reviewed Dec 5, 2023

View reviewed changes

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

stickies-v mentioned this pull request Dec 5, 2023

build: Enable -Wunreachable-code #28999

Merged

TheCharlatan force-pushed the mempoolArgs branch from 29818f5 to 3aa41a3 Compare December 5, 2023 22:32

DrahtBot added the CI failed label Dec 6, 2023

This was referenced Dec 6, 2023

refactor: Replace sets of txiter with CTxMemPoolEntryRefs #28886

Closed

Cluster mempool implementation #28676

Open

stickies-v reviewed Dec 12, 2023

View reviewed changes

DrahtBot added the Needs rebase label Dec 14, 2023

TheCharlatan force-pushed the mempoolArgs branch from 3aa41a3 to 364456f Compare December 15, 2023 09:31

DrahtBot removed CI failed Needs rebase labels Dec 15, 2023

DrahtBot mentioned this pull request Dec 15, 2023

refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition #29086

Merged

stickies-v reviewed Dec 15, 2023

View reviewed changes

TheCharlatan force-pushed the mempoolArgs branch from 364456f to de5353b Compare December 15, 2023 16:01

DrahtBot mentioned this pull request Jan 11, 2024

logging: Update to new logging API #29231

Closed

DrahtBot added the CI failed label Jan 16, 2024

DrahtBot mentioned this pull request Jan 17, 2024

log, refactor: Allow log macros to accept context arguments #29256

Open

DrahtBot added the Needs rebase label Mar 9, 2024

ryanofsky approved these changes May 15, 2024

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

TheCharlatan force-pushed the mempoolArgs branch from 27af391 to 33f2a70 Compare May 15, 2024 14:08

ryanofsky approved these changes May 15, 2024

View reviewed changes

DrahtBot requested a review from stickies-v May 15, 2024 17:46

TheCharlatan force-pushed the mempoolArgs branch from 33f2a70 to 856c856 Compare May 15, 2024 18:02

ryanofsky approved these changes May 15, 2024

View reviewed changes

stickies-v approved these changes May 16, 2024

View reviewed changes

achow101 reviewed May 17, 2024

View reviewed changes

src/test/fuzz/tx_pool.cpp Outdated Show resolved Hide resolved

TheCharlatan force-pushed the mempoolArgs branch from 856c856 to f2f50a4 Compare May 17, 2024 20:25

[[refactor]] Check CTxMemPool options in constructor

09ef322

This ensures that the tests run the same checks on the mempool options that the init code also applies.

TheCharlatan force-pushed the mempoolArgs branch from f2f50a4 to 09ef322 Compare May 17, 2024 21:37

DrahtBot requested review from ryanofsky and achow101 May 18, 2024 09:18

This was referenced May 23, 2024

Fee Estimation via Fee rate Forecasters #30157

Draft

C++20 std::views::reverse #28687

Merged

ryanofsky approved these changes Jun 10, 2024

View reviewed changes

DrahtBot requested a review from ryanofsky June 10, 2024 13:15

achow101 merged commit 2251460 into bitcoin:master Jun 11, 2024

TheCharlatan mentioned this pull request Sep 25, 2024

init: Remove retry for loop #30968

Merged

bitcoin locked and limited conversation to collaborators Jul 5, 2025

[refactor] Check CTxMemPool options in ctor #28830

[refactor] Check CTxMemPool options in ctor #28830

Uh oh!

Conversation

TheCharlatan commented Nov 9, 2023

Uh oh!

DrahtBot commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheCharlatan commented Dec 5, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan commented Dec 15, 2023

Uh oh!

stickies-v Dec 15, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v commented May 15, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheCharlatan commented May 15, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan commented May 15, 2024

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v May 16, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 17, 2024

Uh oh!

Uh oh!

TheCharlatan commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v commented May 18, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 11, 2024

Uh oh!

maflcko commented Jul 5, 2024

DrahtBot commented Nov 9, 2023 •

edited

Loading

ryanofsky left a comment •

edited

Loading

TheCharlatan commented May 17, 2024 •

edited

Loading

ryanofsky left a comment •

edited

Loading