CORDA-2570 address code review comments

core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt

@@ -107,6 +107,19 @@ abstract class TransactionVerificationException(val txId: SecureHash, message: S
     class ConflictingAttachmentsRejection(txId: SecureHash, val contractClass: String)
         : TransactionVerificationException(txId, "Contract constraints failed for: $contractClass, because multiple attachments providing this contract were attached.", null)
 
+    /**
+     * Indicates this transaction violates the "no overlap" rule: two attachments are trying to provide the same file
+     * path. Whereas Java classpaths would normally allow that with the first class taking precedence, this is not
+     * allowed in transactions for security reasons. This usually indicates that two separate apps share a dependency,
+     * in which case you could try 'shading the fat jars' to rename classes of dependencies. Or you could manually
+     * attach dependency JARs when building the transaction.
+     *
+     * @property contractClass The fully qualified class name of the failing contract.
+     */
+    @KeepForDJVM
+    class DuplicateAttachmentsRejection(txId: SecureHash, val attachmentId: Attachment)
+        : TransactionVerificationException(txId, "The attachment: $attachmentId was added multiple times.", null)
+
     /**
      * A [Contract] class named by a state could not be constructed. Most likely you do not have a no-argument
      * constructor, or the class doesn't subclass [Contract].

core/src/main/kotlin/net/corda/core/internal/ConstraintsUtils.kt

@@ -74,9 +74,6 @@ fun AttachmentConstraint.canBeTransitionedFrom(input: AttachmentConstraint, atta
         input is WhitelistedByZoneAttachmentConstraint && output is SignatureAttachmentConstraint ->
             attachment.signerKeys.isNotEmpty() && output.key.keys.containsAll(attachment.signerKeys)
 
-        // TODO Transition from Hash to Signature constraint
-        input is HashAttachmentConstraint && output is SignatureAttachmentConstraint -> false
-
         else -> false
     }
 }

core/src/main/kotlin/net/corda/core/internal/TransactionVerifierServiceInternal.kt

@@ -63,7 +63,7 @@ class Verifier(val ltx: LedgerTransaction,
         // 2. Check that the attachments satisfy the constraints of the states. (The contract verification code is correct.)
         verifyConstraints(contractAttachmentsByContract)
 
-        // 3. Check that the actual state constraints are correct. This is necessary because transactions can be build by potentially malicious nodes
+        // 3. Check that the actual state constraints are correct. This is necessary because transactions can be built by potentially malicious nodes
         // who can create output states with a weaker constraint which can be exploited in a future transaction.
         verifyConstraintsValidity(contractAttachmentsByContract)
 
@@ -82,21 +82,25 @@ class Verifier(val ltx: LedgerTransaction,
     private fun getUniqueContractAttachmentsByContract(): Map<ContractClassName, ContractAttachment> {
         val contractClasses = allStates.map { it.contract }.toSet()
 
+        // Check that there are no duplicate attachments added.
+        if (ltx.attachments.size != ltx.attachments.toSet().size) throw TransactionVerificationException.DuplicateAttachmentsRejection(ltx.id, ltx.attachments.groupBy { it }.filterValues { it.size > 1 }.keys.first())
+
         // For each attachment this finds all the relevant state contracts that it provides.
         // And then maps them to the attachment.
         val contractAttachmentsPerContract: List<Pair<ContractClassName, ContractAttachment>> = ltx.attachments
-                .mapNotNull { it as? ContractAttachment }
-                .distinctBy { it.id }
+                .mapNotNull { it as? ContractAttachment } // only contract attachments are relevant.
                 .flatMap { attachment ->
-                    contractClasses.filter { it in attachment.allContracts }.map { it to attachment }
+                    // Find which relevant contracts are present in the current attachment and return them as a list
+                    contractClasses
+                            .filter { it in attachment.allContracts }
+                            .map { it to attachment }
                 }
 
         // It is forbidden to add multiple attachments for the same contract.
         val contractWithMultipleAttachments = contractAttachmentsPerContract
-                .groupBy { it.first }
-                .filter { (_, attachments) -> attachments.size > 1 }
-                .keys
-                .firstOrNull()
+                .groupBy { it.first }  // Group by contract.
+                .filter { (_, attachments) -> attachments.size > 1 } // And only keep contracts that are in multiple attachments. It's guaranteed that attachments were unique by a previous check.
+                .keys.firstOrNull() // keep the first one - if any - to throw a meaningful exception.
         if (contractWithMultipleAttachments != null) throw TransactionVerificationException.ConflictingAttachmentsRejection(ltx.id, contractWithMultipleAttachments)
 
         val result = contractAttachmentsPerContract.toMap()