[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
Stale ACK	glozow, ryanofsky, achow101

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)
#28687 (C++20 std::views::reverse by stickies-v)

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

TheCharlatan · 2024-05-15T09:20:27Z

Rebased d447bdc -> dacdb79 (mempoolArgs_6 -> mempoolArgs_7, compare)

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

stickies-v

ACK dacdb79

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

git diff on dacdb79

diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp
index a52c566076..3cc28a2b14 100644
--- a/src/test/fuzz/mini_miner.cpp
+++ b/src/test/fuzz/mini_miner.cpp
@@ -12,6 +12,8 @@
 #include <primitives/transaction.h>
 #include <random.h>
 #include <txmempool.h>
+#include <util/check.h>
+#include <util/translation.h>
 
 #include <deque>
 #include <vector>
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index eefa566d31..9b5019696e 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -16,6 +16,7 @@
 #include <test/util/setup_common.h>
 #include <test/util/txmempool.h>
 #include <util/rbf.h>
+#include <util/translation.h>
 #include <validation.h>
 #include <validationinterface.h>
 
diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp
index 8a13a12069..1faf65e6ad 100644
--- a/src/test/fuzz/partially_downloaded_block.cpp
+++ b/src/test/fuzz/partially_downloaded_block.cpp
@@ -10,6 +10,9 @@
 #include <test/util/setup_common.h>
 #include <test/util/txmempool.h>
 #include <txmempool.h>
+#include <util/check.h>
+#include <util/translation.h>
+
 
 #include <cstddef>
 #include <cstdint>
diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp
index 9cf053a290..4c7e70e3b0 100644
--- a/src/test/fuzz/rbf.cpp
+++ b/src/test/fuzz/rbf.cpp
@@ -13,6 +13,8 @@
 #include <test/util/setup_common.h>
 #include <test/util/txmempool.h>
 #include <txmempool.h>
+#include <util/check.h>
+#include <util/translation.h>
 
 #include <cstdint>
 #include <optional>
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 77fd038a79..d62e9f02f2 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -15,7 +15,9 @@
 #include <test/util/script.h>
 #include <test/util/setup_common.h>
 #include <test/util/txmempool.h>
+#include <util/check.h>
 #include <util/rbf.h>
+#include <util/translation.h>
 #include <validation.h>
 #include <validationinterface.h>
 
diff --git a/src/test/fuzz/validation_load_mempool.cpp b/src/test/fuzz/validation_load_mempool.cpp
index 4b0aad8c47..51140ae039 100644
--- a/src/test/fuzz/validation_load_mempool.cpp
+++ b/src/test/fuzz/validation_load_mempool.cpp
@@ -13,7 +13,9 @@
 #include <test/util/setup_common.h>
 #include <test/util/txmempool.h>
 #include <txmempool.h>
+#include <util/check.h>
 #include <util/time.h>
+#include <util/translation.h>
 #include <validation.h>
 
 #include <cstdint>
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index fd153f8207..042a8a9ba5 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -14,8 +14,10 @@
 #include <test/util/txmempool.h>
 #include <txmempool.h>
 #include <uint256.h>
+#include <util/check.h>
 #include <util/strencodings.h>
 #include <util/time.h>
+#include <util/translation.h>
 #include <validation.h>
 #include <versionbits.h>
 
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 816413dbb7..deea6ff27d 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -16,6 +16,7 @@
 #include <policy/settings.h>
 #include <random.h>
 #include <reverse_iterator.h>
+#include <tinyformat.h>
 #include <util/check.h>
 #include <util/feefrac.h>
 #include <util/moneystr.h>
@@ -26,6 +27,7 @@
 #include <util/translation.h>
 #include <validationinterface.h>
 
+#include <algorithm>
 #include <cmath>
 #include <numeric>
 #include <optional>

src/init.cpp

src/test/fuzz/package_eval.cpp

TheCharlatan · 2024-05-15T12:17:50Z

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 · 2024-05-15T12:45:28Z

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 · 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.

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

[WIP] Cluster mempool implementation #28676

Draft

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

Improve new LogDebug/Trace/Info/Warning/Error Macros #29256

Open

DrahtBot added the Needs rebase label Mar 9, 2024

TheCharlatan force-pushed the mempoolArgs branch from d447bdc to dacdb79 Compare May 15, 2024 09:18

DrahtBot removed the Needs rebase label May 15, 2024

stickies-v approved these changes May 15, 2024

View reviewed changes

src/init.cpp Show resolved Hide resolved

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

DrahtBot requested review from ryanofsky and glozow May 15, 2024 11:31

TheCharlatan force-pushed the mempoolArgs branch from dacdb79 to 27af391 Compare May 15, 2024 12:44

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

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] Check CTxMemPool options in ctor #28830

[refactor] Check CTxMemPool options in ctor #28830

TheCharlatan commented Nov 9, 2023

DrahtBot commented Nov 9, 2023 •

edited

stickies-v left a comment

TheCharlatan commented Dec 5, 2023

stickies-v left a comment

stickies-v Dec 12, 2023

TheCharlatan commented Dec 15, 2023

stickies-v Dec 15, 2023

TheCharlatan commented May 15, 2024

stickies-v left a comment

TheCharlatan commented May 15, 2024

TheCharlatan commented May 15, 2024

stickies-v commented May 15, 2024

ryanofsky left a comment

TheCharlatan commented May 15, 2024

ryanofsky left a comment

TheCharlatan commented May 15, 2024

ryanofsky left a comment •

edited

stickies-v left a comment

stickies-v May 16, 2024

achow101 commented May 17, 2024

TheCharlatan commented May 17, 2024 •

edited

stickies-v commented May 18, 2024

[refactor] Check CTxMemPool options in ctor #28830

Are you sure you want to change the base?

[refactor] Check CTxMemPool options in ctor #28830

Conversation

TheCharlatan commented Nov 9, 2023

DrahtBot commented Nov 9, 2023 • edited

Code Coverage

Reviews

Conflicts

stickies-v left a comment

Choose a reason for hiding this comment

TheCharlatan commented Dec 5, 2023

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v Dec 12, 2023

Choose a reason for hiding this comment

TheCharlatan commented Dec 15, 2023

stickies-v Dec 15, 2023

Choose a reason for hiding this comment

TheCharlatan commented May 15, 2024

stickies-v left a comment

Choose a reason for hiding this comment

TheCharlatan commented May 15, 2024

TheCharlatan commented May 15, 2024

stickies-v commented May 15, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

TheCharlatan commented May 15, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

TheCharlatan commented May 15, 2024

ryanofsky left a comment • edited

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v May 16, 2024

Choose a reason for hiding this comment

achow101 commented May 17, 2024

TheCharlatan commented May 17, 2024 • edited

stickies-v commented May 18, 2024

DrahtBot commented Nov 9, 2023 •

edited

ryanofsky left a comment •

edited

TheCharlatan commented May 17, 2024 •

edited