kutils: Make the validation and the preloading of PackageCommitter parametric #7460

remyhaemmerle-da · 2020-09-22T17:06:27Z

This PR creates 3 validation modes:

Strict : Specifies that the committer should validate the package
before committing them to the ledger. When using this mode, the
packages committed to the ledger can be fully trusted and do not
have to be validated when loaded into the engine.
Lenient: Specifies that the committer should perform a fast
validation of the packages before committing them to the ledger.
This mode is useful for ledger integrations that cannot handle
long-running submissions (> 10s). When using this mode, the
packages committed to the ledger cannot be trusted and must be
validated every time they are loaded into the engine.
No : Specifies that the committer should not perform any
validation the packages before committing them to the ledger. This
should be used only by non distributed ledgers, like DAML-on-SQL,
where the validation done in the API server can be trusted.

This PR creates 3 preloading modes:

Synchronous : Specifies that the packages should be preloading
into the engine before committed.
Asynchronous: Specify that the packages should be preloaded into
the engine asynchronously with the rest of the commit process. This
mode is useful for ledger integrations that cannot handle
long-running submissions (> 10s). Failure of the preloading process
will not affect the commit.
Preloading: Specify that the packages should not be preloaded into
the engine.

CHANGELOG_BEGIN

[Integration Kit] In kvutils, add metric
daml.kvutils.committer.package_upload.validate_timer to track
package validation time.

CHANGELOG_END

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

ghost

What's the migration plan? Are we going to leave it in post-commit mode for v1.6 and then switch to pre-commit mode in 1.7? Make pre-commit opt-in forever? Something else?

Whatever the plan, please document it in the changelog.

...r/ledger-on-memory/src/main/scala/com/daml/ledger/on/memory/InMemoryLedgerReaderWriter.scala

...s/src/main/scala/com/daml/ledger/participant/state/kvutils/CommitPackageValidationMode.scala

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageCommitter.scala

ledger/metrics/src/main/scala/com/daml/metrics/Metrics.scala

...-state/kvutils/app/src/main/scala/com/daml/ledger/participant/state/kvutils/app/Config.scala

ghost

Looks like the right direction. It would be good to have a test case for the different validation modes in kvutils. Since it isn't clear how we roll out the change I would at this point strive to get the functionality in (with enough test coverage!) and then work on getting consensus on how to roll this out.

daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala

...s/src/main/scala/com/daml/ledger/participant/state/kvutils/CommitPackageValidationMode.scala

...te/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/KeyValueCommitting.scala

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageCommitter.scala

...s/src/main/scala/com/daml/ledger/participant/state/kvutils/CommitPackageValidationMode.scala

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageCommitter.scala

remyhaemmerle-da · 2020-09-29T13:58:04Z

What's the migration plan? Are we going to leave it in post-commit mode for v1.6 and then switch to pre-commit mode in 1.7? Make pre-commit opt-in forever? Something else?

Whatever the plan, please document it in the changelog.

Currently there is no migration plan. We just add the possibility of pre-commit validation.
I will let the ledger team decide exactly how to roll out the change.
As far I understand what @dajmaki explained me, we cannot simply migrate, some ledger support long submission run but some other don't, so we will have to continue supporting the 3 different modes.

remyhaemmerle-da · 2020-09-29T14:18:43Z

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageValidator.scala

+      "decode_entry" -> decodeEntry,
+      "validate_entry" -> validateEntry,
+      "filter_duplicates" -> filterDuplicates,
+      "preload" -> preload,


Do we need synchronous preloading ?

No I don't think we need that, unless it's more efficient to validate+preload when committing rather than validating and loading the package to engine separately.

Actually, I think the system is more predictable if we preload it synchronously when committing as that means as soon as the package upload completes we can start using the package without having a surprising delay on the first transaction. What do you think?

I see a trade off:

If we preload we are indeed more predictable. It is also slightly more efficient as we decode only once. (I think our bigger dars need around 20s to decode).

If we don't, we do not overload the engine with useless packages. Potentially the code for scenario/trigger/script can end in the dar even it is useless for the Ledger engine.

I see the feature useful for testing/development. In production, dar upload should be so seldom that it should not really matter.

I think the preload should by synchronous for the pro-preload reasons you both mentioned.

We will preload in the 3 cases.

...c/main/scala/com/daml/ledger/participant/state/kvutils/committer/DummyPackageCommitter.scala

ghost · 2020-09-29T14:46:54Z

...ils/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageUploader.scala

