LF: fix contract ID freshness check

[remyhaemmerle-da]

Local contract ids are collected in exercise children during
preprocessing transaction for replay.

CHANGELOG_BEGIN

• [Engine] Fix contract ID fresness check when validating transaction

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.

[cocreature]

LGTM, thanks for fixing this so quickly!

[cocreature]

Can we get a testcase for this? This seems like a related but not the same bug. I think you can trigger it on a transaction like the following:

1. First do an exercise on a global cid.
2. Then do a create which produces the same discriminator as the global cid.

[cocreature]

Missed that this just got shuffled around but I still don’t see an existing test for it so it would be nice to add it.

[remyhaemmerle-da]

I will double check.

[remyhaemmerle-da]

It appear the test is not 100% necessary, as the dynamic check performed by the reinterpretation will catch those problematic cases.
I still prefer to let the test here. I add a comment and a test.

[andreaslochbihler-da]

Are you sure that this works? I'm a bit uneasy that we can't distinguish the following two scenarios:

Scenario 1:

1. There is a contract with cid cid0 on the ledger.
2. We submit a two-command transaction: Command 1's transaction tree contains a create node that creates a contract with local cid cid0. Command 2 is a plain exercise on the contract cid0 that exists on the ledger. However, replay now interprets this command as acting on the newly created contract.

Scenario 2:

1. There is no contract with cid cid0 on the ledger.
2. We submit a two-command transaction: Command 1's transaction tree contains a create node that creates a contract with local cid cid0 and key k. Command 2 is a by-key exercise on the key k.

One could probably argue that scenario 1 should have failed during interpretation in the submission service, but I'm worried that this subtlety gets overlooked at some point in the future and we get the next bug here.

[cocreature]

That is a good point, we definitely violate the spec here, quoting:

* The *local* contract IDs are the IDs of the contracts created by the
  transaction.

* The *global* contract IDs are the contract IDs that:
   
   * appear in the commands that produced the transaction. This
     includes the IDs of the exercised contract, together with all the
     IDs referenced in the arguments of the create and exercise
     commands;
   * that are fetched or looked up by key unless they are local;
   * are referenced in the input contracts.

so cid0 should definitely be global due to the exercise and we should get a conflict.

Not really sure how to fix that. It seems like the only way out is to preserve byKey info on exercise which we should do either way but that doesn’t help for old transaction versions.

[andreaslochbihler-da]

After another offline discussion with @remyhaemmerle-da: There are probably two ways to approach this;

1. Properly distinguish between normal exercise and exerciseByKey nodes using the byKey flag. For backwards compatibility, we could treat all exercise nodes without a byKey field as normal exercises. This preserves the current behaviour, but still needs special treatment for CreateAndExercise commands.
2. In general allow exercises in later commands to use contract IDs created in earlier commands of the same transaction (both as input contract and in their choice arguments). This is the direction of this PR.

In scenario 1, the created contract clashes with an existing contract. So the Daml ledger should reject the submission with Contract cid0 already exists. The interesting part here is that the existing cid0 might have been archived long ago (and just sits in the submitting participant's store of divulged contracts, so pruning may have removed the existence of cid0 from the ledger's memory, but then the skew bounds on the ledger time (a contract ID commits to the ledger time of the creating transaction) should be tight enough that cid0 cannot be created in this transaction.

So, we could update the spec to say that if a later exercise command refers to a contract created by an earlier command, then that's not considered a global contract ID for the transaction. What should we do about an exercise command referring to a contract ID that's created in its consequences or by a later command? I'd argue that this is an internally inconsistent transaction and should be rejected, and AFAICS this PR implements this.

[cocreature]

I think there are two mismatches which bite us here both of which we should fix but they are slightly orthogonal:

1. The mismatch between transaction nodes and LF primitives. This affects by-key flags but not createAndExercise (there is no primitive for that in LF)
2. The mismatch between transaction nodes and ledger API commands. This affects by-key flags but only on root nodes and createAndExercise.

We should absolutely fix 1 and add by-key flags but as you pointed out that does not make the issue go away for createAndExercise. Just doing that leads to what I’d argue are slightly unintuitive semantics: If you modify your scenario 1 such that the exercise in the second command is not the root node, this is already perfectly fine. Your approach 2 (implemented in this PR but probably should also get a spec update) removes that annoying special treatment of root nodes from the cid freshness check afaict. That seems like a more robust solution to address mismatch 2 (in this instance). I’m not really qualified to judge if relaxing the CID check here causes any issues but it sounds like you’re saying it’s fine. Assuming that is the case, my preference would be the following two steps:

1. Implement approach 2, i.e., this PR but also update the spec to match this (can be in a separate PR). This is all we need to fix the CID freshness check.
2. Add byKey flags to transaction nodes after that (soonish). This address mismatch 1 which causes other issues.

wdyt?

[andreaslochbihler-da]

I don't see a problem with relaxing the CID check here, as this should be caught on a higher level (freshness check for created contracts). I recommend to document the need for this freshness check; otherwise we'll forget and eventually run into a problem with some of the Daml ledgers.

The byKey flag is orthogonal in this approach. I would like to see the flag added officially at some point; it's not just at root nodes though, because child nodes can be come root nodes via projection.

[andreaslochbihler-da]

I've left a concern in one comment. I think this works for the moment, but there's the danger that two commands with different semantics might yield the same preprocessing result. It's more a case of being cautious.

[cocreature]

Perhaps worth pointing out in the comment that this is only parts of the definition of global cids. In particular, a contract id is also global if it is referenced in the arguments of a global fetched or lookup’d contract. We detect those cases during interpretation so no need to add it here, just asking to clarify the comment.

[remyhaemmerle-da]

👍

[andreaslochbihler-da]

Suggested change

	* - if `cid` is an input of a `n`, i.e. :
	* - if `cid` is referenced in `n`, i.e. :

Input contract is typically used for the contract ID that an exercise acts on, not the ids that appear in the choice argument.

[andreaslochbihler-da]

Do you want to make clear that the traversal order visits all nodes of a transaction (rather than just the root nodes, which is what the rest of this comment is about)?

[andreaslochbihler-da]

Left a few stylistic comments.