From a630c04df1b3d3301b8c53a6d335fa1a90d73594 Mon Sep 17 00:00:00 2001 From: Remy Haemmerle Date: Thu, 22 Jul 2021 17:54:41 +0200 Subject: [PATCH] LF: drop old serializability check for Value We drop this check for the following reasons: * Its only usage was inside the transaction builder, this is subsumed by the check we added in the SValue to Value translation (#10370) * It seems to origianl check more that nesting (and so to be overcomplicate) * It was used in non-consitent way. It was used to check create arguments and choice arguments, but niether choice result nor contract key. CHANGELOG_BEGIN CHANGELOG_END --- .../daml/lf/speedy/SBuiltin.scala | 4 +- .../lf/transaction/PartialTransaction.scala | 266 ++++++++---------- .../lf/speedy/PartialTransactionSpec.scala | 31 +- .../digitalasset/daml/lf/value/Value.scala | 103 ------- .../daml/lf/value/ValueSpec.scala | 18 -- 5 files changed, 135 insertions(+), 287 deletions(-) diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SBuiltin.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SBuiltin.scala index 0e776e3d742c..f2546ecafa59 100644 --- a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SBuiltin.scala +++ b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SBuiltin.scala @@ -918,7 +918,7 @@ private[lf] object SBuiltin { mbKey.foreach { case Node.KeyWithMaintainers(key, maintainers) => if (maintainers.isEmpty) throw SErrorDamlException( - IE.CreateEmptyContractKeyMaintainers(templateId, createArg.toValue, key) + IE.CreateEmptyContractKeyMaintainers(templateId, createArgValue, key) ) } val auth = machine.auth @@ -933,7 +933,6 @@ private[lf] object SBuiltin { stakeholders = sigs union obs, key = mbKey, ) - .fold(err => crash(err), identity) machine.addLocalContract(coid, templateId, createArg, sigs, obs, mbKey) onLedger.ptx = newPtx @@ -993,7 +992,6 @@ private[lf] object SBuiltin { byKey = byKey, chosenValue = arg, ) - .fold(err => crash(err), identity) checkAborted(onLedger.ptx) machine.returnValue = SUnit } diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/transaction/PartialTransaction.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/transaction/PartialTransaction.scala index 5b9dc0c2fd95..bac4a6b59e38 100644 --- a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/transaction/PartialTransaction.scala +++ b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/transaction/PartialTransaction.scala @@ -395,102 +395,88 @@ private[lf] case class PartialTransaction( signatories: Set[Party], stakeholders: Set[Party], key: Option[Node.KeyWithMaintainers[Value[Nothing]]], - ): Either[String, (Value.ContractId, PartialTransaction)] = { - val serializableErrs = serializable(arg) - if (serializableErrs.nonEmpty) { - Left( - s"""Trying to create a contract with a non-serializable value: ${serializableErrs.iterator - .mkString(",")}""" - ) - } else { - val actionNodeSeed = context.nextActionChildSeed - val discriminator = - crypto.Hash.deriveContractDiscriminator(actionNodeSeed, submissionTime, stakeholders) - val cid = Value.ContractId.V1(discriminator) - val version = packageToTransactionVersion(templateId.packageId) - val createNode = Node.NodeCreate( - cid, - templateId, - arg, - agreementText, - signatories, - stakeholders, - key, - version, - ) - val nid = NodeId(nextNodeIdx) - val ptx = copy( - actionNodeLocations = actionNodeLocations :+ optLocation, - nextNodeIdx = nextNodeIdx + 1, - context = context.addActionChild(nid, version), - nodes = nodes.updated(nid, createNode), - actionNodeSeeds = actionNodeSeeds :+ actionNodeSeed, - localContracts = localContracts + cid, - ).noteAuthFails(nid, CheckAuthorization.authorizeCreate(optLocation, createNode), auth) - - // if we have a contract key being added, include it in the list of - // active keys - key match { - case None => Right((cid, ptx)) - case Some(kWithM) => - val ck = GlobalKey(templateId, kWithM.key) - - // Note (MK) Duplicate key checks in Speedy - // When run in ContractKeyUniquenessMode.On speedy detects duplicate contract keys errors. - // - // Just like for modifying `keys` and `globalKeyInputs` we only consider - // by-key operations, i.e., lookup, exercise and fetch by key as well as creates - // and archives if the key has been brought into scope before. - // - // In the end, those checks mean that ledgers only have to look at inputs and outputs - // of the transaction and check for conflicts on that while speedy checks for internal - // conflicts. - // - // We have to consider the following cases for conflicts: - // 1. Create of a new local contract - // 1.1. KeyInactive in `keys`. This means we saw an archive so the create is valid. - // 1.2. KeyActive(_) in `keys`. This can either be local contract or a global contract. Both are an error. - // 1.3. No entry in `keys` and no entry in `globalKeyInputs`. This is valid. Note that the ledger here will then - // have to check when committing that there is no active contract with this key before the transaction. - // 1.4. No entry in `keys` and `KeyInactive` in `globalKeyInputs`. This is valid. Ledgers need the same check - // as for 1.3. - // 1.5. No entry in `keys` and `KeyActive(_)` in `globalKeyInputs`. This is an error. Note that the case where - // the global contract has already been archived falls under 1.2. - // 2. Global key lookups - // 2.1. Conflicts with other global contracts cannot arise as we query a key at most once. - // 2.2. Conflicts with local contracts also cannot arise: A successful create will either - // 2.2.1: Set `globalKeyInputs` to `KeyInactive`. - // 2.2.2: Not modify `globalKeyInputs` if there already was an entry. - // For both of those cases `globalKeyInputs` already had an entry which means - // we would use that as a cached result and not query the ledger. - val conflict = keys.get(ck).orElse(ptx.globalKeyInputs.get(ck)) match { - case Some(KeyActive(_)) => KeyConflict.Duplicate - case Some(KeyInactive) | None => KeyConflict.None - } - val globalKeyInputs = keys.get(ck).orElse(ptx.globalKeyInputs.get(ck)) match { - case None => ptx.globalKeyInputs.updated(ck, KeyInactive) - case Some(_) => ptx.globalKeyInputs - } - (conflict, contractKeyUniqueness) match { - case (KeyConflict.Duplicate, ContractKeyUniquenessMode.On) => - Right((cid, ptx.noteAbort(Tx.DuplicateContractKey(ck)))) - case _ => - Right( - ( - cid, - ptx.copy( - keys = ptx.keys.updated(ck, KeyActive(cid)), - globalKeyInputs = globalKeyInputs, - ), - ) + ): (Value.ContractId, PartialTransaction) = { + val actionNodeSeed = context.nextActionChildSeed + val discriminator = + crypto.Hash.deriveContractDiscriminator(actionNodeSeed, submissionTime, stakeholders) + val cid = Value.ContractId.V1(discriminator) + val version = packageToTransactionVersion(templateId.packageId) + val createNode = Node.NodeCreate( + cid, + templateId, + arg, + agreementText, + signatories, + stakeholders, + key, + version, + ) + val nid = NodeId(nextNodeIdx) + val ptx = copy( + actionNodeLocations = actionNodeLocations :+ optLocation, + nextNodeIdx = nextNodeIdx + 1, + context = context.addActionChild(nid, version), + nodes = nodes.updated(nid, createNode), + actionNodeSeeds = actionNodeSeeds :+ actionNodeSeed, + localContracts = localContracts + cid, + ).noteAuthFails(nid, CheckAuthorization.authorizeCreate(optLocation, createNode), auth) + + // if we have a contract key being added, include it in the list of + // active keys + key match { + case None => cid -> ptx + case Some(kWithM) => + val ck = GlobalKey(templateId, kWithM.key) + + // Note (MK) Duplicate key checks in Speedy + // When run in ContractKeyUniquenessMode.On speedy detects duplicate contract keys errors. + // + // Just like for modifying `keys` and `globalKeyInputs` we only consider + // by-key operations, i.e., lookup, exercise and fetch by key as well as creates + // and archives if the key has been brought into scope before. + // + // In the end, those checks mean that ledgers only have to look at inputs and outputs + // of the transaction and check for conflicts on that while speedy checks for internal + // conflicts. + // + // We have to consider the following cases for conflicts: + // 1. Create of a new local contract + // 1.1. KeyInactive in `keys`. This means we saw an archive so the create is valid. + // 1.2. KeyActive(_) in `keys`. This can either be local contract or a global contract. Both are an error. + // 1.3. No entry in `keys` and no entry in `globalKeyInputs`. This is valid. Note that the ledger here will then + // have to check when committing that there is no active contract with this key before the transaction. + // 1.4. No entry in `keys` and `KeyInactive` in `globalKeyInputs`. This is valid. Ledgers need the same check + // as for 1.3. + // 1.5. No entry in `keys` and `KeyActive(_)` in `globalKeyInputs`. This is an error. Note that the case where + // the global contract has already been archived falls under 1.2. + // 2. Global key lookups + // 2.1. Conflicts with other global contracts cannot arise as we query a key at most once. + // 2.2. Conflicts with local contracts also cannot arise: A successful create will either + // 2.2.1: Set `globalKeyInputs` to `KeyInactive`. + // 2.2.2: Not modify `globalKeyInputs` if there already was an entry. + // For both of those cases `globalKeyInputs` already had an entry which means + // we would use that as a cached result and not query the ledger. + val conflict = keys.get(ck).orElse(ptx.globalKeyInputs.get(ck)) match { + case Some(KeyActive(_)) => KeyConflict.Duplicate + case Some(KeyInactive) | None => KeyConflict.None + } + val globalKeyInputs = keys.get(ck).orElse(ptx.globalKeyInputs.get(ck)) match { + case None => ptx.globalKeyInputs.updated(ck, KeyInactive) + case Some(_) => ptx.globalKeyInputs + } + (conflict, contractKeyUniqueness) match { + case (KeyConflict.Duplicate, ContractKeyUniquenessMode.On) => + cid -> ptx.noteAbort(Tx.DuplicateContractKey(ck)) + case _ => + cid -> + ptx.copy( + keys = ptx.keys.updated(ck, KeyActive(cid)), + globalKeyInputs = globalKeyInputs, ) - } - } + } } } - private[this] def serializable(a: Value[Value.ContractId]): ImmArray[String] = a.serializable() - def insertFetch( auth: Authorize, coid: Value.ContractId, @@ -557,62 +543,52 @@ private[lf] case class PartialTransaction( mbKey: Option[Node.KeyWithMaintainers[Value[Nothing]]], byKey: Boolean, chosenValue: Value[Value.ContractId], - ): Either[String, PartialTransaction] = { - val serializableErrs = serializable(chosenValue) - if (serializableErrs.nonEmpty) { - Left( - s"""Trying to exercise a choice with a non-serializable value: ${serializableErrs.iterator - .mkString(",")}""" + ): PartialTransaction = { + val nid = NodeId(nextNodeIdx) + val ec = + ExercisesContextInfo( + targetId = targetId, + templateId = templateId, + contractKey = mbKey, + choiceId = choiceId, + consuming = consuming, + actingParties = actingParties, + chosenValue = chosenValue, + signatories = signatories, + stakeholders = stakeholders, + choiceObservers = choiceObservers, + nodeId = nid, + parent = context, + byKey = byKey, ) - } else { - val nid = NodeId(nextNodeIdx) - val ec = - ExercisesContextInfo( - targetId = targetId, - templateId = templateId, - contractKey = mbKey, - choiceId = choiceId, - consuming = consuming, - actingParties = actingParties, - chosenValue = chosenValue, - signatories = signatories, - stakeholders = stakeholders, - choiceObservers = choiceObservers, - nodeId = nid, - parent = context, - byKey = byKey, - ) - Right( - mustBeActive( - targetId, - templateId, - copy( - actionNodeLocations = actionNodeLocations :+ optLocation, - nextNodeIdx = nextNodeIdx + 1, - context = Context(ec), - actionNodeSeeds = actionNodeSeeds :+ ec.actionNodeSeed, // must push before children - // important: the semantics of Daml dictate that contracts are immediately - // inactive as soon as you exercise it. therefore, mark it as consumed now. - consumedBy = if (consuming) consumedBy.updated(targetId, nid) else consumedBy, - keys = mbKey match { - case Some(kWithM) if consuming => - val gkey = GlobalKey(templateId, kWithM.key) - keys.get(gkey).orElse(globalKeyInputs.get(gkey)) match { - // An archive can only mark a key as inactive - // if it was brought into scope before. - case Some(KeyActive(cid)) if cid == targetId => - keys.updated(gkey, KeyInactive) - // If the key was not in scope or mapped to a different cid, we don’t change keys. Instead we will do - // an activeness check when looking it up later. - case _ => keys - } + mustBeActive( + targetId, + templateId, + copy( + actionNodeLocations = actionNodeLocations :+ optLocation, + nextNodeIdx = nextNodeIdx + 1, + context = Context(ec), + actionNodeSeeds = actionNodeSeeds :+ ec.actionNodeSeed, // must push before children + // important: the semantics of Daml dictate that contracts are immediately + // inactive as soon as you exercise it. therefore, mark it as consumed now. + consumedBy = if (consuming) consumedBy.updated(targetId, nid) else consumedBy, + keys = mbKey match { + case Some(kWithM) if consuming => + val gkey = GlobalKey(templateId, kWithM.key) + keys.get(gkey).orElse(globalKeyInputs.get(gkey)) match { + // An archive can only mark a key as inactive + // if it was brought into scope before. + case Some(KeyActive(cid)) if cid == targetId => + keys.updated(gkey, KeyInactive) + // If the key was not in scope or mapped to a different cid, we don’t change keys. Instead we will do + // an activeness check when looking it up later. case _ => keys - }, - ), - ).noteAuthFails(nid, CheckAuthorization.authorizeExercise(optLocation, ec), auth) - ) - } + } + case _ => keys + }, + ), + ).noteAuthFails(nid, CheckAuthorization.authorizeExercise(optLocation, ec), auth) } /** Close normally an exercise context. diff --git a/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/PartialTransactionSpec.scala b/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/PartialTransactionSpec.scala index a33de5542853..e261e87d5d3e 100644 --- a/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/PartialTransactionSpec.scala +++ b/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/PartialTransactionSpec.scala @@ -41,19 +41,18 @@ class PartialTransactionSpec extends AnyWordSpec with Matchers with Inside { private[this] implicit class PartialTransactionExtra(val ptx: PartialTransaction) { def insertCreate_ : PartialTransaction = - ptx.insertCreate( - Authorize(Set(party)), - templateId, - Value.ValueUnit, - "agreement", - None, - Set(party), - Set.empty, - None, - ) match { - case Right((_, newPtx)) => newPtx - case Left(_) => sys.error("unexpected error") - } + ptx + .insertCreate( + Authorize(Set(party)), + templateId, + Value.ValueUnit, + "agreement", + None, + Set(party), + Set.empty, + None, + ) + ._2 def beginExercises_ : PartialTransaction = ptx.beginExercises( @@ -70,11 +69,7 @@ class PartialTransactionSpec extends AnyWordSpec with Matchers with Inside { mbKey = None, byKey = false, chosenValue = Value.ValueUnit, - ) match { - case Right(value) => value - case Left(_) => - sys.error("unexpected failing beginExercises") - } + ) def endExercises_ : PartialTransaction = ptx.endExercises(Value.ValueUnit) diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala index d90985b5cd41..630eaf621b41 100644 --- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala +++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala @@ -11,7 +11,6 @@ import com.daml.lf.language.Ast import com.daml.lf.transaction.TransactionVersion import data.ScalazEqual._ -import scala.annotation.tailrec import scalaz.{@@, Equal, Order, Tag} import scalaz.Ordering.EQ import scalaz.std.option._ @@ -22,7 +21,6 @@ import scalaz.syntax.std.option._ /** Values */ sealed abstract class Value[+Cid] extends CidContainer[Value[Cid]] with Product with Serializable { - import Value._ final override protected def self: this.type = this @@ -32,107 +30,6 @@ sealed abstract class Value[+Cid] extends CidContainer[Value[Cid]] with Product private[lf] final def map1[Cid2](f: Cid => Cid2): Value[Cid2] = Value.map1(f)(this) - /** returns a list of validation errors: if the result is non-empty the value is - * _not_ serializable. - * - * note that this does not check the validity of the [[Identifier]]s, it just checks - * that the shape of the value is serializable. - */ - def serializable(): ImmArray[String] = { - @tailrec - def go( - exceededNesting: Boolean, - errs: BackStack[String], - vs0: FrontStack[(Value[Cid], Int)], - ): BackStack[String] = vs0 match { - case FrontStack() => errs - - case FrontStackCons((v, nesting), vs) => - // we cannot define helper functions because otherwise go is not tail recursive. fun! - val exceedsNestingErr = s"exceeds maximum nesting value of $MAXIMUM_NESTING" - val newNesting = nesting + 1 - - v match { - case ValueRecord(_, flds) => - if (newNesting > MAXIMUM_NESTING) { - if (exceededNesting) { - // we already exceeded the nesting, do not output again - go(exceededNesting, errs, vs) - } else { - go(true, errs :+ exceedsNestingErr, vs) - } - } else { - go(exceededNesting, errs, flds.map(v => (v._2, newNesting)) ++: vs) - } - - case ValueList(values) => - if (newNesting > MAXIMUM_NESTING) { - if (exceededNesting) { - // we already exceeded the nesting, do not output again - go(exceededNesting, errs, vs) - } else { - go(true, errs :+ exceedsNestingErr, vs) - } - } else { - go(exceededNesting, errs, values.toImmArray.map(v => (v, newNesting)) ++: vs) - } - - case ValueVariant(_, _, value) => - if (newNesting > MAXIMUM_NESTING) { - if (exceededNesting) { - // we already exceeded the nesting, do not output again - go(exceededNesting, errs, vs) - } else { - go(true, errs :+ exceedsNestingErr, vs) - } - } else { - go(exceededNesting, errs, (value, newNesting) +: vs) - } - - case _: ValueCidlessLeaf | _: ValueContractId[Cid] => - go(exceededNesting, errs, vs) - case ValueOptional(x) => - if (newNesting > MAXIMUM_NESTING) { - if (exceededNesting) { - // we already exceeded nesting, do not output again - go(exceededNesting, errs, vs) - } else { - go(true, errs :+ exceedsNestingErr, vs) - } - } else { - go(exceededNesting, errs, ImmArray(x.toList.map(v => (v, newNesting))) ++: vs) - } - case ValueTextMap(value) => - if (newNesting > MAXIMUM_NESTING) { - if (exceededNesting) { - // we already exceeded the nesting, do not output again - go(exceededNesting, errs, vs) - } else { - go(true, errs :+ exceedsNestingErr, vs) - } - } else { - go(exceededNesting, errs, value.values.map(v => (v, newNesting)) ++: vs) - } - case ValueGenMap(entries) => - if (newNesting > MAXIMUM_NESTING) { - if (exceededNesting) { - // we already exceeded the nesting, do not output again - go(exceededNesting, errs, vs) - } else { - go(true, errs :+ exceedsNestingErr, vs) - } - } else { - val vs1 = entries.foldLeft(vs) { case (acc, (k, v)) => - (k -> newNesting) +: (v -> newNesting) +: acc - } - go(exceededNesting, errs, vs1) - } - } - } - - go(false, BackStack.empty, FrontStack((this, 0))).toImmArray - } - def foreach1(f: Cid => Unit) = Value.foreach1(f)(this) diff --git a/daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/value/ValueSpec.scala b/daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/value/ValueSpec.scala index 0f1d840f5a2b..59bc4cd6d2be 100644 --- a/daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/value/ValueSpec.scala +++ b/daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/value/ValueSpec.scala @@ -32,24 +32,6 @@ class ValueSpec with ScalaCheckPropertyChecks { import ValueSpec._ - "serialize" - { - val exceedsNesting = (1 to MAXIMUM_NESTING + 1).foldRight[Value[Nothing]](ValueInt64(42)) { - case (_, v) => ValueVariant(None, Ref.Name.assertFromString("foo"), v) - } - val exceedsNestingError = s"exceeds maximum nesting value of $MAXIMUM_NESTING" - val matchesNesting = (1 to MAXIMUM_NESTING).foldRight[Value[Nothing]](ValueInt64(42)) { - case (_, v) => ValueVariant(None, Ref.Name.assertFromString("foo"), v) - } - - "rejects excessive nesting" in { - exceedsNesting.serializable() shouldBe ImmArray(exceedsNestingError) - } - - "accepts just right nesting" in { - matchesNesting.serializable() shouldBe ImmArray.empty - } - } - "VersionedValue" - { val pkgId = Ref.PackageId.assertFromString("pkgId")