From 4428d1070fc7a6c3f072d56afa8a3a26a46a9e09 Mon Sep 17 00:00:00 2001 From: Adrian Sutton Date: Tue, 10 Sep 2024 06:12:56 +1000 Subject: [PATCH] op-dispute-mon: Identify unclaimed credits based on the withdrawal request timestamp (#11488) --- op-dispute-mon/mon/bonds/monitor.go | 28 ++---- op-dispute-mon/mon/bonds/monitor_test.go | 57 +++--------- op-dispute-mon/mon/service.go | 4 +- op-dispute-mon/mon/withdrawals.go | 38 ++++++-- op-dispute-mon/mon/withdrawals_test.go | 105 +++++++++++++++++++---- 5 files changed, 136 insertions(+), 96 deletions(-) diff --git a/op-dispute-mon/mon/bonds/monitor.go b/op-dispute-mon/mon/bonds/monitor.go index efa7804d3408..2ce23dc472e7 100644 --- a/op-dispute-mon/mon/bonds/monitor.go +++ b/op-dispute-mon/mon/bonds/monitor.go @@ -17,22 +17,19 @@ type RClock interface { type BondMetrics interface { RecordCredit(expectation metrics.CreditExpectation, count int) RecordBondCollateral(addr common.Address, required *big.Int, available *big.Int) - RecordHonestWithdrawableAmounts(map[common.Address]*big.Int) } type Bonds struct { - logger log.Logger - clock RClock - metrics BondMetrics - honestActors types.HonestActors + logger log.Logger + clock RClock + metrics BondMetrics } -func NewBonds(logger log.Logger, metrics BondMetrics, honestActors types.HonestActors, clock RClock) *Bonds { +func NewBonds(logger log.Logger, metrics BondMetrics, clock RClock) *Bonds { return &Bonds{ - logger: logger, - clock: clock, - metrics: metrics, - honestActors: honestActors, + logger: logger, + clock: clock, + metrics: metrics, } } @@ -50,10 +47,6 @@ func (b *Bonds) CheckBonds(games []*types.EnrichedGameData) { func (b *Bonds) checkCredits(games []*types.EnrichedGameData) { creditMetrics := make(map[metrics.CreditExpectation]int) - honestWithdrawableAmounts := make(map[common.Address]*big.Int) - for address := range b.honestActors { - honestWithdrawableAmounts[address] = big.NewInt(0) - } for _, game := range games { // Check if the max duration has been reached for this game @@ -101,12 +94,6 @@ func (b *Bonds) checkCredits(games []*types.EnrichedGameData) { } comparison := actual.Cmp(expected) if maxDurationReached { - if actual.Cmp(big.NewInt(0)) > 0 && b.honestActors.Contains(recipient) { - total := honestWithdrawableAmounts[recipient] - total = new(big.Int).Add(total, actual) - honestWithdrawableAmounts[recipient] = total - b.logger.Warn("Found unclaimed credit", "recipient", recipient, "game", game.Proxy, "amount", actual) - } if comparison > 0 { creditMetrics[metrics.CreditAboveWithdrawable] += 1 b.logger.Warn("Credit above expected amount", "recipient", recipient, "expected", expected, "actual", actual, "game", game.Proxy, "withdrawable", "withdrawable") @@ -136,5 +123,4 @@ func (b *Bonds) checkCredits(games []*types.EnrichedGameData) { b.metrics.RecordCredit(metrics.CreditBelowNonWithdrawable, creditMetrics[metrics.CreditBelowNonWithdrawable]) b.metrics.RecordCredit(metrics.CreditEqualNonWithdrawable, creditMetrics[metrics.CreditEqualNonWithdrawable]) b.metrics.RecordCredit(metrics.CreditAboveNonWithdrawable, creditMetrics[metrics.CreditAboveNonWithdrawable]) - b.metrics.RecordHonestWithdrawableAmounts(honestWithdrawableAmounts) } diff --git a/op-dispute-mon/mon/bonds/monitor_test.go b/op-dispute-mon/mon/bonds/monitor_test.go index 5fd766b87061..dbeddd74d9c7 100644 --- a/op-dispute-mon/mon/bonds/monitor_test.go +++ b/op-dispute-mon/mon/bonds/monitor_test.go @@ -17,10 +17,7 @@ import ( ) var ( - frozen = time.Unix(int64(time.Hour.Seconds()), 0) - honestActor1 = common.Address{0x11, 0xaa} - honestActor2 = common.Address{0x22, 0xbb} - honestActor3 = common.Address{0x33, 0xcc} + frozen = time.Unix(int64(time.Hour.Seconds()), 0) ) func TestCheckBonds(t *testing.T) { @@ -64,8 +61,8 @@ func TestCheckBonds(t *testing.T) { } func TestCheckRecipientCredit(t *testing.T) { - addr1 := honestActor1 - addr2 := honestActor2 + addr1 := common.Address{0x11, 0xaa} + addr2 := common.Address{0x22, 0xbb} addr3 := common.Address{0x3c} addr4 := common.Address{0x4d} notRootPosition := types.NewPositionFromGIndex(big.NewInt(2)) @@ -349,14 +346,6 @@ func TestCheckRecipientCredit(t *testing.T) { require.Equal(t, 2, m.credits[metrics.CreditEqualNonWithdrawable], "CreditEqualNonWithdrawable") require.Equal(t, 2, m.credits[metrics.CreditAboveNonWithdrawable], "CreditAboveNonWithdrawable") - require.Len(t, m.honestWithdrawable, 3) - requireBigInt := func(name string, expected, actual *big.Int) { - require.Truef(t, expected.Cmp(actual) == 0, "Expected %v withdrawable to be %v but was %v", name, expected, actual) - } - requireBigInt("honest addr1", m.honestWithdrawable[addr1], big.NewInt(19)) - requireBigInt("honest addr2", m.honestWithdrawable[addr2], big.NewInt(13)) - requireBigInt("honest addr3", m.honestWithdrawable[honestActor3], big.NewInt(0)) - // Logs from game1 // addr1 is correct so has no logs // addr2 is below expected before max duration, so warn about early withdrawal @@ -382,18 +371,8 @@ func TestCheckRecipientCredit(t *testing.T) { testlog.NewAttributesFilter("withdrawable", "non_withdrawable"))) // Logs from game 2 - // addr1 is below expected - no warning as withdrawals may now be possible, but has unclaimed credit - require.NotNil(t, logs.FindLog( - testlog.NewLevelFilter(log.LevelWarn), - testlog.NewMessageFilter("Found unclaimed credit"), - testlog.NewAttributesFilter("game", game2.Proxy.Hex()), - testlog.NewAttributesFilter("recipient", addr1.Hex()))) - // addr2 is correct but has unclaimed credit - require.NotNil(t, logs.FindLog( - testlog.NewLevelFilter(log.LevelWarn), - testlog.NewMessageFilter("Found unclaimed credit"), - testlog.NewAttributesFilter("game", game2.Proxy.Hex()), - testlog.NewAttributesFilter("recipient", addr2.Hex()))) + // addr1 is below expected - no warning as withdrawals may now be possible + // addr2 is correct // addr3 is above expected - warn require.NotNil(t, logs.FindLog( testlog.NewLevelFilter(log.LevelWarn), @@ -422,18 +401,8 @@ func TestCheckRecipientCredit(t *testing.T) { testlog.NewAttributesFilter("withdrawable", "non_withdrawable"))) // Logs from game 4 - // addr1 is correct but has unclaimed credit - require.NotNil(t, logs.FindLog( - testlog.NewLevelFilter(log.LevelWarn), - testlog.NewMessageFilter("Found unclaimed credit"), - testlog.NewAttributesFilter("game", game4.Proxy.Hex()), - testlog.NewAttributesFilter("recipient", addr1.Hex()))) - // addr2 is below expected before max duration, no log because withdrawals may be possible but warn about unclaimed - require.NotNil(t, logs.FindLog( - testlog.NewLevelFilter(log.LevelWarn), - testlog.NewMessageFilter("Found unclaimed credit"), - testlog.NewAttributesFilter("game", game4.Proxy.Hex()), - testlog.NewAttributesFilter("recipient", addr2.Hex()))) + // addr1 is correct + // addr2 is below expected before max duration, no log because withdrawals may be possible // addr3 is not involved so no logs // addr4 is above expected before max duration, so warn require.NotNil(t, logs.FindLog( @@ -450,19 +419,13 @@ func setupBondMetricsTest(t *testing.T) (*Bonds, *stubBondMetrics, *testlog.Capt credits: make(map[metrics.CreditExpectation]int), recorded: make(map[common.Address]Collateral), } - honestActors := monTypes.NewHonestActors([]common.Address{honestActor1, honestActor2, honestActor3}) - bonds := NewBonds(logger, metrics, honestActors, clock.NewDeterministicClock(frozen)) + bonds := NewBonds(logger, metrics, clock.NewDeterministicClock(frozen)) return bonds, metrics, logs } type stubBondMetrics struct { - credits map[metrics.CreditExpectation]int - recorded map[common.Address]Collateral - honestWithdrawable map[common.Address]*big.Int -} - -func (s *stubBondMetrics) RecordHonestWithdrawableAmounts(values map[common.Address]*big.Int) { - s.honestWithdrawable = values + credits map[metrics.CreditExpectation]int + recorded map[common.Address]Collateral } func (s *stubBondMetrics) RecordBondCollateral(addr common.Address, required *big.Int, available *big.Int) { diff --git a/op-dispute-mon/mon/service.go b/op-dispute-mon/mon/service.go index e637b23d63b0..8ce97b7b8dd9 100644 --- a/op-dispute-mon/mon/service.go +++ b/op-dispute-mon/mon/service.go @@ -116,7 +116,7 @@ func (s *Service) initResolutionMonitor() { } func (s *Service) initWithdrawalMonitor() { - s.withdrawals = NewWithdrawalMonitor(s.logger, s.metrics) + s.withdrawals = NewWithdrawalMonitor(s.logger, s.cl, s.metrics, s.honestActors) } func (s *Service) initGameCallerCreator() { @@ -145,7 +145,7 @@ func (s *Service) initForecast(cfg *config.Config) { } func (s *Service) initBonds() { - s.bonds = bonds.NewBonds(s.logger, s.metrics, s.honestActors, s.cl) + s.bonds = bonds.NewBonds(s.logger, s.metrics, s.cl) } func (s *Service) initOutputRollupClient(ctx context.Context, cfg *config.Config) error { diff --git a/op-dispute-mon/mon/withdrawals.go b/op-dispute-mon/mon/withdrawals.go index 5563b099afe5..1c7e9da2f9af 100644 --- a/op-dispute-mon/mon/withdrawals.go +++ b/op-dispute-mon/mon/withdrawals.go @@ -1,6 +1,9 @@ package mon import ( + "math/big" + "time" + "github.com/ethereum-optimism/optimism/op-dispute-mon/mon/types" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/log" @@ -8,25 +11,35 @@ import ( type WithdrawalMetrics interface { RecordWithdrawalRequests(delayedWeth common.Address, matches bool, count int) + RecordHonestWithdrawableAmounts(map[common.Address]*big.Int) } type WithdrawalMonitor struct { - logger log.Logger - metrics WithdrawalMetrics + logger log.Logger + clock RClock + metrics WithdrawalMetrics + honestActors types.HonestActors } -func NewWithdrawalMonitor(logger log.Logger, metrics WithdrawalMetrics) *WithdrawalMonitor { +func NewWithdrawalMonitor(logger log.Logger, clock RClock, metrics WithdrawalMetrics, honestActors types.HonestActors) *WithdrawalMonitor { return &WithdrawalMonitor{ - logger: logger, - metrics: metrics, + logger: logger, + clock: clock, + metrics: metrics, + honestActors: honestActors, } } func (w *WithdrawalMonitor) CheckWithdrawals(games []*types.EnrichedGameData) { + now := w.clock.Now() // Use a consistent time for all checks matching := make(map[common.Address]int) divergent := make(map[common.Address]int) + honestWithdrawableAmounts := make(map[common.Address]*big.Int) + for address := range w.honestActors { + honestWithdrawableAmounts[address] = big.NewInt(0) + } for _, game := range games { - matches, diverges := w.validateGameWithdrawals(game) + matches, diverges := w.validateGameWithdrawals(game, now, honestWithdrawableAmounts) matching[game.WETHContract] += matches divergent[game.WETHContract] += diverges } @@ -36,9 +49,10 @@ func (w *WithdrawalMonitor) CheckWithdrawals(games []*types.EnrichedGameData) { for contract, count := range divergent { w.metrics.RecordWithdrawalRequests(contract, false, count) } + w.metrics.RecordHonestWithdrawableAmounts(honestWithdrawableAmounts) } -func (w *WithdrawalMonitor) validateGameWithdrawals(game *types.EnrichedGameData) (int, int) { +func (w *WithdrawalMonitor) validateGameWithdrawals(game *types.EnrichedGameData, now time.Time, honestWithdrawableAmounts map[common.Address]*big.Int) (int, int) { matching := 0 divergent := 0 for recipient, withdrawalAmount := range game.WithdrawalRequests { @@ -48,6 +62,16 @@ func (w *WithdrawalMonitor) validateGameWithdrawals(game *types.EnrichedGameData divergent++ w.logger.Error("Withdrawal request amount does not match credit", "game", game.Proxy, "recipient", recipient, "credit", game.Credits[recipient], "withdrawal", game.WithdrawalRequests[recipient].Amount) } + + if withdrawalAmount.Amount.Cmp(big.NewInt(0)) > 0 && w.honestActors.Contains(recipient) { + if time.Unix(withdrawalAmount.Timestamp.Int64(), 0).Add(game.WETHDelay).Before(now) { + // Credits are withdrawable + total := honestWithdrawableAmounts[recipient] + total = new(big.Int).Add(total, withdrawalAmount.Amount) + honestWithdrawableAmounts[recipient] = total + w.logger.Warn("Found unclaimed credit", "recipient", recipient, "game", game.Proxy, "amount", withdrawalAmount.Amount) + } + } } return matching, divergent } diff --git a/op-dispute-mon/mon/withdrawals_test.go b/op-dispute-mon/mon/withdrawals_test.go index f598ea38df97..ad17493adf65 100644 --- a/op-dispute-mon/mon/withdrawals_test.go +++ b/op-dispute-mon/mon/withdrawals_test.go @@ -3,9 +3,12 @@ package mon import ( "math/big" "testing" + "time" "github.com/ethereum-optimism/optimism/op-challenger/game/fault/contracts" + "github.com/ethereum-optimism/optimism/op-challenger/game/types" monTypes "github.com/ethereum-optimism/optimism/op-dispute-mon/mon/types" + "github.com/ethereum-optimism/optimism/op-service/clock" "github.com/ethereum-optimism/optimism/op-service/testlog" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/log" @@ -15,58 +18,76 @@ import ( var ( weth1 = common.Address{0x1a} weth2 = common.Address{0x2b} + + honestActor1 = common.Address{0x11, 0xaa} + honestActor2 = common.Address{0x22, 0xbb} + honestActor3 = common.Address{0x33, 0xcc} + dishonestActor4 = common.Address{0x44, 0xdd} + + nowUnix = int64(10_000) ) func makeGames() []*monTypes.EnrichedGameData { weth1Balance := big.NewInt(4200) weth2Balance := big.NewInt(6000) + game1 := &monTypes.EnrichedGameData{ + GameMetadata: types.GameMetadata{Proxy: common.Address{0x11, 0x11, 0x11}}, Credits: map[common.Address]*big.Int{ - common.Address{0x01}: big.NewInt(3), - common.Address{0x02}: big.NewInt(1), + honestActor1: big.NewInt(3), + honestActor2: big.NewInt(1), }, WithdrawalRequests: map[common.Address]*contracts.WithdrawalRequest{ - common.Address{0x01}: &contracts.WithdrawalRequest{Amount: big.NewInt(3)}, - common.Address{0x02}: &contracts.WithdrawalRequest{Amount: big.NewInt(1)}, + honestActor1: {Amount: big.NewInt(3), Timestamp: big.NewInt(nowUnix - 101)}, // Claimable + honestActor2: {Amount: big.NewInt(1), Timestamp: big.NewInt(nowUnix - 99)}, // Not claimable }, WETHContract: weth1, ETHCollateral: weth1Balance, + WETHDelay: 100 * time.Second, } game2 := &monTypes.EnrichedGameData{ + GameMetadata: types.GameMetadata{Proxy: common.Address{0x22, 0x22, 0x22}}, Credits: map[common.Address]*big.Int{ - common.Address{0x01}: big.NewInt(46), - common.Address{0x02}: big.NewInt(1), + honestActor1: big.NewInt(46), + honestActor2: big.NewInt(1), }, WithdrawalRequests: map[common.Address]*contracts.WithdrawalRequest{ - common.Address{0x01}: &contracts.WithdrawalRequest{Amount: big.NewInt(3)}, - common.Address{0x02}: &contracts.WithdrawalRequest{Amount: big.NewInt(1)}, + honestActor1: {Amount: big.NewInt(3), Timestamp: big.NewInt(nowUnix - 501)}, // Claimable + honestActor2: {Amount: big.NewInt(1), Timestamp: big.NewInt(nowUnix)}, // Not claimable }, WETHContract: weth2, ETHCollateral: weth2Balance, + WETHDelay: 500 * time.Second, } game3 := &monTypes.EnrichedGameData{ + GameMetadata: types.GameMetadata{Proxy: common.Address{0x33, 0x33, 0x33}}, Credits: map[common.Address]*big.Int{ - common.Address{0x03}: big.NewInt(2), - common.Address{0x04}: big.NewInt(4), + honestActor3: big.NewInt(2), + dishonestActor4: big.NewInt(4), }, WithdrawalRequests: map[common.Address]*contracts.WithdrawalRequest{ - common.Address{0x03}: &contracts.WithdrawalRequest{Amount: big.NewInt(2)}, - common.Address{0x04}: &contracts.WithdrawalRequest{Amount: big.NewInt(4)}, + honestActor3: {Amount: big.NewInt(2), Timestamp: big.NewInt(nowUnix - 1)}, // Claimable + dishonestActor4: {Amount: big.NewInt(4), Timestamp: big.NewInt(nowUnix - 5)}, // Claimable }, WETHContract: weth2, ETHCollateral: weth2Balance, + WETHDelay: 0 * time.Second, } return []*monTypes.EnrichedGameData{game1, game2, game3} } func TestCheckWithdrawals(t *testing.T) { - logger := testlog.Logger(t, log.LvlInfo) + now := time.Unix(nowUnix, 0) + cl := clock.NewDeterministicClock(now) + logger, logs := testlog.CaptureLogger(t, log.LvlInfo) metrics := &stubWithdrawalsMetrics{ matching: make(map[common.Address]int), divergent: make(map[common.Address]int), } - withdrawals := NewWithdrawalMonitor(logger, metrics) - withdrawals.CheckWithdrawals(makeGames()) + honestActors := monTypes.NewHonestActors([]common.Address{honestActor1, honestActor2, honestActor3}) + withdrawals := NewWithdrawalMonitor(logger, cl, metrics, honestActors) + games := makeGames() + withdrawals.CheckWithdrawals(games) require.Equal(t, metrics.matchCalls, 2) require.Equal(t, metrics.divergeCalls, 2) @@ -80,13 +101,59 @@ func TestCheckWithdrawals(t *testing.T) { require.Equal(t, metrics.matching[weth2], 3) require.Equal(t, metrics.divergent[weth1], 0) require.Equal(t, metrics.divergent[weth2], 1) + + require.Len(t, metrics.honestWithdrawable, 3) + requireBigInt := func(name string, expected, actual *big.Int) { + require.Truef(t, expected.Cmp(actual) == 0, "Expected %v withdrawable to be %v but was %v", name, expected, actual) + } + requireBigInt("honest addr1", big.NewInt(6), metrics.honestWithdrawable[honestActor1]) + requireBigInt("honest addr2", big.NewInt(0), metrics.honestWithdrawable[honestActor2]) + requireBigInt("honest addr3", big.NewInt(2), metrics.honestWithdrawable[honestActor3]) + require.Nil(t, metrics.honestWithdrawable[dishonestActor4], "should only report withdrawable credits for honest actors") + + findUnclaimedCreditWarning := func(game common.Address, actor common.Address) *testlog.CapturedRecord { + return logs.FindLog( + testlog.NewLevelFilter(log.LevelWarn), + testlog.NewMessageFilter("Found unclaimed credit"), + testlog.NewAttributesFilter("game", game.Hex()), + testlog.NewAttributesFilter("recipient", actor.Hex())) + } + requireUnclaimedWarning := func(game common.Address, actor common.Address) { + require.NotNil(t, findUnclaimedCreditWarning(game, actor)) + } + noUnclaimedWarning := func(game common.Address, actor common.Address) { + require.Nil(t, findUnclaimedCreditWarning(game, actor)) + } + // Game 1, unclaimed for honestActor1 only + requireUnclaimedWarning(games[0].Proxy, honestActor1) + noUnclaimedWarning(games[0].Proxy, honestActor2) + noUnclaimedWarning(games[0].Proxy, honestActor3) + noUnclaimedWarning(games[0].Proxy, dishonestActor4) + + // Game 2, unclaimed for honestActor1 only + requireUnclaimedWarning(games[1].Proxy, honestActor1) + noUnclaimedWarning(games[1].Proxy, honestActor2) + noUnclaimedWarning(games[1].Proxy, honestActor3) + noUnclaimedWarning(games[1].Proxy, dishonestActor4) + + // Game 3, unclaimed for honestActor3 only + // dishonestActor4 has unclaimed credits but we don't track them + noUnclaimedWarning(games[2].Proxy, honestActor1) + noUnclaimedWarning(games[2].Proxy, honestActor2) + requireUnclaimedWarning(games[2].Proxy, honestActor3) + noUnclaimedWarning(games[2].Proxy, dishonestActor4) } type stubWithdrawalsMetrics struct { - matchCalls int - divergeCalls int - matching map[common.Address]int - divergent map[common.Address]int + matchCalls int + divergeCalls int + matching map[common.Address]int + divergent map[common.Address]int + honestWithdrawable map[common.Address]*big.Int +} + +func (s *stubWithdrawalsMetrics) RecordHonestWithdrawableAmounts(honestWithdrawable map[common.Address]*big.Int) { + s.honestWithdrawable = honestWithdrawable } func (s *stubWithdrawalsMetrics) RecordWithdrawalRequests(addr common.Address, matches bool, count int) {