Merge #2359: Net code updates [Step 2]

fcdf87a Populate services in GetLocalAddress (Alex Morcos)
a0a079f Return early in IsBanned. (Gregory Maxwell)
7772138 Remove redundant nullptr checks before deallocation (practicalswift)
5390862 net: remove unimplemented functions. (furszy)
74d0482 Limit setAskFor and retire requested entries only when a getdata returns. (Gregory Maxwell)
3b3bf63 prevent peer flooding request queue for an inv (kazcw)
c814967 Fix subscript[0] potential bugs in key.cpp (Jeremy Rubin)
8e2e79e Remove unnecessary branches in utilstrencodings string constructors. (Jeremy Rubin)
25a16b3 Fix subscript[0] in utilstrencodings.cpp (Jeremy Rubin)
df0584e Fix subscript[0] in streams.h (Jeremy Rubin)
b7e64a5 Fix subscript[0] in validation.cpp (Jeremy Rubin)
2f7b73b Fix subscript[0] in torcontrol (Jeremy Rubin)
3b883eb Fix subscript[0] in netaddress.cpp (Jeremy Rubin)
e5f5401 Fix subscript[0] in base58.cpp (Jeremy Rubin)
7d4ec87 Cleanup (safe, it was checked) subscript[0] in MurmurHash3 (and cleanup MurmurHash3 to be more clear). (Jeremy Rubin)
5e14f54 Fix subscript[0] in compressor.cpp (Jeremy Rubin)
48a622d Fix subscript[0] bug in net.cpp if GetGroup returns a 0-sized vector (Jeremy Rubin)
af4cba3 net: use an internal address for fixed seeds (Cory Fields)
36368c5 net: switch to dummy internal ip for dns seed source (Cory Fields)
8f31295 net: do not allow resolving to an internal address (Cory Fields)
8ba3a40 net: add an internal subnet for representing unresolved hostnames (Cory Fields)
c60b875 fix uninitialized read when stringifying an addrLocal (Kaz Wesley)
3d36540 add test demonstrating addrLocal UB (Kaz Wesley)

Pull request description:

  Grouped some net updates (plus a miscellaneous one) coming from upstream.
  Part of another deep rabbit hole that i'm doing in parallel of the tier two work.

  PRs List:
  * bitcoin#7079.
  * bitcoin#9804.
  * bitcoin#10424
  * bitcoin#10446.
  * bitcoin#10564.
  * bitcoin#14728.

ACKs for top commit:
  random-zebra:
    Code ACK fcdf87a

Tree-SHA512: ee81c834641aa6fdb9ca4396657457358a4b32f7862d60716e914dcfc2d355970937bd0fb4e164faaa0f4ea26d263ca8a2af4d8d5d6615b2db5bf89ec70d15f3

src/base58.cpp

@@ -127,7 +127,7 @@ std::string EncodeBase58(const unsigned char* pbegin, const unsigned char* pend)
 
 std::string EncodeBase58(const std::vector<unsigned char>& vch)
 {
-    return EncodeBase58(&vch[0], &vch[0] + vch.size());
+    return EncodeBase58(vch.data(), vch.data() + vch.size());
 }
 
 bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet)
@@ -177,7 +177,7 @@ void CBase58Data::SetData(const std::vector<unsigned char>& vchVersionIn, const
     vchVersion = vchVersionIn;
     vchData.resize(nSize);
     if (!vchData.empty())
-        memcpy(&vchData[0], pdata, nSize);
+        memcpy(vchData.data(), pdata, nSize);
 }
 
 void CBase58Data::SetData(const std::vector<unsigned char>& vchVersionIn, const unsigned char* pbegin, const unsigned char* pend)
@@ -197,8 +197,8 @@ bool CBase58Data::SetString(const char* psz, unsigned int nVersionBytes)
     vchVersion.assign(vchTemp.begin(), vchTemp.begin() + nVersionBytes);
     vchData.resize(vchTemp.size() - nVersionBytes);
     if (!vchData.empty())
-        memcpy(&vchData[0], &vchTemp[nVersionBytes], vchData.size());
-    memory_cleanse(&vchTemp[0], vchTemp.size());
+        memcpy(vchData.data(), vchTemp.data() + nVersionBytes, vchData.size());
+    memory_cleanse(vchTemp.data(), vchTemp.size());
     return true;
 }
 