+
+  override protected val committerName: String = "package_upload"
+
+  metrics.daml.kvutils.committer.packageUpload.loadedPackages(() =>


Not sure if this is inherited from earlier code or not, but this would only update the metric once at startup which isn't the idea. It should be done from within a step or probably preferably this is a metric that the engine should maintain and it shouldn't belong here.

It is from the original code.
Currently the Engine does not handle metric. If we want to do so, I will prefer wrapping it in a kv specific class.
For now, I moved this code at the end of the preload step.

ghost · 2020-09-30T09:53:17Z

daml-lf/validation/src/main/scala/com/digitalasset/daml/lf/validation/ValidationError.scala

+  override def context: Context = NoContext
+
+  override protected def prettyInternal: String =
+    s"the set of packages ${pkgIds.mkString("{", ",", "}")} is not self consistent, it missing the dependencies ${pkgIds


Suggested change

s"the set of packages ${pkgIds.mkString("{", ",", "}")} is not self consistent, it missing the dependencies ${pkgIds

s"The set of packages ${pkgIds.mkString("{", ",", "}")} is not self-consistent, it is missing the dependencies ${pkgIds

ghost · 2020-09-30T10:47:14Z

...s/src/main/scala/com/daml/ledger/participant/state/kvutils/CommitPackageValidationMode.scala

+  // This mode is useful for ledger integrations that cannot handle
+  // long-running submissions (> 10s).
+  // See PackageCommitter for more detail.
+  // FIXME: Currently `PostCommit` is the mode used by default


We should have an issue and a plan for addressing this. Probably not worth leaving a FIXME in the code.

I opened KVL-578 to make the validation mode configurable.

ghost · 2020-09-30T10:55:53Z

...ils/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageUploader.scala

+ * When using this committer, packages on the ledger cannot be trusted and should be
+ * validated each time uploaded to the engine.
+ */
+private[committer] class PackageUploader(


Sorry for not bringing this up in previous review round, but the naming here is also confusing. Could you name these so that it's clear to the reader that these are different "PackageCommitter" variants? E.g. NonValidatingPackageCommitter, HashValidatingPackageCommitter and maybe FullValidatingPackageCommitter (or better names if you can think of them). This way it's clear at a glance what these are.

I agree with Jussi.

ghost · 2020-09-30T10:58:01Z

...cala/com/daml/ledger/participant/state/kvutils/committer/NonValidatingPackageCommitter.scala

+
+  override protected val steps: Iterable[(StepInfo, Step)] =
+    List(
+      "deduplicate_submission" -> deduplicateSubmission,


It makes me little bit nervous that the step lists are duplicated. If one needs to add a new step one needs to remember to add it to all three places. Since all of them are extra validation steps the order isn't as important, so could it be safer to have e.g. "extra_validation_steps" or some such that gets spliced into steps in PackageCommitter? What do you think?

I share @dajmaki's concern.

gerolf-da · 2020-09-30T15:28:29Z

...te/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/KeyValueCommitting.scala

  private val logger = LoggerFactory.getLogger(this.getClass)

-  def this(engine: Engine, metrics: Metrics) = this(engine, metrics, false)
+  def this(engine: Engine, metrics: Metrics) =
+    this(engine, metrics, false, CommitPackageValidationMode.PostCommit)


We should avoid having multiple places with default values. I think this constructor should also have the packageValidation parameter. Any concerns with such an API change, @miklos-da?

I dropped the default values.

...cala/com/daml/ledger/participant/state/kvutils/committer/NonValidatingPackageCommitter.scala

gerolf-da · 2020-09-30T15:29:55Z

...cala/com/daml/ledger/participant/state/kvutils/committer/NonValidatingPackageCommitter.scala

+
+  override protected val steps: Iterable[(StepInfo, Step)] =
+    List(
+      "deduplicate_submission" -> deduplicateSubmission,


I share @dajmaki's concern.

gerolf-da · 2020-09-30T15:37:08Z

...ils/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageUploader.scala

+ * When using this committer, packages on the ledger cannot be trusted and should be
+ * validated each time uploaded to the engine.
+ */
+private[committer] class PackageUploader(


I agree with Jussi.

gerolf-da · 2020-09-30T15:40:59Z

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageValidator.scala

+      "decode_entry" -> decodeEntry,
+      "validate_entry" -> validateEntry,
+      "filter_duplicates" -> filterDuplicates,
+      "preload" -> preload,


I think the preload should by synchronous for the pro-preload reasons you both mentioned.

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageCommitter.scala

daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/Reader.scala

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageCommitter.scala

...t/suite/scala/com/daml/ledger/participant/state/kvutils/committer/PackageCommitterSpec.scala

...ib/scala/com/daml/ledger/participant/state/kvutils/ParticipantStateIntegrationSpecBase.scala

gerolf-da

Thanks, @remyhaemmerle-da! The separation into preloading and validation is great!

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageCommitter.scala

...c/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageValidationMode.scala

gerolf-da · 2020-10-08T07:12:26Z

...ib/scala/com/daml/ledger/participant/state/kvutils/ParticipantStateIntegrationSpecBase.scala

@@ -712,6 +710,15 @@ object ParticipantStateIntegrationSpecBase {
  private val darFile = new File(rlocation("ledger/test-common/model-tests.dar"))
  private val archives = darReader.readArchiveFromFile(darFile).get.all

+  // 2 self consistent archives


They don't have dependencies to other archives.

This PR creates 3 validation modes: + `Strict` : Specifies that the committer should validate the package before committing them to the ledger. When using this mode, the packages committed to the ledger can be fully trusted and do not have to be validated when loaded into the engine. + `Lenient`: Specifies that the committer should perform a fast validation of the packages before committing them to the ledger. This mode is useful for ledger integrations that cannot handle long-running submissions (> 10s). When using this mode, the packages committed to the ledger cannot be trusted and must be validated every time they are loaded into the engine. + `No` : Specifies that the committer should not perform any validation the packages before committing them to the ledger. This should be used only by non distributed ledgers, like DAML-on-SQL, where the validation done in the API server is can be trusted. This PR creates 3 preloading modes: + `Synchronous` : Specifies that the packages should be preloading into the engine before committed. + `Asynchronous`: Specify that the packages should be preloaded into the engine asynchronously with the rest of the commit process. This mode is useful for ledger integrations that cannot handle long-running submissions (> 10s). Failure of the preloading process will not affect the commit. + `Preloading`: Specify that the packages should not be preloaded into the engine. CHANGELOG_BEGIN - [Integration Kit] In kvutils, add metric `daml.kvutils.committer.package_upload.validate_timer` to track package validation time. CHANGELOG_END

Co-authored-by: Miklos <57664299+miklos-da@users.noreply.github.com>

Co-authored-by: Gerolf Seitz <gerolf.seitz@daml.com> Co-authored-by: Miklos <57664299+miklos-da@users.noreply.github.com>

azure-pipelines · 2020-10-08T11:11:57Z

Command 'rerun' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

azure-pipelines · 2020-10-08T11:12:40Z

Azure Pipelines successfully started running 1 pipeline(s).

This PR fixes a bug in the PackageCommitter introduced recently. Dependencies not decoded are assumed to be already preloaded and should not bee looked for. CHANGELOG_BEGIN CHANGELOG_END

remyhaemmerle-da force-pushed the remy-engine-validate-2 branch 4 times, most recently from 91f3b9f to 8e20001 Compare September 23, 2020 07:06

ghost reviewed Sep 23, 2020

View reviewed changes

gerolf-da reviewed Sep 24, 2020

View reviewed changes

remyhaemmerle-da force-pushed the remy-engine-validate-2 branch from 2caec22 to 41b70d7 Compare September 29, 2020 13:10

remyhaemmerle-da added the component/dlik DAML Ledger Integration Kit label Sep 29, 2020

remyhaemmerle-da added this to the HTTP JSON API Maintenance milestone Sep 29, 2020

remyhaemmerle-da force-pushed the remy-engine-validate-2 branch from 41b70d7 to bb1e49f Compare September 29, 2020 13:52

remyhaemmerle-da force-pushed the remy-engine-validate-2 branch 2 times, most recently from 316bd88 to 77f0126 Compare September 29, 2020 14:17

remyhaemmerle-da commented Sep 29, 2020

View reviewed changes

ghost reviewed Sep 29, 2020

View reviewed changes

ghost reviewed Sep 30, 2020

View reviewed changes

gerolf-da reviewed Sep 30, 2020

View reviewed changes

remyhaemmerle-da force-pushed the remy-engine-validate-2 branch 5 times, most recently from fb23c6b to 89ff6d7 Compare October 2, 2020 13:07

gerolf-da reviewed Oct 2, 2020

View reviewed changes

...ls/src/main/scala/com/daml/ledger/participant/state/kvutils/committer/PackageCommitter.scala Outdated Show resolved Hide resolved

remyhaemmerle-da force-pushed the remy-engine-validate-2 branch 3 times, most recently from 850d0e7 to d3b6c9c Compare October 5, 2020 08:26

remyhaemmerle-da marked this pull request as ready for review October 5, 2020 08:28

remyhaemmerle-da requested review from dasormeter and fabiotudone-da as code owners October 5, 2020 08:28

remyhaemmerle-da requested review from miklos-da, mziolekda, rautenrieth-da and stefanobaghino-da as code owners October 5, 2020 08:28

remyhaemmerle-da force-pushed the remy-engine-validate-2 branch from d3b6c9c to bed80e2 Compare October 5, 2020 08:54

remyhaemmerle-da changed the title ~~Add pre commit package validation in KV~~ kutils: Make validation and preloading parametric in PackageCommitter Oct 5, 2020

miklos-da reviewed Oct 5, 2020

View reviewed changes

remyhaemmerle-da changed the title ~~kutils: Make validation and preloading parametric in PackageCommitter~~ kutils: Make the validation and the preloading of PackageCommitter parametric Oct 6, 2020

remyhaemmerle-da requested review from gerolf-da and miklos-da October 6, 2020 07:44

gerolf-da approved these changes Oct 8, 2020

View reviewed changes

remyhaemmerle-da and others added 7 commits October 8, 2020 11:30

cosmetic

b2243cf

Apply suggestions from code review

fb5b114

Co-authored-by: Miklos <57664299+miklos-da@users.noreply.github.com>

Address Miklos' review

cb00800

cosmetic

4eb7d07

Apply suggestions from code review

5d85c9c

Co-authored-by: Gerolf Seitz <gerolf.seitz@daml.com> Co-authored-by: Miklos <57664299+miklos-da@users.noreply.github.com>

Address reviews

0a4599b

remyhaemmerle-da force-pushed the remy-engine-validate-2 branch from 0e9aa62 to 0a4599b Compare October 8, 2020 09:30

remyhaemmerle-da merged commit be35f3a into master Oct 8, 2020

remyhaemmerle-da deleted the remy-engine-validate-2 branch October 8, 2020 13:03

remyhaemmerle-da mentioned this pull request Oct 9, 2020

Ledger: Validate Packages as part of the package upload #7399

Closed

stefanobaghino-da added component/json-api HTTP JSON API team/ledger-clients Related to the Ledger Clients team's components. labels Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kutils: Make the validation and the preloading of PackageCommitter parametric #7460

kutils: Make the validation and the preloading of PackageCommitter parametric #7460

remyhaemmerle-da commented Sep 22, 2020 •

edited

Loading

ghost left a comment

ghost left a comment

remyhaemmerle-da commented Sep 29, 2020

remyhaemmerle-da Sep 29, 2020

ghost Sep 29, 2020

ghost Sep 29, 2020

remyhaemmerle-da Sep 29, 2020

remyhaemmerle-da Sep 29, 2020

gerolf-da Sep 30, 2020

remyhaemmerle-da Oct 2, 2020

ghost Sep 29, 2020

remyhaemmerle-da Sep 29, 2020

ghost Sep 30, 2020 •

edited by gerolf-da

Loading

ghost Sep 30, 2020

gerolf-da Sep 30, 2020

ghost Sep 30, 2020

gerolf-da Sep 30, 2020

ghost Sep 30, 2020

gerolf-da Sep 30, 2020

gerolf-da Sep 30, 2020

remyhaemmerle-da Oct 2, 2020

gerolf-da Sep 30, 2020

gerolf-da Sep 30, 2020

gerolf-da Sep 30, 2020

gerolf-da left a comment

gerolf-da Oct 8, 2020

azure-pipelines bot commented Oct 8, 2020

azure-pipelines bot commented Oct 8, 2020


		override protected val committerName: String = "package_upload"

		metrics.daml.kvutils.committer.packageUpload.loadedPackages(() =>

	s"the set of packages ${pkgIds.mkString("{", ",", "}")} is not self consistent, it missing the dependencies ${pkgIds
	s"The set of packages ${pkgIds.mkString("{", ",", "}")} is not self-consistent, it is missing the dependencies ${pkgIds

kutils: Make the validation and the preloading of PackageCommitter parametric #7460

kutils: Make the validation and the preloading of PackageCommitter parametric #7460

Conversation

remyhaemmerle-da commented Sep 22, 2020 • edited Loading

Pull Request Checklist

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

remyhaemmerle-da commented Sep 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 30, 2020 • edited by gerolf-da Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerolf-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-pipelines bot commented Oct 8, 2020

azure-pipelines bot commented Oct 8, 2020

remyhaemmerle-da commented Sep 22, 2020 •

edited

Loading

ghost Sep 30, 2020 •

edited by gerolf-da

Loading