[refactor] Check CTxMemPool options in ctor #28830

TheCharlatan commented at 11:24 AM on November 9, 2023: contributor

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 (review).

DrahtBot commented at 11:24 AM on November 9, 2023: contributor

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.

TheCharlatan renamed this:
~~[refactor] Check CTxMemPool options in ctor~~
[refactor] Improve CTxMemPool options handling
on Nov 9, 2023

TheCharlatan renamed this:
~~[refactor] Improve CTxMemPool options handling~~
[refactor] Check CTxMemPool options in ctor
on Nov 9, 2023

TheCharlatan marked this as ready for review on Nov 10, 2023

DrahtBot added the label Needs rebase on Dec 1, 2023

TheCharlatan force-pushed on Dec 1, 2023

DrahtBot removed the label Needs rebase on Dec 1, 2023

in src/test/fuzz/package_eval.cpp:130 in 29818f5f83 outdated

 124 | @@ -125,7 +125,9 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 125 |      mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
 126 |  
 127 |      // ...and construct a CTxMemPool from it
 128 | -    return CTxMemPool{mempool_opts};
 129 | +    std::optional<bilingual_str> error{};
 130 | +    return CTxMemPool{mempool_opts, error};
 131 | +    Assert(!error);

stickies-v commented at 11:04 AM on December 5, 2023:

I don't think this actually does anything? (+here)

maflcko commented at 1:42 PM on December 5, 2023:

Funny that compilers compile this code

maflcko commented at 2:19 PM on December 5, 2023:

Clang has an -Wunreachable-code. Maybe someone can integrate this into the build system by default?

Edit: #28999

TheCharlatan commented at 10:41 PM on December 5, 2023:

Uff, embarassing.

in src/txmempool.cpp:403 in 29818f5f83 outdated

 399 | @@ -400,8 +400,8 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
 400 |      assert(int(nSigOpCostWithAncestors) >= 0);
 401 |  }
 402 |  
 403 | -CTxMemPool::CTxMemPool(const Options& opts)
 404 | -    : m_check_ratio{opts.check_ratio},
 405 | +CTxMemPool::CTxMemPool(const Options& opts, std::optional<bilingual_str>& error)

stickies-v commented at 1:32 PM on December 5, 2023:

I think this approach is less safe than it has to be, it's pretty easy to not handle the error. Would it make more sense to make the ctor private and add a util::Result<CTxMemPool> Make(const Options& opts) factory method?

stickies-v commented at 1:33 PM on December 5, 2023: contributor

Concept ACK

TheCharlatan force-pushed on Dec 5, 2023

TheCharlatan commented at 10:38 PM on December 5, 2023: contributor

Rebased 29818f5f839bb0a08980da6f6dbb980ef7754652 -> 1d18cc6cb30a875c4b6bea946a8c19d42e76dbcf (mempoolArgs_0 -> mempoolArgs_1, compare)

Updated 1d18cc6cb30a875c4b6bea946a8c19d42e76dbcf -> 3aa41a31690da2ae22b6a15a9a5de0de78a01757 (mempoolArgs_1 -> mempoolArgs_2, compare)

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

DrahtBot added the label CI failed on Dec 6, 2023

in src/test/fuzz/package_eval.cpp:151 in 3aa41a3169 outdated

 146 | @@ -147,8 +147,8 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
 147 |      auto outpoints_updater = std::make_shared<OutpointsUpdater>(mempool_outpoints);
 148 |      RegisterSharedValidationInterface(outpoints_updater);
 149 |  
 150 | -    CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)};
 151 | -    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_);
 152 | +    auto tx_pool_{Assert(MakeMempool(fuzzed_data_provider, node))};
 153 | +    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&*tx_pool_);

stickies-v commented at 5:00 PM on December 11, 2023:

nit

    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.get());

in src/init.cpp:1534 in 3aa41a3169 outdated

1485 |      for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
1486 | -        node.mempool = std::make_unique<CTxMemPool>(mempool_opts);
1487 | +        auto mempool{CTxMemPool::Make(mempool_opts)};
1488 | +        if (!mempool) {
1489 | +            return InitError(ErrorString(mempool));
1490 | +        }

