Merged in mike-tx-types-refactoring-september (pull request #334)

Small usability improvements to the core tx types API and add an additional check to NCP

contracts/src/main/kotlin/com/r3corda/contracts/clause/AbstractConserveAmount.kt

@@ -47,7 +47,7 @@ abstract class AbstractConserveAmount<S : FungibleAsset<T>, C : CommandData, T :
      * @param tx transaction builder to add states and commands to.
      * @param amountIssued the amount to be exited, represented as a quantity of issued currency.
      * @param assetStates the asset states to take funds from. No checks are done about ownership of these states, it is
-     * the responsibility of the caller to check that they do not exit funds held by others.
+     * the responsibility of the caller to check that they do not attempt to exit funds held by others.
      * @return the public key of the assets issuer, who must sign the transaction for it to be valid.
      */
     fun generateExit(tx: TransactionBuilder, amountIssued: Amount<Issued<T>>,

contracts/src/main/kotlin/com/r3corda/protocols/TwoPartyTradeProtocol.kt

@@ -7,7 +7,6 @@ import com.r3corda.core.contracts.*
 import com.r3corda.core.crypto.DigitalSignature
 import com.r3corda.core.crypto.Party
 import com.r3corda.core.crypto.signWithECDSA
-import com.r3corda.core.crypto.toStringsShort
 import com.r3corda.core.node.NodeInfo
 import com.r3corda.core.protocols.ProtocolLogic
 import com.r3corda.core.random63BitValue
@@ -19,7 +18,6 @@ import com.r3corda.core.utilities.ProgressTracker
 import com.r3corda.core.utilities.trace
 import java.security.KeyPair
 import java.security.PublicKey
-import java.security.SignatureException
 import java.util.*
 
 /**
@@ -120,16 +118,11 @@ object TwoPartyTradeProtocol {
 
             progressTracker.currentStep = VERIFYING
 
-            maybeSTX.validate {
+            maybeSTX.unwrap {
                 progressTracker.nextStep()
 
                 // Check that the tx proposed by the buyer is valid.
-                val missingSigs: Set<PublicKey> = it.verifySignatures(throwIfSignaturesAreMissing = false)
+                val wtx: WireTransaction = it.verifySignatures(myKeyPair.public, notaryNode.identity.owningKey)
-                val expected = setOf(myKeyPair.public, notaryNode.identity.owningKey)
-                if (missingSigs != expected)
-                    throw SignatureException("The set of missing signatures is not as expected: ${missingSigs.toStringsShort()} vs ${expected.toStringsShort()}")
-
-                val wtx: WireTransaction = it.tx
                 logger.trace { "Received partially signed transaction: ${it.id}" }
 
                 // Download and check all the things that this transaction depends on and verify it is contract-valid,
@@ -214,7 +207,7 @@ object TwoPartyTradeProtocol {
             val maybeTradeRequest = receive<SellerTradeInfo>(sessionID)
 
             progressTracker.currentStep = VERIFYING
-            maybeTradeRequest.validate {
+            maybeTradeRequest.unwrap {
                 // What is the seller trying to sell us?
                 val asset = it.assetForSale.state.data
                 val assetTypeName = asset.javaClass.name
@@ -242,7 +235,7 @@ object TwoPartyTradeProtocol {
 
             // TODO: Protect against the seller terminating here and leaving us in the lurch without the final tx.
 
-            return sendAndReceive<SignaturesFromSeller>(otherSide, theirSessionID, sessionID, stx).validate { it }
+            return sendAndReceive<SignaturesFromSeller>(otherSide, theirSessionID, sessionID, stx).unwrap { it }
         }
 
         private fun signWithOurKeys(cashSigningPubKeys: List<PublicKey>, ptx: TransactionBuilder): SignedTransaction {

contracts/src/test/kotlin/com/r3corda/contracts/asset/ObligationTests.kt

@@ -6,9 +6,6 @@ import com.r3corda.core.crypto.NullPublicKey
 import com.r3corda.core.crypto.SecureHash
 import com.r3corda.core.serialization.OpaqueBytes
 import com.r3corda.core.utilities.*
-import com.r3corda.testing.LedgerDSL
-import com.r3corda.testing.TestLedgerDSLInterpreter
-import com.r3corda.testing.TestTransactionDSLInterpreter
 import com.r3corda.testing.*
 import org.junit.Test
 import java.security.PublicKey

core/src/main/kotlin/com/r3corda/core/contracts/TransactionTypes.kt

@@ -30,7 +30,7 @@ sealed class TransactionType {
         if (notaryKey.size > 1) throw TransactionVerificationException.MoreThanOneNotary(tx)
 
         val requiredKeys = getRequiredSigners(tx) + notaryKey
-        val missing = requiredKeys - tx.signers
+        val missing = requiredKeys - tx.mustSign
 
         return missing
     }

core/src/main/kotlin/com/r3corda/core/serialization/Kryo.kt

@@ -234,7 +234,7 @@ object WireTransactionSerializer : Serializer<WireTransaction>() {
         kryo.writeClassAndObject(output, obj.outputs)
         kryo.writeClassAndObject(output, obj.commands)
         kryo.writeClassAndObject(output, obj.notary)
-        kryo.writeClassAndObject(output, obj.signers)
+        kryo.writeClassAndObject(output, obj.mustSign)
         kryo.writeClassAndObject(output, obj.type)
         kryo.writeClassAndObject(output, obj.timestamp)
     }

core/src/main/kotlin/com/r3corda/core/transactions/BaseTransaction.kt

@@ -25,7 +25,7 @@ abstract class BaseTransaction(
          * transaction until the transaction is verified by using [LedgerTransaction.verify]. It includes the
          * notary key, if the notary field is set.
          */
-        val signers: List<PublicKey>,
+        val mustSign: List<PublicKey>,
         /**
          * Pointer to a class that defines the behaviour of this transaction: either normal, or "notary changing".
          */
@@ -37,7 +37,7 @@ abstract class BaseTransaction(
         val timestamp: Timestamp?
 ) : NamedByHash {
 
-    fun checkInvariants() {
+    protected fun checkInvariants() {
         if (notary == null) check(inputs.isEmpty()) { "The notary must be specified explicitly for any transaction that has inputs." }
         if (timestamp != null) check(notary != null) { "If a timestamp is provided, there must be a notary." }
     }
@@ -45,9 +45,9 @@ abstract class BaseTransaction(
     override fun equals(other: Any?) =
             other is BaseTransaction &&
             notary == other.notary &&
-            signers == other.signers &&
+            mustSign == other.mustSign &&
             type == other.type &&
             timestamp == other.timestamp
 
-    override fun hashCode() = Objects.hash(notary, signers, type, timestamp)
+    override fun hashCode() = Objects.hash(notary, mustSign, type, timestamp)
 }

core/src/main/kotlin/com/r3corda/core/transactions/SignedTransaction.kt

@@ -23,7 +23,7 @@ import java.util.*
 data class SignedTransaction(val txBits: SerializedBytes<WireTransaction>,
                              val sigs: List<DigitalSignature.WithKey>) : NamedByHash {
     init {
-        check(sigs.isNotEmpty())
+        require(sigs.isNotEmpty())
     }
 
     // TODO: This needs to be reworked to ensure that the inner WireTransaction is only ever deserialised sandboxed.
@@ -34,27 +34,59 @@ data class SignedTransaction(val txBits: SerializedBytes<WireTransaction>,
     /** A transaction ID is the hash of the [WireTransaction]. Thus adding or removing a signature does not change it. */
     override val id: SecureHash get() = txBits.hash
 
+    class SignaturesMissingException(val missing: Set<PublicKey>, val descriptions: List<String>, override val id: SecureHash) : NamedByHash, SignatureException() {
+        override fun toString(): String {
+            return "Missing signatures for $descriptions on transaction ${id.prefixChars()} for ${missing.toStringsShort()}"
+        }
+    }
+
     /**
-     * Verify the signatures, deserialise the wire transaction and then check that the set of signatures found contains
+     * Verifies the signatures on this transaction and throws if any are missing which aren't passed as parameters.
-     * the set of pubkeys in the signers list. If any signatures are missing, either throws an exception (by default) or
+     * In this context, "verifying" means checking they are valid signatures and that their public keys are in
-     * returns the list of keys that have missing signatures, depending on the parameter.
+     * the contained transactions [BaseTransaction.mustSign] property.
      *
-     * @throws SignatureException if a signature is invalid, does not match or if any signature is missing.
+     * Normally you would not provide any keys to this function, but if you're in the process of building a partial
+     * transaction and you want to access the contents before you've signed it, you can specify your own keys here
+     * to bypass that check.
+     *
+     * @throws SignatureException if any signatures are invalid or unrecognised.
+     * @throws SignaturesMissingException if any signatures should have been present but were not.
      */
     @Throws(SignatureException::class)
-    fun verifySignatures(throwIfSignaturesAreMissing: Boolean = true): Set<PublicKey> {
+    fun verifySignatures(vararg allowedToBeMissing: PublicKey): WireTransaction {
         // Embedded WireTransaction is not deserialised until after we check the signatures.
+        checkSignaturesAreValid()
+
+        val missing = getMissingSignatures()
+        if (missing.isNotEmpty()) {
+            val allowed = setOf(*allowedToBeMissing)
+            val needed = missing - allowed
+            if (needed.isNotEmpty())
+                throw SignaturesMissingException(needed, getMissingKeyDescriptions(needed), id)
+        }
+        return tx
+    }
+
+    /**
+     * Mathematically validates the signatures that are present on this transaction. This does not imply that
+     * the signatures are by the right keys, or that there are sufficient signatures, just that they aren't
+     * corrupt. If you use this function directly you'll need to do the other checks yourself. Probably you
+     * want [verifySignatures] instead.
+     *
+     * @throws SignatureException if a signature fails to verify.
+     */
+    @Throws(SignatureException::class)
+    fun checkSignaturesAreValid() {
         for (sig in sigs)
             sig.verifyWithECDSA(txBits.bits)
+    }
 
-        // Now examine the contents and ensure the sigs we have line up with the advertised list of signers.
+    private fun getMissingSignatures(): Set<PublicKey> {
-        val missing = getMissingSignatures()
+        val requiredKeys = tx.mustSign.toSet()
-        if (missing.isNotEmpty() && throwIfSignaturesAreMissing) {
+        val sigKeys = sigs.map { it.by }.toSet()
-            val missingElements = getMissingKeyDescriptions(missing)
-            throw SignatureException("Missing signatures for ${missingElements} on transaction ${id.prefixChars()} for ${missing.toStringsShort()}")
-        }
 
-        return missing
+        if (sigKeys.containsAll(requiredKeys)) return emptySet()
+        return requiredKeys - sigKeys
     }
 
     /**
@@ -65,13 +97,11 @@ data class SignedTransaction(val txBits: SerializedBytes<WireTransaction>,
         // TODO: We need a much better way of structuring this data
         val missingElements = ArrayList<String>()
         this.tx.commands.forEach { command ->
-            if (command.signers.any { signer -> missing.contains(signer) })
+            if (command.signers.any { it in missing })
                 missingElements.add(command.toString())
         }
-        this.tx.notary?.owningKey.apply {
+        if (this.tx.notary?.owningKey in missing)
-            if (missing.contains(this))
+            missingElements.add("notary")
-                missingElements.add("notary")
-        }
         return missingElements
     }
 
@@ -85,17 +115,6 @@ data class SignedTransaction(val txBits: SerializedBytes<WireTransaction>,
     /** Alias for [withAdditionalSignatures] to let you use Kotlin operator overloading. */
     operator fun plus(sigList: Collection<DigitalSignature.WithKey>) = withAdditionalSignatures(sigList)
 
-    /**
-     * Returns the set of missing signatures - a signature must be present for each signer public key.
-     */
-    private fun getMissingSignatures(): Set<PublicKey> {
-        val requiredKeys = tx.signers.toSet()
-        val sigKeys = sigs.map { it.by }.toSet()
-
-        if (sigKeys.containsAll(requiredKeys)) return emptySet()
-        return requiredKeys - sigKeys
-    }
-
     /**
      * Calls [verifySignatures] to check all required signatures are present, and then calls
      * [WireTransaction.toLedgerTransaction] with the passed in [ServiceHub] to resolve the dependencies,
@@ -103,10 +122,9 @@ data class SignedTransaction(val txBits: SerializedBytes<WireTransaction>,
      *
      * @throws FileNotFoundException if a required attachment was not found in storage.
      * @throws TransactionResolutionException if an input points to a transaction not found in storage.
+     * @throws SignatureException if any signatures were invalid or unrecognised
+     * @throws SignaturesMissingException if any signatures that should have been present are missing.
      */
-    @Throws(FileNotFoundException::class, TransactionResolutionException::class)
+    @Throws(FileNotFoundException::class, TransactionResolutionException::class, SignaturesMissingException::class)
-    fun toLedgerTransaction(services: ServiceHub): LedgerTransaction {
+    fun toLedgerTransaction(services: ServiceHub) = verifySignatures().toLedgerTransaction(services)
-        verifySignatures()
-        return tx.toLedgerTransaction(services)
-    }
 }

core/src/main/kotlin/com/r3corda/core/transactions/WireTransaction.kt

@@ -77,7 +77,7 @@ class WireTransaction(
             services.storageService.attachments.openAttachment(it) ?: throw FileNotFoundException(it.toString())
         }
         val resolvedInputs = inputs.map { StateAndRef(services.loadState(it), it) }
-        return LedgerTransaction(resolvedInputs, outputs, authenticatedArgs, attachments, id, notary, signers, timestamp, type)
+        return LedgerTransaction(resolvedInputs, outputs, authenticatedArgs, attachments, id, notary, mustSign, timestamp, type)
     }
 
     override fun toString(): String {

core/src/main/kotlin/com/r3corda/core/utilities/UntrustworthyData.kt

@@ -17,5 +17,9 @@ class UntrustworthyData<out T>(private val fromUntrustedWorld: T) {
         get() = fromUntrustedWorld
 
     @Suppress("DEPRECATION")
+    inline fun <R> unwrap(validator: (T) -> R) = validator(data)
+
+    @Suppress("DEPRECATION")
+    @Deprecated("This old name was confusing, use unwrap instead", replaceWith = ReplaceWith("unwrap"))
     inline fun <R> validate(validator: (T) -> R) = validator(data)
 }

core/src/main/kotlin/com/r3corda/protocols/AbstractStateReplacementProtocol.kt

@@ -1,7 +1,9 @@
 package com.r3corda.protocols
 
 import co.paralleluniverse.fibers.Suspendable
-import com.r3corda.core.contracts.*
+import com.r3corda.core.contracts.ContractState
+import com.r3corda.core.contracts.StateAndRef
+import com.r3corda.core.contracts.StateRef
 import com.r3corda.core.crypto.DigitalSignature
 import com.r3corda.core.crypto.Party
 import com.r3corda.core.crypto.signWithECDSA
@@ -12,6 +14,7 @@ import com.r3corda.core.random63BitValue
 import com.r3corda.core.transactions.SignedTransaction
 import com.r3corda.core.transactions.WireTransaction
 import com.r3corda.core.utilities.ProgressTracker
+import com.r3corda.core.utilities.UntrustworthyData
 import com.r3corda.protocols.AbstractStateReplacementProtocol.Acceptor
 import com.r3corda.protocols.AbstractStateReplacementProtocol.Instigator
 import java.security.PublicKey
@@ -100,7 +103,7 @@ abstract class AbstractStateReplacementProtocol<T> {
             sendAndReceive<Ack>(node.identity, 0, sessionIdForReceive, handshake)
 
             val response = sendAndReceive<Result>(node.identity, sessionIdForSend, sessionIdForReceive, proposal)
-            val participantSignature = response.validate {
+            val participantSignature = response.unwrap {
                 if (it.sig == null) throw StateReplacementException(it.error!!)
                 else {
                     check(it.sig.by == node.identity.owningKey) { "Not signed by the required participant" }
@@ -119,7 +122,7 @@ abstract class AbstractStateReplacementProtocol<T> {
         }
     }
 
-    abstract class Acceptor<in T>(val otherSide: Party,
+    abstract class Acceptor<T>(val otherSide: Party,
                                   val sessionIdForSend: Long,
                                   val sessionIdForReceive: Long,
                                   override val progressTracker: ProgressTracker = tracker()) : ProtocolLogic<Unit>() {
@@ -137,24 +140,21 @@ abstract class AbstractStateReplacementProtocol<T> {
         @Suspendable
         override fun call() {
             progressTracker.currentStep = VERIFYING
-            val proposal = receive<Proposal<T>>(sessionIdForReceive).validate { it }
+            val maybeProposal: UntrustworthyData<Proposal<T>> = receive(sessionIdForReceive)
-
             try {
-                verifyProposal(proposal)
+                val stx: SignedTransaction = maybeProposal.unwrap { verifyProposal(maybeProposal).stx }
-                verifyTx(proposal.stx)
+                verifyTx(stx)
+                approve(stx)
             } catch(e: Exception) {
                 // TODO: catch only specific exceptions. However, there are numerous validation exceptions
                 //       that might occur (tx validation/resolution, invalid proposal). Need to rethink how
                 //       we manage exceptions and maybe introduce some platform exception hierarchy
                 val myIdentity = serviceHub.storageService.myLegalIdentity
-                val state = proposal.stateRef
+                val state = maybeProposal.unwrap { it.stateRef }
                 val reason = StateReplacementRefused(myIdentity, state, e.message)
 
                 reject(reason)
-                return
             }
-
-            approve(proposal.stx)
         }
 
         @Suspendable
@@ -166,7 +166,7 @@ abstract class AbstractStateReplacementProtocol<T> {
             val swapSignatures = sendAndReceive<List<DigitalSignature.WithKey>>(otherSide, sessionIdForSend, sessionIdForReceive, response)
 
             // TODO: This step should not be necessary, as signatures are re-checked in verifySignatures.
-            val allSignatures = swapSignatures.validate { signatures ->
+            val allSignatures = swapSignatures.unwrap { signatures ->
                 signatures.forEach { it.verifyWithECDSA(stx.txBits) }
                 signatures
             }
@@ -185,9 +185,10 @@ abstract class AbstractStateReplacementProtocol<T> {
 
         /**
          * Check the state change proposal to confirm that it's acceptable to this node. Rules for verification depend
-         * on the change proposed, and may further depend on the node itself (for example configuration).
+         * on the change proposed, and may further depend on the node itself (for example configuration). The
+         * proposal is returned if acceptable, otherwise an exception is thrown.
          */
-        abstract internal fun verifyProposal(proposal: Proposal<T>)
+        abstract fun verifyProposal(maybeProposal: UntrustworthyData<Proposal<T>>): Proposal<T>
 
         @Suspendable
         private fun verifyTx(stx: SignedTransaction) {
@@ -201,7 +202,7 @@ abstract class AbstractStateReplacementProtocol<T> {
         private fun checkMySignatureRequired(tx: WireTransaction) {
             // TODO: use keys from the keyManagementService instead
             val myKey = serviceHub.storageService.myLegalIdentity.owningKey
-            require(tx.signers.contains(myKey)) { "Party is not a participant for any of the input states of transaction ${tx.id}" }
+            require(myKey in tx.mustSign) { "Party is not a participant for any of the input states of transaction ${tx.id}" }
         }
 
         @Suspendable

core/src/main/kotlin/com/r3corda/protocols/FetchDataProtocol.kt

@@ -81,7 +81,7 @@ abstract class FetchDataProtocol<T : NamedByHash, in W : Any>(
 
     private fun validateFetchResponse(maybeItems: UntrustworthyData<ArrayList<W?>>,
                                       requests: List<SecureHash>): List<T> =
-            maybeItems.validate { response ->
+            maybeItems.unwrap { response ->
                 if (response.size != requests.size)
                     throw BadAnswer()
                 for ((index, resp) in response.withIndex()) {

core/src/main/kotlin/com/r3corda/protocols/FinalityProtocol.kt

@@ -1,18 +1,11 @@
 package com.r3corda.protocols
 
 import co.paralleluniverse.fibers.Suspendable
-import com.google.common.util.concurrent.ListenableFuture
 import com.r3corda.core.contracts.ClientToServiceCommand
-import com.r3corda.core.transactions.SignedTransaction
-import com.r3corda.core.transactions.TransactionBuilder
-import com.r3corda.core.transactions.WireTransaction
 import com.r3corda.core.crypto.Party
-import com.r3corda.core.node.ServiceHub
 import com.r3corda.core.protocols.ProtocolLogic
-import com.r3corda.core.random63BitValue
+import com.r3corda.core.transactions.SignedTransaction
-import com.r3corda.core.serialization.serialize
 import com.r3corda.core.utilities.ProgressTracker
-import java.util.*
 
 
 /**
@@ -43,6 +36,8 @@ class FinalityProtocol(val transaction: SignedTransaction,
 
     @Suspendable
     override fun call() {
+        // TODO: Resolve the tx here: it's probably already been done, but re-resolution is a no-op and it'll make the API more forgiving.
+
         progressTracker.currentStep = NOTARISING
         // Notarise the transaction if needed
         val notarisedTransaction = if (needsNotarySignature(transaction)) {
@@ -57,7 +52,6 @@ class FinalityProtocol(val transaction: SignedTransaction,
         subProtocol(BroadcastTransactionProtocol(notarisedTransaction, events, participants))
     }
 
-    private fun needsNotarySignature(transaction: SignedTransaction) = expectsNotarySignature(transaction.tx) && hasNoNotarySignature(transaction)
+    private fun needsNotarySignature(stx: SignedTransaction) = stx.tx.notary != null && hasNoNotarySignature(stx)
-    private fun expectsNotarySignature(transaction: WireTransaction) = transaction.notary != null && transaction.notary.owningKey in transaction.signers
+    private fun hasNoNotarySignature(stx: SignedTransaction) = stx.tx.notary?.owningKey !in stx.sigs.map { it.by }
-    private fun hasNoNotarySignature(transaction: SignedTransaction) = transaction.tx.notary?.owningKey !in transaction.sigs.map { it.by }
 }

core/src/main/kotlin/com/r3corda/protocols/NotaryChangeProtocol.kt

@@ -1,10 +1,16 @@
 package com.r3corda.protocols
 
 import co.paralleluniverse.fibers.Suspendable
-import com.r3corda.core.contracts.*
+import com.r3corda.core.contracts.ContractState
+import com.r3corda.core.contracts.StateAndRef
+import com.r3corda.core.contracts.StateRef
+import com.r3corda.core.contracts.TransactionType
 import com.r3corda.core.crypto.Party
 import com.r3corda.core.transactions.SignedTransaction
 import com.r3corda.core.utilities.ProgressTracker
+import com.r3corda.core.utilities.UntrustworthyData
+import com.r3corda.protocols.NotaryChangeProtocol.Acceptor
+import com.r3corda.protocols.NotaryChangeProtocol.Instigator
 import java.security.PublicKey
 
 /**
@@ -62,18 +68,25 @@ object NotaryChangeProtocol: AbstractStateReplacementProtocol<Party>() {
          * TODO: In more difficult cases this should call for human attention to manually verify and approve the proposal
          */
         @Suspendable
-        override fun verifyProposal(proposal: AbstractStateReplacementProtocol.Proposal<Party>) {
+        override fun verifyProposal(maybeProposal: UntrustworthyData<AbstractStateReplacementProtocol.Proposal<Party>>): AbstractStateReplacementProtocol.Proposal<Party> {
-            val newNotary = proposal.modification
+            return maybeProposal.unwrap { proposal ->
-            val isNotary = serviceHub.networkMapCache.notaryNodes.any { it.identity == newNotary }
+                val newNotary = proposal.modification
-            require(isNotary) { "The proposed node $newNotary does not run a Notary service " }
+                val isNotary = serviceHub.networkMapCache.notaryNodes.any { it.identity == newNotary }
+                require(isNotary) { "The proposed node $newNotary does not run a Notary service " }
 
-            val state = proposal.stateRef
+                val state = proposal.stateRef
-            val proposedTx = proposal.stx.tx
+                val proposedTx = proposal.stx.tx
-            require(proposedTx.inputs.contains(state)) { "The proposed state $state is not in the proposed transaction inputs" }
+                require(state in proposedTx.inputs) { "The proposed state $state is not in the proposed transaction inputs" }
+                require(proposedTx.type.javaClass == TransactionType.NotaryChange::class.java) {
+                    "The proposed transaction is not a notary change transaction."
+                }
 
-            // An example requirement
+                // An example requirement
-            val blacklist = listOf("Evil Notary")
+                val blacklist = listOf("Evil Notary")
-            require(!blacklist.contains(newNotary.name)) { "The proposed new notary $newNotary is not trusted by the party" }
+                require(!blacklist.contains(newNotary.name)) { "The proposed new notary $newNotary is not trusted by the party" }
+
+                proposal
+            }
         }
     }
 }

core/src/main/kotlin/com/r3corda/protocols/NotaryProtocol.kt

@@ -72,7 +72,7 @@ object NotaryProtocol {
         private fun validateResponse(response: UntrustworthyData<Result>): Result {
             progressTracker.currentStep = VALIDATING
 
-            response.validate {
+            response.unwrap {
                 if (it.sig != null) validateSignature(it.sig, stx.txBits)
                 else if (it.error is NotaryError.Conflict) it.error.conflict.verified()
                 else if (it.error == null || it.error !is NotaryError)
@@ -105,7 +105,7 @@ object NotaryProtocol {
 
         @Suspendable
         override fun call() {
-            val (stx, reqIdentity) = receive<SignRequest>(receiveSessionID).validate { it }
+            val (stx, reqIdentity) = receive<SignRequest>(receiveSessionID).unwrap { it }
             val wtx = stx.tx
 
             val result = try {
@@ -205,5 +205,5 @@ sealed class NotaryError {
 
     class TransactionInvalid : NotaryError()
 
-    class SignaturesMissing(val missingSigners: List<PublicKey>) : NotaryError()
+    class SignaturesMissing(val missingSigners: Set<PublicKey>) : NotaryError()
 }

core/src/main/kotlin/com/r3corda/protocols/RatesFixProtocol.kt

@@ -85,7 +85,7 @@ open class RatesFixProtocol(protected val tx: TransactionBuilder,
         val req = SignRequest(wtx, serviceHub.storageService.myLegalIdentity, sessionID)
         val resp = sendAndReceive<DigitalSignature.LegallyIdentifiable>(oracle, 0, sessionID, req)
 
-        return resp.validate { sig ->
+        return resp.unwrap { sig ->
             check(sig.signer == oracle)
             tx.checkSignature(sig)
             sig
@@ -100,7 +100,7 @@ open class RatesFixProtocol(protected val tx: TransactionBuilder,
         // TODO: add deadline to receive
         val resp = sendAndReceive<ArrayList<Fix>>(oracle, 0, sessionID, req)
 
-        return resp.validate {
+        return resp.unwrap {
             val fix = it.first()
             // Check the returned fix is for what we asked for.
             check(fix.of == fixOf)

core/src/main/kotlin/com/r3corda/protocols/ResolveTransactionsProtocol.kt

@@ -2,12 +2,12 @@ package com.r3corda.protocols
 
 import co.paralleluniverse.fibers.Suspendable
 import com.r3corda.core.checkedAdd
-import com.r3corda.core.transactions.LedgerTransaction
-import com.r3corda.core.transactions.SignedTransaction
-import com.r3corda.core.transactions.WireTransaction
 import com.r3corda.core.crypto.Party
 import com.r3corda.core.crypto.SecureHash
 import com.r3corda.core.protocols.ProtocolLogic
+import com.r3corda.core.transactions.LedgerTransaction
+import com.r3corda.core.transactions.SignedTransaction
+import com.r3corda.core.transactions.WireTransaction
 import java.util.*
 
 // TODO: This code is currently unit tested by TwoPartyTradeProtocolTests, it should have its own tests.
@@ -82,7 +82,7 @@ class ResolveTransactionsProtocol(private val txHashes: Set<SecureHash>,
         // be a clearer API if we do that. But for consistency with the other c'tor we currently do not.
         //
         // If 'stx' is set, then 'wtx' is the contents (from the c'tor).
-        stx?.verifySignatures()
+        val wtx = stx?.verifySignatures() ?: wtx
         wtx?.let {
             fetchMissingAttachments(listOf(it))
             val ltx = it.toLedgerTransaction(serviceHub)

core/src/main/kotlin/com/r3corda/protocols/TwoPartyDealProtocol.kt

@@ -20,7 +20,6 @@ import com.r3corda.core.utilities.trace
 import java.math.BigDecimal
 import java.security.KeyPair
 import java.security.PublicKey
-import java.security.SignatureException
 import java.time.Duration
 
 /**
@@ -101,15 +100,11 @@ object TwoPartyDealProtocol {
         fun verifyPartialTransaction(untrustedPartialTX: UntrustworthyData<SignedTransaction>): SignedTransaction {
             progressTracker.currentStep = VERIFYING
 
-            untrustedPartialTX.validate { stx ->
+            untrustedPartialTX.unwrap { stx ->
                 progressTracker.nextStep()
 
                 // Check that the tx proposed by the buyer is valid.
-                val missingSigs = stx.verifySignatures(throwIfSignaturesAreMissing = false)
+                val wtx: WireTransaction = stx.verifySignatures(myKeyPair.public, notaryNode.identity.owningKey)
-                if (missingSigs != setOf(myKeyPair.public, notaryNode.identity.owningKey))
-                    throw SignatureException("The set of missing signatures is not as expected: $missingSigs")
-
-                val wtx: WireTransaction = stx.tx
                 logger.trace { "Received partially signed transaction: ${stx.id}" }
 
                 checkDependencies(stx)
@@ -245,7 +240,7 @@ object TwoPartyDealProtocol {
             val handshake = receive<Handshake<U>>(sessionID)
 
             progressTracker.currentStep = VERIFYING
-            handshake.validate {
+            handshake.unwrap {
                 return validateHandshake(it)
             }
         }
@@ -257,7 +252,7 @@ object TwoPartyDealProtocol {
 
             // TODO: Protect against the seller terminating here and leaving us in the lurch without the final tx.
 
-            return sendAndReceive<SignaturesFromPrimary>(otherSide, theirSessionID, sessionID, stx).validate { it }
+            return sendAndReceive<SignaturesFromPrimary>(otherSide, theirSessionID, sessionID, stx).unwrap { it }
         }
 
         private fun signWithOurKeys(signingPubKeys: List<PublicKey>, ptx: TransactionBuilder): SignedTransaction {

core/src/main/kotlin/com/r3corda/protocols/ValidatingNotaryProtocol.kt

@@ -1,12 +1,12 @@
 package com.r3corda.protocols
 
 import co.paralleluniverse.fibers.Suspendable
-import com.r3corda.core.transactions.SignedTransaction
 import com.r3corda.core.contracts.TransactionVerificationException
-import com.r3corda.core.transactions.WireTransaction
 import com.r3corda.core.crypto.Party
 import com.r3corda.core.node.services.TimestampChecker
 import com.r3corda.core.node.services.UniquenessProvider
+import com.r3corda.core.transactions.SignedTransaction
+import com.r3corda.core.transactions.WireTransaction
 import java.security.SignatureException
 
 /**
@@ -37,10 +37,11 @@ class ValidatingNotaryProtocol(otherSide: Party,
     }
 
     private fun checkSignatures(stx: SignedTransaction) {
-        val myKey = serviceHub.storageService.myLegalIdentity.owningKey
+        try {
-        val missing = stx.verifySignatures(throwIfSignaturesAreMissing = false) - myKey
+            stx.verifySignatures(serviceHub.storageService.myLegalIdentity.owningKey)
-
+        } catch(e: SignedTransaction.SignaturesMissingException) {
-        if (missing.isNotEmpty()) throw NotaryException(NotaryError.SignaturesMissing(missing.toList()))
+            throw NotaryException(NotaryError.SignaturesMissing(e.missing))
+        }
     }
 
     @Suspendable

core/src/test/kotlin/com/r3corda/core/contracts/TransactionTests.kt

@@ -1,17 +1,61 @@
 package com.r3corda.core.contracts
 
+import com.r3corda.contracts.asset.DUMMY_CASH_ISSUER_KEY
 import com.r3corda.core.crypto.Party
 import com.r3corda.core.crypto.SecureHash
+import com.r3corda.core.crypto.signWithECDSA
+import com.r3corda.core.serialization.SerializedBytes
 import com.r3corda.core.transactions.LedgerTransaction
+import com.r3corda.core.transactions.SignedTransaction
+import com.r3corda.core.transactions.WireTransaction
+import com.r3corda.core.utilities.DUMMY_KEY_1
+import com.r3corda.core.utilities.DUMMY_KEY_2
 import com.r3corda.core.utilities.DUMMY_NOTARY
 import com.r3corda.core.utilities.DUMMY_NOTARY_KEY
 import com.r3corda.testing.ALICE
 import com.r3corda.testing.ALICE_PUBKEY
 import com.r3corda.testing.BOB
 import org.junit.Test
+import java.security.KeyPair
+import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
 
-class TransactionTypeTests {
+class TransactionTests {
+    @Test
+    fun `signed transaction missing signatures`() {
+        val wtx = WireTransaction(
+                inputs = listOf(StateRef(SecureHash.randomSHA256(), 0)),
+                attachments = emptyList(),
+                outputs = emptyList(),
+                commands = emptyList(),
+                notary = DUMMY_NOTARY,
+                signers = listOf(DUMMY_KEY_1.public, DUMMY_KEY_2.public),
+                type = TransactionType.General(),
+                timestamp = null
+        )
+        val bits: SerializedBytes<WireTransaction> = wtx.serialized
+        fun make(vararg keys: KeyPair) = SignedTransaction(bits, keys.map { it.signWithECDSA(bits) })
+        assertFailsWith<IllegalArgumentException> { make().verifySignatures() }
+
+        assertEquals(
+                setOf(DUMMY_KEY_1.public),
+                assertFailsWith<SignedTransaction.SignaturesMissingException> { make(DUMMY_KEY_2).verifySignatures() }.missing
+        )
+        assertEquals(
+                setOf(DUMMY_KEY_2.public),
+                assertFailsWith<SignedTransaction.SignaturesMissingException> { make(DUMMY_KEY_1).verifySignatures() }.missing
+        )
+        assertEquals(
+                setOf(DUMMY_KEY_2.public),
+                assertFailsWith<SignedTransaction.SignaturesMissingException> { make(DUMMY_CASH_ISSUER_KEY).verifySignatures(DUMMY_KEY_1.public) }.missing
+        )
+
+        make(DUMMY_KEY_1).verifySignatures(DUMMY_KEY_2.public)
+        make(DUMMY_KEY_2).verifySignatures(DUMMY_KEY_1.public)
+
+        make(DUMMY_KEY_1, DUMMY_KEY_2).verifySignatures()
+    }
+
     @Test
     fun `transactions with no inputs can have any notary`() {
         val baseOutState = TransactionState(DummyContract.SingleOwnerState(0, ALICE_PUBKEY), DUMMY_NOTARY)

core/src/test/kotlin/com/r3corda/core/serialization/TransactionSerializationTests.kt

@@ -5,7 +5,8 @@ import com.r3corda.core.crypto.SecureHash
 import com.r3corda.core.seconds
 import com.r3corda.core.transactions.TransactionBuilder
 import com.r3corda.core.utilities.*
-import com.r3corda.testing.*
+import com.r3corda.testing.MINI_CORP
+import com.r3corda.testing.generateStateRef
 import org.junit.Before
 import org.junit.Test
 import java.security.PublicKey
@@ -84,7 +85,7 @@ class TransactionSerializationTests {
         val signedTX = tx.toSignedTransaction()
 
         // Cannot construct with an empty sigs list.
-        assertFailsWith(IllegalStateException::class) {
+        assertFailsWith(IllegalArgumentException::class) {
             signedTX.copy(sigs = emptyList())
         }
 

docs/source/index.rst

@@ -57,6 +57,7 @@ Read on to learn:
    protocol-state-machines
    oracles
    event-scheduling
+   secure-coding-guidelines
 
 .. toctree::
    :maxdepth: 2

docs/source/protocol-state-machines.rst

@@ -260,7 +260,7 @@ Let's fill out the ``receiveAndCheckProposedTransaction()`` method.
 
           val maybeSTX = sendAndReceive<SignedTransaction>(otherSide, buyerSessionID, sessionID, hello)
 
-          maybeSTX.validate {
+          maybeSTX.unwrap {
               // Check that the tx proposed by the buyer is valid.
               val missingSigs: Set<PublicKey> = it.verifySignatures(throwIfSignaturesAreMissing = false)
               val expected = setOf(myKeyPair.public, notaryNode.identity.owningKey)
@@ -421,7 +421,7 @@ OK, let's do the same for the buyer side:
       private fun receiveAndValidateTradeRequest(): SellerTradeInfo {
           // Wait for a trade request to come in on our pre-provided session ID.
           val maybeTradeRequest = receive<SellerTradeInfo>(sessionID)
-          maybeTradeRequest.validate {
+          maybeTradeRequest.unwrap {
               // What is the seller trying to sell us?
               val asset = it.assetForSale.state.data
               val assetTypeName = asset.javaClass.name
@@ -449,7 +449,7 @@ OK, let's do the same for the buyer side:
 
           // TODO: Protect against the seller terminating here and leaving us in the lurch without the final tx.
 
-          return sendAndReceive<SignaturesFromSeller>(otherSide, theirSessionID, sessionID, stx).validate { it }
+          return sendAndReceive<SignaturesFromSeller>(otherSide, theirSessionID, sessionID, stx).unwrap { it }
       }
 
       private fun signWithOurKeys(cashSigningPubKeys: List<PublicKey>, ptx: TransactionBuilder): SignedTransaction {

docs/source/release-notes.rst

@@ -3,6 +3,17 @@ Release notes
 
 Here are brief summaries of what's changed between each snapshot release.
 
+Unreleased
+----------
+
+API changes:
+
+* The transaction types (Signed, Wire, LedgerTransaction) have moved to ``com.r3corda.core.transactions``. You can
+  update your code by just deleting the broken import lines and letting your IDE re-import them from the right
+  location.
+* ``AbstractStateReplacementProtocol.verifyProposal`` has changed its prototype in a minor way.
+* The ``UntrustworthyData<T>.validate`` method has been renamed to ``unwrap`` - the old name is now deprecated.
+
 Milestone 3
 -----------
 

docs/source/secure-coding-guidelines.rst

@@ -0,0 +1,47 @@
+Secure coding guidelines
+------------------------
+
+The platform does what it can to be secure by default and safe by design. Unfortunately the platform cannot
+prevent every kind of security mistake. This document describes what to think about when writing applications
+to block various kinds of attack. Whilst it may be tempting to just assume no reasonable counterparty would
+attempt to subvert your trades using protocol level attacks, relying on trust for software security makes it
+harder to scale up your operations later when you might want to add counterparties quickly and without
+extensive vetting.
+
+Protocols
+---------
+
+:doc:`protocol-state-machines` are how your app communicates with other parties on the network. Therefore they
+are the typical entry point for malicious data into your app and must be treated with care.
+
+The ``receive`` methods return data wrapped in the ``UntrustworthyData<T>`` marker type. This type doesn't add
+any functionality, it's only there to remind you to properly validate everything that you get from the network.
+Remember that the other side may *not* be running the code you provide to take part in the protocol: they are
+allowed to do anything! Things to watch out for:
+
+* A transaction that doesn't match a partial transaction built or proposed earlier in the protocol, for instance,
+  if you propose to trade a cash state worth $100 for an asset, and the transaction to sign comes back from the
+  other side, you must check that it points to the state you actually requested. Otherwise the attacker could
+  get you to sign a transaction that spends a much larger state to you, if they know the ID of one!
+* A transaction that isn't of the right type. There are two transaction types: general and notary change. If you
+  are expecting one type but get the other you may find yourself signing a transaction that transfers your assets
+  to the control of a hostile notary.
+* Unexpected changes in any part of the states in a transaction. If you have access to all the needed data, you
+  could re-run the builder logic and do a comparison of the resulting states to ensure that it's what you expected.
+  For instance if the data needed to construct the next state is available to both parties, the function to
+  calculate the transaction you want to mutually agree could be shared between both classes implementing both
+  sides of the protocol.
+
+The theme should be clear: signing is a very sensitive operation, so you need to be sure you know what it is you
+are about to sign, and that nothing has changed in the small print!
+
+Contracts
+---------
+
+Contracts are arbitrary functions inside a JVM sandbox and therefore they have a lot of leeway to shoot themselves
+in the foot. Things to watch out for:
+
+* Changes in states that should not be allowed by the current state transition. You will want to check that no
+  fields are changing except the intended fields!
+* Accidentally catching and discarding exceptions that might be thrown by validation logic.
+* Calling into other contracts via virtual methods if you don't know what those other contracts are or might do.

node/src/test/kotlin/com/r3corda/node/services/InMemoryNetworkMapServiceTest.kt

@@ -78,7 +78,7 @@ class InMemoryNetworkMapServiceTest {
         override fun call(): Collection<NodeRegistration>? {
             val sessionID = random63BitValue()
             val req = NetworkMapService.FetchMapRequest(subscribe, ifChangedSinceVersion, serviceHub.networkService.myAddress, sessionID)
-            return sendAndReceive<NetworkMapService.FetchMapResponse>(server.identity, 0, sessionID, req).validate { it.nodes }
+            return sendAndReceive<NetworkMapService.FetchMapResponse>(server.identity, 0, sessionID, req).unwrap { it.nodes }
         }
     }
 
@@ -89,7 +89,7 @@ class InMemoryNetworkMapServiceTest {
         override fun call(): NetworkMapService.RegistrationResponse {
             val sessionID = random63BitValue()
             val req = NetworkMapService.RegistrationRequest(reg.toWire(privateKey), serviceHub.networkService.myAddress, sessionID)
-            return sendAndReceive<NetworkMapService.RegistrationResponse>(server.identity, 0, sessionID, req).validate { it }
+            return sendAndReceive<NetworkMapService.RegistrationResponse>(server.identity, 0, sessionID, req).unwrap { it }
         }
     }
 
@@ -100,7 +100,7 @@ class InMemoryNetworkMapServiceTest {
         override fun call(): NetworkMapService.SubscribeResponse {
             val sessionID = random63BitValue()
             val req = NetworkMapService.SubscribeRequest(subscribe, serviceHub.networkService.myAddress, sessionID)
-            return sendAndReceive<NetworkMapService.SubscribeResponse>(server.identity, 0, sessionID, req).validate { it }
+            return sendAndReceive<NetworkMapService.SubscribeResponse>(server.identity, 0, sessionID, req).unwrap { it }
         }
     }
 

node/src/test/kotlin/com/r3corda/node/services/NotaryChangeTests.kt

@@ -7,13 +7,13 @@ import com.r3corda.core.seconds
 import com.r3corda.core.utilities.DUMMY_NOTARY
 import com.r3corda.core.utilities.DUMMY_NOTARY_KEY
 import com.r3corda.node.internal.AbstractNode
-import com.r3corda.testing.node.MockNetwork
 import com.r3corda.node.services.network.NetworkMapService
 import com.r3corda.node.services.transactions.SimpleNotaryService
 import com.r3corda.protocols.NotaryChangeProtocol
 import com.r3corda.protocols.NotaryChangeProtocol.Instigator
 import com.r3corda.protocols.StateReplacementException
 import com.r3corda.protocols.StateReplacementRefused
+import com.r3corda.testing.node.MockNetwork
 import org.junit.Before
 import org.junit.Test
 import java.time.Instant
@@ -93,6 +93,7 @@ class NotaryChangeTests {
     //       - The requesting party wants to change additional state fields
     //       - Multiple states in a single "notary change" transaction
     //       - Transaction contains additional states and commands with business logic
+    //       - The transaction type is not a notary change transaction at all.
 }
 
 fun issueState(node: AbstractNode): StateAndRef<*> {

node/src/test/kotlin/com/r3corda/node/services/ValidatingNotaryServiceTests.kt

@@ -5,7 +5,6 @@ import com.r3corda.core.contracts.DummyContract
 import com.r3corda.core.contracts.TransactionType
 import com.r3corda.core.utilities.DUMMY_NOTARY
 import com.r3corda.core.utilities.DUMMY_NOTARY_KEY
-import com.r3corda.testing.node.MockNetwork
 import com.r3corda.node.services.network.NetworkMapService
 import com.r3corda.node.services.transactions.ValidatingNotaryService
 import com.r3corda.protocols.NotaryError
@@ -13,6 +12,7 @@ import com.r3corda.protocols.NotaryException
 import com.r3corda.protocols.NotaryProtocol
 import com.r3corda.testing.MEGA_CORP_KEY
 import com.r3corda.testing.MINI_CORP_KEY
+import com.r3corda.testing.node.MockNetwork
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.Before
 import org.junit.Test
@@ -73,6 +73,6 @@ class ValidatingNotaryServiceTests {
         assertThat(notaryError).isInstanceOf(NotaryError.SignaturesMissing::class.java)
 
         val missingKeys = (notaryError as NotaryError.SignaturesMissing).missingSigners
-        assertEquals(missingKeys, listOf(expectedMissingKey))
+        assertEquals(setOf(expectedMissingKey), missingKeys)
     }
 }

node/src/test/kotlin/com/r3corda/node/services/WalletMonitorServiceTests.kt

@@ -58,7 +58,7 @@ class WalletMonitorServiceTests {
         override val topic: String get() = WalletMonitorService.IN_EVENT_TOPIC
         @Suspendable
         override fun call(): ServiceToClientEvent.OutputState
-            = receive<ServiceToClientEvent.OutputState>(sessionID).validate { it }
+            = receive<ServiceToClientEvent.OutputState>(sessionID).unwrap { it }
     }
 
     class TestRegisterPSM(val server: NodeInfo, val sessionID: Long)
@@ -67,7 +67,7 @@ class WalletMonitorServiceTests {
         @Suspendable
         override fun call(): RegisterResponse {
             val req = RegisterRequest(serviceHub.networkService.myAddress, sessionID)
-            return sendAndReceive<RegisterResponse>(server.identity, 0, sessionID, req).validate { it }
+            return sendAndReceive<RegisterResponse>(server.identity, 0, sessionID, req).unwrap { it }
         }
     }
 

node/src/test/kotlin/com/r3corda/node/services/statemachine/StateMachineManagerTests.kt

@@ -102,7 +102,7 @@ class StateMachineManagerTests {
 
         @Suspendable
         override fun doCall() {
-            receivedPayload = receive<Any>(sessionID).validate { it }
+            receivedPayload = receive<Any>(sessionID).unwrap { it }
         }
     }
 

src/main/kotlin/com/r3corda/demos/TraderDemo.kt

@@ -306,7 +306,7 @@ private class TraderDemoProtocolSeller(val otherSide: Party,
     override fun call(): SignedTransaction {
         progressTracker.currentStep = ANNOUNCING
 
-        val sessionID = sendAndReceive<Long>(otherSide, 0, 0, serviceHub.storageService.myLegalIdentity).validate { it }
+        val sessionID = sendAndReceive<Long>(otherSide, 0, 0, serviceHub.storageService.myLegalIdentity).unwrap { it }
 
         progressTracker.currentStep = SELF_ISSUING
 

test-utils/src/main/kotlin/com/r3corda/testing/TestDSL.kt

@@ -314,7 +314,7 @@ data class TestLedgerDSLInterpreter private constructor (
  * @return List of [SignedTransaction]s.
  */
 fun signAll(transactionsToSign: List<WireTransaction>, extraKeys: List<KeyPair>) = transactionsToSign.map { wtx ->
-    check(wtx.signers.isNotEmpty())
+    check(wtx.mustSign.isNotEmpty())
     val bits = wtx.serialize()
     require(bits == wtx.serialized)
     val signatures = ArrayList<DigitalSignature.WithKey>()
@@ -323,7 +323,7 @@ fun signAll(transactionsToSign: List<WireTransaction>, extraKeys: List<KeyPair>)
     (ALL_TEST_KEYS + extraKeys).forEach {
         keyLookup[it.public] = it
     }
-    wtx.signers.forEach {
+    wtx.mustSign.forEach {
         val key = keyLookup[it] ?: throw IllegalArgumentException("Missing required key for ${it.toStringShort()}")
         signatures += key.signWithECDSA(bits)
     }