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

Switch all RNG code to the built-in PRNG #14955

Merged
merged 14 commits into from Jan 21, 2019

Conversation

Projects
None yet
@sipa
Copy link
Member

commented Dec 14, 2018

This does not remove OpenSSL, but makes our own PRNG the 'main' one; for GetStrongRandBytes, the OpenSSL RNG is still used (indirectly, by feeding its output into our PRNG state).

It includes a few policy changes (regarding what entropy is seeded when).

Before this PR:

  • GetRand*:
    • OpenSSL
  • GetStrongRand*:
    • CPU cycle counter
    • Perfmon data (on Windows, once 10 min)
    • /dev/urandom (or equivalent)
    • rdrand (if available)
  • From scheduler when idle:
    • CPU cycle counter before and after 1ms sleep
  • At startup:
    • CPU cycle counter before and after 1ms sleep

After this PR:

  • GetRand*:
    • Stack pointer (which indirectly identifies thread and some call stack information)
    • rdrand (if available)
    • CPU cycle counter
  • GetStrongRand*:
    • Stack pointer (which indirectly identifies thread and some call stack information)
    • rdrand (if available)
    • CPU cycle counter
    • /dev/urandom (or equivalent)
    • OpenSSL
    • CPU cycle counter again
  • From scheduler when idle:
    • Stack pointer (which indirectly identifies thread and some call stack information)
    • rdrand (if available)
    • CPU cycle counter before and after 1ms sleep
    • Perfmon data (on Windows, once every 10 min)
  • At startup:
    • Stack pointer (which indirectly identifies thread and some call stack information)
    • rdrand (if available)
    • CPU cycle counter
    • /dev/urandom (or equivalent)
    • OpenSSL
    • CPU cycle counter again
    • Perfmon data (on Windows, once every 10 min)

The interface of random.h is also simplified, and documentation is added.

This implements most of #14623.

@fanquake

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

Compile failure on macOS (10.14.2):

./autogen.sh
./configure
make check

  CXX      libbitcoin_util_a-logging.o
  CXX      libbitcoin_util_a-random.o
random.cpp:394:41: error: expected ';' after top level declarator
static unsigned char rng_state[32] = {0} GUARDED_BY(cs_rng_state);
                                        ^
                                        ;
random.cpp:394:42: warning: declaration does not declare anything [-Wmissing-declarations]
static unsigned char rng_state[32] = {0} GUARDED_BY(cs_rng_state);
                                         ^
./threadsafety.h:18:23: note: expanded from macro 'GUARDED_BY'
#define GUARDED_BY(x) __attribute__((guarded_by(x)))
                      ^
random.cpp:395:32: error: expected ';' after top level declarator
static uint64_t rng_counter = 0 GUARDED_BY(cs_rng_state);
                               ^
                               ;
random.cpp:395:33: warning: declaration does not declare anything [-Wmissing-declarations]
static uint64_t rng_counter = 0 GUARDED_BY(cs_rng_state);
                                ^
./threadsafety.h:18:23: note: expanded from macro 'GUARDED_BY'
#define GUARDED_BY(x) __attribute__((guarded_by(x)))
                      ^
2 warnings and 2 errors generated.
make[2]: *** [libbitcoin_util_a-random.o] Error 1
make[1]: *** [check-recursive] Error 1
make: *** [check-recursive] Error 1
@gmaxwell

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

Might be desirable to stick the openssl stuff behind a define already, I expect we'll be able to ship 0.18 without linking bitcoind to openssl.

@sipa sipa force-pushed the sipa:201812_generic_rand branch Dec 14, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

@fanquake Hopefully fixed.