stickies-v commented at 5:27 PM on December 11, 2023:

nit: could limit scope of mempool:

diff --git a/src/init.cpp b/src/init.cpp
index eb88a3fa73..1fa34b77e8 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1476,11 +1476,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     }
 
     for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
-        auto mempool{CTxMemPool::Make(mempool_opts)};
-        if (!mempool) {
+        if (auto mempool{CTxMemPool::Make(mempool_opts)}) {
+            node.mempool = std::move(mempool.value());
+        } else {
             return InitError(ErrorString(mempool));
         }
-        node.mempool = std::move(mempool.value());
         LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));

</details>

in src/test/fuzz/mini_miner.cpp:113 in 3aa41a3169 outdated

 109 | @@ -109,7 +110,8 @@ FUZZ_TARGET(mini_miner, .init = initialize_miner)
 110 |  FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
 111 |  {
 112 |      FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
 113 | -    CTxMemPool pool{CTxMemPool::Options{}};
 114 | +    auto tx_pool{std::move(Assert(CTxMemPool::Make(CTxMemPool::Options{})).value())};

stickies-v commented at 5:35 PM on December 11, 2023:

nit: is there a meaningful use for Assert here (and in most other places in this pull) since we already use util::Result::value() and there are no side effects?

in src/txmempool.h:434 in 3aa41a3169 outdated

 430 | @@ -428,12 +431,11 @@ class CTxMemPool
 431 |                                                                const Limits& limits
 432 |                                                                ) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 433 |  
 434 | +

stickies-v commented at 12:22 PM on December 12, 2023:

nit: unnecessary newline?

in src/txmempool.h:457 in 3aa41a3169 outdated

 453 | @@ -452,7 +454,7 @@ class CTxMemPool
 454 |       * accepting transactions becomes O(N^2) where N is the number of transactions
 455 |       * in the pool.
 456 |       */
 457 | -    explicit CTxMemPool(const Options& opts);
 458 | +    static util::Result<std::unique_ptr<CTxMemPool>> Make(const Options& opts);

stickies-v commented at 12:29 PM on December 12, 2023:

nit: I'd intuitively expect Make to return a (wrapped) CTxMemPool. Would it make sense to name this method MakeUnique?

in src/test/fuzz/rbf.cpp:42 in 3aa41a3169 outdated

  37 | @@ -38,7 +38,8 @@ FUZZ_TARGET(rbf, .init = initialize_rbf)
  38 |          return;
  39 |      }
  40 |  
  41 | -    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
  42 | +    auto mempool{std::move(Assert(CTxMemPool::Make(MemPoolOptionsForTest(g_setup->m_node))).value())};
  43 | +    CTxMemPool& pool = *mempool;

stickies-v commented at 1:35 PM on December 12, 2023:

nit: if we're okay dropping the Assert, would just one-line this (+here , here). Otherwise, not a fan of using mempool (or tx_pool) as var names, when the difference with pool is one being a ptr to the other. ppool or pool_ptr (my preference) might be more clear?

    CTxMemPool& pool{*std::move(CTxMemPool::Make(MemPoolOptionsForTest(g_setup->m_node)).value())};

TheCharlatan commented at 9:49 PM on December 12, 2023:

Doesn't this leave an unsafe dangling reference if we don't have a variable capturing ownership of the unique_ptr?

stickies-v commented at 9:26 AM on December 13, 2023:

Ah, you're right, that'd be bad.

in src/test/fuzz/package_eval.cpp:128 in 3aa41a3169 outdated

 124 | @@ -125,7 +125,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 125 |      mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
 126 |  
 127 |      // ...and construct a CTxMemPool from it
 128 | -    return CTxMemPool{mempool_opts};
 129 | +    return std::move(Assert(CTxMemPool::Make(mempool_opts)).value());

stickies-v commented at 2:39 PM on December 12, 2023:

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.

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)

</details>

stickies-v commented at 2:42 PM on December 12, 2023: contributor

LGTM 3aa41a31690da2ae22b6a15a9a5de0de78a01757 when fuzzer is fixed

DrahtBot added the label Needs rebase on Dec 14, 2023

