diff --git a/node/src/main/kotlin/net/corda/node/services/api/ServiceHubInternal.kt b/node/src/main/kotlin/net/corda/node/services/api/ServiceHubInternal.kt index d589a6c8a6..62381d8272 100644 --- a/node/src/main/kotlin/net/corda/node/services/api/ServiceHubInternal.kt +++ b/node/src/main/kotlin/net/corda/node/services/api/ServiceHubInternal.kt @@ -58,6 +58,61 @@ interface NetworkMapCacheBaseInternal : NetworkMapCacheBase { interface ServiceHubInternal : ServiceHub { companion object { private val log = contextLogger() + + fun recordTransactions(statesToRecord: StatesToRecord, txs: Iterable, + validatedTransactions: WritableTransactionStorage, + stateMachineRecordedTransactionMapping: StateMachineRecordedTransactionMappingStorage, + vaultService: VaultServiceInternal) { + + require(txs.any()) { "No transactions passed in for recording" } + val recordedTransactions = txs.filter { validatedTransactions.addTransaction(it) } + val stateMachineRunId = FlowStateMachineImpl.currentStateMachine()?.id + if (stateMachineRunId != null) { + recordedTransactions.forEach { + stateMachineRecordedTransactionMapping.addMapping(stateMachineRunId, it.id) + } + } else { + log.warn("Transactions recorded from outside of a state machine") + } + + if (statesToRecord != StatesToRecord.NONE) { + // When the user has requested StatesToRecord.ALL we may end up recording and relationally mapping states + // that do not involve us and that we cannot sign for. This will break coin selection and thus a warning + // is present in the documentation for this feature (see the "Observer nodes" tutorial on docs.corda.net). + // + // The reason for this is three-fold: + // + // 1) We are putting in place the observer mode feature relatively quickly to meet specific customer + // launch target dates. + // + // 2) The right design for vaults which mix observations and relevant states isn't entirely clear yet. + // + // 3) If we get the design wrong it could create security problems and business confusions. + // + // Back in the bitcoinj days I did add support for "watching addresses" to the wallet code, which is the + // Bitcoin equivalent of observer nodes: + // + // https://bitcoinj.github.io/working-with-the-wallet#watching-wallets + // + // The ability to have a wallet containing both irrelevant and relevant states complicated everything quite + // dramatically, even methods as basic as the getBalance() API which required additional modes to let you + // query "balance I can spend" vs "balance I am observing". In the end it might have been better to just + // require the user to create an entirely separate wallet for observing with. + // + // In Corda we don't support a single node having multiple vaults (at the time of writing), and it's not + // clear that's the right way to go: perhaps adding an "origin" column to the VAULT_STATES table is a better + // solution. Then you could select subsets of states depending on where the report came from. + // + // The risk of doing this is that apps/developers may use 'canned SQL queries' not written by us that forget + // to add a WHERE clause for the origin column. Those queries will seem to work most of the time until + // they're run on an observer node and mix in irrelevant data. In the worst case this may result in + // erroneous data being reported to the user, which could cause security problems. + // + // 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, recordedTransactions.map { it.coreTransaction }) + } + } } override val vaultService: VaultServiceInternal @@ -80,54 +135,7 @@ interface ServiceHubInternal : ServiceHub { val networkMapUpdater: NetworkMapUpdater override val cordappProvider: CordappProviderInternal override fun recordTransactions(statesToRecord: StatesToRecord, txs: Iterable) { - require(txs.any()) { "No transactions passed in for recording" } - val recordedTransactions = txs.filter { validatedTransactions.addTransaction(it) } - val stateMachineRunId = FlowStateMachineImpl.currentStateMachine()?.id - if (stateMachineRunId != null) { - recordedTransactions.forEach { - stateMachineRecordedTransactionMapping.addMapping(stateMachineRunId, it.id) - } - } else { - log.warn("Transactions recorded from outside of a state machine") - } - - if (statesToRecord != StatesToRecord.NONE) { - // When the user has requested StatesToRecord.ALL we may end up recording and relationally mapping states - // that do not involve us and that we cannot sign for. This will break coin selection and thus a warning - // is present in the documentation for this feature (see the "Observer nodes" tutorial on docs.corda.net). - // - // The reason for this is three-fold: - // - // 1) We are putting in place the observer mode feature relatively quickly to meet specific customer - // launch target dates. - // - // 2) The right design for vaults which mix observations and relevant states isn't entirely clear yet. - // - // 3) If we get the design wrong it could create security problems and business confusions. - // - // Back in the bitcoinj days I did add support for "watching addresses" to the wallet code, which is the - // Bitcoin equivalent of observer nodes: - // - // https://bitcoinj.github.io/working-with-the-wallet#watching-wallets - // - // The ability to have a wallet containing both irrelevant and relevant states complicated everything quite - // dramatically, even methods as basic as the getBalance() API which required additional modes to let you - // query "balance I can spend" vs "balance I am observing". In the end it might have been better to just - // require the user to create an entirely separate wallet for observing with. - // - // In Corda we don't support a single node having multiple vaults (at the time of writing), and it's not - // clear that's the right way to go: perhaps adding an "origin" column to the VAULT_STATES table is a better - // solution. Then you could select subsets of states depending on where the report came from. - // - // The risk of doing this is that apps/developers may use 'canned SQL queries' not written by us that forget - // to add a WHERE clause for the origin column. Those queries will seem to work most of the time until - // they're run on an observer node and mix in irrelevant data. In the worst case this may result in - // erroneous data being reported to the user, which could cause security problems. - // - // 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 }) - } + recordTransactions(statesToRecord, txs, validatedTransactions, stateMachineRecordedTransactionMapping, vaultService) } fun getFlowFactory(initiatingFlowClass: Class>): InitiatedFlowFactory<*>? diff --git a/node/src/test/kotlin/net/corda/node/services/vault/NodeVaultServiceTest.kt b/node/src/test/kotlin/net/corda/node/services/vault/NodeVaultServiceTest.kt index d439a72c7c..88420afbeb 100644 --- a/node/src/test/kotlin/net/corda/node/services/vault/NodeVaultServiceTest.kt +++ b/node/src/test/kotlin/net/corda/node/services/vault/NodeVaultServiceTest.kt @@ -140,6 +140,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`() { diff --git a/testing/node-driver/src/main/kotlin/net/corda/testing/node/MockServices.kt b/testing/node-driver/src/main/kotlin/net/corda/testing/node/MockServices.kt index db43d527df..3c59bf41e7 100644 --- a/testing/node-driver/src/main/kotlin/net/corda/testing/node/MockServices.kt +++ b/testing/node-driver/src/main/kotlin/net/corda/testing/node/MockServices.kt @@ -19,10 +19,13 @@ import net.corda.core.contracts.StateRef import net.corda.core.cordapp.CordappProvider 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 @@ -37,6 +40,7 @@ import net.corda.node.services.api.WritableTransactionStorage import net.corda.node.services.config.ConfigHelper import net.corda.node.services.config.configOf import net.corda.node.services.config.parseToDbSchemaFriendlyName +import net.corda.node.services.api.* import net.corda.node.services.identity.InMemoryIdentityService import net.corda.node.services.schema.HibernateObserver import net.corda.node.services.schema.NodeSchemaService @@ -148,9 +152,10 @@ open class MockServices private constructor( override val vaultService: VaultService = makeVaultService(database.hibernateConfig, schemaService) override fun recordTransactions(statesToRecord: StatesToRecord, txs: Iterable) { - super.recordTransactions(statesToRecord, txs) - // Refactored to use notifyAll() as we have no other unit test for that method with multiple transactions. - (vaultService as VaultServiceInternal).notifyAll(statesToRecord, txs.map { it.coreTransaction }) + ServiceHubInternal.recordTransactions(statesToRecord, txs, + validatedTransactions as WritableTransactionStorage, + mockStateMachineRecordedTransactionMappingStorage, + vaultService as VaultServiceInternal) } override fun jdbcSession(): Connection = database.createSession() @@ -166,6 +171,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, StateMachineTransactionMapping> { + throw UnsupportedOperationException() + } } private constructor(cordappLoader: CordappLoader, identityService: IdentityService, networkParameters: NetworkParameters,