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).
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).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
Reviewers, this pull request conflicts with the following ones:
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.
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);
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)
error
. Would it make more sense to make the ctor private and add a util::Result<CTxMemPool> Make(const Options& opts)
factory method?
Rebased 29818f5f839bb0a08980da6f6dbb980ef7754652 -> 1d18cc6cb30a875c4b6bea946a8c19d42e76dbcf (mempoolArgs_0 -> mempoolArgs_1, compare)
Updated 1d18cc6cb30a875c4b6bea946a8c19d42e76dbcf -> 3aa41a31690da2ae22b6a15a9a5de0de78a01757 (mempoolArgs_1 -> mempoolArgs_2, compare)
Result
type and a factory method instead of error string parameters.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_);
nit
0 MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.get());
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+ }
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
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())};
Assert
here (and in most other places in this pull) since we already use util::Result::value()
and there are no side effects?
430@@ -428,12 +431,11 @@ class CTxMemPool
431 const Limits& limits
432 ) const EXCLUSIVE_LOCKS_REQUIRED(cs);
433
434+
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);
Make
to return a (wrapped) CTxMemPool
. Would it make sense to name this method MakeUnique
?
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;
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())};
unique_ptr
?
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());
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)
Rebased 3aa41a31690da2ae22b6a15a9a5de0de78a01757 -> 571fa4f4c5c9792457a8a6ec77da980ae2d239e7 (mempoolArgs_2 -> mempoolArgs_3, compare)
Updated 571fa4f4c5c9792457a8a6ec77da980ae2d239e7 -> 364456f6598b135fcc0acab8a660658b4407f837 (mempoolArgs_3 -> mempoolArgs_4, compare)
get()
to access underlying pointermempool
var in init
.Make
to MakeUnique
.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?
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());
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());
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 to std::error& error
std::string& error
above. That was just a confusing typo.)
Reworked 364456f6598b135fcc0acab8a660658b4407f837 -> 30a1024b63105a97d04149e83ae2d8cf0830cf69 (mempoolArgs_4 -> mempoolArgs_5, compare)
std::error& error
referred to, so just chose util::Error
, but would be easy enough to use something else.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};
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.)
Code review ACK 30a1024b63105a97d04149e83ae2d8cf0830cf69
Wasn’t sure what
std::error& error
referred to, so just choseutil::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.
Thanks for taking another look at @ryanofsky!
Updated 30a1024b63105a97d04149e83ae2d8cf0830cf69 -> d447bdcfb0e38353940e4a7fc89d09482d8d39c3 (mempoolArgs_5 -> mempoolArgs_6, compare)
util::Error
with bilingual_str
.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);) {
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.
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.
reindex
option is true, making it return an InitError
.
Rebased d447bdcfb0e38353940e4a7fc89d09482d8d39c3 -> dacdb7962c5fef8db26f6fa31facb606165d1d1e (mempoolArgs_6 -> mempoolArgs_7, compare)
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));
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));
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
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
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>
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.
Thank you for the review @stickies-v,
Updated dacdb7962c5fef8db26f6fa31facb606165d1d1e -> 27af3910e5fa9915d4913b82443db216d1b1d61b (mempoolArgs_7 -> mempoolArgs_8, compare)
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)
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.
ChainstateManager
construction, but I like this more. Taking.
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:
Whichever approach you like seems good to me, and the PR also seems fine in its current form.
Flatten avoid an unnecessary move:
What do you mean with this?
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.
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.
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)
Sorry for the many pushes, but I think it’s important to get this consistent.
Updated 33f2a708e392edb2501f555efef34a558da9d717 -> 856c8563ca342303a83977146df22768bb6e2c7e (mempoolArgs_9 -> mempoolArgs_10, compare)
ChainstateManager
.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.
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.
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()
)
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());
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.
856c8563ca342303a83977146df22768bb6e2c7e -> 09ef322acc0a88a9e119f74923399598984c68f6 (mempoolArgs_10 -> mempoolArgs_11, compare)
This ensures that the tests run the same checks on the mempool options
that the init code also applies.
tx_pool
fuzz test, which makes sense when looking at how the mempool options are constructed in SetMempoolConstraints
.
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)
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};