TheCharlatan force-pushed on Dec 15, 2023

TheCharlatan commented at 9:32 AM on December 15, 2023: contributor

Rebased 3aa41a31690da2ae22b6a15a9a5de0de78a01757 -> 571fa4f4c5c9792457a8a6ec77da980ae2d239e7 (mempoolArgs_2 -> mempoolArgs_3, compare)

Updated 571fa4f4c5c9792457a8a6ec77da980ae2d239e7 -> 364456f6598b135fcc0acab8a660658b4407f837 (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?

DrahtBot removed the label CI failed on Dec 15, 2023

DrahtBot removed the label Needs rebase on Dec 15, 2023

in src/test/fuzz/tx_pool.cpp:212 in 364456f659 outdated

 213 | +            SetMempoolConstraints(*node.args, fuzzed_data_provider);
 214 | +            tx_pool_ =  MakeMempool(fuzzed_data_provider, node);
 215 | +        }
 216 | +    }
 217 | +
 218 | +    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.value().get());

stickies-v commented at 2:27 PM on December 15, 2023:

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

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

</details>

TheCharlatan force-pushed on Dec 15, 2023

DrahtBot added the label CI failed on Jan 16, 2024

DrahtBot added the label Needs rebase on Mar 9, 2024

achow101 requested review from ryanofsky on Apr 9, 2024

ryanofsky commented at 5:30 PM on April 16, 2024: contributor

This seems like an improvement, but I think the original suggestion of just adding a std::string& error output parameter to the CTxMemPool constructor (or a util::Result<void>& result output parameter) would provide a less rigid API than making the CTxMemPool constructor private and requiring every instance to be allocated on the heap and owned by a unique_ptr. This would also allow keeping the fuzz test behavior unchanged. The new fuzz test behavior is probably better, but maybe fuzz testing the max_size_bytes < descendant_limit_bytes case could find some bugs.

I don't think this pr is so high priority, so it could be closed if it too much work to update. I'm also happy to review it with or without suggested changes if it is updated.

(EDIT: Changed ~~std::error& error~~ to std::string& error above. That was just a confusing typo.)

TheCharlatan force-pushed on Apr 21, 2024

TheCharlatan commented at 9:14 AM on April 21, 2024: contributor

Reworked 364456f6598b135fcc0acab8a660658b4407f837 -> 30a1024b63105a97d04149e83ae2d8cf0830cf69 (mempoolArgs_4 -> mempoolArgs_5, compare)

Reworked this after @ryanofsky's comment. I agree that the better option here is to have an output parameter. Wasn't sure what std::error& error referred to, so just chose util::Error, but would be easy enough to use something else.

DrahtBot removed the label Needs rebase on Apr 21, 2024

DrahtBot removed the label CI failed on Apr 21, 2024

in src/test/fuzz/package_eval.cpp:133 in 30a1024b63 outdated

 126 | @@ -127,7 +127,8 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 127 |      mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
 128 |  
 129 |      // ...and construct a CTxMemPool from it
 130 | -    return CTxMemPool{mempool_opts};
 131 | +    util::Error error{};
 132 | +    return CTxMemPool{mempool_opts, error};

ryanofsky commented at 2:35 PM on April 25, 2024:

In commit "[[refactor]] Check CTxMemPool options in constructor" (30a1024b63105a97d04149e83ae2d8cf0830cf69)

Would be good to add comment pointing out that error is currently ignored. (And maybe that this ensures mempool code passes fuzz tests even when max mempool size is unreasonably small.)

ryanofsky approved

ryanofsky commented at 2:41 PM on April 25, 2024: contributor

Code review ACK 30a1024b63105a97d04149e83ae2d8cf0830cf69

Wasn't sure what std::error& error referred to, so just chose util::Error, but would be easy enough to use something else.

Sorry, that was just a confusing typo. I mean to write std::string& error. But I guess that doesn't work because the error message is actually translated. In that case, I do think bilingual& error would be better than util::Error& error just to make code simpler and more obvious. But no strong opinion.

DrahtBot requested review from stickies-v on Apr 25, 2024

TheCharlatan force-pushed on Apr 25, 2024

