Proper types for UNIX timestamps #1990

pm47 · 2021-10-05T15:41:26Z

We define TimestampSecond and TimestampMilli for second and millisecond precision UNIX-style timestamps.

Let me know what you think of the syntaxic sugar, I went for 123456 unix and 123456789 unixms.

Json serialization is as follows for resp. second and millisecond precision. Note that in both case we display the unix format in second precision, but the iso format is more precise:

{
  "iso": "2021-10-04T14:32:41Z",
  "unix": 1633357961
}
{
  "iso": "2021-10-04T14:32:41.456Z",
  "unix": 1633357961
}

codecov-commenter · 2021-10-05T16:22:22Z

Codecov Report

Merging #1990 (e44d0d3) into master (c803da6) will increase coverage by 0.15%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #1990      +/-   ##
==========================================
+ Coverage   87.59%   87.74%   +0.15%     
==========================================
  Files         159      160       +1     
  Lines       12444    12477      +33     
  Branches      525      549      +24     
==========================================
+ Hits        10900    10948      +48     
+ Misses       1544     1529      -15

Impacted Files	Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	`42.46% <0.00%> (-6.57%)`	⬇️
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala	`88.13% <ø> (-1.87%)`	⬇️
...ir/blockchain/bitcoind/rpc/BitcoinCoreClient.scala	`94.23% <ø> (ø)`
.../main/scala/fr/acinq/eclair/db/DualDatabases.scala	`0.00% <ø> (ø)`
...src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala	`92.85% <ø> (ø)`
...in/scala/fr/acinq/eclair/io/ReconnectionTask.scala	`97.82% <ø> (ø)`
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala	`87.09% <ø> (ø)`
.../eclair/payment/relay/PostRestartHtlcCleaner.scala	`89.63% <ø> (-1.11%)`	⬇️
...scala/fr/acinq/eclair/payment/send/Autoprobe.scala	`0.00% <0.00%> (ø)`
...ire/internal/channel/version0/ChannelCodecs0.scala	`96.77% <ø> (ø)`
... and 48 more

pm47

A few highlights to facilitate review.

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala

pm47 · 2021-10-11T12:21:12Z

eclair-core/src/main/scala/fr/acinq/eclair/Timestamp.scala

