From 494654cea6e12e162f4ae4411cb402cda96d0c69 Mon Sep 17 00:00:00 2001
From: Edoardo Ierina <eierina@users.noreply.github.com>
Date: Thu, 12 Nov 2020 17:03:43 +0000
Subject: [PATCH] CORDA-3823: hash agility updates for rc03 (#6800)

* wip

* wip

* wip (need to review IEE comments)

* wip

* wip

* Small refactoring, fixed network-verifier's TestNotaryFlow

* Added command line option to explicitly enable hash agility support

* wip-do-not-push

* wip

* wip

* wip

* aligned merkletree/transaction hash algorithms

* wip

* Added mixed algorithm support for nodes vs leaves and corrected mixed algorithm tests

* moved global computeNonce and componentHash to DigestService

* added comment for failing test to fix

* wip

* Minor cleanups, added deprecated componentHash/computeNonce

* restored exploratory changes to failing SignedTransaction test

* cleaned up and minor rafactoring

* Fixed some tests with hardcoded hash algorithm

* some changes and cleanups following code review

* WIP commit before large change

* WIP Fixed 3 tests

* WIP removed direct references to randomSHA256() and sha256()

* Updated/added liquibase migrations to support larger hash algorithms

* Reviewed, cleanups, comments, fixes

* removing direct references to sha256()

* WIP verifying obligations test errors

* reviewing obligation/attachment issues with sha3_256

* Full review before PR - intermediate commits

* Reviewed and cleaned up

* Futher cleanup

* Fixed partial tree backward compatible json and cleanups

* all tests passing

* Removed couple of unused imports

* Reworked global componentHash function to avoid deprecated warnings

* replaced SHA3s with some alternate SHA2s

* Removed SHA3-256 and SHA3-512 references

* fixed some tests using non ubiquitous hash algorithms

* Fixed ABI compatibility (not for TransactionBuilder)

* Fixed ABI compatibility to TransactionBuilder

* couple of fixes

* fixed DigestService's randomHash

* Removed constructor with loosely typed args for private constructor of LedgerTransaction class (API removal)

* re-introduced LedgerTransaction deprecated ctor for deserialization

* Add possibility to load CustomMessageDigest bypassing JCA (#6798)

* Change api-current for DigestAlgorithm

* disable flaky tests

* addressed liquibase migration script versions

* Removed TODOs and cleanups

* relaxed privacy salt validation

* Fixed privacy salt test to comply with relaxed validation

* detekt and privacySalt validation

* diff cleanup

* diff cleanup

* removed unused import

* removed PrivacySalt's validateFor method and references

* removed invalid character

Co-authored-by: Denis Rekalov <denis.rekalov@r3.com>
---
 .../corda/deterministic/contracts/PrivacySaltTest.kt |  6 ------
 .../transactions/CompatibleTransactionTests.kt       |  2 --
 .../kotlin/net/corda/core/contracts/Structures.kt    |  5 -----
 .../contracts/TransactionVerificationException.kt    |  1 -
 .../net/corda/core/internal/TransactionUtils.kt      | 12 ++----------
 .../core/transactions/ContractUpgradeTransactions.kt |  7 +++----
 .../net/corda/core/transactions/LedgerTransaction.kt |  9 +--------
 .../core/transactions/NotaryChangeTransactions.kt    |  8 --------
 .../net/corda/core/transactions/WireTransaction.kt   |  2 --
 .../corda/finance/contracts/asset/ObligationTests.kt |  2 --
 .../nodeapi/internal/serialization/kryo/Kryo.kt      |  3 ---
 .../corda/node/internal/schemas/NodeInfoSchema.kt    |  5 -----
 .../transactions/PersistentUniquenessProvider.kt     |  1 -
 .../kotlin/net/corda/notary/common/BatchSigning.kt   |  3 ---
 14 files changed, 6 insertions(+), 60 deletions(-)

diff --git a/core-deterministic/testing/src/test/kotlin/net/corda/deterministic/contracts/PrivacySaltTest.kt b/core-deterministic/testing/src/test/kotlin/net/corda/deterministic/contracts/PrivacySaltTest.kt
index 3b9854d63f..61fb7071a6 100644
--- a/core-deterministic/testing/src/test/kotlin/net/corda/deterministic/contracts/PrivacySaltTest.kt
+++ b/core-deterministic/testing/src/test/kotlin/net/corda/deterministic/contracts/PrivacySaltTest.kt
@@ -25,10 +25,4 @@ class PrivacySaltTest {
         val ex = assertFailsWith<IllegalArgumentException> { PrivacySalt(ByteArray(SALT_SIZE - 1) { 0x7f }) }
         assertEquals("Privacy salt should be at least 32 bytes.", ex.message)
     }
-
-    @Test(timeout=300_000)
-	fun testTooShortPrivacySaltForSHA512() {
-        val ex = assertFailsWith<IllegalArgumentException> { PrivacySalt(ByteArray(SALT_SIZE) { 0x7f }).apply { validateFor("SHA-512") } }
-        assertEquals("Privacy salt should be at least 64 bytes for SHA-512.", ex.message)
-    }
 }
diff --git a/core-tests/src/test/kotlin/net/corda/coretests/transactions/CompatibleTransactionTests.kt b/core-tests/src/test/kotlin/net/corda/coretests/transactions/CompatibleTransactionTests.kt
index d3a9ac43cb..10ea1e949c 100644
--- a/core-tests/src/test/kotlin/net/corda/coretests/transactions/CompatibleTransactionTests.kt
+++ b/core-tests/src/test/kotlin/net/corda/coretests/transactions/CompatibleTransactionTests.kt
@@ -417,8 +417,6 @@ class CompatibleTransactionTests {
     @Test(timeout=300_000)
 	fun `FilteredTransaction signer manipulation tests`() {
         // Required to call the private constructor.
-        //val ftxConstructor = FilteredTransaction::class.constructors.first()
-        // TODO(iee): verify if it was a hard requirement that the constructor must be the first()
         val ftxConstructor = FilteredTransaction::class.constructors.last()
 
         // 1st and 3rd commands require a signature from KEY_1.
diff --git a/core/src/main/kotlin/net/corda/core/contracts/Structures.kt b/core/src/main/kotlin/net/corda/core/contracts/Structures.kt
index 1bafcbb619..9172236207 100644
--- a/core/src/main/kotlin/net/corda/core/contracts/Structures.kt
+++ b/core/src/main/kotlin/net/corda/core/contracts/Structures.kt
@@ -340,11 +340,6 @@ class PrivacySalt(bytes: ByteArray) : OpaqueBytes(bytes) {
         require(bytes.size >= MINIMUM_SIZE) { "Privacy salt should be at least $MINIMUM_SIZE bytes." }
     }
 
-    fun validateFor(algorithm: String) {
-        val digestLength = SecureHash.digestLengthFor(algorithm)
-        require(bytes.size >= digestLength) { "Privacy salt should be at least $digestLength bytes for $algorithm." }
-    }
-
     companion object {
         private const val MINIMUM_SIZE = 32
 
diff --git a/core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt b/core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt
index ff43e2d138..e62feb845f 100644
--- a/core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt
+++ b/core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt
@@ -342,7 +342,6 @@ abstract class TransactionVerificationException(val txId: SecureHash, message: S
                     "At this time these are not loadable because the DJVM sandbox has not yet been integrated. " +
                     "You will need to manually install the CorDapp to whitelist it for use.")
 
-    // TODO(iee): verify if this is an acceptable exception
     @KeepForDJVM
     class UnsupportedHashTypeException(txId: SecureHash) : TransactionVerificationException(txId, "The transaction Id is defined by an unsupported hash type", null);
 
diff --git a/core/src/main/kotlin/net/corda/core/internal/TransactionUtils.kt b/core/src/main/kotlin/net/corda/core/internal/TransactionUtils.kt
index d9ba9e8903..e807329a92 100644
--- a/core/src/main/kotlin/net/corda/core/internal/TransactionUtils.kt
+++ b/core/src/main/kotlin/net/corda/core/internal/TransactionUtils.kt
@@ -5,7 +5,6 @@ import net.corda.core.contracts.*
 import net.corda.core.crypto.DigestService
 import net.corda.core.crypto.SecureHash
 import net.corda.core.crypto.algorithm
-import net.corda.core.crypto.hashAs
 import net.corda.core.crypto.internal.DigestAlgorithmFactory
 import net.corda.core.flows.FlowLogic
 import net.corda.core.identity.Party
@@ -50,19 +49,12 @@ class ContractUpgradeTransactionBuilder(
 }
 
 /** Concatenates the hash components into a single [ByteArray] and returns its hash. */
-fun combinedHash(components: Iterable<SecureHash>/*, digestService: DigestService = DigestService.default*/): SecureHash {
+fun combinedHash(components: Iterable<SecureHash>, digestService: DigestService): SecureHash {
     val stream = ByteArrayOutputStream()
     components.forEach {
         stream.write(it.bytes)
     }
-    // TODO(iee): need to re-visit and review this code to understand which is the right
-    //            way to combine the hashes. Is this meant to match a pre-existing tx id,
-    //            or create a new tx id with the [default] hash algorithm from whatever
-    //            components are passed in and independently from their algorithm?
-    //            This is used to build the tx id of ContractUpgradeFilteredTransaction and
-    //            ContractUpgradeWireTransaction
-    return stream.toByteArray().hashAs(components.first().algorithm)
-    //return digestService.hash(stream.toByteArray());
+    return digestService.hash(stream.toByteArray());
 }
 
 /**
diff --git a/core/src/main/kotlin/net/corda/core/transactions/ContractUpgradeTransactions.kt b/core/src/main/kotlin/net/corda/core/transactions/ContractUpgradeTransactions.kt
index 98065aa283..f1f53f90b8 100644
--- a/core/src/main/kotlin/net/corda/core/transactions/ContractUpgradeTransactions.kt
+++ b/core/src/main/kotlin/net/corda/core/transactions/ContractUpgradeTransactions.kt
@@ -88,7 +88,6 @@ data class ContractUpgradeWireTransaction(
     init {
         check(inputs.isNotEmpty()) { "A contract upgrade transaction must have inputs" }
         checkBaseInvariants()
-        // privacySalt.validateFor(digestService.hashAlgorithm)
     }
 
     /**
@@ -114,7 +113,7 @@ data class ContractUpgradeWireTransaction(
         val componentHashes = serializedComponents.mapIndexed { index, component ->
             digestService.componentHash(nonces[index], component)
         }
-        combinedHash(componentHashes/*, digestService*/)
+        combinedHash(componentHashes, digestService)
     }
 
     /** Required for filtering transaction components. */
@@ -211,7 +210,7 @@ data class ContractUpgradeFilteredTransaction(
          * Required for computing the transaction id.
          */
         val hiddenComponents: Map<Int, SecureHash>,
-        val digestService: DigestService = DigestService.sha2_256
+        val digestService: DigestService
 ) : CoreTransaction() {
 
     /**
@@ -250,7 +249,7 @@ data class ContractUpgradeFilteredTransaction(
                 else -> throw IllegalStateException("Missing component hashes")
             }
         }
-        combinedHash(hashList/*, digestService*/)
+        combinedHash(hashList, digestService)
     }
     override val outputs: List<TransactionState<ContractState>> get() = emptyList()
     override val references: List<StateRef> get() = emptyList()
diff --git a/core/src/main/kotlin/net/corda/core/transactions/LedgerTransaction.kt b/core/src/main/kotlin/net/corda/core/transactions/LedgerTransaction.kt
index 4e75a7567d..d65bb5209e 100644
--- a/core/src/main/kotlin/net/corda/core/transactions/LedgerTransaction.kt
+++ b/core/src/main/kotlin/net/corda/core/transactions/LedgerTransaction.kt
@@ -92,7 +92,7 @@ private constructor(
         private val isAttachmentTrusted: (Attachment) -> Boolean,
         private val verifierFactory: (LedgerTransaction, ClassLoader) -> Verifier,
         private val attachmentsClassLoaderCache: AttachmentsClassLoaderCache?,
-        val digestService: DigestService = DigestService.sha2_256
+        val digestService: DigestService
 ) : FullTransaction() {
 
     /**
@@ -120,13 +120,6 @@ private constructor(
             networkParameters, references, componentGroups, serializedInputs, serializedReferences,
             isAttachmentTrusted, verifierFactory, attachmentsClassLoaderCache, DigestService.sha2_256)
 
-    // TODO(iee): add missing => Removed from API txt
-    //  public <init>(java.util.List, java.util.List, java.util.List, java.util.List, net.corda.core.crypto.SecureHash,
-    //      net.corda.core.identity.Party, net.corda.core.contracts.TimeWindow, net.corda.core.contracts.PrivacySalt,
-    //      net.corda.core.node.NetworkParameters, java.util.List, java.util.List, java.util.List, java.util.List,
-    //      kotlin.jvm.functions.Function1, kotlin.jvm.functions.Function2,
-    //      net.corda.core.serialization.internal.AttachmentsClassLoaderCache, kotlin.jvm.internal.DefaultConstructorMarker)
-
     init {
         if (timeWindow != null) check(notary != null) { "Transactions with time-windows must be notarised" }
         checkNotaryWhitelisted()
diff --git a/core/src/main/kotlin/net/corda/core/transactions/NotaryChangeTransactions.kt b/core/src/main/kotlin/net/corda/core/transactions/NotaryChangeTransactions.kt
index 625a2febc6..d9fdee9173 100644
--- a/core/src/main/kotlin/net/corda/core/transactions/NotaryChangeTransactions.kt
+++ b/core/src/main/kotlin/net/corda/core/transactions/NotaryChangeTransactions.kt
@@ -50,14 +50,6 @@ data class NotaryChangeWireTransaction(
         return NotaryChangeWireTransaction(serializedComponents, DigestService.sha2_256)
     }
 
-    // TODO(iee): add missing:
-    //  public <init>(net.corda.core.identity.Party, java.util.UUID, java.util.List<net.corda.core.contracts.StateRef>,
-    //      java.util.List<net.corda.core.crypto.SecureHash>,
-    //      java.util.List<net.corda.core.contracts.TransactionState<net.corda.core.contracts.ContractState>>,
-    //      java.util.List<net.corda.core.contracts.Command<?>>, net.corda.core.contracts.TimeWindow,
-    //      net.corda.core.contracts.PrivacySalt, java.util.List<net.corda.core.contracts.StateRef>,
-    //      net.corda.core.node.ServiceHub)
-
     override val inputs: List<StateRef> = serializedComponents[INPUTS.ordinal].deserialize()
     override val references: List<StateRef> = emptyList()
     override val notary: Party = serializedComponents[NOTARY.ordinal].deserialize()
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 dea67bcd79..fbcb012d10 100644
--- a/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt
+++ b/core/src/main/kotlin/net/corda/core/transactions/WireTransaction.kt
@@ -80,8 +80,6 @@ class WireTransaction(componentGroups: List<ComponentGroup>, val privacySalt: Pr
         check(inputs.isNotEmpty() || outputs.isNotEmpty()) { "A transaction must contain at least one input or output state" }
         check(commands.isNotEmpty()) { "A transaction must contain at least one command" }
         if (timeWindow != null) check(notary != null) { "Transactions with time-windows must be notarised" }
-        // TODO(iee): review salt validation - Should we just warn when privacy bytes already >= 32 but <= digestLength?
-        // privacySalt.validateFor(digestService.hashAlgorithm)
     }
 
     /** The transaction id is represented by the root hash of Merkle tree over the transaction components. */
diff --git a/finance/contracts/src/test/kotlin/net/corda/finance/contracts/asset/ObligationTests.kt b/finance/contracts/src/test/kotlin/net/corda/finance/contracts/asset/ObligationTests.kt
index 28b02a29cd..6b0d18f71e 100644
--- a/finance/contracts/src/test/kotlin/net/corda/finance/contracts/asset/ObligationTests.kt
+++ b/finance/contracts/src/test/kotlin/net/corda/finance/contracts/asset/ObligationTests.kt
@@ -58,8 +58,6 @@ class ObligationTests {
         val MEGA_CORP_PUBKEY get() = megaCorp.publicKey
         val MINI_CORP get() = miniCorp.party
         val MINI_CORP_PUBKEY get() = miniCorp.publicKey
-        // TODO(iee): Obligations fail to find attachments if we use anything else than sha2_256. All attachment logic is still
-        //            using direct calls to .sha256() which still needs to be replaced with DigestService.default
     }
 
     @Rule
diff --git a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/kryo/Kryo.kt b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/kryo/Kryo.kt
index b08866495b..7866d51e08 100644
--- a/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/kryo/Kryo.kt
+++ b/node-api/src/main/kotlin/net/corda/nodeapi/internal/serialization/kryo/Kryo.kt
@@ -221,7 +221,6 @@ object WireTransactionSerializer : Serializer<WireTransaction>() {
     override fun read(kryo: Kryo, input: Input, type: Class<WireTransaction>): WireTransaction {
         val componentGroups: List<ComponentGroup> = uncheckedCast(kryo.readClassAndObject(input))
         val privacySalt = kryo.readClassAndObject(input) as PrivacySalt
-        // TODO(iee): handle backward compatibility when deserializing old version of WTX
         val digestService = kryo.readClassAndObject(input) as? DigestService
         return WireTransaction(componentGroups, privacySalt, digestService ?: DigestService.sha2_256)
     }
@@ -236,7 +235,6 @@ object NotaryChangeWireTransactionSerializer : Serializer<NotaryChangeWireTransa
 
     override fun read(kryo: Kryo, input: Input, type: Class<NotaryChangeWireTransaction>): NotaryChangeWireTransaction {
         val components: List<OpaqueBytes> = uncheckedCast(kryo.readClassAndObject(input))
-        // TODO(iee): handle backward compatibility when deserializing old version of NCWTX
         val digestService = kryo.readClassAndObject(input) as? DigestService
         return NotaryChangeWireTransaction(components, digestService ?: DigestService.sha2_256)
     }
@@ -253,7 +251,6 @@ object ContractUpgradeWireTransactionSerializer : Serializer<ContractUpgradeWire
     override fun read(kryo: Kryo, input: Input, type: Class<ContractUpgradeWireTransaction>): ContractUpgradeWireTransaction {
         val components: List<OpaqueBytes> = uncheckedCast(kryo.readClassAndObject(input))
         val privacySalt = kryo.readClassAndObject(input) as PrivacySalt
-        // TODO(iee): handle backward compatibility when deserializing old version of WTX
         val digestService = kryo.readClassAndObject(input) as? DigestService
         return ContractUpgradeWireTransaction(components, privacySalt, digestService ?: DigestService.sha2_256)
     }
diff --git a/node/src/main/kotlin/net/corda/node/internal/schemas/NodeInfoSchema.kt b/node/src/main/kotlin/net/corda/node/internal/schemas/NodeInfoSchema.kt
index da1c80d2c6..c7640267ab 100644
--- a/node/src/main/kotlin/net/corda/node/internal/schemas/NodeInfoSchema.kt
+++ b/node/src/main/kotlin/net/corda/node/internal/schemas/NodeInfoSchema.kt
@@ -29,11 +29,6 @@ object NodeInfoSchemaV1 : MappedSchema(
             @Column(name = "node_info_id", nullable = false)
             var id: Int,
 
-            // TODO(iee): this field receives a hardcoded SHA2_256 string that comes from SerializationAPI's
-            // SerializedBytes' hash method that calls directly to .sha256(). It appears to be used
-            // only for serialized NodeInfo instances. Requires review and discussion to determine if
-            // it should follow the node/system hash algorithm or remain sha256 (in case it does change,
-            // it will be necessary to change length from 64 to 144.
             @Suppress("MagicNumber") // database column width
             @Column(name = "node_info_hash", length = 64, nullable = false)
             val hash: String,
diff --git a/node/src/main/kotlin/net/corda/node/services/transactions/PersistentUniquenessProvider.kt b/node/src/main/kotlin/net/corda/node/services/transactions/PersistentUniquenessProvider.kt
index 8d0b6e1bf7..66ec2007fa 100644
--- a/node/src/main/kotlin/net/corda/node/services/transactions/PersistentUniquenessProvider.kt
+++ b/node/src/main/kotlin/net/corda/node/services/transactions/PersistentUniquenessProvider.kt
@@ -265,7 +265,6 @@ class PersistentUniquenessProvider(val clock: Clock, val database: CordaPersiste
     }
 
     private fun handleConflicts(txId: SecureHash, conflictingStates: LinkedHashMap<StateRef, StateConsumptionDetails>) {
-        // TODO(iee): is the use of reHash correct here? Check backward/forward compatibility
         if (isConsumedByTheSameTx(txId.reHash(), conflictingStates)) {
             log.info("Transaction $txId already notarised. TxId: $txId")
             return
diff --git a/node/src/main/kotlin/net/corda/notary/common/BatchSigning.kt b/node/src/main/kotlin/net/corda/notary/common/BatchSigning.kt
index ad25833db8..595b9c2095 100644
--- a/node/src/main/kotlin/net/corda/notary/common/BatchSigning.kt
+++ b/node/src/main/kotlin/net/corda/notary/common/BatchSigning.kt
@@ -25,12 +25,9 @@ fun signBatch(
     require(algorithms.size > 0) {
         "Cannot sign an empty batch"
     }
-    // TODO(iee): too strict? will be valid in the future?
     require(algorithms.size == 1) {
         "Cannot sign a batch with multiple hash algorithms: $algorithms"
     }
-    // TODO(iee): assuming this is running on a notary node, and therefore using only the default
-    //            hash algorithm. Review and discuss.
     val merkleTree = MerkleTree.getMerkleTree(txIds.map { it.reHash() }, services.digestService)
     val merkleTreeRoot = merkleTree.hash
     val signableData = SignableData(