TheCharlatan commented at 8:16 PM on April 25, 2024: contributor

Thanks for taking another look at @ryanofsky!

Updated 30a1024b63105a97d04149e83ae2d8cf0830cf69 -> d447bdcfb0e38353940e4a7fc89d09482d8d39c3 (mempoolArgs_5 -> mempoolArgs_6, compare)

Addressed @ryanofsky's comment, replacing util::Error with bilingual_str.
Addressed @ryanofsky's comment, adding a comment explaining with the error in the package_eval fuzz test is ignored.

ryanofsky approved

ryanofsky commented at 2:37 PM on May 6, 2024: contributor

Code review ACK d447bdcfb0e38353940e4a7fc89d09482d8d39c3. Thanks for the updates!

in src/init.cpp:1529 in d447bdcfb0 outdated

1529 | -    if (mempool_opts.max_size_bytes < 0 || mempool_opts.max_size_bytes < descendant_limit_bytes) {
1530 | -        return InitError(strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0)));
1531 | -    }
1532 | -    LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));
1533 |  
1534 |      for (bool fLoaded = false; !fLoaded && !ShutdownRequested(node);) {

glozow commented at 12:30 PM on May 13, 2024:

The downside to this patch is that the log line may now be printed more than once in the for loop.

Question: do we expect this to loop many times / under what circumstances? I tried adding logging to see what happens on my node, and I haven't seen it go more than once yet.

ryanofsky commented at 12:49 PM on May 13, 2024:

re: #28830 (review)

Question: do we expect this to loop many times / under what circumstances? I tried adding logging to see what happens on my node, and I haven't seen it go more than once yet.

I think this is only intended to loop if there is an error loading, and the user is using a gui, and they are asked "Do you want to rebuild the block database now?" and they answer yes.

glozow commented at 12:55 PM on May 13, 2024:

Thanks @ryanofsky! That would explain why I didn't see it loop.

TheCharlatan commented at 1:06 PM on May 13, 2024:

I'm not too concerned over the message being printed again, since there a whole bunch of log lines that get printed again too. The for loop should also only be executed once more, since if there is a failure on its second pass, the reindex option is true, making it return an InitError.

glozow commented at 1:08 PM on May 13, 2024:

Thanks sgtm :+1: I didn't want to ignore the comment, but also wasn't sure how to test.

glozow commented at 12:30 PM on May 13, 2024: member

ACK d447bdcfb0e38353940e4a7fc89d09482d8d39c3 - code review, light fuzzing

DrahtBot added the label Needs rebase on May 15, 2024

TheCharlatan force-pushed on May 15, 2024

TheCharlatan commented at 9:20 AM on May 15, 2024: contributor

Rebased d447bdcfb0e38353940e4a7fc89d09482d8d39c3 -> dacdb7962c5fef8db26f6fa31facb606165d1d1e (mempoolArgs_6 -> mempoolArgs_7, compare)

Fixed conflict with https://github.com/bitcoin/bitcoin/pull/29086

DrahtBot removed the label Needs rebase on May 15, 2024

in src/init.cpp:1535 in dacdb7962c outdated

1538 | +        bilingual_str mempool_error;
1539 | +        node.mempool = std::make_unique<CTxMemPool>(mempool_opts, mempool_error);
1540 | +        if (!mempool_error.empty()) {
1541 | +            return InitError(mempool_error);
1542 | +        }
1543 | +        LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));

stickies-v commented at 10:36 AM on May 15, 2024:

nit: perhaps move semantics would make sense here?

        node.mempool = std::make_unique<CTxMemPool>(std::move(mempool_opts), mempool_error);
        if (!mempool_error.empty()) {
            return InitError(mempool_error);
        }
        LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), node.mempool->m_opts.max_size_bytes * (1.0 / 1024 / 1024));

TheCharlatan commented at 11:43 AM on May 15, 2024:

Not sure about this if it gets moved again when the for loop iterates.

stickies-v commented at 12:52 PM on May 15, 2024:

Oops no you're absolutely right, I didn't see the for-loop.

