ENT-2014 Deletes of NodeInfo can fail to propagate leading to infinite retries (#1101)

* ENT-2014 Deletes of NodeInfo can fail to propagate leading to infinite retries
ENT-1880 Move identity key generation to network registration process

node/src/main/kotlin/net/corda/node/internal/NodeStartup.kt

@@ -328,6 +328,13 @@ open class NodeStartup(val args: Array<String>) {
         println("*                                                                *")
         println("******************************************************************")
         NodeRegistrationHelper(conf, HTTPNetworkRegistrationService(compatibilityZoneURL), nodeRegistrationConfig).buildKeystore()
+
+        // Minimal changes to make registration tool create node identity.
+        // TODO: Move node identity generation logic from node to registration helper.
+        createNode(conf, getVersionInfo()).generateAndSaveNodeInfo()
+
+        println("Successfully registered Corda node with compatibility zone, node identity keys and certificates are stored in '${conf.certificatesDirectory}', it is advised to backup the private keys and certificates.")
+        println("Corda node will now terminate.")
     }
 
     protected open fun loadConfigFile(cmdlineOptions: CmdLineOptions): Pair<Config, Try<NodeConfiguration>> = cmdlineOptions.loadConfig()

node/src/main/kotlin/net/corda/node/services/network/NetworkMapUpdater.kt

@@ -124,6 +124,15 @@ class NetworkMapUpdater(private val networkMapCache: NetworkMapCacheInternal,
         }
 
         val currentNodeHashes = networkMapCache.allNodeHashes
+
+        // Remove node info from network map.
+        (currentNodeHashes - allHashesFromNetworkMap - fileWatcher.processedNodeInfoHashes)
+                .mapNotNull {
+                    if (it != ourNodeInfoHash) {
+                        networkMapCache.getNodeByHash(it)
+                    } else null
+                }.forEach(networkMapCache::removeNode)
+
         (allHashesFromNetworkMap - currentNodeHashes).mapNotNull {
             // Download new node info from network map
             try {
@@ -138,14 +147,6 @@ class NetworkMapUpdater(private val networkMapCache: NetworkMapCacheInternal,
             networkMapCache.addNode(it)
         }
 
-        // Remove node info from network map.
-        (currentNodeHashes - allHashesFromNetworkMap - fileWatcher.processedNodeInfoHashes)
-                .mapNotNull {
-                    if (it != ourNodeInfoHash) {
-                        networkMapCache.getNodeByHash(it)
-                    } else null
-                }.forEach(networkMapCache::removeNode)
-
         return cacheTimeout
     }
 

node/src/main/kotlin/net/corda/node/services/network/PersistentNetworkMapCache.kt

@@ -42,7 +42,6 @@ import org.hibernate.Session
 import rx.Observable
 import rx.subjects.PublishSubject
 import java.security.PublicKey
-import java.sql.Connection
 import java.util.*
 import javax.annotation.concurrent.ThreadSafe
 import kotlin.collections.HashSet
@@ -266,7 +265,8 @@ open class PersistentNetworkMapCache(
     }
 
     private fun removeInfoDB(session: Session, nodeInfo: NodeInfo) {
-        val info = findByIdentityKey(session, nodeInfo.legalIdentitiesAndCerts.first().owningKey).singleOrNull()
+        // findByIdentityKey might returns multiple node info with the same key, need to pick the right one by comparing serial.
+        val info = findByIdentityKey(session, nodeInfo.legalIdentitiesAndCerts.first().owningKey).singleOrNull { it.serial == nodeInfo.serial }
         info?.let { session.remove(it) }
         // invalidate cache last - this way, we might serve up the wrong info for a short time, but it will get refreshed
         // on the next load

node/src/main/kotlin/net/corda/node/utilities/registration/NetworkRegistrationHelper.kt

@@ -107,9 +107,6 @@ open class NetworkRegistrationHelper(private val config: SSLConfiguration,
         onSuccess(keyPair, certificates)
         // All done, clean up temp files.
         requestIdStore.deleteIfExists()
-
-        println("Successfully registered Corda node with compatibility zone, node identity keys and certificates are stored in '${config.certificatesDirectory}', it is advised to backup the private keys and certificates.")
-        println("Corda node will now terminate.")
     }
 
     private fun validateCertificates(registeringPublicKey: PublicKey, certificates: List<X509Certificate>) {

node/src/test/kotlin/net/corda/node/services/network/NetworkMapUpdaterTest.kt

@@ -26,7 +26,10 @@ import net.corda.core.serialization.serialize
 import net.corda.core.utilities.millis
 import net.corda.node.services.api.NetworkMapCacheInternal
 import net.corda.nodeapi.internal.NodeInfoAndSigned
-import net.corda.nodeapi.internal.network.*
+import net.corda.nodeapi.internal.network.NETWORK_PARAMS_UPDATE_FILE_NAME
+import net.corda.nodeapi.internal.network.NodeInfoFilesCopier
+import net.corda.nodeapi.internal.network.SignedNetworkParameters
+import net.corda.nodeapi.internal.network.verifiedNetworkMapCert
 import net.corda.testing.common.internal.testNetworkParameters
 import net.corda.testing.core.*
 import net.corda.testing.driver.PortAllocation
@@ -196,7 +199,7 @@ class NetworkMapUpdaterTest {
             sequence(
                     expect { update: ParametersUpdateInfo ->
                         assertEquals(update.updateDeadline, updateDeadline)
-                        assertEquals(update.description,"Test update")
+                        assertEquals(update.description, "Test update")
                         assertEquals(update.hash, newParameters.serialize().hash)
                         assertEquals(update.parameters, newParameters)
                     }
@@ -214,7 +217,7 @@ class NetworkMapUpdaterTest {
         Thread.sleep(2L * cacheExpiryMs)
         val newHash = newParameters.serialize().hash
         val keyPair = Crypto.generateKeyPair()
-        updater.acceptNewNetworkParameters(newHash, { hash -> hash.serialize().sign(keyPair)})
+        updater.acceptNewNetworkParameters(newHash, { hash -> hash.serialize().sign(keyPair) })
         val updateFile = baseDir / NETWORK_PARAMS_UPDATE_FILE_NAME
         val signedNetworkParams = updateFile.readObject<SignedNetworkParameters>()
         val paramsFromFile = signedNetworkParams.verifiedNetworkMapCert(DEV_ROOT_CA.certificate)
@@ -309,6 +312,38 @@ class NetworkMapUpdaterTest {
         assertThat(networkMapCache.allNodeHashes).containsExactlyInAnyOrder(signedMyInfo.raw.hash, signedOtherInfo.raw.hash)
     }
 
+    @Test
+    fun `network map updater removes the correct node info after node info changes`() {
+        setUpdater()
+
+        val builder = TestNodeInfoBuilder()
+
+        builder.addLegalIdentity(CordaX500Name("Test", "London", "GB"))
+
+        val signedNodeInfo1 = builder.buildWithSigned(1).signed
+        val signedNodeInfo2 = builder.buildWithSigned(2).signed
+
+        // Test adding new node.
+        networkMapClient.publish(signedNodeInfo1)
+        // Not subscribed yet.
+        verify(networkMapCache, times(0)).addNode(any())
+
+        updater.subscribeToNetworkMap()
+
+        // TODO: Remove sleep in unit test.
+        Thread.sleep(2L * cacheExpiryMs)
+        verify(networkMapCache, times(1)).addNode(signedNodeInfo1.verified())
+        assert(networkMapCache.allNodeHashes.size == 1)
+        networkMapClient.publish(signedNodeInfo2)
+        Thread.sleep(2L * cacheExpiryMs)
+        scheduler.advanceTimeBy(10, TimeUnit.SECONDS)
+
+        verify(networkMapCache, times(1)).addNode(signedNodeInfo2.verified())
+        verify(networkMapCache, times(1)).removeNode(signedNodeInfo1.verified())
+
+        assert(networkMapCache.allNodeHashes.size == 1)
+    }
+
     private fun createMockNetworkMapCache(): NetworkMapCacheInternal {
         return mock {
             val data = ConcurrentHashMap<Party, NodeInfo>()

testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/network/NetworkMapServer.kt

@@ -18,8 +18,6 @@ import net.corda.core.node.NetworkParameters
 import net.corda.core.node.NodeInfo
 import net.corda.core.serialization.serialize
 import net.corda.core.utilities.NetworkHostAndPort
-import net.corda.core.utilities.contextLogger
-import net.corda.core.utilities.days
 import net.corda.nodeapi.internal.SignedNodeInfo
 import net.corda.nodeapi.internal.createDevNetworkMapCa
 import net.corda.nodeapi.internal.crypto.CertificateAndKeyPair
@@ -138,7 +136,12 @@ class NetworkMapServer(private val pollInterval: Duration,
                 val hash = signedNodeInfo.raw.hash
                 val nodeInfo = signedNodeInfo.verified()
                 val privateNetwork = nodeNamesUUID[nodeInfo.legalIdentities[0].name]
-                networkMaps.computeIfAbsent(privateNetwork, { mutableSetOf() }).add(hash)
+                val map = networkMaps.computeIfAbsent(privateNetwork) { mutableSetOf() }
+                map.add(hash)
+                nodeInfoMap.filter { it.value.verified().legalIdentities.first().name == signedNodeInfo.verified().legalIdentities.first().name }.forEach { t, _ ->
+                    nodeInfoMap.remove(t)
+                    map.remove(t)
+                }
                 nodeInfoMap[hash] = signedNodeInfo
                 ok()
             } catch (e: Exception) {