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

pull sipa wants to merge 14 commits into bitcoin:master from sipa:201812_generic_rand changing 8 files +344 −185
  1. sipa commented at 3:02 am on December 14, 2018: member

    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.

  2. fanquake added the label Refactoring on Dec 14, 2018
  3. fanquake added the label Utils/log/libs on Dec 14, 2018
  4. fanquake commented at 4:33 am on December 14, 2018: member

    Compile failure on macOS (10.14.2):

     0./autogen.sh
 1./configure
 2make check
 3
 4  CXX      libbitcoin_util_a-logging.o
 5  CXX      libbitcoin_util_a-random.o
 6random.cpp:394:41: error: expected ';' after top level declarator
 7static unsigned char rng_state[32] = {0} GUARDED_BY(cs_rng_state);
 8                                        ^
 9                                        ;
10random.cpp:394:42: warning: declaration does not declare anything [-Wmissing-declarations]
11static unsigned char rng_state[32] = {0} GUARDED_BY(cs_rng_state);
12                                         ^
13./threadsafety.h:18:23: note: expanded from macro 'GUARDED_BY'
14#define GUARDED_BY(x) __attribute__((guarded_by(x)))
15                      ^
16random.cpp:395:32: error: expected ';' after top level declarator
17static uint64_t rng_counter = 0 GUARDED_BY(cs_rng_state);
18                               ^
19                               ;
20random.cpp:395:33: warning: declaration does not declare anything [-Wmissing-declarations]
21static uint64_t rng_counter = 0 GUARDED_BY(cs_rng_state);
22                                ^
23./threadsafety.h:18:23: note: expanded from macro 'GUARDED_BY'
24#define GUARDED_BY(x) __attribute__((guarded_by(x)))
25                      ^
262 warnings and 2 errors generated.
27make[2]: *** [libbitcoin_util_a-random.o] Error 1
28make[1]: *** [check-recursive] Error 1
29make: *** [check-recursive] Error 1
  5. gmaxwell commented at 6:04 am on December 14, 2018: contributor
    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.
  6. sipa force-pushed on Dec 14, 2018
  7. sipa commented at 7:08 am on December 14, 2018: member
    @fanquake Hopefully fixed. @gmaxwell There are probably a few entropy sources we want to add first, I think.
  8. gmaxwell commented at 7:44 am on December 14, 2018: contributor
    @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)
  9. DrahtBot commented at 8:39 am on December 14, 2018: member

    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.

  10. in src/random.cpp:146 in b2fbb03a2e outdated 
    172+    memory_cleanse(buffer, sizeof(buffer));
173+    local_hasher.Reset();
174 }
175 
176-static void RandAddSeedPerfmon()
177+static void RandAddSeedPerfmon(CSHA512& hasher)
    practicalswift commented at 12:54 pm on December 14, 2018:
    Surround RandAddSeedPerfmon(CSHA512& hasher) with #ifdef WIN32?
    sipa commented at 7:06 pm on December 14, 2018:
    I’d rather not. Limiting the ifdefs to just one place is preferable imo.
  11. in src/random.cpp:454 in b2fbb03a2e outdated 
    390+    // High-precision timestamp after sleeping
391+    int64_t perfcounter = GetPerformanceCounter();
392+    hasher.Write((const unsigned char*)&perfcounter, sizeof(perfcounter));
393+
394+    // Windows performance monitor data (once every 10 minutes)
395+    RandAddSeedPerfmon(hasher);
    practicalswift commented at 12:55 pm on December 14, 2018:
    #ifdef WIN32 for this call?
    sipa commented at 7:05 pm on December 14, 2018:
    No need for. Perhaps we’ll extend the function to do something useful on other platforms too.
  12. in src/random.cpp:459 in b2fbb03a2e outdated 
    457+    // Best effort clean up of randomness data on the stack.
458+    memory_cleanse(buf, sizeof(buf));
459+    hasher.Reset();
460 }
461 
462+void GetRandBytes(unsigned char* out, int num) { ProcRand(out, num, RNGLevel::FAST); }
    practicalswift commented at 12:56 pm on December 14, 2018:
    Make sure definition and declaration parameter names match.
    sipa commented at 1:33 am on December 18, 2018:
    Done.
  13. in src/random.cpp:610 in b2fbb03a2e outdated 
    556@@ -448,10 +557,6 @@ bool Random_SanityCheck()
557     uint64_t stop = GetPerformanceCounter();
558     if (stop == start) return false;
559 
560-    // We called GetPerformanceCounter. Use it as entropy.
561-    RAND_add((const unsigned char*)&start, sizeof(start), 1);
562-    RAND_add((const unsigned char*)&stop, sizeof(stop), 1);
563-
564     return true;
    practicalswift commented at 12:57 pm on December 14, 2018:
    Change to return stop != start;?
    sipa commented at 7:06 pm on December 14, 2018:
    I’m not touching this code.
  14. in src/random.cpp:476 in b2fbb03a2e outdated 
    433+    MEDIUM, //!< Automatically called by GetStrongRandBytes
