From b409e04734d4240968fe221981ceac70fd659630 Mon Sep 17 00:00:00 2001
From: Simon Walton <simon@highfidelity.io>
Date: Wed, 28 Mar 2018 17:42:23 -0700
Subject: [PATCH] Handle all case of nodes being deleted from nodelist

Delete erased nodes from local ID mapping, as this uses
SharedPointer. Don't keep the mapping from local IDs
to UUIDs in GateKeeper as it isn't used.
---
 domain-server/src/DomainGatekeeper.cpp       |  6 ++++--
 domain-server/src/DomainGatekeeper.h         |  5 +++--
 domain-server/src/DomainServer.cpp           |  2 +-
 libraries/networking/src/LimitedNodeList.cpp |  4 ++++
 libraries/networking/src/NLPacket.h          | 18 +++++++++++++++++-
 5 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp
index 40130688fb..fb3c305b84 100644
--- a/domain-server/src/DomainGatekeeper.cpp
+++ b/domain-server/src/DomainGatekeeper.cpp
@@ -1033,13 +1033,15 @@ Node::LocalID DomainGatekeeper::findOrCreateLocalID(const QUuid& uuid) {
         return existingLocalIDIt->second;
     }
 
+    assert(_localIDs.size() < std::numeric_limits<LocalIDs::value_type>::max() - 2);
+
     Node::LocalID newLocalID;
     do {
         newLocalID = _currentLocalID;
         _currentLocalID += _idIncrement;
-    } while (newLocalID == 0 || _localIDToUUID.find(newLocalID) != _localIDToUUID.end());
+    } while (newLocalID == 0 || _localIDs.find(newLocalID) != _localIDs.end());
 
     _uuidToLocalID.emplace(uuid, newLocalID);
-    _localIDToUUID.emplace(newLocalID, uuid);
+    _localIDs.insert(newLocalID);
     return newLocalID;
 }
diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h
index afb848f271..edd976a77b 100644
--- a/domain-server/src/DomainGatekeeper.h
+++ b/domain-server/src/DomainGatekeeper.h
@@ -15,6 +15,7 @@
 #define hifi_DomainGatekeeper_h
 
 #include <unordered_map>
+#include <unordered_set>
 
 #include <QtCore/QObject>
 #include <QtNetwork/QNetworkReply>
@@ -129,9 +130,9 @@ private:
         size_t operator()(const QUuid& uuid) const { return qHash(uuid); }
     };
     using UUIDToLocalID = std::unordered_map<QUuid, Node::LocalID, UuidHash> ;
-    using LocalIDToUUID = std::unordered_map<Node::LocalID, QUuid>;
+    using LocalIDs = std::unordered_set<Node::LocalID>;
+    LocalIDs _localIDs;
     UUIDToLocalID _uuidToLocalID;
-    LocalIDToUUID _localIDToUUID;
 
     Node::LocalID _currentLocalID;
     quint16 _idIncrement;
diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp
index 4077becc57..77f25d4c7e 100644
--- a/domain-server/src/DomainServer.cpp
+++ b/domain-server/src/DomainServer.cpp
@@ -2905,7 +2905,7 @@ void DomainServer::updateReplicationNodes(ReplicationServerDirection direction)
                         // manually add the replication node to our node list
                         auto node = nodeList->addOrUpdateNode(QUuid::createUuid(), replicationServer.nodeType,
                                                               replicationServer.sockAddr, replicationServer.sockAddr,
-                                                              false, direction == Upstream);
+                                                              0, false, direction == Upstream);
                         node->setIsForcedNeverSilent(true);
 
                         qDebug() << "Adding" << (direction == Upstream ? "upstream" : "downstream")
diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp
index 31345c3c14..6ef119bf3b 100644
--- a/libraries/networking/src/LimitedNodeList.cpp
+++ b/libraries/networking/src/LimitedNodeList.cpp
@@ -695,6 +695,7 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t
 
                 auto oldSoloNode = previousSoloIt->second;
 
+                _localIDMap.erase(oldSoloNode->getLocalID());
                 _nodeHash.unsafe_erase(previousSoloIt);
                 handleNodeKill(oldSoloNode);
 
@@ -850,6 +851,9 @@ void LimitedNodeList::removeSilentNodes() {
     });
 
     foreach(const SharedNodePointer& killedNode, killedNodes) {
+        _nodeMutex.lockForWrite();
+        _localIDMap.erase(killedNode->getLocalID());
+        _nodeMutex.unlock();
         handleNodeKill(killedNode);
     }
 }
diff --git a/libraries/networking/src/NLPacket.h b/libraries/networking/src/NLPacket.h
index e3107fae3f..a994b7bde0 100644
--- a/libraries/networking/src/NLPacket.h
+++ b/libraries/networking/src/NLPacket.h
@@ -21,6 +21,22 @@
 class NLPacket : public udt::Packet {
     Q_OBJECT
 public:
+    //
+    //    Current NLPacket format:
+    //
+    //    |      BYTE     |      BYTE     |      BYTE     |      BYTE     |
+    //     0                   1                   2                   3
+    //     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+    //    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+    //    |  Packet Type  |    Version    | Local Node ID - sourced only  |
+    //    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+    //    |                                                               |
+    //    |                 MD5 Verification - 16 bytes                   |
+    //    |                 (ONLY FOR VERIFIED PACKETS)                   |
+    //    |                                                               |
+    //    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+
+    //    Old NLPacket format:
     //
     //    |      BYTE     |      BYTE     |      BYTE     |      BYTE     |
     //
@@ -41,7 +57,7 @@ public:
     //    |                               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     //    |                               |                               |
     //    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+                               |
-    //    NLPacket Header Format
+    //
 
     using LocalID = NetworkLocalID;
     static const int NUM_BYTES_LOCALID = sizeof(LocalID);