Make submission ID optional [KVL-1107]

[hubert-da]

Also:

• Stop generating the submission ID in the GrpcCommandSubmissionService.
• Propagate submission IDs in the in-memory sandbox classic.
• Simplify CommandTracker

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.

[hubert-da]

Extracted from CommandClient.

[andreaslochbihler-da]

The CommandTracker tries to make sure that there is ever only a single submission in flight for a given change ID. I don't think that this can work, for the following reasons:

1. With replicated participants like in a VMBC setup, the same change ID may be sent to multiple participants and those participants each see just a single submission, but they get the completion events for both submissions. That should be taken care of by the CommandService of each participant picking a unique submission ID and checking in the completion that the submission ID is the expected one.
2. Commands may be sent concurrently via the CommandService and the CommandSubmissionService, and submission IDs are optional for Ledger API submissions. So if the CommandSubmissionService no longer assigns a submission ID, we may actually get back a completion without a submission ID from the submission to the CommandSubmissionService that is also processed by the CommandService. With the current logic of treating an empty submission ID in a completion as "matches all in-flight submissions", this re-introduces the very confusion that the submission IDs were supposed to eliminate. For example, if the submission via the CommandSubmissionService completes first with a rejection, then the CommandService will report a rejection even though the still-in-flight submission may end up being accepted.

To avoid the confusion, I'd suggest to introduce two modes to the CommandTracker. For legacy ledgers that do not propagate the submission ID, we keep the current behaviour of "empty submission ID matches all in-flight submissions", and for the upgraded ledgers it ignores completions without a submission ID (as it has ensured that the submission ID is set on all commands that it is tracking).

[hubert-da]

@andreaslochbihler-da I'm a bit confused about the comment, as this PR reflects what we discussed is the right thing to do.

So if the CommandSubmissionService no longer assigns a submission ID, we may actually get back a completion without a submission ID from the submission to the CommandSubmissionService that is also processed by the CommandService. With the current logic of treating an empty submission ID in a completion as "matches all in-flight submissions"

This command won't be present in the tracked commands, so it will try to "match all in-flight submissions" with the same command id. Do I understand correctly that due to point 1. this also breaks? It doesn't seem ok that the same application submits the same commands with both services.

[andreaslochbihler-da]

Do I understand correctly that due to point 1. this also breaks?

In my scenario, all submissions have the same change ID, the same command Id, same application ID and same set of actAs. So yes, this breaks, but independently of point 1.

[miklos-da]

The CommandTracker tries to make sure that there is ever only a single submission in flight for a given change ID. I don't think that this can work, for the following reasons:

1. With replicated participants like in a VMBC setup, the same change ID may be sent to multiple participants and those participants each see just a single submission, but they get the completion events for both submissions. That should be taken care of by the CommandService of each participant picking a unique submission ID and checking in the completion that the submission ID is the expected one.
2. Commands may be sent concurrently via the CommandService and the CommandSubmissionService, and submission IDs are optional for Ledger API submissions. So if the CommandSubmissionService no longer assigns a submission ID, we may actually get back a completion without a submission ID from the submission to the CommandSubmissionService that is also processed by the CommandService. With the current logic of treating an empty submission ID in a completion as "matches all in-flight submissions", this re-introduces the very confusion that the submission IDs were supposed to eliminate. For example, if the submission via the CommandSubmissionService completes first with a rejection, then the CommandService will report a rejection even though the still-in-flight submission may end up being accepted.

To avoid the confusion, I'd suggest to introduce two modes to the CommandTracker. For legacy ledgers that do not propagate the submission ID, we keep the current behaviour of "empty submission ID matches all in-flight submissions", and for the upgraded ledgers it ignores completions without a submission ID (as it has ensured that the submission ID is set on all commands that it is tracking).

@andreaslochbihler-da What stops us from implementing only the latter, i.e., assuming that we always populate the submission ID?

[andreaslochbihler-da]

What stops us from implementing only the latter, i.e., assuming that we always populate the submission ID?

