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

DPP-587 Use Timestamp instead of Instant #11183

Merged
merged 4 commits into from
Oct 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ class GrpcLedgerClient(val grpcClient: LedgerClient, val applicationId: Applicat
resp <- ClientAdapter
.serverStreaming(GetTimeRequest(grpcClient.ledgerId.unwrap), timeService.getTime)
.runWith(Sink.head)
} yield Time.Timestamp.assertFromInstant(TimestampConversion.toInstant(resp.getCurrentTime))
} yield TimestampConversion.toLf(resp.getCurrentTime, TimestampConversion.ConversionMode.HalfUp)
}

override def setStaticTime(time: Time.Timestamp)(implicit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@ package com.daml.api.util
import java.time.{Clock, Instant}

rautenrieth-da marked this conversation as resolved.
Show resolved Hide resolved
import com.daml.api.util.TimeProvider.MappedTimeProvider
import com.daml.lf.data.Time.Timestamp

trait TimeProvider { self =>

def getCurrentTime: Instant

def getCurrentTimestamp: Timestamp = Timestamp.assertFromInstant(getCurrentTime)

def map(transform: Instant => Instant): TimeProvider = MappedTimeProvider(this, transform)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import java.time.Instant
import java.util.concurrent.TimeUnit

import com.daml.ledger.api.v1.value.Value
import com.google.protobuf.timestamp.Timestamp
import com.google.protobuf.timestamp.{Timestamp => ProtoTimestamp}
import com.daml.lf.data.Time.{Timestamp => LfTimestamp}

object TimestampConversion {
val MIN = Instant parse "0001-01-01T00:00:00Z"
Expand All @@ -32,11 +33,46 @@ object TimestampConversion {

}

def toInstant(protoTimestamp: Timestamp): Instant = {
def toInstant(protoTimestamp: ProtoTimestamp): Instant = {
Instant.ofEpochSecond(protoTimestamp.seconds, protoTimestamp.nanos.toLong)
}

def fromInstant(instant: Instant): Timestamp = {
new Timestamp().withSeconds(instant.getEpochSecond).withNanos(instant.getNano)
def fromInstant(instant: Instant): ProtoTimestamp = {
new ProtoTimestamp().withSeconds(instant.getEpochSecond).withNanos(instant.getNano)
}

def toLf(protoTimestamp: ProtoTimestamp, mode: ConversionMode): LfTimestamp = {
val instant = roundToMicros(toInstant(protoTimestamp), mode)
LfTimestamp.assertFromInstant(instant)
}

def fromLf(timestamp: LfTimestamp): ProtoTimestamp = {
fromInstant(timestamp.toInstant)
}

private def roundToMicros(t: Instant, mode: ConversionMode): Instant = {
val fractionNanos = t.getNano % 1000L
if (fractionNanos != 0) {
mode match {
case ConversionMode.Exact =>
throw new IllegalArgumentException(
s"Conversion of $t to microsecond granularity would result in loss of precision."
)
case ConversionMode.HalfUp =>
t.plusNanos(if (fractionNanos >= 500L) 1000L - fractionNanos else -fractionNanos)
}
} else {
t
}
}

sealed trait ConversionMode
object ConversionMode {

/** Throw an exception if the input can not be represented in microsecond resolution */
case object Exact extends ConversionMode

/** Round to the nearest microsecond */
case object HalfUp extends ConversionMode
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import TimestampConversion._
import org.scalacheck.Gen
import org.scalacheck.Prop
import Prop.exists
import com.daml.lf.data.Time
import org.scalatestplus.scalacheck.{Checkers, ScalaCheckDrivenPropertyChecks}
import org.scalatest.matchers.should.Matchers
import org.scalatest.wordspec.AnyWordSpec
Expand Down Expand Up @@ -81,6 +82,32 @@ class TimestampConversionSpec
}
}
}

"fromLf" when {
"given a value in specified domain" should {
"be retracted by toLf" in forAll(lfTimestampGen) { ts =>
toLf(fromLf(ts), ConversionMode.Exact) shouldBe ts
}
}
}

"toLf" when {
"given a valid microsecond timestamp" should {
"be retracted by fromLf" in forAll(anyMicroInRange) { ts =>
val protoTs = fromInstant(ts)
fromLf(toLf(protoTs, ConversionMode.Exact)) shouldBe protoTs
}
}

"given a valid nanosecond timestamp" should {
"round half up" in forAll(anyTimeInRange) { ts =>
val protoTs = fromInstant(ts)
val halfUp = toLf(protoTs, ConversionMode.HalfUp)
halfUp.toInstant should be > ts.plusNanos(-500)
halfUp.toInstant should be <= ts.plusNanos(500)
}
}
}
}

object TimestampConversionSpec {
Expand Down Expand Up @@ -110,4 +137,9 @@ object TimestampConversionSpec {

val anyMicroInRange: Gen[Instant] =
timeGen(MIN, MAX, microsOnly = true)

val lfTimestampGen: Gen[Time.Timestamp] = Gen.choose(
Time.Timestamp.MinValue.micros,
Time.Timestamp.MaxValue.micros,
) map Time.Timestamp.assertFromLong
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ final class CommandsValidator(ledgerId: LedgerId) {
submissionId = submissionId,
actAs = submitters.actAs,
readAs = submitters.readAs,
submittedAt = currentUtcTime,
submittedAt = Time.Timestamp.assertFromInstant(currentUtcTime),
deduplicationPeriod = deduplicationPeriod,
commands = Commands(
commands = validatedCommands.to(ImmArray),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class SubmitRequestValidatorTest
submissionId = submissionId,
actAs = Set(DomainMocks.party),
readAs = Set.empty,
submittedAt = submittedAt,
submittedAt = Time.Timestamp.assertFromInstant(submittedAt),
deduplicationPeriod = DeduplicationPeriod.DeduplicationDuration(deduplicationDuration),
commands = LfCommands(
ImmArray(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@

package com.daml.ledger.api

import java.time.{Duration, Instant}

import java.time.Duration
import com.daml.ledger.offset.Offset
import com.daml.lf.data.Time.Timestamp
import com.daml.logging.entries.{LoggingValue, ToLoggingValue}

/** Specifies the deduplication period for a command submission.
Expand All @@ -18,17 +18,17 @@ sealed trait DeduplicationPeriod extends Product with Serializable

object DeduplicationPeriod {

/** Transforms the [[period]] into an [[Instant]] to be used for deduplication into the future(deduplicateUntil).
/** Transforms the [[period]] into a [[Timestamp]] to be used for deduplication into the future(deduplicateUntil).
* Only used for backwards compatibility
* @param time The time to use for calculating the [[Instant]]. It can either be submission time or current time, based on usage
* @param time The time to use for calculating the [[Timestamp]]. It can either be submission time or current time, based on usage
* @param period The deduplication period
*/
def deduplicateUntil(
time: Instant,
time: Timestamp,
period: DeduplicationPeriod,
): Instant = period match {
): Timestamp = period match {
case DeduplicationDuration(duration) =>
time.plus(duration)
time.addMicros(duration.toNanos / 1000)
case DeduplicationOffset(_) =>
throw new NotImplementedError("Offset deduplication is not supported")
}
Expand All @@ -37,18 +37,18 @@ object DeduplicationPeriod {
* We measure `deduplicationStart` on the ledger’s clock, and thus
* we need to add the minSkew to compensate for the maximal skew that the participant might be behind the ledger’s clock.
* @param time submission time or current time
* @param deduplicationStart the [[Instant]] from where we should start deduplication, must be < than time
* @param deduplicationStart the [[Timestamp]] from where we should start deduplication, must be < than time
* @param minSkew the minimum skew as specified by the current ledger time model
*/
def deduplicationDurationFromTime(
time: Instant,
deduplicationStart: Instant,
time: Timestamp,
deduplicationStart: Timestamp,
minSkew: Duration,
): Duration = {
assert(deduplicationStart.isBefore(time), "Deduplication must start in the past")
assert(deduplicationStart < time, "Deduplication must start in the past")
Duration.between(
deduplicationStart,
time.plus(minSkew),
deduplicationStart.toInstant,
time.toInstant.plus(minSkew),
)
}

Expand All @@ -62,6 +62,10 @@ object DeduplicationPeriod {
*/
final case class DeduplicationDuration(duration: Duration) extends DeduplicationPeriod {
require(!duration.isNegative, s"The deduplication window must not be negative: $duration")
require(
duration.toNanos % 1000 == 0,
Copy link
Contributor

@oliverse-da oliverse-da Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rautenrieth-da , one of the canton unit tests shows that it is possible to trigger a Long overflow inside java.time.Duration.toNanos. Perhaps we could use Duration.toNanosPart instead?

When passing in a sufficiently large value you get:

[info]   java.lang.ArithmeticException: long overflow
[info]   at java.base/java.lang.Math.multiplyExact(Math.java:949)
[info]   at java.base/java.time.Duration.toNanos(Duration.java:1248)
[info]   at com.daml.ledger.api.DeduplicationPeriod$DeduplicationDuration.<init>(DeduplicationPeriod.scala:66)

triggered, e.g. on huge values such as 10K years:

    "complain about time underflow" in {
      val fix = mk()
      val timeUnderflowDuration =
        DeduplicationPeriod.DeduplicationDuration(java.time.Duration.ofDays(356 * 10000))
      for {
        error <- fix.dedup
          .checkDuplication(changeId1Hash, timeUnderflowDuration)
          .leftOrFail("time underflow")

      } yield {
        error shouldBe DeduplicationPeriodTooEarly(
          timeUnderflowDuration,
          DeduplicationPeriod.DeduplicationDuration(
            java.time.Duration.between(CantonTimestamp.MinValue.toInstant, clock.now.toInstant)
          ),
        )
      }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the report! That's an impressive test coverage if you test deduplication periods of 10k years 🙂
I have opened #11432 to fix this.

s"The deduplication window must not use nanosecond precision: $duration",
)
}

/** The `offset` defines the start of the deduplication period. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,12 @@

package com.daml.ledger.api

import java.time.Instant

import com.daml.ledger.api.domain.Event.{CreateOrArchiveEvent, CreateOrExerciseEvent}
import com.daml.ledger.configuration.Configuration
import com.daml.lf.command.{Commands => LfCommands}
import com.daml.lf.data.Ref
import com.daml.lf.data.Ref.LedgerString.ordering
import com.daml.lf.data.Time.Timestamp
import com.daml.lf.data.logging._
import com.daml.lf.value.{Value => Lf}
import com.daml.logging.entries.{LoggingValue, ToLoggingValue}
Expand Down Expand Up @@ -131,7 +130,7 @@ object domain {

def workflowId: Option[WorkflowId]

def effectiveAt: Instant
def effectiveAt: Timestamp

def offset: LedgerOffset.Absolute
}
Expand All @@ -140,7 +139,7 @@ object domain {
transactionId: TransactionId,
commandId: Option[CommandId],
workflowId: Option[WorkflowId],
effectiveAt: Instant,
effectiveAt: Timestamp,
offset: LedgerOffset.Absolute,
eventsById: immutable.Map[EventId, CreateOrExerciseEvent],
rootEventIds: immutable.Seq[EventId],
Expand All @@ -150,31 +149,31 @@ object domain {
transactionId: TransactionId,
commandId: Option[CommandId],
workflowId: Option[WorkflowId],
effectiveAt: Instant,
effectiveAt: Timestamp,
events: immutable.Seq[CreateOrArchiveEvent],
offset: LedgerOffset.Absolute,
) extends TransactionBase

sealed trait CompletionEvent extends Product with Serializable {
def offset: LedgerOffset.Absolute
def recordTime: Instant
def recordTime: Timestamp
}

object CompletionEvent {

final case class Checkpoint(offset: LedgerOffset.Absolute, recordTime: Instant)
final case class Checkpoint(offset: LedgerOffset.Absolute, recordTime: Timestamp)
extends CompletionEvent

final case class CommandAccepted(
offset: LedgerOffset.Absolute,
recordTime: Instant,
recordTime: Timestamp,
commandId: CommandId,
transactionId: TransactionId,
) extends CompletionEvent

final case class CommandRejected(
offset: LedgerOffset.Absolute,
recordTime: Instant,
recordTime: Timestamp,
commandId: CommandId,
reason: RejectionReason,
) extends CompletionEvent
Expand Down Expand Up @@ -282,7 +281,7 @@ object domain {
submissionId: SubmissionId,
actAs: Set[Ref.Party],
readAs: Set[Ref.Party],
submittedAt: Instant,
submittedAt: Timestamp,
deduplicationPeriod: DeduplicationPeriod,
commands: LfCommands,
)
Expand All @@ -291,6 +290,9 @@ object domain {

import Logging._

implicit val `Timestamp to LoggingValue`: ToLoggingValue[Timestamp] =
ToLoggingValue.ToStringToLoggingValue

implicit val `Commands to LoggingValue`: ToLoggingValue[Commands] = commands =>
LoggingValue.Nested.fromEntries(
"ledgerId" -> commands.ledgerId,
Expand Down Expand Up @@ -347,12 +349,12 @@ object domain {
object PackageEntry {
final case class PackageUploadAccepted(
submissionId: String,
recordTime: Instant,
recordTime: Timestamp,
) extends PackageEntry

final case class PackageUploadRejected(
submissionId: String,
recordTime: Instant,
recordTime: Timestamp,
reason: String,
) extends PackageEntry
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,22 @@

package com.daml.ledger.api

import java.time.{Duration, Instant}
import com.daml.lf.data.Time.Timestamp

import java.time.Duration
import org.scalatest.matchers.should.Matchers
import org.scalatest.wordspec.AnyWordSpec

class DeduplicationPeriodSpec extends AnyWordSpec with Matchers {
"calculating deduplication until" should {
val time = Instant.ofEpochSecond(100)
val time = Timestamp.assertFromLong(100 * 1000 * 1000)

"return expected result when sending duration" in {
val deduplicateUntil = DeduplicationPeriod.deduplicateUntil(
time,
DeduplicationPeriod.DeduplicationDuration(Duration.ofSeconds(3)),
)
deduplicateUntil shouldEqual time.plusSeconds(3)
deduplicateUntil shouldEqual time.add(Duration.ofSeconds(3))
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package com.daml.ledger.configuration
import java.time.{Duration, Instant}

import com.daml.ledger.configuration.LedgerTimeModel._
import com.daml.lf.data.Time.Timestamp

import scala.util.Try

Expand Down Expand Up @@ -43,6 +44,13 @@ case class LedgerTimeModel private (
}
}

def checkTime(
ledgerTime: Timestamp,
recordTime: Timestamp,
): Either[OutOfRange, Unit] = {
checkTime(ledgerTime.toInstant, recordTime.toInstant)
}

private[ledger] def minLedgerTime(recordTime: Instant): Instant =
recordTime.minus(minSkew)

Expand Down
Loading