ENT-11458: Make sure external verifier is involved when verifying transactions in collect signatures flow (#7703)

* ENT-11458: Make sure external verifier is involved when verifying transactions in collect signatures flow

* Using SignedTransaction.verify(checkSufficientSignatures = false) after the observation that the current check for notSigned is effectively the same as just calling with checkSufficientSignatures = false.
This commit is contained in:
Shams Asari 2024-04-02 16:56:09 +01:00 committed by GitHub
parent 3ffd77add3
commit af62c36986
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 20 additions and 19 deletions

View File

@ -8,6 +8,7 @@ import net.corda.core.identity.AbstractParty
import net.corda.core.identity.AnonymousParty
import net.corda.core.identity.Party
import net.corda.core.identity.groupPublicKeysByWellKnownParty
import net.corda.core.internal.mapToSet
import net.corda.core.node.ServiceHub
import net.corda.core.transactions.SignedTransaction
import net.corda.core.transactions.WireTransaction
@ -90,7 +91,7 @@ class CollectSignaturesFlow @JvmOverloads constructor(val partiallySignedTx: Sig
// Check the signatures which have already been provided and that the transaction is valid.
// Usually just the Initiator and possibly an oracle would have signed at this point.
val myKeys: Iterable<PublicKey> = myOptionalKeys ?: listOf(ourIdentity.owningKey)
val signed = partiallySignedTx.sigs.map { it.by }
val signed = partiallySignedTx.sigs.mapToSet { it.by }
val notSigned = partiallySignedTx.tx.requiredSigningKeys - signed
// One of the signatures collected so far MUST be from the initiator of this flow.
@ -99,9 +100,7 @@ class CollectSignaturesFlow @JvmOverloads constructor(val partiallySignedTx: Sig
}
// The signatures must be valid and the transaction must be valid.
partiallySignedTx.verifySignaturesExcept(notSigned)
// TODO Should this be calling SignedTransaction.verify directly? https://r3-cev.atlassian.net/browse/ENT-11458
partiallySignedTx.tx.toLedgerTransaction(serviceHub).verify()
partiallySignedTx.verify(serviceHub, checkSufficientSignatures = false)
// Determine who still needs to sign.
progressTracker.currentStep = COLLECTING
@ -286,10 +285,7 @@ abstract class SignTransactionFlow @JvmOverloads constructor(val otherSideSessio
progressTracker.currentStep = VERIFYING
// Check that the Responder actually needs to sign.
checkMySignaturesRequired(stx, signingKeys)
// Check the signatures which have already been provided. Usually the Initiators and possibly an Oracle's.
checkSignatures(stx)
// TODO Should this be calling SignedTransaction.verify directly? https://r3-cev.atlassian.net/browse/ENT-11458
stx.tx.toLedgerTransaction(serviceHub).verify()
stx.verify(serviceHub, checkSufficientSignatures = false)
// Perform some custom verification over the transaction.
try {
checkTransaction(stx)
@ -310,14 +306,6 @@ abstract class SignTransactionFlow @JvmOverloads constructor(val otherSideSessio
return stx + mySignatures
}
@Suspendable
private fun checkSignatures(stx: SignedTransaction) {
val signed = stx.sigs.map { it.by }
val allSigners = stx.tx.requiredSigningKeys
val notSigned = allSigners - signed
stx.verifySignaturesExcept(notSigned)
}
/**
* The [checkTransaction] method allows the caller of this flow to provide some additional checks over the proposed
* transaction received from the counterparty. For example:

View File

@ -183,7 +183,10 @@ class AttachmentsClassLoader(attachments: List<Attachment>,
@Suppress("ThrowsCount", "ComplexMethod", "NestedBlockDepth")
private fun checkAttachments(attachments: List<Attachment>) {
require(attachments.isNotEmpty()) { "attachments list is empty" }
require(attachments.isNotEmpty()) {
"Transaction attachments list is empty. This can happen if verifying a legacy transaction (4.11 or older) with " +
"LedgerTransaction.verify(). Try using SignedTransaction.verify() instead."
}
// Here is where we enforce the no-overlap and package ownership rules.
//

View File

@ -240,6 +240,10 @@ private constructor(
* The reason for this is that classes (contract states) deserialized in this classloader would actually be a different type from what
* the calling code would expect.
*
* If receiving [SignedTransaction]s over the wire from other nodes, then it is recommended the verification be done via
* [SignedTransaction.verify] directly rather than via [SignedTransaction.toLedgerTransaction]. If transaction is from a legacy node
* (4.11 or older) then the later solution will not work.
*
* @throws TransactionVerificationException if anything goes wrong.
*/
@Throws(TransactionVerificationException::class)

View File

@ -693,8 +693,8 @@ open class TransactionBuilder(
@Throws(AttachmentResolutionException::class, TransactionResolutionException::class, TransactionVerificationException::class)
fun verify(services: ServiceHub) {
// TODO ENT-11445: Need to verify via SignedTransaction to ensure legacy components also work
toLedgerTransaction(services).verify()
// Make sure the external verifier is involved if the transaction has a legacy component.
toWireTransaction(services).tryVerify(services.toVerifyingServiceHub()).enforceSuccess()
}
private fun checkNotary(stateAndRef: StateAndRef<*>) {

View File

@ -375,6 +375,12 @@ class WireTransaction(componentGroups: List<ComponentGroup>, val privacySalt: Pr
sig.verify(id)
}
/**
* Perform verification of this [WireTransaction] and return the result as a [VerificationResult]. Depending on what types of attachments
* this transaction has, verification may have been done in-process by the node, or via the external verifier, or both.
*
* It's important that [VerificationResult.enforceSuccess] be called to make sure the verification was successful or its value analysed.
*/
@CordaInternal
@JvmSynthetic
internal fun tryVerify(verificationSupport: NodeVerificationSupport): VerificationResult {