in src/test/fuzz/package_eval.cpp:130 in dacdb7962c outdated

 125 | @@ -126,8 +126,11 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 126 |      mempool_opts.check_ratio = 1;
 127 |      mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
 128 |  
 129 | +    // ... ignore the error since it might be beneficial to fuzz even when the
 130 | +    // mempool size is unreasonably small

stickies-v commented at 11:28 AM on May 15, 2024:

nit: this approach has the potential risk that if additional error handling is added to the CTxMemPool ctor, the fuzzer would silently ignore it if this test is not also updated. Could avoid that by specifically silencing only this error?

<details> <summary>git diff on dacdb7962c</summary>

diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index eefa566d31..1a95ff4ab6 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -107,7 +107,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)
 {
     // Take the default options for tests...
     CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)};
@@ -126,11 +126,13 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
     mempool_opts.check_ratio = 1;
     mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
 
-    // ... ignore the error since it might be beneficial to fuzz even when the
-    // mempool size is unreasonably small
     bilingual_str error;
     // ...and construct a CTxMemPool from it
-    return CTxMemPool{mempool_opts, error};
+    auto mempool{std::make_unique<CTxMemPool>(std::move(mempool_opts), error)};
+    // ... ignore the error since it might be beneficial to fuzz even when the
+    // mempool size is unreasonably small
+    assert(error.empty() || error.original.starts_with("-maxmempool must be at least"));
+    return mempool;
 }
 
 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
@@ -152,8 +154,8 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
     auto outpoints_updater = std::make_shared<OutpointsUpdater>(mempool_outpoints);
     node.validation_signals->RegisterSharedValidationInterface(outpoints_updater);
 
-    CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)};
-    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_);
+    auto tx_pool_{MakeMempool(fuzzed_data_provider, node)};
+    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.get());
 
     chainstate.SetMempool(&tx_pool);

</details>

stickies-v approved

stickies-v commented at 11:31 AM on May 15, 2024: contributor

ACK dacdb7962c5fef8db26f6fa31facb606165d1d1e

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

<details> <summary>git diff on dacdb7962c</summary>

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>

</details>

DrahtBot requested review from ryanofsky on May 15, 2024

DrahtBot requested review from glozow on May 15, 2024

TheCharlatan commented at 12:17 PM on May 15, 2024: contributor

Re #28830#pullrequestreview-2057592650

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 force-pushed on May 15, 2024

TheCharlatan commented at 12:45 PM on May 15, 2024: contributor

Thank you for the review @stickies-v,

Updated dacdb7962c5fef8db26f6fa31facb606165d1d1e -> 27af3910e5fa9915d4913b82443db216d1b1d61b (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 commented at 12:59 PM on May 15, 2024: contributor

re-ACK 27af3910e5fa9915d4913b82443db216d1b1d61b

in src/txmempool.cpp:400 in 27af3910e5 outdated

 396 | @@ -395,9 +397,19 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
 397 |      assert(int(nSigOpCostWithAncestors) >= 0);
 398 |  }
 399 |  
 400 | -CTxMemPool::CTxMemPool(const Options& opts)
 401 | -    : m_opts{opts}
 402 | +static CTxMemPool::Options SetOptionLimits(CTxMemPool::Options&& opts)

ryanofsky commented at 1:27 PM on May 15, 2024:

In commit "[[refactor]] Check CTxMemPool options in constructor" (27af3910e5fa9915d4913b82443db216d1b1d61b)

I think it would be clearer to avoid using && and std::move, since using these doesn't actually make things more efficient. In 27af3910e5fa9915d4913b82443db216d1b1d61b, one copy and one move of the option struct will happen. The copy happens when calling the CTxMemPool constructor (since the parameter type is a value parameter), the move happens when returning from SetOptionLimits.

But you could get the same result just using plain references and values:

--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -397,19 +397,20 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
     assert(int(nSigOpCostWithAncestors) >= 0);
 }
 
-static CTxMemPool::Options SetOptionLimits(CTxMemPool::Options&& opts)
+//! Clamp option values and return error if options are not valid.
+static CTxMemPool::Options NormalizeOptions(CTxMemPool::Options opts, bilingual_str& error)
 {
     opts.check_ratio = std::clamp<int>(opts.check_ratio, 0, 1'000'000);
+    int64_t descendant_limit_bytes = opts.limits.descendant_size_vbytes * 40;
+    if (opts.max_size_bytes < 0 || opts.max_size_bytes < descendant_limit_bytes) {
+        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
+    }
     return opts;
 }
 
