refactor: Clean up nScriptCheckThreads #17342

jnewbery · 2019-11-01T15:29:30Z

The meaning of this value is confusing. Refactor it and add comments.

practicalswift · 2019-11-01T16:01:47Z

ACK 6775a18 -- agree regarding the confusion

laanwj · 2019-11-02T12:23:30Z

I think it would be much more straightforward if 0 (anything smaller than 1) was the disallowed value, not 1.

Either that, or make the meaning the number of additional threads spawned, which can be 0.

(Not that I disagree on adding documentation, but maybe it would be better to make this code less confusing instead, there is no deeper reason why it needs to be like this, the value isn't even exposed on the RPC interface)

jnewbery · 2019-11-02T21:29:45Z

@laanwj

I think it would be much more straightforward if...

Turns out that a global int is not needed at all. We only need the bool-ness of whether any script checking threads have been spawned. I have a branch here: https://github.com/jnewbery/bitcoin/tree/2019-11-nScriptCheckThreads2 that changes the int for a bool, improves commenting everywhere and makes the code clearer.

I fear it'd be NACKed by the "don't change code for purely cosmetic reasons" crowd, but if you prefer it, let me know.

promag · 2019-11-03T21:48:26Z

@jnewbery FWIW the other branch LGTM.

I fear it'd be NACKed by the "don't change code for purely cosmetic reasons" crowd

It's not cosmetic, it's a good refactor IMO.

laanwj · 2019-11-04T14:31:58Z

Refactor LGTM, we've both been in the group of people that were confused by this code at some point.

Turns out that a global int is not needed at all. We only need the bool-ness of whether any script checking threads have been spawned.

Wonder if we can take this a step further and not have any global at all, passing the flag into the ChainState constructor instead.

jnewbery · 2019-11-04T18:53:17Z

Thanks for the feedback everyone. I've changed this PR so it now refactors nScriptCheckThreads and adds clarifying comments.

Wonder if we can take this a step further and not have any global at all, passing the flag into the ChainState constructor instead.

Yes, it'd be great to remove more globals and make validation initialization take these as arguments, but I think that's too far to go in this PR.

jnewbery · 2019-11-04T19:36:00Z

Tested that number of script threads started is the same before and after this PR:

(bitcoind-pre17342 is a bitcoind compiled from commit bc3fcf3 in master and bitcoind-post17342 is a bitcoind compiled from this branch)

for par in {-4..17}; do echo par=$par; ./bitcoind-pre17342 -par=$par && sleep 2 && bitcoin-cli stop && grep "Script verification" .bitcoin/regtest/debug.log && rm .bitcoin/regtest/debug.log && sleep 1; ./bitcoind-post17342 -par=$par && sleep 2 && bitcoin-cli stop && grep "Script verification" .bitcoin/regtest/debug.log && rm .bitcoin/regtest/debug.log && sleep 1;done
par=-4
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:30:57Z Script verification uses 0 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:00Z Script verification uses 0 additional threads
par=-3
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:03Z Script verification uses 0 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:06Z Script verification uses 0 additional threads
par=-2
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:10Z Script verification uses 1 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:13Z Script verification uses 1 additional threads
par=-1
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:16Z Script verification uses 2 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:19Z Script verification uses 2 additional threads
par=0
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:22Z Script verification uses 3 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:26Z Script verification uses 3 additional threads
par=1
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:29Z Script verification uses 0 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:32Z Script verification uses 0 additional threads
par=2
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:35Z Script verification uses 1 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:31:38Z Script verification uses 1 additional threads
[...]
ar=15
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:32:58Z Script verification uses 14 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:33:01Z Script verification uses 14 additional threads
par=16
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:33:04Z Script verification uses 15 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:33:07Z Script verification uses 15 additional threads
par=17
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:33:11Z Script verification uses 15 additional threads
Bitcoin Core starting
Bitcoin Core stopping
2019-11-04T19:33:14Z Script verification uses 15 additional threads

DrahtBot · 2019-11-04T23:44:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17383 (Refactor: Move consts to their correct translation units by jnewbery)
#16658 (validation: Rename CheckInputs to CheckInputScripts by jnewbery)

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.

promag

Tested ACK 1856073.

This is not a breaking change since the configured -par is clamped so existing configurations will work. Also verified that bitcoin-qt does work if configured with 16 before this change - the setting nThreadsScriptVerif remains 16 but the GUI also clamps to 15.

src/validation.cpp

src/test/setup_common.cpp

src/test/checkqueue_tests.cpp

laanwj · 2019-11-06T09:37:06Z

code review ACK 1856073

It's only needed for a hardcoded int, which we can define locally.

The global nScriptCheckThreads int is confusing and is only needed for its int-ness in AppInitMain. Move all `-par` parsing logic there and replace the int nScriptCheckThreads with a bool g_parallel_script_checks. Also tidy up logic and improve comments.

jnewbery · 2019-11-06T20:05:46Z

I wanted to remove the unused validation.h include, so I picked up @promag's other changes along the way.

Thanks for the review!

sipa · 2019-11-06T20:54:53Z

ACK 5506ecf

promag · 2019-11-06T22:59:33Z

ACK 5506ecf, only change was addressing my nits.

laanwj · 2019-11-07T10:53:21Z

Code review ACK 5506ecf

DrahtBot · 2019-11-07T14:27:05Z

Needs rebase

maflcko · 2019-11-07T15:06:48Z

ACK 5506ecf 🥐

Show signature and timestamp

Signature:

Timestamp of file with hash 6804ad6c56d3cf372758297498bf984f409018ae306e96929df7d927368d1bd1 -

5506ecf [refactor] Replace global int nScriptCheckThreads with bool (John Newbery) d995762 [tests] Don't use TestingSetup in the checkqueue_tests (John Newbery) Pull request description: The meaning of this value is confusing. Refactor it and add comments. ACKs for top commit: sipa: ACK 5506ecf promag: ACK 5506ecf, only change was addressing my nits. laanwj: Code review ACK 5506ecf MarcoFalke: ACK 5506ecf 🥐 Tree-SHA512: 78536727c98d2c23f3c0f3f169131474fef9a4486ae65029011caf06eab30f6f70ff73a65b2fb04a5d969fc1150858d1c6ea4767f04d48c1eea6b829316d0e63

maflcko · 2019-11-07T15:11:34Z

src/test/setup_common.cpp

-    nScriptCheckThreads = 3;
-    for (int i = 0; i < nScriptCheckThreads - 1; i++)
+    // Start script-checking threads. Set g_parallel_script_checks to true so they are used.
+    constexpr int script_check_threads = 2;


style-nit: compile time constants are generally ALL_UPPER_CASE

5506ecf [refactor] Replace global int nScriptCheckThreads with bool (John Newbery) d995762 [tests] Don't use TestingSetup in the checkqueue_tests (John Newbery) Pull request description: The meaning of this value is confusing. Refactor it and add comments. ACKs for top commit: sipa: ACK 5506ecf promag: ACK 5506ecf, only change was addressing my nits. laanwj: Code review ACK 5506ecf MarcoFalke: ACK 5506ecf 🥐 Tree-SHA512: 78536727c98d2c23f3c0f3f169131474fef9a4486ae65029011caf06eab30f6f70ff73a65b2fb04a5d969fc1150858d1c6ea4767f04d48c1eea6b829316d0e63

…ue_tests Summary: ~~It's only needed for a hardcoded int, which we can define locally.~~ Note from Backporter: Core's description of this is indeed somewhat confusing. nScriptCheckThreads is removed in the next commit. bitcoin/bitcoin@d995762 --- Partial backport of Core [[bitcoin/bitcoin#17342 | PR17342]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7274

Summary: The global nScriptCheckThreads int is confusing and is only needed for its int-ness in AppInitMain. Move all `-par` parsing logic there and ~~replace~~ remove the int nScriptCheckThreads ~~with a bool g_parallel_script_checks.~~ (see D521) Also tidy up logic and improve comments. bitcoin/bitcoin@5506ecf --- Concludes backport of Core [[bitcoin/bitcoin#17342 | PR17342]] Depends on D7274 Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7275

Summary: > test/functional/rpc_fundrawtransaction.py is fairly long to run and has no logging, so it can appear to be stalled. > > This commit adds info logging at each test to provide feedback on the test run. This is a backport of Core [[bitcoin/bitcoin#17342 | PR17342]] [1/2] bitcoin/bitcoin@94fcc08 Test Plan: `ninja && test/functional/test_runner.py rpc_fundr*` While the test is running, in another terminal: `tail -f test/tmp/test_runner_₿₵_🏃_20201102_112740/rpc_fundrawtransaction_475/test_framework.log` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8234

5506ecf [refactor] Replace global int nScriptCheckThreads with bool (John Newbery) d995762 [tests] Don't use TestingSetup in the checkqueue_tests (John Newbery) Pull request description: The meaning of this value is confusing. Refactor it and add comments. ACKs for top commit: sipa: ACK 5506ecf promag: ACK 5506ecf, only change was addressing my nits. laanwj: Code review ACK 5506ecf MarcoFalke: ACK 5506ecf 🥐 Tree-SHA512: 78536727c98d2c23f3c0f3f169131474fef9a4486ae65029011caf06eab30f6f70ff73a65b2fb04a5d969fc1150858d1c6ea4767f04d48c1eea6b829316d0e63

17342

partial bitcoin#15842, merge bitcoin#15849, bitcoin#17342, bitcoin#18710: Add local thread pool to CCheckQueue

DrahtBot added Docs Validation labels Nov 1, 2019

jnewbery force-pushed the 2019-11-nScriptCheckThreads-comment branch from 6775a18 to fa080f1 Compare November 4, 2019 18:48

jnewbery changed the title ~~Doc: Add comment to nScriptCheckThreads~~ Refactor: Clean up nScriptCheckThreads Nov 4, 2019

jnewbery removed the Docs label Nov 4, 2019

jnewbery force-pushed the 2019-11-nScriptCheckThreads-comment branch from fa080f1 to 1856073 Compare November 4, 2019 19:32

promag reviewed Nov 6, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/test/setup_common.cpp Outdated Show resolved Hide resolved

src/test/checkqueue_tests.cpp Show resolved Hide resolved

jnewbery added 2 commits November 6, 2019 15:03

[tests] Don't use TestingSetup in the checkqueue_tests

d995762

It's only needed for a hardcoded int, which we can define locally.

jnewbery force-pushed the 2019-11-nScriptCheckThreads-comment branch from 1856073 to 5506ecf Compare November 6, 2019 20:05

fanquake changed the title ~~Refactor: Clean up nScriptCheckThreads~~ refactor: Clean up nScriptCheckThreads Nov 6, 2019

DrahtBot added the Needs rebase label Nov 7, 2019

maflcko merged commit 5506ecf into bitcoin:master Nov 7, 2019

maflcko reviewed Nov 7, 2019

View reviewed changes

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

fanquake removed the Needs rebase label Jul 26, 2020

kwvg added a commit to kwvg/dash that referenced this pull request Jun 4, 2021

Merge bitcoin#17342: Clean up nScriptCheckThreads

0a7ba5c

kwvg added a commit to kwvg/dash that referenced this pull request Jun 5, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

078caf7

17342

kwvg mentioned this pull request Jun 5, 2021

partial #15842, merge #15849, #17342, #18710: Add local thread pool to CCheckQueue dashpay/dash#4191

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jun 6, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

6ae21bf

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 6, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

17ab6b4

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 7, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

8da0e1f

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 8, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

0a08b5f

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 8, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

770755e

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 9, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

d3cefbf

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 10, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

d6f92c2

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 11, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

902951c

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 16, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

f669b3a

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 16, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

a6fa6ed

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 24, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

9e1c8a3

17342

kwvg added a commit to kwvg/dash that referenced this pull request Jun 25, 2021

merge bitcoin#17342: Clean up nScriptCheckThreads

03a3f6c

17342

UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 26, 2021

Merge pull request #4191 from kittywhiskers/checkqueue

c5cc285

partial bitcoin#15842, merge bitcoin#15849, bitcoin#17342, bitcoin#18710: Add local thread pool to CCheckQueue

This was referenced Sep 8, 2021

Boost usage reduction DeFiCh/ain#714

Closed

C++17 and reduce Boost usage DeFiCh/ain#701

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Clean up nScriptCheckThreads #17342

refactor: Clean up nScriptCheckThreads #17342

jnewbery commented Nov 1, 2019 •

edited

Loading

practicalswift commented Nov 1, 2019

laanwj commented Nov 2, 2019 •

edited

Loading

jnewbery commented Nov 2, 2019

promag commented Nov 3, 2019 •

edited

Loading

laanwj commented Nov 4, 2019

jnewbery commented Nov 4, 2019

jnewbery commented Nov 4, 2019

DrahtBot commented Nov 4, 2019 •

edited

Loading

promag left a comment

laanwj commented Nov 6, 2019

jnewbery commented Nov 6, 2019

sipa commented Nov 6, 2019

promag commented Nov 6, 2019

laanwj commented Nov 7, 2019

DrahtBot commented Nov 7, 2019

maflcko commented Nov 7, 2019

maflcko Nov 7, 2019

refactor: Clean up nScriptCheckThreads #17342

refactor: Clean up nScriptCheckThreads #17342

Conversation

jnewbery commented Nov 1, 2019 • edited Loading

practicalswift commented Nov 1, 2019

laanwj commented Nov 2, 2019 • edited Loading

jnewbery commented Nov 2, 2019

promag commented Nov 3, 2019 • edited Loading

laanwj commented Nov 4, 2019

jnewbery commented Nov 4, 2019

jnewbery commented Nov 4, 2019

DrahtBot commented Nov 4, 2019 • edited Loading

Conflicts

promag left a comment

Choose a reason for hiding this comment

laanwj commented Nov 6, 2019

jnewbery commented Nov 6, 2019

sipa commented Nov 6, 2019

promag commented Nov 6, 2019

laanwj commented Nov 7, 2019

DrahtBot commented Nov 7, 2019

maflcko commented Nov 7, 2019

maflcko Nov 7, 2019

Choose a reason for hiding this comment

jnewbery commented Nov 1, 2019 •

edited

Loading

laanwj commented Nov 2, 2019 •

edited

Loading

promag commented Nov 3, 2019 •

edited

Loading

DrahtBot commented Nov 4, 2019 •

edited

Loading