+}
+
+object TimestampSecond {
+  def now: TimestampSecond = TimestampSecond(System.currentTimeMillis() / 1000)


This is mostly the only file now where we call System.currentTimeMillis(), apart from very few cases where it made sense to be as close to the JVM as possible (e.g. for the RNG) .

I think it should be:

Suggested change

def now: TimestampSecond = TimestampSecond(System.currentTimeMillis() / 1000)

def now(): TimestampSecond = TimestampSecond(System.currentTimeMillis() / 1000)

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

pm47 · 2021-10-11T12:24:14Z

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgPaymentsDb.scala

    preimage_opt match {
      // If we have a pre-image, the payment succeeded.
      case Some(preimage) => OutgoingPaymentStatus.Succeeded(
        preimage, fees_opt.getOrElse(MilliSatoshi(0)), paymentRoute_opt.map(b => paymentRouteCodec.decode(b) match {
          case Attempt.Successful(route) => route.value
          case Attempt.Failure(_) => Nil
        }).getOrElse(Nil),
-        completedAt_opt.getOrElse(0)
+        completedAt_opt.getOrElse(0 unixms)


Example of unixms usage:

Suggested change

completedAt_opt.getOrElse(0 unixms)

completedAt_opt.getOrElse(0 unixms)

pm47 · 2021-10-11T12:32:26Z

eclair-core/src/main/scala/fr/acinq/eclair/router/StaleChannels.scala

    // BOLT 7: "nodes MAY prune channels should the timestamp of the latest channel_update be older than 2 weeks"
    // but we don't want to prune brand new channels for which we didn't yet receive a channel update
-    val staleThresholdSeconds = (System.currentTimeMillis.milliseconds - 14.days).toSeconds
+    val staleThresholdSeconds = TimestampSecond.now - 14.days


pm47 · 2021-10-11T12:33:58Z

eclair-core/src/main/scala/fr/acinq/eclair/router/StaleChannels.scala

+    val staleThreshold = TimestampSecond.now - 14.days
+    timestamp < staleThreshold


pm47 · 2021-10-11T12:35:55Z

eclair-core/src/main/scala/fr/acinq/eclair/router/Sync.scala

+    val timestamp1 = c.update_1_opt.map(_.timestamp).getOrElse(0L unix)
+    val timestamp2 = c.update_2_opt.map(_.timestamp).getOrElse(0L unix)


Example of unix usage.

pm47 · 2021-10-11T12:36:17Z

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/version0/ChannelCodecs0.scala

@@ -352,7 +353,7 @@ private[channel] object ChannelCodecs0 {
    val DATA_WAIT_FOR_FUNDING_CONFIRMED_COMPAT_01_Codec: Codec[DATA_WAIT_FOR_FUNDING_CONFIRMED] = (
      ("commitments" | commitmentsCodec) ::
        ("fundingTx" | provide[Option[Transaction]](None)) ::
-        ("waitingSince" | provide(System.currentTimeMillis.milliseconds.toSeconds)) ::
+        ("waitingSince" | provide(TimestampSecond.now.toLong)) ::


Please review carrefully.

pm47 · 2021-10-11T12:36:38Z

eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/CommonCodecs.scala

@@ -69,6 +69,8 @@ object CommonCodecs {
  // this codec will fail if the amount does not fit on 32 bits
  val millisatoshi32: Codec[MilliSatoshi] = uint32.xmapc(l => MilliSatoshi(l))(_.toLong)

+  val timestampSecond: Codec[TimestampSecond] = uint32.xmapc(TimestampSecond(_))(_.toLong)


Codecs are now typed.

pm47

(continued)

pm47 · 2021-10-11T12:38:21Z

eclair-node/src/main/scala/fr/acinq/eclair/api/directives/ExtraDirectives.scala

+  val fromFormParam: NameDefaultUnmarshallerReceptacle[TimestampSecond] = "from".as[TimestampSecond](timestampSecondUnmarshaller).?(0 unix)
+  val toFormParam: NameDefaultUnmarshallerReceptacle[TimestampSecond] = "to".as[TimestampSecond](timestampSecondUnmarshaller).?(Long.MaxValue unix)


Note how we provide a default value here now.

pm47 · 2021-10-11T12:38:55Z

eclair-node/src/main/scala/fr/acinq/eclair/api/handlers/Channel.scala

@@ -101,8 +101,8 @@ trait Channel {
  }

  val channelStats: Route = postRequest("channelstats") { implicit t =>
-    formFields(fromFormParam.?, toFormParam.?) { (from_opt, to_opt) =>
-      complete(eclairApi.channelStats(from_opt, to_opt))
+    formFields(fromFormParam, toFormParam) { (from, to) =>


Note how the param extractors return a non-optional value.

t-bast

That must have been quite painful, thanks!
Worth integrating quickly to avoid lengthy merge/rebase IMO.

t-bast · 2021-10-18T08:25:19Z

eclair-core/src/main/scala/fr/acinq/eclair/Timestamp.scala

+}
+
+object TimestampSecond {
+  def now: TimestampSecond = TimestampSecond(System.currentTimeMillis() / 1000)


I think it should be:

Suggested change

def now: TimestampSecond = TimestampSecond(System.currentTimeMillis() / 1000)

def now(): TimestampSecond = TimestampSecond(System.currentTimeMillis() / 1000)

eclair-core/src/main/scala/fr/acinq/eclair/Timestamp.scala

eclair-core/src/main/scala/fr/acinq/eclair/package.scala

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala

eclair-core/src/main/scala/fr/acinq/eclair/json/JsonSerializers.scala

eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentRequest.scala

eclair-core/src/main/scala/fr/acinq/eclair/router/Monitoring.scala

This one of the last places where we still don't enforce strong typing. Only the minimum has been done here, as a proof of concept.

t-bast

🚀

pm47 commented Oct 11, 2021

View reviewed changes

t-bast reviewed Oct 18, 2021

View reviewed changes

pm47 added 14 commits October 18, 2021 15:37

use proper type for unix timestamps

e88d0ca

This one of the last places where we still don't enforce strong typing. Only the minimum has been done here, as a proof of concept.

typed sec-precision timestamps for protocol msgs

3968f71

use timestamp types for audit/payments

1f99e5a

remove all calls to System.currentTimeMillis

0991238

fixup! use proper type for unix timestamps

61e0822

fixup! use proper type for unix timestamps

253099b

use sugar conversions unix/unixms

86c8ae7

nit: remove TimestampQueryFilters

bdd7af5

nits

6a45ff8

fixup! use sugar conversions unix/unixms

8866cb5

add parentheses to 'now' methods

796598c

unix->unixsec

bbee520

nit: remove old commented code

b9c7bba

fixup! nits

2c2262b

t-bast previously approved these changes Oct 18, 2021

View reviewed changes

update release notes

a7fff98

pm47 dismissed t-bast’s stale review via a7fff98 October 18, 2021 13:57

pm47 force-pushed the timestamp-types branch from e44d0d3 to a7fff98 Compare October 18, 2021 13:57

pm47 requested a review from t-bast October 18, 2021 13:59

t-bast approved these changes Oct 18, 2021

View reviewed changes

pm47 merged commit b4d285f into master Oct 18, 2021

pm47 deleted the timestamp-types branch October 18, 2021 14:13

pm47 mentioned this pull request Oct 22, 2021

audit and listinvoices API calls are broken #2031

Closed

t-bast mentioned this pull request Dec 10, 2021

Compatibility issue with latest master and RTL #2098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper types for UNIX timestamps #1990

Proper types for UNIX timestamps #1990

pm47 commented Oct 5, 2021 •

edited

Loading

codecov-commenter commented Oct 5, 2021 •

edited

Loading

pm47 left a comment

pm47 Oct 11, 2021

t-bast Oct 18, 2021

pm47 Oct 11, 2021

pm47 Oct 11, 2021

pm47 Oct 11, 2021

pm47 Oct 11, 2021

pm47 Oct 11, 2021

pm47 Oct 11, 2021

pm47 left a comment

pm47 Oct 11, 2021

pm47 Oct 11, 2021

t-bast left a comment

t-bast Oct 18, 2021

t-bast left a comment

	def now: TimestampSecond = TimestampSecond(System.currentTimeMillis() / 1000)
	def now(): TimestampSecond = TimestampSecond(System.currentTimeMillis() / 1000)

	completedAt_opt.getOrElse(0 unixms)
	completedAt_opt.getOrElse(0 unixms)

		val staleThreshold = TimestampSecond.now - 14.days
		timestamp < staleThreshold

		val timestamp1 = c.update_1_opt.map(_.timestamp).getOrElse(0L unix)
		val timestamp2 = c.update_2_opt.map(_.timestamp).getOrElse(0L unix)

		val fromFormParam: NameDefaultUnmarshallerReceptacle[TimestampSecond] = "from".as[TimestampSecond](timestampSecondUnmarshaller).?(0 unix)
		val toFormParam: NameDefaultUnmarshallerReceptacle[TimestampSecond] = "to".as[TimestampSecond](timestampSecondUnmarshaller).?(Long.MaxValue unix)

Proper types for UNIX timestamps #1990

Proper types for UNIX timestamps #1990

Conversation

pm47 commented Oct 5, 2021 • edited Loading

codecov-commenter commented Oct 5, 2021 • edited Loading

Codecov Report

pm47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

pm47 commented Oct 5, 2021 •

edited

Loading

codecov-commenter commented Oct 5, 2021 •

edited

Loading