Some simple improvements to the RNG code #14624

This improves a few minor issues with the RNG code:

• Avoid calling GetRand*() functions (which currently invoke OpenSSL, later may switch to using our own RNG pool) inside loops in addrman, networking code, KnapsackSolver, and LimitOrphanSize
• Fix a currently unreachable bug in FastRandomContext::randbytes.
• Make a number of simplifications to the unit tests' randomness code (some tests unnecessarily used their own RNG or the OpenSSL one, instead of using the unit test specific insecure_rand_ctx).
• As a precaution, make it illegal to copy a FastRandomContext.

<!--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:

• #14626 (Select orphan transaction uniformly for eviction by sipa)
• #14605 (Return of the Banman by dongcarl)
• #13462 (Simplify common case of CHashWriter and drop SerializeHash by Empact)

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.

Concept ACK @sipa Regarding randbytes – very nice find! How was that issue found?

This also works around a bug in libstdc++ std::shuffle that may cause type::operator=(type&&) to be invoked on itself, which the library's debug mode detects and panics on.

How did you trigger this? I've built with -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC in the past and haven't encountered this. I also tried now and I was unable to reproduce.

Assuming a build with -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC – would the issue be triggered by make check or running the test suite?

I'm running:

$ g++ --version
g++ (Ubuntu 7.3.0-16ubuntu3) 7.3.0
$ dpkg -l | grep libstd
ii  libstdc++-7-dev:amd64                                       7.3.0-16ubuntu3                   amd64        GNU Standard C++ Library v3 (development files)
ii  libstdc++6:amd64                                            8-20180414-1ubuntu2               amd64        GNU Standard C++ Library v3

Travis failure:

/bin/bash: line 1: 27706 Aborted                 (core dumped) test/test_bitcoin -l test_suite -t "`cat wallet/test/coinselector_tests.cpp | grep -E "(BOOST_FIXTURE_TEST_SUITE\\(|BOOST_AUTO_TEST_SUITE\\()" | cut -d '(' -f 2 | cut -d ',' -f 1 | cut -d ')' -f 1`" > wallet/test/coinselector_tests.cpp.log 2>&1
Running 4 test cases...
Test cases order is shuffled using seed: 1449235911
Entering test module "Bitcoin Test Suite"
wallet/test/coinselector_tests.cpp(17): Entering test suite "coinselector_tests"
wallet/test/coinselector_tests.cpp(569): Entering test case "SelectCoins_test"
wallet/test/coinselector_tests.cpp(569): Leaving test case "SelectCoins_test"; testing time: 3726243us
wallet/test/coinselector_tests.cpp(267): Entering test case "knapsack_solver_test"
/usr/include/c++/7/debug/safe_iterator.h:374:
Error: attempt to advance a past-the-end iterator 1 steps, which falls 
outside its valid range.
Objects involved in the operation:
    iterator @ 0x0x7fff06875e20 {
      type = __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<OutputGroup*, std::__cxx1998::vector<OutputGroup, std::allocator<OutputGroup> > >, std::__debug::vector<OutputGroup, std::allocator<OutputGroup> > > (mutable iterator);
      state = past-the-end;
      references sequence with type 'std::__debug::vector<OutputGroup, std::allocator<OutputGroup> >' @ 0x0x7fff068766e0
    }
unknown location(0): fatal error: in "coinselector_tests/knapsack_solver_test": signal: SIGABRT (application abort requested)
wallet/test/coinselector_tests.cpp(281): last checkpoint
wallet/test/coinselector_tests.cpp(267): Leaving test case "knapsack_solver_test"; testing time: 78734us
wallet/test/coinselector_tests.cpp(122): Entering test case "bnb_search_test"
test_bitcoin: key.cpp:344: void ECC_Start(): Assertion `secp256k1_context_sign == nullptr' failed.
unknown location(0): fatal error: in "coinselector_tests/bnb_search_test": signal: SIGABRT (application abort requested)
wallet/test/coinselector_tests.cpp(122): last checkpoint: "bnb_search_test" fixture entry.
wallet/test/coinselector_tests.cpp(122): Leaving test case "bnb_search_test"; testing time: 271us
wallet/test/coinselector_tests.cpp(546): Entering test case "ApproximateBestSubset"
test_bitcoin: key.cpp:344: void ECC_Start(): Assertion `secp256k1_context_sign == nullptr' failed.
unknown location(0): fatal error: in "coinselector_tests/ApproximateBestSubset": signal: SIGABRT (application abort requested)
wallet/test/coinselector_tests.cpp(546): last checkpoint: "ApproximateBestSubset" fixture entry.
wallet/test/coinselector_tests.cpp(546): Leaving test case "ApproximateBestSubset"; testing time: 158us
wallet/test/coinselector_tests.cpp(17): Leaving test suite "coinselector_tests"; testing time: 3805580us
Leaving test module "Bitcoin Test Suite"; testing time: 3805777us
*** 3 failures are detected in the test module "Bitcoin Test Suite"

@practicalswift

@sipa Regarding randbytes – very nice find! How was that issue found?

In a follow-up change I was working on, which replaced more use sites of GetRand* functions with FastRandomContexts. One unit test failed which tested that the leveldb obfuscation key was not all zeroes...

How did you trigger this? I've built with -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC in the past and haven't encountered this. I also tried now and I was unable to reproduce.

Assuming a build with -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC – would the issue be triggered by make check or running the test suite?

Travis failed in an earlier version of this PR, in the Qt (!) unit tests. @MarcoFalke

Travis failure:

Fixed now.

Concept ACK

utACK

Rebased.

Looks like the unit tests don't pass with the thread sanitizer enabled

Rebased.

all straightforward changes utACK e414486d56b9f06af7aeb07ce13e3c3780c2b69b