From add0c9acd7140de340f469329178b70d3d556e73 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Fri, 16 Jul 2021 14:18:57 -0700 Subject: [PATCH 1/3] fix: make timestamps strictly increasing On Linux, this is almost always the case. Windows, however, doesn't have nanosecond accuracy. We make the timestamp sequence numbers strictly increasing by returning the last timestamp + 1 where necessary. --- peer/record.go | 15 ++++++++++++++- peer/record_test.go | 13 +++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/peer/record.go b/peer/record.go index 3968fc22..95c8f6c6 100644 --- a/peer/record.go +++ b/peer/record.go @@ -2,6 +2,7 @@ package peer import ( "fmt" + "sync/atomic" "time" pb "github.com/libp2p/go-libp2p-core/peer/pb" @@ -125,9 +126,21 @@ func PeerRecordFromProtobuf(msg *pb.PeerRecord) (*PeerRecord, error) { return record, nil } +var lastTimestamp uint64 + // TimestampSeq is a helper to generate a timestamp-based sequence number for a PeerRecord. func TimestampSeq() uint64 { - return uint64(time.Now().UnixNano()) + now := uint64(time.Now().UnixNano()) + previous := atomic.LoadUint64(&lastTimestamp) + // If the new time is not greater than the last tiemstamp, or if someone else beats us to + // updateing the timestamp, just use last+1. + // + // Technically, last+1 could be before "now". But it's still strictly increasing and close + // enough. + if now <= previous || !atomic.CompareAndSwapUint64(&lastTimestamp, previous, now) { + now = atomic.AddUint64(&lastTimestamp, 1) + } + return now } // Domain is used when signing and validating PeerRecords contained in Envelopes. diff --git a/peer/record_test.go b/peer/record_test.go index adbe03ae..e5aa0792 100644 --- a/peer/record_test.go +++ b/peer/record_test.go @@ -52,3 +52,16 @@ func TestSignedPeerRecordFromEnvelope(t *testing.T) { } }) } + +// This is pretty much guaranteed to pass on Linux no matter how we implement it, but Windows has +// low clock precision. This makes sure we never get a duplicate. +func TestTimestampSeq(t *testing.T) { + last := uint64(0) + for i := 0; i < 1000; i++ { + next := TimestampSeq() + if next <= last { + t.Errorf("non-increasing timestampfound: %d <= %d", next, last) + } + last = next + } +} From 375d7b41b0bba498a1f4af549a4e1dba827bf8bf Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Fri, 16 Jul 2021 14:50:37 -0700 Subject: [PATCH 2/3] apply code review Co-authored-by: Marten Seemann --- peer/record_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/peer/record_test.go b/peer/record_test.go index e5aa0792..4022d929 100644 --- a/peer/record_test.go +++ b/peer/record_test.go @@ -56,11 +56,11 @@ func TestSignedPeerRecordFromEnvelope(t *testing.T) { // This is pretty much guaranteed to pass on Linux no matter how we implement it, but Windows has // low clock precision. This makes sure we never get a duplicate. func TestTimestampSeq(t *testing.T) { - last := uint64(0) + var last uint64 for i := 0; i < 1000; i++ { next := TimestampSeq() if next <= last { - t.Errorf("non-increasing timestampfound: %d <= %d", next, last) + t.Errorf("non-increasing timestamp found: %d <= %d", next, last) } last = next } From 7a741a9f08d325695f7fa475d4caf6fa62efdc28 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Fri, 16 Jul 2021 14:51:05 -0700 Subject: [PATCH 3/3] use a lock --- peer/record.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/peer/record.go b/peer/record.go index 95c8f6c6..212cea72 100644 --- a/peer/record.go +++ b/peer/record.go @@ -2,7 +2,7 @@ package peer import ( "fmt" - "sync/atomic" + "sync" "time" pb "github.com/libp2p/go-libp2p-core/peer/pb" @@ -126,20 +126,22 @@ func PeerRecordFromProtobuf(msg *pb.PeerRecord) (*PeerRecord, error) { return record, nil } -var lastTimestamp uint64 +var ( + lastTimestampMu sync.Mutex + lastTimestamp uint64 +) // TimestampSeq is a helper to generate a timestamp-based sequence number for a PeerRecord. func TimestampSeq() uint64 { now := uint64(time.Now().UnixNano()) - previous := atomic.LoadUint64(&lastTimestamp) - // If the new time is not greater than the last tiemstamp, or if someone else beats us to - // updateing the timestamp, just use last+1. - // - // Technically, last+1 could be before "now". But it's still strictly increasing and close - // enough. - if now <= previous || !atomic.CompareAndSwapUint64(&lastTimestamp, previous, now) { - now = atomic.AddUint64(&lastTimestamp, 1) + lastTimestampMu.Lock() + defer lastTimestampMu.Unlock() + // Not all clocks are strictly increasing, but we need these sequence numbers to be strictly + // increasing. + if now <= lastTimestamp { + now = lastTimestamp + 1 } + lastTimestamp = now return now }