This seems overkill, as most tests are single-threaded. The multi-threaded test could just have one RNG per thread?

By that I mean having an explicit "FastRandomContext rng(true);" in each of the threads.

The alternative you suggest (only give each thread their own randomness context when they are multithreaded) would involve (re)writing the InsecureRandRange helpers. I'd prefer to just add the thread_local keyword unless there are observable performance regressions.

Oh, I forgot about the wrappers for the global test RNG. Objection withdrawn.

utACK

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14464 (refactor: make checkqueue manage the threads by itself (also removed some boost dependencies) by ken2812221)
• #14193 (validation: Add missing mempool locks by MarcoFalke)
• #13804 (WIP: Transaction Pool Layer by MarcoFalke)

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.

No idea if this is significant, but mentioning just in case:

Thread-local support used to be optional before this (was only used in sync.h in debug mode optionally). This change ignores HAVE_THREAD_LOCAL and makes it mandatory.

Should those tests perhaps fail explicitly, like in sync.cpp?

#if !defined(HAVE_THREAD_LOCAL)
static_assert(false, "thread_local is not supported");
#endif

Or should we make it mandatory in ./configure?

Some earlier related discussion:

• Thread names in logs and deadlock debug tools (take 2) #13168 (comment)
• Switched sync.{cpp,h} to std threading primitives. #11722

We bumped the requirement from g++ 4.7 to g++ 4.8 in #11755

@theuni wrote there:

thread_local was also off-limits due to missing support in osx's clang. It's been added as of XCode 8 on September 13, 2016. I'm uneasy with that as a minimum requirement.

From Apple docs: "Xcode 8.1 requires a Mac running macOS 10.11.5 or later". We still support macOS 10.10. We could consider bumping that to 10.11.5, or accept that some tests won't run.

Additionally, last time I checked this, the use of thread_local causes new symbols to be pulled in from glibc on Linux, meaning that we would lose back-compat with older versions. I'd have to re-test to see where the break lies.

So perhaps this depends on if we drop e.g. Ubuntu Trusty 14.04 in the next release.

thread_local was added in C++11, which we require right now and we are about to switch to C++14 soon, so a compiler not supporting it shouldn't be a problem.

@MarcoFalke That reasoning seems backwards. We identify what C++ version (and features) we can use based on the platforms we want to support. I don't mean to say that this particular change is a problem, but just because we're considering C++14 does not mean we should drop support for every platform that doesn't support every feature from C++11 or C++14.

Does this mean we have now removed support for OSX 10.10 for the unit tests with this change?

It would be easier to decide which C++11 (or C++14) features are acceptable to use when there was a guideline/process that defines compatibility requirements we want to achieve.

We identify what C++ version (and features) we can use based on the platforms we want to support.

It's probably a bit of both. It's easier to support older platforms if it only causes minor inconveniences.

This change indeed breaks macOS 10.10 support for the unit tests. Afaik it takes a patch with an ifdef to restore support for the majority of tests. @jonasschnelli might be able to confirm since he has VMs with older macOS versions.