From 4aeeb859a2685cb3746eabe41a60db0659c21855 Mon Sep 17 00:00:00 2001 From: refcell Date: Mon, 18 Mar 2024 19:26:44 -0400 Subject: [PATCH] fix(challenger,dispute-mon): refactor out min game timestamp into (#9890) op-service. --- op-challenger/game/monitor.go | 16 ++------ op-challenger/game/monitor_test.go | 28 ------------- op-dispute-mon/mon/monitor.go | 15 +------ op-dispute-mon/mon/monitor_test.go | 26 ------------ op-service/clock/util.go | 25 ++++++++++++ op-service/clock/util_test.go | 64 ++++++++++++++++++++++++++++++ 6 files changed, 94 insertions(+), 80 deletions(-) create mode 100644 op-service/clock/util.go create mode 100644 op-service/clock/util_test.go diff --git a/op-challenger/game/monitor.go b/op-challenger/game/monitor.go index 16d19639d476..fe263a661692 100644 --- a/op-challenger/game/monitor.go +++ b/op-challenger/game/monitor.go @@ -9,6 +9,7 @@ import ( "github.com/ethereum-optimism/optimism/op-challenger/game/scheduler" "github.com/ethereum-optimism/optimism/op-challenger/game/types" + "github.com/ethereum-optimism/optimism/op-service/clock" "github.com/ethereum-optimism/optimism/op-service/eth" "github.com/ethereum/go-ethereum" @@ -107,20 +108,9 @@ func (m *gameMonitor) allowedGame(game common.Address) bool { return false } -func (m *gameMonitor) minGameTimestamp() uint64 { - if m.gameWindow.Seconds() == 0 { - return 0 - } - // time: "To compute t-d for a duration d, use t.Add(-d)." - // https://pkg.go.dev/time#Time.Sub - if m.clock.Now().Unix() > int64(m.gameWindow.Seconds()) { - return uint64(m.clock.Now().Add(-m.gameWindow).Unix()) - } - return 0 -} - func (m *gameMonitor) progressGames(ctx context.Context, blockHash common.Hash, blockNumber uint64) error { - games, err := m.source.GetGamesAtOrAfter(ctx, blockHash, m.minGameTimestamp()) + minGameTimestamp := clock.MinCheckedTimestamp(m.clock, m.gameWindow) + games, err := m.source.GetGamesAtOrAfter(ctx, blockHash, minGameTimestamp) if err != nil { return fmt.Errorf("failed to load games: %w", err) } diff --git a/op-challenger/game/monitor_test.go b/op-challenger/game/monitor_test.go index 376c94844220..e4fe24e5b799 100644 --- a/op-challenger/game/monitor_test.go +++ b/op-challenger/game/monitor_test.go @@ -20,34 +20,6 @@ import ( "github.com/ethereum-optimism/optimism/op-service/clock" ) -func TestMonitorMinGameTimestamp(t *testing.T) { - t.Parallel() - - t.Run("zero game window returns zero", func(t *testing.T) { - monitor, _, _, _, _, _ := setupMonitorTest(t, []common.Address{}) - monitor.gameWindow = time.Duration(0) - require.Equal(t, monitor.minGameTimestamp(), uint64(0)) - }) - - t.Run("non-zero game window with zero clock", func(t *testing.T) { - monitor, _, _, _, _, _ := setupMonitorTest(t, []common.Address{}) - monitor.gameWindow = time.Minute - monitor.clock = clock.NewSimpleClock() - monitor.clock.SetTime(0) - require.Equal(t, uint64(0), monitor.minGameTimestamp()) - }) - - t.Run("minimum computed correctly", func(t *testing.T) { - monitor, _, _, _, _, _ := setupMonitorTest(t, []common.Address{}) - monitor.gameWindow = time.Minute - monitor.clock = clock.NewSimpleClock() - frozen := uint64(time.Hour.Seconds()) - monitor.clock.SetTime(frozen) - expected := uint64(time.Unix(int64(frozen), 0).Add(-time.Minute).Unix()) - require.Equal(t, monitor.minGameTimestamp(), expected) - }) -} - // TestMonitorGames tests that the monitor can handle a new head event // and resubscribe to new heads if the subscription errors. func TestMonitorGames(t *testing.T) { diff --git a/op-dispute-mon/mon/monitor.go b/op-dispute-mon/mon/monitor.go index 1c83fdd6a519..5e8fe55df2df 100644 --- a/op-dispute-mon/mon/monitor.go +++ b/op-dispute-mon/mon/monitor.go @@ -68,18 +68,6 @@ func newGameMonitor( } } -func (m *gameMonitor) minGameTimestamp() uint64 { - if m.gameWindow.Seconds() == 0 { - return 0 - } - // time: "To compute t-d for a duration d, use t.Add(-d)." - // https://pkg.go.dev/time#Time.Sub - if m.clock.Now().Unix() > int64(m.gameWindow.Seconds()) { - return uint64(m.clock.Now().Add(-m.gameWindow).Unix()) - } - return 0 -} - func (m *gameMonitor) monitorGames() error { blockNumber, err := m.fetchBlockNumber(m.ctx) if err != nil { @@ -90,7 +78,8 @@ func (m *gameMonitor) monitorGames() error { if err != nil { return fmt.Errorf("failed to fetch block hash: %w", err) } - enrichedGames, err := m.extract(m.ctx, blockHash, m.minGameTimestamp()) + minGameTimestamp := clock.MinCheckedTimestamp(m.clock, m.gameWindow) + enrichedGames, err := m.extract(m.ctx, blockHash, minGameTimestamp) if err != nil { return fmt.Errorf("failed to load games: %w", err) } diff --git a/op-dispute-mon/mon/monitor_test.go b/op-dispute-mon/mon/monitor_test.go index 68ef62d0312a..6127d145fbef 100644 --- a/op-dispute-mon/mon/monitor_test.go +++ b/op-dispute-mon/mon/monitor_test.go @@ -20,32 +20,6 @@ var ( mockErr = errors.New("mock error") ) -func TestMonitor_MinGameTimestamp(t *testing.T) { - t.Parallel() - - t.Run("ZeroGameWindow", func(t *testing.T) { - monitor, _, _, _, _ := setupMonitorTest(t) - monitor.gameWindow = time.Duration(0) - require.Equal(t, monitor.minGameTimestamp(), uint64(0)) - }) - - t.Run("ZeroClock", func(t *testing.T) { - monitor, _, _, _, _ := setupMonitorTest(t) - monitor.gameWindow = time.Minute - monitor.clock = clock.NewDeterministicClock(time.Unix(0, 0)) - require.Equal(t, uint64(0), monitor.minGameTimestamp()) - }) - - t.Run("ValidArithmetic", func(t *testing.T) { - monitor, _, _, _, _ := setupMonitorTest(t) - monitor.gameWindow = time.Minute - frozen := time.Unix(int64(time.Hour.Seconds()), 0) - monitor.clock = clock.NewDeterministicClock(frozen) - expected := uint64(frozen.Add(-time.Minute).Unix()) - require.Equal(t, monitor.minGameTimestamp(), expected) - }) -} - func TestMonitor_MonitorGames(t *testing.T) { t.Parallel() diff --git a/op-service/clock/util.go b/op-service/clock/util.go new file mode 100644 index 000000000000..dc564c652fff --- /dev/null +++ b/op-service/clock/util.go @@ -0,0 +1,25 @@ +package clock + +import ( + "time" +) + +type RWClock interface { + Now() time.Time +} + +// MinCheckedTimestamp returns the minimum checked unix timestamp. +// If the duration is 0, the returned minimum timestamp is 0. +// Otherwise, the minimum timestamp is the current unix time minus the duration. +// The subtraction operation is checked and returns 0 on underflow. +func MinCheckedTimestamp(clock RWClock, duration time.Duration) uint64 { + if duration.Seconds() == 0 { + return 0 + } + // To compute t-d for a duration d, use t.Add(-d). + // See https://pkg.go.dev/time#Time.Sub + if clock.Now().Unix() > int64(duration.Seconds()) { + return uint64(clock.Now().Add(-duration).Unix()) + } + return 0 +} diff --git a/op-service/clock/util_test.go b/op-service/clock/util_test.go new file mode 100644 index 000000000000..b2c5e3f50259 --- /dev/null +++ b/op-service/clock/util_test.go @@ -0,0 +1,64 @@ +package clock + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestMinCheckedTimestamp(t *testing.T) { + tests := []struct { + name string + now time.Time + duration time.Duration + want uint64 + }{ + { + name: "ZeroDurationZeroClock", + now: time.Unix(0, 0), + duration: 0, + want: 0, + }, + { + name: "ZeroDurationPositiveClock", + now: time.Unix(1, 0), + duration: 0, + want: 0, + }, + { + name: "UnderflowZeroClock", + now: time.Unix(0, 0), + duration: time.Second, + want: 0, + }, + { + name: "UnderflowPositiveClock", + now: time.Unix(1, 0), + duration: time.Second * 2, + want: 0, + }, + { + name: "CorrectArithmetic", + now: time.Unix(100, 0), + duration: time.Second * 10, + want: 90, + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + clock := &mockClock{now: test.now} + require.Equal(t, test.want, MinCheckedTimestamp(clock, test.duration)) + }) + } +} + +type mockClock struct { + now time.Time +} + +func (m *mockClock) Now() time.Time { + return m.now +}