[refactor] Check CTxMemPool options in ctor

[TheCharlatan]

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]

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:

• #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
• #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

[TheCharlatan]

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

[stickies-v]

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]

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]

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());

[TheCharlatan]

Re #28830 (review)

nit: some IWYU fixes, mostly because of the newly introduced bilingual_str and Assert usage:

A bunch of these were broken before this PR, so I wasn't sure if they make sense to repair here. For test files I stopped asking for repairing them when reviewing other PRs, it just feels too Sisyphean if they are not consistently checked by other reviewers anyway. I wish we'd start enforcing this properly though.

[TheCharlatan]

Thank you for the review @stickies-v,

Updated dacdb79 -> 27af391 (mempoolArgs_7 -> mempoolArgs_8, compare)

• Addressed @stickies-v's comment, checking specific error message in fuzz test.
• Addressed @stickies-v's comment, cleaned up some includes.

[stickies-v]

re-ACK 27af391

[ryanofsky]

Code review ACK 27af391

[TheCharlatan]

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]

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]

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]

ACK 856c856

[TheCharlatan]

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.

[stickies-v]

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]

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]

ACK 09ef322