refactor: Make scheduler methods type safe #18289

In that case, should stop() also assert(nThreadsServicingQueue > 0) at least when drain == true?

I agree. Maybe relax the assert to assert(nThreadsServicingQueue > 0 || taskQueue.empty()) because otherwise CScheduler c; c.stop(); would assert.

I don't think we can do that. The scheduler accepts events before any service thread has been started, also we can ask it to stop before any service threads have been started (they might be started later on).

I will leave it as is for now.

Makes sense

Nit: const std::chrono::milliseconds& to avoid copy (here and below).

Nit on nit:

I'm afraid that might be a pessimisation :)

From a performance perspective the typical rule-of-thumb recommendation is to pass "in" parameters by value (instead of reference to const) if the parameter has a size up to 16 bytes ("cheaply-copied" types).

Thus in this case the rule-of-thumb recommendation would be to pass by value instead of reference to const.

(Disclaimer: As always with optimisations -- if it is worth trying to optimise then it is worth trying to quantify the impact of the optimisation by measuring! :))

From the C++ Core Guidelines (Stroustrup & Sutter):

F.16: For “in” parameters, pass cheaply-copied types by value and others by reference to const

Reason: Both let the caller know that a function will not modify the argument, and both allow initialization by rvalues.

What is “cheap to copy” depends on the machine architecture, but two or three words (doubles, pointers, references) are usually best passed by value. When copying is cheap, nothing beats the simplicity and safety of copying, and for small objects (up to two or three words) it is also faster than passing by reference because it does not require an extra indirection to access from the function.

Examples:

void f1(const string& s);  // OK: pass by reference to const; always cheap

void f2(string s);         // bad: potentially expensive

void f3(int x);            // OK: Unbeatable

void f4(const int& x);     // bad: overhead on access in f4()

It feels unstable to me to depend on architecture and/or implementation rather than just go for const x& when you can, but it's a nit; I'll live. :)

std::chrono::milliseconds is just a 64-bit int (well, 45-bit or greater int), so passing it as const & means you're passing a 64-bit pointer instead, and have to deref it later.

Leaving as is for now, to not invalidate review

The other feedback aside, I prefer the copy here over a reference because other arguments (like the function) must be copied anyway.

Yep, copy seems sensible after all.

Nit: the comment warrants s/ (900s)//, or even reword to something like "How often to dump addresses to banlist.dat". Same below for DUMP_PEERS_INTERVAL.

Thanks, done.

    scheduler.scheduleEvery([this] { DumpAddresses(); }, DUMP_PEERS_INTERVAL);

Same below.

Same below.

Where exactly?

src/net_processing.cpp:1127?

consensusParams needs to be passed in, not just this

Would this be better as a Doxygen comment?

Yes, but leaving as is for now

suggestion:

    /**
     * Repeat f indefinitely until the scheduler is stopped. The first run
     * occurs after delta time.
     *
     * The timing is not exact; every time f finishes, it is scheduled again
     * to run delta time later. If you need more accurate scheduling, don't
     * use this method.
     */

might take, if I have to force push for other reasons

The referenced global consensusParams can change in tests, so it needs to be copied. Fixed in #18376