refactor: Clean up nScriptCheckThreads #17342

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:2019-11-nScriptCheckThreads-comment changing 5 files +39 −29
  1. jnewbery commented at 3:29 PM on November 1, 2019: member

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

  2. DrahtBot added the label Docs on Nov 1, 2019
  3. DrahtBot added the label Validation on Nov 1, 2019
  4. practicalswift commented at 4:01 PM on November 1, 2019: contributor

    ACK 6775a18cab043af0a9ea69a364804d6d75faf939 -- agree regarding the confusion

  5. laanwj commented at 12:23 PM on November 2, 2019: member

    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)

  6. jnewbery commented at 9:29 PM on November 2, 2019: member

    @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.

  7. promag commented at 9:48 PM on November 3, 2019: member

    @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.

  8. laanwj commented at 2:31 PM on November 4, 2019: member

    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.

  9. jnewbery force-pushed on Nov 4, 2019
  10. jnewbery renamed this:
    Doc: Add comment to nScriptCheckThreads
    Refactor: Clean up nScriptCheckThreads
    on Nov 4, 2019
  11. jnewbery removed the label Docs on Nov 4, 2019
  12. jnewbery commented at 6:53 PM on November 4, 2019: member

    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.

  13. jnewbery force-pushed on Nov 4, 2019
  14. jnewbery commented at 7:36 PM on November 4, 2019: member

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

    (bitcoind-pre17342 is a bitcoind compiled from commit bc3fcf3c0d1d4c83da0fab14d40f92ffca35789e 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
  15. DrahtBot commented at 11:44 PM on November 4, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  16. in src/validation.cpp:110 in 1856073d6e outdated 
     106 | @@ -107,7 +107,7 @@ CBlockIndex *pindexBestHeader = nullptr;
 107 |  Mutex g_best_block_mutex;
 108 |  std::condition_variable g_best_block_cv;
 109 |  uint256 g_best_block;
 110 | -int nScriptCheckThreads = 0;
 111 | +bool g_parallel_script_checks {false};
    promag commented at 9:01 AM on November 6, 2019:

    1856073d6e8c774cc0a606275f264133884ef7bd

    nit, is this a new format? The space makes sense when the initialization comes after an annotation but in this case we don't add the space.

    jnewbery commented at 8:04 PM on November 6, 2019:

    fixed

  17. in src/test/setup_common.cpp:107 in 1856073d6e outdated 
     101 | @@ -102,9 +102,12 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
 102 |          throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", FormatStateMessage(state)));
 103 |      }
 104 |  
 105 | -    nScriptCheckThreads = 3;
 106 | -    for (int i = 0; i < nScriptCheckThreads - 1; i++)
 107 | +    // Start script-checking threads. Set g_parallel_script_checks to true so they are used.
 108 | +    constexpr int script_check_threads = 2;
 109 | +    for (int i = 0; i < script_check_threads; i++) {
    promag commented at 9:02 AM on November 6, 2019:

    nit, since you are touching this, ++i?

    laanwj commented at 9:35 AM on November 6, 2019:

    come on, this doesn't matter for integers

    jnewbery commented at 8:05 PM on November 6, 2019:

    fixed

  18. in src/test/checkqueue_tests.cpp:152 in 6e6cb418ac outdated 
     147 | @@ -149,7 +148,7 @@ static void Correct_Queue_range(std::vector<size_t> range)
 148 |  {
 149 |      auto small_queue = MakeUnique<Correct_Queue>(QUEUE_BATCH_SIZE);
 150 |      boost::thread_group tg;
 151 | -    for (auto x = 0; x < nScriptCheckThreads; ++x) {
    promag commented at 9:25 AM on November 6, 2019:

    6e6cb418accf9286148cff9f1966fca27d38e0d7

    Locally it builds if #include <validation.h> is removed.

    jnewbery commented at 8:05 PM on November 6, 2019:

    fixed

  19. promag commented at 9:30 AM on November 6, 2019: member

    Tested ACK 1856073d6e8c774cc0a606275f264133884ef7bd.

    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.

  20. laanwj commented at 9:37 AM on November 6, 2019: member

    code review ACK 1856073d6e8c774cc0a606275f264133884ef7bd

  21. [tests] Don't use TestingSetup in the checkqueue_tests
    It's only needed for a hardcoded int, which we can define locally.
    d9957623b4
  22. [refactor] Replace global int nScriptCheckThreads with bool
    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.
    5506ecfe7a
  23. jnewbery force-pushed on Nov 6, 2019
  24. jnewbery commented at 8:05 PM on November 6, 2019: member

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

    Thanks for the review!

  25. sipa commented at 8:54 PM on November 6, 2019: member

    ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb

  26. fanquake renamed this:
    Refactor: Clean up nScriptCheckThreads
    refactor: Clean up nScriptCheckThreads
    on Nov 6, 2019
  27. promag commented at 10:59 PM on November 6, 2019: member

    ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb, only change was addressing my nits.

  28. laanwj commented at 10:53 AM on November 7, 2019: member

    Code review ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb

  29. DrahtBot commented at 2:27 PM on November 7, 2019: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  30. DrahtBot added the label Needs rebase on Nov 7, 2019
  31. MarcoFalke commented at 3:06 PM on November 7, 2019: member

    ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb 🥐

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 5506ecfe7a65d5705616bc048f2f1735b89993fb 🥐
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiO9QwAz6nYOBxbVWtpsV2BAXLethFVuhGT9eD0joGWjF3ZFJpwXxeEckKYM3Yd
8HyfPK8ZQ0vq2I7rQUwUSqPoacnSvEIhFKx34CAMtc+TLnI0g4HB4vXwDw1Ifjcw
3ljujzu3lW255jARdKo+1VTKSL7CpDKlKn1vsJFviiNi0DLRJ2UFjSrYxvYVrGgb
8s2hIIwjFF41iT0FoSfati5OE1A6wYPJlUAPX6I5MDAwaFiDfMPeTlDOEaXHrG7V
EsfICRMKLF4ahH+HN9x1Ce0jIeyB1jGsU9EAQZT1NEDj3kwsuvmVS4RBfhpZauKE
KLkbLRm32L5fKi5N/xmqyODxW4XKDzAkvDnncpOuS7xHMqbBF4OfI9wUY2jjarhb
vWCBgGNe+nzc4FMU58+Nd4Q/yjXWUrFRA2fUKoZT2MZmgjz1SfMdzWzJMqK9Q51c
ZL0l/SKJnMpy6OqjwgYExmbgP+wvhEqL9ywBxc97wqSToizF6Ex7lvNpz/9+3u/K
F9wuppNR
=bJqw
-----END PGP SIGNATURE-----

    Timestamp of file with hash 6804ad6c56d3cf372758297498bf984f409018ae306e96929df7d927368d1bd1 -

    </details>

  32. MarcoFalke referenced this in commit 7d14e35f3f on Nov 7, 2019
  33. MarcoFalke merged this on Nov 7, 2019
  34. MarcoFalke closed this on Nov 7, 2019
  35. in src/test/setup_common.cpp:106 in 5506ecfe7a 
     101 | @@ -102,9 +102,12 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
 102 |          throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", FormatStateMessage(state)));
 103 |      }
 104 |  
 105 | -    nScriptCheckThreads = 3;
 106 | -    for (int i = 0; i < nScriptCheckThreads - 1; i++)
 107 | +    // Start script-checking threads. Set g_parallel_script_checks to true so they are used.
 108 | +    constexpr int script_check_threads = 2;
    MarcoFalke commented at 3:11 PM on November 7, 2019:

    style-nit: compile time constants are generally ALL_UPPER_CASE

  36. sidhujag referenced this in commit dfdeef989f on Nov 9, 2019
  37. fanquake removed the label Needs rebase on Jul 26, 2020
  38. deadalnix referenced this in commit 02aca79cc3 on Sep 1, 2020
  39. jasonbcox referenced this in commit 6b134a6895 on Sep 1, 2020
  40. jasonbcox referenced this in commit ef8e11aa8f on Nov 2, 2020
  41. sidhujag referenced this in commit 7303d1614e on Nov 10, 2020
  42. kittywhiskers referenced this in commit 0a7ba5ce40 on Jun 4, 2021
  43. kittywhiskers referenced this in commit 078caf70fc on Jun 5, 2021
  44. kittywhiskers referenced this in commit 6ae21bf3d0 on Jun 6, 2021
  45. kittywhiskers referenced this in commit 17ab6b4a22 on Jun 6, 2021
  46. kittywhiskers referenced this in commit 8da0e1fe7b on Jun 7, 2021
  47. kittywhiskers referenced this in commit 0a08b5f47b on Jun 8, 2021
  48. kittywhiskers referenced this in commit 770755ee6e on Jun 8, 2021
  49. kittywhiskers referenced this in commit d3cefbf1d6 on Jun 9, 2021
  50. kittywhiskers referenced this in commit d6f92c2061 on Jun 10, 2021
  51. kittywhiskers referenced this in commit 902951caa3 on Jun 11, 2021
  52. kittywhiskers referenced this in commit f669b3aa5b on Jun 16, 2021
  53. kittywhiskers referenced this in commit a6fa6edb8d on Jun 16, 2021
  54. kittywhiskers referenced this in commit 9e1c8a3f59 on Jun 24, 2021
  55. kittywhiskers referenced this in commit 03a3f6c2e6 on Jun 25, 2021
  56. UdjinM6 referenced this in commit c5cc285d0e on Jun 26, 2021
  57. DrahtBot locked this on Feb 15, 2022
Contributors
jnewberypracticalswiftlaanwjpromagDrahtBotsipaMarcoFalke
Labels
Validation
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-30 12:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me