src/base58.h

@@ -118,8 +118,8 @@ class CBitcoinExtKeyBase : public CBase58Data
     {
         K ret;
         if (vchData.size() == Size) {
-            //if base58 encouded data not holds a ext key, return a !IsValid() key
-            ret.Decode(&vchData[0]);
+            // If base58 encoded data does not hold an ext key, return a !IsValid() key
+            ret.Decode(vchData.data());
         }
         return ret;
     }

src/chainparams.cpp

@@ -228,8 +228,8 @@ class CMainParams : public CChainParams
         nDefaultPort = 51472;
 
         // Note that of those with the service bits flag, most only support a subset of possible options
-        vSeeds.emplace_back("fuzzbawls.pw", "pivx.seed.fuzzbawls.pw", true);     // Primary DNS Seeder from Fuzzbawls
-        vSeeds.emplace_back("fuzzbawls.pw", "pivx.seed2.fuzzbawls.pw", true);    // Secondary DNS Seeder from Fuzzbawls
+        vSeeds.emplace_back("pivx.seed.fuzzbawls.pw", true);     // Primary DNS Seeder from Fuzzbawls
+        vSeeds.emplace_back("pivx.seed2.fuzzbawls.pw", true);    // Secondary DNS Seeder from Fuzzbawls
 
         base58Prefixes[PUBKEY_ADDRESS] = std::vector<unsigned char>(1, 30);
         base58Prefixes[SCRIPT_ADDRESS] = std::vector<unsigned char>(1, 13);
@@ -353,8 +353,8 @@ class CTestNetParams : public CChainParams
         nDefaultPort = 51474;
 
         // nodes with support for servicebits filtering should be at the top
-        vSeeds.emplace_back("fuzzbawls.pw", "pivx-testnet.seed.fuzzbawls.pw", true);
-        vSeeds.emplace_back("fuzzbawls.pw", "pivx-testnet.seed2.fuzzbawls.pw", true);
+        vSeeds.emplace_back("pivx-testnet.seed.fuzzbawls.pw", true);
+        vSeeds.emplace_back("pivx-testnet.seed2.fuzzbawls.pw", true);
 
         base58Prefixes[PUBKEY_ADDRESS] = std::vector<unsigned char>(1, 139); // Testnet pivx addresses start with 'x' or 'y'
         base58Prefixes[SCRIPT_ADDRESS] = std::vector<unsigned char>(1, 19);  // Testnet pivx script addresses start with '8' or '9'

src/chainparams.h

