refactor(bootstrap): Use std::shuffle instead of custom not fully random logic

Simplifies logic and naming.

src/core/core.cpp

@@ -42,6 +42,7 @@
 #include <QStringBuilder>
 #include <QTimer>
 
+#include <algorithm>
 #include <cassert>
 #include <chrono>
 #include <memory>
@@ -473,13 +474,21 @@ bool parseErr(Tox_Err_Conference_Delete error, int line)
     }
 }
 
+QList<DhtServer> shuffleBootstrapNodes(QList<DhtServer> bootstrapNodes)
+{
+    std::mt19937 rng(std::chrono::high_resolution_clock::now().time_since_epoch().count());
+    std::shuffle(bootstrapNodes.begin(), bootstrapNodes.end(), rng);
+    return bootstrapNodes;
+}
+
 } // namespace
 
-Core::Core(QThread* coreThread, IBootstrapListGenerator& _bootstrapNodes)
+Core::Core(QThread* coreThread, IBootstrapListGenerator& _bootstrapListGenerator, const ICoreSettings& _settings)
     : tox(nullptr)
     , toxTimer{new QTimer{this}}
     , coreThread(coreThread)
-    , bootstrapNodes(_bootstrapNodes)
+    , bootstrapListGenerator(_bootstrapListGenerator)
+    , settings(_settings)
 {
     assert(toxTimer);
     toxTimer->setSingleShot(true);
@@ -527,7 +536,7 @@ void Core::registerCallbacks(Tox* tox)
  * @param settings Settings specific to Core
  * @return nullptr or a Core object ready to start
  */
-ToxCorePtr Core::makeToxCore(const QByteArray& savedata, const ICoreSettings* const settings,
+ToxCorePtr Core::makeToxCore(const QByteArray& savedata, const ICoreSettings& settings,
                              IBootstrapListGenerator& bootstrapNodes, ToxCoreErrors* err)
 {
     QThread* thread = new QThread();
@@ -546,7 +555,7 @@ ToxCorePtr Core::makeToxCore(const QByteArray& savedata, const ICoreSettings* co
         return {};
     }
 
-    ToxCorePtr core(new Core(thread, bootstrapNodes));
+    ToxCorePtr core(new Core(thread, bootstrapNodes, settings));
     if (core == nullptr) {
         if (err) {
             *err = ToxCoreErrors::ERROR_ALLOC;
@@ -813,31 +822,30 @@ void Core::bootstrapDht()
 {
     ASSERT_CORE_THREAD;
 
-    QList<DhtServer> bootstrapNodesList = bootstrapNodes.getBootstrapnodes();
 
-    int listSize = bootstrapNodesList.size();
-    if (!listSize) {
+    auto const shuffledBootstrapNodes = shuffleBootstrapNodes(bootstrapListGenerator.getBootstrapnodes());
+    if (shuffledBootstrapNodes.empty()) {
         qWarning() << "No bootstrap node list";
         return;
     }
 
-    int i = 0;
-    std::mt19937 rng(std::chrono::high_resolution_clock::now().time_since_epoch().count());
-    std::uniform_int_distribution<int> distribution(0, listSize - 1);
-    static int j = distribution(rng);
     // i think the more we bootstrap, the more we jitter because the more we overwrite nodes
-    while (i < 2) {
-        const DhtServer& dhtServer = bootstrapNodesList[j % listSize];
-        qDebug("Connecting to bootstrap node %d", j % listSize);
-
+    auto numNewNodes = 2;
+    for (int i = 0; i < numNewNodes && i < shuffledBootstrapNodes.size(); ++i) {
+        const auto& dhtServer = shuffledBootstrapNodes.at(i);
         QByteArray address;
-        if (dhtServer.ipv4.isEmpty() && !dhtServer.ipv6.isEmpty()) {
+        if (!dhtServer.ipv4.isEmpty()) {
+            address = dhtServer.ipv4.toLatin1();
+        } else if (!dhtServer.ipv6.isEmpty() && settings.getEnableIPv6()) {
             address = dhtServer.ipv6.toLatin1();
         } else {
-            address = dhtServer.ipv4.toLatin1();
+            ++numNewNodes;
+            continue;
         }
 
-        const uint8_t* pkPtr = dhtServer.publicKey.getData();
+        ToxPk pk{dhtServer.publicKey};
+        qDebug() << "Connecting to bootstrap node" << pk.toString();
+        const uint8_t* pkPtr = pk.getData();
 
         Tox_Err_Bootstrap error;
         if (dhtServer.statusUdp) {
@@ -850,10 +858,6 @@ void Core::bootstrapDht()
             tox_add_tcp_relay(tox.get(), address.constData(), tcpPort, pkPtr, &error);
             PARSE_ERR(error);
         }
-
-        // bootstrap off every 5th node (+ a special case to avoid cycles when listSize % 5 == 0)
-        j += 5 + !(listSize % 5);
-        ++i;
     }
 }
 

src/core/core.h

@@ -52,6 +52,7 @@ class GroupInvite;
 class Profile;
 class Core;
 class IBootstrapListGenerator;
+class DhtServer;
 
 using ToxCorePtr = std::unique_ptr<Core>;
 
@@ -71,7 +72,7 @@ public:
         ERROR_ALLOC
     };
 
-    static ToxCorePtr makeToxCore(const QByteArray& savedata, const ICoreSettings* const settings,
+    static ToxCorePtr makeToxCore(const QByteArray& savedata, const ICoreSettings& settings,
                                   IBootstrapListGenerator& bootstrapNodes, ToxCoreErrors* err = nullptr);
     const CoreAV* getAv() const;
     CoreAV* getAv();
@@ -195,7 +196,7 @@ signals:
     void failedToRemoveFriend(uint32_t friendId);
 
 private:
-    Core(QThread* coreThread, IBootstrapListGenerator& _bootstrapNodes);
+    Core(QThread* coreThread, IBootstrapListGenerator& _bootstrapNodes, const ICoreSettings& settings);
 
     static void onFriendRequest(Tox* tox, const uint8_t* cUserId, const uint8_t* cMessage,
                                 size_t cMessageSize, void* core);
@@ -262,5 +263,6 @@ private:
     mutable CompatibleRecursiveMutex coreLoopLock;
 
     std::unique_ptr<QThread> coreThread;
-    IBootstrapListGenerator& bootstrapNodes;
+    const IBootstrapListGenerator& bootstrapListGenerator;
+    const ICoreSettings& settings;
 };

src/core/toxoptions.cpp

@@ -67,7 +67,7 @@ ToxOptions::operator Tox_Options*()
  * @return ToxOptions instance initialized to create Tox instance
  */
 std::unique_ptr<ToxOptions> ToxOptions::makeToxOptions(const QByteArray& savedata,
-                                                       const ICoreSettings* s)
+                                                       const ICoreSettings& s)
 {
     Tox_Options* tox_opts = tox_options_new(nullptr);
 
@@ -77,7 +77,7 @@ std::unique_ptr<ToxOptions> ToxOptions::makeToxOptions(const QByteArray& savedat
     }
 
     // need to init proxyAddr here, because we need it to construct ToxOptions
-    const QString proxyAddr = s == nullptr ? QString{} : s->getProxyAddr();
+    const QString proxyAddr = s.getProxyAddr();
 
     auto toxOptions = std::unique_ptr<ToxOptions>(new ToxOptions(tox_opts, proxyAddr.toUtf8()));
     // register log first, to get messages as early as possible
@@ -89,19 +89,14 @@ std::unique_ptr<ToxOptions> ToxOptions::makeToxOptions(const QByteArray& savedat
     tox_options_set_savedata_data(*toxOptions, reinterpret_cast<const uint8_t*>(savedata.data()),
                                   savedata.size());
 
-    if(s == nullptr) {
-        qDebug() << "Using Tox default settings";
-        return toxOptions;
-    }
-
     // IPv6 needed for LAN discovery, but can crash some weird routers. On by default, can be
     // disabled in options.
-    const bool enableIPv6 = s->getEnableIPv6();
-    bool forceTCP = s->getForceTCP();
+    const bool enableIPv6 = s.getEnableIPv6();
+    bool forceTCP = s.getForceTCP();
     // LAN requiring UDP is a toxcore limitation, ideally wouldn't be related
-    const bool enableLanDiscovery = s->getEnableLanDiscovery() && !forceTCP;
-    ICoreSettings::ProxyType proxyType = s->getProxyType();
-    quint16 proxyPort = s->getProxyPort();
+    const bool enableLanDiscovery = s.getEnableLanDiscovery() && !forceTCP;
+    ICoreSettings::ProxyType proxyType = s.getProxyType();
+    quint16 proxyPort = s.getProxyPort();
 
     if (!enableLanDiscovery) {
         qWarning() << "Core starting without LAN discovery. Peers can only be found through DHT.";

src/core/toxoptions.h

@@ -34,7 +34,7 @@ public:
     operator Tox_Options*();
     const char* getProxyAddrData() const;
     static std::unique_ptr<ToxOptions> makeToxOptions(const QByteArray& savedata,
-                                                      const ICoreSettings* s);
+                                                      const ICoreSettings& s);
     bool getIPv6Enabled() const;
     void setIPv6Enabled(bool enabled);
 

src/model/ibootstraplistgenerator.h

@@ -32,5 +32,5 @@ public:
     IBootstrapListGenerator(IBootstrapListGenerator&&) = default;
     IBootstrapListGenerator& operator=(IBootstrapListGenerator&&) = default;
 
-    virtual QList<DhtServer> getBootstrapnodes() = 0;
+    virtual QList<DhtServer> getBootstrapnodes() const = 0;
 };

src/net/bootstrapnodeupdater.cpp

@@ -213,7 +213,7 @@ BootstrapNodeUpdater::BootstrapNodeUpdater(const QNetworkProxy& proxy, Paths& _p
     , QObject{parent}
 {}
 
-QList<DhtServer> BootstrapNodeUpdater::getBootstrapnodes()
+QList<DhtServer> BootstrapNodeUpdater::getBootstrapnodes() const
 {
     auto userFilePath = paths.getUserNodesFilePath();
     if (!QFile(userFilePath).exists()) {

src/net/bootstrapnodeupdater.h

@@ -35,7 +35,7 @@ class BootstrapNodeUpdater : public QObject, public IBootstrapListGenerator
     Q_OBJECT
 public:
     explicit BootstrapNodeUpdater(const QNetworkProxy& proxy, Paths& _paths, QObject* parent = nullptr);
-    QList<DhtServer> getBootstrapnodes() override;
+    QList<DhtServer> getBootstrapnodes() const override;
     void requestBootstrapNodes();
     static QList<DhtServer> loadDefaultBootstrapNodes();
 

src/persistence/profile.cpp

@@ -248,7 +248,7 @@ void Profile::initCore(const QByteArray& toxsave, Settings& s, bool isNewProfile
         new BootstrapNodeUpdater(s.getProxy(), paths));
 
     Core::ToxCoreErrors err;
-    core = Core::makeToxCore(toxsave, &s, *bootstrapNodes, &err);
+    core = Core::makeToxCore(toxsave, s, *bootstrapNodes, &err);
     if (!core) {
         switch (err) {
         case Core::ToxCoreErrors::BAD_PROXY:

test/core/core_test.cpp

@@ -79,10 +79,10 @@ private:
 
 class MockNodeListGenerator : public IBootstrapListGenerator
 {
-    QList<DhtServer> getBootstrapnodes();
+    QList<DhtServer> getBootstrapnodes() const override;
 };
 
-QList<DhtServer> MockNodeListGenerator::getBootstrapnodes() {
+QList<DhtServer> MockNodeListGenerator::getBootstrapnodes() const {
     return BootstrapNodeUpdater::loadDefaultBootstrapNodes();
 }
 
@@ -117,7 +117,7 @@ void TestCore::startup_without_proxy()
 
     MockNodeListGenerator nodesGenerator{};
 
-    test_core = Core::makeToxCore(savedata, settings.get(), nodesGenerator, err);
+    test_core = Core::makeToxCore(savedata, *settings, nodesGenerator, err);
 
     if (test_core == nullptr) {
         QFAIL("ToxCore initialisation failed");
@@ -144,7 +144,7 @@ void TestCore::startup_with_invalid_proxy()
 
     MockNodeListGenerator nodesGenerator{};
 
-    test_core = Core::makeToxCore(savedata, settings.get(), nodesGenerator, err);
+    test_core = Core::makeToxCore(savedata, *settings, nodesGenerator, err);
 
     if (test_core != nullptr) {
         QFAIL("ToxCore initialisation passed with invalid SOCKS5 proxy address");
@@ -156,7 +156,7 @@ void TestCore::startup_with_invalid_proxy()
     settings->setProxyPort(9985);
     settings->setProxyType(MockSettings::ProxyType::ptHTTP);
 
-    test_core = Core::makeToxCore(savedata, settings.get(), nodesGenerator, err);
+    test_core = Core::makeToxCore(savedata, *settings, nodesGenerator, err);
 
     if (test_core != nullptr) {
         QFAIL("ToxCore initialisation passed with invalid HTTP proxy address");