-CTxMemPool::CTxMemPool(Options opts, bilingual_str& error)
-    : m_opts{SetOptionLimits(std::move(opts))}
+CTxMemPool::CTxMemPool(const Options& opts, bilingual_str& error)
+    : m_opts{NormalizeOptions(opts, error)}
 {
-    int64_t descendant_limit_bytes = m_opts.limits.descendant_size_vbytes * 40;
-    if (m_opts.max_size_bytes < 0 || m_opts.max_size_bytes < descendant_limit_bytes) {
-        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
-    }
 }
 
 bool CTxMemPool::isSpent(const COutPoint& outpoint) const

This avoids need for std::move and &&, keeps the CTxMemPool constructor parameter unchanged, still does one copy and one move when calling and returning from NormalizeOptions. This also keeps error checking and clamping in the same function, which could be helpful if more options are added, because if clamping happens in one function and error checking happens in another function it could make the checking less flexible or require duplicate code.

TheCharlatan commented at 2:07 PM on May 15, 2024:

I was just re-using the pattern used for the ChainstateManager construction, but I like this more. Taking.

ryanofsky commented at 2:47 PM on May 15, 2024:

re: #28830 (review)

I was just re-using the pattern used for the ChainstateManager construction, but I like this more. Taking.

Oh, that's interesting. Looking at that, I can see how it's better than what I suggested because it allows callers to potentially move options without copying them at all. Current init code does copy the options because it is looping, but we don't need to require other callers to copy as well.

So you might want to keep this code like it is, though I would still suggest combining error checking and clamping into the same function.

You could also consider using the name Flatten to be consistent with other code. And you could also change the return type to Options&& instead of Options to be consistent with Flatten avoid an unnecessary move:

https://github.com/bitcoin/bitcoin/blob/33303b2b296cdb21b6ade3e95663e9ed58c08753/src/validation.cpp#L6029

Whichever approach you like seems good to me, and the PR also seems fine in its current form.

TheCharlatan commented at 3:11 PM on May 15, 2024:

Flatten avoid an unnecessary move:

What do you mean with this?

ryanofsky commented at 5:42 PM on May 15, 2024:

Flatten avoid an unnecessary move:

What do you mean with this?

I was suggesting returning Options&& instead of Options to be consistent with the validation.cpp flatten code, and to avoid two moves potentially happening on return: One move on the return opts; statement which the compiler can't elide because opts is not a local variable, and another move after the function returns to initialize m_opts member.

In practice, however, it looks like only one move will happen, because even though the compiler can't apply NRVO when returning opts it can use RVO to initialize m_opts in-place. So in practice returning either Options or Options&& should be equivalent, and I could see reasons for choosing either approach.

I think I still might opt to copy the validation.cpp flatten name and types so the pattern is the same both places, while also adding a string& error output parameter so error checking can happen alongside flattening. But any approach you want to take seems fine to me. And thanks for asking what was wrong with returning an Options value, because I was incorrectly assuming it was less efficient than it really is.

maflcko commented at 1:07 PM on July 5, 2024:

The reason to use std::move in ChainstateManager was not to optimize for anything, but to get the tidy-error for use-after-move, because using the options un-flattened may later on lead to an assertion error or other runtime error.

ryanofsky approved

ryanofsky commented at 1:51 PM on May 15, 2024: contributor

Code review ACK 27af3910e5fa9915d4913b82443db216d1b1d61b

TheCharlatan force-pushed on May 15, 2024

TheCharlatan commented at 2:09 PM on May 15, 2024: contributor

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

Updated 27af3910e5fa9915d4913b82443db216d1b1d61b -> 33f2a708e392edb2501f555efef34a558da9d717 (mempoolArgs_8 -> mempoolArgs_9, compare)

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

ryanofsky approved

ryanofsky commented at 5:46 PM on May 15, 2024: contributor

Code review ACK 33f2a708e392edb2501f555efef34a558da9d717. 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.

