Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transaction is recorded AFTER the provided MRT #987

Closed
gerolf-da opened this issue May 7, 2019 · 8 comments

Comments

Projects
None yet
3 participants
@gerolf-da
Copy link
Contributor

commented May 7, 2019

It is possible to create a transaction with a record time AFTER the maximum record time provided in the original command.

image

Reproducible by the attached SDK project with:

da run damlc -- package daml/Main.daml target/daml/iou
da run sandbox -- target/daml/iou.dar -w
mvn compile exec:java@run-quickstart

Currently, the record time for a transaction is set in the backends at Ledger#publishTransaction (InMemory and SQL) .

@gerolf-da

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Sadly, this doesn't currently hold:

* @param maximumRecordTime:
* The maximum record time for this transaction to be committed to the
* ledger. The [[TransactionSubmission]] will never result in an
* [[LedgerSyncEvent.AcceptedTransaction]] event with a record time strictly larger than
* its maximum record time.

@gerolf-da gerolf-da referenced this issue May 7, 2019

Merged

Sandbox: Respect MRT #990

5 of 5 tasks complete
@gerolf-da

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

The maximum record time (MRT) is used by applications to detect timeouts of commands. The MRT of a command is specified in the time domain of the ledger. Therefore only timestamps received from the ledger may be compared to the MRT of a command.

Can transactions have a record time (Ledger API field effective_at)? On first glance it seems this shouldn't be possible.

However, there is one scenario where a transaction's record time may be greater than the maximum record time:
The application can only conclude that a command has timed out, if it receives a checkpoint on the completion stream from the ledger with a record_time greater than the command's MRT without having received the completion for the command.
If the ledger does not send any checkpoints between the client sending the command and the completion of that command, the ledger would be free to accept the transaction with a record time after MRT.
Having said that, it is in practice more complicated to make this scenario work than to reject the transaction.

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@gerolf-da , thanks for explaining. This might hint at the fact that maybe we should advertise the role of the timeout differently: it's "you should try to retry after this much time" rather than "you will not receive a completion after this time". This would suffice for command submission, since commands are deduplicated anyway and "useless" retries are thus harmless.

However that would mean that one cannot rely on the MRT to reliably determine that the command has failed, as the command service does.

cc @meiersi-da @dajmaki @gaboraranyossy-da what do you think?

@meiersi-da

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

My expectation was that the ledger maintains both the list of committed transactions and updates to the record time.

This way it can and does guarantees that a transaction does not get committed after the record time has been bumped to a value strictly larger than the maximum record time in the submitted transaction. This is how I've documented the maximum record time field in the DAML Integration Kit: https://github.com/digital-asset/daml/blob/master/ledger/participant-state/src/main/scala/com/daml/ledger/participant/state/v1/SubmitterInfo.scala#L23

From my perspective the screenshot shows a bug in the SQL and InMemory backends of the sandbox.

Also @gerolf-da : effective_at is a different timestamp than the record time of a transaction. Currently the record time of transactions is not exposed on the Ledger API AFAIK. Can you clarify what you mean by your statemement

Can transactions have a record time (Ledger API field effective_at)? On first glance it seems this shouldn't be possible.

As per my understanding, the effective_at field can be larger than the maximum record time, as the max_ttl in the configuration can allow values larger than the maximum record time. Or is there another requirement / invariant that I'm missing?

@meiersi-da meiersi-da reopened this May 9, 2019

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@meiersi-da my comment was proposing an alternative perspective, but I agree we have been advertising the semantics as you described, which is why @gerolf-da fixed the behavior exhibited here in #990.

@bitonic bitonic closed this May 9, 2019

@meiersi-da

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@bitonic : note that it depends on where deduplication happens for deciding how harmless duplicate submissions are. I believe the current advice for users to wait for a confirmation of either a command completion or the record time having surpassed the maximum record time of their command submission until they retry is correct.

@gerolf-da

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@meiersi-da: The invariant should be: max_ttl <= MRT - LET.
Regarding

Can transactions have a record time (Ledger API field effective_at)?

The current implementation uses the record time for the effective_at field of the emitted transactions:

PTransaction(
transactionId = trans.transactionId,
commandId = if (submitterIsSubscriber) trans.commandId.getOrElse("") else "",
workflowId = trans.workflowId,
effectiveAt = Some(fromInstant(trans.recordTime)),
events = events,
offset = trans.offset
))

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Le't continue the discussion on #1068, to keep this issue about fixing the code in the sandbox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.