KV: do not use "Inputs" part of InputsAndEffects #9429

remyhaemmerle-da · 2021-04-16T09:06:42Z

CHANGELOG_BEGIN
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.

miklos-da

@remyhaemmerle-da If you merge #9361 you won't need this PR. Am I missing something?

remyhaemmerle-da · 2021-04-16T09:37:07Z

@remyhaemmerle-da If you merge #9361 you won't need this PR. Am I missing something?

I think it is better to go this way. Review #9361 and make it only about Effect and make this PR about Inputs only.

In the concrete case we do not need a special computation of Input because almost everything (expect contract key) is already in GenTransaction.

I tried to address some of the comment you make in #9361, here.

daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/Transaction.scala

cocreature · 2021-04-16T09:51:15Z

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

+        (tx.informees ++ submitterInfo.actAs).toList.map(Conversions.partyStateKey)
+      val contractIdStates =
+        tx.inputContracts[ContractId].map(Conversions.contractIdToStateKey)
+      val contractKeyStates =


Why do we treat contract keys created with the transaction as inputs but don’t do the same for contract ids? That seems a bit inconsistent. I assume it has something to do with duplicate key detection or something along those lines?

As far I understand, for validation, KV has to collect beforehand all the info needed to replay the transaction. For created contract, it need to know the contract key does not exists.

alright so the input is the “non-existence of the key” or something like that, makes sense 👍

The PR is based #9429. CHANGELOG_BEGIN CHANGELOG_END

CHANGELOG_BEGIN CHANGELOG_END

daml-lf/transaction-test-lib/src/main/scala/lf/transaction/test/TransactionBuilder.scala

ghost · 2021-04-19T07:35:27Z

daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/Transaction.scala

