CORDA-2190: Improve notary service flow exception handling (#4200)

* CORDA-2190: Improve notary service flow exception handling

- Refactored notary flows to reduce validation code duplication
- Improved notarisation error handling to provide more helpful responses to the client

core/src/main/kotlin/net/corda/core/flows/NotaryError.kt

@@ -49,6 +49,7 @@ sealed class NotaryError {
     }
 
     /** Occurs when the transaction sent for notarisation is assigned to a different notary identity. */
+    @Deprecated("Deprecated since platform version 4. This object is no longer used, [TransactionInvalid] will be reported in case of notary mismatch")
     object WrongNotary : NotaryError()
 
     /** Occurs when the notarisation request signature does not verify for the provided transaction. */

core/src/main/kotlin/net/corda/core/internal/notary/NotaryServiceFlow.kt

@@ -23,61 +23,84 @@ abstract class NotaryServiceFlow(val otherSideSession: FlowSession, val service:
         private const val maxAllowedInputsAndReferences = 10_000
     }
 
+    private var transactionId: SecureHash? = null
+
     @Suspendable
     override fun call(): Void? {
         check(serviceHub.myInfo.legalIdentities.any { serviceHub.networkMapCache.isNotary(it) }) {
             "We are not a notary on the network"
         }
         val requestPayload = otherSideSession.receive<NotarisationPayload>().unwrap { it }
-        var txId: SecureHash? = null
+
         try {
-            val parts = validateRequest(requestPayload)
-            txId = parts.id
-            checkNotary(parts.notary)
+            val tx: TransactionParts = validateRequest(requestPayload)
+            val request = NotarisationRequest(tx.inputs, tx.id)
+            validateRequestSignature(request, requestPayload.requestSignature)
+
+            verifyTransaction(requestPayload)
+
             service.commitInputStates(
-                    parts.inputs,
-                    txId,
+                    tx.inputs,
+                    tx.id,
                     otherSideSession.counterparty,
                     requestPayload.requestSignature,
-                    parts.timestamp,
-                    parts.references
-            )
-            signTransactionAndSendResponse(txId)
+                    tx.timeWindow,
+                    tx.references)
+
         } catch (e: NotaryInternalException) {
-            throw NotaryException(e.error, txId)
+            logError(e.error)
+            // Any exception that's not a NotaryInternalException is assumed to be an unexpected internal error
+            // that is not relayed back to the client.
+            throw NotaryException(e.error, transactionId)
         }
+
+        signTransactionAndSendResponse(transactionId!!)
         return null
     }
 
