Fix NodeDB review follow-ups

Agent-Logs-Url: https://github.com/meshtastic/firmware/sessions/6b1d6cf6-ed6b-43b6-95cb-8e141757664e

Co-authored-by: thebentern <9000580+thebentern@users.noreply.github.com>

src/mesh/MeshService.cpp

@@ -401,8 +401,7 @@ meshtastic_NodeInfoLite *MeshService::refreshLocalMeshNode()
     // Make sure our own NodeNum has a slot in the position map so subsequent
     // updates (and the bundled NodeInfo emission to the phone) have somewhere
     // to read from. Insert a default-zero entry on first call.
-    auto &mySlot = nodeDB->nodePositions[node->num];
+    nodeDB->touchNodePositionTime(node->num, getValidTime(RTCQualityFromNet));
-    mySlot.time = getValidTime(RTCQualityFromNet);
 #endif
 
     if (powerStatus->getHasBattery() == 1) {

src/mesh/NodeDB.cpp

@@ -542,7 +542,10 @@ NodeDB::NodeDB()
         fixedGPS.location_source = meshtastic_Position_LocSource_LOC_MANUAL;
         config.has_position = true;
 #if !MESHTASTIC_EXCLUDE_POSITIONDB
-        nodePositions[info->num] = TypeConversions::ConvertToPositionLite(fixedGPS);
+        {
+            concurrency::LockGuard guard(&satelliteMutex);
+            nodePositions[info->num] = TypeConversions::ConvertToPositionLite(fixedGPS);
+        }
 #endif
         nodeDB->setLocalPosition(fixedGPS);
         config.position.fixed_position = true;
@@ -579,6 +582,28 @@ bool isBroadcast(uint32_t dest)
     return dest == NODENUM_BROADCAST || dest == NODENUM_BROADCAST_NO_LORA;
 }
 
+namespace {
+template <typename Map, typename Value> bool copySatelliteEntry(const Map &map, NodeNum n, Value &out)
+{
+    auto it = map.find(n);
+    if (it == map.end())
+        return false;
+    out = it->second;
+    return true;
+}
+
+template <typename Map> std::vector<NodeNum> snapshotSatelliteNodeNums(const Map &map, NodeNum exclude)
+{
+    std::vector<NodeNum> result;
+    result.reserve(map.size());
+    for (const auto &kv : map) {
+        if (kv.first != exclude)
+            result.push_back(kv.first);
+    }
+    return result;
+}
+} // namespace
+
 void NodeDB::resetRadioConfig(bool is_fresh_install)
 {
     if (is_fresh_install) {
@@ -648,6 +673,7 @@ void NodeDB::installDefaultNodeDatabase()
     nodeDatabase.nodes = std::vector<meshtastic_NodeInfoLite>(MAX_NUM_NODES);
     numMeshNodes = 0;
     meshNodes = &nodeDatabase.nodes;
+    concurrency::LockGuard satelliteGuard(&satelliteMutex);
 #if !MESHTASTIC_EXCLUDE_POSITIONDB
     nodePositions.clear();
 #endif
@@ -1183,6 +1209,7 @@ void NodeDB::removeNodeByNum(NodeNum nodeNum)
 void NodeDB::clearLocalPosition()
 {
 #if !MESHTASTIC_EXCLUDE_POSITIONDB
+    concurrency::LockGuard guard(&satelliteMutex);
     nodePositions.erase(getNodeNum());
 #endif
     setLocalPosition(meshtastic_Position_init_default);
@@ -1200,6 +1227,18 @@ const meshtastic_PositionLite *NodeDB::getNodePosition(NodeNum n) const
 #endif
 }
 
+bool NodeDB::copyNodePosition(NodeNum n, meshtastic_PositionLite &out) const
+{
+#if MESHTASTIC_EXCLUDE_POSITIONDB
+    (void)n;
+    (void)out;
+    return false;
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    return copySatelliteEntry(nodePositions, n, out);
+#endif
+}
+
 const meshtastic_DeviceMetrics *NodeDB::getNodeTelemetry(NodeNum n) const
 {
 #if MESHTASTIC_EXCLUDE_TELEMETRYDB
@@ -1211,6 +1250,18 @@ const meshtastic_DeviceMetrics *NodeDB::getNodeTelemetry(NodeNum n) const
 #endif
 }
 
+bool NodeDB::copyNodeTelemetry(NodeNum n, meshtastic_DeviceMetrics &out) const
+{
+#if MESHTASTIC_EXCLUDE_TELEMETRYDB
+    (void)n;
+    (void)out;
+    return false;
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    return copySatelliteEntry(nodeTelemetry, n, out);
+#endif
+}
+
 const meshtastic_EnvironmentMetrics *NodeDB::getNodeEnvironment(NodeNum n) const
 {
 #if MESHTASTIC_EXCLUDE_ENVIRONMENTDB
@@ -1222,6 +1273,18 @@ const meshtastic_EnvironmentMetrics *NodeDB::getNodeEnvironment(NodeNum n) const
 #endif
 }
 
+bool NodeDB::copyNodeEnvironment(NodeNum n, meshtastic_EnvironmentMetrics &out) const
+{
+#if MESHTASTIC_EXCLUDE_ENVIRONMENTDB
+    (void)n;
+    (void)out;
+    return false;
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    return copySatelliteEntry(nodeEnvironment, n, out);
+#endif
+}
+
 const meshtastic_StatusMessage *NodeDB::getNodeStatus(NodeNum n) const
 {
 #if MESHTASTIC_EXCLUDE_STATUSDB
@@ -1233,18 +1296,87 @@ const meshtastic_StatusMessage *NodeDB::getNodeStatus(NodeNum n) const
 #endif
 }
 
+bool NodeDB::copyNodeStatus(NodeNum n, meshtastic_StatusMessage &out) const
+{
+#if MESHTASTIC_EXCLUDE_STATUSDB
+    (void)n;
+    (void)out;
+    return false;
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    return copySatelliteEntry(nodeStatus, n, out);
+#endif
+}
+
+std::vector<NodeNum> NodeDB::snapshotPositionNodeNums(NodeNum exclude) const
+{
+#if MESHTASTIC_EXCLUDE_POSITIONDB
+    (void)exclude;
+    return {};
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    return snapshotSatelliteNodeNums(nodePositions, exclude);
+#endif
+}
+
+std::vector<NodeNum> NodeDB::snapshotTelemetryNodeNums(NodeNum exclude) const
+{
+#if MESHTASTIC_EXCLUDE_TELEMETRYDB
+    (void)exclude;
+    return {};
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    return snapshotSatelliteNodeNums(nodeTelemetry, exclude);
+#endif
+}
+
+std::vector<NodeNum> NodeDB::snapshotEnvironmentNodeNums(NodeNum exclude) const
+{
+#if MESHTASTIC_EXCLUDE_ENVIRONMENTDB
+    (void)exclude;
+    return {};
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    return snapshotSatelliteNodeNums(nodeEnvironment, exclude);
+#endif
+}
+
+std::vector<NodeNum> NodeDB::snapshotStatusNodeNums(NodeNum exclude) const
+{
+#if MESHTASTIC_EXCLUDE_STATUSDB
+    (void)exclude;
+    return {};
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    return snapshotSatelliteNodeNums(nodeStatus, exclude);
+#endif
+}
+
 void NodeDB::setNodeStatus(NodeNum n, const meshtastic_StatusMessage &status)
 {
 #if MESHTASTIC_EXCLUDE_STATUSDB
     (void)n;
     (void)status;
 #else
+    concurrency::LockGuard guard(&satelliteMutex);
     nodeStatus[n] = status;
 #endif
 }
 
+void NodeDB::touchNodePositionTime(NodeNum n, uint32_t time)
+{
+#if MESHTASTIC_EXCLUDE_POSITIONDB
+    (void)n;
+    (void)time;
+#else
+    concurrency::LockGuard guard(&satelliteMutex);
+    nodePositions[n].time = time;
+#endif
+}
+
 void NodeDB::eraseNodeSatellites(NodeNum n)
 {
+    concurrency::LockGuard guard(&satelliteMutex);
 #if !MESHTASTIC_EXCLUDE_POSITIONDB
     nodePositions.erase(n);
 #endif
@@ -1372,7 +1504,8 @@ LoadFileResult NodeDB::loadProto(const char *filename, size_t protoSize, size_t
     if (f) {
         LOG_INFO("Load %s", filename);
         pb_istream_t stream = {&readcb, &f, protoSize};
-        if (fields != &meshtastic_NodeDatabase_msg) // contains a vector object
+        if (fields != &meshtastic_NodeDatabase_msg &&
+            fields != &meshtastic_NodeDatabase_Legacy_msg) // contains a vector object
             memset(dest_struct, 0, objSize);
         if (!pb_decode(&stream, fields, dest_struct)) {
             LOG_ERROR("Error: can't decode protobuf %s", PB_GET_ERROR(&stream));
@@ -1464,62 +1597,65 @@ void NodeDB::loadFromDisk()
             nodeDatabase.nodes.clear();
             const size_t maxToMigrate = std::min<size_t>(legacyDb.nodes.size(), MAX_NUM_NODES);
             nodeDatabase.nodes.reserve(maxToMigrate);
-            for (size_t i = 0; i < maxToMigrate; ++i) {
+            size_t posCount = 0, telCount = 0;
-                const auto &legacy = legacyDb.nodes[i];
+            {
-                meshtastic_NodeInfoLite slim = meshtastic_NodeInfoLite_init_default;
+                concurrency::LockGuard satelliteGuard(&satelliteMutex);
-                slim.num = legacy.num;
+                for (size_t i = 0; i < maxToMigrate; ++i) {
-                slim.snr = legacy.snr;
+                    const auto &legacy = legacyDb.nodes[i];
-                slim.last_heard = legacy.last_heard;
+                    meshtastic_NodeInfoLite slim = meshtastic_NodeInfoLite_init_default;
-                slim.channel = legacy.channel;
+                    slim.num = legacy.num;
-                slim.has_hops_away = legacy.has_hops_away;
+                    slim.snr = legacy.snr;
-                slim.hops_away = legacy.hops_away;
+                    slim.last_heard = legacy.last_heard;
-                slim.next_hop = legacy.next_hop;
+                    slim.channel = legacy.channel;
-                slim.bitfield = legacy.bitfield;
+                    slim.has_hops_away = legacy.has_hops_away;
-                if (legacy.via_mqtt)
+                    slim.hops_away = legacy.hops_away;
-                    slim.bitfield |= NODEINFO_BITFIELD_VIA_MQTT_MASK;
+                    slim.next_hop = legacy.next_hop;
-                if (legacy.is_favorite)
+                    slim.bitfield = legacy.bitfield;
-                    slim.bitfield |= NODEINFO_BITFIELD_IS_FAVORITE_MASK;
+                    if (legacy.via_mqtt)
-                if (legacy.is_ignored)
+                        slim.bitfield |= NODEINFO_BITFIELD_VIA_MQTT_MASK;
-                    slim.bitfield |= NODEINFO_BITFIELD_IS_IGNORED_MASK;
+                    if (legacy.is_favorite)
-                if (legacy.has_user) {
+                        slim.bitfield |= NODEINFO_BITFIELD_IS_FAVORITE_MASK;
-                    slim.bitfield |= NODEINFO_BITFIELD_HAS_USER_MASK;
+                    if (legacy.is_ignored)
-                    strncpy(slim.long_name, legacy.user.long_name, sizeof(slim.long_name));
+                        slim.bitfield |= NODEINFO_BITFIELD_IS_IGNORED_MASK;
-                    slim.long_name[sizeof(slim.long_name) - 1] = '\0';
+                    if (legacy.has_user) {
-                    strncpy(slim.short_name, legacy.user.short_name, sizeof(slim.short_name));
+                        slim.bitfield |= NODEINFO_BITFIELD_HAS_USER_MASK;
-                    slim.short_name[sizeof(slim.short_name) - 1] = '\0';
+                        strncpy(slim.long_name, legacy.user.long_name, sizeof(slim.long_name));
-                    slim.hw_model = legacy.user.hw_model;
+                        slim.long_name[sizeof(slim.long_name) - 1] = '\0';
-                    slim.role = legacy.user.role;
+                        strncpy(slim.short_name, legacy.user.short_name, sizeof(slim.short_name));
-                    if (legacy.user.is_licensed)
+                        slim.short_name[sizeof(slim.short_name) - 1] = '\0';
-                        slim.bitfield |= NODEINFO_BITFIELD_IS_LICENSED_MASK;
+                        slim.hw_model = legacy.user.hw_model;
-                    slim.public_key.size = legacy.user.public_key.size;
+                        slim.role = legacy.user.role;
-                    memcpy(slim.public_key.bytes, legacy.user.public_key.bytes, sizeof(slim.public_key.bytes));
+                        if (legacy.user.is_licensed)
-                    if (legacy.user.has_is_unmessagable) {
+                            slim.bitfield |= NODEINFO_BITFIELD_IS_LICENSED_MASK;
-                        slim.bitfield |= NODEINFO_BITFIELD_HAS_IS_UNMESSAGABLE_MASK;
+                        slim.public_key.size = legacy.user.public_key.size;
-                        if (legacy.user.is_unmessagable)
+                        memcpy(slim.public_key.bytes, legacy.user.public_key.bytes, sizeof(slim.public_key.bytes));
-                            slim.bitfield |= NODEINFO_BITFIELD_IS_UNMESSAGABLE_MASK;
+                        if (legacy.user.has_is_unmessagable) {
+                            slim.bitfield |= NODEINFO_BITFIELD_HAS_IS_UNMESSAGABLE_MASK;
+                            if (legacy.user.is_unmessagable)
+                                slim.bitfield |= NODEINFO_BITFIELD_IS_UNMESSAGABLE_MASK;
+                        }
+                        // macaddr deprecated since 1.2.11 - dropped from slim header.
                     }
-                    // macaddr deprecated since 1.2.11 - dropped from slim header.
+                    nodeDatabase.nodes.push_back(slim);
-                }
-                nodeDatabase.nodes.push_back(slim);
 #if !MESHTASTIC_EXCLUDE_POSITIONDB
-                if (legacy.has_position)
+                    if (legacy.has_position)
-                    nodePositions[legacy.num] = legacy.position;
+                        nodePositions[legacy.num] = legacy.position;
 #endif
 #if !MESHTASTIC_EXCLUDE_TELEMETRYDB
-                if (legacy.has_device_metrics)
+                    if (legacy.has_device_metrics)
-                    nodeTelemetry[legacy.num] = legacy.device_metrics;
+                        nodeTelemetry[legacy.num] = legacy.device_metrics;
+#endif
+                }
+#if !MESHTASTIC_EXCLUDE_POSITIONDB
+                posCount = nodePositions.size();
+#endif
+#if !MESHTASTIC_EXCLUDE_TELEMETRYDB
+                telCount = nodeTelemetry.size();
 #endif
             }
             nodeDatabase.version = DEVICESTATE_CUR_VER;
             meshNodes = &nodeDatabase.nodes;
             numMeshNodes = nodeDatabase.nodes.size();
-            size_t posCount = 0, telCount = 0;
-#if !MESHTASTIC_EXCLUDE_POSITIONDB
-            posCount = nodePositions.size();
-#endif
-#if !MESHTASTIC_EXCLUDE_TELEMETRYDB
-            telCount = nodeTelemetry.size();
-#endif
             LOG_INFO("Migrated %u nodes from legacy -> v%u (positions: %u, telemetry: %u)", (unsigned)numMeshNodes,
                      DEVICESTATE_CUR_VER, (unsigned)posCount, (unsigned)telCount);
             saveNodeDatabaseToDisk();
@@ -1532,6 +1668,7 @@ void NodeDB::loadFromDisk()
         numMeshNodes = nodeDatabase.nodes.size();
         // Hydrate the satellite maps; the on-disk vectors stay empty in steady
         // state and are repopulated only at save time.
+        concurrency::LockGuard guard(&satelliteMutex);
 #if !MESHTASTIC_EXCLUDE_POSITIONDB
         nodePositions.clear();
         nodePositions.reserve(nodeDatabase.positions.size());
@@ -1857,6 +1994,7 @@ bool NodeDB::saveNodeDatabaseToDisk()
 #endif
     // Project the maps into the on-disk vectors just before encoding; cleared
     // again on the way out so we don't carry duplicate state.
+    concurrency::LockGuard guard(&satelliteMutex);
 #if !MESHTASTIC_EXCLUDE_POSITIONDB
     nodeDatabase.positions.clear();
     nodeDatabase.positions.reserve(nodePositions.size());
@@ -2140,37 +2278,40 @@ void NodeDB::updatePosition(uint32_t nodeId, const meshtastic_Position &p, RxSou
     updateGUIforNode = info;
     notifyObservers(true);
 #else
-    meshtastic_PositionLite &slot = nodePositions[nodeId]; // creates default-zero entry if missing
+    {
+        concurrency::LockGuard guard(&satelliteMutex);
+        meshtastic_PositionLite &slot = nodePositions[nodeId]; // creates default-zero entry if missing
 
-    if (src == RX_SRC_LOCAL) {
+        if (src == RX_SRC_LOCAL) {
-        // Local packet, fully authoritative
+            // Local packet, fully authoritative
-        LOG_INFO("updatePosition LOCAL pos@%x time=%u lat=%d lon=%d alt=%d", p.timestamp, p.time, p.latitude_i, p.longitude_i,
+            LOG_INFO("updatePosition LOCAL pos@%x time=%u lat=%d lon=%d alt=%d", p.timestamp, p.time, p.latitude_i,
-                 p.altitude);
+                     p.longitude_i, p.altitude);
 
-        setLocalPosition(p);
+            setLocalPosition(p);
-        slot = TypeConversions::ConvertToPositionLite(p);
+            slot = TypeConversions::ConvertToPositionLite(p);
-    } else if ((p.time > 0) && !p.latitude_i && !p.longitude_i && !p.timestamp && !p.location_source) {
+        } else if ((p.time > 0) && !p.latitude_i && !p.longitude_i && !p.timestamp && !p.location_source) {
-        // FIXME SPECIAL TIME SETTING PACKET FROM EUD TO RADIO
+            // FIXME SPECIAL TIME SETTING PACKET FROM EUD TO RADIO
-        // (stop-gap fix for issue #900)
+            // (stop-gap fix for issue #900)
-        LOG_DEBUG("updatePosition SPECIAL time setting time=%u", p.time);
+            LOG_DEBUG("updatePosition SPECIAL time setting time=%u", p.time);
-        slot.time = p.time;
+            slot.time = p.time;
-    } else {
+        } else {
-        // Be careful to only update fields that have been set by the REMOTE sender
+            // Be careful to only update fields that have been set by the REMOTE sender
-        // A lot of position reports don't have time populated.  In that case, be careful to not blow away the time we
+            // A lot of position reports don't have time populated.  In that case, be careful to not blow away the time we
-        // recorded based on the packet rxTime
+            // recorded based on the packet rxTime
-        //
+            //
-        // FIXME perhaps handle RX_SRC_USER separately?
+            // FIXME perhaps handle RX_SRC_USER separately?
-        LOG_INFO("updatePosition REMOTE node=0x%x time=%u lat=%d lon=%d", nodeId, p.time, p.latitude_i, p.longitude_i);
+            LOG_INFO("updatePosition REMOTE node=0x%x time=%u lat=%d lon=%d", nodeId, p.time, p.latitude_i, p.longitude_i);
 
-        // First, back up fields that we want to protect from overwrite
+            // First, back up fields that we want to protect from overwrite
-        uint32_t tmp_time = slot.time;
+            uint32_t tmp_time = slot.time;
 
-        // Next, update atomically
+            // Next, update atomically
-        slot = TypeConversions::ConvertToPositionLite(p);
+            slot = TypeConversions::ConvertToPositionLite(p);
 
-        // Last, restore any fields that may have been overwritten
+            // Last, restore any fields that may have been overwritten
-        if (!slot.time)
+            if (!slot.time)
-            slot.time = tmp_time;
+                slot.time = tmp_time;
+        }
     }
     updateGUIforNode = info;
     notifyObservers(true); // Force an update whether or not our node counts have changed
@@ -2191,11 +2332,13 @@ void NodeDB::updateTelemetry(uint32_t nodeId, const meshtastic_Telemetry &t, RxS
     if (t.which_variant == meshtastic_Telemetry_device_metrics_tag) {
         LOG_DEBUG(src == RX_SRC_LOCAL ? "updateTelemetry LOCAL device" : "updateTelemetry REMOTE device node=0x%x", nodeId);
 #if !MESHTASTIC_EXCLUDE_TELEMETRYDB
+        concurrency::LockGuard guard(&satelliteMutex);
         nodeTelemetry[nodeId] = t.variant.device_metrics;
 #endif
     } else if (t.which_variant == meshtastic_Telemetry_environment_metrics_tag) {
         LOG_DEBUG(src == RX_SRC_LOCAL ? "updateTelemetry LOCAL env" : "updateTelemetry REMOTE env node=0x%x", nodeId);
 #if !MESHTASTIC_EXCLUDE_ENVIRONMENTDB
+        concurrency::LockGuard guard(&satelliteMutex);
         nodeEnvironment[nodeId] = t.variant.environment_metrics;
 #endif
     } else {

src/mesh/NodeDB.h

@@ -11,6 +11,7 @@
 
 #include "MeshTypes.h"
 #include "NodeStatus.h"
+#include "concurrency/Lock.h"
 #include "configuration.h"
 #include "mesh-pb-constants.h"
 #include "mesh/generated/meshtastic/mesh.pb.h" // For CriticalErrorCode
@@ -300,8 +301,17 @@ class NodeDB
     const meshtastic_DeviceMetrics *getNodeTelemetry(NodeNum n) const;
     const meshtastic_EnvironmentMetrics *getNodeEnvironment(NodeNum n) const;
     const meshtastic_StatusMessage *getNodeStatus(NodeNum n) const;
+    bool copyNodePosition(NodeNum n, meshtastic_PositionLite &out) const;
+    bool copyNodeTelemetry(NodeNum n, meshtastic_DeviceMetrics &out) const;
+    bool copyNodeEnvironment(NodeNum n, meshtastic_EnvironmentMetrics &out) const;
+    bool copyNodeStatus(NodeNum n, meshtastic_StatusMessage &out) const;
+    std::vector<NodeNum> snapshotPositionNodeNums(NodeNum exclude) const;
+    std::vector<NodeNum> snapshotTelemetryNodeNums(NodeNum exclude) const;
+    std::vector<NodeNum> snapshotEnvironmentNodeNums(NodeNum exclude) const;
+    std::vector<NodeNum> snapshotStatusNodeNums(NodeNum exclude) const;
 
     void setNodeStatus(NodeNum n, const meshtastic_StatusMessage &status);
+    void touchNodePositionTime(NodeNum n, uint32_t time);
 
     bool hasNodePosition(NodeNum n) const { return getNodePosition(n) != nullptr; }
     bool hasNodeTelemetry(NodeNum n) const { return getNodeTelemetry(n) != nullptr; }
@@ -366,6 +376,7 @@ class NodeDB
     }
 
   private:
+    mutable concurrency::Lock satelliteMutex;
     bool duplicateWarned = false;
     bool localPositionUpdatedSinceBoot = false;
     uint32_t lastNodeDbSave = 0;    // when we last saved our db to flash

src/mesh/PhoneAPI.cpp

@@ -871,13 +871,7 @@ void PhoneAPI::beginReplayPositions()
     // Snapshot the keyset at phase start so concurrent inserts/erases on the
     // map don't invalidate iteration. Skip our own node - the phone already
     // got our position bundled in STATE_SEND_OWN_NODEINFO.
-    replayPositionOrder.clear();
+    replayPositionOrder = nodeDB->snapshotPositionNodeNums(nodeDB->getNodeNum());
-    NodeNum ourNum = nodeDB->getNodeNum();
-    replayPositionOrder.reserve(nodeDB->nodePositions.size());
-    for (const auto &kv : nodeDB->nodePositions) {
-        if (kv.first != ourNum)
-            replayPositionOrder.push_back(kv.first);
-    }
     replayPositionIndex = 0;
     LOG_INFO("Begin position replay: %u entries millis=%u", (unsigned)replayPositionOrder.size(), millis());
 #endif
@@ -897,10 +891,10 @@ void PhoneAPI::prefetchReplayPositions()
         wasEmpty = replayQueue.empty();
         while (replayQueue.size() < kReplayPrefetchDepth && replayPositionIndex < replayPositionOrder.size()) {
             NodeNum num = replayPositionOrder[replayPositionIndex++];
-            const meshtastic_PositionLite *pos = nodeDB->getNodePosition(num);
+            meshtastic_PositionLite pos;
-            if (!pos)
+            if (!nodeDB->copyNodePosition(num, pos))
                 continue; // entry was evicted between snapshot and now
-            replayQueue.push_back(makeReplayPositionPacket(num, *pos));
+            replayQueue.push_back(makeReplayPositionPacket(num, pos));
             added = true;
         }
     }
@@ -920,13 +914,7 @@ void PhoneAPI::beginReplayTelemetry()
         replayTelemetryIndex = 0;
         return;
     }
-    replayTelemetryOrder.clear();
+    replayTelemetryOrder = nodeDB->snapshotTelemetryNodeNums(nodeDB->getNodeNum());
-    NodeNum ourNum = nodeDB->getNodeNum();
-    replayTelemetryOrder.reserve(nodeDB->nodeTelemetry.size());
-    for (const auto &kv : nodeDB->nodeTelemetry) {
-        if (kv.first != ourNum)
-            replayTelemetryOrder.push_back(kv.first);
-    }
     replayTelemetryIndex = 0;
     LOG_INFO("Begin telemetry replay: %u entries millis=%u", (unsigned)replayTelemetryOrder.size(), millis());
 #endif
@@ -946,10 +934,10 @@ void PhoneAPI::prefetchReplayTelemetry()
         wasEmpty = replayQueue.empty();
         while (replayQueue.size() < kReplayPrefetchDepth && replayTelemetryIndex < replayTelemetryOrder.size()) {
             NodeNum num = replayTelemetryOrder[replayTelemetryIndex++];
-            const meshtastic_DeviceMetrics *dm = nodeDB->getNodeTelemetry(num);
+            meshtastic_DeviceMetrics dm;
-            if (!dm)
+            if (!nodeDB->copyNodeTelemetry(num, dm))
                 continue;
-            replayQueue.push_back(makeReplayTelemetryPacket(num, *dm));
+            replayQueue.push_back(makeReplayTelemetryPacket(num, dm));
             added = true;
         }
     }
@@ -989,13 +977,7 @@ void PhoneAPI::beginReplayEnvironment()
         replayEnvironmentIndex = 0;
         return;
     }
-    replayEnvironmentOrder.clear();
+    replayEnvironmentOrder = nodeDB->snapshotEnvironmentNodeNums(nodeDB->getNodeNum());
-    NodeNum ourNum = nodeDB->getNodeNum();
-    replayEnvironmentOrder.reserve(nodeDB->nodeEnvironment.size());
-    for (const auto &kv : nodeDB->nodeEnvironment) {
-        if (kv.first != ourNum)
-            replayEnvironmentOrder.push_back(kv.first);
-    }
     replayEnvironmentIndex = 0;
     LOG_INFO("Begin environment replay: %u entries millis=%u", (unsigned)replayEnvironmentOrder.size(), millis());
 #endif
@@ -1015,10 +997,10 @@ void PhoneAPI::prefetchReplayEnvironment()
         wasEmpty = replayQueue.empty();
         while (replayQueue.size() < kReplayPrefetchDepth && replayEnvironmentIndex < replayEnvironmentOrder.size()) {
             NodeNum num = replayEnvironmentOrder[replayEnvironmentIndex++];
-            const meshtastic_EnvironmentMetrics *env = nodeDB->getNodeEnvironment(num);
+            meshtastic_EnvironmentMetrics env;
-            if (!env)
+            if (!nodeDB->copyNodeEnvironment(num, env))
                 continue;
-            replayQueue.push_back(makeReplayEnvironmentPacket(num, *env));
+            replayQueue.push_back(makeReplayEnvironmentPacket(num, env));
             added = true;
         }
     }
@@ -1055,13 +1037,7 @@ void PhoneAPI::beginReplayStatus()
         replayStatusIndex = 0;
         return;
     }
-    replayStatusOrder.clear();
+    replayStatusOrder = nodeDB->snapshotStatusNodeNums(nodeDB->getNodeNum());
-    NodeNum ourNum = nodeDB->getNodeNum();
-    replayStatusOrder.reserve(nodeDB->nodeStatus.size());
-    for (const auto &kv : nodeDB->nodeStatus) {
-        if (kv.first != ourNum)
-            replayStatusOrder.push_back(kv.first);
-    }
     replayStatusIndex = 0;
     LOG_INFO("Begin status replay: %u entries millis=%u", (unsigned)replayStatusOrder.size(), millis());
 #endif
@@ -1081,10 +1057,10 @@ void PhoneAPI::prefetchReplayStatus()
         wasEmpty = replayQueue.empty();
         while (replayQueue.size() < kReplayPrefetchDepth && replayStatusIndex < replayStatusOrder.size()) {
             NodeNum num = replayStatusOrder[replayStatusIndex++];
-            const meshtastic_StatusMessage *status = nodeDB->getNodeStatus(num);
+            meshtastic_StatusMessage status;
-            if (!status || status->status[0] == '\0')
+            if (!nodeDB->copyNodeStatus(num, status) || status.status[0] == '\0')
                 continue;
-            replayQueue.push_back(makeReplayStatusPacket(num, *status));
+            replayQueue.push_back(makeReplayStatusPacket(num, status));
             added = true;
         }
     }

src/mesh/ProtobufModule.h

@@ -58,7 +58,7 @@ template <class T> class ProtobufModule : protected SinglePortModule
     const char *getSenderShortName(const meshtastic_MeshPacket &mp)
     {
         auto node = nodeDB->getMeshNode(getFrom(&mp));
-        const char *sender = (node) ? node->short_name : "???";
+        const char *sender = (node && nodeInfoLiteHasUser(node) && node->short_name[0]) ? node->short_name : "???";
         return sender;
     }
 
@@ -121,4 +121,4 @@ template <class T> class ProtobufModule : protected SinglePortModule
             return alterReceivedProtobuf(mp, decoded);
         }
     }
-};
+};

src/modules/KeyVerificationModule.cpp

@@ -10,6 +10,17 @@
 
 KeyVerificationModule *keyVerificationModule;
 
+namespace {
+void copyNodeLongNameOrUnknown(char *dest, size_t destSize, const meshtastic_NodeInfoLite *node)
+{
+    if (!dest || destSize == 0)
+        return;
+    const char *name = (node && nodeInfoLiteHasUser(node) && node->long_name[0]) ? node->long_name : "Unknown";
+    strncpy(dest, name, destSize);
+    dest[destSize - 1] = '\0';
+}
+} // namespace
+
 KeyVerificationModule::KeyVerificationModule()
     : ProtobufModule("KeyVerification", meshtastic_PortNum_KEY_VERIFICATION_APP, &meshtastic_KeyVerification_msg)
 {
@@ -37,7 +48,8 @@ AdminMessageHandleResult KeyVerificationModule::handleAdminMessageForModule(cons
         } else if (request->key_verification.message_type == meshtastic_KeyVerificationAdmin_MessageType_DO_VERIFY &&
                    request->key_verification.nonce == currentNonce) {
             auto remoteNodePtr = nodeDB->getMeshNode(currentRemoteNode);
-            remoteNodePtr->bitfield |= NODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK;
+            if (remoteNodePtr)
+                remoteNodePtr->bitfield |= NODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK;
             resetToIdle();
         } else if (request->key_verification.message_type == meshtastic_KeyVerificationAdmin_MessageType_DO_NOT_VERIFY) {
             resetToIdle();
@@ -72,9 +84,9 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket &
         sprintf(cn->message, "Enter Security Number for Key Verification");
         cn->which_payload_variant = meshtastic_ClientNotification_key_verification_number_request_tag;
         cn->payload_variant.key_verification_number_request.nonce = currentNonce;
-        strncpy(cn->payload_variant.key_verification_number_request.remote_longname, // should really check for nulls, etc
+        copyNodeLongNameOrUnknown(cn->payload_variant.key_verification_number_request.remote_longname,
-                nodeDB->getMeshNode(currentRemoteNode)->long_name,
+                                  sizeof(cn->payload_variant.key_verification_number_request.remote_longname),
-                sizeof(cn->payload_variant.key_verification_number_request.remote_longname));
+                                  nodeDB->getMeshNode(currentRemoteNode));
         service->sendClientNotification(cn);
         LOG_INFO("Received hash2");
         currentState = KEY_VERIFICATION_SENDER_AWAITING_NUMBER;
@@ -95,7 +107,8 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket &
                           [=](int selected) {
                               if (selected == 1) {
                                   auto remoteNodePtr = nodeDB->getMeshNode(currentRemoteNode);
-                                  remoteNodePtr->bitfield |= NODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK;
+                                  if (remoteNodePtr)
+                                      remoteNodePtr->bitfield |= NODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK;
                               }
                           };
                       screen->showOverlayBanner(options);)
@@ -104,9 +117,9 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket &
             sprintf(cn->message, "Final confirmation for incoming manual key verification %s", message);
             cn->which_payload_variant = meshtastic_ClientNotification_key_verification_final_tag;
             cn->payload_variant.key_verification_final.nonce = currentNonce;
-            strncpy(cn->payload_variant.key_verification_final.remote_longname, // should really check for nulls, etc
+            copyNodeLongNameOrUnknown(cn->payload_variant.key_verification_final.remote_longname,
-                    nodeDB->getMeshNode(currentRemoteNode)->long_name,
+                                      sizeof(cn->payload_variant.key_verification_final.remote_longname),
-                    sizeof(cn->payload_variant.key_verification_final.remote_longname));
+                                      nodeDB->getMeshNode(currentRemoteNode));
             cn->payload_variant.key_verification_final.isSender = false;
             service->sendClientNotification(cn);
 
@@ -202,9 +215,9 @@ meshtastic_MeshPacket *KeyVerificationModule::allocReply()
             currentSecurityNumber % 1000);
     cn->which_payload_variant = meshtastic_ClientNotification_key_verification_number_inform_tag;
     cn->payload_variant.key_verification_number_inform.nonce = currentNonce;
-    strncpy(cn->payload_variant.key_verification_number_inform.remote_longname, // should really check for nulls, etc
+    copyNodeLongNameOrUnknown(cn->payload_variant.key_verification_number_inform.remote_longname,
-            nodeDB->getMeshNode(currentRemoteNode)->long_name,
+                              sizeof(cn->payload_variant.key_verification_number_inform.remote_longname),
-            sizeof(cn->payload_variant.key_verification_number_inform.remote_longname));
+                              nodeDB->getMeshNode(currentRemoteNode));
     cn->payload_variant.key_verification_number_inform.security_number = currentSecurityNumber;
     service->sendClientNotification(cn);
     LOG_WARN("Security Number %04u, nonce %llu", currentSecurityNumber, currentNonce);
@@ -219,7 +232,7 @@ void KeyVerificationModule::processSecurityNumber(uint32_t incomingNumber)
     LOG_WARN("received security number: %u", incomingNumber);
     meshtastic_NodeInfoLite *remoteNodePtr = nullptr;
     remoteNodePtr = nodeDB->getMeshNode(currentRemoteNode);
-    if (!nodeInfoLiteHasUser(remoteNodePtr) || remoteNodePtr->public_key.size != 32) {
+    if (!remoteNodePtr || !nodeInfoLiteHasUser(remoteNodePtr) || remoteNodePtr->public_key.size != 32) {
         currentState = KEY_VERIFICATION_IDLE;
         return; // should we throw an error here?
     }
@@ -265,9 +278,9 @@ void KeyVerificationModule::processSecurityNumber(uint32_t incomingNumber)
     sprintf(cn->message, "Final confirmation for outgoing manual key verification %s", message);
     cn->which_payload_variant = meshtastic_ClientNotification_key_verification_final_tag;
     cn->payload_variant.key_verification_final.nonce = currentNonce;
-    strncpy(cn->payload_variant.key_verification_final.remote_longname, // should really check for nulls, etc
+    copyNodeLongNameOrUnknown(cn->payload_variant.key_verification_final.remote_longname,
-            nodeDB->getMeshNode(currentRemoteNode)->long_name,
+                              sizeof(cn->payload_variant.key_verification_final.remote_longname),
-            sizeof(cn->payload_variant.key_verification_final.remote_longname));
+                              nodeDB->getMeshNode(currentRemoteNode));
     cn->payload_variant.key_verification_final.isSender = true;
     service->sendClientNotification(cn);
     LOG_INFO(message);
@@ -310,4 +323,4 @@ void KeyVerificationModule::generateVerificationCode(char *readableCode)
         readableCode[i] = (hash1[i] >> 2) + 48; // not a standardized base64, but workable and avoids having a dictionary.
     }
 }
