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.

Sign up for GitHub

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

skip liquidation task loop if last committed block height is the same #2124

Merged
merged 3 commits into from
Aug 21, 2024
Merged

skip liquidation task loop if last committed block height is the same #2124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
231436f
skip liquidation task loop if last committed block height is the same
jayy04 Aug 21, 2024
78cb3fe
update block height
jayy04 Aug 21, 2024
1eeb66d
comments
jayy04 Aug 21, 2024
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
6 changes: 5 additions & 1 deletion protocol/daemons/liquidation/client/grpc_helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,11 @@ func TestSendLiquidatableSubaccountIds(t *testing.T) {
tc.subaccountOpenPositionInfo,
1000,
)
require.Equal(t, tc.expectedError, err)
if tc.expectedError != nil {
require.ErrorContains(t, err, tc.expectedError.Error())
} else {
require.NoError(t, err)
}
})
}
}
17 changes: 16 additions & 1 deletion protocol/daemons/liquidation/client/sub_task_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ type SubTaskRunner interface {
) error
}

type SubTaskRunnerImpl struct{}
type SubTaskRunnerImpl struct {
lastLoopBlockHeight uint32
}

// Ensure SubTaskRunnerImpl implements the SubTaskRunner interface.
var _ SubTaskRunner = (*SubTaskRunnerImpl)(nil)
Expand All @@ -50,6 +52,19 @@ func (s *SubTaskRunnerImpl) RunLiquidationDaemonTaskLoop(
return err
}

// Skip the loop if no new block has been committed.
// Note that lastLoopBlockHeight is initialized to 0, so the first loop will always run.
if lastCommittedBlockHeight == s.lastLoopBlockHeight {
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC if application restarts, lastLoopBlockHeight is by default 0 and we always continue below logic. If so could we document?

daemonClient.logger.Info(
"Skipping liquidation daemon task loop as no new block has been committed",
"blockHeight", lastCommittedBlockHeight,
)
return nil
}

// Update the last loop block height.
s.lastLoopBlockHeight = lastCommittedBlockHeight
Copy link
Contributor

@teddyding teddyding Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Playing devil's advocate: are we sure this is thread-safe? Could two liquidation daemon loop overlap? Should we add a sync.Mutex similar to LiquidationServer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no daemon loops don't overlap since we are not spawning new go routines

for {
		select {
		case <-ticker.C:
			if err := s.RunLiquidationDaemonTaskLoop(
				ctx,
				client,
				flags.Liquidation,
			); err != nil {
				// TODO(DEC-947): Move daemon shutdown to application.
				client.logger.Error("Liquidations daemon returned error", "error", err)
				client.ReportFailure(err)
			} else {
				client.ReportSuccess()
			}
		case <-stop:
			return
		}
	}

Copy link
Contributor

@teddyding teddyding Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: I wonder if here's the best place to assign s.lastLoopBlockHeight = lastCommittedBlockHeight since we haven't sent any current block info to application yet. Wdyt think about defering the statement s.lastLoopBlockHeight = lastCommittedBlockHeight

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question. thought about it a little bit more

putting it here strictly guarantees that loop doesn't run for the same block height, regardless of errors. If we defer the statement to the end (e.g. after SendLiquidatableSubaccountIds), theoretically it's possible that SendLiquidatableSubaccountIds only partially succeeds when first few requests goes through and later requests fail. In this case, we would be running into the same issue. I think might be slightly better here for correctness?

Copy link
Contributor

Choose a reason for hiding this comment

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

defering guarantees that the s.lastLoopBlockHeight = lastCommittedBlockHeight is always run before return, even if SendLiquidatableSubaccountIds fails right? Trying to understand when we'd send SendLiquidatableSubaccountIdsagain for the same block.

No strong preference though, the current implementation also works. Up to you. Approved to unblock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh you mean defer and update block height regardless of error? I misunderstood

yeah I think it's basically the same - let's keep it as is to unstuck the testnet first


// 1. Fetch all information needed to calculate total net collateral and margin requirements.
subaccounts,
perpInfos,
Expand Down
Loading