fix(NodeDB): re-derive my_node_num when ensurePkiKeys() mints the identity keypair (#11426)

* fix(pki): re-derive NodeNum when setting a region mints the identity key

A node's mesh address is derived from its identity key:

    my_node_num == crc32Buffer(config.security.public_key.bytes, 32)

NodeDB::createNewIdentity() is what establishes that, and NodeDB::
generateCryptoKeyPair() is the only thing that called it.

CryptoEngine::ensurePkiKeys() generates or re-derives the keypair and writes
security.public_key, security.private_key and user.public_key - but never
re-derives my_node_num. Boot-time keygen is suppressed while the LoRa region is
UNSET (generateCryptoKeyPair()'s regionBlocksKeygen guard), so on a fresh device
my_node_num is still the MAC-derived value from pickNewNodeNum(). The user then
sets the region - the stock onboarding flow - ensurePkiKeys() mints a key, and
the invariant is broken.

The node then signs its broadcasts (Router.cpp signs when !pki_encrypted &&
(owner.is_licensed || isBroadcast(p->to))). Every receiver runs
verifyFirstContactNodeInfo, fails crc32Buffer(user.public_key) != p->from, and
drops the NodeInfo. The node's identity beacons are invisible to the mesh.

Nothing reboots to repair it: AdminModule sets requiresReboot = false for LoRa
changes ("All LoRa radio changes apply live via configChanged observer") and
MenuHandler ends at service->reloadConfig(changes).

Four call sites reached ensurePkiKeys():

  1. AdminModule set_config LORA, region first set   (phone app - the common path)
  2. MenuHandler applyLoraRegion                     (on-device region picker)
  3. InkHUD MenuApplet applyLoRaRegion               (schedules a reboot, so it
                                                      self-healed at next boot)
  4. portduino wasm wasm_set_region

The reference implementation was already in the tree: the *licensed* branch of
call site 1, thirteen lines below the broken unlicensed one, calls
nodeDB->generateCryptoKeyPair() (which reaches createNewIdentity()) and widens
the persisted mask with SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE.

Rather than repeat that at four call sites, the key-mint is routed through one
chokepoint that owns both halves of the identity: NodeDB::ensurePkiIdentity()
calls crypto->ensurePkiKeys() and then createNewIdentity(). It lives in NodeDB
because createNewIdentity() operates on the devicestate/node-DB globals, which
CryptoEngine deliberately does not touch - ensurePkiKeys() takes the security
config and user by reference precisely so it stays free of that dependency, and
it is unit-tested against a standalone CryptoEngine.

ensurePkiIdentity() returns true only when my_node_num actually moved
(createNewIdentity() early-returns when the key is unchanged, so a repeat region
change does not disturb the self entry or force a needless flash write). Callers
use that to widen their save mask; my_node_num lives in devicestate and the self
row moves in the node DB, so both segments must be persisted or the fix would
revert at the next boot. SEGMENT_CONFIG, which carries the key itself, is
already unconditional on all four paths.

The InkHUD reboot is left as-is. It is now redundant for this invariant, but it
covers the rest of that menu's behaviour and a redundant reboot is not a bug.

Adds test_handleSetConfig_persistsUnlicensedFirstRegionIdentity, the unlicensed
twin of the existing licensed test, asserting both the segment mask and
my_node_num == crc32(public_key).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* style(NodeDB): trim identity-recovery comments and guard the WASM nodeDB deref

Two review asks, no behaviour change on any built target.

Copilot flagged the unguarded nodeDB deref in the WASM region setter; it is the
only ensurePkiIdentity() call site that did not check the pointer first.

The rest is comment length. AGENTS.md:83 caps code comments at two lines, and the
identity-recovery comments across the four call sites plus the NodeDB.h doc block
ran to four and six lines. The rationale they carried is in the commit messages
and the PR body, which is where AGENTS.md says it belongs.

The PR's own fix in AdminModule.cpp is deliberately untouched.

* fix(NodeDB): keep the identity move authoritative when the self record cannot be created

createNewIdentity() removes the old node entry and assigns myNodeInfo.my_node_num
before it tries to create the row for the new number. If getOrCreateMeshNode()
came back null it returned false, so the first-region callers left
SEGMENT_DEVICESTATE and SEGMENT_NODEDATABASE out of the save mask.

The number had already moved in RAM at that point, and the freshly minted key
goes to flash under SEGMENT_CONFIG regardless. The next boot therefore reloads
the old number alongside the new key, which is exactly the
crc32(public_key) != my_node_num break this path exists to prevent, reached
through the error branch instead of the happy one.

Rolling the number back is not an option either, since the key has already been
replaced by the time this runs. So the move is now reported as the fact it is and
the missing self record is logged separately; getOrCreateMeshNode() will recreate
that row on the next contact. Reachable when the self record is absent and the
table is full of protected nodes.

Reported by CodeRabbit on #11426.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

src/graphics/draw/MenuHandler.cpp

@@ -245,8 +245,9 @@ static void applyLoraRegion(meshtastic_Config_LoRaConfig_RegionCode region, bool
     }
     auto changes = SEGMENT_CONFIG;
 #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI)
-    if (crypto) {
-        crypto->ensurePkiKeys(config.security, owner);
+    // Minting the key moves our node num with it, and nothing reboots on this path to repair it later.
+    if (nodeDB->ensurePkiIdentity()) {
+        changes |= SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE;
     }
 #endif
     initRegion();

src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp

@@ -324,8 +324,9 @@ static void applyLoRaRegion(meshtastic_Config_LoRaConfig_RegionCode region)
     auto changes = SEGMENT_CONFIG;
 
 #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI)
-    if (crypto) {
-        crypto->ensurePkiKeys(config.security, owner);
+    // Minting the key moves our node num with it, and the reboot below only re-derives after the save.
+    if (nodeDB->ensurePkiIdentity()) {
+        changes |= SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE;
     }
 #endif
 

src/mesh/NodeDB.cpp

@@ -4446,14 +4446,32 @@ bool NodeDB::createNewIdentity()
 
     myNodeInfo.my_node_num = newNodeNum;
 
+    // The number has moved, so the caller must persist it whatever happens next. Returning false here
+    // would leave the new key saved against the old number, which is the break this exists to prevent.
     meshtastic_NodeInfoLite *info = getOrCreateMeshNode(getNodeNum());
-    if (!info)
-        return false;
-    TypeConversions::CopyUserToNodeInfoLite(info, owner);
+    if (info)
+        TypeConversions::CopyUserToNodeInfoLite(info, owner);
+    else
+        LOG_ERROR("No room for our own node 0x%08x, identity moved without a self record", newNodeNum);
 
     return true;
 }
 
+bool NodeDB::ensurePkiIdentity()
+{
+#if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI)
+    // A failed or declined keygen leaves the existing key, and so the existing node num, untouched.
+    if (!crypto || !crypto->ensurePkiKeys(config.security, owner))
+        return false;
+
+    // ensurePkiKeys() writes key material only, so my_node_num is still the stale MAC-derived value.
+    // createNewIdentity() early-returns when the key, and so the node num, did not actually change.
+    return createNewIdentity();
+#else
+    return false;
+#endif
+}
+
 bool NodeDB::backupPreferences(meshtastic_AdminMessage_BackupLocation location)
 {
     bool success = false;

src/mesh/NodeDB.h

@@ -596,6 +596,10 @@ class NodeDB
 
     bool createNewIdentity();
 
+    /// Mint the identity keypair outside the boot path and re-seat my_node_num == crc32(public_key).
+    /// @return true if my_node_num moved; the caller must then also persist SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE.
+    bool ensurePkiIdentity();
+
     bool backupPreferences(meshtastic_AdminMessage_BackupLocation location);
     bool restorePreferences(meshtastic_AdminMessage_BackupLocation location,
                             int restoreWhat = SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_CHANNELS);

src/modules/AdminModule.cpp

@@ -1034,8 +1034,10 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
                 // If we're setting region for the first time, init the region and regenerate the keys
                 if (isRegionUnset && validatedLora.region > meshtastic_Config_LoRaConfig_RegionCode_UNSET) {
 #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI)
-                    if (crypto && !owner.is_licensed) {
-                        crypto->ensurePkiKeys(config.security, owner);
+                    // Minting the key moves our node num with it (my_node_num == crc32(public_key)), so
+                    // persist devicestate + the node DB too - exactly as the licensed branch below does.
+                    if (!owner.is_licensed && nodeDB->ensurePkiIdentity()) {
+                        changes |= SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE;
                     }
 #endif
                     // new region is valid and we're coming from an unset region, so enable tx

src/platform/portduino/wasm/portduino_glue_wasm.cpp

@@ -12,10 +12,9 @@
 //   - exec() short-circuits to "" (no popen/shell in the browser).
 // Downstream is unchanged: Ch341Hal -> libpinedio_webusb.c -> WebUSB.
 
-#include "CryptoEngine.h"   // crypto->ensurePkiKeys()
 #include "MeshRadio.h"      // initRegion()
 #include "MeshService.h"    // service->reloadConfig()
-#include "NodeDB.h"         // config, owner globals + SEGMENT_CONFIG
+#include "NodeDB.h"         // config globals, SEGMENT_*, nodeDB->ensurePkiIdentity()
 #include "PhoneAPI.h"       // the transport-agnostic client API seam
 #include "PortduinoFS.h"    // portduinoVFS
 #include "PortduinoGlue.h"  // declares `portduino_config` + Ch341Hal
@@ -260,11 +259,13 @@ extern "C" EMSCRIPTEN_KEEPALIVE int wasm_set_region(int region)
     if (!(RadioInterface::validateConfigRegion(validated) && RadioInterface::validateConfigLora(validated)))
         return -1;
 
+    int changes = SEGMENT_CONFIG;
     bool wasUnset = (config.lora.region == meshtastic_Config_LoRaConfig_RegionCode_UNSET);
     if (wasUnset && newRegion > meshtastic_Config_LoRaConfig_RegionCode_UNSET) {
 #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI)
-        if (crypto)
-            crypto->ensurePkiKeys(config.security, owner); // first real region -> generate keys
+        // Minting the key moves our node num with it, so persist devicestate + the node DB too.
+        if (nodeDB && nodeDB->ensurePkiIdentity())
+            changes |= SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE;
 #endif
         validated.tx_enabled = true;
     }
@@ -274,8 +275,8 @@ extern "C" EMSCRIPTEN_KEEPALIVE int wasm_set_region(int region)
     config.lora = validated;
     initRegion(); // repoint myRegion at the new region table
     if (service)
-        service->reloadConfig(SEGMENT_CONFIG); // reconfigure radio (new freq) + persist
-    wasm_fs_sync();                            // browser: flush config.proto to IndexedDB
+        service->reloadConfig(changes); // reconfigure radio (new freq) + persist
+    wasm_fs_sync();                     // browser: flush config.proto to IndexedDB
     return 0;
 }
 

test/test_admin_radio/test_main.cpp

@@ -23,6 +23,7 @@
 #include "mesh/Channels.h"
 #include "modules/AdminModule.h"
 #include "modules/NodeInfoModule.h"
+#include <ErriezCRC32.h> // crc32Buffer(), for the my_node_num == crc32(public_key) invariant
 #include <pb_decode.h>
 #include <pb_encode.h>
 #include <string>
@@ -1153,6 +1154,32 @@ static void test_handleSetConfig_persistsLicensedFirstRegionIdentity()
     TEST_ASSERT_EQUAL(32, owner.public_key.size);
 }
 
+// Unlicensed twin of the test above. Without the re-derivation the node signs broadcasts every receiver
+// drops (verifyFirstContactNodeInfo: crc32(user.public_key) != from).
+static void test_handleSetConfig_persistsUnlicensedFirstRegionIdentity()
+{
+    owner = meshtastic_User_init_zero;
+    owner.is_licensed = false;
+    config.security = meshtastic_Config_SecurityConfig_init_zero;
+    config.lora = meshtastic_Config_LoRaConfig_init_zero;
+    config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_UNSET;
+    initRegion();
+
+    testAdmin->deferSaves();
+    const meshtastic_Config c =
+        makeLoraSetConfig(meshtastic_Config_LoRaConfig_RegionCode_US, true, meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST);
+    testAdmin->handleSetConfig(c, false);
+
+    const int expectedSegments = SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE;
+    TEST_ASSERT_EQUAL_INT(expectedSegments, testAdmin->savedSegments());
+    TEST_ASSERT_EQUAL(32, config.security.private_key.size);
+    TEST_ASSERT_EQUAL(32, config.security.public_key.size);
+    TEST_ASSERT_EQUAL(32, owner.public_key.size);
+    // The invariant: a node's mesh address is derived from its identity key.
+    TEST_ASSERT_EQUAL_UINT32(crc32Buffer(config.security.public_key.bytes, config.security.public_key.size),
+                             nodeDB->getNodeNum());
+}
+
 static void test_handleSetConfig_fromOthers_invalidPresetRejected()
 {
     // Set up a known-good baseline in the global config
@@ -1975,6 +2002,7 @@ void setup()
     // getRegion()
     RUN_TEST(test_handleSetOwner_persistsLicensedChannelSanitation);
     RUN_TEST(test_handleSetConfig_persistsLicensedFirstRegionIdentity);
+    RUN_TEST(test_handleSetConfig_persistsUnlicensedFirstRegionIdentity);
     RUN_TEST(test_bootDefense_sanitizesStaleLicensedChannelsOnce);
     RUN_TEST(test_restorePreferences_sanitizesLicensedBackupBeforeReturn);
     RUN_TEST(test_getRegion_returnsCorrectRegion_US);