Move static global randomizer seeds into CConnman #8688

pull sipa wants to merge 1 commits into bitcoin:master from sipa:detrandconnman changing 7 files +34 −24

sipa commented at 10:51 AM on September 9, 2016: member

This converts two scattered static local variables (which are really just globals) that influence randomization for the P2P network into a CConnman field, and make it accessible after providing an id for the randomizer.
fanquake added the label Refactoring on Sep 9, 2016
theuni commented at 7:48 PM on September 9, 2016: member

Concept ACK, will review in detail.

I'd really prefer to pass in the seed, though. I'm hoping to avoid adding external influence back in as much as possible.
sipa commented at 7:50 PM on September 9, 2016: member

You mean passing the two 64-bit integers in to the CConnman constructor? That sounds fine to me.
theuni commented at 8:01 PM on September 9, 2016: member

Exactly, thanks.
in src/main.cpp:None in d92409cdbf outdated
```
 112 | @@ -113,6 +113,8 @@ CScript COINBASE_FLAGS;
 113 |  
 114 |  const string strMessageMagic = "Bitcoin Signed Message:\n";
 115 |  
 116 | +static const uint64_t RANDOMIZER_ID_ADDRESS_RELAY = 0x3cac0035b5866b90ULL;
```
theuni commented at 8:11 PM on September 9, 2016:

Anything up your sleeve? :)

sipa commented at 8:12 PM on September 9, 2016:

Yes, SHA256("main address relay").

theuni commented at 8:15 PM on September 9, 2016:

Heh, I was way off. Comment for those, please.
theuni commented at 8:15 PM on September 9, 2016: member

utACK with those things addressed.
Move static global randomizer seeds into CConnman d9ff591d42
in src/net.h:None in d92409cdbf outdated
```
 294 | @@ -294,6 +295,8 @@ class CConnman
 295 |      void SetBestHeight(int height);
 296 |      int GetBestHeight() const;
 297 |  
 298 | +    /** Get a unique deterministic randomizer. */
 299 | +    CSipHasher GetDeterministicRandomizer(uint64_t id);
```
laanwj commented at 8:40 AM on September 14, 2016:

Is this something that belongs on CConnman? Conceptually it seems a bit odd to have randomization on the network manager. E.g. Could there be reasons to have deterministic randomizers that don't have (directly) to do with networking? Not sure. Maybe for randomizing data structures such as caches?

sipa commented at 11:41 AM on September 19, 2016:

Well it's only used for randomization that is specific to networking. The chainstate cache for example also has a randomized, and that one is independent.

I expect longer term that there will be something like network event/message handlers, and the ProcessMessage code in main now will move to such a handler that registers itself with a connman. The randomizer code could then go into the specific network handler instance, instead of with connman itself.

laanwj commented at 1:34 PM on September 19, 2016:

Fair enough
sipa force-pushed on Sep 19, 2016
sipa commented at 1:55 PM on September 19, 2016: member

Updated to move the randomizer seeds to the constructor arguments.
laanwj approved
laanwj commented at 4:01 PM on September 19, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/8688/commits/d9ff591d42158e8a0a4ebdcf5fbb74978c483202
laanwj merged this on Sep 19, 2016
laanwj closed this on Sep 19, 2016
laanwj referenced this in commit 047ded0b12 on Sep 19, 2016
codablock referenced this in commit 31bd2fc1cf on Sep 19, 2017
codablock referenced this in commit b04e32c43d on Jan 11, 2018
andvgal referenced this in commit 474ed8ab90 on Jan 6, 2019
random-zebra referenced this in commit cbd9271afb on Sep 7, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels