Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some simple improvements to the RNG code #14624

Merged
merged 8 commits into from Dec 13, 2018

Conversation

Projects
None yet
8 participants
@sipa
Copy link
Member

commented Oct 31, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

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

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.

Show resolved Hide resolved src/test/random_tests.cpp Outdated
@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Concept ACK

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

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
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

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"

@sipa sipa force-pushed the sipa:201810_randfast branch 2 times, most recently Nov 1, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2018

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

Show resolved Hide resolved src/random.cpp Outdated
Show resolved Hide resolved src/addrman.cpp Outdated

@sipa sipa force-pushed the sipa:201810_randfast branch 6 times, most recently Nov 9, 2018

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

Concept ACK

Show resolved Hide resolved src/addrman.h Outdated
Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/random.h

@sipa sipa force-pushed the sipa:201810_randfast branch Nov 14, 2018

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

utACK

@sipa

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

Rebased.

@sipa sipa force-pushed the sipa:201810_randfast branch Nov 30, 2018

@MarcoFalke
Copy link
Member

left a comment

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

@@ -473,7 +470,7 @@ class CAddrMan
{
LOCK(cs);
std::vector<int>().swap(vRandom);
nKey = GetRandHash();
nKey = insecure_rand.rand256();

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 30, 2018

Member

nit: Might as well rename it to m_insecure_rand, since it is only used in two places previously.

@sipa sipa force-pushed the sipa:201810_randfast branch to e414486 Dec 12, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Dec 12, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

all straightforward changes
utACK e414486

@laanwj laanwj merged commit e414486 into bitcoin:master Dec 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 13, 2018

Merge #14624: Some simple improvements to the RNG code
e414486 Do not permit copying FastRandomContexts (Pieter Wuille)
022cf47 Simplify testing RNG code (Pieter Wuille)
fd3e797 Make unit tests use the insecure_rand_ctx exclusively (Pieter Wuille)
8d98d42 Bugfix: randbytes should seed when needed (non reachable issue) (Pieter Wuille)
273d025 Use a FastRandomContext in LimitOrphanTxSize (Pieter Wuille)
3db746b Introduce a Shuffle for FastRandomContext and use it in wallet and coinselection (Pieter Wuille)
8098379 Use a local FastRandomContext in a few more places in net (Pieter Wuille)
9695f31 Make addrman use its local RNG exclusively (Pieter Wuille)

Pull request description:

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

Tree-SHA512: 084c70b533ea68ca7adc0186c39f0b3e0a5c0ae43a12c37286e5d42086e056a8cd026dde61b12c0a296dc80f87fdc87fe303b9e8e6161b460ac2086cf7615f9d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.