It's just separations of concerns, i.e., clear responsibilities in our architecture. The command submission service and everything below don't need the submission ID, so why should they bother populating it?

[miklos-da]

What stops us from implementing only the latter, i.e., assuming that we always populate the submission ID?

It's just separations of concerns, i.e., clear responsibilities in our architecture. The command submission service and everything below don't need the submission ID, so why should they bother populating it?

Agreed. Assuming we'll shortly implement generating the submission ID (if missing) for the JSON-API, removing generation of a submission ID from the command submission service and ignoring completions without a submission ID should be fine, right?

[andreaslochbihler-da]

I'm not sure I understand what the JSON API has to do with this. My point is: The CommandTracker implements functionality of tracking commands. It's used in the CommandService, in the CommandClient, in Canton, and possibly in the JSON API (I don't know about the latter). Submission IDs are meant to track commands. So as long as someone uses the CommandTracker with an old ledger that doesn't pass through the submission ID, we'll have a conflict of interest around the CommandTracker and where it's used:

• Preserve the old behaviour on old ledgers (completion with empty submission ID completes any tracking of the affected command ID)
• Support the submission IDs on new ledgers (ignore completions with empty submission ID)

So it's not about "when will this be in the JSON API", but about "when will the last user of the CommandTracker have moved off a ledger that doesn't propagate submission IDs". I don't know when that will happen, but given the longlivety of sandbox-classic, my hunch is that this will take a while.

[miklos-da]

I'm not sure I understand what the JSON API has to do with this. My point is: The CommandTracker implements functionality of tracking commands. It's used in the CommandService, in the CommandClient, in Canton, and possibly in the JSON API (I don't know about the latter). Submission IDs are meant to track commands. So as long as someone uses the CommandTracker with an old ledger that doesn't pass through the submission ID, we'll have a conflict of interest around the CommandTracker and where it's used:

• Preserve the old behaviour on old ledgers (completion with empty submission ID completes any tracking of the affected command ID)
• Support the submission IDs on new ledgers (ignore completions with empty submission ID)

So it's not about "when will this be in the JSON API", but about "when will the last user of the CommandTracker have moved off a ledger that doesn't propagate submission IDs". I don't know when that will happen, but given the longlivety of sandbox-classic, my hunch is that this will take a while.

Wouldn't it be a good tradeoff then to implement propagation of submission IDs for sandbox-classic so that we don't have to support two modes (completion has/doesn't have submission ID)? @SamirTalwar-DA Have you looked into implementing submission ID propagation for sandbox-classic earlier?

[ghost]

I haven't, but I think it's a good idea to just get that done.

[hubert-da]

Sorry for making this PR rather huge. The scope was unknown beforehand. I also thought it will be easier to discuss if everything is in one place. Please let me know if I should think about splitting it.

@miklos-da @SamirTalwar-DA @andreaslochbihler-da, WDYT about the latest changes? I'm not entirely happy about them, just don't have a better idea.

[andreaslochbihler-da]

WDYT about the latest changes?

I can't really comment on whether your choice of tying the submission ID propagation mode to whether the append-only schema is enabled will work. I have too little insight into what combinations of ledgers and ledger API servers are actually used. But if that's the right heuristics, then I think this looks fine.

[ghost]

Staring at this again, I don't think I'm a fan of making such a visible change. CommandClientConfiguration is supported API and I find it somewhat unreasonable to expect a user of our Java API to know whether their ledger supports submission ID propagation.

I would very much prefer if we could just make all ledgers work the same way.

[hubert-da]

Depends on: #11211

[ghost]

Can you explain why you changed this behavior?

[hubert-da]

According to the error message that has been removed, I could change this, as the mutating schema has been removed, so we no longer have Completions without the submission ID.

[ghost]

Can we use a generator here that pulls values from a list or an incrementing counter, so we can be sure that the code is working correctly?

Something like:

```suggestion
        generateSubmissionId = {
          val counter = new AtomicInteger
          () => Ref.SubmissionId.assertFromString(s"submission ID ${counter.incrementAndGet()}")
        },