DrahtBot requested review from stickies-v on May 15, 2024

TheCharlatan force-pushed on May 15, 2024

TheCharlatan commented at 6:02 PM on May 15, 2024: contributor

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

Updated 33f2a708e392edb2501f555efef34a558da9d717 -> 856c8563ca342303a83977146df22768bb6e2c7e (mempoolArgs_9 -> mempoolArgs_10, compare)

Changed the constructor to be more like the ChainstateManager.

ryanofsky approved

ryanofsky commented at 6:07 PM on May 15, 2024: contributor

Code review ACK 856c8563ca342303a83977146df22768bb6e2c7e, 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.

in src/txmempool.cpp:400 in 856c8563ca outdated

 396 | @@ -395,8 +397,19 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
 397 |      assert(int(nSigOpCostWithAncestors) >= 0);
 398 |  }
 399 |  
 400 | -CTxMemPool::CTxMemPool(const Options& opts)
 401 | -    : m_opts{opts}
 402 | +//! Clamp option values and return error if options are not valid.

stickies-v commented at 2:50 PM on May 16, 2024:

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

stickies-v approved

stickies-v commented at 2:55 PM on May 16, 2024: contributor

ACK 856c8563ca342303a83977146df22768bb6e2c7e

achow101 commented at 7:57 PM on May 17, 2024: member

ACK 856c8563ca342303a83977146df22768bb6e2c7e

in src/test/fuzz/tx_pool.cpp:133 in 856c8563ca outdated

 127 | @@ -126,7 +128,9 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 128 |      mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
 129 |  
 130 |      // ...and construct a CTxMemPool from it
 131 | -    return CTxMemPool{mempool_opts};
 132 | +    bilingual_str error;
 133 | +    return CTxMemPool{mempool_opts, error};
 134 | +    Assert(error.empty());

achow101 commented at 8:02 PM on May 17, 2024:

../../../src/test/fuzz/tx_pool.cpp: In function ‘CTxMemPool {anonymous}::MakeMempool(FuzzedDataProvider&, const node::NodeContext&)’:
../../../src/test/fuzz/tx_pool.cpp:134:1: error: control reaches end of non-void function [-Werror=return-type]
  134 | }
      | ^

Also seems like the Assert is unreachable too.

TheCharlatan commented at 8:06 PM on May 17, 2024:

Ugh, I'll push a fix.

TheCharlatan force-pushed on May 17, 2024

TheCharlatan commented at 8:26 PM on May 17, 2024: contributor

856c8563ca342303a83977146df22768bb6e2c7e -> 09ef322acc0a88a9e119f74923399598984c68f6 (mempoolArgs_10 -> mempoolArgs_11, compare)

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

[[refactor]] Check CTxMemPool options in constructor

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

09ef322acc

TheCharlatan force-pushed on May 17, 2024

stickies-v commented at 9:18 AM on May 18, 2024: contributor

re-ACK 09ef322acc0a88a9e119f74923399598984c68f6 . 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.

DrahtBot requested review from ryanofsky on May 18, 2024

DrahtBot requested review from achow101 on May 18, 2024

in src/test/fuzz/package_eval.cpp:112 in 09ef322acc

 108 | @@ -107,7 +109,7 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains
 109 |      SetMockTime(time);
 110 |  }
 111 |  
 112 | -CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
 113 | +std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)

ryanofsky commented at 12:59 PM on June 10, 2024:

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

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 approved

ryanofsky commented at 1:02 PM on June 10, 2024: contributor

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

ryanofsky approved

ryanofsky commented at 1:15 PM on June 10, 2024: contributor

[deleted comment accidentally posted to wrong pr]

DrahtBot requested review from ryanofsky on Jun 10, 2024

achow101 commented at 7:04 PM on June 11, 2024: member

ACK 09ef322acc0a88a9e119f74923399598984c68f6

achow101 merged this on Jun 11, 2024

achow101 closed this on Jun 11, 2024

maflcko commented at 1:11 PM on July 5, 2024: member

pm-lgtm

achow101 referenced this in commit d7f956a309 on Sep 30, 2024

bitcoin locked this on Jul 5, 2025