[refactor] Check CTxMemPool options in ctor #28830

pull TheCharlatan wants to merge 1 commits into bitcoin:master from TheCharlatan:mempoolArgs changing 11 files +83 −30
  1. 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).

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

  3. TheCharlatan renamed this:
    [refactor] Check CTxMemPool options in ctor
    [refactor] Improve CTxMemPool options handling
    on Nov 9, 2023
  4. TheCharlatan renamed this:
    [refactor] Improve CTxMemPool options handling
    [refactor] Check CTxMemPool options in ctor
    on Nov 9, 2023
  5. TheCharlatan marked this as ready for review on Nov 10, 2023
  6. DrahtBot added the label Needs rebase on Dec 1, 2023
  7. TheCharlatan force-pushed on Dec 1, 2023
  8. DrahtBot removed the label Needs rebase on Dec 1, 2023
  9. 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.
  10. 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?
  11. stickies-v commented at 1:33 pm on December 5, 2023: contributor
    Concept ACK
  12. TheCharlatan force-pushed on Dec 5, 2023
  13. 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.
  14. DrahtBot added the label CI failed on Dec 6, 2023
  15. 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

    0    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.get());
    
  16. 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:

     0diff --git a/src/init.cpp b/src/init.cpp
     1index eb88a3fa73..1fa34b77e8 100644
     2--- a/src/init.cpp
     3+++ b/src/init.cpp
     4@@ -1476,11 +1476,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     5     }
     6 
     7     for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
     8-        auto mempool{CTxMemPool::Make(mempool_opts)};
     9-        if (!mempool) {
    10+        if (auto mempool{CTxMemPool::Make(mempool_opts)}) {
    11+            node.mempool = std::move(mempool.value());
    12+        } else {
    13             return InitError(ErrorString(mempool));
    14         }
    15-        node.mempool = std::move(mempool.value());
    16         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));
    17 
    18 
    
  17. 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?
  18. 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?
  19. 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?
  20. 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?

    0    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.
  21. 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.

     0diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
     1index ec708a5c53..7f8aecb1ae 100644
     2--- a/src/test/fuzz/package_eval.cpp
     3+++ b/src/test/fuzz/package_eval.cpp
     4@@ -111,13 +111,15 @@ std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider
     5     // Take the default options for tests...
     6     CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)};
     7 
     8-
     9     // ...override specific options for this specific fuzz suite
    10     mempool_opts.limits.ancestor_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 50);
    11     mempool_opts.limits.ancestor_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000;
    12     mempool_opts.limits.descendant_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 50);
    13-    mempool_opts.limits.descendant_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000;
    14-    mempool_opts.max_size_bytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200) * 1'000'000;
    15+    auto set_descendants_size = [&]() {
    16+        mempool_opts.limits.descendant_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000;
    17+        mempool_opts.max_size_bytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200) * 1'000'000;
    18+    };
    19+    set_descendants_size();
    20     mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 999)};
    21     nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(1, 999);
    22 
    23@@ -125,7 +127,15 @@ std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider
    24     mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
    25 
    26     // ...and construct a CTxMemPool from it
    27-    return std::move(Assert(CTxMemPool::Make(mempool_opts)).value());
    28+    while (true) {
    29+        if (auto res{CTxMemPool::Make(mempool_opts)}) {
    30+            return std::move(res.value());
    31+        } else {
    32+            // Ensure this harness is updated when additional mempool construction failure paths are introduced
    33+            assert(util::ErrorString(res).original.starts_with("-maxmempool must be at least"));
    34+            set_descendants_size();
    35+        }
    36+    }
    37 }
    38 
    39 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
    
  22. stickies-v commented at 2:42 pm on December 12, 2023: contributor
    LGTM 3aa41a31690da2ae22b6a15a9a5de0de78a01757 when fuzzer is fixed
  23. DrahtBot added the label Needs rebase on Dec 14, 2023
  24. TheCharlatan force-pushed on Dec 15, 2023
  25. 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)

    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?

  26. DrahtBot removed the label CI failed on Dec 15, 2023
  27. DrahtBot removed the label Needs rebase on Dec 15, 2023
  28. 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)

     0diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
     1index 1400fadea0..7ddc45c4b4 100644
     2--- a/src/test/fuzz/tx_pool.cpp
     3+++ b/src/test/fuzz/tx_pool.cpp
     4@@ -200,17 +200,10 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
     5     // The sum of the values of all spendable outpoints
     6     constexpr CAmount SUPPLY_TOTAL{COINBASE_MATURITY * 50 * COIN};
     7 
     8-    SetMempoolConstraints(*node.args, fuzzed_data_provider);
     9-    auto tx_pool_{MakeMempool(fuzzed_data_provider, node)};
    10-    while (true) {
    11-        if (tx_pool_) {
    12-            break;
    13-        } else {
    14-            // Ensure this harness is updated when additional mempool construction failure paths are introduced
    15-            assert(util::ErrorString(tx_pool_).original.starts_with("-maxmempool must be at least"));
    16-            SetMempoolConstraints(*node.args, fuzzed_data_provider);
    17-            tx_pool_ =  MakeMempool(fuzzed_data_provider, node);
    18-        }
    19+    util::Result<std::unique_ptr<CTxMemPool>> tx_pool_;
    20+    while (!tx_pool_) {
    21+        SetMempoolConstraints(*node.args, fuzzed_data_provider);
    22+        tx_pool_ = MakeMempool(fuzzed_data_provider, node);
    23     }
    24 
    25     MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.value().get());
    
  29. TheCharlatan force-pushed on Dec 15, 2023
  30. DrahtBot added the label CI failed on Jan 16, 2024
  31. DrahtBot added the label Needs rebase on Mar 9, 2024
  32. achow101 requested review from ryanofsky on Apr 9, 2024
  33. 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.)

  34. TheCharlatan force-pushed on Apr 21, 2024
  35. 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.
  36. DrahtBot removed the label Needs rebase on Apr 21, 2024
  37. DrahtBot removed the label CI failed on Apr 21, 2024
  38. 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.)

  39. ryanofsky approved
  40. 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.

  41. DrahtBot requested review from stickies-v on Apr 25, 2024
  42. TheCharlatan force-pushed on Apr 25, 2024
  43. 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.
  44. ryanofsky approved
  45. ryanofsky commented at 2:37 pm on May 6, 2024: contributor
    Code review ACK d447bdcfb0e38353940e4a7fc89d09482d8d39c3. Thanks for the updates!
  46. 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.
  47. glozow commented at 12:30 pm on May 13, 2024: member
    ACK d447bdcfb0e38353940e4a7fc89d09482d8d39c3 - code review, light fuzzing
  48. DrahtBot added the label Needs rebase on May 15, 2024
  49. TheCharlatan force-pushed on May 15, 2024
  50. TheCharlatan commented at 9:20 am on May 15, 2024: contributor

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

  51. DrahtBot removed the label Needs rebase on May 15, 2024
  52. 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?

    0        node.mempool = std::make_unique<CTxMemPool>(std::move(mempool_opts), mempool_error);
    1        if (!mempool_error.empty()) {
    2            return InitError(mempool_error);
    3        }
    4        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.
  53. 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?

     0diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
     1index eefa566d31..1a95ff4ab6 100644
     2--- a/src/test/fuzz/package_eval.cpp
     3+++ b/src/test/fuzz/package_eval.cpp
     4@@ -107,7 +107,7 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains
     5     SetMockTime(time);
     6 }
     7 
     8-CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
     9+std::unique_ptr<CTxMemPool> MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
    10 {
    11     // Take the default options for tests...
    12     CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)};
    13@@ -126,11 +126,13 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
    14     mempool_opts.check_ratio = 1;
    15     mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
    16 
    17-    // ... ignore the error since it might be beneficial to fuzz even when the
    18-    // mempool size is unreasonably small
    19     bilingual_str error;
    20     // ...and construct a CTxMemPool from it
    21-    return CTxMemPool{mempool_opts, error};
    22+    auto mempool{std::make_unique<CTxMemPool>(std::move(mempool_opts), error)};
    23+    // ... ignore the error since it might be beneficial to fuzz even when the
    24+    // mempool size is unreasonably small
    25+    assert(error.empty() || error.original.starts_with("-maxmempool must be at least"));
    26+    return mempool;
    27 }
    28 
    29 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
    30@@ -152,8 +154,8 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
    31     auto outpoints_updater = std::make_shared<OutpointsUpdater>(mempool_outpoints);
    32     node.validation_signals->RegisterSharedValidationInterface(outpoints_updater);
    33 
    34-    CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)};
    35-    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_);
    36+    auto tx_pool_{MakeMempool(fuzzed_data_provider, node)};
    37+    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.get());
    38 
    39     chainstate.SetMempool(&tx_pool);
    40 
    
  54. stickies-v approved
  55. 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:

      0diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp
      1index a52c566076..3cc28a2b14 100644
      2--- a/src/test/fuzz/mini_miner.cpp
      3+++ b/src/test/fuzz/mini_miner.cpp
      4@@ -12,6 +12,8 @@
      5 #include <primitives/transaction.h>
      6 #include <random.h>
      7 #include <txmempool.h>
      8+#include <util/check.h>
      9+#include <util/translation.h>
     10 
     11 #include <deque>
     12 #include <vector>
     13diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
     14index eefa566d31..9b5019696e 100644
     15--- a/src/test/fuzz/package_eval.cpp
     16+++ b/src/test/fuzz/package_eval.cpp
     17@@ -16,6 +16,7 @@
     18 #include <test/util/setup_common.h>
     19 #include <test/util/txmempool.h>
     20 #include <util/rbf.h>
     21+#include <util/translation.h>
     22 #include <validation.h>
     23 #include <validationinterface.h>
     24 
     25diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp
     26index 8a13a12069..1faf65e6ad 100644
     27--- a/src/test/fuzz/partially_downloaded_block.cpp
     28+++ b/src/test/fuzz/partially_downloaded_block.cpp
     29@@ -10,6 +10,9 @@
     30 #include <test/util/setup_common.h>
     31 #include <test/util/txmempool.h>
     32 #include <txmempool.h>
     33+#include <util/check.h>
     34+#include <util/translation.h>
     35+
     36 
     37 #include <cstddef>
     38 #include <cstdint>
     39diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp
     40index 9cf053a290..4c7e70e3b0 100644
     41--- a/src/test/fuzz/rbf.cpp
     42+++ b/src/test/fuzz/rbf.cpp
     43@@ -13,6 +13,8 @@
     44 #include <test/util/setup_common.h>
     45 #include <test/util/txmempool.h>
     46 #include <txmempool.h>
     47+#include <util/check.h>
     48+#include <util/translation.h>
     49 
     50 #include <cstdint>
     51 #include <optional>
     52diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
     53index 77fd038a79..d62e9f02f2 100644
     54--- a/src/test/fuzz/tx_pool.cpp
     55+++ b/src/test/fuzz/tx_pool.cpp
     56@@ -15,7 +15,9 @@
     57 #include <test/util/script.h>
     58 #include <test/util/setup_common.h>
     59 #include <test/util/txmempool.h>
     60+#include <util/check.h>
     61 #include <util/rbf.h>
     62+#include <util/translation.h>
     63 #include <validation.h>
     64 #include <validationinterface.h>
     65 
     66diff --git a/src/test/fuzz/validation_load_mempool.cpp b/src/test/fuzz/validation_load_mempool.cpp
     67index 4b0aad8c47..51140ae039 100644
     68--- a/src/test/fuzz/validation_load_mempool.cpp
     69+++ b/src/test/fuzz/validation_load_mempool.cpp
     70@@ -13,7 +13,9 @@
     71 #include <test/util/setup_common.h>
     72 #include <test/util/txmempool.h>
     73 #include <txmempool.h>
     74+#include <util/check.h>
     75 #include <util/time.h>
     76+#include <util/translation.h>
     77 #include <validation.h>
     78 
     79 #include <cstdint>
     80diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
     81index fd153f8207..042a8a9ba5 100644
     82--- a/src/test/miner_tests.cpp
     83+++ b/src/test/miner_tests.cpp
     84@@ -14,8 +14,10 @@
     85 #include <test/util/txmempool.h>
     86 #include <txmempool.h>
     87 #include <uint256.h>
     88+#include <util/check.h>
     89 #include <util/strencodings.h>
     90 #include <util/time.h>
     91+#include <util/translation.h>
     92 #include <validation.h>
     93 #include <versionbits.h>
     94 
     95diff --git a/src/txmempool.cpp b/src/txmempool.cpp
     96index 816413dbb7..deea6ff27d 100644
     97--- a/src/txmempool.cpp
     98+++ b/src/txmempool.cpp
     99@@ -16,6 +16,7 @@
    100 #include <policy/settings.h>
    101 #include <random.h>
    102 #include <reverse_iterator.h>
    103+#include <tinyformat.h>
    104 #include <util/check.h>
    105 #include <util/feefrac.h>
    106 #include <util/moneystr.h>
    107@@ -26,6 +27,7 @@
    108 #include <util/translation.h>
    109 #include <validationinterface.h>
    110 
    111+#include <algorithm>
    112 #include <cmath>
    113 #include <numeric>
    114 #include <optional>
    
  56. DrahtBot requested review from ryanofsky on May 15, 2024
  57. DrahtBot requested review from glozow on May 15, 2024
  58. 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.

  59. TheCharlatan force-pushed on May 15, 2024
  60. 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)

  61. stickies-v commented at 12:59 pm on May 15, 2024: contributor
    re-ACK 27af3910e5fa9915d4913b82443db216d1b1d61b
  62. 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:

     0--- a/src/txmempool.cpp
     1+++ b/src/txmempool.cpp
     2@@ -397,19 +397,20 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
     3     assert(int(nSigOpCostWithAncestors) >= 0);
     4 }
     5 
     6-static CTxMemPool::Options SetOptionLimits(CTxMemPool::Options&& opts)
     7+//! Clamp option values and return error if options are not valid.
     8+static CTxMemPool::Options NormalizeOptions(CTxMemPool::Options opts, bilingual_str& error)
     9 {
    10     opts.check_ratio = std::clamp<int>(opts.check_ratio, 0, 1'000'000);
    11+    int64_t descendant_limit_bytes = opts.limits.descendant_size_vbytes * 40;
    12+    if (opts.max_size_bytes < 0 || opts.max_size_bytes < descendant_limit_bytes) {
    13+        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
    14+    }
    15     return opts;
    16 }
    17 
    18-CTxMemPool::CTxMemPool(Options opts, bilingual_str& error)
    19-    : m_opts{SetOptionLimits(std::move(opts))}
    20+CTxMemPool::CTxMemPool(const Options& opts, bilingual_str& error)
    21+    : m_opts{NormalizeOptions(opts, error)}
    22 {
    23-    int64_t descendant_limit_bytes = m_opts.limits.descendant_size_vbytes * 40;
    24-    if (m_opts.max_size_bytes < 0 || m_opts.max_size_bytes < descendant_limit_bytes) {
    25-        error = strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1'000'000.0));
    26-    }
    27 }
    28 
    29 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.
  63. ryanofsky approved
  64. ryanofsky commented at 1:51 pm on May 15, 2024: contributor
    Code review ACK 27af3910e5fa9915d4913b82443db216d1b1d61b
  65. TheCharlatan force-pushed on May 15, 2024
  66. 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.
  67. ryanofsky approved
  68. 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.
  69. DrahtBot requested review from stickies-v on May 15, 2024
  70. TheCharlatan force-pushed on May 15, 2024
  71. 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.
  72. ryanofsky approved
  73. 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.

  74. 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())
  75. stickies-v approved
  76. stickies-v commented at 2:55 pm on May 16, 2024: contributor
    ACK 856c8563ca342303a83977146df22768bb6e2c7e
  77. achow101 commented at 7:57 pm on May 17, 2024: member
    ACK 856c8563ca342303a83977146df22768bb6e2c7e
  78. 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:
    0../../../src/test/fuzz/tx_pool.cpp: In function CTxMemPool {anonymous}::MakeMempool(FuzzedDataProvider&, const node::NodeContext&):
    1../../../src/test/fuzz/tx_pool.cpp:134:1: error: control reaches end of non-void function [-Werror=return-type]
    2  134 | }
    3      | ^
    

    Also seems like the Assert is unreachable too.


    TheCharlatan commented at 8:06 pm on May 17, 2024:
    Ugh, I’ll push a fix.
  79. TheCharlatan force-pushed on May 17, 2024
  80. TheCharlatan commented at 8:26 pm on May 17, 2024: contributor

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

  81. [[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
  82. TheCharlatan force-pushed on May 17, 2024
  83. 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.
  84. DrahtBot requested review from ryanofsky on May 18, 2024
  85. DrahtBot requested review from achow101 on May 18, 2024
  86. 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.

    0    bilingual_str error;
    1    auto error_check{std::experimental::scope_exit([&] { Assert(error.empty()); })};
    2    return CTxMemPool{mempool_opts, error};
    
  87. ryanofsky approved
  88. 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
  89. ryanofsky approved
  90. ryanofsky commented at 1:15 pm on June 10, 2024: contributor
    [deleted comment accidentally posted to wrong pr]
  91. DrahtBot requested review from ryanofsky on Jun 10, 2024
  92. achow101 commented at 7:04 pm on June 11, 2024: member
    ACK 09ef322acc0a88a9e119f74923399598984c68f6
  93. achow101 merged this on Jun 11, 2024
  94. achow101 closed this on Jun 11, 2024

  95. maflcko commented at 1:11 pm on July 5, 2024: member
    pm-lgtm

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me