434+    SLOW, //!< Called by RandSeed() and at startup
435+    STARTUP, //!< Called only at startup.
436+};
437+
438+static void ProcRand(unsigned char* out, int num, RNGLevel level)
    practicalswift commented at 12:59 pm on December 14, 2018:
    ProcRand(…) doesn’t throw – candidate for noexcept?
    sipa commented at 11:42 pm on December 17, 2018:
    It does throw when threads get interrupted apparently (as it has a sleep inside).
    practicalswift commented at 9:56 am on December 19, 2018:
    Uh, ok. Thanks!
  15. in src/random.h:20 in b2fbb03a2e outdated 
    19 /**
20- * Functions to gather random data via the OpenSSL PRNG
21+ * Generate random data via the internal PRNG.
22+ *
23+ * These functions are designed to be fast (sub microsecond), but do not necessarily
24+ * meaninfully add entropy to the PRNG state.
    practicalswift commented at 12:59 pm on December 14, 2018:
    Should be “meaningfully” :-)
    sipa commented at 1:34 am on December 18, 2018:
    Fixed.
  16. sipa force-pushed on Dec 14, 2018
  17. in src/random.cpp:159 in 0af8348738 outdated 
    154+            local_hasher.Finalize(buffer);
155+            local_hasher.Reset();
156+            local_hasher.Write(buffer, sizeof(buffer));
157+        }
158+        // Write benchmark data (into output hasher)
159+        int64_t perf = GetPerformanceCounter();
    gmaxwell commented at 9:49 pm on December 14, 2018:
    SeedTimestamp() ?
    gmaxwell commented at 9:54 pm on December 14, 2018:
    Maybe not because I think you should change the strengthening function (just the local_hasher) to 4-wide sha256d64 in order to increase the “work” done per unit time, and seed timestamp takes a hasher. We’ll do a lot more work with sha-ni/aes and that sha256d64-4-way is probably the most optimized work function in the codebase.
    sipa commented at 1:35 am on December 18, 2018:

    Done (using SeedTimestamp()).

    Leaving this for now; switching to SHA256-based code requires some refactoring to make sure there are no initialization order issues.

  18. in src/random.cpp:325 in ff85bace11 outdated 
    351-        memcpy(rng_state, buf + 32, 32);
352-    }
353-    memory_cleanse(buf, 64);
354+    // High-precision timestamp after sleeping
355+    int64_t perfcounter = GetPerformanceCounter();
356+    hasher.Write((const unsigned char*)&perfcounter, sizeof(perfcounter));
    gmaxwell commented at 9:54 pm on December 14, 2018:
    SeedTimestamp()
  19. in src/random.cpp:369 in 0af8348738 outdated 
    364+    static std::atomic<int64_t> last_strengthen;
365+    int64_t current_time = GetTimeMicros();
366+    if (current_time > last_strengthen + 60000) {
367+        // Write randomness from the actual RNG in the state, so it goes through the strengthening as well.
368+        unsigned char buffer[32];
369+        GetRandBytes(buffer, 32);
    gmaxwell commented at 10:00 pm on December 14, 2018:
    I’d kinda feel better about this if it just directly took the cs_rng_state lock and read the state here instead of calling GetRandBytes… at least then the code won’t mysteriously crash if someone upgrades GetRandBytes to use ::Slow.
    sipa commented at 1:36 am on December 18, 2018:
    I’ve abstracted the update-the-global-RNG-and-extract-entropy into a separate function, which is invoked from both this seeder and RandProc.
  20. MarcoFalke added the label Needs gitian build on Dec 17, 2018
  21. sipa force-pushed on Dec 18, 2018
  22. sipa force-pushed on Dec 18, 2018
  23. sipa commented at 1:37 am on December 18, 2018: member
    Made a number of improvements, and split the history out into hopefully more self-contained commits.
  24. sipa force-pushed on Dec 18, 2018
  25. DrahtBot removed the label Needs gitian build on Dec 18, 2018
  26. ken2812221 commented at 12:43 pm on December 18, 2018: contributor

    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++) https://github.com/bitcoin/bitcoin/blob/e7b88ecbc920321290941bc68e4a71634889c3cb/src/validation.cpp#L244

     0 	test_bitcoin.exe!SeedStartup(CSHA512 & hasher={...}) 第 417 行	C++
 1 	test_bitcoin.exe!ProcRand(unsigned char * out=0x00000031da8febb8, int num=8, RNGLevel level=STARTUP) 第 462 行	C++
 2 	test_bitcoin.exe!GetRandBytes(unsigned char * buf=0x00000031da8febb8, int num=8) 第 476 行	C++
 3 	test_bitcoin.exe!GetRand(unsigned __int64 nMax=18446744073709551615) 第 491 行	C++
 4 	test_bitcoin.exe!SaltedTxidHasher::SaltedTxidHasher() 第 1093 行	C++
 5 	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++
 6 	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++
 7 	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++
 8 	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++
 9 	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++
10 	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++
11 	test_bitcoin.exe!CTxMemPool::CTxMemPool(CBlockPolicyEstimator * estimator=0x00007ff65afbfe60) 第 328 行	C++
12>	test_bitcoin.exe!`dynamic initializer for 'mempool''() 第 244 行	C++
13 	test_bitcoin.exe!_initterm(void(*)() * first=0x00007ff65a49c000, void(*)() * last=0x00007ff65a49f848) 第 22 行	C++
14 	[外部程式碼]
  27. sipa force-pushed on Dec 18, 2018
  28. sipa force-pushed on Dec 19, 2018
  29. sipa commented at 6:04 am on December 19, 2018: member
    @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?
  30. ken2812221 commented at 9:21 am on December 19, 2018: contributor 
     0 	test_bitcoin.exe!std::_Load_relaxed_4(volatile unsigned long * _Tgt=0x0000000000000074) 第 1338 行	C++
 1 	test_bitcoin.exe!std::_Atomic_load_4(volatile unsigned long * _Tgt=0x0000000000000074, std::memory_order _Order=memory_order_relaxed) 第 1357 行	C++
 2 	test_bitcoin.exe!std::atomic_load_explicit(const std::_Atomic_uint * _Atom=0x0000000000000074, std::memory_order _Order=memory_order_relaxed) 第 495 行	C++
 3 	test_bitcoin.exe!std::_Atomic_uint::load(std::memory_order _Order=memory_order_relaxed) 第 630 行	C++
 4 	test_bitcoin.exe!BCLog::Logger::WillLogCategory(BCLog::LogFlags category=RAND) 第 83 行	C++
 5 	test_bitcoin.exe!LogAcceptCategory(BCLog::LogFlags category=RAND) 第 117 行	C++
 6 	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++
 7>	test_bitcoin.exe!RandAddSeedPerfmon(CSHA512 & hasher={...}) 第 199 行	C++
 8 	test_bitcoin.exe!SeedStartup(CSHA512 & hasher={...}, `anonymous-namespace'::RNGState & rng={...}) 第 456 行	C++
 9 	test_bitcoin.exe!ProcRand(unsigned char * out=0x0000009f8cd8eb78, int num=8, RNGLevel level=FAST) 第 493 行	C++
10 	test_bitcoin.exe!GetRandBytes(unsigned char * buf=0x0000009f8cd8eb78, int num=8) 第 508 行	C++
11 	test_bitcoin.exe!GetRand(unsigned __int64 nMax=18446744073709551615) 第 523 行	C++
12 	test_bitcoin.exe!SaltedTxidHasher::SaltedTxidHasher() 第 1093 行	C++
13 	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++
14 	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++
15 	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++
16 	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++
17 	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++
18 	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++
19 	test_bitcoin.exe!CTxMemPool::CTxMemPool(CBlockPolicyEstimator * estimator=0x00007ff689f81fb0) 第 328 行	C++
20 	test_bitcoin.exe!`dynamic initializer for 'mempool''() 第 244 行	C++
21 	test_bitcoin.exe!_initterm(void(*)() * first=0x00007ff68945e000, void(*)() * last=0x00007ff689461830) 第 22 行	C++
22 	[外部程式碼]

    g_logger has not been newed. https://github.com/bitcoin/bitcoin/blob/f080c65a09a8f3b223c9b5d8e3562320bf258fcd/src/logging.cpp#L24

  31. sipa commented at 9:51 am on December 19, 2018: member
    @ken2812221 Thanks so much; this was an actual bug.
  32. sipa force-pushed on Dec 19, 2018
  33. MarcoFalke deleted a comment on Dec 19, 2018
  34. fanquake deleted a comment on Dec 19, 2018
  35. sipa commented at 11:25 pm on December 19, 2018: member

    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.

  36. in src/random.cpp:161 in 0644236758 outdated 
    161+            local_hasher.Write(buffer, sizeof(buffer));
162+        }
163+        // Write benchmark data (into output hasher)
164+        int64_t perf = GetPerformanceCounter();
165+        hasher.Write((const unsigned char*)&perf, sizeof(perf));
166+    } while (GetTimeMicros() < start + microseconds);
    gmaxwell commented at 10:56 am on December 20, 2018:
    Maybe we should be checking for shutdown interruption in this loop? At 10ms it isn’t critical…
    sipa commented at 1:08 am on January 16, 2019:
    Marking as resolved as the hardening code is removed from this PR.
  37. fanquake deleted a comment on Dec 20, 2018
  38. in src/random.cpp:483 in 0644236758 outdated 
    528-        hasher.Write((const unsigned char*)&rng_counter, sizeof(rng_counter));
529-        ++rng_counter;
530-        hasher.Finalize(buf);
531-        memcpy(rng_state, buf + 32, 32);
532+    if (!rng.MixExtract(out, num, std::move(hasher), false)) {
533+        // On the first invocation, restart but with SeedStartup.
    gmaxwell commented at 1:28 am on December 21, 2018:
    Kinda weird that on non-init it throws out all its work. Why not on non-init, have mixextract still write to out? Then the research can read that in. I feel a little uncomfortable with the brittleness of mixextract having a codepath that just returns without updating out.
    sipa commented at 9:27 am on December 21, 2018:
    Fixed.
  39. sipa force-pushed on Dec 21, 2018
  40. in src/random.cpp:407 in c2f3b3f4a4 outdated 
    435+    static std::atomic<int64_t> last_strengthen;
436+    int64_t current_time = GetTimeMicros();
437+    if (current_time > last_strengthen + 60000) {
438+        // Write the actual RNG state into the hasher, so it goes through the strengthening as well.
439+        unsigned char buf[32];
440+        if (rng.MixExtract(buf, 32, CSHA512(), false)) {
    gmaxwell commented at 1:43 am on December 22, 2018:
    I think this one can ignore the return value of mix extract.
    sipa commented at 10:54 am on December 25, 2018:
    Done.
  41. sipa force-pushed on Dec 23, 2018
  42. gmaxwell commented at 8:00 pm on December 31, 2018: contributor
    utACK
  43. sipa commented at 5:39 pm on January 3, 2019: member
    Anything I can do to simplify review here? I can move the strengthening loop to a separate PR if that helps.
  44. sipa force-pushed on Jan 4, 2019
  45. sipa commented at 11:00 am on January 4, 2019: member
    I’ve removed the hash strengthening from this PR; I’ll open a separate PR for it later.
  46. practicalswift commented at 4:42 pm on January 5, 2019: contributor
    Bonus points for the removal of NO_THREAD_SAFETY_ANALYSIS (for locking_callback) and also the various noexcept additions.
  47. laanwj commented at 4:33 pm on January 8, 2019: member

    Bonus points for the removal of NO_THREAD_SAFETY_ANALYSIS (for locking_callback) and also the various noexcept additions.

    Getting rid of CInit is wonderful too :tada:

    utACK 3a82a46eaa1f1c9e55635f8940a2bda7d3d2a817

  48. instagibbs commented at 3:40 pm on January 9, 2019: member
    @sipa when you say it implements “most of” the issue, which parts are missing?
  49. sipa commented at 4:00 pm on January 9, 2019: member
    @instagibbs There are some potential entropy sources that can still be added (environment data, and a strengthening loop). And of course removing OpenSSL once we have sufficient replacements for all of it.
  50. instagibbs commented at 4:01 pm on January 9, 2019: member
    “Abstract out seeding/extracting entropy into RandExtract.” <— commit message refers to presumably old name of function
  51. in src/random.cpp:299 in 0c442f32d8 outdated 
    291@@ -292,6 +292,32 @@ struct RNGState {
292     {
293         HWRandInit();
294     }
295+
296+    /** Extract up to 32 bytes of entropy from the RNG state, mixing in new entropy from hasher. */
297+    void MixExtract(unsigned char* out, int num, CSHA512&& hasher)
298+    {
299+        assert(num >= 0 && num <= 32);
    instagibbs commented at 4:23 pm on January 9, 2019:
    nit: also assert if out==nullptr that num==0, I believe memcpy to nullptr is technically undefined
    sipa commented at 6:20 pm on January 9, 2019:
    Done (by adding an assert in the if (num) { branch).
  52. in src/random.cpp:297 in 0c442f32d8 outdated 
    291@@ -292,6 +292,32 @@ struct RNGState {
292     {
293         HWRandInit();
294     }
295+
296+    /** Extract up to 32 bytes of entropy from the RNG state, mixing in new entropy from hasher. */
297+    void MixExtract(unsigned char* out, int num, CSHA512&& hasher)
    instagibbs commented at 4:24 pm on January 9, 2019:
    mu-nit: any reason num is a signed integer? could simplify assertion below by being unsigned
    sipa commented at 6:20 pm on January 9, 2019:
    Fixed.
  53. in src/random.cpp:435 in ca3459b669 outdated 
    465-    memory_cleanse(buf, 64);
466+    // For anything but the 'fast' level, feed the resulting RNG output (after an additional hashing step) back into OpenSSL.
467+    if (level != RNGLevel::FAST) {
468+        unsigned char buf[64];
469+        CSHA512().Write(out, num).Finalize(buf);
470+        RAND_add(buf, sizeof(buf), 8 * num);
    instagibbs commented at 4:52 pm on January 9, 2019:

    can there be a description on why 8 * num? Not following this one based on the docs.

    “The entropy argument is (the lower bound of) an estimate of how much randomness is contained in buf, measured in bytes.”

    sipa commented at 5:41 pm on January 9, 2019:

    I misunderstood the API and assumed the entropy argument was measured in bits.

    I don’t think this matters much, modern RNGs don’t really measure entropy anymore. But I’ll fix it.

    sipa commented at 6:20 pm on January 9, 2019:
    Fixed.
  54. instagibbs approved
  55. instagibbs commented at 5:00 pm on January 9, 2019: member
    utACK https://github.com/bitcoin/bitcoin/pull/14955/commits/3a82a46eaa1f1c9e55635f8940a2bda7d3d2a817, though I’d like a “RAND_add” explanation as noted
  56. sipa force-pushed on Jan 9, 2019
  58. laanwj added this to the "Blockers" column in a project
  59. sipa commented at 3:20 am on January 11, 2019: member

    Added two commits:

    • Turn RNGState into a class with private fields (makes it easier to make sure no code can accidentally wipe it or so), suggested by @pstratem on IRC.
    • Allocate the RNGState in the mlocked pool, suggested by @gmaxwell on IRC.
  60. in src/qt/test/paymentservertests.cpp:184 in 5dfb990594 outdated 
    180@@ -181,12 +181,12 @@ void PaymentServerTests::paymentServerTests()
181     QCOMPARE(PaymentServer::verifyExpired(r.paymentRequest.getDetails()), true);
182 
183     // Test BIP70 DoS protection:
184-    unsigned char randData[BIP70_MAX_PAYMENTREQUEST_SIZE + 1];
185-    GetRandBytes(randData, sizeof(randData));
186+    auto randdata = FastRandomContext().randbytes(BIP70_MAX_PAYMENTREQUEST_SIZE + 1);
    ryanofsky commented at 6:55 pm on January 11, 2019:

    In commit “Use FRC::randbytes instead of reading >32 bytes from RNG” (5dfb99059491b1db9133cf0e6b0158ff4bdc9ef0)

    Note: seeding behavior is unchanged here because FastRandomContext just calls GetRandBytes internally.

    sipa commented at 6:21 pm on January 13, 2019:
    Added comment in commit.
  61. in src/random.cpp:173 in ede7e302d5 outdated 
    165@@ -166,13 +166,6 @@ static void RandAddSeedPerfmon()
166     if (ret == ERROR_SUCCESS) {
167         RAND_add(vData.data(), nSize, nSize / 100.0);
168         memory_cleanse(vData.data(), nSize);
169-        LogPrint(BCLog::RAND, "%s: %lu bytes\n", __func__, nSize);
170-    } else {
171-        static bool warned = false; // Warn only once
172-        if (!warned) {
173-            LogPrintf("%s: Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code %i\n", __func__, ret);
    ryanofsky commented at 7:01 pm on January 11, 2019:

    In commit “Don’t log RandAddSeedPerfmon details” (ede7e302d55eb37936268caffcc4b31839e58283)

    I think this code needs a comment saying why it’s safe to ignore the error and skip seeding when the call fails. Just reading the new code, I would be wondering why RandFailure() isn’t called.

    It would also be good to mention loss of the printf warning in release notes.

    sipa commented at 6:22 pm on January 13, 2019:
    Added a comment and a TODO.
  62. in src/random.cpp:451 in 2ce2c82711 outdated 
    440@@ -441,10 +441,6 @@ bool Random_SanityCheck()
441     uint64_t stop = GetPerformanceCounter();
442     if (stop == start) return false;
443 
444-    // We called GetPerformanceCounter. Use it as entropy.
    ryanofsky commented at 7:10 pm on January 11, 2019:

    In commit “Remove adding timestamps in sanity check to OpenSSL RNG” (2ce2c827115748ebea121e6e2abfded260f50ea6)

    What’s the benefit to dropping these calls? Commit message only says why it is not harmful to remove them, not why it is good to remove them.

    sipa commented at 6:22 pm on January 13, 2019:
    They were hard to maintain in an earlier iteration of this patch. I’ve removed the commit now, and kept the functionality in the big switchover commit.
  63. in src/random.h:181 in 2dcb3e938a outdated 
    177@@ -178,7 +178,7 @@ void GetOSRand(unsigned char *ent32);
178  */
179 bool Random_SanityCheck();
180 
181-/** Initialize the RNG. */
182+/** Report information about the RNG used. */
    ryanofsky commented at 7:30 pm on January 11, 2019:

    In commit “Automatically initialize RNG on first use.” (2dcb3e938a210b1bbaa178ad6f9a5d418c4e2939)

    New comment is confusing about when this should be called. Would suggest:

    0/**
1 * Initialize global RNG state and log any CPU features that are used.
2 *
3 * Calling this function is optional. RNG state will be initialized when first
4 * needed if it is not called.
5 */
    sipa commented at 6:22 pm on January 13, 2019:
    Done.
  64. in src/random.cpp:302 in cef6d52cbe outdated 
    297+    void MixExtract(unsigned char* out, size_t num, CSHA512&& hasher)
298+    {
299+        assert(num <= 32);
300+        unsigned char buf[64];
301+        {
302+            WAIT_LOCK(cs, lock);
    ryanofsky commented at 7:40 pm on January 11, 2019:

    In commit “Abstract out seeding/extracting entropy into RNGState::MixExtract” (cef6d52cbed7f30bcdec5b6c5a72e9a49e26eeab)

    This is moved code, but can it just use LOCK instead of WAIT_LOCK?

    sipa commented at 6:22 pm on January 13, 2019:
    Done.
  65. in src/random.cpp:330 in cef6d52cbe outdated 
    291@@ -292,6 +292,33 @@ struct RNGState {
292     {
293         HWRandInit();
294     }
295+
296+    /** Extract up to 32 bytes of entropy from the RNG state, mixing in new entropy from hasher. */
297+    void MixExtract(unsigned char* out, size_t num, CSHA512&& hasher)
298+    {
299+        assert(num <= 32);
300+        unsigned char buf[64];
    ryanofsky commented at 7:46 pm on January 11, 2019:

    In commit “Abstract out seeding/extracting entropy into RNGState::MixExtract” (cef6d52cbed7f30bcdec5b6c5a72e9a49e26eeab)

    It would be good to check the buffer size. Maybe:

    0static_assert(sizeof(buf) == hasher.OUTPUT_SIZE);
    sipa commented at 6:23 pm on January 13, 2019:
    Done.
    sipa commented at 0:46 am on January 14, 2019:
    I had to change it to CSHA512::OUTPUT_SIZE, as clang didn’t agree hasher.OUTPUT_SIZE was a compile-time constant.
  66. ryanofsky commented at 8:03 pm on January 11, 2019: member

    Mostly reviewed this, but need to finish up last few commits. Two suggestions:

    • It would be good to add information from #14623 and #14955 descriptions to a code comment (or maybe a doc/random.md document) to document the new design. It would be a shame if this PR were merged as-is and all the considerations that went into designing it wound up buried in commit history.

    • There should be a release note about this change. Probably not necessary to go into detail about the design, but it would be good to have a short overall description and call out any changes to user-visible behavior like logging.

    My review status:

    • 5dfb99059491b1db9133cf0e6b0158ff4bdc9ef0 Use FRC::randbytes instead of reading >32 bytes from RNG (1/13)
    • ede7e302d55eb37936268caffcc4b31839e58283 Don’t log RandAddSeedPerfmon details (2/13)
    • 2ce2c827115748ebea121e6e2abfded260f50ea6 Remove adding timestamps in sanity check to OpenSSL RNG (3/13)
    • 2dcb3e938a210b1bbaa178ad6f9a5d418c4e2939 Automatically initialize RNG on first use. (4/13)
    • d29f3b967313db206b43c92df93f76cafa0fb843 Add thread safety annotations to RNG state (5/13)
    • cef6d52cbed7f30bcdec5b6c5a72e9a49e26eeab Abstract out seeding/extracting entropy into RNGState::MixExtract (6/13)
    • 7c82fd09b9e7bdf62f8aa4d4d92acf9865a0476b Switch all RNG code to the built-in PRNG. (7/13)
    • d121e29d26836efaad600d5cfb1fcc884c48d17a Remove hwrand_initialized. (8/13)
    • 5749bfea805a1c37b2122ebc42827d4fef1fc25b Sprinkle some sweet noexcepts over the RNG code (9/13)
    • 214fcf143c3f2cafb8d89c551d5de00b5da6ffa5 Use our own locking instead of using OpenSSL in multithreaded mode (10/13)
    • b1ee8608f2b1700505e24f09ee141d4608c36078 DRY: Implement GetRand using FastRandomContext::randrange (11/13)
    • 0259c0bbdd8cb4dd6d924c78279bb9382fbdc445 Encapsulate RNGState better (12/13)
    • 10a00393aecafe42cef65074f04656f2f05e9d13 Use secure allocator for RNG state (13/13)
  67. Use FRC::randbytes instead of reading >32 bytes from RNG 
    There was only one place in the codebase where we're directly reading >32 bytes from
the RNG. One possibility would be to make the built-in RNG support large reads, but
using FastRandomContext lets us reuse code better.

There is no change in behavior here, because the FastRandomContext constructor
uses GetRandBytes internally.
    6a57ca91da
  68. sipa force-pushed on Jan 13, 2019
  69. sipa commented at 6:54 pm on January 13, 2019: member

    @ryanofsky Thanks for the review. I’ve addressed most of your comments, and added a bunch of comments to random.h. I’ll let others comment on the need for adding release notes about it, but my thinking was to only make a note once we fully remove OpenSSL (which may or may not be in the same release).

    I’ve tried to not interfere with existing review too much by keeping the commit order and not rebased, but if desired by reviewers I can probably split up the big “Switch all RNG code to the built-in PRNG.” commit.

  70. sipa force-pushed on Jan 13, 2019
  71. sipa force-pushed on Jan 13, 2019
  72. sipa force-pushed on Jan 13, 2019
  73. laanwj commented at 1:55 pm on January 14, 2019: member

    I’ll let others comment on the need for adding release notes about it, but my thinking was to only make a note once we fully remove OpenSSL (which may or may not be in the same release).

    IMO, a release note is unnecessary for this (apart from the automatically-generated changelog line). It is an internal change, there are hardly or no user-visible changes, the user doesn’t need to do anything like change the configuration. Warning sort of “the random number generator changed, be careful” would be bad signaling (better not upgrade at all then?), we need to be really sure that this is safe before merging.

  74. in src/random.cpp:353 in d76cc6dbb9 outdated 
    322+            assert(out != nullptr);
323+            memcpy(out, buf, num);
324+        }
325+        // Best effort cleanup of internal state
326+        hasher.Reset();
327+        memory_cleanse(buf, 64);
    ryanofsky commented at 8:54 pm on January 14, 2019:

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

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

    sipa commented at 0:57 am on January 16, 2019:

    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?

    Sjors commented at 11:41 am on January 16, 2019:
    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 [] :-)
  75. in src/random.cpp:167 in 85f8f99e0a outdated 
    163@@ -174,7 +164,7 @@ static void RandAddSeedPerfmon()
164     }
165     RegCloseKey(HKEY_PERFORMANCE_DATA);
166     if (ret == ERROR_SUCCESS) {
167-        RAND_add(vData.data(), nSize, nSize / 100.0);
    ryanofsky commented at 9:13 pm on January 14, 2019:

    In commit “Switch all RNG code to the built-in PRNG.” (85f8f99e0ae9d1c98f676ae8722cda4b967dcfb4)

    Commit message could mention now seeding openssl differently than before: no longer here, no longer in cinit, additionally in MixExtract and SeedStartup.

    sipa commented at 0:58 am on January 16, 2019:
    Done.
  76. in src/random.cpp:284 in 85f8f99e0a outdated 
    332+{
333+    int64_t perfcounter = GetPerformanceCounter();
334+    hasher.Write((const unsigned char*)&perfcounter, sizeof(perfcounter));
335+}
336 
337-void RandAddSeedSleep()
    ryanofsky commented at 9:27 pm on January 14, 2019:

    In commit “Switch all RNG code to the built-in PRNG.” (85f8f99e0ae9d1c98f676ae8722cda4b967dcfb4)

    Note: functionality previously in RandAddSeedSleep() is now in SeedSleep().

    sipa commented at 0:58 am on January 16, 2019:
    Done.
  77. in src/random.cpp:319 in 85f8f99e0a outdated 
    385+    // Note that we also commit to a timestamp in the Fast seeder, so we indirectly commit to a
386+    // benchmark of all the entropy gathering sources in this function).
387+    SeedTimestamp(hasher);
388 }
389 
390-void GetStrongRandBytes(unsigned char* out, int num)
    ryanofsky commented at 9:29 pm on January 14, 2019:

    In commit “Switch all RNG code to the built-in PRNG.” (85f8f99e0ae9d1c98f676ae8722cda4b967dcfb4)

    Note: functionality previously in GetStrongRandBytes() is now in SeedSlow().

    sipa commented at 0:58 am on January 16, 2019:
    Done.
  78. in src/random.cpp:514 in 6ff775ff80 outdated 
    440@@ -441,11 +441,11 @@ static void ProcRand(unsigned char* out, int num, RNGLevel level)
441     }
442 }
443 
444-void GetRandBytes(unsigned char* buf, int num) { ProcRand(buf, num, RNGLevel::FAST); }
445-void GetStrongRandBytes(unsigned char* buf, int num) { ProcRand(buf, num, RNGLevel::SLOW); }
446+void GetRandBytes(unsigned char* buf, int num) noexcept { ProcRand(buf, num, RNGLevel::FAST); }
447+void GetStrongRandBytes(unsigned char* buf, int num) noexcept { ProcRand(buf, num, RNGLevel::SLOW); }
    ryanofsky commented at 9:43 pm on January 14, 2019:

    In commit “Sprinkle some sweet noexcepts over the RNG code” (6ff775ff80b090494d115753ddcf488a3cf923a9)

    I don’t know best practices around noexcept, but it seems like there are parts of ProcRand that could throw exceptions. Maybe it would be worth saying in a comment somewhere how noexcept is used in random.cpp/h code.

    sipa commented at 0:58 am on January 16, 2019:
    I’ve added a comment on the use of noexcept and why it’s not used for everything.
  79. in src/util/system.cpp:128 in fa2eabe154 outdated 
    113-    }
114-
115-    ~CInit()
116-    {
117-        // Securely erase the memory used by the PRNG
118-        RAND_cleanup();
    ryanofsky commented at 9:54 pm on January 14, 2019:

    In commit “Use our own locking instead of using OpenSSL in multithreaded mode” (fa2eabe154d00bf6c54f604acbe758186d044639)

    Why is calling RAND_cleanup no longer necessary?

    sipa commented at 0:59 am on January 16, 2019:
    I’ve reverted this change for other reasons. I don’t think RAND_cleanup is particularly important, but let’s keep it for now as long as we’re using the OpenSSL RNG.
  80. in src/util/system.cpp:108 in fa2eabe154 outdated 
    100-public:
101-    CInit()
102-    {
103-        // Init OpenSSL library multithreading support
104-        ppmutexOpenSSL.reset(new CCriticalSection[CRYPTO_num_locks()]);
105-        CRYPTO_set_locking_callback(locking_callback);
    ryanofsky commented at 9:56 pm on January 14, 2019:

    In commit “Use our own locking instead of using OpenSSL in multithreaded mode” (fa2eabe154d00bf6c54f604acbe758186d044639)

    I don’t understand how the two parts of this commit relate:

    • removing CRYPTO_set_locking_callback() call
    • adding cs_openssl mutex

    Is the idea that calls like RAND_add and RAND_bytes are no longer thread safe without set_locking_callback, so now we need to serialize calls with our own mutex? If this is the case, why don’t we need to lock the same mutex in qt payment server code using openssl?

    sipa commented at 1:02 am on January 16, 2019:

    Right, that was the reasoning. I assumed the payment server code was already not multithreaded (because it’s synchronized through the UI), but reading more about OpenSSL’s multithreading design, it seems that X509 processing has a bunch of global state which needs locking, so your comment made me uneasy about this.

    I’ve replaced it with a (less satisfying…) commit that moves the CInit logic to RNGState. This is necessary, as we’re now invoking OpenSSL based RNG code from global constructors, possibly before the CInit object is creator (and RNGState is always created whenever needed, even before global constructors). Perhaps this was a problem in the old code too.

    Sjors commented at 12:09 pm on January 16, 2019:
    Maybe add the above as a comment, so we can consider this once BIP70 goes away.
  81. ryanofsky approved
  82. ryanofsky commented at 10:09 pm on January 14, 2019: member

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

    • 6a57ca91da23c6a5d91399ffc7fc09a99b6d4c76 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)
  83. sipa force-pushed on Jan 16, 2019
  84. sipa force-pushed on Jan 16, 2019
  85. in src/random.h:29 in 3b001b09e8 outdated 
    26+ *   perform 'fast' seeding, consisting of mixing in:
27+ *   - A stack pointer (indirectly committing to calling thread and call stack)
28+ *   - A high-precision timestamp (rdtsc when available, c++ high_resolution_clock otherwise)
29+ *   - Hardware RNG (rdrand) when available.
30+ *   These entropy sources are very fast, and mostly designed to protect against situations
31+ *   where a VM state restore/copy results in multiple systems with the same randomness.
    Sjors commented at 8:52 am on January 16, 2019:

    Does this refer to an accidental scenario, where a user ends up generating the same private keys or accidentally signing with the same nonce? Or also a malicious scenario?

    So do I understand correctly that the reason FastRandomContext needs to fast is (mostly) this, or are there also areas of code that need the additional performance?

    sipa commented at 6:22 pm on January 16, 2019:

    Does this refer to an accidental scenario, where a user ends up generating the same private keys or accidentally signing with the same nonce? Or also a malicious scenario?

    Does it matter whether it can be triggered accidentally or intentionally?

    So do I understand correctly that the reason FastRandomContext needs to fast is (mostly) this, or are there also areas of code that need the additional performance?

    I don’t understand the question or the relation with the code you’re commenting on.

    Sjors commented at 6:46 pm on January 16, 2019:
    I was trying to understand why it needs to be fast. The explanation here suggests it’s mostly about protecting against some sort of VM replay scenario. But in later commits it’s clear that there’s also a performance concern, e.g. the Perfmon can take two seconds.
    sipa commented at 6:51 pm on January 16, 2019:
    Ah, I see. I think you’re looking at it the wrong way. Of course there are many places in the code where we need fast randomness (of various sizes); the issue that the speed concern means we can’t do very good seeding, so we need a separate function for the few places where we really need good randomness (GetStrongRandBytes).
    Sjors commented at 7:05 pm on January 16, 2019:

    I do understand that fast also implies weak. The question is why you would ever accept weak, if you already code from strong. The obvious answer is performance. But the comment here talks about a VM replay scenario.

    But now I see what confused me: “These entropy sources are very fast, and mostly designed to protect against situations where a VM state restore” - the part after “and” doesn’t refer to “fast”; they are an additional benefit of these sources.

    Sjors commented at 7:06 pm on January 16, 2019:
    FastButStillHardToReplayRandomContext :-P
    sipa commented at 7:08 pm on January 16, 2019:

    No, FastRandomContext does not protect against replay. GetRand*() does.

    The constructor of FRC does use GetRandBytes, but once you have a RFC, any replay is going to result in repeated randomness produced by RFC.

    Sjors commented at 7:29 pm on January 16, 2019:
    Ah yes, see #14623 about keeping FastRandomContext available for “non-critical random numbers needed inside tight loops”.
  86. in src/random.cpp:170 in 1a3b26e8e3 outdated 
    171-        static bool warned = false; // Warn only once
172-        if (!warned) {
173-            LogPrintf("%s: Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code %i\n", __func__, ret);
174-            warned = true;
175-        }
176+        // Performance data is only used as a last resort to improve entropy.
    Sjors commented at 9:30 am on January 16, 2019:

    “as a last resort” makes it sound really important, which contradicts “it is isn’t considered critical”.

    Maybe say (in a followup PR):

    0// Performance data provides additional entropy, but failure to read it isn't
1// considered critical, so we don't call RandFailure().
    sipa commented at 6:25 pm on January 16, 2019:

    “as a last resort” makes it sound really important, which contradicts “it is isn’t considered critical”.

    I don’t understand the contradiction.

    We generally assume that the OS RNG is providing us with good randomness. It’s only in highly unlikely (but hard to prevent) scenarios that it fails at that. As a last resort belt-and-suspenders, we also seed entropy from elsewhere. However, that additional entropy (perfmon data here) isn’t necessarily good, or to be relied upon. It’s just there to make an otherwise fatal but unlikely error less dramatic, but it can’t be more than a best effort regardless.

    Sjors commented at 6:53 pm on January 16, 2019:

    The way I read the phrase “as a last resort” is: if there was a way to detect that our RNG attempts thus far have failed, we try this thing as a last resort. In the last resort also fails, that’s critical. Hence the contraction.

    But because we normally can’t detect if we actually needed the last resort, we don’t know if the failure was critical, and most likely it wasn’t.

    sipa commented at 0:00 am on January 17, 2019:
    Reformulated the text a bit.
  87. in src/random.cpp:171 in 1a3b26e8e3 outdated 
    172-        if (!warned) {
173-            LogPrintf("%s: Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code %i\n", __func__, ret);
174-            warned = true;
175-        }
176+        // Performance data is only used as a last resort to improve entropy.
177+        // Failure to read it is isn't conidered critical, so we don't call
    Sjors commented at 9:30 am on January 16, 2019:
    Typo “conidered” (but see above)
    sipa commented at 0:00 am on January 17, 2019:
    Fixed.
  88. in src/random.cpp:90 in 2a02d2c369 outdated 
    88         rdrand_supported = true;
89     }
90     hwrand_initialized.store(true);
91 }
92+
93+static void HWRandReport()
    Sjors commented at 10:05 am on January 16, 2019:

    “and” is ambiguous, is it part of “Rand” or does the function name mean “(init?) HardWare Random (number generator) and Report”?

    Suggestion: ReportHardwareRNG()

    sipa commented at 6:26 pm on January 16, 2019:
    You’re nitpicking. Nowhere do we use a naming style where a lowercase character after an uppercase one starts a new word.
    Sjors commented at 6:56 pm on January 16, 2019:
    I spent at least half an hour being confused about what HWRandInit and HWRandReport were doing, otherwise I wouldn’t have brought up this up.
  89. in src/random.cpp:92 in 2a02d2c369 outdated 
    90     hwrand_initialized.store(true);
91 }
92+
93+static void HWRandReport()
94+{
95+    assert(hwrand_initialized.load(std::memory_order_relaxed));
    Sjors commented at 10:29 am on January 16, 2019:

    Suggested comment above (for those not intimately familiar with std::atomic, who might initially think load() is some sort of initialisation performing side-effect):

    0// Ensure `HWRandInit()` was called first

    Then again, it goes away in a later commit.

    sipa commented at 6:27 pm on January 16, 2019:
    Seriously, read the documentation if this isn’t clear.
  90. in src/random.cpp:81 in 2a02d2c369 outdated 
    78 #if defined(__x86_64__) || defined(__amd64__) || defined(__i386__)
79 static std::atomic<bool> hwrand_initialized{false};
80 static bool rdrand_supported = false;
81 static constexpr uint32_t CPUID_F1_ECX_RDRAND = 0x40000000;
82-static void RDRandInit()
83+static void HWRandInit()
    Sjors commented at 10:31 am on January 16, 2019:
    Add comment that this currently looks for rdrand support in the CPU. Suggested name: InitHardwareRNG()
    sipa commented at 0:01 am on January 17, 2019:
    Done, added a separate commit with renames and the comments on HWRNG performance you suggested elsewhere.
  91. in src/random.cpp:101 in 2a02d2c369 outdated 
    100+    }
101+}
102+
103 #else
104-static void RDRandInit() {}
105+static void HWRandInit() {}
    Sjors commented at 10:38 am on January 16, 2019:
    Maybe clarify constraints that would apply to future hardware RNGs. E.g. is this an appropriate place to interface with a USB based RNG? Or does it have to be in the CPU for availability and latency reasons? Is it OK to use a lower quality CPU RNG for this, because it’s strictly additional entropy?
    sipa commented at 0:01 am on January 17, 2019:
    Added a comment.
  92. in src/random.cpp:294 in 2a02d2c369 outdated 
    287@@ -278,6 +288,26 @@ void GetRandBytes(unsigned char* buf, int num)
288     }
289 }
290 
291+namespace {
292+struct RNGState {
293+    Mutex cs;
294+    unsigned char state[32] = {0};
    Sjors commented at 10:42 am on January 16, 2019:

    Maybe clarify why you chose 32 back in #10338, assuming that reasoning still applies:

    0// As we're already using SHA512 to combine the multiple entropy sources,
1// which produces 64 bytes, we can use the last 32 of those as an additional
2// input for the next call. This makes sure that the produced data is secure
3// as long as any entropy source in the past was reliable.
    sipa commented at 0:01 am on January 17, 2019:
    Added a similar comment to the RNGState::m_state variable.
  93. in src/random.h:235 in 2a02d2c369 outdated 
    177@@ -178,7 +178,12 @@ void GetOSRand(unsigned char *ent32);
178  */
179 bool Random_SanityCheck();
180 
181-/** Initialize the RNG. */
182+/**
183+ * Initialize global RNG state and log any CPU features that are used.
184+ *
185+ * Calling this function is optional. RNG state will be initialized when first
186+ * needed if it is not called.
    Sjors commented at 10:58 am on January 16, 2019:
    Maybe add comment that rng.state is initialised to zeros, i.e. initialisation just refers to the C++ action, not to the broader sense of the word (which might include seeding).
    sipa commented at 6:31 pm on January 16, 2019:
    No, it does refer to the broader sense, including seeding.
  94. in src/random.cpp:309 in d76cc6dbb9 outdated 
    304+    {
305+        assert(num <= 32);
306+        unsigned char buf[64];
307+        static_assert(sizeof(buf) == CSHA512::OUTPUT_SIZE, "Buffer needs to have hasher's output size");
308+        {
309+            LOCK(cs);
    Sjors commented at 11:29 am on January 16, 2019:
    Thread safety n00b question: what did WAIT_LOCK do and why is LOCK fine here?
    sipa commented at 6:31 pm on January 16, 2019:
    The only difference is that it lets you specify the name of the lock variable.
  95. in src/random.cpp:312 in c6fb96c876 outdated 
    306@@ -331,14 +307,19 @@ struct RNGState {
307         CRYPTO_set_locking_callback(nullptr);
308     }
309 
310-    /** Extract up to 32 bytes of entropy from the RNG state, mixing in new entropy from hasher. */
311-    void MixExtract(unsigned char* out, size_t num, CSHA512&& hasher)
312+    /** Extract up to 32 bytes of entropy from the RNG state, mixing in new entropy from hasher.
313+     *
314+     * If this function has never been called with full_init = true, false is returned.
    Sjors commented at 12:23 pm on January 16, 2019:
    Can you explain why this function must be called at least twice before it’s OK to use?
    sipa commented at 6:28 pm on January 16, 2019:

    It doesn’t?

    We just want to make sure it’s at least once initialized with sufficient entropy before the caller (ProcRand) wants to use its output.

  96. in src/random.cpp:283 in c6fb96c876 outdated 
    279@@ -297,6 +280,7 @@ struct RNGState {
280     Mutex cs;
281     unsigned char state[32] GUARDED_BY(cs) = {0};
282     uint64_t counter GUARDED_BY(cs) = 0;
283+    bool initialized GUARDED_BY(cs) = false;
    Sjors commented at 12:28 pm on January 16, 2019:
    This might lead to confusion with the other meaning of initialized that’s referred to in “Initialize global RNG state”.
    sipa commented at 0:02 am on January 17, 2019:
    Renamed to m_strongly_seeded.
  97. in src/random.cpp:453 in c6fb96c876 outdated 
    449+    SeedTimestamp(hasher);
450 
451-    // Third source: HW RNG, if available.
452-    if (GetHWRand(buf)) {
453-        hasher.Write(buf, 32);
454+    // Windows performance monitor data (once every 10 minutes)
    Sjors commented at 12:59 pm on January 16, 2019:
    “every 10 minutes” includes the first usage, right?
    sipa commented at 6:28 pm on January 16, 2019:
    Yes, but not through this code path.
    sipa commented at 0:02 am on January 17, 2019:
    Oh, I see the confusion. Removed that phrasing.
  98. in src/random.cpp:409 in c6fb96c876 outdated 
    383+    hasher.Write((const unsigned char*)&ptr, sizeof(ptr));
384 
385-    // Combine with and update state
386-    AddDataToRng(&nPerfCounter1, sizeof(nPerfCounter1), rng);
387-    AddDataToRng(&nPerfCounter2, sizeof(nPerfCounter2), rng);
388+    // Hardware randomness is very fast when available; use it always.
    Sjors commented at 1:03 pm on January 16, 2019:
    Assuming it’s always CPU based and not same external device, so this assumption should be stated in the empty / placeholder HWRandInit implementations for other architectures.
    sipa commented at 0:02 am on January 17, 2019:
    Done.
  99. Sjors approved
  100. Sjors commented at 1:18 pm on January 16, 2019: member

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

    • 6a57ca91da23c6a5d91399ffc7fc09a99b6d4c76 “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).

  101. sipa commented at 6:19 pm on January 16, 2019: member

    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.

  102. in src/random.cpp:293 in 2a02d2c369 outdated 
    287@@ -278,6 +288,26 @@ void GetRandBytes(unsigned char* buf, int num)
288     }
289 }
290 
291+namespace {
292+struct RNGState {
293+    Mutex cs;
    ryanofsky commented at 9:32 pm on January 16, 2019:

    In commit “Automatically initialize RNG on first use.” (2a02d2c369ee06cbc334a0dd7150ec72ef83a182)

    Ignore if this is a pain, or if there is nostalgia for critical sections, but you might want to use m_mutex instead of cs here, and m_openssl_mutex instead of cs_openssl in 772fce745c51aa9d09f93e13eeb822e889ae25f7

    sipa commented at 0:03 am on January 17, 2019:
    Done.
  103. in src/random.cpp:341 in bfd3a23f27 outdated 
    336@@ -336,6 +337,8 @@ struct RNGState {
337         memory_cleanse(buf, 64);
338         return ret;
339     }
340+
341+    Mutex& GetOpenSSLLock(int i) { return cs_openssl[i]; }
    ryanofsky commented at 10:02 pm on January 16, 2019:

    In commit “Encapsulate RNGState better” (bfd3a23f277c16236ba6ea7391a98277226c1470)

    Seems more accurate to call it Mutex than Lock (as in 7939daa351a31a36f2086771611b501418536989), if Mutex is the object that gets locked, and Lock is the object that locks it.

    sipa commented at 0:03 am on January 17, 2019:
    Done.
  104. ryanofsky approved
  105. ryanofsky commented at 10:05 pm on January 16, 2019: member
    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.
  106. 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.
    2d1cc50939
  107. Automatically initialize RNG on first use. 05fde14e3a
  108. Rename some hardware RNG related functions d3f54d1c82
  109. sipa force-pushed on Jan 17, 2019
  110. Add thread safety annotations to RNG state aae8b9bf0f
  111. Abstract out seeding/extracting entropy into RNGState::MixExtract 2ccc3d3aa3
  112. 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.
    16e40a8b56
  113. 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().
    9d7032e4f0
  114. Remove hwrand_initialized. 
     All access to hwrand is now gated by GetRNGState, which initializes the hwrand code.
    4ea8e50837
  115. Sprinkle some sweet noexcepts over the RNG code a1f252eda8
  116. DRY: Implement GetRand using FastRandomContext::randrange 152146e782
  117. Encapsulate RNGState better cddb31bb0a
  118. Use secure allocator for RNG state f2e60ca985
  119. Document RNG design in random.h 223de8d94d
  120. sipa force-pushed on Jan 17, 2019
  121. Sjors commented at 12:16 pm on January 17, 2019: member
    Wonderful, re-tACK 223de8d
  122. sipa commented at 7:05 pm on January 17, 2019: member
    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.
  123. ryanofsky approved
  124. ryanofsky commented at 6:04 pm on January 18, 2019: member
    utACK 223de8d94d6522f795ec3c2e7db27469f24aa68c. All changes since last review are renames or comment improvements.
  125. laanwj merged this on Jan 21, 2019
  126. laanwj closed this on Jan 21, 2019
  127. laanwj referenced this in commit 6e6b3b944d on Jan 21, 2019
  128. fanquake removed this from the "Blockers" column in a project
  129. random-zebra referenced this in commit 93f43f0f81 on Apr 14, 2021
  130. dzutto referenced this in commit 4cbecd4510 on Sep 10, 2021
  131. UdjinM6 referenced this in commit be79070f60 on Sep 11, 2021
  132. MarcoFalke locked this on Dec 16, 2021


sipa fanquake gmaxwell DrahtBot practicalswift ken2812221 laanwj instagibbs ryanofsky Sjors

Labels
Refactoring Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me