This ensures the options are applied consistently from contexts where they might not pass through the args manager, such as in some tests, or when used through the kernel library.
This is similar to the patch applied in 09ef322acc0a88a9e119f74923399598984c68f6, used to make applying the mempool options consistent.
This is part of the libbitcoinkernel project #27587
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31313.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | furszy, stickies-v, maflcko, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
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.
130 | @@ -130,6 +131,7 @@ class CCheckQueue 131 | explicit CCheckQueue(unsigned int batch_size, int worker_threads_num) 132 | : nBatchSize(batch_size) 133 | { 134 | + LogPrintf("Script verification uses %d additional threads\n", worker_threads_num);
Seems deliberate to not clamp here, could you explain the reason?
nit: For new code it would be good to use LogInfo instead of the deprecated alias. Also, no need for the trailing newline.
We already control the batch size from the chainman, so seemed logical to me to control the number of threads too.
Do we expect every call-site to do the clamping themselves?
I don't think this really matters, but seems consistent to control the arguments in the same place, just like with the batch size.
77 | @@ -78,6 +78,9 @@ static constexpr int DEFAULT_CHECKLEVEL{3}; 78 | // Setting the target to >= 550 MiB will make it likely we can respect the target. 79 | static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024; 80 | 81 | +/** Maximum number of dedicated script-checking threads allowed */ 82 | +static constexpr int MAX_SCRIPTCHECK_THREADS{15};
Do you happen to know how this threshold was derived and why it's needed in the first place?
If it's because GetNumCores is considered only a hint, why not clamp that instead?
Looking at the original PR there seems to be little justification for it. It was also submitted in 2013, where machines with more than 16 cores were not common.
This current PR is just a refactor, if this should be changed, a separate PR should be opened.
Is script verification completely CPU bound? If so, this limit may still be reasonable for a few more years - but unlike in 2013, it's not an unreasonably big value.
lgtm ACK 09c9ba846ec44ba903de2b32a1cb0f49c7f93cb0
This ensures the options are applied consistently from contexts where
they might not pass through the args manager, such as in some tests, or
when used through the kernel library.
This is similar to the patch applied in 09ef322acc0a88a9e119f74923399598984c68f6.
59 | @@ -60,8 +60,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
60 | script_threads += GetNumCores();
Slightly unrelated, but this doesn't actually return the number of cores, but number of concurrent threads
6259 | @@ -6260,7 +6260,7 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts) 6260 | } 6261 | 6262 | ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Options options, node::BlockManager::Options blockman_options) 6263 | - : m_script_check_queue{/*batch_size=*/128, options.worker_threads_num}, 6264 | + : m_script_check_queue{/*batch_size=*/128, std::clamp(options.worker_threads_num, 0, MAX_SCRIPTCHECK_THREADS)},
as mentioned in the other comment, could we push this into CCheckQueue instead to avoid having to repeat this for other such calls, e.g. https://github.com/bitcoin/bitcoin/pull/31132/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R6278
Updated 09c9ba846ec44ba903de2b32a1cb0f49c7f93cb0 -> 8f85d36d68ab33ba237407a2ed16667eb149d61f (chainstatemanagerArgs_0 -> chainstatemanagerArgs_1, compare)
59 | @@ -60,8 +60,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage 60 | script_threads += GetNumCores(); 61 | } 62 | // Subtract 1 because the main thread counts towards the par threads. 63 | - opts.worker_threads_num = std::clamp(script_threads - 1, 0, MAX_SCRIPTCHECK_THREADS); 64 | - LogPrintf("Script verification uses %d additional threads\n", opts.worker_threads_num);
The max number of workers threads before was 15 and now seems to be 14. E.g. provide -par=16:
Previous Code:
script_threads = 16.
worker_threads_num = std::clamp(script_threads-1, 0, 15);
...
Outcome: 15 workers
Current Code
script_threads = std::clamp(16, 0, 15); -> output 15
worker_threads_num = script_threads - 1;
..
Outcome: 14 workers.
I'm confused, this is printing Script verification uses 15 additional threads for both master and this commit when I provide -par=16. As far as I can tell the logic as applied is:
worker_threads_num = script_threads - 1;
worker_threads_num = std::clamp(worker_threads_num, 0 , 15);
Which should be equivalent?
Yeah, never mind. The subtraction happens before the clamping, not after as I wrote above. So the behavior remains the same. I guess I'm a bit sleepy today.. sorry.
Code ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
130 | @@ -130,6 +131,7 @@ class CCheckQueue 131 | explicit CCheckQueue(unsigned int batch_size, int worker_threads_num) 132 | : nBatchSize(batch_size) 133 | { 134 | + LogInfo("Script verification uses %d additional threads", worker_threads_num);
nit: CCheckQueue is used for other things than ScriptCheck (in test code), but I don't see a better place to put the log statement, and making it a non-refactor commit by e..g updating the wording to "Verification queue uses ..." is probably not worth it either, so disregarding this nit might be the best way forward.
Yeah, I thought of different approaches here too, happy to take an alternative here if somebody comes up with something. Otherwise we'll handle this once we get more queues, or a unified thread pool.
9 | @@ -10,8 +10,6 @@ 10 | 11 | class ArgsManager; 12 | 13 | -/** Maximum number of dedicated script-checking threads allowed */ 14 | -static constexpr int MAX_SCRIPTCHECK_THREADS{15}; 15 | /** -par default (number of script-checking threads, 0 = auto) */ 16 | static constexpr int DEFAULT_SCRIPTCHECK_THREADS{0};
Related (but not required for this PR), the below patch removing default value duplication would ensure the default work_threads_num value doesn't depend on argsmanager being used either:
<details> <summary>git diff on 8f85d36d68</summary>
diff --git a/src/init.cpp b/src/init.cpp
index 94a5a08463..61c6f060f1 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -32,6 +32,7 @@
#include <interfaces/ipc.h>
#include <interfaces/mining.h>
#include <interfaces/node.h>
+#include <kernel/chainstatemanager_opts.h>
#include <kernel/context.h>
#include <key.h>
#include <logging.h>
diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
index 1b605f3d55..95827c5d00 100644
--- a/src/kernel/chainstatemanager_opts.h
+++ b/src/kernel/chainstatemanager_opts.h
@@ -8,6 +8,7 @@
#include <kernel/notifications_interface.h>
#include <arith_uint256.h>
+#include <common/system.h>
#include <dbwrapper.h>
#include <script/sigcache.h>
#include <txdb.h>
@@ -23,6 +24,8 @@ class ValidationSignals;
static constexpr bool DEFAULT_CHECKPOINTS_ENABLED{true};
static constexpr auto DEFAULT_MAX_TIP_AGE{24h};
+/** -par default (number of script-checking threads) */
+static const int DEFAULT_SCRIPTCHECK_THREADS{GetNumCores() - 1};
namespace kernel {
@@ -48,7 +51,7 @@ struct ChainstateManagerOpts {
Notifications& notifications;
ValidationSignals* signals{nullptr};
//! Number of script check worker threads. Zero means no parallel verification.
- int worker_threads_num{0};
+ int worker_threads_num{DEFAULT_SCRIPTCHECK_THREADS};
size_t script_execution_cache_bytes{DEFAULT_SCRIPT_EXECUTION_CACHE_BYTES};
size_t signature_cache_bytes{DEFAULT_SIGNATURE_CACHE_BYTES};
};
diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
index cdc8bdd43e..e4a2546b2b 100644
--- a/src/node/chainstatemanager_args.cpp
+++ b/src/node/chainstatemanager_args.cpp
@@ -53,14 +53,13 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
ReadDatabaseArgs(args, opts.coins_db);
ReadCoinsViewArgs(args, opts.coins_view);
- int script_threads = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS);
- if (script_threads <= 0) {
+ if (auto script_threads{args.GetIntArg("-par")}) {
// -par=0 means autodetect (number of cores - 1 script threads)
// -par=-n means "leave n cores free" (number of cores - n - 1 script threads)
- script_threads += GetNumCores();
+ if (*script_threads <= 0) *script_threads += GetNumCores();
+ // Subtract 1 because the main thread counts towards the par threads.
+ opts.worker_threads_num = *script_threads - 1;
}
- // Subtract 1 because the main thread counts towards the par threads.
- opts.worker_threads_num = script_threads - 1;
if (auto max_size = args.GetIntArg("-maxsigcachesize")) {
// 1. When supplied with a max_size of 0, both the signature cache and
diff --git a/src/node/chainstatemanager_args.h b/src/node/chainstatemanager_args.h
index af13aa8d3c..701515953e 100644
--- a/src/node/chainstatemanager_args.h
+++ b/src/node/chainstatemanager_args.h
@@ -10,9 +10,6 @@
class ArgsManager;
-/** -par default (number of script-checking threads, 0 = auto) */
-static constexpr int DEFAULT_SCRIPTCHECK_THREADS{0};
-
namespace node {
[[nodiscard]] util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts);
} // namespace node
</details>
I'm a bit apprehensive on moving system-related code into the options. It just makes it a bit more complicated to patch out system-related code if we ever choose to ship support for the kernel library on bare metal. It is also why moved system.h into common/ in the first place.
Ah, I hadn't considered that. I suppose there's also no real harm in default values being different when accessed from different contexts. Can be marked as resolved.
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
I noticed this issue while reviewing #30595, and started working on an alternative approach before @TheCharlatan made me aware of this PR. I thought it would be nice to introduce a new BoundedInt type, that avoids relying on input validation and instead forces the underlying value to be bound to a (compile time constant) certain range.
However, there were much less places than I'd anticipated where this could be an immediate drop-in, because the exact bounds often depend on other runtime variables (including worker_threads_num after my comment here). So, I think the current approach in this PR is most suitable - and I'll look into if introducing BoundedInt might be useful in other places in the code, in a (potential) separate PR.
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f 🛳
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f 🛳
734xeHF9f3ity3igNXyAMAL6TkSeQwh210l3ePSperJeZRaGjWiwT2JXpcoFGGpGKEJeuhQNMdAHMP1/krz5CQ==
</details>
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f