From 715c38766d1bc6520afee2a8d2441cd4ba1ccc71 Mon Sep 17 00:00:00 2001 From: Andrius Dagys Date: Tue, 16 Oct 2018 16:33:04 +0100 Subject: [PATCH] CORDA-2109: Fix a bug that prevents consecutive multiparty contract upgrades The contract upgrade handler assumes that the state to be upgraded is created by a WireTransaction. This breaks the upgrade process if it was in fact issued by a ContractUpgradeWireTransactions or a NotaryChangeWireTransaction. --- .../core/flows/ContractUpgradeFlowTest.kt | 96 ++++++++++--------- .../corda/core/flows/mixins/WithContracts.kt | 10 +- .../corda/node/services/CoreFlowHandlers.kt | 2 +- .../testing/contracts/DummyContractV3.kt | 36 +++++++ 4 files changed, 93 insertions(+), 51 deletions(-) create mode 100644 testing/test-utils/src/main/kotlin/net/corda/testing/contracts/DummyContractV3.kt diff --git a/core/src/test/kotlin/net/corda/core/flows/ContractUpgradeFlowTest.kt b/core/src/test/kotlin/net/corda/core/flows/ContractUpgradeFlowTest.kt index 8d30b83b62..889b8ceeee 100644 --- a/core/src/test/kotlin/net/corda/core/flows/ContractUpgradeFlowTest.kt +++ b/core/src/test/kotlin/net/corda/core/flows/ContractUpgradeFlowTest.kt @@ -3,15 +3,12 @@ package net.corda.core.flows import com.natpryce.hamkrest.* import com.natpryce.hamkrest.assertion.assert import net.corda.core.contracts.* -import net.corda.testing.internal.matchers.flow.willReturn -import net.corda.testing.internal.matchers.flow.willThrow import net.corda.core.flows.mixins.WithContracts import net.corda.core.flows.mixins.WithFinality import net.corda.core.identity.AbstractParty import net.corda.core.internal.Emoji import net.corda.core.transactions.ContractUpgradeLedgerTransaction import net.corda.core.transactions.LedgerTransaction -import net.corda.core.transactions.SignedTransaction import net.corda.core.utilities.OpaqueBytes import net.corda.core.utilities.getOrThrow import net.corda.finance.USD @@ -20,9 +17,12 @@ import net.corda.finance.contracts.asset.Cash import net.corda.finance.flows.CashIssueFlow import net.corda.testing.contracts.DummyContract import net.corda.testing.contracts.DummyContractV2 +import net.corda.testing.contracts.DummyContractV3 import net.corda.testing.core.ALICE_NAME import net.corda.testing.core.BOB_NAME import net.corda.testing.core.singleIdentity +import net.corda.testing.internal.matchers.flow.willReturn +import net.corda.testing.internal.matchers.flow.willThrow import net.corda.testing.node.internal.InternalMockNetwork import net.corda.testing.node.internal.TestStartedNode import net.corda.testing.node.internal.cordappsForPackages @@ -57,54 +57,67 @@ class ContractUpgradeFlowTest : WithContracts, WithFinality { aliceNode.finalise(stx, bob) - val atx = aliceNode.getValidatedTransaction(stx) - val btx = bobNode.getValidatedTransaction(stx) + val aliceTx = aliceNode.getValidatedTransaction(stx) + val bobTx = bobNode.getValidatedTransaction(stx) // The request is expected to be rejected because party B hasn't authorised the upgrade yet. assert.that( - aliceNode.initiateDummyContractUpgrade(atx), + aliceNode.initiateContractUpgrade(aliceTx, DummyContractV2::class), willThrow()) - // Party B authorise the contract state upgrade, and immediately deauthorise the same. - assert.that(bobNode.authoriseDummyContractUpgrade(btx), willReturn()) - assert.that(bobNode.deauthoriseContractUpgrade(btx), willReturn()) + // Party B authorises the contract state upgrade, and immediately de-authorises the same. + assert.that(bobNode.authoriseContractUpgrade(bobTx, DummyContractV2::class), willReturn()) + assert.that(bobNode.deauthoriseContractUpgrade(bobTx), willReturn()) - // The request is expected to be rejected because party B has subsequently deauthorised a previously authorised upgrade. + // The request is expected to be rejected because party B has subsequently de-authorised a previously authorised upgrade. assert.that( - aliceNode.initiateDummyContractUpgrade(atx), + aliceNode.initiateContractUpgrade(aliceTx, DummyContractV2::class), willThrow()) - // Party B authorise the contract state upgrade - assert.that(bobNode.authoriseDummyContractUpgrade(btx), willReturn()) + // Party B authorises the contract state upgrade. + assert.that(bobNode.authoriseContractUpgrade(bobTx, DummyContractV2::class), willReturn()) // Party A initiates contract upgrade flow, expected to succeed this time. assert.that( - aliceNode.initiateDummyContractUpgrade(atx), + aliceNode.initiateContractUpgrade(aliceTx, DummyContractV2::class), willReturn( - aliceNode.hasDummyContractUpgradeTransaction() - and bobNode.hasDummyContractUpgradeTransaction())) + aliceNode.hasContractUpgradeTransaction() + and bobNode.hasContractUpgradeTransaction())) + + val upgradedState = aliceNode.getStateFromVault(DummyContractV2.State::class) + + // We now test that the upgraded state can be upgraded further, to V3. + // Party B authorises the contract state upgrade. + assert.that(bobNode.authoriseContractUpgrade(upgradedState, DummyContractV3::class), willReturn()) + + // Party A initiates contract upgrade flow which is expected to succeed. + assert.that( + aliceNode.initiateContractUpgrade(upgradedState, DummyContractV3::class), + willReturn( + aliceNode.hasContractUpgradeTransaction() + and bobNode.hasContractUpgradeTransaction())) } private fun TestStartedNode.issueCash(amount: Amount = Amount(1000, USD)) = - services.startFlow(CashIssueFlow(amount, OpaqueBytes.of(1), notary)) - .andRunNetwork() - .resultFuture.getOrThrow() + services.startFlow(CashIssueFlow(amount, OpaqueBytes.of(1), notary)) + .andRunNetwork() + .resultFuture.getOrThrow() private fun TestStartedNode.getBaseStateFromVault() = getStateFromVault(ContractState::class) private fun TestStartedNode.getCashStateFromVault() = getStateFromVault(CashV2.State::class) private fun hasIssuedAmount(expected: Amount>) = - hasContractState(has(CashV2.State::amount, equalTo(expected))) + hasContractState(has(CashV2.State::amount, equalTo(expected))) private fun belongsTo(vararg recipients: AbstractParty) = - hasContractState(has(CashV2.State::owners, equalTo(recipients.toList()))) + hasContractState(has(CashV2.State::owners, equalTo(recipients.toList()))) private fun hasContractState(expectation: Matcher) = - has, T>( - "contract state", - { it.state.data }, - expectation) + has, T>( + "contract state", + { it.state.data }, + expectation) @Test fun `upgrade Cash to v2`() { @@ -123,14 +136,14 @@ class ContractUpgradeFlowTest : WithContracts, WithFinality { val upgradedState = aliceNode.getCashStateFromVault() assert.that(upgradedState, hasIssuedAmount(Amount(1000000, USD) `issued by` (alice.ref(1))) - and belongsTo(anonymisedRecipient)) + and belongsTo(anonymisedRecipient)) // Make sure the upgraded state can be spent val movedState = upgradedState.state.data.copy(amount = upgradedState.state.data.amount.times(2)) val spendUpgradedTx = aliceNode.signInitialTransaction { addInputState(upgradedState) addOutputState( - upgradedState.state.copy(data = movedState) + upgradedState.state.copy(data = movedState) ) addCommand(CashV2.Move(), alice.owningKey) } @@ -161,35 +174,24 @@ class ContractUpgradeFlowTest : WithContracts, WithFinality { override fun verify(tx: LedgerTransaction) {} } - //region Operations - private fun TestStartedNode.initiateDummyContractUpgrade(tx: SignedTransaction) = - initiateContractUpgrade(tx, DummyContractV2::class) - - private fun TestStartedNode.authoriseDummyContractUpgrade(tx: SignedTransaction) = - authoriseContractUpgrade(tx, DummyContractV2::class) - //endregion - //region Matchers - private fun TestStartedNode.hasDummyContractUpgradeTransaction() = - hasContractUpgradeTransaction() - - private inline fun TestStartedNode.hasContractUpgradeTransaction() = - has, ContractUpgradeLedgerTransaction>( - "a contract upgrade transaction", - { getContractUpgradeTransaction(it) }, - isUpgrade()) + private inline fun TestStartedNode.hasContractUpgradeTransaction() = + has, ContractUpgradeLedgerTransaction>( + "a contract upgrade transaction", + { getContractUpgradeTransaction(it) }, + isUpgrade()) private fun TestStartedNode.getContractUpgradeTransaction(state: StateAndRef) = - services.validatedTransactions.getTransaction(state.ref.txhash)!! - .resolveContractUpgradeTransaction(services) + services.validatedTransactions.getTransaction(state.ref.txhash)!! + .resolveContractUpgradeTransaction(services) private inline fun isUpgrade() = isUpgradeFrom() and isUpgradeTo() - private inline fun isUpgradeFrom() = + private inline fun isUpgradeFrom() = has("input data", { it.inputs.single().state.data }, isA(anything)) - private inline fun isUpgradeTo() = + private inline fun isUpgradeTo() = has("output data", { it.outputs.single().data }, isA(anything)) //endregion } diff --git a/core/src/test/kotlin/net/corda/core/flows/mixins/WithContracts.kt b/core/src/test/kotlin/net/corda/core/flows/mixins/WithContracts.kt index c5794007fe..7206b1ce76 100644 --- a/core/src/test/kotlin/net/corda/core/flows/mixins/WithContracts.kt +++ b/core/src/test/kotlin/net/corda/core/flows/mixins/WithContracts.kt @@ -51,9 +51,13 @@ interface WithContracts : WithMockNet { fun > TestStartedNode.authoriseContractUpgrade( tx: SignedTransaction, toClass: KClass) = - startFlow( - ContractUpgradeFlow.Authorise(tx.tx.outRef(0), toClass.java) - ) + authoriseContractUpgrade(tx.tx.outRef(0), toClass) + + fun > TestStartedNode.authoriseContractUpgrade( + stateAndRef: StateAndRef, toClass: KClass) = + startFlow( + ContractUpgradeFlow.Authorise(stateAndRef, toClass.java) + ) fun TestStartedNode.deauthoriseContractUpgrade(tx: SignedTransaction) = startFlow( ContractUpgradeFlow.Deauthorise(tx.tx.outRef(0).ref) diff --git a/node/src/main/kotlin/net/corda/node/services/CoreFlowHandlers.kt b/node/src/main/kotlin/net/corda/node/services/CoreFlowHandlers.kt index df2a1ec019..7b6bbb75a6 100644 --- a/node/src/main/kotlin/net/corda/node/services/CoreFlowHandlers.kt +++ b/node/src/main/kotlin/net/corda/node/services/CoreFlowHandlers.kt @@ -56,7 +56,7 @@ class ContractUpgradeHandler(otherSide: FlowSession) : AbstractStateReplacementF // verify outputs matches the proposed upgrade. val ourSTX = serviceHub.validatedTransactions.getTransaction(proposal.stateRef.txhash) requireNotNull(ourSTX) { "We don't have a copy of the referenced state" } - val oldStateAndRef = ourSTX!!.tx.outRef(proposal.stateRef.index) + val oldStateAndRef = ourSTX!!.resolveBaseTransaction(serviceHub).outRef(proposal.stateRef.index) val authorisedUpgrade = serviceHub.contractUpgradeService.getAuthorisedContractUpgrade(oldStateAndRef.ref) ?: throw IllegalStateException("Contract state upgrade is unauthorised. State hash : ${oldStateAndRef.ref}") val proposedTx = stx.coreTransaction as ContractUpgradeWireTransaction val expectedTx = ContractUpgradeUtils.assembleUpgradeTx(oldStateAndRef, proposal.modification, proposedTx.privacySalt, serviceHub) diff --git a/testing/test-utils/src/main/kotlin/net/corda/testing/contracts/DummyContractV3.kt b/testing/test-utils/src/main/kotlin/net/corda/testing/contracts/DummyContractV3.kt new file mode 100644 index 0000000000..eb8b9a1e97 --- /dev/null +++ b/testing/test-utils/src/main/kotlin/net/corda/testing/contracts/DummyContractV3.kt @@ -0,0 +1,36 @@ +package net.corda.testing.contracts + +import net.corda.core.contracts.* +import net.corda.core.identity.AbstractParty +import net.corda.core.transactions.LedgerTransaction + +// The dummy contract doesn't do anything useful. It exists for testing purposes. + +/** + * Dummy contract state for testing of the upgrade process. + */ +class DummyContractV3 : UpgradedContractWithLegacyConstraint { + companion object { + const val PROGRAM_ID: ContractClassName = "net.corda.testing.contracts.DummyContractV3" + } + + override val legacyContract: String = DummyContractV2.PROGRAM_ID + override val legacyContractConstraint: AttachmentConstraint = AlwaysAcceptAttachmentConstraint + + data class State(val magicNumber: Int = 0, val owners: List) : ContractState { + override val participants: List = owners + } + + interface Commands : CommandData { + class Create : TypeOnlyCommandData(), Commands + class Move : TypeOnlyCommandData(), Commands + } + + override fun upgrade(state: DummyContractV2.State): State { + return State(state.magicNumber, state.participants) + } + + override fun verify(tx: LedgerTransaction) { + // Other verifications. + } +}