bugfix: Make `CCheckQueue` RAII-styled (attempt 2) #26762

pull hebasto wants to merge 6 commits into bitcoin:master from hebasto:221228-queue changing 15 files +62 −110
  1. hebasto commented at 1:17 PM on December 28, 2022: member

    This PR:

    • makes CCheckQueue RAII-styled
    • gets rid of the global scriptcheckqueue
    • fixes #25448

    The previous attempt was in #18731.

  2. DrahtBot commented at 1:17 PM on December 28, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan, martinus, achow101
    Approach ACK w0xlt
    Stale ACK ajtowns

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28960 (kernel: Remove dependency on CScheduler by TheCharlatan)

    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.

  3. DrahtBot added the label Refactoring on Dec 28, 2022
  4. w0xlt commented at 1:37 AM on December 30, 2022: contributor

    Approach ACK

  5. in src/checkqueue.h:177 in 47dad39e87 outdated 
     174 | @@ -187,24 +175,16 @@ class CCheckQueue
 175 |          }
 176 |      }
 177 |  
 178 | -    //! Stop all of the worker threads.
 179 | -    void StopWorkerThreads() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
 180 | +    ~CCheckQueue()
 181 |      {
 182 |          WITH_LOCK(m_mutex, m_request_stop = true);
    ajtowns commented at 5:29 AM on January 3, 2023:

    clang turns off thread safety analysis in constructors and destructors, so I think if locking is needed it would be better to keep a separate (private?) function, that the destructor calls when necessary to maximise compile time checking.

    hebasto commented at 3:17 PM on January 3, 2023:

    Thanks. Updated.

  6. in src/init.cpp:274 in 47dad39e87 outdated 
     243 | @@ -244,7 +244,6 @@ void Shutdown(NodeContext& node)
 244 |      // CScheduler/checkqueue, scheduler and load block thread.
 245 |      if (node.scheduler) node.scheduler->stop();
 246 |      if (node.chainman && node.chainman->m_load_block.joinable()) node.chainman->m_load_block.join();
 247 | -    StopScriptCheckWorkerThreads();
    ajtowns commented at 5:44 AM on January 3, 2023:

    cf #23234 (comment)

    I don't think this change makes anything worse, but Shutdown() is pretty fragile.

  7. in src/checkqueue.h:134 in 47dad39e87 outdated 
     133 | @@ -134,22 +134,10 @@ class CCheckQueue
 134 |      Mutex m_control_mutex;
 135 |  
 136 |      //! Create a new check queue
 137 | -    explicit CCheckQueue(unsigned int nBatchSizeIn)
 138 | -        : nBatchSize(nBatchSizeIn)
    ajtowns commented at 5:54 AM on January 3, 2023:

    FWIW, I would probably have broken this up into multiple commits:

    1. refactor/behaviour change: move the global scriptcheckqueue into a class (and move its params out of init.cpp)
    2. possible behaviour change: move all the StopWorkerThreads() calls to wherever the queue goes out of scope (~ChainStateManager?)
    3. refactor: add a constructor that accepts worker_threads_num and just calls StartWorkerThreads()
    4. refactor: replace all the calls to StartWorkerThreads with the new constructor
    5. refactor: call StopWorkerThreads() from the destructor only
    hebasto commented at 3:18 PM on January 3, 2023:

    Thanks! Broken into a few commits in way to assure each commit passes tests.

  8. in src/validation.cpp:1495 in 47dad39e87 outdated 
    1491 | @@ -1492,6 +1492,7 @@ Chainstate::Chainstate(
1492 |      ChainstateManager& chainman,
1493 |      std::optional<uint256> from_snapshot_blockhash)
1494 |      : m_mempool(mempool),
1495 | +      m_script_check_queue{std::make_unique<CCheckQueue<CScriptCheck>>(/*batch_size=*/128, chainman.m_options.worker_threads_num)},
    ajtowns commented at 5:58 AM on January 3, 2023:

    This seems to be doubling the number of script check workers if the assumeutxo snapshot chainstate is allocated; shouldn't m_script_check_queue be part of ChainstateManager instead, so it remains a single global queue?

    hebasto commented at 3:18 PM on January 3, 2023:

    Thanks! Updated.

  9. in src/checkqueue.h:145 in 2d248d6b80 outdated 
     142 | @@ -143,6 +143,9 @@ class CCheckQueue
 143 |          }
 144 |      }
 145 |  
 146 | +    CCheckQueue(const CCheckQueue&) = delete;
 147 | +    CCheckQueue& operator=(const CCheckQueue&) = delete;
    ajtowns commented at 6:03 AM on January 3, 2023:

    Should apply rule-of-5 and default the move operations, no? (EDIT: also, move the explanation of why these are deleted from the commit message into the file itself?)

    hebasto commented at 3:19 PM on January 3, 2023:

    Thanks! Done.

  10. ajtowns commented at 6:04 AM on January 3, 2023: contributor

    Concept ACK

  11. hebasto force-pushed on Jan 3, 2023
  12. hebasto commented at 3:15 PM on January 3, 2023: member

    Updated 2d248d6b80147e5f9f62f4c543d4dbb10c0d95c7 -> 5a19c3962c8702ba3b17f986524e1eb5de049c0e (pr26762.01 -> pr26762.02, diff):

  13. DrahtBot added the label Needs rebase on Jan 16, 2023
  14. hebasto force-pushed on Jan 16, 2023
  15. hebasto commented at 3:42 PM on January 16, 2023: member

    Rebased 5a19c3962c8702ba3b17f986524e1eb5de049c0e -> f0209248e1baea41aae72147c973f1f5aa27c97b (pr26762.02 -> pr26762.03) due to the conflict with #26251.

  16. DrahtBot removed the label Needs rebase on Jan 16, 2023
  17. in src/node/chainstatemanager_args.cpp:46 in 3567969a47 outdated 
      40 | +    if (par_value <= 0) {
  41 | +        // -par=0 means autodetect (number of cores - 1 script threads)
  42 | +        // -par=-n means "leave n cores free" (number of cores - n - 1 script threads)
  43 | +        par_value += GetNumCores();
  44 | +    }
  45 | +    opts.worker_threads_num = std::clamp(par_value - 1, 0, MAX_SCRIPTCHECK_THREADS);
    ajtowns commented at 4:03 AM on January 27, 2023:

    Missing the comment explaining the - 1

    hebasto commented at 12:06 PM on January 30, 2023:

    Thanks! Updated.

  18. in src/validation.cpp:5307 in 3567969a47 outdated 
    5299 | @@ -5307,10 +5300,17 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts)
