Internal mock node clean up (#3715)

* InMemoryMessagingNetwork.InMemoryMessaging renamed to MockNodeMessagingService and moved to internal package
* start method added to MockNodeMessagingService which enables AbstractNode to call makeMessagingService in its c'tor
* Removed TopicStringValidator as it's no longer used
* Clean up of TestStartedNode
* Merged InMemoryMessagingTests into InternalMockNetworkTests as it's testing InternalMockNetwork

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

@@ -107,15 +107,12 @@ import net.corda.core.crypto.generateKeyPair as cryptoGenerateKeyPair
  * sweeping up the Node into the Kryo checkpoint serialization via any flows holding a reference to ServiceHub.
  */
 // TODO Log warning if this node is a notary but not one of the ones specified in the network parameters, both for core and custom
-
-// In theory the NodeInfo for the node should be passed in, instead, however currently this is constructed by the
-// AbstractNode. It should be possible to generate the NodeInfo outside of AbstractNode, so it can be passed in.
 abstract class AbstractNode<S>(val configuration: NodeConfiguration,
-                                             val platformClock: CordaClock,
-                                             protected val versionInfo: VersionInfo,
-                                             protected val cordappLoader: CordappLoader,
-                                             protected val serverThread: AffinityExecutor.ServiceAffinityExecutor,
-                                             private val busyNodeLatch: ReusableLatch = ReusableLatch()) : SingletonSerializeAsToken() {
+                               val platformClock: CordaClock,
+                               protected val versionInfo: VersionInfo,
+                               protected val cordappLoader: CordappLoader,
+                               protected val serverThread: AffinityExecutor.ServiceAffinityExecutor,
+                               private val busyNodeLatch: ReusableLatch = ReusableLatch()) : SingletonSerializeAsToken() {
 
     protected abstract val log: Logger
 
@@ -180,6 +177,8 @@ abstract class AbstractNode<S>(val configuration: NodeConfiguration,
     val transactionVerifierService = InMemoryTransactionVerifierService(transactionVerifierWorkerCount).tokenize()
     val contractUpgradeService = ContractUpgradeServiceImpl().tokenize()
     val auditService = DummyAuditService().tokenize()
+    @Suppress("LeakingThis")
+    protected val network: MessagingService = makeMessagingService().tokenize()
     val services = ServiceHubInternalImpl().tokenize()
     @Suppress("LeakingThis")
     val smm = makeStateMachineManager()
@@ -194,8 +193,6 @@ abstract class AbstractNode<S>(val configuration: NodeConfiguration,
             configuration.drainingModePollPeriod,
             unfinishedSchedules = busyNodeLatch
     ).tokenize().closeOnStop()
-    // TODO Making this non-lateinit requires MockNode being able to create a blank InMemoryMessaging instance
-    protected lateinit var network: MessagingService
 
     private val cordappServices = MutableClassToInstanceMap.create<SerializeAsToken>()
     private val flowFactories = ConcurrentHashMap<Class<out FlowLogic<*>>, InitiatedFlowFactory<*>>()
@@ -286,10 +283,6 @@ abstract class AbstractNode<S>(val configuration: NodeConfiguration,
         }
         log.info("Node starting up ...")
 
-        // TODO First thing we do is create the MessagingService. This should have been done by the c'tor but it's not
-        // possible (yet) to due restriction from MockNode
-        network = makeMessagingService().tokenize()
-
         val trustRoot = initKeyStore()
         val nodeCa = configuration.loadNodeKeyStore().getCertificate(X509Utilities.CORDA_CLIENT_CA)
         initialiseJVMAgents()

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

@@ -4,7 +4,6 @@ import com.codahale.metrics.JmxReporter
 import net.corda.client.rpc.internal.serialization.amqp.AMQPClientSerializationScheme
 import net.corda.core.concurrent.CordaFuture
 import net.corda.core.flows.FlowLogic
-import net.corda.core.flows.InitiatedBy
 import net.corda.core.identity.CordaX500Name
 import net.corda.core.identity.PartyAndCertificate
 import net.corda.core.internal.Emoji
@@ -13,7 +12,6 @@ import net.corda.core.internal.concurrent.thenMatch
 import net.corda.core.internal.div
 import net.corda.core.internal.errors.AddressBindingException
 import net.corda.core.internal.notary.NotaryService
-import net.corda.node.services.api.StartedNodeServices
 import net.corda.core.messaging.CordaRPCOps
 import net.corda.core.messaging.RPCOps
 import net.corda.core.node.NetworkParameters
@@ -39,6 +37,7 @@ import net.corda.node.serialization.kryo.KryoServerSerializationScheme
 import net.corda.node.services.Permissions
 import net.corda.node.services.api.FlowStarter
 import net.corda.node.services.api.ServiceHubInternal
+import net.corda.node.services.api.StartedNodeServices
 import net.corda.node.services.config.NodeConfiguration
 import net.corda.node.services.config.SecurityConfiguration
 import net.corda.node.services.config.shouldInitCrashShell
@@ -58,6 +57,7 @@ import net.corda.serialization.internal.*
 import org.h2.jdbc.JdbcSQLException
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
+import rx.Observable
 import rx.Scheduler
 import rx.schedulers.Schedulers
 import java.net.BindException
@@ -66,7 +66,6 @@ import java.time.Clock
 import java.util.concurrent.atomic.AtomicInteger
 import javax.management.ObjectName
 import kotlin.system.exitProcess
-import rx.Observable
 
 class NodeWithInfo(val node: Node, val info: NodeInfo) {
     val services: StartedNodeServices = object : StartedNodeServices, ServiceHubInternal by node.services, FlowStarter by node.flowStarter {}
@@ -195,7 +194,7 @@ open class Node(configuration: NodeConfiguration,
     override fun startMessagingService(rpcOps: RPCOps, nodeInfo: NodeInfo, myNotaryIdentity: PartyAndCertificate?, networkParameters: NetworkParameters) {
         require(nodeInfo.legalIdentities.size in 1..2) { "Currently nodes must have a primary address and optionally one serviced address" }
 
-        val client = network as P2PMessagingClient
+        network as P2PMessagingClient
 
         // Construct security manager reading users data either from the 'security' config section
         // if present or from rpcUsers list if the former is missing from config.
@@ -219,7 +218,7 @@ open class Node(configuration: NodeConfiguration,
             startLocalRpcBroker(securityManager)
         }
 
-        val bridgeControlListener = BridgeControlListener(configuration, client.serverAddress, networkParameters.maxMessageSize)
+        val bridgeControlListener = BridgeControlListener(configuration, network.serverAddress, networkParameters.maxMessageSize)
 
         printBasicNodeInfo("Advertised P2P messaging addresses", nodeInfo.addresses.joinToString())
         val rpcServerConfiguration = RPCServerConfiguration.DEFAULT
@@ -248,8 +247,8 @@ open class Node(configuration: NodeConfiguration,
             closeOnStop()
             init(rpcOps, securityManager)
         }
-        client.closeOnStop()
-        client.start(
+        network.closeOnStop()
+        network.start(
                 myIdentity = nodeInfo.legalIdentities[0].owningKey,
                 serviceIdentity = if (nodeInfo.legalIdentities.size == 1) null else nodeInfo.legalIdentities[1].owningKey,
                 advertisedAddress = nodeInfo.addresses[0],

node/src/main/kotlin/net/corda/node/services/messaging/Messaging.kt

@@ -146,13 +146,6 @@ interface ReceivedMessage : Message {
     val isSessionInit: Boolean
 }
 
-/** A singleton that's useful for validating topic strings */
-object TopicStringValidator {
-    private val regex = "[a-zA-Z0-9.]+".toPattern()
-    /** @throws IllegalArgumentException if the given topic contains invalid characters */
-    fun check(tag: String) = require(regex.matcher(tag).matches())
-}
-
 /**
  * This handler is used to implement exactly-once delivery of an external event on top of an at-least-once delivery. This is done
  * using two hooks that are called from the event processor, one called from the database transaction committing the

node/src/main/kotlin/net/corda/node/services/statemachine/SingleThreadedStateMachineManager.kt

@@ -101,7 +101,7 @@ class SingleThreadedStateMachineManager(
     private val flowMessaging: FlowMessaging = FlowMessagingImpl(serviceHub)
     private val fiberDeserializationChecker = if (serviceHub.configuration.shouldCheckCheckpoints()) FiberDeserializationChecker() else null
     private val transitionExecutor = makeTransitionExecutor()
-    private val ourSenderUUID get() = serviceHub.networkService.ourSenderUUID // This is a getter since AbstractNode.network is still lateinit
+    private val ourSenderUUID = serviceHub.networkService.ourSenderUUID
 
     private var checkpointSerializationContext: SerializationContext? = null
     private var actionExecutor: ActionExecutor? = null

node/src/test/kotlin/net/corda/node/messaging/InMemoryMessagingTests.kt

@@ -1,115 +0,0 @@
-package net.corda.node.messaging
-
-import net.corda.core.messaging.AllPossibleRecipients
-import net.corda.node.services.messaging.Message
-import net.corda.node.services.messaging.TopicStringValidator
-import net.corda.testing.internal.rigorousMock
-import net.corda.testing.node.internal.InternalMockNetwork
-import org.junit.After
-import org.junit.Before
-import org.junit.Test
-import java.util.*
-import kotlin.test.assertEquals
-import kotlin.test.assertFails
-import kotlin.test.assertTrue
-
-class InMemoryMessagingTests {
-    lateinit var mockNet: InternalMockNetwork
-
-    @Before
-    fun setUp() {
-        mockNet = InternalMockNetwork()
-    }
-
-    @After
-    fun tearDown() {
-        mockNet.stopNodes()
-    }
-
-    @Test
-    fun `topic string validation`() {
-        TopicStringValidator.check("this.is.ok")
-        TopicStringValidator.check("this.is.OkAlso")
-        assertFails {
-            TopicStringValidator.check("this.is.not-ok")
-        }
-        assertFails {
-            TopicStringValidator.check("")
-        }
-        assertFails {
-            TopicStringValidator.check("this.is not ok")   // Spaces
-        }
-    }
-
-    @Test
-    fun basics() {
-        val node1 = mockNet.createNode()
-        val node2 = mockNet.createNode()
-        val node3 = mockNet.createNode()
-
-        val bits = "test-content".toByteArray()
-        var finalDelivery: Message? = null
-        node2.network.addMessageHandler("test.topic") { msg, _, _ ->
-            node2.network.send(msg, node3.network.myAddress)
-        }
-        node3.network.addMessageHandler("test.topic") { msg, _, _ ->
-            finalDelivery = msg
-        }
-
-        // Node 1 sends a message and it should end up in finalDelivery, after we run the network
-        node1.network.send(node1.network.createMessage("test.topic", data = bits), node2.network.myAddress)
-
-        mockNet.runNetwork(rounds = 1)
-
-        assertTrue(Arrays.equals(finalDelivery!!.data.bytes, bits))
-    }
-
-    @Test
-    fun broadcast() {
-        val node1 = mockNet.createNode()
-        val node2 = mockNet.createNode()
-        val node3 = mockNet.createNode()
-
-        val bits = "test-content".toByteArray()
-
-        var counter = 0
-        listOf(node1, node2, node3).forEach { it.network.addMessageHandler("test.topic") { _, _, _ -> counter++ } }
-        node1.network.send(node2.network.createMessage("test.topic", data = bits), rigorousMock<AllPossibleRecipients>())
-        mockNet.runNetwork(rounds = 1)
-        assertEquals(3, counter)
-    }
-
-    /**
-     * Tests that unhandled messages in the received queue are skipped and the next message processed, rather than
-     * causing processing to return null as if there was no message.
-     */
-    @Test
-    fun `skip unhandled messages`() {
-        val node1 = mockNet.createNode()
-        val node2 = mockNet.createNode()
-        var received = 0
-
-        node1.network.addMessageHandler("valid_message") { _, _, _ ->
-            received++
-        }
-
-        val invalidMessage = node2.network.createMessage("invalid_message", data = ByteArray(1))
-        val validMessage = node2.network.createMessage("valid_message", data = ByteArray(1))
-        node2.network.send(invalidMessage, node1.network.myAddress)
-        mockNet.runNetwork()
-        assertEquals(0, received)
-
-        node2.network.send(validMessage, node1.network.myAddress)
-        mockNet.runNetwork()
-        assertEquals(1, received)
-
-        // Here's the core of the test; previously the unhandled message would cause runNetwork() to abort early, so
-        // this would fail. Make fresh messages to stop duplicate uniqueMessageId causing drops
-        val invalidMessage2 = node2.network.createMessage("invalid_message", data = ByteArray(1))
-        val validMessage2 = node2.network.createMessage("valid_message", data = ByteArray(1))
-        node2.network.send(invalidMessage2, node1.network.myAddress)
-        node2.network.send(validMessage2, node1.network.myAddress)
-        mockNet.runNetwork()
-        assertEquals(2, received)
-    }
-}

node/src/test/kotlin/net/corda/node/messaging/TwoPartyTradeFlowTests.kt

@@ -47,7 +47,6 @@ import net.corda.testing.internal.LogHelper
 import net.corda.testing.internal.TEST_TX_TIME
 import net.corda.testing.internal.rigorousMock
 import net.corda.testing.internal.vault.VaultFiller
-import net.corda.testing.node.InMemoryMessagingNetwork
 import net.corda.testing.node.MockServices
 import net.corda.testing.node.internal.*
 import net.corda.testing.node.ledger
@@ -225,7 +224,7 @@ class TwoPartyTradeFlowTests(private val anonymous: Boolean) {
             aliceNode.internals.disableDBCloseOnStop()
             bobNode.internals.disableDBCloseOnStop()
 
-            val bobAddr = bobNode.network.myAddress as InMemoryMessagingNetwork.PeerHandle
+            val bobAddr = bobNode.network.myAddress
             mockNet.runNetwork() // Clear network map registration messages
 
             val notary = mockNet.defaultNotaryIdentity

node/src/test/kotlin/net/corda/node/services/statemachine/FlowFrameworkTests.kt

@@ -821,7 +821,7 @@ private inline fun <reified P : FlowLogic<*>> TestStartedNode.registerFlowFactor
         initiatingFlowClass: KClass<out FlowLogic<*>>,
         initiatedFlowVersion: Int = 1,
         noinline flowFactory: (FlowSession) -> P): CordaFuture<P> {
-    val observable = internalRegisterFlowFactory(
+    val observable = registerFlowFactory(
             initiatingFlowClass.java,
             InitiatedFlowFactory.CorDapp(initiatedFlowVersion, "", flowFactory),
             P::class.java,

node/src/test/kotlin/net/corda/node/services/statemachine/RetryFlowMockTest.kt

@@ -15,11 +15,6 @@ import net.corda.node.services.FinalityHandler
 import net.corda.node.services.messaging.Message
 import net.corda.node.services.persistence.DBTransactionStorage
 import net.corda.nodeapi.internal.persistence.contextTransaction
-import net.corda.testing.node.internal.cordappsForPackages
-import net.corda.testing.node.internal.InternalMockNetwork
-import net.corda.testing.node.internal.MessagingServiceSpy
-import net.corda.testing.node.internal.newContext
-import net.corda.testing.node.internal.setMessagingServiceSpy
 import net.corda.testing.node.internal.*
 import org.assertj.core.api.Assertions.assertThat
 import org.assertj.core.api.Assertions.assertThatThrownBy
@@ -79,7 +74,7 @@ class RetryFlowMockTest {
     fun `Retry does not set senderUUID`() {
         val messagesSent = Collections.synchronizedList(mutableListOf<Message>())
         val partyB = nodeB.info.legalIdentities.first()
-        nodeA.setMessagingServiceSpy(object : MessagingServiceSpy(nodeA.network) {
+        nodeA.setMessagingServiceSpy(object : MessagingServiceSpy() {
             override fun send(message: Message, target: MessageRecipients, sequenceKey: Any) {
                 messagesSent.add(message)
                 messagingService.send(message, target)
@@ -95,7 +90,7 @@ class RetryFlowMockTest {
     fun `Restart does not set senderUUID`() {
         val messagesSent = Collections.synchronizedList(mutableListOf<Message>())
         val partyB = nodeB.info.legalIdentities.first()
-        nodeA.setMessagingServiceSpy(object : MessagingServiceSpy(nodeA.network) {
+        nodeA.setMessagingServiceSpy(object : MessagingServiceSpy() {
             override fun send(message: Message, target: MessageRecipients, sequenceKey: Any) {
                 messagesSent.add(message)
                 messagingService.send(message, target)
@@ -109,7 +104,7 @@ class RetryFlowMockTest {
         assertNotNull(messagesSent.first().senderUUID)
         nodeA = mockNet.restartNode(nodeA)
         // This is a bit racy because restarting the node actually starts it, so we need to make sure there's enough iterations we get here with flow still going.
-        nodeA.setMessagingServiceSpy(object : MessagingServiceSpy(nodeA.network) {
+        nodeA.setMessagingServiceSpy(object : MessagingServiceSpy() {
             override fun send(message: Message, target: MessageRecipients, sequenceKey: Any) {
                 messagesSent.add(message)
                 messagingService.send(message, target)
@@ -117,7 +112,7 @@ class RetryFlowMockTest {
         })
         // Now short circuit the iterations so the flow finishes soon.
         KeepSendingFlow.count.set(count - 2)
-        while (nodeA.smm.allStateMachines.size > 0) {
+        while (nodeA.smm.allStateMachines.isNotEmpty()) {
             Thread.sleep(10)
         }
         assertNull(messagesSent.last().senderUUID)

node/src/test/kotlin/net/corda/node/services/transactions/ValidatingNotaryServiceTests.kt

@@ -4,18 +4,8 @@ import net.corda.core.concurrent.CordaFuture
 import net.corda.core.contracts.Command
 import net.corda.core.contracts.StateAndRef
 import net.corda.core.contracts.StateRef
-import net.corda.core.crypto.Crypto
-import net.corda.core.crypto.SecureHash
-import net.corda.core.crypto.TransactionSignature
-import net.corda.core.crypto.generateKeyPair
-import net.corda.core.crypto.sha256
-import net.corda.core.crypto.sign
-import net.corda.core.flows.NotarisationPayload
-import net.corda.core.flows.NotarisationRequest
-import net.corda.core.flows.NotarisationRequestSignature
-import net.corda.core.flows.NotaryError
-import net.corda.core.flows.NotaryException
-import net.corda.core.flows.NotaryFlow
+import net.corda.core.crypto.*
+import net.corda.core.flows.*
 import net.corda.core.identity.Party
 import net.corda.core.internal.notary.generateSignature
 import net.corda.core.messaging.MessageRecipients
@@ -35,13 +25,6 @@ import net.corda.testing.core.ALICE_NAME
 import net.corda.testing.core.dummyCommand
 import net.corda.testing.core.singleIdentity
 import net.corda.testing.node.TestClock
-import net.corda.testing.node.internal.cordappsForPackages
-import net.corda.testing.node.internal.InMemoryMessage
-import net.corda.testing.node.internal.InternalMockNetwork
-import net.corda.testing.node.internal.InternalMockNodeParameters
-import net.corda.testing.node.internal.MessagingServiceSpy
-import net.corda.testing.node.internal.setMessagingServiceSpy
-import net.corda.testing.node.internal.startFlow
 import net.corda.testing.node.internal.*
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.After
@@ -308,7 +291,7 @@ class ValidatingNotaryServiceTests {
     }
 
     private fun runNotarisationAndInterceptClientPayload(payloadModifier: (NotarisationPayload) -> NotarisationPayload) {
-        aliceNode.setMessagingServiceSpy(object : MessagingServiceSpy(aliceNode.network) {
+        aliceNode.setMessagingServiceSpy(object : MessagingServiceSpy() {
             override fun send(message: Message, target: MessageRecipients, sequenceKey: Any) {
                 val messageData = message.data.deserialize<Any>() as? InitialSessionMessage
                 val payload = messageData?.firstPayload!!.deserialize()
@@ -318,7 +301,6 @@ class ValidatingNotaryServiceTests {
                     val alteredMessageData = messageData.copy(firstPayload = alteredPayload.serialize())
                     val alteredMessage = InMemoryMessage(message.topic, OpaqueBytes(alteredMessageData.serialize().bytes), message.uniqueMessageId)
                     messagingService.send(alteredMessage, target)
-
                 } else {
                     messagingService.send(message, target)
                 }

node/src/test/kotlin/net/corda/node/services/vault/VaultSoftLockManagerTest.kt

@@ -66,7 +66,7 @@ class NodePair(private val mockNet: InternalMockNetwork) {
         private set
 
     fun <T> communicate(clientLogic: AbstractClientLogic<T>, rebootClient: Boolean): FlowStateMachine<T> {
-        server.internalRegisterFlowFactory(AbstractClientLogic::class.java, InitiatedFlowFactory.Core { ServerLogic(it, serverRunning) }, ServerLogic::class.java, false)
+        server.registerFlowFactory(AbstractClientLogic::class.java, InitiatedFlowFactory.Core { ServerLogic(it, serverRunning) }, ServerLogic::class.java, false)
         client.services.startFlow(clientLogic)
         while (!serverRunning.get()) mockNet.runNetwork(1)
         if (rebootClient) {