From 0ea140f51eda91c909eccab86c1f7d393c8233e4 Mon Sep 17 00:00:00 2001 From: Marton Nagy Date: Tue, 3 May 2022 16:02:25 +0200 Subject: [PATCH] Fix random exercise node's childEventId order [DPP-1018] (#13740) Fixes Index DB insertion order for childEventId. Adds backwards compatibility treatment: recovering childEventId order from the order of events at API stream rendering. Extends TransactionServiceVisibilityIT with the ordering criterion. Fixes compatibility tests with exclusion. Adds documentation to event.proto. CHANGELOG_BEGIN Fixing Ledger API Bug: Exercise nodes in transaction trees have child_event_ids out of order. CHANGELOG_END --- compatibility/bazel_tools/testing.bzl | 12 ++ .../com/daml/ledger/api/v1/event.proto | 1 + .../platform/api/v1/event/EventOps.scala | 8 ++ .../BUILD.bazel | 1 + ledger/ledger-api-tests/suites/deps.bzl | 1 + .../v1_8/TransactionServiceVisibilityIT.scala | 106 +++++++++++++++++- .../scala/platform/store/backend/DbDto.scala | 2 +- .../store/backend/UpdateToDbDto.scala | 2 +- .../store/dao/events/EventsTable.scala | 11 +- .../TransactionLogUpdatesConversions.scala | 11 +- .../backend/StorageBackendTestValues.scala | 2 +- .../DbDtoToStringsForInterningSpec.scala | 2 +- .../store/backend/UpdateToDbDtoSpec.scala | 16 +-- ledger/sandbox-on-x/BUILD.bazel | 14 +++ 14 files changed, 172 insertions(+), 17 deletions(-) diff --git a/compatibility/bazel_tools/testing.bzl b/compatibility/bazel_tools/testing.bzl index b1aa4a50c9..9ae533a302 100644 --- a/compatibility/bazel_tools/testing.bzl +++ b/compatibility/bazel_tools/testing.bzl @@ -819,6 +819,18 @@ excluded_test_tool_tests = [ }, ], }, + { + # Fixing childEventId ordering: this test is now checking conformance to ordering, so it needs to be excluded for conformance tests which come after, and for versions which are older + "start": "2.2.0-snapshot.20220425.9780.1", + "platform_ranges": [ + { + "end": "2.2.0-snapshot.20220425.9780.0.f4d60375", + "exclusions": [ + "TransactionServiceVisibilityIT:TXTreeChildOrder", + ], + }, + ], + }, ] def in_range(version, range): diff --git a/ledger-api/grpc-definitions/com/daml/ledger/api/v1/event.proto b/ledger-api/grpc-definitions/com/daml/ledger/api/v1/event.proto index f596039ec1..12cd666f7d 100644 --- a/ledger-api/grpc-definitions/com/daml/ledger/api/v1/event.proto +++ b/ledger-api/grpc-definitions/com/daml/ledger/api/v1/event.proto @@ -167,6 +167,7 @@ message ExercisedEvent { // References to further events in the same transaction that appeared as a result of this ``ExercisedEvent``. // It contains only the immediate children of this event, not all members of the subtree rooted at this node. + // The order of the children is the same as the event order in the transaction. // Each element must be a valid LedgerString (as described in ``value.proto``). // Optional repeated string child_event_ids = 11; diff --git a/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/api/v1/event/EventOps.scala b/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/api/v1/event/EventOps.scala index 0d85663f73..be5747a2ca 100644 --- a/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/api/v1/event/EventOps.scala +++ b/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/api/v1/event/EventOps.scala @@ -93,6 +93,14 @@ object EventOps { TreeEvent(TreeExercised(exercise.copy(childEventIds = exercise.childEventIds.filter(f)))), create => TreeEvent(TreeCreated(create)), ) + def sortChildEventIdsBy(order: Map[String, Int]): TreeEvent = + event.kind.fold( + exercise => + TreeEvent( + TreeExercised(exercise.copy(childEventIds = exercise.childEventIds.sortBy(order))) + ), + create => TreeEvent(TreeCreated(create)), + ) def witnessParties: Seq[String] = event.kind.fold(_.witnessParties, _.witnessParties) def modifyWitnessParties(f: Seq[String] => Seq[String]): TreeEvent = event.kind.fold( diff --git a/ledger/ledger-api-test-tool-on-canton/BUILD.bazel b/ledger/ledger-api-test-tool-on-canton/BUILD.bazel index f8bd1065ca..1681f388f5 100644 --- a/ledger/ledger-api-test-tool-on-canton/BUILD.bazel +++ b/ledger/ledger-api-test-tool-on-canton/BUILD.bazel @@ -104,6 +104,7 @@ conformance_test( "TLSAtLeastOnePointTwoIT", "CommandDeduplicationPeriodValidationIT:OffsetPruned", # requires pruning not available in canton community "DeeplyNestedValueIT", # FIXME: Too deeply nested values flake with a time out (half of the time) + "TransactionServiceVisibilityIT:TXTreeChildOrder", # FIXME: Temporarily disabled: test enhanced testing childEventId order ]), ], ) if not is_windows else None diff --git a/ledger/ledger-api-tests/suites/deps.bzl b/ledger/ledger-api-tests/suites/deps.bzl index ad566c6e9b..535a20fe0c 100644 --- a/ledger/ledger-api-tests/suites/deps.bzl +++ b/ledger/ledger-api-tests/suites/deps.bzl @@ -4,6 +4,7 @@ def deps(lf_version): return [ "//daml-lf/data", + "//daml-lf/transaction", "//language-support/scala/bindings", "//ledger/error", "//ledger/ledger-api-common", diff --git a/ledger/ledger-api-tests/suites/src/main/scala/com/daml/ledger/api/testtool/suites/v1_8/TransactionServiceVisibilityIT.scala b/ledger/ledger-api-tests/suites/src/main/scala/com/daml/ledger/api/testtool/suites/v1_8/TransactionServiceVisibilityIT.scala index cb7b217dd9..4d0a634b87 100644 --- a/ledger/ledger-api-tests/suites/src/main/scala/com/daml/ledger/api/testtool/suites/v1_8/TransactionServiceVisibilityIT.scala +++ b/ledger/ledger-api-tests/suites/src/main/scala/com/daml/ledger/api/testtool/suites/v1_8/TransactionServiceVisibilityIT.scala @@ -21,6 +21,7 @@ import com.daml.ledger.test.model.Iou.IouTransfer._ import com.daml.ledger.test.model.IouTrade.IouTrade import com.daml.ledger.test.model.IouTrade.IouTrade._ import com.daml.ledger.test.model.Test._ +import com.daml.lf.ledger.EventId import com.daml.test.evidence.tag.EvidenceTag import com.daml.test.evidence.tag.Security.SecurityTest import com.daml.test.evidence.tag.Security.SecurityTest.Property.Privacy @@ -89,7 +90,6 @@ class TransactionServiceVisibilityIT extends LedgerTestSuite { dkkTree <- eventually("transactionTreeById3") { delta.transactionTreeById(tree.transactionId, dkk_bank) } - } yield { def treeIsWellformed(tree: TransactionTree): Unit = { val eventsToObserve = mutable.Map.empty[String, TreeEvent] ++= tree.eventsById @@ -141,6 +141,110 @@ class TransactionServiceVisibilityIT extends LedgerTestSuite { } }) + test( + "TXTreeChildOrder", + "Trees formed by childEventIds should be maintaining Transaction event order", + allocate(TwoParties, SingleParty, SingleParty), + enabled = _.committerEventLog.eventLogType.isCentralized, + tags = privacyHappyCase( + asset = "Transaction Tree", + happyCase = "Trees formed by childEventIds should be maintaining Transaction event order", + ), + )(implicit ec => { + case Participants( + Participant(alpha, alice, gbp_bank), + Participant(beta, bob), + Participant(delta, dkk_bank), + ) => + for { + gbpIouIssue <- alpha.create(gbp_bank, Iou(gbp_bank, gbp_bank, "GBP", 100, Nil)) + gbpTransfer <- + alpha.exerciseAndGetContract(gbp_bank, gbpIouIssue.exerciseIou_Transfer(_, alice)) + dkkIouIssue <- delta.create(dkk_bank, Iou(dkk_bank, dkk_bank, "DKK", 110, Nil)) + dkkTransfer <- + delta.exerciseAndGetContract(dkk_bank, dkkIouIssue.exerciseIou_Transfer(_, bob)) + + aliceIou1 <- eventually("exerciseIouTransfer_Accept") { + alpha.exerciseAndGetContract(alice, gbpTransfer.exerciseIouTransfer_Accept(_)) + } + aliceIou <- eventually("exerciseIou_AddObserver") { + alpha.exerciseAndGetContract(alice, aliceIou1.exerciseIou_AddObserver(_, bob)) + } + bobIou <- eventually("exerciseIouTransfer_Accept") { + beta.exerciseAndGetContract(bob, dkkTransfer.exerciseIouTransfer_Accept(_)) + } + + trade <- eventually("create") { + alpha.create( + alice, + IouTrade(alice, bob, aliceIou, gbp_bank, "GBP", 100, dkk_bank, "DKK", 110), + ) + } + tree <- eventually("exerciseIouTrade_Accept") { + beta.exercise(bob, trade.exerciseIouTrade_Accept(_, bobIou)) + } + + aliceTree <- eventually("transactionTreeById1") { + alpha.transactionTreeById(tree.transactionId, alice) + } + bobTree <- beta.transactionTreeById(tree.transactionId, bob) + gbpTree <- eventually("transactionTreeById2") { + alpha.transactionTreeById(tree.transactionId, gbp_bank) + } + dkkTree <- eventually("transactionTreeById3") { + delta.transactionTreeById(tree.transactionId, dkk_bank) + } + aliceTrees <- alpha.transactionTrees(alice) + bobTrees <- alpha.transactionTrees(bob) + gbpTrees <- alpha.transactionTrees(gbp_bank) + dkkTrees <- alpha.transactionTrees(dkk_bank) + } yield { + def treeIsWellformed(tree: TransactionTree): Unit = { + val eventsToObserve = mutable.Map.empty[String, TreeEvent] ++= tree.eventsById + + def go(eventId: String): Unit = { + eventsToObserve.remove(eventId) match { + case Some(TreeEvent(Exercised(exercisedEvent))) => + val expected = exercisedEvent.childEventIds.sortBy(stringEventId => + EventId.assertFromString(stringEventId).nodeId.index + ) + val actual = exercisedEvent.childEventIds + assertEquals( + context = s"childEventIds are out of order. Expected: $expected, actual: $actual", + actual = actual, + expected = expected, + ) + exercisedEvent.childEventIds.foreach(go) + case Some(TreeEvent(_)) => + () + case None => + throw new AssertionError( + s"Referenced eventId $eventId is not available as node in the transaction." + ) + } + } + + tree.rootEventIds.foreach(go) + assert( + eventsToObserve.isEmpty, + s"After traversing the transaction, there are still unvisited nodes: $eventsToObserve", + ) + } + + treeIsWellformed(aliceTree) + treeIsWellformed(bobTree) + treeIsWellformed(gbpTree) + treeIsWellformed(dkkTree) + + Iterator( + aliceTrees, + bobTrees, + gbpTrees, + dkkTrees, + ).flatten.foreach(treeIsWellformed) + } + }) + test( "TXNotDivulge", "Data should not be exposed to parties unrelated to a transaction", diff --git a/ledger/participant-integration-api/src/main/scala/platform/store/backend/DbDto.scala b/ledger/participant-integration-api/src/main/scala/platform/store/backend/DbDto.scala index 1d3b915dd7..6ffdd61349 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/store/backend/DbDto.scala +++ b/ledger/participant-integration-api/src/main/scala/platform/store/backend/DbDto.scala @@ -71,7 +71,7 @@ object DbDto { exercise_argument: Option[Array[Byte]], exercise_result: Option[Array[Byte]], exercise_actors: Option[Set[String]], - exercise_child_event_ids: Option[Set[String]], + exercise_child_event_ids: Option[Vector[String]], create_key_value_compression: Option[Int], exercise_argument_compression: Option[Int], exercise_result_compression: Option[Int], diff --git a/ledger/participant-integration-api/src/main/scala/platform/store/backend/UpdateToDbDto.scala b/ledger/participant-integration-api/src/main/scala/platform/store/backend/UpdateToDbDto.scala index 72007ebd16..7780267880 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/store/backend/UpdateToDbDto.scala +++ b/ledger/participant-integration-api/src/main/scala/platform/store/backend/UpdateToDbDto.scala @@ -217,7 +217,7 @@ object UpdateToDbDto { exercise_child_event_ids = Some( exercise.children.iterator .map(EventId(u.transactionId, _).toLedgerString.toString) - .toSet + .toVector ), create_key_value_compression = compressionStrategy.createKeyValueCompression.id, exercise_argument_compression = diff --git a/ledger/participant-integration-api/src/main/scala/platform/store/dao/events/EventsTable.scala b/ledger/participant-integration-api/src/main/scala/platform/store/dao/events/EventsTable.scala index 5c85e96e9e..6fcf445851 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/store/dao/events/EventsTable.scala +++ b/ledger/participant-integration-api/src/main/scala/platform/store/dao/events/EventsTable.scala @@ -87,14 +87,21 @@ object EventsTable { // The identifiers of all visible events in this transactions, preserving // the order in which they are retrieved from the index val visible = events.map(_.event.eventId) - val visibleSet = visible.toSet + val visibleOrder = visible.view.zipWithIndex.toMap // All events in this transaction by their identifier, with their children // filtered according to those visible for this request val eventsById = events.iterator .map(_.event) - .map(e => e.eventId -> e.filterChildEventIds(visibleSet)) + .map(e => + e.eventId -> e + .filterChildEventIds(visibleOrder.contains) + // childEventIds need to be returned in the event order in the original transaction. + // Unfortunately, we did not store them ordered in the past so we have to sort it to recover this order. + // The order is determined by the order of the events, which follows the event order of the original transaction. + .sortChildEventIdsBy(visibleOrder) + ) .toMap // All event identifiers that appear as a child of another item in this response diff --git a/ledger/participant-integration-api/src/main/scala/platform/store/dao/events/TransactionLogUpdatesConversions.scala b/ledger/participant-integration-api/src/main/scala/platform/store/dao/events/TransactionLogUpdatesConversions.scala index aa1276370b..9d27a1ee0d 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/store/dao/events/TransactionLogUpdatesConversions.scala +++ b/ledger/participant-integration-api/src/main/scala/platform/store/dao/events/TransactionLogUpdatesConversions.scala @@ -211,9 +211,16 @@ private[events] object TransactionLogUpdatesConversions { ) .map { treeEvents => val visible = treeEvents.map(_.eventId) - val visibleSet = visible.toSet + val visibleOrder = visible.view.zipWithIndex.toMap val eventsById = treeEvents.iterator - .map(e => e.eventId -> e.filterChildEventIds(visibleSet)) + .map(e => + e.eventId -> e + .filterChildEventIds(visibleOrder.contains) + // childEventIds need to be returned in the event order in the original transaction. + // Unfortunately, we did not store them ordered in the past so we have to sort it to recover this order. + // The order is determined by the order of the events, which follows the event order of the original transaction. + .sortChildEventIdsBy(visibleOrder) + ) .toMap // All event identifiers that appear as a child of another item in this response diff --git a/ledger/participant-integration-api/src/test/lib/scala/platform/store/backend/StorageBackendTestValues.scala b/ledger/participant-integration-api/src/test/lib/scala/platform/store/backend/StorageBackendTestValues.scala index 32fdbb39c9..6a4743994e 100644 --- a/ledger/participant-integration-api/src/test/lib/scala/platform/store/backend/StorageBackendTestValues.scala +++ b/ledger/participant-integration-api/src/test/lib/scala/platform/store/backend/StorageBackendTestValues.scala @@ -184,7 +184,7 @@ private[backend] object StorageBackendTestValues { exercise_argument = Some(someSerializedDamlLfValue), exercise_result = Some(someSerializedDamlLfValue), exercise_actors = Some(Set(actor)), - exercise_child_event_ids = Some(Set.empty), + exercise_child_event_ids = Some(Vector.empty), create_key_value_compression = None, exercise_argument_compression = None, exercise_result_compression = None, diff --git a/ledger/participant-integration-api/src/test/suite/scala/platform/store/backend/DbDtoToStringsForInterningSpec.scala b/ledger/participant-integration-api/src/test/suite/scala/platform/store/backend/DbDtoToStringsForInterningSpec.scala index 6d889c56ae..959cf42bfd 100644 --- a/ledger/participant-integration-api/src/test/suite/scala/platform/store/backend/DbDtoToStringsForInterningSpec.scala +++ b/ledger/participant-integration-api/src/test/suite/scala/platform/store/backend/DbDtoToStringsForInterningSpec.scala @@ -131,7 +131,7 @@ class DbDtoToStringsForInterningSpec extends AnyFlatSpec with Matchers { event_sequential_id = 1, exercise_choice = Some("60"), exercise_result = None, - exercise_child_event_ids = Some(Set("61", "62", "63")), + exercise_child_event_ids = Some(Vector("61", "62", "63")), exercise_result_compression = Some(1), ), DbDto.CommandCompletion( diff --git a/ledger/participant-integration-api/src/test/suite/scala/platform/store/backend/UpdateToDbDtoSpec.scala b/ledger/participant-integration-api/src/test/suite/scala/platform/store/backend/UpdateToDbDtoSpec.scala index 72035b180b..d9716be4db 100644 --- a/ledger/participant-integration-api/src/test/suite/scala/platform/store/backend/UpdateToDbDtoSpec.scala +++ b/ledger/participant-integration-api/src/test/suite/scala/platform/store/backend/UpdateToDbDtoSpec.scala @@ -473,7 +473,7 @@ class UpdateToDbDtoSpec extends AnyWordSpec with Matchers { exercise_argument = Some(emptyArray), exercise_result = Some(emptyArray), exercise_actors = Some(Set("signatory")), - exercise_child_event_ids = Some(Set.empty), + exercise_child_event_ids = Some(Vector.empty), create_key_value_compression = compressionAlgorithmId, exercise_argument_compression = compressionAlgorithmId, exercise_result_compression = compressionAlgorithmId, @@ -558,7 +558,7 @@ class UpdateToDbDtoSpec extends AnyWordSpec with Matchers { exercise_argument = Some(emptyArray), exercise_result = Some(emptyArray), exercise_actors = Some(Set("signatory")), - exercise_child_event_ids = Some(Set.empty), + exercise_child_event_ids = Some(Vector.empty), create_key_value_compression = compressionAlgorithmId, exercise_argument_compression = compressionAlgorithmId, exercise_result_compression = compressionAlgorithmId, @@ -670,7 +670,7 @@ class UpdateToDbDtoSpec extends AnyWordSpec with Matchers { exercise_result = Some(emptyArray), exercise_actors = Some(Set("signatory")), exercise_child_event_ids = Some( - Set( + Vector( EventId(update.transactionId, exerciseNodeBId).toLedgerString, EventId(update.transactionId, exerciseNodeCId).toLedgerString, ) @@ -700,7 +700,7 @@ class UpdateToDbDtoSpec extends AnyWordSpec with Matchers { exercise_argument = Some(emptyArray), exercise_result = Some(emptyArray), exercise_actors = Some(Set("signatory")), - exercise_child_event_ids = Some(Set.empty), + exercise_child_event_ids = Some(Vector.empty), create_key_value_compression = compressionAlgorithmId, exercise_argument_compression = compressionAlgorithmId, exercise_result_compression = compressionAlgorithmId, @@ -726,7 +726,7 @@ class UpdateToDbDtoSpec extends AnyWordSpec with Matchers { exercise_argument = Some(emptyArray), exercise_result = Some(emptyArray), exercise_actors = Some(Set("signatory")), - exercise_child_event_ids = Some(Set.empty), + exercise_child_event_ids = Some(Vector.empty), create_key_value_compression = compressionAlgorithmId, exercise_argument_compression = compressionAlgorithmId, exercise_result_compression = compressionAlgorithmId, @@ -881,7 +881,7 @@ class UpdateToDbDtoSpec extends AnyWordSpec with Matchers { exercise_argument = Some(emptyArray), exercise_result = Some(emptyArray), exercise_actors = Some(Set("signatory")), - exercise_child_event_ids = Some(Set.empty), + exercise_child_event_ids = Some(Vector.empty), create_key_value_compression = compressionAlgorithmId, exercise_argument_compression = compressionAlgorithmId, exercise_result_compression = compressionAlgorithmId, @@ -1010,7 +1010,7 @@ class UpdateToDbDtoSpec extends AnyWordSpec with Matchers { exercise_argument = Some(emptyArray), exercise_result = Some(emptyArray), exercise_actors = Some(Set("signatory")), - exercise_child_event_ids = Some(Set.empty), + exercise_child_event_ids = Some(Vector.empty), create_key_value_compression = compressionAlgorithmId, exercise_argument_compression = compressionAlgorithmId, exercise_result_compression = compressionAlgorithmId, @@ -1114,7 +1114,7 @@ class UpdateToDbDtoSpec extends AnyWordSpec with Matchers { exercise_argument = Some(emptyArray), exercise_result = Some(emptyArray), exercise_actors = Some(Set("signatory")), - exercise_child_event_ids = Some(Set.empty), + exercise_child_event_ids = Some(Vector.empty), create_key_value_compression = compressionAlgorithmId, exercise_argument_compression = compressionAlgorithmId, exercise_result_compression = compressionAlgorithmId, diff --git a/ledger/sandbox-on-x/BUILD.bazel b/ledger/sandbox-on-x/BUILD.bazel index e053d798cb..83e74ad3c9 100644 --- a/ledger/sandbox-on-x/BUILD.bazel +++ b/ledger/sandbox-on-x/BUILD.bazel @@ -506,6 +506,20 @@ conformance_test( name = "conformance-test-in-memory-fan-out", ports = [6865], server = ":app", + server_args = [ + "--contract-id-seeding=testing-weak", + "--participant participant-id=example,port=6865,ledger-api-transactions-buffer-max-size=100000", + "--buffered-ledger-api-streams", + ], + test_tool_args = [ + "--verbose", + ], +) + +conformance_test( + name = "conformance-test-in-memory-fan-out-tiny-buffer", + ports = [6865], + server = ":app", server_args = [ "--contract-id-seeding=testing-weak", "--participant participant-id=example,port=6865,ledger-api-transactions-buffer-max-size=10",