5300 |      return std::move(opts);
5301 |  }
5302 |  
5303 | -ChainstateManager::ChainstateManager(Options options) : m_options{Flatten(std::move(options))} {}
5304 | +ChainstateManager::ChainstateManager(Options options)
5305 | +    : m_script_check_queue{/*nBatchSizeIn=*/128},
5306 | +      m_options{Flatten(std::move(options))}
5307 | +{
5308 | +    m_script_check_queue.StartWorkerThreads(m_options.worker_threads_num);
    ajtowns commented at 4:07 AM on January 27, 2023:

    Previously this was conditional on worker_threads_num > 0 (since StartScriptCheckWorkerThreads would not be called unless script_threads >= 1). I think this is okay, since in that case the function only sets some variables that will never be used and skips over a loop.

    hebasto commented at 12:07 PM on January 30, 2023:

    This code will gone in the "refactor: Make CCheckQueue constructor start worker threads" commit.

  19. ajtowns commented at 4:54 AM on January 27, 2023: contributor

    utACK f0209248e1baea41aae72147c973f1f5aa27c97b

    Seems reasonable to me. Not 100% confident over delaying stopping the threads until the destructor; but also about equally unsure about how it works now...

  20. hebasto force-pushed on Jan 30, 2023
  21. hebasto commented at 12:06 PM on January 30, 2023: member

    Updated f0209248e1baea41aae72147c973f1f5aa27c97b -> 5a7932f395c675fad332cbcd0498bb9fefcb33e0 (pr26762.03 -> pr26762.04, diff):

    • addressed @ajtowns's comment
    • add a default initializer for kernel::ChainstateManagerOpts::worker_threads_num

    Not 100% confident over delaying stopping the threads until the destructor; but also about equally unsure about how it works now...

    Actually, it is how the #25448 has been fixed now.

  22. ajtowns commented at 3:30 AM on January 31, 2023: contributor

    add a default initializer for kernel::ChainstateManagerOpts::worker_threads_num

    Yikes. Work on adding cppcoreguidelines-pro-type-member-init` to clang-tidy maybe?

    reACK 5a7932f395c675fad332cbcd0498bb9fefcb33e0

  23. DrahtBot added the label Needs rebase on Feb 17, 2023
  24. in src/node/chainstatemanager_args.cpp:39 in 5a7932f395 outdated 
      35 | @@ -34,6 +36,16 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, Chains
  36 |  
  37 |      if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};
  38 |  
  39 | +    int par_value = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS);
    TheCharlatan commented at 8:23 AM on March 21, 2023:

    This is not consistent with how the other options are handled. This overrides the value of the passed in option even if no argument was passed in. I would handle this the same way as max_tip_age and also move the MAX_SCRIPTCHECK_THREADS to chainstatemanager_opts.h.

    hebasto commented at 2:27 PM on March 22, 2023:

    This is not consistent with how the other options are handled. This overrides the value of the passed in option even if no argument was passed in. I would handle this the same way as max_tip_age...

    I'm not sure about this change considering this PR scope. The semantic of the "-par" and "-maxtipage" options are quite different.

    ... and also move the MAX_SCRIPTCHECK_THREADS to chainstatemanager_opts.h.

    Thanks! Updated.

  25. in src/node/chainstatemanager_args.cpp:40 in 5a7932f395 outdated 
      35 | @@ -34,6 +36,16 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, Chains
  36 |  
  37 |      if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};
  38 |  
  39 | +    int par_value = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS);
  40 | +    if (par_value <= 0) {
    TheCharlatan commented at 8:26 AM on March 21, 2023:

    Nit: This diff would be more move only if par_value were still called script_threads. I also find script_threads a better name than par_value.

    hebasto commented at 2:23 PM on March 22, 2023:

    Thanks! Updated.

  26. TheCharlatan commented at 9:00 AM on March 21, 2023: contributor

    Concept ACK

  27. hebasto commented at 1:19 PM on March 21, 2023: member

    Yikes. Work on adding cppcoreguidelines-pro-type-member-init` to clang-tidy maybe?

    Well, it fires too many false warnings for our code base (for example, for CTxOut::CTxOut() constructor).

  28. hebasto force-pushed on Mar 22, 2023
  29. hebasto commented at 2:22 PM on March 22, 2023: member

    Updated 5a7932f395c675fad332cbcd0498bb9fefcb33e0 -> f1370b2c1586f7fe487d9f17ee53bcd9b87a9f23 (pr26762.04 -> pr26762.05):

  30. DrahtBot removed the label Needs rebase on Mar 22, 2023
  31. in src/checkqueue.h:180 in f1370b2c15 outdated 
     173 | @@ -181,24 +174,16 @@ class CCheckQueue
 174 |          }
 175 |      }
 176 |  
 177 | -    //! Stop all of the worker threads.
 178 | -    void StopWorkerThreads() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
 179 | +    ~CCheckQueue()
 180 |      {
 181 | -        WITH_LOCK(m_mutex, m_request_stop = true);
 182 | +        m_request_stop = true;
    martinus commented at 3:52 PM on April 2, 2023:

    I think that removing the lock here could cause a deadlock. m_request_stop is set to true and then notify_all() is called; but when there is a worker thread currently standing inside the while (queue.empty() && !m_request_stop) { loop but before the cond.wait(lock); it will not receive the notification and then proceed to wait in the cond indefinitely.

    That deadlock was not possible before when the m_mutex had to be acquired to change m_request_stop because then it was guaranteed that all threads were either waiting on the condition variable or waiting outside that loop to get the lock

    hebasto commented at 8:39 AM on April 5, 2023:

    Doesn't the default memory_order_seq_cst memory order for std::atomic<bool> m_request_stop guarantee the correct behaviour?

    martinus commented at 10:26 AM on April 5, 2023:

    The only thing guaranteed is the order; so m_request_stop will be set to true before notify_all is called. But there is no guarantee where the other threads currently are because there is now no lock anymore. E.g. a worker thread could be currently here:

    while (queue.empty() && !m_request_stop) {
    if (fMaster && nTodo == 0) {   // <-------- Worker thread waiting here due to context switch
        nTotal--;
        bool fRet = fAllOk;
        // reset the status for new work later
        fAllOk = true;
        // return the current status
        return fRet;
    }
    nIdle++;
    cond.wait(lock); // wait
    nIdle--;
}

    Now while the worker thread is not running for whatever reason (context switch, high CPU load, ...) the main thread runs, calls m_request_stop = true, then m_worker_cv.notify_all();, and then hangs in the t.join() loop.

    Eventually the worker thread will get its share of the CPU and continues running, and will run right into the cond.wait(lock); without checking m_request_stop. There it won't receive the notification any more because by now it's too late; notifications have already been sent. So the worker thread will now wait.

    The result is that the master hangs in the join() loop, and the worker waits for notification => deadlock.

    hebasto commented at 7:13 PM on April 6, 2023:

    You're right:

    Even if the shared variable is atomic, it must be modified while owning the mutex to correctly publish the modification to the waiting thread.

    Going to amend the patch.

    hebasto commented at 9:28 PM on April 8, 2023:

    Thanks! Updated.

  32. in src/checkqueue.h:134 in f1370b2c15 outdated 
     145 | -            nTotal = 0;
 146 | -            fAllOk = true;
 147 | -        }
 148 | -        assert(m_worker_threads.empty());
 149 | -        for (int n = 0; n < threads_num; ++n) {
 150 | +        for (int n = 0; n < worker_threads_num; ++n) {
    martinus commented at 4:36 PM on April 2, 2023:

    nit: might want to add m_worker_threads.reserve(worker_threads_num); before the loop

    hebasto commented at 9:28 PM on April 8, 2023:

    Thanks! Updated.

  33. in src/checkqueue.h:148 in f1370b2c15 outdated 
     140 | @@ -155,6 +141,13 @@ class CCheckQueue
 141 |          }
 142 |      }
 143 |  
 144 | +    // Since this class manages its own resources, which is a thread
 145 | +    // pool `m_worker_threads`, copy operations are not appropriate.
 146 | +    CCheckQueue(const CCheckQueue&) = delete;
 147 | +    CCheckQueue& operator=(const CCheckQueue&) = delete;
 148 | +    CCheckQueue(CCheckQueue&&) = default;
    martinus commented at 5:23 PM on April 2, 2023:

    I'd explicitly set the move operations to = delete. I think std::mutex is not movable so these will be deleted anyways, but it feels safer to just mark them as deleted too. There will be threads running throughout the lifetime of CCheckQueue, so moving the object while something might be going on or threads waiting on a lock wouldn't be safe

    hebasto commented at 9:28 PM on April 8, 2023:

    Thanks! Updated.

  34. hebasto force-pushed on Apr 5, 2023
  35. hebasto force-pushed on Apr 8, 2023
  36. hebasto commented at 9:27 PM on April 8, 2023: member

    Updated f1370b2c1586f7fe487d9f17ee53bcd9b87a9f23 -> d462e3da7fc6be75269e88928fd80fc98c405474 (pr26762.05 -> pr26762.07, diff):

  37. in src/bitcoin-chainstate.cpp:291 in d462e3da7f outdated 
     249 | @@ -250,7 +250,6 @@ int main(int argc, char* argv[])
 250 |      // dereferencing and UB.
 251 |      scheduler.stop();
    martinus commented at 6:43 AM on April 10, 2023:

    nit, just a thought: maybe it would make sense to have a method like signalStop() or similar in CCheckQueue that only sets m_request_stop to true, and call this as soon as we know we don't need the queue's result any more. That way the threads will stop processing and stop wasting CPU while shutting down.

    On the other hand the queue will usually pretty quickly stop working unless it has lots of work items, so I guess in most cases this wouldn't make any difference.

  38. martinus approved
  39. martinus commented at 7:25 AM on April 10, 2023: contributor

    Code review ACK d462e3da7fc6be75269e88928fd80fc98c405474, as far as I can say now deadlocks are not possible any more when shutting down. I started an initial sync with -assumevalid=0 so script checks are done, stopping the process a few times with Ctrl+C, all good; shutdown is still quick. -par still works too. Also did a rebase with --exec "make -j18 check", so each commit individually builds & all tests work.

  40. DrahtBot requested review from ajtowns on Apr 10, 2023
  41. DrahtBot added the label Needs rebase on Apr 21, 2023
  42. hebasto force-pushed on Apr 21, 2023
  43. hebasto commented at 12:12 PM on April 21, 2023: member

    Rebased d462e3da7fc6be75269e88928fd80fc98c405474 -> 52129e335cbc68ac5d863f283f7d1a328ce79581 (pr26762.07 -> pr26762.08) due to the conflict with #27419.

  44. DrahtBot removed the label Needs rebase on Apr 21, 2023
  45. martinus commented at 4:21 PM on April 23, 2023: contributor

    re-ack 52129e335cbc68ac5d863f283f7d1a328ce79581

  46. seannybird approved
  47. fanquake requested review from ryanofsky on May 8, 2023
  48. DrahtBot added the label Needs rebase on May 26, 2023
  49. hebasto force-pushed on May 26, 2023
  50. hebasto commented at 1:53 PM on May 26, 2023: member

    Rebased 52129e335cbc68ac5d863f283f7d1a328ce79581 -> 2076d846cc917cbafe61937a99b7867067011341 (pr26762.08 -> pr26762.09) due to the conflict with #25977.

  51. DrahtBot removed the label Needs rebase on May 26, 2023
  52. DrahtBot added the label Needs rebase on May 30, 2023
  53. hebasto force-pushed on May 31, 2023
  54. hebasto commented at 3:13 PM on May 31, 2023: member

    Rebased 2076d846cc917cbafe61937a99b7867067011341 -> b023c26eb8eda4c7f80ad2e7ebe1fb046e87d2ee (pr26762.09 -> pr26762.10) due to the conflict with #27636.

  55. DrahtBot removed the label Needs rebase on May 31, 2023
  56. DrahtBot added the label Needs rebase on Jun 9, 2023
  57. hebasto force-pushed on Jun 12, 2023
  58. hebasto renamed this:
    refactor: Make `CCheckQueue` RAII-styled
    Make `CCheckQueue` RAII-styled
    on Jun 12, 2023
  59. hebasto commented at 9:53 AM on June 12, 2023: member

    Rebased b023c26eb8eda4c7f80ad2e7ebe1fb046e87d2ee -> e89273e80765219ff545ee5c26a257aef7d69f9c (pr26762.10 -> pr26762.11) due to the conflict with #27576.

  60. DrahtBot removed the label Needs rebase on Jun 12, 2023
  61. DrahtBot added the label Needs rebase on Jul 6, 2023
  62. hebasto renamed this:
    Make `CCheckQueue` RAII-styled
    bugfix: Make `CCheckQueue` RAII-styled
    on Jul 7, 2023
  63. hebasto force-pushed on Jul 7, 2023
  64. hebasto commented at 9:48 AM on July 7, 2023: member

    Rebased e89273e80765219ff545ee5c26a257aef7d69f9c -> b7ebc999ee373f0429939ac0bdb8e38fb37404aa (pr26762.11 -> pr26762.12) due to the conflict with #27861.

  65. DrahtBot removed the label Needs rebase on Jul 7, 2023
  66. DrahtBot added the label Needs rebase on Jul 10, 2023
  67. hebasto force-pushed on Jul 12, 2023
  68. hebasto commented at 10:28 AM on July 12, 2023: member

    Rebased b7ebc999ee373f0429939ac0bdb8e38fb37404aa -> 6e079015707188ac15405662ce396dd837da569a (pr26762.12 -> pr26762.13) due to the conflict with #27607.

  69. DrahtBot removed the label Needs rebase on Jul 12, 2023
  70. DrahtBot added the label Needs rebase on Jul 14, 2023
  71. hebasto renamed this:
    bugfix: Make `CCheckQueue` RAII-styled
    bugfix: Make `CCheckQueue` RAII-styled (attempt 2)
    on Jul 15, 2023
  72. hebasto force-pushed on Jul 15, 2023
  73. hebasto commented at 7:06 AM on July 15, 2023: member

    Rebased 6e079015707188ac15405662ce396dd837da569a -> 2235765f7fd946d3b08fd9e109584967407e46d1 (pr26762.13 -> pr26762.14) due to the conflict with #28048.

  74. DrahtBot removed the label Needs rebase on Jul 15, 2023
  75. DrahtBot added the label CI failed on Jul 22, 2023
  76. DrahtBot removed the label CI failed on Jul 27, 2023
  77. DrahtBot added the label CI failed on Sep 17, 2023
  78. achow101 requested review from stickies-v on Sep 20, 2023
  79. achow101 requested review from martinus on Sep 20, 2023
  80. DrahtBot removed the label CI failed on Sep 21, 2023
  81. martinus commented at 9:33 AM on September 30, 2023: contributor

    code review ACK 2235765f7fd946d3b08fd9e109584967407e46d1, also rebased & built & tested each individual commit with make check

  82. DrahtBot removed review request from martinus on Sep 30, 2023
  83. martinus approved
  84. DrahtBot added the label Needs rebase on Oct 2, 2023
  85. Move global `scriptcheckqueue` into `ChainstateManager` class be4ff3060b
  86. Make `CCheckQueue` destructor stop worker threads d03eaacbcf
  87. refactor: Make `CCheckQueue` constructor start worker threads 9cf89f7a5b
  88. refactor: Drop unneeded declaration 8111e74653
  89. refactor: Make `CCheckQueue` non-copyable and non-movable explicitly 6e17b31680
  90. refactor: Move `{MAX,DEFAULT}_SCRIPTCHECK_THREADS` constants 5b3ea5fa2e
  91. hebasto force-pushed on Oct 3, 2023
  92. hebasto commented at 10:03 AM on October 3, 2023: member

    Rebased 2235765f7fd946d3b08fd9e109584967407e46d1 -> 5b3ea5fa2e7f6dc1c9161ed8b74c9be4bd1e92dd (pr26762.14 -> pr26762.15) due to the conflict with #27596.

  93. DrahtBot removed the label Needs rebase on Oct 3, 2023
  94. cacrowley commented at 5:00 AM on November 7, 2023: none

    #26762

  95. in src/qt/optionsdialog.cpp:20 in 5b3ea5fa2e 
      16 | @@ -17,9 +17,9 @@
  17 |  
  18 |  #include <common/system.h>
  19 |  #include <interfaces/node.h>
  20 | +#include <node/chainstatemanager_args.h>
    TheCharlatan commented at 4:33 PM on November 29, 2023:

    Nit: fix include order.

  96. TheCharlatan approved
  97. TheCharlatan commented at 4:35 PM on November 29, 2023: contributor

    ACK 5b3ea5fa2e7f6dc1c9161ed8b74c9be4bd1e92dd

  98. DrahtBot requested review from martinus on Nov 29, 2023
  99. DrahtBot requested review from w0xlt on Nov 29, 2023
  100. DrahtBot removed review request from martinus on Nov 29, 2023
  101. DrahtBot removed review request from w0xlt on Nov 29, 2023
  102. DrahtBot requested review from martinus on Nov 29, 2023
  103. DrahtBot requested review from w0xlt on Nov 29, 2023
  104. martinus commented at 6:01 PM on November 29, 2023: contributor

    ACK 5b3ea5fa2e7

  105. DrahtBot removed review request from martinus on Nov 29, 2023
  106. DrahtBot removed review request from w0xlt on Nov 29, 2023
  107. DrahtBot requested review from w0xlt on Nov 29, 2023
  108. achow101 commented at 7:17 PM on November 30, 2023: member

    ACK 5b3ea5fa2e7f6dc1c9161ed8b74c9be4bd1e92dd

  109. DrahtBot removed review request from w0xlt on Nov 30, 2023
  110. DrahtBot requested review from w0xlt on Nov 30, 2023
  111. achow101 merged this on Nov 30, 2023
  112. achow101 closed this on Nov 30, 2023
  113. in src/test/checkqueue_tests.cpp:356 in 9cf89f7a5b outdated 
     352 | @@ -362,7 +353,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup)
 353 |  /** Test that CCheckQueueControl is threadsafe */
 354 |  BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks)
 355 |  {
 356 | -    auto queue = std::make_unique<Standard_Queue>(QUEUE_BATCH_SIZE);
 357 | +    auto queue = std::make_unique<Standard_Queue>(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS);
    maflcko commented at 11:34 AM on December 1, 2023:

    This is a change in behavior for the test in 9cf89f7a5b81197e38f58b24be0793b28fe41477?

  114. in src/qt/optionsmodel.cpp:23 in 5b3ea5fa2e 
      16 | @@ -17,6 +17,7 @@
  17 |  #include <mapport.h>
  18 |  #include <net.h>
  19 |  #include <netbase.h>
  20 | +#include <node/chainstatemanager_args.h>
  21 |  #include <txdb.h> // for -dbcache defaults
  22 |  #include <util/string.h>
  23 |  #include <validation.h>    // For DEFAULT_SCRIPTCHECK_THREADS
    maflcko commented at 11:36 AM on December 1, 2023:

    5b3ea5fa2e7f6dc1c9161ed8b74c9be4bd1e92dd: Comment is now wrong, no?

  115. maflcko commented at 11:36 AM on December 1, 2023: member

    lgtm

  116. bitcoin locked this on Dec 3, 2024
Contributors
hebastoDrahtBotw0xltajtownsTheCharlatanmartinusseannybirdcacrowleyachow101maflcko


ajtownsryanofskystickies-vw0xlt

Labels
Refactoring
Linked (view graph)
#18731 refactor: Make CCheckQueue RAII-styled#25448 ~CCheckQueue assertion failure during unclean exits#27315 tidy: enable `cppcoreguidelines-pro-type-member-init`
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-05-02 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me