-    /** Checks whether the number of input states is too large. */
-    protected fun checkInputs(inputs: List<StateRef>) {
-        if (inputs.size > maxAllowedInputsAndReferences) {
-            val error = NotaryError.TransactionInvalid(
-                    IllegalArgumentException("A transaction cannot have more than $maxAllowedInputsAndReferences " +
-                            "inputs or references, received: ${inputs.size}")
-            )
+    private fun validateRequest(requestPayload: NotarisationPayload): TransactionParts {
+        try {
+            val transaction = extractParts(requestPayload)
+            transactionId = transaction.id
+            checkNotary(transaction.notary)
+            checkInputs(transaction.inputs + transaction.references)
+            return transaction
+        } catch (e: Exception) {
+            val error = NotaryError.TransactionInvalid(e)
             throw NotaryInternalException(error)
         }
     }
 
-    /**
-     * Implement custom logic to perform transaction verification based on validity and privacy requirements.
-     */
+    /** Extract the common transaction components required for notarisation. */
+    protected abstract fun extractParts(requestPayload: NotarisationPayload): TransactionParts
+
+    /** Check if transaction is intended to be signed by this notary. */
     @Suspendable
-    protected abstract fun validateRequest(requestPayload: NotarisationPayload): TransactionParts
+    private fun checkNotary(notary: Party?) {
+        require(notary?.owningKey == service.notaryIdentityKey) {
+            "The notary specified on the transaction: [$notary] does not match the notary service's identity: [${service.notaryIdentityKey}] "
+        }
+    }
+
+    /** Checks whether the number of input states is too large. */
+    private fun checkInputs(inputs: List<StateRef>) {
+        require(inputs.size < maxAllowedInputsAndReferences) {
+            "A transaction cannot have more than $maxAllowedInputsAndReferences " +
+                    "inputs or references, received: ${inputs.size}"
+        }
+    }
 
     /** Verifies that the correct notarisation request was signed by the counterparty. */
-    protected fun validateRequestSignature(request: NotarisationRequest, signature: NotarisationRequestSignature) {
+    private fun validateRequestSignature(request: NotarisationRequest, signature: NotarisationRequestSignature) {
         val requestingParty = otherSideSession.counterparty
         request.verifySignature(signature, requestingParty)
     }
 
-    /** Check if transaction is intended to be signed by this notary. */
+    /**
+     * Override to implement custom logic to perform transaction verification based on validity and privacy requirements.
+     */
     @Suspendable
-    protected fun checkNotary(notary: Party?) {
-        if (notary?.owningKey != service.notaryIdentityKey) {
-            throw NotaryInternalException(NotaryError.WrongNotary)
-        }
+    protected open fun verifyTransaction(requestPayload: NotarisationPayload) {
     }
 
     @Suspendable
@@ -90,15 +113,23 @@ abstract class NotaryServiceFlow(val otherSideSession: FlowSession, val service:
      * The minimum amount of information needed to notarise a transaction. Note that this does not include
      * any sensitive transaction details.
      */
-    protected data class TransactionParts @JvmOverloads constructor(
+    protected data class TransactionParts(
             val id: SecureHash,
             val inputs: List<StateRef>,
-            val timestamp: TimeWindow?,
+            val timeWindow: TimeWindow?,
             val notary: Party?,
             val references: List<StateRef> = emptyList()
-    ) {
-        fun copy(id: SecureHash, inputs: List<StateRef>, timestamp: TimeWindow?, notary: Party?): TransactionParts {
-            return TransactionParts(id, inputs, timestamp, notary, references)
+    )
+
+    private fun logError(error: NotaryError) {
+        val errorCause = when (error) {
+            is NotaryError.RequestSignatureInvalid -> error.cause
+            is NotaryError.TransactionInvalid ->  error.cause
+            is NotaryError.General -> error.cause
+            else -> null
+        }
+        if (errorCause != null) {
+            logger.error("Error notarising transaction $transactionId", errorCause)
         }
     }
 }

core/src/main/kotlin/net/corda/core/internal/notary/NotaryUtils.kt

@@ -2,42 +2,31 @@ package net.corda.core.internal.notary
 
 import net.corda.core.contracts.StateRef
 import net.corda.core.contracts.TimeWindow
-import net.corda.core.crypto.DigitalSignature
 import net.corda.core.crypto.SecureHash
 import net.corda.core.crypto.isFulfilledBy
 import net.corda.core.flows.*
 import net.corda.core.identity.Party
 import net.corda.core.node.ServiceHub
 import net.corda.core.serialization.serialize
-import java.security.InvalidKeyException
-import java.security.SignatureException
+import net.corda.core.utilities.toBase58String
 import java.time.Instant
 
 /** Verifies the signature against this notarisation request. Checks that the signature is issued by the right party. */
 fun NotarisationRequest.verifySignature(requestSignature: NotarisationRequestSignature, intendedSigner: Party) {
-    val signature = requestSignature.digitalSignature
-    if (intendedSigner.owningKey != signature.by) {
-        val errorMessage = "Expected a signature by ${intendedSigner.owningKey}, but received by ${signature.by}}"
-        throw NotaryInternalException(NotaryError.RequestSignatureInvalid(IllegalArgumentException(errorMessage)))
-    }
-    // TODO: if requestSignature was generated over an old version of NotarisationRequest, we need to be able to
-    // reserialize it in that version to get the exact same bytes. Modify the serialization logic once that's
-    // available.
-    val expectedSignedBytes = this.serialize().bytes
-    verifyCorrectBytesSigned(signature, expectedSignedBytes)
-}
-
-private fun verifyCorrectBytesSigned(signature: DigitalSignature.WithKey, bytes: ByteArray) {
     try {
-        signature.verify(bytes)
-    } catch (e: Exception) {
-        when (e) {
-            is InvalidKeyException, is SignatureException -> {
-                val error = NotaryError.RequestSignatureInvalid(e)
-                throw NotaryInternalException(error)
-            }
-            else -> throw e
+        val signature = requestSignature.digitalSignature
+        require(intendedSigner.owningKey == signature.by) {
+            "Expected a signature by ${intendedSigner.owningKey.toBase58String()}, but received by ${signature.by.toBase58String()}}"
         }
+
+        // TODO: if requestSignature was generated over an old version of NotarisationRequest, we need to be able to
+        // reserialize it in that version to get the exact same bytes. Modify the serialization logic once that's
+        // available.
+        val expectedSignedBytes = this.serialize().bytes
+        signature.verify(expectedSignedBytes)
+    } catch (e: Exception) {
+        val error = NotaryError.RequestSignatureInvalid(e)
+        throw NotaryInternalException(error)
     }
 }
 

core/src/main/kotlin/net/corda/core/internal/notary/SinglePartyNotaryService.kt

@@ -53,6 +53,7 @@ abstract class SinglePartyNotaryService : NotaryService() {
                         references
                 )
         )
+
         if (result is UniquenessProvider.Result.Failure) {
             throw NotaryInternalException(result.error)
         }

core/src/main/kotlin/net/corda/core/transactions/SignedTransaction.kt

@@ -265,8 +265,11 @@ data class SignedTransaction(val txBits: SerializedBytes<CoreTransaction>,
     override fun toString(): String = "${javaClass.simpleName}(id=$id)"
 
     private companion object {
-        private fun missingSignatureMsg(missing: Set<PublicKey>, descriptions: List<String>, id: SecureHash): String =
-                "Missing signatures for $descriptions on transaction ${id.prefixChars()} for ${missing.joinToString()}"
+        private fun missingSignatureMsg(missing: Set<PublicKey>, descriptions: List<String>, id: SecureHash): String {
+            return "Missing signatures on transaction ${id.prefixChars()} for " +
+                    "keys: ${missing.joinToString { it.toStringShort() }}, " +
+                    "by signers: ${descriptions.joinToString()} "
+        }
     }
 
     @KeepForDJVM

node/src/main/kotlin/net/corda/node/services/transactions/NonValidatingNotaryFlow.kt

@@ -1,38 +1,25 @@
 package net.corda.node.services.transactions
 
-import co.paralleluniverse.fibers.Suspendable
 import net.corda.core.contracts.ComponentGroupEnum
 import net.corda.core.flows.FlowSession
 import net.corda.core.flows.NotarisationPayload
-import net.corda.core.flows.NotarisationRequest
-import net.corda.core.internal.notary.SinglePartyNotaryService
 import net.corda.core.internal.notary.NotaryServiceFlow
+import net.corda.core.internal.notary.SinglePartyNotaryService
 import net.corda.core.transactions.ContractUpgradeFilteredTransaction
-import net.corda.core.transactions.CoreTransaction
 import net.corda.core.transactions.FilteredTransaction
 import net.corda.core.transactions.NotaryChangeWireTransaction
 
+/**
+ * The received transaction is not checked for contract-validity, as that would require fully
+ * resolving it into a [TransactionForVerification], for which the caller would have to reveal the whole transaction
+ * history chain.
+ * As a result, the Notary _will commit invalid transactions_ as well, but as it also records the identity of
+ * the caller, it is possible to raise a dispute and verify the validity of the transaction and subsequently
+ * undo the commit of the input states (the exact mechanism still needs to be worked out).
+ */
 class NonValidatingNotaryFlow(otherSideSession: FlowSession, service: SinglePartyNotaryService) : NotaryServiceFlow(otherSideSession, service) {
-    /**
-     * The received transaction is not checked for contract-validity, as that would require fully
-     * resolving it into a [TransactionForVerification], for which the caller would have to reveal the whole transaction
-     * history chain.
-     * As a result, the Notary _will commit invalid transactions_ as well, but as it also records the identity of
-     * the caller, it is possible to raise a dispute and verify the validity of the transaction and subsequently
-     * undo the commit of the input states (the exact mechanism still needs to be worked out).
-     */
-    @Suspendable
-    override fun validateRequest(requestPayload: NotarisationPayload): TransactionParts {
-        val transaction = requestPayload.coreTransaction
-        checkInputs(transaction.inputs + transaction.references)
-        val request = NotarisationRequest(transaction.inputs, transaction.id)
-        validateRequestSignature(request, requestPayload.requestSignature)
-        val parts = extractParts(transaction)
-        checkNotary(parts.notary)
-        return parts
-    }
-
-    private fun extractParts(tx: CoreTransaction): TransactionParts {
+    override fun extractParts(requestPayload: NotarisationPayload): TransactionParts {
+        val tx = requestPayload.coreTransaction
         return when (tx) {
             is FilteredTransaction -> {
                 tx.apply {
@@ -43,7 +30,7 @@ class NonValidatingNotaryFlow(otherSideSession: FlowSession, service: SinglePart
                 }
                 TransactionParts(tx.id, tx.inputs, tx.timeWindow, tx.notary, tx.references)
             }
-            is ContractUpgradeFilteredTransaction -> TransactionParts(tx.id, tx.inputs, null, tx.notary)
+            is ContractUpgradeFilteredTransaction,
             is NotaryChangeWireTransaction -> TransactionParts(tx.id, tx.inputs, null, tx.notary)
             else -> {
                 throw IllegalArgumentException("Received unexpected transaction type: ${tx::class.java.simpleName}," +

node/src/main/kotlin/net/corda/node/services/transactions/ValidatingNotaryFlow.kt

@@ -2,19 +2,16 @@ package net.corda.node.services.transactions
 
 import co.paralleluniverse.fibers.Suspendable
 import net.corda.core.contracts.TimeWindow
-import net.corda.core.contracts.TransactionVerificationException
 import net.corda.core.flows.FlowSession
 import net.corda.core.flows.NotarisationPayload
-import net.corda.core.flows.NotarisationRequest
 import net.corda.core.flows.NotaryError
 import net.corda.core.internal.ResolveTransactionsFlow
-import net.corda.core.internal.notary.SinglePartyNotaryService
 import net.corda.core.internal.notary.NotaryInternalException
 import net.corda.core.internal.notary.NotaryServiceFlow
+import net.corda.core.internal.notary.SinglePartyNotaryService
 import net.corda.core.transactions.SignedTransaction
 import net.corda.core.transactions.TransactionWithSignatures
 import net.corda.core.transactions.WireTransaction
-import java.security.SignatureException
 
 /**
  * A notary commit flow that makes sure a given transaction is valid before committing it. This does mean that the calling
@@ -22,29 +19,25 @@ import java.security.SignatureException
  * has its input states "blocked" by a transaction from another party, and needs to establish whether that transaction was
  * indeed valid.
  */
-class ValidatingNotaryFlow(otherSideSession: FlowSession, service: SinglePartyNotaryService) : NotaryServiceFlow(otherSideSession, service) {
+open class ValidatingNotaryFlow(otherSideSession: FlowSession, service: SinglePartyNotaryService) : NotaryServiceFlow(otherSideSession, service) {
+    override fun extractParts(requestPayload: NotarisationPayload): TransactionParts {
+        val stx = requestPayload.signedTransaction
+        val timeWindow: TimeWindow? = if (stx.coreTransaction is WireTransaction) stx.tx.timeWindow else null
+        return TransactionParts(stx.id, stx.inputs, timeWindow, stx.notary, stx.references)
+    }
+
     /**
      * Fully resolves the received transaction and its dependencies, runs contract verification logic and checks that
      * the transaction in question has all required signatures apart from the notary's.
      */
     @Suspendable
-    override fun validateRequest(requestPayload: NotarisationPayload): TransactionParts {
+    override fun verifyTransaction(requestPayload: NotarisationPayload) {
         try {
             val stx = requestPayload.signedTransaction
-            checkInputs(stx.inputs + stx.references)
-            validateRequestSignature(NotarisationRequest(stx.inputs, stx.id), requestPayload.requestSignature)
-            val notary = stx.notary
-            checkNotary(notary)
             resolveAndContractVerify(stx)
             verifySignatures(stx)
-            val timeWindow: TimeWindow? = if (stx.coreTransaction is WireTransaction) stx.tx.timeWindow else null
-            return TransactionParts(stx.id, stx.inputs, timeWindow, notary!!, stx.references)
         } catch (e: Exception) {
-            throw when (e) {
-                is TransactionVerificationException,
-                is SignatureException -> NotaryInternalException(NotaryError.TransactionInvalid(e))
-                else -> e
-            }
+            throw  NotaryInternalException(NotaryError.TransactionInvalid(e))
         }
     }
 
@@ -60,10 +53,6 @@ class ValidatingNotaryFlow(otherSideSession: FlowSession, service: SinglePartyNo
     }
 
     private fun checkSignatures(tx: TransactionWithSignatures) {
-        try {
-            tx.verifySignaturesExcept(service.notaryIdentityKey)
-        } catch (e: SignatureException) {
-            throw NotaryInternalException(NotaryError.TransactionInvalid(e))
-        }
+        tx.verifySignaturesExcept(service.notaryIdentityKey)
     }
 }

node/src/test/kotlin/net/corda/node/services/TimedFlowTests.kt

@@ -13,7 +13,6 @@ import net.corda.core.internal.FlowIORequest
 import net.corda.core.internal.ResolveTransactionsFlow
 import net.corda.core.internal.bufferUntilSubscribed
 import net.corda.core.internal.concurrent.openFuture
-import net.corda.core.internal.notary.NotaryServiceFlow
 import net.corda.core.internal.notary.SinglePartyNotaryService
 import net.corda.core.internal.notary.UniquenessProvider
 import net.corda.core.node.NotaryInfo
@@ -22,6 +21,7 @@ import net.corda.core.transactions.TransactionBuilder
 import net.corda.core.utilities.ProgressTracker
 import net.corda.core.utilities.seconds
 import net.corda.node.services.api.ServiceHubInternal
+import net.corda.node.services.transactions.ValidatingNotaryFlow
 import net.corda.nodeapi.internal.DevIdentityGenerator
 import net.corda.nodeapi.internal.network.NetworkParametersCopier
 import net.corda.testing.common.internal.testNetworkParameters
@@ -174,20 +174,21 @@ class TimedFlowTests {
         override val uniquenessProvider = object : UniquenessProvider {
             /** A dummy commit method that immediately returns a success message. */
             override fun commit(states: List<StateRef>, txId: SecureHash, callerIdentity: Party, requestSignature: NotarisationRequestSignature, timeWindow: TimeWindow?, references: List<StateRef>): CordaFuture<UniquenessProvider.Result> {
-                return openFuture<UniquenessProvider.Result>(). apply {
+                return openFuture<UniquenessProvider.Result>().apply {
                     set(UniquenessProvider.Result.Success)
                 }
             }
         }
+
         override fun createServiceFlow(otherPartySession: FlowSession): FlowLogic<Void?> = TestNotaryFlow(otherPartySession, this)
         override fun start() {}
         override fun stop() {}
     }
 
     /** A notary flow that will yield without returning a response on the very first received request. */
-    private class TestNotaryFlow(otherSide: FlowSession, service: TestNotaryService) : NotaryServiceFlow(otherSide, service) {
+    private class TestNotaryFlow(otherSide: FlowSession, service: TestNotaryService) : ValidatingNotaryFlow(otherSide, service) {
         @Suspendable
-        override fun validateRequest(requestPayload: NotarisationPayload): TransactionParts {
+        override fun verifyTransaction(requestPayload: NotarisationPayload) {
             val myIdentity = serviceHub.myInfo.legalIdentities.first()
             MDC.put("name", myIdentity.name.toString())
             logger.info("Received a request from ${otherSideSession.counterparty.name}")
@@ -201,7 +202,6 @@ class TimedFlowTests {
             } else {
                 logger.info("Processing")
             }
-            return TransactionParts(stx.id, stx.inputs, stx.tx.timeWindow, stx.notary)
         }
     }
 }

samples/notary-demo/src/main/kotlin/net/corda/notarydemo/MyCustomNotaryService.kt

@@ -1,20 +1,19 @@
 package net.corda.notarydemo
 
 import co.paralleluniverse.fibers.Suspendable
-import net.corda.core.contracts.TimeWindow
-import net.corda.core.contracts.TransactionVerificationException
-import net.corda.core.flows.*
+import net.corda.core.flows.FlowLogic
+import net.corda.core.flows.FlowSession
+import net.corda.core.flows.NotarisationPayload
+import net.corda.core.flows.NotaryError
 import net.corda.core.internal.ResolveTransactionsFlow
 import net.corda.core.internal.notary.NotaryInternalException
-import net.corda.core.internal.notary.NotaryServiceFlow
 import net.corda.core.internal.notary.SinglePartyNotaryService
 import net.corda.core.transactions.SignedTransaction
 import net.corda.core.transactions.TransactionWithSignatures
-import net.corda.core.transactions.WireTransaction
 import net.corda.node.services.api.ServiceHubInternal
 import net.corda.node.services.transactions.PersistentUniquenessProvider
+import net.corda.node.services.transactions.ValidatingNotaryFlow
 import java.security.PublicKey
-import java.security.SignatureException
 
 /**
  * A custom notary service should provide a constructor that accepts two parameters of types [ServiceHubInternal] and [PublicKey].
@@ -35,28 +34,15 @@ class MyCustomValidatingNotaryService(override val services: ServiceHubInternal,
 
 @Suppress("UNUSED_PARAMETER")
 // START 2
-class MyValidatingNotaryFlow(otherSide: FlowSession, service: MyCustomValidatingNotaryService) : NotaryServiceFlow(otherSide, service) {
-    /**
-     * The received transaction is checked for contract-validity, for which the caller also has to to reveal the whole
-     * transaction dependency chain.
-     */
-    @Suspendable
-    override fun validateRequest(requestPayload: NotarisationPayload): TransactionParts {
+class MyValidatingNotaryFlow(otherSide: FlowSession, service: MyCustomValidatingNotaryService) : ValidatingNotaryFlow(otherSide, service) {
+    override fun verifyTransaction(requestPayload: NotarisationPayload) {
         try {
             val stx = requestPayload.signedTransaction
-            validateRequestSignature(NotarisationRequest(stx.inputs, stx.id), requestPayload.requestSignature)
-            val notary = stx.notary
-            checkNotary(notary)
-            verifySignatures(stx)
             resolveAndContractVerify(stx)
-            val timeWindow: TimeWindow? = if (stx.coreTransaction is WireTransaction) stx.tx.timeWindow else null
-            return TransactionParts(stx.id, stx.inputs, timeWindow, notary!!, stx.references)
+            verifySignatures(stx)
+            customVerify(stx)
         } catch (e: Exception) {
-            throw when (e) {
-                is TransactionVerificationException,
-                is SignatureException -> NotaryInternalException(NotaryError.TransactionInvalid(e))
-                else -> e
-            }
+            throw  NotaryInternalException(NotaryError.TransactionInvalid(e))
         }
     }
 
@@ -64,7 +50,6 @@ class MyValidatingNotaryFlow(otherSide: FlowSession, service: MyCustomValidating
     private fun resolveAndContractVerify(stx: SignedTransaction) {
         subFlow(ResolveTransactionsFlow(stx, otherSideSession))
         stx.verify(serviceHub, false)
-        customVerify(stx)
     }
 
     private fun verifySignatures(stx: SignedTransaction) {
@@ -73,11 +58,7 @@ class MyValidatingNotaryFlow(otherSide: FlowSession, service: MyCustomValidating
     }
 
     private fun checkSignatures(tx: TransactionWithSignatures) {
-        try {
-            tx.verifySignaturesExcept(service.notaryIdentityKey)
-        } catch (e: SignatureException) {
-            throw NotaryInternalException(NotaryError.TransactionInvalid(e))
-        }
+        tx.verifySignaturesExcept(service.notaryIdentityKey)
     }
 
     private fun customVerify(stx: SignedTransaction) {