Kill insecure_random and associated global state #8914

laanwj · 2016-10-13T16:35:48Z

There are only a few uses of insecure_random outside the tests. This PR replaces uses of insecure_random (and its accompanying global state) in the core code with an FastRandomContext that is automatically seeded on creation.

This is meant to be used for inner loops. The FastRandomContext can be in the outer scope, or the class itself, then rand32() is used inside the loop. Useful e.g. for pushing addresses in CNode or the fee
rounding, or randomization for coin selection.

As a context is created per purpose, thus it gets rid of cross-thread unprotected shared usage of a single set of globals, this should also get rid of the potential race conditions.

I'd say TxMempool::check is not called enough to warrant using a special fast random context, this is switched to GetRand() (open for discussion...)
The use of insecure_rand in ConnectThroughProxy has been replaced by an atomic integer counter. The only goal here is to have a different credentials pair for each connection to go on a different Tor circuit, it does not need to be random nor unpredictable.
To avoid having a FastRandomContext on every CNode, the context is passed into PushAddress as appropriate.

There remains an insecure_random for test usage in test_random.h.

Replaces #8903. Intends to fix @JeremyRubin's concerns about race conditions.

TODO

Rename instances to insecure_rand i.s.o rand.

maflcko · 2016-10-13T16:57:37Z

src/wallet/wallet.cpp

I'd prefer to keep the name at FastRandomContext insecure_rand, especially in the wallet code.

Agreed. Read the patch from the bottom up and went "huh? the whole reason we had this function was to use it in the subset sum solver!" doh. :)

Yes, makes sense. It didn't become any less insecure, but at least there's no magic global state anymore.

gmaxwell · 2016-10-14T09:51:28Z

Concept ACK.

JeremyRubin · 2016-10-14T18:11:42Z

src/random.h

It may be nice to throw in a:

FastRandomContext() = delete;

just for clarify where the syntax

FastRandomContext rand;

is used.

Alternatively, because we know fDeterministic everywhere it is used, could also be made a template parameter to this class (which has the added benefit of a callee being able to specify what kind of insecure_rand they are getting...).

just for clarify where the syntax FastRandomContext rand;

If you want to find those, isn't removing the default argument =false enough (and maybe making the constructor explicit)?
I think the default (non-deterministic) makes sense though - only the tests have to override the parameter.

could also be made a template parameter to this class

The problem is that the tests switch from/to deterministic with the same object, so the determinism flag needs to be overwritable as part of the state.

I think explicit constructors would be good. I think the current behavior (this is a change from prior specs) under c++11 is that the constructor is considered a converting constructor, meaning "FastRandomContext rand = 0" is valid I think?

Perhaps a more idiomatic version of this would be to make FastRandomContext always deterministic, and then add a "non_deterministic_reseed" method to it. This has the downside that the non-test use case always uses the non_det version, so maybe making it always non_det with a method to set a deterministic seed?

Ultimately, it's a minor nit I just think making it more clear how those objects are initialized is good.

JeremyRubin · 2016-10-14T18:25:55Z

src/netbase.cpp

nit: technically, you can get away with

counter.fetch_add(1, std::memory_order_relaxed)

if it is any concern.

Thanks. Though this is only called one time per new connection, so I don't think performance would be enough reason to make this more verbose. This could even just use a simple counter protected by a lock, but c++11 makes it so easy to use atomics :)

JeremyRubin · 2016-10-14T18:26:52Z

Seems to address what I was concerned about and is cleaner than the global state, looks good!

Concept ACK.

jonasschnelli · 2016-10-17T06:39:27Z

Concept ACK, agree with using FastRandomContext insecure_rand.

laanwj · 2016-10-17T10:44:27Z

Ok:

renamed all instances to insecure_rand
FastRandomContext constructor is now explicit
And squashed

paveljanik · 2016-10-17T10:47:19Z

src/net.cpp

rand -> insecure_rand for consistency?

gah did I forget one, good catch

@paveljanik there was another one in fees.h. Should have them all now (checked through grep)

There are only a few uses of `insecure_random` outside the tests. This PR replaces uses of insecure_random (and its accompanying global state) in the core code with an FastRandomContext that is automatically seeded on creation. This is meant to be used for inner loops. The FastRandomContext can be in the outer scope, or the class itself, then rand32() is used inside the loop. Useful e.g. for pushing addresses in CNode or the fee rounding, or randomization for coin selection. As a context is created per purpose, thus it gets rid of cross-thread unprotected shared usage of a single set of globals, this should also get rid of the potential race conditions. - I'd say TxMempool::check is not called enough to warrant using a special fast random context, this is switched to GetRand() (open for discussion...) - The use of `insecure_rand` in ConnectThroughProxy has been replaced by an atomic integer counter. The only goal here is to have a different credentials pair for each connection to go on a different Tor circuit, it does not need to be random nor unpredictable. - To avoid having a FastRandomContext on every CNode, the context is passed into PushAddress as appropriate. There remains an insecure_random for test usage in `test_random.h`.

paveljanik

ACK 5eaaa83

sipa · 2016-10-17T12:28:48Z

src/test/scheduler_tests.cpp

    boost::mutex counterMutex[10];
    int counter[10] = { 0 };
-    boost::random::mt19937 rng(insecure_rand());
+    boost::random::mt19937 rng(42);


Good choice. Chosen by a fair dice roll, I assume?

Hah. It's the result of a certain long-running computation to answer a question that I can't quite remember anymore but seemed ultimately important.

5eaaa83 Kill insecure_random and associated global state (Wladimir J. van der Laan)

