bugfix: Make `CCheckQueue` RAII-styled (attempt 2) by hebasto · Pull Request #26762 · bitcoin/bitcoin

hebasto · 2022-12-28T13:17:37Z

This PR:

makes CCheckQueue RAII-styled
gets rid of the global scriptcheckqueue
fixes ~CCheckQueue assertion failure during unclean exits #25448

The previous attempt was in #18731.

DrahtBot · 2022-12-28T13:17:39Z

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

Code Coverage

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

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.

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.

w0xlt

Approach ACK

ajtowns

Concept ACK

ajtowns · 2023-01-03T05:29:15Z

src/checkqueue.h

-    void StopWorkerThreads() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    ~CCheckQueue()
    {
        WITH_LOCK(m_mutex, m_request_stop = true);


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.

Thanks. Updated.

ajtowns · 2023-01-03T05:44:03Z

src/init.cpp

    // CScheduler/checkqueue, scheduler and load block thread.
    if (node.scheduler) node.scheduler->stop();
    if (node.chainman && node.chainman->m_load_block.joinable()) node.chainman->m_load_block.join();
-    StopScriptCheckWorkerThreads();


cf #23234 (comment)

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

ajtowns · 2023-01-03T05:54:02Z

src/checkqueue.h


    //! Create a new check queue
-    explicit CCheckQueue(unsigned int nBatchSizeIn)
-        : nBatchSize(nBatchSizeIn)


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

refactor/behaviour change: move the global scriptcheckqueue into a class (and move its params out of init.cpp)

possible behaviour change: move all the StopWorkerThreads() calls to wherever the queue goes out of scope (~ChainStateManager?)

refactor: add a constructor that accepts worker_threads_num and just calls StartWorkerThreads()

refactor: replace all the calls to StartWorkerThreads with the new constructor

refactor: call StopWorkerThreads() from the destructor only

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

ajtowns · 2023-01-03T05:58:54Z

src/validation.cpp

    ChainstateManager& chainman,
    std::optional<uint256> from_snapshot_blockhash)
    : m_mempool(mempool),
+      m_script_check_queue{std::make_unique<CCheckQueue<CScriptCheck>>(/*batch_size=*/128, chainman.m_options.worker_threads_num)},


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?

Thanks! Updated.

ajtowns · 2023-01-03T06:03:24Z

src/checkqueue.h

    }

+    CCheckQueue(const CCheckQueue&) = delete;
+    CCheckQueue& operator=(const CCheckQueue&) = delete;


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?)

Thanks! Done.

hebasto · 2023-01-03T15:15:46Z

Updated 2d248d6 -> 5a19c39 (pr26762.01 -> pr26762.02, diff):

addressed @ajtowns's comments

hebasto · 2023-01-16T15:42:38Z

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

ajtowns · 2023-01-27T04:54:05Z

utACK f020924

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

src/node/chainstatemanager_args.cpp

ajtowns · 2023-01-27T04:07:13Z

src/validation.cpp

+    : m_script_check_queue{/*nBatchSizeIn=*/128},
+      m_options{Flatten(std::move(options))}
+{
+    m_script_check_queue.StartWorkerThreads(m_options.worker_threads_num);


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.

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

hebasto · 2023-01-30T12:06:19Z

Updated f020924 -> 5a7932f (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.

ajtowns · 2023-01-31T03:30:22Z

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

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

reACK 5a7932f

sedited

Concept ACK

sedited · 2023-03-21T08:23:32Z

src/node/chainstatemanager_args.cpp


    if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};

+    int par_value = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS);


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.

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.

src/node/chainstatemanager_args.cpp

hebasto · 2023-03-21T13:19:29Z

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

hebasto · 2023-10-03T10:03:01Z

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

cacrowley

#26762

sedited

ACK 5b3ea5f

sedited · 2023-11-29T16:33:54Z

src/qt/optionsdialog.cpp


 #include <common/system.h>
 #include <interfaces/node.h>
+#include <node/chainstatemanager_args.h>


Nit: fix include order.

martinus · 2023-11-29T18:01:34Z

ACK 5b3ea5f

achow101 · 2023-11-30T19:17:32Z

ACK 5b3ea5f

maflcko · 2023-12-01T11:34:13Z

