From 0a9f4c68ae1669f9d66f04d7d31c7e2b8462616f Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Fri, 1 Feb 2019 22:43:17 +0100 Subject: [PATCH] Minor refactorings. Take out a useless parameter from a method that was added to the public API, document it. Add some comments explaining more about why we are looking up attachment versions in WireTransaction.toLedgerTransaction. --- .../TransactionVerifierServiceInternal.kt | 4 +- .../kotlin/net/corda/core/node/ServiceHub.kt | 6 ++- .../core/transactions/WireTransaction.kt | 21 +++++---- .../contracts/ConstraintsPropagationTests.kt | 3 +- .../TransactionSerializationTests.kt | 2 +- .../internal/ServicesForResolutionImpl.kt | 44 ++++++++++--------- .../net/corda/testing/node/MockServices.kt | 2 +- 7 files changed, 47 insertions(+), 35 deletions(-) diff --git a/core/src/main/kotlin/net/corda/core/internal/TransactionVerifierServiceInternal.kt b/core/src/main/kotlin/net/corda/core/internal/TransactionVerifierServiceInternal.kt index 44e0670994..f11a347506 100644 --- a/core/src/main/kotlin/net/corda/core/internal/TransactionVerifierServiceInternal.kt +++ b/core/src/main/kotlin/net/corda/core/internal/TransactionVerifierServiceInternal.kt @@ -28,7 +28,7 @@ fun LedgerTransaction.prepareVerify(extraAttachments: List) = this.i * Because we create a separate [LedgerTransaction] onto which we need to perform verification, it becomes important we don't verify the * wrong object instance. This class helps avoid that. */ -class Verifier(val ltx: LedgerTransaction, val transactionClassLoader: ClassLoader, private val inputStatesContractClassNameToMaxVersion: Map) { +class Verifier(val ltx: LedgerTransaction, private val transactionClassLoader: ClassLoader, private val inputStatesContractClassNameToMaxVersion: Map) { private val inputStates: List> = ltx.inputs.map { it.state } private val allStates: List> = inputStates + ltx.references.map { it.state } + ltx.outputs private val contractAttachmentsByContract: Map> = getContractAttachmentsByContract() @@ -207,7 +207,7 @@ class Verifier(val ltx: LedgerTransaction, val transactionClassLoader: ClassLoad } /** - * Verify that contract class versions of output states are not lower that versions of relevant input states. + * Verify that contract class versions of output states are greater than or equal to the versions of the input states. */ private fun validateContractVersions() { contractAttachmentsByContract.forEach { contractClassName, attachments -> diff --git a/core/src/main/kotlin/net/corda/core/node/ServiceHub.kt b/core/src/main/kotlin/net/corda/core/node/ServiceHub.kt index 00354743bb..c20d6357a3 100644 --- a/core/src/main/kotlin/net/corda/core/node/ServiceHub.kt +++ b/core/src/main/kotlin/net/corda/core/node/ServiceHub.kt @@ -55,6 +55,7 @@ interface ServicesForResolution { */ @Throws(TransactionResolutionException::class) fun loadState(stateRef: StateRef): TransactionState<*> + /** * Given a [Set] of [StateRef]'s loads the referenced transaction and looks up the specified output [ContractState]. * @@ -65,8 +66,11 @@ interface ServicesForResolution { @Throws(TransactionResolutionException::class) fun loadStates(stateRefs: Set): Set> + /** + * Returns the [Attachment] that defines the given [StateRef], which must be in the visible subset of the ledger. + */ @Throws(TransactionResolutionException::class, AttachmentResolutionException::class) - fun loadContractAttachment(stateRef: StateRef, forContractClassName: ContractClassName? = null): Attachment + fun loadContractAttachment(stateRef: StateRef): Attachment } /** diff --git a/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt b/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt index 27a9254119..2c3a86e670 100644 --- a/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt +++ b/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt @@ -190,13 +190,18 @@ class WireTransaction(componentGroups: List, val privacySalt: Pr val resolvedNetworkParameters = resolveParameters(networkParametersHash) ?: throw TransactionResolutionException(id) - // Keep resolvedInputs lazy and resolve the inputs separately here to get Version. - val inputStateContractClassToStateRefs: Map>> = serializedResolvedInputs.map { - it.toStateAndRef() - }.groupBy { it.state.contract } - val inputStateContractClassToMaxVersion: Map = inputStateContractClassToStateRefs.mapValues { - it.value.map { resolveContractAttachment(it.ref).contractVersion }.max() ?: DEFAULT_CORDAPP_VERSION - } + // For each contract referenced in the inputs, figure out the highest version being used. The outputs must be + // at least that version or higher, to prevent adversaries from downgrading the app to an old version that has + // known bugs they can then exploit. This is part of the version ratchet that ensures apps can only ever be + // upgraded, not downgraded. We don't use resolvedInputs here to keep it lazy. TODO: why? + // We do this resolution now instead of in LedgerTransaction because here we have the function to map + // StateRefs to their attachments directly. + val appVersionsInInputs: Map = serializedResolvedInputs + .map { it.toStateAndRef() } + .groupBy { it.state.contract } + .mapValues { (_ , statesAndRefs) -> + statesAndRefs.map { resolveContractAttachment(it.ref).contractVersion }.max() ?: DEFAULT_CORDAPP_VERSION + } val ltx = LedgerTransaction.create( resolvedInputs, @@ -212,7 +217,7 @@ class WireTransaction(componentGroups: List, val privacySalt: Pr componentGroups, serializedResolvedInputs, serializedResolvedReferences, - inputStateContractClassToMaxVersion + appVersionsInInputs ) checkTransactionSize(ltx, resolvedNetworkParameters.maxTransactionSize, serializedResolvedInputs, serializedResolvedReferences) diff --git a/core/src/test/kotlin/net/corda/core/contracts/ConstraintsPropagationTests.kt b/core/src/test/kotlin/net/corda/core/contracts/ConstraintsPropagationTests.kt index b7a9566b9c..1283bcfeb4 100644 --- a/core/src/test/kotlin/net/corda/core/contracts/ConstraintsPropagationTests.kt +++ b/core/src/test/kotlin/net/corda/core/contracts/ConstraintsPropagationTests.kt @@ -32,7 +32,6 @@ import net.corda.testing.core.internal.ContractJarTestUtils import net.corda.testing.core.internal.JarSignatureTestUtils.generateKey import net.corda.testing.core.internal.SelfCleaningDir import net.corda.testing.internal.MockCordappProvider -import net.corda.testing.internal.rigorousMock import net.corda.testing.node.MockServices import net.corda.testing.node.ledger import org.junit.* @@ -94,7 +93,7 @@ class ConstraintsPropagationTests { packageOwnership = mapOf("net.corda.finance.contracts.asset" to hashToSignatureConstraintsKey), notaries = listOf(NotaryInfo(DUMMY_NOTARY, true))) ) { - override fun loadContractAttachment(stateRef: StateRef, forContractClassName: ContractClassName?) = servicesForResolution.loadContractAttachment(stateRef) + override fun loadContractAttachment(stateRef: StateRef) = servicesForResolution.loadContractAttachment(stateRef) } } diff --git a/core/src/test/kotlin/net/corda/core/serialization/TransactionSerializationTests.kt b/core/src/test/kotlin/net/corda/core/serialization/TransactionSerializationTests.kt index 55d6c72c76..1b5ea3dc37 100644 --- a/core/src/test/kotlin/net/corda/core/serialization/TransactionSerializationTests.kt +++ b/core/src/test/kotlin/net/corda/core/serialization/TransactionSerializationTests.kt @@ -72,7 +72,7 @@ class TransactionSerializationTests { val megaCorpServices = object : MockServices(listOf("net.corda.core.serialization"), MEGA_CORP.name, mock(), testNetworkParameters(notaries = listOf(NotaryInfo(DUMMY_NOTARY, true))), MEGA_CORP_KEY) { //override mock implementation with a real one - override fun loadContractAttachment(stateRef: StateRef, forContractClassName: ContractClassName?): Attachment = servicesForResolution.loadContractAttachment(stateRef, forContractClassName) + override fun loadContractAttachment(stateRef: StateRef): Attachment = servicesForResolution.loadContractAttachment(stateRef) } val notaryServices = MockServices(listOf("net.corda.core.serialization"), DUMMY_NOTARY.name, rigorousMock(), DUMMY_NOTARY_KEY) lateinit var tx: TransactionBuilder diff --git a/node/src/main/kotlin/net/corda/node/internal/ServicesForResolutionImpl.kt b/node/src/main/kotlin/net/corda/node/internal/ServicesForResolutionImpl.kt index e4b447dcf5..b8463ee5ec 100644 --- a/node/src/main/kotlin/net/corda/node/internal/ServicesForResolutionImpl.kt +++ b/node/src/main/kotlin/net/corda/node/internal/ServicesForResolutionImpl.kt @@ -40,29 +40,33 @@ data class ServicesForResolutionImpl( } @Throws(TransactionResolutionException::class, AttachmentResolutionException::class) - override fun loadContractAttachment(stateRef: StateRef, forContractClassName: ContractClassName?): Attachment { - val coreTransaction = validatedTransactions.getTransaction(stateRef.txhash)?.coreTransaction - ?: throw TransactionResolutionException(stateRef.txhash) - when (coreTransaction) { - is WireTransaction -> { - val transactionState = coreTransaction.outRef(stateRef.index).state - for (attachmentId in coreTransaction.attachments) { - val attachment = attachments.openAttachment(attachmentId) - if (attachment is ContractAttachment && (forContractClassName ?: transactionState.contract) in attachment.allContracts) { - return attachment + override fun loadContractAttachment(stateRef: StateRef): Attachment { + // We may need to recursively chase transactions if there are notary changes. + fun inner(stateRef: StateRef, forContractClassName: String?): Attachment { + val ctx = validatedTransactions.getTransaction(stateRef.txhash)?.coreTransaction + ?: throw TransactionResolutionException(stateRef.txhash) + when (ctx) { + is WireTransaction -> { + val transactionState = ctx.outRef(stateRef.index).state + for (attachmentId in ctx.attachments) { + val attachment = attachments.openAttachment(attachmentId) + if (attachment is ContractAttachment && (forContractClassName ?: transactionState.contract) in attachment.allContracts) { + return attachment + } } + throw AttachmentResolutionException(stateRef.txhash) } - throw AttachmentResolutionException(stateRef.txhash) + is ContractUpgradeWireTransaction -> { + return attachments.openAttachment(ctx.upgradedContractAttachmentId) ?: throw AttachmentResolutionException(stateRef.txhash) + } + is NotaryChangeWireTransaction -> { + val transactionState = SerializedStateAndRef(resolveStateRefBinaryComponent(stateRef, this)!!, stateRef).toStateAndRef().state + // TODO: check only one (or until one is resolved successfully), max recursive invocations check? + return ctx.inputs.map { inner(it, transactionState.contract) }.firstOrNull() ?: throw AttachmentResolutionException(stateRef.txhash) + } + else -> throw UnsupportedOperationException("Attempting to resolve attachment for index ${stateRef.index} of a ${ctx.javaClass} transaction. This is not supported.") } - is ContractUpgradeWireTransaction -> { - return attachments.openAttachment(coreTransaction.upgradedContractAttachmentId) ?: throw AttachmentResolutionException(stateRef.txhash) - } - is NotaryChangeWireTransaction -> { - val transactionState = SerializedStateAndRef(resolveStateRefBinaryComponent(stateRef, this)!!, stateRef).toStateAndRef().state - //TODO check only one (or until one is resolved successfully), max recursive invocations check? - return coreTransaction.inputs.map { loadContractAttachment(it, transactionState.contract) }.firstOrNull() ?: throw AttachmentResolutionException(stateRef.txhash) - } - else -> throw UnsupportedOperationException("Attempting to resolve attachment ${stateRef.index} of a ${coreTransaction.javaClass} transaction. This is not supported.") } + return inner(stateRef, null) } } 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 8d14ee7cc9..17fb3e36f2 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 @@ -352,7 +352,7 @@ open class MockServices private constructor( override fun loadStates(stateRefs: Set) = servicesForResolution.loadStates(stateRefs) /** Returns a dummy Attachment, in context of signature constrains non-downgrade rule this default to contract class version `1`. */ - override fun loadContractAttachment(stateRef: StateRef, forContractClassName: ContractClassName?) = dummyAttachment + override fun loadContractAttachment(stateRef: StateRef) = dummyAttachment } /**