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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31313.
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.
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);
LogInfo instead of the deprecated alias. Also, no need for the trailing newline.
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};
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.
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();
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)},
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:
0script_threads = 16.
1worker_threads_num = std::clamp(script_threads-1, 0, 15);
2...
3Outcome: 15 workers
Current Code
0script_threads = std::clamp(16, 0, 15); -> output 15
1worker_threads_num = script_threads - 1;
2..
3Outcome: 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:
0worker_threads_num = script_threads - 1;
1worker_threads_num = std::clamp(worker_threads_num, 0 , 15);
Which should be equivalent?
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);
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.
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:
0diff --git a/src/init.cpp b/src/init.cpp
1index 94a5a08463..61c6f060f1 100644
2--- a/src/init.cpp
3+++ b/src/init.cpp
4@@ -32,6 +32,7 @@
5 #include <interfaces/ipc.h>
6 #include <interfaces/mining.h>
7 #include <interfaces/node.h>
8+#include <kernel/chainstatemanager_opts.h>
9 #include <kernel/context.h>
10 #include <key.h>
11 #include <logging.h>
12diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
13index 1b605f3d55..95827c5d00 100644
14--- a/src/kernel/chainstatemanager_opts.h
15+++ b/src/kernel/chainstatemanager_opts.h
16@@ -8,6 +8,7 @@
17 #include <kernel/notifications_interface.h>
18
19 #include <arith_uint256.h>
20+#include <common/system.h>
21 #include <dbwrapper.h>
22 #include <script/sigcache.h>
23 #include <txdb.h>
24@@ -23,6 +24,8 @@ class ValidationSignals;
25
26 static constexpr bool DEFAULT_CHECKPOINTS_ENABLED{true};
27 static constexpr auto DEFAULT_MAX_TIP_AGE{24h};
28+/** -par default (number of script-checking threads) */
29+static const int DEFAULT_SCRIPTCHECK_THREADS{GetNumCores() - 1};
30
31 namespace kernel {
32
33@@ -48,7 +51,7 @@ struct ChainstateManagerOpts {
34 Notifications& notifications;
35 ValidationSignals* signals{nullptr};
36 //! Number of script check worker threads. Zero means no parallel verification.
37- int worker_threads_num{0};
38+ int worker_threads_num{DEFAULT_SCRIPTCHECK_THREADS};
39 size_t script_execution_cache_bytes{DEFAULT_SCRIPT_EXECUTION_CACHE_BYTES};
40 size_t signature_cache_bytes{DEFAULT_SIGNATURE_CACHE_BYTES};
41 };
42diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
43index cdc8bdd43e..e4a2546b2b 100644
44--- a/src/node/chainstatemanager_args.cpp
45+++ b/src/node/chainstatemanager_args.cpp
46@@ -53,14 +53,13 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
47 ReadDatabaseArgs(args, opts.coins_db);
48 ReadCoinsViewArgs(args, opts.coins_view);
49
50- int script_threads = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS);
51- if (script_threads <= 0) {
52+ if (auto script_threads{args.GetIntArg("-par")}) {
53 // -par=0 means autodetect (number of cores - 1 script threads)
54 // -par=-n means "leave n cores free" (number of cores - n - 1 script threads)
55- script_threads += GetNumCores();
56+ if (*script_threads <= 0) *script_threads += GetNumCores();
57+ // Subtract 1 because the main thread counts towards the par threads.
58+ opts.worker_threads_num = *script_threads - 1;
59 }
60- // Subtract 1 because the main thread counts towards the par threads.
61- opts.worker_threads_num = script_threads - 1;
62
63 if (auto max_size = args.GetIntArg("-maxsigcachesize")) {
64 // 1. When supplied with a max_size of 0, both the signature cache and
65diff --git a/src/node/chainstatemanager_args.h b/src/node/chainstatemanager_args.h
66index af13aa8d3c..701515953e 100644
67--- a/src/node/chainstatemanager_args.h
68+++ b/src/node/chainstatemanager_args.h
69@@ -10,9 +10,6 @@
70
71 class ArgsManager;
72
73-/** -par default (number of script-checking threads, 0 = auto) */
74-static constexpr int DEFAULT_SCRIPTCHECK_THREADS{0};
75-
76 namespace node {
77 [[nodiscard]] util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts);
78 } // namespace node
system.h into common/ in the first place.
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 🛳
Signature:
0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f 🛳
3734xeHF9f3ity3igNXyAMAL6TkSeQwh210l3ePSperJeZRaGjWiwT2JXpcoFGGpGKEJeuhQNMdAHMP1/krz5CQ==