Micro-benchmarking framework part 1 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6733 - bitcoin/bitcoin#6770 - bitcoin/bitcoin#6892 - Excluding changes to `src/policy/policy.h` which we don't have yet. - bitcoin/bitcoin#7934 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#8039 - bitcoin/bitcoin#8107 - bitcoin/bitcoin#8115 - bitcoin/bitcoin#8914 - Required resolving several merge conflicts in code that had been refactored upstream. The changes were simple enough that I decided it was okay to impose merge conflicts on pulling in those refactors later. - bitcoin/bitcoin#9200 - bitcoin/bitcoin#9202 - Adds support for measuring CPU cycles, which is later removed in an upstream PR after the refactor. I am including it to reduce future merge conflicts. - bitcoin/bitcoin#9281 - Only changes to `src/bench/bench.cpp` - bitcoin/bitcoin#9498 - bitcoin/bitcoin#9712 - bitcoin/bitcoin#9547 - bitcoin/bitcoin#9505 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#9792 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#10272 - bitcoin/bitcoin#10395 - Only changes to `src/bench/` - bitcoin/bitcoin#10735 - Only changes to `src/bench/base58.cpp` - bitcoin/bitcoin#10963 - bitcoin/bitcoin#11303 - Only the benchmark backend change. - bitcoin/bitcoin#11562 - bitcoin/bitcoin#11646 - bitcoin/bitcoin#11654 This pulls in all changes to the micro-benchmark framework prior to December 2017, when it was rewritten. The rewrite depends on other upstream PRs we have not pulled in yet. This does not pull in all benchmarks prior to December 2017. It leaves out benchmarks that either test code we do not have yet (except for the `FastRandomContext` refactor, which I decided to pull in), or would require rewrites to work with our changes to the codebase.

b886a4c Fix header guards using reserved identifiers (Dan Raviv) b389b3f speed up Unserialize_impl for prevector (Akio Nakamura) 13d2102 Minimal fix to slow prevector tests as stopgap measure (Jeremy Rubin) Pull request description: Backports bitcoin#12324. The `DeserializeAndCheckBlock` benchmark (introduced in #2146) shows a speedup of about 4% (not as much as the upstream PR, due to the optimizations already included in #2083). Cherry-picks also - bitcoin#8671 (with minimal changes to the random context, due to bitcoin#8914 and bitcoin#9792 being already ported out of order). - bitcoin#11151 ACKs for top commit: Fuzzbawls: utACK b886a4c furszy: utACK b886a4c and merging.. Tree-SHA512: f5de40e5acfb0b875d413d8995d71dd90489730fe4853f0be03d76a1c44ec95eaeb28c0c40d8e91906f23529ad26501bda4f9779ce466cd8603ed97f1662ca98

laanwj added the Refactoring label Oct 13, 2016

maflcko reviewed Oct 13, 2016

View reviewed changes

JeremyRubin reviewed Oct 14, 2016

View reviewed changes

laanwj mentioned this pull request Oct 15, 2016

Seed insecure_rand during start #8903

Closed

laanwj force-pushed the 2016_10_kill_insecurerandom branch from 7d507ca to 1c8f590 Compare October 17, 2016 10:44

paveljanik reviewed Oct 17, 2016

View reviewed changes

laanwj force-pushed the 2016_10_kill_insecurerandom branch from 1c8f590 to 5eaaa83 Compare October 17, 2016 11:08

paveljanik approved these changes Oct 17, 2016

View reviewed changes

sipa reviewed Oct 17, 2016

View reviewed changes

laanwj merged commit 5eaaa83 into bitcoin:master Oct 18, 2016

laanwj added a commit that referenced this pull request Oct 18, 2016

Merge #8914: Kill insecure_random and associated global state

cdfb775

5eaaa83 Kill insecure_random and associated global state (Wladimir J. van der Laan)

sipa mentioned this pull request Nov 3, 2016

Do not fully sort all nodes for addr relay #8690

Merged

maflcko mentioned this pull request Nov 7, 2016

test: Fix test_random includes #9095

Merged

sickpig mentioned this pull request Oct 1, 2017

dbcache phase 3 enhancements BitcoinUnlimited/BitcoinUnlimited#785

Merged

codablock pushed a commit to codablock/dash that referenced this pull request Jan 12, 2018

Merge bitcoin#8914: Kill insecure_random and associated global state

239ce53

5eaaa83 Kill insecure_random and associated global state (Wladimir J. van der Laan)

dagurval mentioned this pull request Jan 23, 2018

Kill insecure_random and associated global state bitcoinxt/bitcoinxt#308

Merged

sickpig mentioned this pull request Mar 12, 2018

Ports of Core's PR 7888, 8166, 8914 BitcoinUnlimited/BitcoinUnlimited#1004

Merged

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#8914: Kill insecure_random and associated global state

03e5273

5eaaa83 Kill insecure_random and associated global state (Wladimir J. van der Laan)

str4d mentioned this pull request Feb 22, 2019

Micro-benchmarking framework part 1 zcash/zcash#3858

Merged

Warrows mentioned this pull request Jun 30, 2019

Bring PIVX random on par with bitcoin PIVX-Project/PIVX#799

Closed

42 tasks

random-zebra mentioned this pull request Jan 19, 2021

[Core] Prevector Optimizations 2 PIVX-Project/PIVX#2148

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Kill insecure_random and associated global state #8914

Kill insecure_random and associated global state #8914

Uh oh!

Conversation

laanwj commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Oct 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Oct 14, 2016

Uh oh!

jonasschnelli commented Oct 17, 2016

Uh oh!

laanwj commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveljanik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

laanwj commented Oct 13, 2016 •

edited

Loading

laanwj Oct 13, 2016 •

edited

Loading

laanwj Oct 15, 2016 •

edited

Loading

laanwj commented Oct 17, 2016 •

edited

Loading