@gmaxwell There are probably a few entropy sources we want to add first, I think.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@sipa I am eager to add entropy sources! (but I meant behind a ifdef that is currently on... just since I think you're touching the only remaining callsites)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 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:

  • #13746 (-masterdatadir for datadir bootstrapping by kallewoof)
  • #13339 (wallet: Replace %w by wallet name in -walletnotify script by promag)
  • #11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)

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/random.cpp
Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/random.cpp Outdated
Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/random.h Outdated

@sipa sipa force-pushed the sipa:201812_generic_rand branch Dec 14, 2018

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

@sipa sipa force-pushed the sipa:201812_generic_rand branch 2 times, most recently Dec 18, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

Made a number of improvements, and split the history out into hopefully more self-contained commits.

@sipa sipa force-pushed the sipa:201812_generic_rand branch Dec 18, 2018

@ken2812221

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

In constructor of CTxMemPool require to call random functions, but the constructor of the mutex has not being called yet. (Maybe it is an undefined behavior or it's a bug of VC++)

CTxMemPool mempool(&feeEstimator);

 	test_bitcoin.exe!SeedStartup(CSHA512 & hasher={...}) 第 417 行	C++
 	test_bitcoin.exe!ProcRand(unsigned char * out=0x00000031da8febb8, int num=8, RNGLevel level=STARTUP) 第 462 行	C++
 	test_bitcoin.exe!GetRandBytes(unsigned char * buf=0x00000031da8febb8, int num=8) 第 476 行	C++
 	test_bitcoin.exe!GetRand(unsigned __int64 nMax=18446744073709551615) 第 491 行	C++
 	test_bitcoin.exe!SaltedTxidHasher::SaltedTxidHasher() 第 1093 行	C++
 	test_bitcoin.exe!boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> >::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> >() 第 284 行	C++
 	test_bitcoin.exe!boost::tuples::cons<mempoolentry_txid,boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> > >::cons<mempoolentry_txid,boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> > >() 第 284 行	C++
 	test_bitcoin.exe!boost::tuples::cons<unsigned __int64,boost::tuples::cons<mempoolentry_txid,boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> > > >::cons<unsigned __int64,boost::tuples::cons<mempoolentry_txid,boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> > > >() 第 284 行	C++
 	test_bitcoin.exe!boost::tuples::tuple<unsigned __int64,mempoolentry_txid,SaltedTxidHasher,std::equal_to<uint256>,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>::tuple<unsigned __int64,mempoolentry_txid,SaltedTxidHasher,std::equal_to<uint256>,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>() 第 485 行	C++
 	test_bitcoin.exe!boost::tuples::cons<boost::tuples::tuple<unsigned __int64,mempoolentry_txid,SaltedTxidHasher,std::equal_to<uint256>,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByDescendantScore,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByEntryTime,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByAncestorFee,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::null_type> > > >::cons<boost::tuples::tuple<unsigned __int64,mempoolentry_txid,SaltedTxidHasher,std::equal_to<uint256>,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByDescendantScore,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByEntryTime,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByAncestorFee,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::null_type> > > >() 第 284 行	C++
 	test_bitcoin.exe!boost::multi_index::multi_index_container<CTxMemPoolEntry,boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid,SaltedTxidHasher,boost::mpl::na,boost::mpl::na>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByDescendantScore>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByEntryTime>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByAncestorFee>,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,std::allocator<CTxMemPoolEntry> >::multi_index_container<CTxMemPoolEntry,boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid,SaltedTxidHasher,boost::mpl::na,boost::mpl::na>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByDescendantScore>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByEntryTime>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByAncestorFee>,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,std::allocator<CTxMemPoolEntry> >() 第 177 行	C++
 	test_bitcoin.exe!CTxMemPool::CTxMemPool(CBlockPolicyEstimator * estimator=0x00007ff65afbfe60) 第 328 行	C++
>	test_bitcoin.exe!`dynamic initializer for 'mempool''() 第 244 行	C++
 	test_bitcoin.exe!_initterm(void(*)() * first=0x00007ff65a49c000, void(*)() * last=0x00007ff65a49f848) 第 22 行	C++
 	[外部程式碼]	

@sipa sipa force-pushed the sipa:201812_generic_rand branch 2 times, most recently Dec 18, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

@ken2812221 That was helpful, thanks! I think I've fixed it, but the same error still appears. Can you check what line number it's on now?

@ken2812221

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

 	test_bitcoin.exe!std::_Load_relaxed_4(volatile unsigned long * _Tgt=0x0000000000000074) 第 1338 行	C++
 	test_bitcoin.exe!std::_Atomic_load_4(volatile unsigned long * _Tgt=0x0000000000000074, std::memory_order _Order=memory_order_relaxed) 第 1357 行	C++
 	test_bitcoin.exe!std::atomic_load_explicit(const std::_Atomic_uint * _Atom=0x0000000000000074, std::memory_order _Order=memory_order_relaxed) 第 495 行	C++
 	test_bitcoin.exe!std::_Atomic_uint::load(std::memory_order _Order=memory_order_relaxed) 第 630 行	C++
 	test_bitcoin.exe!BCLog::Logger::WillLogCategory(BCLog::LogFlags category=RAND) 第 83 行	C++
 	test_bitcoin.exe!LogAcceptCategory(BCLog::LogFlags category=RAND) 第 117 行	C++
 	test_bitcoin.exe!LogPrint<char [15],char [19],unsigned long>(const BCLog::LogFlags & category=RAND, const char[15] & <args_0>=..., const char[19] & <args_1>=..., const unsigned long & <args_2>=707688) 第 150 行	C++
>	test_bitcoin.exe!RandAddSeedPerfmon(CSHA512 & hasher={...}) 第 199 行	C++
 	test_bitcoin.exe!SeedStartup(CSHA512 & hasher={...}, `anonymous-namespace'::RNGState & rng={...}) 第 456 行	C++
 	test_bitcoin.exe!ProcRand(unsigned char * out=0x0000009f8cd8eb78, int num=8, RNGLevel level=FAST) 第 493 行	C++
 	test_bitcoin.exe!GetRandBytes(unsigned char * buf=0x0000009f8cd8eb78, int num=8) 第 508 行	C++
 	test_bitcoin.exe!GetRand(unsigned __int64 nMax=18446744073709551615) 第 523 行	C++
 	test_bitcoin.exe!SaltedTxidHasher::SaltedTxidHasher() 第 1093 行	C++
 	test_bitcoin.exe!boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> >::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> >() 第 284 行	C++
 	test_bitcoin.exe!boost::tuples::cons<mempoolentry_txid,boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> > >::cons<mempoolentry_txid,boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> > >() 第 284 行	C++
 	test_bitcoin.exe!boost::tuples::cons<unsigned __int64,boost::tuples::cons<mempoolentry_txid,boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> > > >::cons<unsigned __int64,boost::tuples::cons<mempoolentry_txid,boost::tuples::cons<SaltedTxidHasher,boost::tuples::cons<std::equal_to<uint256>,boost::tuples::null_type> > > >() 第 284 行	C++
 	test_bitcoin.exe!boost::tuples::tuple<unsigned __int64,mempoolentry_txid,SaltedTxidHasher,std::equal_to<uint256>,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>::tuple<unsigned __int64,mempoolentry_txid,SaltedTxidHasher,std::equal_to<uint256>,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>() 第 485 行	C++
 	test_bitcoin.exe!boost::tuples::cons<boost::tuples::tuple<unsigned __int64,mempoolentry_txid,SaltedTxidHasher,std::equal_to<uint256>,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByDescendantScore,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByEntryTime,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByAncestorFee,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::null_type> > > >::cons<boost::tuples::tuple<unsigned __int64,mempoolentry_txid,SaltedTxidHasher,std::equal_to<uint256>,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByDescendantScore,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByEntryTime,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::cons<boost::tuples::tuple<boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByAncestorFee,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type,boost::tuples::null_type>,boost::tuples::null_type> > > >() 第 284 行	C++
 	test_bitcoin.exe!boost::multi_index::multi_index_container<CTxMemPoolEntry,boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid,SaltedTxidHasher,boost::mpl::na,boost::mpl::na>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByDescendantScore>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByEntryTime>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByAncestorFee>,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,std::allocator<CTxMemPoolEntry> >::multi_index_container<CTxMemPoolEntry,boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid,SaltedTxidHasher,boost::mpl::na,boost::mpl::na>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByDescendantScore>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByEntryTime>,boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,boost::multi_index::identity<CTxMemPoolEntry>,CompareTxMemPoolEntryByAncestorFee>,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na,boost::mpl::na>,std::allocator<CTxMemPoolEntry> >() 第 177 行	C++
 	test_bitcoin.exe!CTxMemPool::CTxMemPool(CBlockPolicyEstimator * estimator=0x00007ff689f81fb0) 第 328 行	C++
 	test_bitcoin.exe!`dynamic initializer for 'mempool''() 第 244 行	C++
 	test_bitcoin.exe!_initterm(void(*)() * first=0x00007ff68945e000, void(*)() * last=0x00007ff689461830) 第 22 行	C++
 	[外部程式碼]	

g_logger has not been newed.

BCLog::Logger* const g_logger = new BCLog::Logger();

@sipa

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

@ken2812221 Thanks so much; this was an actual bug.

@sipa sipa force-pushed the sipa:201812_generic_rand branch Dec 19, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Dec 19, 2018

@bitcoin bitcoin deleted a comment Dec 19, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

I've made a few policy changes still:

  • Startup no longer includes the benchmark-1ms-sleep test, but still does strengthening.
  • The background seeding (called by the scheduler during idle times) no longer sources OpenSSL and /dev/random, as doing so every millisecond seems very much overkill. (it still includes rdrand as that has negligable CPU overhead).
  • The seeders are renamed to "fast" (called by GetRand*), "slow" (called by GetStrongRand*), "background" (called by idle scheduler), and "startup" (called only once at startup).

The global-order-independent initialization now uses a function which stores the RNG state in a local static variable. C++11 guarantees that it is initialized on first call, even if called multiple times simultaneously. I've benchmarked this approach and it's even faster than using std::call_once (2ns vs 1.8ns); I believe it's due to this approach using inline code generated by the compiler instead of a library call into pthread.

Show resolved Hide resolved src/random.cpp Outdated
@ryanofsky
Copy link
Contributor

left a comment

utACK 76831bf79bb453dc00bc8f5a94a784581e1212d3. Changes since last review: dropped SanityCheck cleanup commit 2ce2c827115748ebea121e6e2abfded260f50ea6, added documentation commit 76831bf79bb453dc00bc8f5a94a784581e1212d3, made various suggested changes in other commits.

  • 6a57ca9 Use FRC::randbytes instead of reading >32 bytes from RNG (1/13)
  • 1a3b26e8e3ee64420cb5081de97ce1944f8b9a7a Don't log RandAddSeedPerfmon details (2/13)
  • 2a02d2c369ee06cbc334a0dd7150ec72ef83a182 Automatically initialize RNG on first use. (3/13)
  • 9a4aae5427a3a71298413b9348073512dbb43056 Add thread safety annotations to RNG state (4/13)
  • d76cc6dbb9e1c5c6a0f3b002932ba4033b722255 Abstract out seeding/extracting entropy into RNGState::MixExtract (5/13)
  • 85f8f99e0ae9d1c98f676ae8722cda4b967dcfb4 Switch all RNG code to the built-in PRNG. (6/13)
  • 604ae2bb9789a213711f5f53e0c74225e74bb085 Remove hwrand_initialized. (7/13)
  • 6ff775ff80b090494d115753ddcf488a3cf923a9 Sprinkle some sweet noexcepts over the RNG code (8/13)
  • fa2eabe154d00bf6c54f604acbe758186d044639 Use our own locking instead of using OpenSSL in multithreaded mode (9/13)
  • d8972a68a63a2dddd910d340b8ff05b81fd877bc DRY: Implement GetRand using FastRandomContext::randrange (10/13)
  • 7939daa351a31a36f2086771611b501418536989 Encapsulate RNGState better (11/13)
  • 9505dc55c4fcaa61d0fc97558dd22c7fdb879900 Use secure allocator for RNG state (12/13)
  • 76831bf79bb453dc00bc8f5a94a784581e1212d3 Document RNG design in random.h (13/13)
}
// Best effort cleanup of internal state
hasher.Reset();
memory_cleanse(buf, 64);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 14, 2019

Contributor

In commit "Abstract out seeding/extracting entropy into RNGState::MixExtract" (d76cc6dbb9e1c5c6a0f3b002932ba4033b722255)

Does it matter that memory_cleanse isn't called on the hasher object?

This comment has been minimized.

Copy link
@sipa

sipa Jan 16, 2019

Author Member

Perhaps it does, though we're all over the code only using memory_cleanse for memory buffers directly, and I feel slightly uneasy to invoke on the state of complex objects.

Maybe in a separate change we can add a Cleanse() method to CSHA512 and others, and start using those were useful?

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 16, 2019

Member

Such a Cleanse() method might indeed improve readability, and maybe even offers a way to automatically detect where it's potentially missing. Would it then also make sense to have a unsigned char [] alternative with a Cleanse() method? E.g. hygienic_char [] :-)

Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/util/system.cpp
Show resolved Hide resolved src/util/system.cpp

@sipa sipa force-pushed the sipa:201812_generic_rand branch 2 times, most recently Jan 16, 2019

@Sjors

Sjors approved these changes Jan 16, 2019

Copy link
Member

left a comment

tACK 3b001b0 on macOS 10.14.2

Despite the volume of my comments, they're all about additional clarification, which can always be done later (though I think they'll be helpful for other reviewers).

This new code is more readable than before, which is very nice for something as critical as RNG stuff. The breakdown into smaller commits is really helpful.

This needs testing on Windows, e.g. because there's #ifdef WIN32 stuff.

Checking the benchmarks would also be useful (IIRC there's a bot for that).

  • 6a57ca9 "There was only one place in the codebase where we're directly reading >32 bytes from the RNG": what happens in that case? If bad, maybe add an assert or comment in GenerateAuthCookie that const size_t COOKIE_SIZE = 32; shouldn't be increased.

  • 1a3b26e8e3ee64420cb5081de97ce1944f8b9a7a the usage of RandAddSeedPerfmon is a bit weird

It's called by all operating systems, but its entire content is wrapped in #ifdef WIN32. On top of that, it can now fail without complaining (because it's non-critical). If static bool warned is hard to deal with, why not give the function a return value? Maybe move both the ifdef and failure handling to the caller of this function (with or without the log statement). Both callers already know this is a Windows only thing, as evidenced by their comments.

  • 2a02d2c369ee06cbc334a0dd7150ec72ef83a182 I got confused by the names HWRandInit and HWRandReport, so suggested alternative names inline.

  • 772fce745c51aa9d09f93e13eeb822e889ae25f7 OpenSSL docs say "OpenSSL can generally be used safely in multi-threaded applications provided that at least two callback functions are set, the locking_function and threadid_func", but we're not using the latter. Do we meet the conditions stated further down the doc for when that's safe?

  • b83c06a622652853238f3c35b9303194eafec1c1 what benefits do you expect from the noexcept sprinkling? Safety, readability or performance? What's the worst case scenario if you're wrong about any of them?

  • 63dc0adf7c98b20eb9ca590c97547b9681d2b48c previously we only used secure_allocator in CKey and (wallet) CCrypter. Not sure if it matters at all to expand its usage, but I don't pretent to even remotely understand the existing magic in secure_allocator (written by @theuni four years ago).

Show resolved Hide resolved src/random.h
Show resolved Hide resolved src/random.cpp Outdated
Show resolved Hide resolved src/random.cpp Outdated
Show resolved Hide resolved src/random.cpp Outdated
Show resolved Hide resolved src/random.cpp Outdated
Show resolved Hide resolved src/util/system.cpp
Show resolved Hide resolved src/random.cpp Outdated
Show resolved Hide resolved src/random.cpp Outdated
Show resolved Hide resolved src/random.cpp
Show resolved Hide resolved src/random.cpp
@sipa

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2019

6a57ca9 "There was only one place in the codebase where we're directly reading >32 bytes from the RNG": what happens in that case? If bad, maybe add an assert or comment in GenerateAuthCookie that const size_t COOKIE_SIZE = 32; shouldn't be increased.

There already is an assert: assert(num <= 32); in ProcRand().

1a3b26e the usage of RandAddSeedPerfmon is a bit weird. It's called by all operating systems, but its entire content is wrapped in #ifdef WIN32. On top of that, it can now fail without complaining (because it's non-critical). If static bool warned is hard to deal with, why not give the function a return value? Maybe move both the ifdef and failure handling to the caller of this function (with or without the log statement). Both callers already know this is a Windows only thing, as evidenced by their comments.

As I've pointed out before, we may want to add similar perfmon data for other platforms.

772fce7 OpenSSL docs say "OpenSSL can generally be used safely in multi-threaded applications provided that at least two callback functions are set, the locking_function and threadid_func", but we're not using the latter. Do we meet the conditions stated further down the doc for when that's safe?

The docs also point out that on systems with thread-safe errno, its address is used as thread identifier. I believe that's the case on all supported platforms.

b83c06a what benefits do you expect from the noexcept sprinkling? Safety, readability or performance? What's the worst case scenario if you're wrong about any of them?

Making the function's behavior more explicit to developers, and performance. The worst case about being wrong about them is that certain optimizations can't be used (for example, when a constructor can directly or indirectly throw, more complicated deconstructors are needed; or when a move operator can throw it can't be used for efficiently moving data between containers and necessitating a copy/delete instead).

63dc0ad previously we only used secure_allocator in CKey and (wallet) CCrypter. Not sure if it matters at all to expand its usage, but I don't pretent to even remotely understand the existing magic in secure_allocator

It shouldn't. It's the right tool for the job (preventing sensitive material from leaking into swap files), so I don't see why we shouldn't use it.

@ryanofsky
Copy link
Contributor

left a comment

utACK 3b001b09e840c04b06c0ec6689ed666d68e58cb1. Changes since last review: moving openssl init/locking code instead of removing it, adding commit comments about openssl seeding and function renames, adding code comment about noexcept.

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

sipa added some commits Dec 19, 2018

Don't log RandAddSeedPerfmon details
These are hard to deal with, as in a follow-up this function can get
called before the logging infrastructure is initialized.

@sipa sipa force-pushed the sipa:201812_generic_rand branch Jan 17, 2019

sipa added some commits Dec 18, 2018

Integrate util/system's CInit into RNGState
This guarantees that OpenSSL is initialized properly whenever randomness
is used, even when that randomness is invoked from global constructors.

Note that this patch uses Mutex directly, rather than CCriticalSection.
This is because the lock-detection code is not necessarily initialized
during global constructors.
Switch all RNG code to the built-in PRNG.
It includes the following policy changes:
* All GetRand* functions seed the stack pointer and rdrand result
  (in addition to the performance counter)
* The periodic entropy added by the idle scheduler now seeds stack pointer,
  rdrand and perfmon data (once every 10 minutes) in addition to
  just a sleep timing.
* The entropy added when calling GetStrongRandBytes no longer includes
  the once-per-10-minutes perfmon data on windows (it is moved to the
  idle scheduler instead, where latency matters less).

Other changes:
* OpenSSL is no longer seeded directly anywhere. Instead, any generated
  randomness through our own RNG is fed back to OpenSSL (after an
  additional hashing step to prevent leaking our RNG state).
* Seeding that was previously done directly in RandAddSeedSleep is now
  moved to SeedSleep(), which is indirectly invoked through ProcRand
  from RandAddSeedSleep.
* Seeding that was previously done directly in GetStrongRandBytes()
  is now moved to SeedSlow(), which is indirectly invoked through
  ProcRand from GetStrongRandBytes().
Remove hwrand_initialized.
 All access to hwrand is now gated by GetRNGState, which initializes the hwrand code.

@sipa sipa force-pushed the sipa:201812_generic_rand branch to 223de8d Jan 17, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

Wonderful, re-tACK 223de8d

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

FWIW, I've benchmarked this before and after this PR on my system. GetRandBytes goes from around ~1 μs to ~3 μs. GetStrongRandBytes stays around ~10 μs. Note that GetRandBytes is no longer called inside tight loops since #14624.

@ryanofsky
Copy link
Contributor

left a comment

utACK 223de8d. All changes since last review are renames or comment improvements.

@laanwj laanwj merged commit 223de8d into bitcoin:master Jan 21, 2019

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 Jan 21, 2019

Merge #14955: Switch all RNG code to the built-in PRNG
223de8d Document RNG design in random.h (Pieter Wuille)
f2e60ca Use secure allocator for RNG state (Pieter Wuille)
cddb31b Encapsulate RNGState better (Pieter Wuille)
152146e DRY: Implement GetRand using FastRandomContext::randrange (Pieter Wuille)
a1f252e Sprinkle some sweet noexcepts over the RNG code (Pieter Wuille)
4ea8e50 Remove hwrand_initialized. (Pieter Wuille)
9d7032e Switch all RNG code to the built-in PRNG. (Pieter Wuille)
16e40a8 Integrate util/system's CInit into RNGState (Pieter Wuille)
2ccc3d3 Abstract out seeding/extracting entropy into RNGState::MixExtract (Pieter Wuille)
aae8b9b Add thread safety annotations to RNG state (Pieter Wuille)
d3f54d1 Rename some hardware RNG related functions (Pieter Wuille)
05fde14 Automatically initialize RNG on first use. (Pieter Wuille)
2d1cc50 Don't log RandAddSeedPerfmon details (Pieter Wuille)
6a57ca9 Use FRC::randbytes instead of reading >32 bytes from RNG (Pieter Wuille)

Pull request description:

  This does not remove OpenSSL, but makes our own PRNG the 'main' one; for GetStrongRandBytes, the OpenSSL RNG is still used (indirectly, by feeding its output into our PRNG state).

  It includes a few policy changes (regarding what entropy is seeded when).

  Before this PR:
  * GetRand*:
    * OpenSSL
  * GetStrongRand*:
    * CPU cycle counter
    * Perfmon data (on Windows, once 10 min)
    * /dev/urandom (or equivalent)
    * rdrand (if available)
  * From scheduler when idle:
    * CPU cycle counter before and after 1ms sleep
  * At startup:
    * CPU cycle counter before and after 1ms sleep

  After this PR:
  * GetRand*:
    * Stack pointer (which indirectly identifies thread and some call stack information)
    * rdrand (if available)
    * CPU cycle counter
  * GetStrongRand*:
    * Stack pointer (which indirectly identifies thread and some call stack information)
    * rdrand (if available)
    * CPU cycle counter
    * /dev/urandom (or equivalent)
    * OpenSSL
    * CPU cycle counter again
  * From scheduler when idle:
    * Stack pointer (which indirectly identifies thread and some call stack information)
    * rdrand (if available)
    * CPU cycle counter before and after 1ms sleep
    * Perfmon data (on Windows, once every 10 min)
  * At startup:
    * Stack pointer (which indirectly identifies thread and some call stack information)
    * rdrand (if available)
    * CPU cycle counter
    * /dev/urandom (or equivalent)
    * OpenSSL
    * CPU cycle counter again
    * Perfmon data (on Windows, once every 10 min)

  The interface of random.h is also simplified, and documentation is added.

  This implements most of #14623.

Tree-SHA512: 0120e19bd4ce80a509b5c180a4f29497d299ce8242e25755880851344b825bc2d64a222bc245e659562fb5463fb7c70fbfcf003616be4dc59d0ed6534f93dd20

@fanquake fanquake removed this from Blockers in High-priority for review Jan 21, 2019

@Warrows Warrows referenced this pull request Jan 24, 2019

Open

Bring PIVX random on par with bitcoin #799

3 of 48 tasks complete
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.