kvutils: Refined transaction validation [KVL-1015]

[hubert-da]

Implements transaction validation as described in the comment: #10066 (comment)

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]

As far as I can tell with this change our checks are now ordered as follows:

1. Check key consistency for all nodes that have a key
2. Check key monotonicity
3. Reinterpret with the keys & cid lookups being checked via contractIsActive which not only checks activeness but also monotonicity. However, here we do not blow up with a monotonicity error. Instead we blow up with Disputed.

I think this is fine if you have an honest submitter and will only produce bad error messages but there are a few points that worry me:

1. You do checks on inputs before checking that the transaction is okay, e.g., well authorized. This seems potentially leaky. E.g., I send you a negative lookup by key that is ill-authorized. You tell me that there is a mismatch and the key actually exists. Similarly you can leak activeness checks. If you trust your participant that may be not an issue but it’s at least sketchy.
2. You report monotonicity errors differently for keys & contract ids. For keys there is a dedicated error. For contract ids they are folded under general "transaction mismatch" so Disputed afaict. Not necessarily an issue I think but at least confusing.
3. You mix up activeness of contract ids with monotonicity afaict and both are handled implicitly during reinterpretation. That seems potentially problematic. Activeness can be a race. Monotonicity is a malicious participant.Both are reported the same way.

As mentioned above, I’m not sure you can exploit any of this without a malicious participant but I think the same was also true for the previous order of the checks so if we fix this it might be worth fixing it properly.

I’m by no means an expert on kvutils validation so take all of this with a grain of salt. I might just be missing something and this is all perfectly sensible but until someone explains it to me I’m at least not comfortable approving it.

[gerolf-da]

Thanks, @cocreature. I had a strong conviction that the model conformance was checked before contract key consistency that I read past it.

For this PR: The order of the steps validate_contract_keys and validate_model_conformance should be changed.

For a later review: I think there should be a single monotonicity check for both contracts and keys after the model validation, and the contract lookup during interpretation shouldn't do a ledger effective time check.

[cocreature]

If you do model conformance before contract keys validation but keep the consistency check during interpretation, then the example that produced causal monotonicity violation will fail with Disputed I believe which seems arguably worse.

[cocreature]

Ah no I think works for the case we encountered but breaks others. This is what we saw:

1. During submission, the key is inactive.
2. During model conformance during validation the key is active. The arguably incorrectly placed ledger effective time check however saves you and means interpretation will still treat the key as inactive.
3. So because of that you get past model conformance and than we fail with InconsistentKeys.

However now consider a case where in 2 the key is active and passes the monotonicity check. This will now blow up with Disputed even though it was just a race.

What seems questionable here to me (and I could just be missing something) is that you check model conformance with the current ledger state instead of the one during submission. If you look at the ledger model, model conformance does not depend on the ledger state. It is a property of a transaction.

So I think the checks maybe should look like the following:

1. Check conformance against the ledger state at submission. Reinterpret using that state, diff. Monotonicity checks can be done here but again they should be done against the legder state at submission not the one during validation.
2. Assuming the transaction is conformant, check consistency of key & contract ids.

[gerolf-da]

Summary of a conversation with @cocreature, @hubert-da, @gerolf-da:

We need to do two types of checks:

1. check the conformance of the transaction with the Daml model purely in terms of the transaction inputs itself.
   A violation in this category generally falls into the Disputed class of errors.
2. check that the transaction is consistent with the current ledger state
   A violation in this category generally falls into the Inconsistent class of errors.

Transaction model conformance

1. Validate model conformance of the transaction

   • use the contract keys of the transaction (Transaction#contractKeyInput) as input to TransactionCommitter#lookupKey instead of the contract keys from the commit context
   • because fetch nodes don't actually contain the contract, we still need to get the contracts from the commit context, but we must not check for activeness or causal monotonicity in TransactionCommitter#lookupContract during re-interpretation.
   • Any error in validation should result in Disputed.

2. check causal monotonicity based on contracts used in the transaction (GenTransaction#inputContracts, contains contracts used in exercise, fetch, lookupByKey)

   • violations of causal monotonicity should result in InvalidLedgerTime.

Check that transaction is still valid given the ledger state

1. check consistency of keys
   • check that keys in the transaction are the same as keys in state (already done in ContractKeysValidation#performTraversalContractKeysChecks)
2. check consistency of contracts
   • all contracts used in the transaction (GenTransaction#inputContracts) are still active. A violation results in the error Inconsistent.

Cleanup

• Remove KeyMonotonicityValidation, it is superseded by the causal monotonicity check during model conformance validation.

Ledger API Test Tool

The above changes will result in different errors than what the ledger api test tool currently checks.
We should also document in the release notes in which cases the error is now different.

---

@andreaslochbihler-da, @cocreature, @miklos-da: Could you please review this summary and point out any issues or give a 👍?

[cocreature]

I think they should be InvalidLedgerTime at least until we fixed the issue around divulged contracts I mentioned above. Otherwise you can get disputed from an honest submitter which is bad imho. Once that is fixed I agree with you that they can be treated the same.

[gerolf-da]

Thanks for the reminder, @cocreature.
There is a scenario where a participant could submit a transaction that violates causal monotonicity:

1. The participant's system clock lags behind in time of the committer's system clock.
2. The transaction only references divulged contracts, for which the participant doesn't (yet) keep a "substitute" ledger effective time (e.g. the LET from the divulging transaction), that were created and divulged within the window of the participants clock drift.
3. Therefore the participant will use its (lagging) system clock to determine LET and the contract's actual LET will be in the "future" compared to the submitted ledger effective time of the transaction.

I don't think we should fix this issue as part of KVL-990. And it already happens currently as well. So technically the situation doesn't get worse.

[cocreature]

Fine to leave it out as long as we don’t report is as Disputed which would be worse or at least different from the current state.

[andreaslochbihler-da]

There is a scenario where a participant could submit a transaction that violates causal monotonicity:

Oh right. I forgot that the participant server doesn't keep track of ledger create time for divulged contracts (something we do in Canton).

[gerolf-da]

Fine to leave it out as long as we don’t report is as Disputed which would be worse or at least different from the current state.

Currently, prior to the change described above, this case would also end up with a Disputed error.

[hubert-da]

@gerolf-da @cocreature @fabiotudone-da JFYI I addressed the review comments

[cocreature]

Nice work, thank you!