-#endif
+#endif

src/modules/RangeTestModule.cpp

@@ -270,7 +270,7 @@ bool RangeTestModuleRadio::appendFile(const meshtastic_MeshPacket &mp)
 
     fileToAppend.printf("%d,", getFrom(&mp));          // From
     fileToAppend.printf("%s,", n ? n->long_name : ""); // Long Name
-    const meshtastic_PositionLite *senderPos = nodeDB->getNodePosition(n->num);
+    const meshtastic_PositionLite *senderPos = nodeDB->getNodePosition(getFrom(&mp));
     if (senderPos) {
         fileToAppend.printf("%f,", senderPos->latitude_i * 1e-7);  // Sender Lat
         fileToAppend.printf("%f,", senderPos->longitude_i * 1e-7); // Sender Long
@@ -349,4 +349,4 @@ bool RangeTestModuleRadio::removeFile()
 
     return 0;
 #endif
-}
+}

src/serialization/MeshPacketSerializer.cpp

@@ -321,8 +321,10 @@ std::string MeshPacketSerializer::JsonSerialize(const meshtastic_MeshPacket *mp,
                         char long_name[40] = "Unknown";
                         meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(num);
                         bool name_known = nodeInfoLiteHasUser(node);
-                        if (name_known)
+                        if (name_known) {
-                            memcpy(long_name, node->long_name, sizeof(long_name));
+                            memcpy(long_name, node->long_name, sizeof(node->long_name));
+                            long_name[sizeof(node->long_name)] = '\0';
+                        }
                         route->push_back(new JSONValue(long_name));
                     };
                     addToRoute(&route, mp->to); // Started at the original transmitter (destination of response)
@@ -473,4 +475,4 @@ std::string MeshPacketSerializer::JsonSerializeEncrypted(const meshtastic_MeshPa
     delete value;
     return jsonStr;
 }
-#endif
+#endif

src/serialization/MeshPacketSerializer_nRF52.cpp

@@ -293,8 +293,10 @@ std::string MeshPacketSerializer::JsonSerialize(const meshtastic_MeshPacket *mp,
                         char long_name[40] = "Unknown";
                         meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(num);
                         bool name_known = nodeInfoLiteHasUser(node);
-                        if (name_known)
+                        if (name_known) {
-                            memcpy(long_name, node->long_name, sizeof(long_name));
+                            memcpy(long_name, node->long_name, sizeof(node->long_name));
+                            long_name[sizeof(node->long_name)] = '\0';
+                        }
                         route->add(long_name);
                     };
 
@@ -418,4 +420,4 @@ std::string MeshPacketSerializer::JsonSerializeEncrypted(const meshtastic_MeshPa
 
     return jsonStr;
 }
-#endif
+#endif