fix: stop unexpected NodeNum regeneration from PKI key loss (#10808)

* fix: stop unexpected NodeNum regeneration from PKI key loss

A node's NodeNum is derived from its PKI public key
(my_node_num == crc32(public_key)), so it changes only when the keypair is
regenerated -- which happens only when a keygen path runs while
config.security.private_key.size != 32. Two paths could trigger that without
the user intending a new identity:

1. Boot: loadFromDisk() collapsed every non-success loadProto() result for the
   config file into installDefaultConfig() (preserveKey=false), which memset()s
   config and wipes the private key. loadProto() distinguishes DECODE_FAILED
   (file present but undecodable/undecryptable) from OTHER_FAILURE (absent), so
   a transient/corrupt read silently minted a new identity. A new
   configDecodeFailed flag now gates the boot keygen and the boot config-save
   directly (not via region, so it survives the userprefs/region overrides that
   run later in loadFromDisk): identity is frozen, the unreadable file is left
   intact for a later clean boot to recover, and the node boots radio-silent
   (region UNSET, TX off). A genuinely-absent config still gets defaults + keygen.

2. AdminModule security SET: config.security = c.payload_variant.security
   wholesale-clobbered the keypair, then regenerated whenever the incoming
   private_key.size != 32. A client editing an unrelated security field without
   round-tripping the private key would re-mint identity. The existing keypair
   is now preserved when the incoming config omits it; first provisioning and
   key import are unchanged. Intentional reset goes through factory_reset.

Native PlatformIO unit suite (Docker): 499/499 test cases pass.

* test: cover security keypair preservation; clarify load-failure comment

Addresses PR review feedback:
- Add native unit tests (test_admin_radio) asserting handleSetConfig preserves the
  existing identity keypair when a security SET omits the private key, and applies a
  full supplied keypair (key import). Guards against reintroduced NodeNum/keypair
  regeneration. AdminModuleTestShim (now a friend) defers saves so the lightweight
  harness doesn't saveToDisk an uninitialized node database.
- Clarify the non-DECODE_FAILED config-load comment: OTHER_FAILURE / NO_FILESYSTEM
  cover an absent or unopenable file, not just first boot.

src/mesh/NodeDB.cpp

@@ -480,8 +480,10 @@ NodeDB::NodeDB()
 #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI)
     // Generate crypto keys if needed using consolidated function
     // Set my node num uint32 value to bytes from the public key (if we have one)
-    // Generate identity and crypto keys if needed; this will create a new identity if one does not exist
+    // Generate identity and crypto keys if needed; this will create a new identity if one does not exist.
-    generateCryptoKeyPair(nullptr);
+    // Skip on a degraded boot: the keypair isn't in RAM, so minting one would change our NodeNum.
+    if (!configDecodeFailed)
+        generateCryptoKeyPair(nullptr);
 #elif !(MESHTASTIC_EXCLUDE_PKI)
     // Calculate Curve25519 public and private keys
     if (config.security.private_key.size == 32 && config.security.public_key.size == 32) {
@@ -625,7 +627,9 @@ NodeDB::NodeDB()
         saveWhat |= SEGMENT_DEVICESTATE;
     if (nodeDatabaseCRC != crc32Buffer(&nodeDatabase, sizeof(nodeDatabase)))
         saveWhat |= SEGMENT_NODEDATABASE;
-    if (configCRC != crc32Buffer(&config, sizeof(config)))
+    // Don't persist on a degraded boot: it would overwrite the unreadable-but-maybe-transient config file
+    // with no-key UNSET defaults. Runtime reconfiguration (admin set) still persists normally.
+    if (!configDecodeFailed && configCRC != crc32Buffer(&config, sizeof(config)))
         saveWhat |= SEGMENT_CONFIG;
     if (channelFileCRC != crc32Buffer(&channelFile, sizeof(channelFile)))
         saveWhat |= SEGMENT_CHANNELS;
@@ -2120,6 +2124,7 @@ void NodeDB::loadFromDisk()
 #endif
 
     migrationSavePending = false;
+    configDecodeFailed = false;
 
     meshtastic_Config_SecurityConfig backupSecurity = meshtastic_Config_SecurityConfig_init_zero;
 
@@ -2309,15 +2314,26 @@ void NodeDB::loadFromDisk()
 
     state = loadProto(configFileName, meshtastic_LocalConfig_size, sizeof(meshtastic_LocalConfig), &meshtastic_LocalConfig_msg,
                       &config);
-    if (state != LoadFileResult::LOAD_SUCCESS) {
+    if (state == LoadFileResult::DECODE_FAILED) {
-        installDefaultConfig(); // Our in RAM copy might now be corrupt
+        // Config file present but undecodable this boot (corruption / torn write / transient decrypt fail).
+        // loadProto() already zeroed `config`, so the keypair is gone from RAM; minting a new one would change
+        // our NodeNum (== crc32(public_key)) and orphan us on the mesh. configDecodeFailed freezes identity and
+        // skips persisting (see ctor), so a transient failure self-heals on the next clean boot. A genuinely
+        // absent config returns OTHER_FAILURE, so this never fires on first boot. Boot degraded + radio-silent.
+        LOG_ERROR("Config decode failed - freezing identity, booting degraded (radio silent until restored)");
+        configDecodeFailed = true;
+        installDefaultConfig(true);
+        config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_UNSET;
+        config.lora.tx_enabled = false;
+    } else if (state != LoadFileResult::LOAD_SUCCESS) {
+        // No decodable config to work with: the file is absent (first boot) or could not be opened (OTHER_FAILURE
+        // / NO_FILESYSTEM). Unlike DECODE_FAILED there are no usable contents to protect, so install defaults.
+        installDefaultConfig();
+    } else if (config.version < DEVICESTATE_MIN_VER) {
+        LOG_WARN("config %d is old, discard", config.version);
+        installDefaultConfig(true);
     } else {
-        if (config.version < DEVICESTATE_MIN_VER) {
+        LOG_INFO("Loaded saved config version %d", config.version);
-            LOG_WARN("config %d is old, discard", config.version);
-            installDefaultConfig(true);
-        } else {
-            LOG_INFO("Loaded saved config version %d", config.version);
-        }
     }
 
     // Coerce LoRa config fields derived from presets while bootstrapping.
@@ -2334,7 +2350,9 @@ void NodeDB::loadFromDisk()
     // take effect even when NVS already has a valid config (e.g. region-locked
     // dev boards with no BLE/serial to set the region at runtime).
 #ifdef USERPREFS_CONFIG_LORA_REGION
-    config.lora.region = USERPREFS_CONFIG_LORA_REGION;
+    // Skip on a degraded boot to keep the radio silent (identity is already protected by the keygen gate).
+    if (!configDecodeFailed)
+        config.lora.region = USERPREFS_CONFIG_LORA_REGION;
 #endif
 
 #ifdef USERPREFS_LORACONFIG_USE_PRESET

src/mesh/NodeDB.h

@@ -506,6 +506,10 @@ class NodeDB
     bool duplicateWarned = false;
     bool localPositionUpdatedSinceBoot = false;
     bool migrationSavePending = false;
+    /// Set when loadFromDisk() hit a present-but-undecodable config (DECODE_FAILED). The ctor uses it to
+    /// skip boot keygen and skip persisting defaults, so a transient read failure can't change our NodeNum
+    /// or overwrite the on-disk config. Cleared at the top of every loadFromDisk() run.
+    bool configDecodeFailed = false;
     uint32_t lastNodeDbSave = 0;    // when we last saved our db to flash
     uint32_t lastBackupAttempt = 0; // when we last tried a backup automatically or manually
     uint32_t lastSort = 0;          // When last sorted the nodeDB

src/modules/AdminModule.cpp

@@ -1037,16 +1037,24 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
         config.has_bluetooth = true;
         config.bluetooth = c.payload_variant.bluetooth;
         break;
-    case meshtastic_Config_security_tag:
+    case meshtastic_Config_security_tag: {
         LOG_INFO("Set config: Security");
-        config.security = c.payload_variant.security;
+        meshtastic_Config_SecurityConfig incoming = c.payload_variant.security;
+        // Preserve our keypair when a SET omits the private key but we already hold one: regenerating would
+        // change our NodeNum (== crc32(public_key)) and orphan us on the mesh. A SET without the key is a
+        // partial/legacy client, not an identity reset (that goes through factory_reset). Done outside the
+        // PKI guard so non-PKI builds keep their key bytes too.
+        if (incoming.private_key.size != 32 && config.security.private_key.size == 32) {
+            LOG_WARN("Security set omitted private key; preserving existing identity keypair");
+            incoming.private_key = config.security.private_key;
+            incoming.public_key = config.security.public_key;
+        }
+        config.security = incoming;
 #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN) && !(MESHTASTIC_EXCLUDE_PKI)
-        // Only regenerate keys if the private key is not 32 bytes
+        // First provisioning (no key) generates one; a private key supplied without its public key derives it.
         if (config.security.private_key.size != 32) {
             nodeDB->generateCryptoKeyPair();
-        }
+        } else if (config.security.public_key.size == 0) {
-        // If user provided a private key of correct size but no public key, generate the public key from private key
-        else if (config.security.private_key.size == 32 && config.security.public_key.size == 0) {
             nodeDB->generateCryptoKeyPair(config.security.private_key.bytes);
         }
 #endif
@@ -1063,6 +1071,7 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
         requiresReboot = true;
 
         break;
+    }
     case meshtastic_Config_device_ui_tag:
         // NOOP! This is handled by handleStoreDeviceUIConfig
         break;

src/modules/AdminModule.h

@@ -22,6 +22,8 @@ struct AdminModule_ObserverData {
  */
 class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Observable<AdminModule_ObserverData *>
 {
+    friend class AdminModuleTestShim; // native unit tests reach handleSetConfig + hasOpenEditTransaction
+
   public:
     /** Constructor
      * name is for debugging output

test/test_admin_radio/test_main.cpp

@@ -933,6 +933,9 @@ class AdminModuleTestShim : public AdminModule
 {
   public:
     using AdminModule::handleSetConfig;
+    // Defer persistence so handleSetConfig() exercises the in-RAM config logic without saveToDisk() touching
+    // the (uninitialized) node database in this lightweight harness.
+    void deferSaves() { hasOpenEditTransaction = true; }
 };
 
 static AdminModuleTestShim *testAdmin;
@@ -1028,6 +1031,64 @@ static void test_handleSetConfig_fromOthers_invalidChannelNumFullyRejected()
     TEST_ASSERT_EQUAL_UINT32(0, config.lora.channel_num);
 }
 
+// A security-config SET that omits the private key (partial/legacy client editing some other security field)
+// must NOT regenerate our keypair: our NodeNum is crc32(public_key), so a new keypair would silently change
+// our identity. The existing keypair has to be preserved.
+static void test_handleSetConfig_security_preservesKeypairWhenPrivateOmitted()
+{
+    config.security = meshtastic_Config_SecurityConfig_init_zero;
+    config.security.private_key.size = 32;
+    memset(config.security.private_key.bytes, 0x11, 32);
+    config.security.public_key.size = 32;
+    memset(config.security.public_key.bytes, 0x22, 32);
+
+    // Incoming SET carries no private/public key, just another security field.
+    meshtastic_Config c = meshtastic_Config_init_zero;
+    c.which_payload_variant = meshtastic_Config_security_tag;
+    c.payload_variant.security.serial_enabled = true;
+
+    testAdmin->deferSaves();
+    testAdmin->handleSetConfig(c, false);
+
+    uint8_t expectedPriv[32];
+    memset(expectedPriv, 0x11, 32);
+    uint8_t expectedPub[32];
+    memset(expectedPub, 0x22, 32);
+    TEST_ASSERT_EQUAL_UINT(32, config.security.private_key.size);
+    TEST_ASSERT_EQUAL_MEMORY(expectedPriv, config.security.private_key.bytes, 32);
+    TEST_ASSERT_EQUAL_UINT(32, config.security.public_key.size);
+    TEST_ASSERT_EQUAL_MEMORY(expectedPub, config.security.public_key.bytes, 32);
+    // The non-key field still applies.
+    TEST_ASSERT_TRUE(config.security.serial_enabled);
+}
+
+// A SET that DOES supply a full 32-byte keypair (legitimate key import) must apply it, not preserve the old one.
+static void test_handleSetConfig_security_acceptsSuppliedKeypair()
+{
+    config.security = meshtastic_Config_SecurityConfig_init_zero;
+    config.security.private_key.size = 32;
+    memset(config.security.private_key.bytes, 0x11, 32);
+    config.security.public_key.size = 32;
+    memset(config.security.public_key.bytes, 0x22, 32);
+
+    meshtastic_Config c = meshtastic_Config_init_zero;
+    c.which_payload_variant = meshtastic_Config_security_tag;
+    c.payload_variant.security.private_key.size = 32;
+    memset(c.payload_variant.security.private_key.bytes, 0x33, 32);
+    c.payload_variant.security.public_key.size = 32;
+    memset(c.payload_variant.security.public_key.bytes, 0x44, 32);
+
+    testAdmin->deferSaves();
+    testAdmin->handleSetConfig(c, false);
+
+    uint8_t expectedPriv[32];
+    memset(expectedPriv, 0x33, 32);
+    uint8_t expectedPub[32];
+    memset(expectedPub, 0x44, 32);
+    TEST_ASSERT_EQUAL_MEMORY(expectedPriv, config.security.private_key.bytes, 32);
+    TEST_ASSERT_EQUAL_MEMORY(expectedPub, config.security.public_key.bytes, 32);
+}
+
 static void test_regionInfo_supportsPreset()
 {
     const RegionInfo *eu868 = getRegion(meshtastic_Config_LoRaConfig_RegionCode_EU_868);
@@ -1197,6 +1258,8 @@ void setup()
     RUN_TEST(test_handleSetConfig_fromLocal_invalidPresetClamped);
     RUN_TEST(test_handleSetConfig_fromOthers_validPresetAccepted);
     RUN_TEST(test_handleSetConfig_fromOthers_invalidChannelNumFullyRejected);
+    RUN_TEST(test_handleSetConfig_security_preservesKeypairWhenPrivateOmitted);
+    RUN_TEST(test_handleSetConfig_security_acceptsSuppliedKeypair);
     RUN_TEST(test_regionInfo_supportsPreset);
     RUN_TEST(test_checkConfigRegion_quietCheckReportsReason);
     RUN_TEST(test_handleSetConfig_fromOthers_siblingLockedPresetSwapsRegion);