Kill insecure_random and associated global state #8914

laanwj commented at 4:35 PM on October 13, 2016: member

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.

laanwj added the label Refactoring on Oct 13, 2016

in src/wallet/wallet.cpp:None in 7d507ca73b outdated

1906 | @@ -1907,7 +1907,7 @@ static void ApproximateBestSubset(vector<pair<CAmount, pair<const CWalletTx*,uns
1907 |      vfBest.assign(vValue.size(), true);
1908 |      nBest = nTotalLower;
1909 |  
1910 | -    seed_insecure_rand();
1911 | +    FastRandomContext rand;

MarcoFalke commented at 4:57 PM on October 13, 2016:

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

gmaxwell commented at 5:04 PM on October 13, 2016:

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

laanwj commented at 5:08 PM on October 13, 2016:

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

gmaxwell commented at 9:51 AM on October 14, 2016: contributor

Concept ACK.

in src/random.h:None in 7d507ca73b outdated

  51 | -    insecure_rand_Rw = 18000 * (insecure_rand_Rw & 65535) + (insecure_rand_Rw >> 16);
  52 | -    return (insecure_rand_Rw << 16) + insecure_rand_Rz;
  53 | -}
  54 | +class FastRandomContext {
  55 | +public:
  56 | +    FastRandomContext(bool fDeterministic=false);

JeremyRubin commented at 6:11 PM on October 14, 2016:

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

laanwj commented at 7:23 AM on October 15, 2016:

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.

JeremyRubin commented at 2:53 AM on October 17, 2016:

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.

in src/netbase.cpp:None in 7d507ca73b outdated

 595 | @@ -596,8 +596,8 @@ static bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDe
 596 |      // do socks negotiation
 597 |      if (proxy.randomize_credentials) {
 598 |          ProxyCredentials random_auth;
 599 | -        random_auth.username = strprintf("%i", insecure_rand());
 600 | -        random_auth.password = strprintf("%i", insecure_rand());
 601 | +        static std::atomic_int counter;
 602 | +        random_auth.username = random_auth.password = strprintf("%i", counter++);

JeremyRubin commented at 6:25 PM on October 14, 2016:

nit: technically, you can get away with

counter.fetch_add(1, std::memory_order_relaxed)

if it is any concern.

laanwj commented at 7:24 AM on October 15, 2016:

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 commented at 6:26 PM on October 14, 2016: contributor

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

Concept ACK.

jonasschnelli commented at 6:39 AM on October 17, 2016: contributor

Concept ACK, agree with using FastRandomContext insecure_rand.

laanwj force-pushed on Oct 17, 2016

laanwj commented at 10:44 AM on October 17, 2016: member

Ok:

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

Kill insecure_random and associated global state

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

5eaaa83ac1

in src/net.cpp:None in 1c8f590063 outdated

 186 | @@ -187,7 +187,8 @@ void AdvertiseLocal(CNode *pnode)
 187 |          if (addrLocal.IsRoutable())
 188 |          {
 189 |              LogPrint("net", "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
 190 | -            pnode->PushAddress(addrLocal);
 191 | +            FastRandomContext rand;

paveljanik commented at 10:47 AM on October 17, 2016:

rand -> insecure_rand for consistency?

laanwj commented at 11:01 AM on October 17, 2016:

gah did I forget one, good catch

laanwj commented at 11:09 AM on October 17, 2016:

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

laanwj force-pushed on Oct 17, 2016

paveljanik approved

paveljanik commented at 11:22 AM on October 17, 2016: contributor

ACK https://github.com/bitcoin/bitcoin/pull/8914/commits/5eaaa83ac1f5eb525f93e2808fafd73f5ed97013

in src/test/scheduler_tests.cpp:None in 5eaaa83ac1

  55 | @@ -58,7 +56,7 @@ BOOST_AUTO_TEST_CASE(manythreads)
  56 |  
  57 |      boost::mutex counterMutex[10];
  58 |      int counter[10] = { 0 };
  59 | -    boost::random::mt19937 rng(insecure_rand());
  60 | +    boost::random::mt19937 rng(42);

sipa commented at 12:28 PM on October 17, 2016:

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

laanwj commented at 1:56 PM on October 17, 2016:

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.

laanwj merged this on Oct 18, 2016

laanwj closed this on Oct 18, 2016

laanwj referenced this in commit cdfb7755a6 on Oct 18, 2016

codablock referenced this in commit 0f26388064 on Sep 19, 2017

codablock referenced this in commit 239ce534c2 on Jan 12, 2018

andvgal referenced this in commit 03e5273a17 on Jan 6, 2019

zkbot referenced this in commit aa225ebb0b on Jan 24, 2020

zkbot referenced this in commit 74ff73abab on Jan 24, 2020

furszy referenced this in commit 07b88da888 on Jan 25, 2021

DrahtBot locked this on Sep 8, 2021