src/test/checkqueue_tests.cpp

 BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks)
 {
-    auto queue = std::make_unique<Standard_Queue>(QUEUE_BATCH_SIZE);
+    auto queue = std::make_unique<Standard_Queue>(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS);


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

maflcko · 2023-12-01T11:36:15Z

src/qt/optionsmodel.cpp

+#include <node/chainstatemanager_args.h>
 #include <txdb.h> // for -dbcache defaults
 #include <util/string.h>
 #include <validation.h>    // For DEFAULT_SCRIPTCHECK_THREADS


5b3ea5f: Comment is now wrong, no?

maflcko

lgtm

DrahtBot added the Refactoring label Dec 28, 2022

This was referenced Dec 28, 2022

refactor: Make CCheckQueue RAII-styled #18731

Closed

~CCheckQueue assertion failure during unclean exits #25448

Closed

This was referenced Dec 29, 2022

refactor / kernel: Move non-gArgs chainparams functionality to kernel #26177

Merged

refactor, kernel: Remove gArgs accesses from dbwrapper and txdb #25862

Merged

assumeutxo: background validation completion #25740

Merged

w0xlt reviewed Dec 30, 2022

View reviewed changes

DrahtBot mentioned this pull request Dec 31, 2022

Drop CCheckQueue::nIdle #26776

Closed

ajtowns reviewed Jan 3, 2023

View reviewed changes

hebasto force-pushed the 221228-queue branch from 2d248d6 to 5a19c39 Compare January 3, 2023 15:12

This was referenced Jan 3, 2023

Remove almost all blockstorage globals #25781

Merged

assumeutxo: net_processing changes #24008

Closed

refactor: add kernel/cs_main.h #26251

Merged

DrahtBot added the Needs rebase label Jan 16, 2023

hebasto force-pushed the 221228-queue branch from 5a19c39 to f020924 Compare January 16, 2023 15:41

DrahtBot removed the Needs rebase label Jan 16, 2023

DrahtBot mentioned this pull request Jan 17, 2023

refactor: Use move semantics instead of custom swap functions #26749

Merged

ajtowns reviewed Jan 27, 2023

View reviewed changes

hebasto force-pushed the 221228-queue branch from f020924 to 5a7932f Compare January 30, 2023 12:01

DrahtBot mentioned this pull request Feb 4, 2023

OP_VAULT draft #26857

Closed

DrahtBot added the Needs rebase label Feb 17, 2023

sedited reviewed Mar 21, 2023

View reviewed changes

maflcko mentioned this pull request Mar 21, 2023

ci: Integrate bitcoin-tidy clang-tidy plugin #26296

Merged

hebasto added 5 commits October 3, 2023 10:52

Make CCheckQueue destructor stop worker threads

d03eaac

refactor: Make CCheckQueue constructor start worker threads

9cf89f7

refactor: Drop unneeded declaration

8111e74

refactor: Make CCheckQueue non-copyable and non-movable explicitly

6e17b31

refactor: Move {MAX,DEFAULT}_SCRIPTCHECK_THREADS constants

5b3ea5f

hebasto force-pushed the 221228-queue branch from 2235765 to 5b3ea5f Compare October 3, 2023 10:01

DrahtBot removed the Needs rebase label Oct 3, 2023

DrahtBot mentioned this pull request Oct 7, 2023

assumeutxo state and locking cleanup #28608

Closed

cacrowley reviewed Nov 7, 2023

View reviewed changes

DrahtBot mentioned this pull request Nov 28, 2023

kernel: Remove dependency on CScheduler #28960

Merged

sedited approved these changes Nov 29, 2023

View reviewed changes

DrahtBot requested review from martinus and w0xlt and removed request for martinus and w0xlt November 29, 2023 16:35

DrahtBot requested review from w0xlt and removed request for martinus and w0xlt November 29, 2023 18:01

DrahtBot requested review from w0xlt and removed request for w0xlt November 30, 2023 19:17

achow101 merged commit 498994b into bitcoin:master Nov 30, 2023

maflcko reviewed Dec 1, 2023

View reviewed changes

bitcoin locked and limited conversation to collaborators Dec 3, 2024


		if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};

		int par_value = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS);

Conversation

hebasto commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 3, 2023

Uh oh!

hebasto commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Jan 27, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 30, 2023

Uh oh!

ajtowns commented Jan 31, 2023

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Mar 21, 2023

Uh oh!

hebasto commented Oct 3, 2023

Uh oh!

cacrowley left a comment

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinus commented Nov 29, 2023

Uh oh!

achow101 commented Nov 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 28, 2022 •

edited

Loading

DrahtBot commented Dec 28, 2022 •

edited

Loading

ajtowns Jan 3, 2023 •

edited

Loading

hebasto commented Jan 16, 2023 •

edited

Loading