CORDA-1303 Regression: Recording a duplicate transaction attempts sec… (#2935)

* CORDA-1303 Regression: Recording a duplicate transaction attempts second insert to vault.

Added unit test, and merged mock and real logic.

node/src/main/kotlin/net/corda/node/services/api/ServiceHubInternal.kt

@@ -47,28 +47,12 @@ interface NetworkMapCacheBaseInternal : NetworkMapCacheBase {
 interface ServiceHubInternal : ServiceHub {
     companion object {
         private val log = contextLogger()
-    }
 
-    override val vaultService: VaultServiceInternal
+        fun recordTransactions(statesToRecord: StatesToRecord, txs: Iterable<SignedTransaction>,
-    /**
+                               validatedTransactions: WritableTransactionStorage,
-     * A map of hash->tx where tx has been signature/contract validated and the states are known to be correct.
+                               stateMachineRecordedTransactionMapping: StateMachineRecordedTransactionMappingStorage,
-     * The signatures aren't technically needed after that point, but we keep them around so that we can relay
+                               vaultService: VaultServiceInternal) {
-     * the transaction data to other nodes that need it.
+
-     */
-    override val validatedTransactions: WritableTransactionStorage
-    val stateMachineRecordedTransactionMapping: StateMachineRecordedTransactionMappingStorage
-    val monitoringService: MonitoringService
-    val schemaService: SchemaService
-    override val networkMapCache: NetworkMapCacheInternal
-    val auditService: AuditService
-    val rpcFlows: List<Class<out FlowLogic<*>>>
-    val networkService: MessagingService
-    val database: CordaPersistence
-    val configuration: NodeConfiguration
-    val nodeProperties: NodePropertiesStore
-    val networkMapUpdater: NetworkMapUpdater
-    override val cordappProvider: CordappProviderInternal
-    override fun recordTransactions(statesToRecord: StatesToRecord, txs: Iterable<SignedTransaction>) {
             require(txs.any()) { "No transactions passed in for recording" }
             val recordedTransactions = txs.filter { validatedTransactions.addTransaction(it) }
             val stateMachineRunId = FlowStateMachineImpl.currentStateMachine()?.id
@@ -115,9 +99,33 @@ interface ServiceHubInternal : ServiceHub {
                 //
                 // Because the primary use case for recording irrelevant states is observer/regulator nodes, who are unlikely
                 // to make writes to the ledger very often or at all, we choose to punt this issue for the time being.
-            vaultService.notifyAll(statesToRecord, txs.map { it.coreTransaction })
+                vaultService.notifyAll(statesToRecord, recordedTransactions.map { it.coreTransaction })
             }
         }
+    }
+
+    override val vaultService: VaultServiceInternal
+    /**
+     * A map of hash->tx where tx has been signature/contract validated and the states are known to be correct.
+     * The signatures aren't technically needed after that point, but we keep them around so that we can relay
+     * the transaction data to other nodes that need it.
+     */
+    override val validatedTransactions: WritableTransactionStorage
+    val stateMachineRecordedTransactionMapping: StateMachineRecordedTransactionMappingStorage
+    val monitoringService: MonitoringService
+    val schemaService: SchemaService
+    override val networkMapCache: NetworkMapCacheInternal
+    val auditService: AuditService
+    val rpcFlows: List<Class<out FlowLogic<*>>>
+    val networkService: MessagingService
+    val database: CordaPersistence
+    val configuration: NodeConfiguration
+    val nodeProperties: NodePropertiesStore
+    val networkMapUpdater: NetworkMapUpdater
+    override val cordappProvider: CordappProviderInternal
+    override fun recordTransactions(statesToRecord: StatesToRecord, txs: Iterable<SignedTransaction>) {
+        recordTransactions(statesToRecord, txs, validatedTransactions, stateMachineRecordedTransactionMapping, vaultService)
+    }
 
     fun getFlowFactory(initiatingFlowClass: Class<out FlowLogic<*>>): InitiatedFlowFactory<*>?
 }

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

@@ -130,6 +130,18 @@ class NodeVaultServiceTest {
         return tryLockFungibleStatesForSpending(lockId, baseCriteria, amount, Cash.State::class.java)
     }
 
+    @Test
+    fun `duplicate insert of transaction does not fail`() {
+        database.transaction {
+            val cash = Cash()
+            val howMuch = 100.DOLLARS
+            val issuance = TransactionBuilder(null as Party?)
+            cash.generateIssue(issuance, Amount(howMuch.quantity, Issued(DUMMY_CASH_ISSUER, howMuch.token)), services.myInfo.singleIdentity(), dummyNotary.party)
+            val transaction = issuerServices.signInitialTransaction(issuance, DUMMY_CASH_ISSUER.party.owningKey)
+            services.recordTransactions(transaction)
+            services.recordTransactions(transaction)
+        }
+    }
 
     @Test
     fun `states not local to instance`() {

testing/node-driver/src/main/kotlin/net/corda/testing/node/MockServices.kt

@@ -2,28 +2,26 @@ package net.corda.testing.node
 
 import com.google.common.collect.MutableClassToInstanceMap
 import net.corda.core.contracts.ContractClassName
-import net.corda.core.contracts.ContractState
-import net.corda.core.contracts.StateAndRef
 import net.corda.core.contracts.StateRef
 import net.corda.core.cordapp.CordappProvider
-import net.corda.core.crypto.*
+import net.corda.core.crypto.SecureHash
 import net.corda.core.flows.FlowLogic
+import net.corda.core.flows.StateMachineRunId
 import net.corda.core.identity.CordaX500Name
 import net.corda.core.identity.PartyAndCertificate
+import net.corda.core.messaging.DataFeed
 import net.corda.core.messaging.FlowHandle
 import net.corda.core.messaging.FlowProgressHandle
+import net.corda.core.messaging.StateMachineTransactionMapping
 import net.corda.core.node.*
 import net.corda.core.node.services.*
 import net.corda.core.serialization.SerializeAsToken
 import net.corda.core.transactions.SignedTransaction
 import net.corda.core.utilities.NetworkHostAndPort
-import net.corda.node.VersionInfo
 import net.corda.node.internal.ServicesForResolutionImpl
 import net.corda.node.internal.configureDatabase
 import net.corda.node.internal.cordapp.CordappLoader
-import net.corda.node.services.api.SchemaService
+import net.corda.node.services.api.*
-import net.corda.node.services.api.VaultServiceInternal
-import net.corda.node.services.api.WritableTransactionStorage
 import net.corda.node.services.identity.InMemoryIdentityService
 import net.corda.node.services.schema.HibernateObserver
 import net.corda.node.services.schema.NodeSchemaService
@@ -108,9 +106,10 @@ open class MockServices private constructor(
                     override val vaultService: VaultService = makeVaultService(database.hibernateConfig, schemaService)
 
                     override fun recordTransactions(statesToRecord: StatesToRecord, txs: Iterable<SignedTransaction>) {
-                        super.recordTransactions(statesToRecord, txs)
+                        ServiceHubInternal.recordTransactions(statesToRecord, txs,
-                        // Refactored to use notifyAll() as we have no other unit test for that method with multiple transactions.
+                                validatedTransactions as WritableTransactionStorage,
-                        (vaultService as VaultServiceInternal).notifyAll(statesToRecord, txs.map { it.coreTransaction })
+                                mockStateMachineRecordedTransactionMappingStorage,
+                                vaultService as VaultServiceInternal)
                     }
 
                     override fun jdbcSession(): Connection = database.createSession()
@@ -126,6 +125,19 @@ open class MockServices private constructor(
             // compiler and then the c'tor itself.
             return Throwable().stackTrace[3].className.split('.').dropLast(1).joinToString(".")
         }
+
+        // Because Kotlin is dumb and makes not publicly visible objects public, thus changing the public API.
+        private val mockStateMachineRecordedTransactionMappingStorage = MockStateMachineRecordedTransactionMappingStorage()
+    }
+
+    private class MockStateMachineRecordedTransactionMappingStorage : StateMachineRecordedTransactionMappingStorage {
+        override fun addMapping(stateMachineRunId: StateMachineRunId, transactionId: SecureHash) {
+            throw UnsupportedOperationException()
+        }
+
+        override fun track(): DataFeed<List<StateMachineTransactionMapping>, StateMachineTransactionMapping> {
+            throw UnsupportedOperationException()
+        }
     }
 
     private constructor(cordappLoader: CordappLoader, identityService: IdentityService, networkParameters: NetworkParameters,