@@ -19,9 +19,9 @@
 #include <vector>
 
 struct CDNSSeedData {
-    std::string name, host;
+    std::string host;
     bool supportsServiceBitsFiltering;
-    CDNSSeedData(const std::string& strName, const std::string& strHost, bool supportsServiceBitsFilteringIn = false) : name(strName), host(strHost), supportsServiceBitsFiltering(supportsServiceBitsFilteringIn) {}
+    CDNSSeedData(const std::string& strHost, bool supportsServiceBitsFilteringIn = false) : host(strHost), supportsServiceBitsFiltering(supportsServiceBitsFilteringIn) {}
 };
 
 struct SeedSpec6 {

src/compressor.cpp

@@ -89,30 +89,30 @@ bool CScriptCompressor::Decompress(unsigned int nSize, const std::vector<unsigne
         script[0] = OP_DUP;
         script[1] = OP_HASH160;
         script[2] = 20;
-        memcpy(&script[3], &in[0], 20);
+        memcpy(&script[3], in.data(), 20);
         script[23] = OP_EQUALVERIFY;
         script[24] = OP_CHECKSIG;
         return true;
     case 0x01:
         script.resize(23);
         script[0] = OP_HASH160;
         script[1] = 20;
-        memcpy(&script[2], &in[0], 20);
+        memcpy(&script[2], in.data(), 20);
         script[22] = OP_EQUAL;
         return true;
     case 0x02:
     case 0x03:
         script.resize(35);
         script[0] = 33;
         script[1] = nSize;
-        memcpy(&script[2], &in[0], 32);
+        memcpy(&script[2], in.data(), 32);
         script[34] = OP_CHECKSIG;
         return true;
     case 0x04:
     case 0x05:
         unsigned char vch[33] = {};
         vch[0] = nSize - 2;
-        memcpy(&vch[1], &in[0], 32);
+        memcpy(&vch[1], in.data(), 32);
         CPubKey pubkey(&vch[0], &vch[33]);
         if (!pubkey.Decompress())
             return false;

src/hash.cpp

@@ -17,35 +17,34 @@ unsigned int MurmurHash3(unsigned int nHashSeed, const std::vector<unsigned char
 {
     // The following is MurmurHash3 (x86_32), see http://code.google.com/p/smhasher/source/browse/trunk/MurmurHash3.cpp
     uint32_t h1 = nHashSeed;
-    if (vDataToHash.size() > 0) {
-        const uint32_t c1 = 0xcc9e2d51;
-        const uint32_t c2 = 0x1b873593;
+    const uint32_t c1 = 0xcc9e2d51;
+    const uint32_t c2 = 0x1b873593;
 
-        const int nblocks = vDataToHash.size() / 4;
+    const int nblocks = vDataToHash.size() / 4;
 
-        //----------
-        // body
-        const uint8_t* blocks = &vDataToHash[0] + nblocks * 4;
+    //----------
+    // body
+    const uint8_t* blocks = vDataToHash.data();
 
-        for (int i = -nblocks; i; i++) {
-            uint32_t k1 = ReadLE32(blocks + i*4);
+    for (int i = 0; i < nblocks; ++i) {
+        uint32_t k1 = ReadLE32(blocks + i*4);
 
-            k1 *= c1;
-            k1 = ROTL32(k1, 15);
-            k1 *= c2;
+        k1 *= c1;
+        k1 = ROTL32(k1, 15);
+        k1 *= c2;
 
-            h1 ^= k1;
-            h1 = ROTL32(h1, 13);
-            h1 = h1 * 5 + 0xe6546b64;
-        }
+        h1 ^= k1;
+        h1 = ROTL32(h1, 13);
+        h1 = h1 * 5 + 0xe6546b64;
+    }
 
-        //----------
-        // tail
-        const uint8_t* tail = (const uint8_t*)(&vDataToHash[0] + nblocks * 4);
+    //----------
+    // tail
+    const uint8_t* tail = vDataToHash.data() + nblocks * 4;
 
-        uint32_t k1 = 0;
+    uint32_t k1 = 0;
 
-        switch (vDataToHash.size() & 3) {
+    switch (vDataToHash.size() & 3) {
         case 3:
             k1 ^= tail[2] << 16;
         case 2:
@@ -56,7 +55,6 @@ unsigned int MurmurHash3(unsigned int nHashSeed, const std::vector<unsigned char
             k1 = ROTL32(k1, 15);
             k1 *= c2;
             h1 ^= k1;
-        };
     }
 
     //----------

src/key.cpp

@@ -189,7 +189,7 @@ CPrivKey CKey::GetPrivKey() const
     size_t privkeylen;
     privkey.resize(PRIVATE_KEY_SIZE);
     privkeylen = PRIVATE_KEY_SIZE;
-    int ret = ec_privkey_export_der(secp256k1_context_sign, (unsigned char*)&privkey[0], &privkeylen, begin(), fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
+    int ret = ec_privkey_export_der(secp256k1_context_sign, (unsigned char*)privkey.data(), &privkeylen, begin(), fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
     assert(ret);
     privkey.resize(privkeylen);
     return privkey;
@@ -306,7 +306,6 @@ void CExtKey::SetSeed(const unsigned char* seed, unsigned int nSeedLen)
     CHMAC_SHA512(hashkey, sizeof(hashkey)).Write(seed, nSeedLen).Finalize(vout.data());
     key.Set(vout.data(), vout.data() + 32, true);
     memcpy(chaincode.begin(), vout.data() + 32, 32);
-
     nDepth = 0;
     nChild = 0;
     memset(vchFingerprint, 0, sizeof(vchFingerprint));

src/net.cpp

@@ -150,7 +150,7 @@ static std::vector<CAddress> convertSeed6(const std::vector<SeedSpec6>& vSeedsIn
 // one by discovery.
 CAddress GetLocalAddress(const CNetAddr* paddrPeer, ServiceFlags nLocalServices)
 {
-    CAddress ret(CService(CNetAddr(), GetListenPort()), NODE_NONE);
+    CAddress ret(CService(CNetAddr(), GetListenPort()), nLocalServices);
     CService addr;
     if (GetLocal(addr, paddrPeer)) {
         ret = CAddress(addr, nLocalServices);
@@ -238,7 +238,7 @@ bool RemoveLocal(const CService& addr)
 /** Make a particular network entirely off-limits (no automatic connects to it) */
 void SetLimited(enum Network net, bool fLimited)
 {
-    if (net == NET_UNROUTABLE)
+    if (net == NET_UNROUTABLE || net == NET_INTERNAL)
         return;
     LOCK(cs_mapLocalHost);
     vfLimited[net] = fLimited;
@@ -456,34 +456,30 @@ void CConnman::ClearBanned()
 
 bool CConnman::IsBanned(CNetAddr ip)
 {
-    bool fResult = false;
+    LOCK(cs_setBanned);
+    for (banmap_t::iterator it = setBanned.begin(); it != setBanned.end(); it++)
     {
-        LOCK(cs_setBanned);
-        for (banmap_t::iterator it = setBanned.begin(); it != setBanned.end(); it++)
-        {
-            CSubNet subNet = (*it).first;
-            CBanEntry banEntry = (*it).second;
+        CSubNet subNet = (*it).first;
+        CBanEntry banEntry = (*it).second;
 
-            if(subNet.Match(ip) && GetTime() < banEntry.nBanUntil)
-                fResult = true;
+        if (subNet.Match(ip) && GetTime() < banEntry.nBanUntil) {
+            return true;
         }
     }
-    return fResult;
+    return false;
 }
 
 bool CConnman::IsBanned(CSubNet subnet)
 {
-    bool fResult = false;
-    {
-        LOCK(cs_setBanned);
-        banmap_t::iterator i = setBanned.find(subnet);
-        if (i != setBanned.end()) {
-            CBanEntry banEntry = (*i).second;
-            if (GetTime() < banEntry.nBanUntil)
-                fResult = true;
+    LOCK(cs_setBanned);
+    banmap_t::iterator i = setBanned.find(subnet);
+    if (i != setBanned.end()) {
+        CBanEntry banEntry = (*i).second;
+        if (GetTime() < banEntry.nBanUntil) {
+            return true;
         }
     }
-    return fResult;
+    return false;
 }
 
 void CConnman::Ban(const CNetAddr& addr, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch)
@@ -1508,26 +1504,20 @@ void CConnman::ThreadDNSAddressSeed()
             std::vector<CNetAddr> vIPs;
             std::vector<CAddress> vAdd;
             ServiceFlags requiredServiceBits = nRelevantServices;
-            if (LookupHost(GetDNSHost(seed, &requiredServiceBits).c_str(), vIPs, 0, true)) {
+            std::string host = GetDNSHost(seed, &requiredServiceBits);
+            CNetAddr resolveSource;
+            if (!resolveSource.SetInternal(host)) {
+                continue;
+            }
+            if (LookupHost(host.c_str(), vIPs, 0, true)) {
                 for (CNetAddr& ip : vIPs) {
-                    int nOneDay = 24 * 3600;
+                    int nOneDay = 24*3600;
                     CAddress addr = CAddress(CService(ip, Params().GetDefaultPort()), requiredServiceBits);
                     addr.nTime = GetTime() - 3 * nOneDay - GetRand(4 * nOneDay); // use a random age between 3 and 7 days old
                     vAdd.push_back(addr);
                     found++;
                 }
-            }
-            if (interruptNet) {
-                return;
-            }
-            // TODO: The seed name resolve may fail, yielding an IP of [::], which results in
-            // addrman assigning the same source to results from different seeds.
-            // This should switch to a hard-coded stable dummy IP for each seed name, so that the
-            // resolve is not required at all.
-            if (!vIPs.empty()) {
-                CService seedSource;
-                Lookup(seed.name.c_str(), seedSource, 0, true);
-                addrman.Add(vAdd, seedSource);
+                addrman.Add(vAdd, resolveSource);
             }
         }
     }
@@ -1621,7 +1611,7 @@ void CConnman::ThreadOpenConnections()
             if (!done) {
                 LogPrintf("Adding fixed seed nodes as DNS doesn't seem to be available.\n");
                 CNetAddr local;
-                LookupHost("127.0.0.1", local, false);
+                local.SetInternal("fixedseeds");
                 addrman.Add(convertSeed6(Params().FixedSeeds()), local);
                 done = true;
             }
@@ -2476,14 +2466,17 @@ CNode::~CNode()
 {
     CloseSocket(hSocket);
 
-    if (pfilter)
-        delete pfilter;
+    delete pfilter;
 }
 
 void CNode::AskFor(const CInv& inv)
 {
-    if (mapAskFor.size() > MAPASKFOR_MAX_SZ)
+    if (mapAskFor.size() > MAPASKFOR_MAX_SZ || setAskFor.size() > SETASKFOR_MAX_SZ)
         return;
+    // a peer may not have multiple non-responded queue positions for a single inv item
+    if (!setAskFor.insert(inv.hash).second)
+        return;
+
     // We're using mapAskFor as a priority queue,
     // the key is the earliest time the request can be sent
     int64_t nRequestTime;
@@ -2599,6 +2592,6 @@ uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& ad)
 {
     std::vector<unsigned char> vchNetGroup(ad.GetGroup());
 
-    return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(&vchNetGroup[0], vchNetGroup.size()).Finalize();
+    return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize();
 }
 

src/net.h

@@ -69,6 +69,8 @@ static const bool DEFAULT_UPNP = false;
 #endif
 /** The maximum number of entries in mapAskFor */
 static const size_t MAPASKFOR_MAX_SZ = MAX_INV_SZ;
+/** The maximum number of entries in setAskFor (larger due to getdata latency)*/
+static const size_t SETASKFOR_MAX_SZ = 2 * MAX_INV_SZ;
 /** The maximum number of peer connections to maintain. */
 static const unsigned int DEFAULT_MAX_PEER_CONNECTIONS = 125;
 /** Disconnected peers are added to setOffsetDisconnectedPeers only if node has less than ENOUGH_CONNECTIONS */
@@ -222,7 +224,6 @@ class CConnman
     void AddNewAddress(const CAddress& addr, const CAddress& addrFrom, int64_t nTimePenalty = 0);
     void AddNewAddresses(const std::vector<CAddress>& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0);
     std::vector<CAddress> GetAddresses();
-    void AddressCurrentlyConnected(const CService& addr);
 
     // Denial-of-service detection/prevention
     // The idea is to detect peers that are behaving
@@ -624,6 +625,7 @@ class CNode
     std::vector<CInv> vInventoryTierTwoToSend;
     RecursiveMutex cs_inventory;
     std::multimap<int64_t, CInv> mapAskFor;
+    std::set<uint256> setAskFor;
     std::vector<uint256> vBlockRequested;
     int64_t nNextInvSend;
     // Used for BIP35 mempool sending, also protected by cs_inventory
@@ -791,9 +793,6 @@ class CNode
         vecRequestsFulfilled.push_back(strRequest);
     }
 
-    bool IsSubscribed(unsigned int nChannel);
-    void Subscribe(unsigned int nChannel, unsigned int nHops = 0);
-    void CancelSubscribe(unsigned int nChannel);
     void CloseSocketDisconnect();
     bool DisconnectOldProtocol(int nVersionIn, int nVersionRequired, std::string strLastCommand = "");
 

src/net_processing.cpp

@@ -1512,6 +1512,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
         bool fMissingInputs = false;
         CValidationState state;
 
+        pfrom->setAskFor.erase(inv.hash);
         mapAlreadyAskedFor.erase(inv);
 
         if (ptx->ContainsZerocoins()) {
@@ -2377,6 +2378,9 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic<bool>& interruptMsg
                     connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
                     vGetData.clear();
                 }
+            } else {
+                //If we're not going to ask, don't expect a response.
+                pto->setAskFor.erase(inv.hash);
             }
             pto->mapAskFor.erase(pto->mapAskFor.begin());
         }