Avoid out-of-bounds user input for PeerManager::Options by safely clamping -maxorphantx and -blockreconstructionextratxn, and avoid platform-specific behaviour by changing PeerManager::Options::max_extra_txs from size_t to a uint32_t. Addresses #27499#pullrequestreview-1544114932.
Also documents all PeerManager::Options members, addressing #27499 (review).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
52+ //! Whether transaction reconciliation protocol is enabled
53 bool reconcile_txs{DEFAULT_TXRECONCILIATION_ENABLE};
54+ //! Maximum number of orphan transactions kept in memory
55 uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS};
56- size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN};
57+ //! Number of orphan+recently-replaced transactions to keep around for block reconstruction
nit, includes a bit more than replaced and orphans
0 //! Number of non-mempool transactions to keep around for block reconstruction. Includes orphan, replaced, and rejected transactions.
utACK 128ad03792cd4aeeaf32807d07f01e3f85adaf28
Thanks for the followup
17+ options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
18 }
19
20 if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) {
21- options.max_extra_txs = size_t(std::max(int64_t{0}, *value));
22+ options.max_extra_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
I’m not really keen on setting limits. I don’t know what good values would be, and ultimately it’s not a huge problem if the value is too high. I think issuing a warning could be sensible as it does have meaningful impact, but I don’t have a strong view. Too many warnings/popups is also not great UX.
For example, this would log a warning in the console, and on qt issue a warning pop-up if a value > 100x default value is entered:
0diff --git a/src/node/peerman_args.cpp b/src/node/peerman_args.cpp
1index efe4514271..53034d6277 100644
2--- a/src/node/peerman_args.cpp
3+++ b/src/node/peerman_args.cpp
4@@ -2,6 +2,8 @@
5
6 #include <common/args.h>
7 #include <net_processing.h>
8+#include <node/interface_ui.h>
9+#include <util/translation.h>
10
11 #include <algorithm>
12 #include <limits>
13@@ -13,10 +15,16 @@ void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& optio
14 if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;
15
16 if (auto value{argsman.GetIntArg("-maxorphantx")}) {
17+ if (*value > (100 * options.max_orphan_txs)) {
18+ InitWarning(_("-maxorphantx is > 100x larger than the default value, affecting memory consumption"));
19+ }
20 options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
21 }
22
23 if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) {
24+ if (*value > (100 * options.max_extra_txs)) {
25+ InitWarning(_("-blockreconstructionextratxn is > 100x larger than the default value, affecting memory consumption"));
26+ }
27 options.max_extra_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
28 }
29
What do you think?
Also changes max_extra_txs into a uint32_t to avoid platform-specific
behaviour
12 {
13 if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;
14
15 if (auto value{argsman.GetIntArg("-maxorphantx")}) {
16- options.max_orphan_txs = uint32_t(std::max(int64_t{0}, *value));
17+ options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
unrelated: May be good to write a clang-tidy plugin to enforce the limits are compile-time constants and in range to avoid silent UB at runtime?
The in-range one can be submitted to upstream and the other check can be done in this repo.
0 options.max_orphan_txs = uint32_t(std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max()));
nit, if you re-touch?