+  final def contractKeys(implicit
+      ev: HasTxNodes[Nid, Cid] <:< HasTxNodes[_, Value.ContractId]
+  ): Set[GlobalKey] = {
+    ev(this).fold(Set.empty[GlobalKey]) {


I'd find this a little prettier with some kind of foldMap. Is it worth implementing that?

Could be sensible but I’d prefer keeping it for a separate PR.

daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/transaction/TransactionSpec.scala

…ansaction/TransactionSpec.scala Co-authored-by: Samir Talwar <samir.talwar@digitalasset.com>

changelog_begin changelog_end

ghost

I'm happy but if I were you, I'd wait for @miklos-da or @fabiotudone-da to verify. I am not too familiar with this part of the code.

The PR is based #9429. CHANGELOG_BEGIN CHANGELOG_END

daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/Transaction.scala

daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/transaction/TransactionSpec.scala

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

Co-authored-by: fabiotudone-da <fabio.tudone@digitalasset.com>

changelog_begin changelog_end

The PR is based #9429. CHANGELOG_BEGIN CHANGELOG_END . changelog_begin changelog_end . changelog_begin changelog_end . changelog_begin changelog_end

The PR is based #9429. changelog_begin changelog_end

miklos-da · 2021-04-19T13:37:19Z

daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/Transaction.scala

+    * that refer transient contracts or that appear under a rollback node.
+    */
+  final def contractKeys(implicit
+      ev: HasTxNodes[Nid, Cid] <:< HasTxNodes[_, Value.ContractId]


What is an ev? Can you use meaningful parameter names, please?

ev is pretty conventional in Scala-ese for "evidence".

I prefer the latter, but I think it's quite common (kind of like "i" for an iterator or "e" for an exception).

Thanks for the explanation, @SamirTalwar-DA.

The method Iterable#toMap is the standard example of method using such implicit (and syntax)

* KV: do not use "Effects" part of InputsAndEffects The PR is based #9429. changelog_begin changelog_end * Factor out helpers changelog_begin changelog_end * Apply suggestions from code review Co-authored-by: Miklos <57664299+miklos-da@users.noreply.github.com> * review comments changelog_begin changelog_end * fmt changelog_begin changelog_end Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org> Co-authored-by: Miklos <57664299+miklos-da@users.noreply.github.com>

@remyhaemmerle-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @remyhaemmerle-da is in charge of this release. Commit log: ``` cf2be78 Support rollback nodes in BlindingInfo (#9445) adde90b KV: do not use "Effects" part of InputsAndEffects (#9433) 75fa86a Additional metrics for mutable contract state cache (#9444) d8c34a4 Rename normalization-related params in the integrity checker config (#9455) e335244 Hash submitters in the deduplication key [DPP-347] (#9417) 43ffa42 Test for duplicate contracts when querying on behalf of multiple parties (#9443) 7644844 Document daml ledger export script (#9446) 3193027 DPP-334 Manually partition the event table (#9394) 14661a8 Clearer variable name for subtype evidence (#9442) 40c85d8 Release v1.13.0-snapshot.20210419.6730.0.8c3a8c04 (#9439) 1cb907c KV: do not use "Inputs" part of InputsAndEffects (#9429) ``` Changelog: ``` - [Integration Kit] - deduplication key will contain hashed submitters instead of concatenated submitters * [Daml export] Refer to the "Ledger Export" chapter under the "Early Access Features" for a description of the new Daml ledger export command. This is an early access feature. ``` CHANGELOG_BEGIN CHANGELOG_END

remyhaemmerle-da

Thanks

remyhaemmerle-da requested review from fabiotudone-da, gerolf-da and miklos-da as code owners April 16, 2021 09:06

remyhaemmerle-da requested a review from a user April 16, 2021 09:06

miklos-da reviewed Apr 16, 2021

View reviewed changes

cocreature approved these changes Apr 16, 2021

View reviewed changes

remyhaemmerle-da requested a review from S11001001 as a code owner April 16, 2021 11:54

remyhaemmerle-da changed the title ~~KV: refactor transactionToSubmission to not use InputsAndEffects~~ KV: do not use "Inputs" part of InputsAndEffects Apr 16, 2021

remyhaemmerle-da added a commit that referenced this pull request Apr 16, 2021

KV: do not use "Effects" part of InputsAndEffects

f2a4d5c

The PR is based #9429. CHANGELOG_BEGIN CHANGELOG_END

This was referenced Apr 16, 2021

KV: do not use "Effects" part of InputsAndEffects #9433

Merged

LF: Move KV's InputsAndEffects to LF land #9361

Closed

remyhaemmerle-da added a commit that referenced this pull request Apr 16, 2021

KV: do not use "Effects" part of InputsAndEffects

2561f7a

The PR is based #9429. CHANGELOG_BEGIN CHANGELOG_END

cocreature self-assigned this Apr 16, 2021

remyhaemmerle-da added 5 commits April 19, 2021 09:27

KV: refactor transactionToSubmission to not use InputsAndEffects

07b8e24

CHANGELOG_BEGIN CHANGELOG_END

cosmetic

0f07599

Address Moritz' review

10a2cad

cleanup

ac6a5cb

delete wrong files

96f2d82

cocreature force-pushed the remy-kv-input branch from 6f17c71 to 96f2d82 Compare April 19, 2021 07:27

ghost reviewed Apr 19, 2021

View reviewed changes

cocreature and others added 2 commits April 19, 2021 09:38

Update daml-lf/transaction/src/test/scala/com/digitalasset/daml/lf/tr…

0aad1bf

…ansaction/TransactionSpec.scala Co-authored-by: Samir Talwar <samir.talwar@digitalasset.com>

Fix borked merge

0667cc7

changelog_begin changelog_end

ghost approved these changes Apr 19, 2021

View reviewed changes

cocreature pushed a commit that referenced this pull request Apr 19, 2021

KV: do not use "Effects" part of InputsAndEffects

ec081be

The PR is based #9429. CHANGELOG_BEGIN CHANGELOG_END

fabiotudone-da reviewed Apr 19, 2021

View reviewed changes

cocreature and others added 2 commits April 19, 2021 11:14

Apply suggestions from code review

b57a817

Co-authored-by: fabiotudone-da <fabio.tudone@digitalasset.com>

Extract submissionParties

7ace682

changelog_begin changelog_end

fabiotudone-da approved these changes Apr 19, 2021

View reviewed changes

cocreature added the automerge label Apr 19, 2021

mergify bot merged commit 1cb907c into main Apr 19, 2021

mergify bot deleted the remy-kv-input branch April 19, 2021 11:11

cocreature pushed a commit that referenced this pull request Apr 19, 2021

KV: do not use "Effects" part of InputsAndEffects

70e714e

The PR is based #9429. CHANGELOG_BEGIN CHANGELOG_END . changelog_begin changelog_end . changelog_begin changelog_end . changelog_begin changelog_end

cocreature pushed a commit that referenced this pull request Apr 19, 2021

KV: do not use "Effects" part of InputsAndEffects

861c0cc

The PR is based #9429. changelog_begin changelog_end

cocreature pushed a commit that referenced this pull request Apr 19, 2021

KV: do not use "Effects" part of InputsAndEffects

8476a49

The PR is based #9429. changelog_begin changelog_end

miklos-da reviewed Apr 19, 2021

View reviewed changes

remyhaemmerle-da commented May 3, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KV: do not use "Inputs" part of InputsAndEffects #9429

KV: do not use "Inputs" part of InputsAndEffects #9429

remyhaemmerle-da commented Apr 16, 2021

miklos-da left a comment

remyhaemmerle-da commented Apr 16, 2021

cocreature Apr 16, 2021

remyhaemmerle-da Apr 16, 2021 •

edited

Loading

cocreature Apr 16, 2021

ghost Apr 19, 2021

cocreature Apr 19, 2021

ghost left a comment

miklos-da Apr 19, 2021

ghost Apr 19, 2021

cocreature Apr 19, 2021

miklos-da Apr 19, 2021

remyhaemmerle-da May 3, 2021

remyhaemmerle-da left a comment •

edited

Loading

KV: do not use "Inputs" part of InputsAndEffects #9429

KV: do not use "Inputs" part of InputsAndEffects #9429

Conversation

remyhaemmerle-da commented Apr 16, 2021

Pull Request Checklist

miklos-da left a comment

Choose a reason for hiding this comment

remyhaemmerle-da commented Apr 16, 2021

Choose a reason for hiding this comment

remyhaemmerle-da Apr 16, 2021 • edited 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

ghost left a 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

remyhaemmerle-da left a comment • edited Loading

Choose a reason for hiding this comment

remyhaemmerle-da Apr 16, 2021 •

edited

Loading

remyhaemmerle-da left a comment •

edited

Loading