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.

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

feat(ntp): add ntp util #1274

Merged
merged 6 commits into from
May 18, 2024

Conversation

ambersun1234
Copy link
Contributor

Description

This PR add NTP util

The availability score may drop due to unstable network connection or mismatch system clock with the whole blockchain network
Thus we provide an information in CLI, GUI and gRPC endpoint, show that how many milliseconds your node is ahead/behind the network.

We use NTP servers to get the correct time
and if the absolute difference between your node and the whole network is greater than 1 second, the score may drop

All in all, this functionality allows user get the basic idea on what's going on with the node's status

Related issue(s)

@ambersun1234 ambersun1234 requested a review from b00f May 12, 2024 13:07
@ambersun1234 ambersun1234 force-pushed the feature/ntp-util branch 2 times, most recently from 7fb7b9c to 2f1a71c Compare May 12, 2024 16:43
util/ntp/ntp.go Outdated
serverList []string
}

func NewNtpServer() *Server {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically it is not NTPServer.
We better name it NTPChecker.

@b00f
Copy link
Collaborator

b00f commented May 13, 2024

We can use validate to check the time drift.

@ambersun1234 ambersun1234 requested a review from b00f May 13, 2024 13:31
Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 31.74603% with 43 lines in your changes are missing coverage. Please review.

Project coverage is 75.81%. Comparing base (20e991a) to head (a749e67).
Report is 9 commits behind head on main.

❗ Current head a749e67 differs from pull request most recent head ad15e4a. Consider uploading reports for the commit ad15e4a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1274      +/-   ##
==========================================
- Coverage   76.05%   75.81%   -0.25%     
==========================================
  Files         206      207       +1     
  Lines       10672    10742      +70     
==========================================
+ Hits         8117     8144      +27     
- Misses       2169     2205      +36     
- Partials      386      393       +7     

@ambersun1234 ambersun1234 force-pushed the feature/ntp-util branch 2 times, most recently from be6ae23 to 484a9b0 Compare May 14, 2024 08:32
util/ntp/ntp.go Outdated
offset time.Duration
interval time.Duration
logger *logger.SubLogger
retryTimes int
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the point of retrying?
Generally speaking, we repeat this check every few minutes, and we don't plan to use it as a "single source of truth." Therefore, we don't need to force retries on failure.

util/ntp/ntp.go Outdated
threshold time.Duration
offset time.Duration
interval time.Duration
logger *logger.SubLogger
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid defining sub logger for small modules.
Here we can use the global logger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should have a consistent approach to this

util/ntp/ntp.go Outdated
server := "pool.ntp.org"
response, err := ntp.Query(server)
if err != nil {
c.logger.Debug(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we have an error, so better we log it as Error not Debug

util/ntp/ntp.go Outdated
}

if err := response.Validate(); err != nil {
c.logger.Debug(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. An error has occurred but we log it as Debug.

return

case <-ticker.C:
c.lock.Lock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has flaw.
Imagine c.clockOffset took time to process. and we locked till it finishes.
Therefore GetClockOffset() will be blocked.

util/ntp/ntp.go Outdated
return math.Abs(float64(offset)) > float64(c.threshold)
}

func (*Checker) String() string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason for defining this?

util/ntp/ntp.go Outdated
}

func (c *Checker) checkClockOffset() {
ticker := time.NewTicker(c.interval)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep ticker instance and close it on Stop.

util/ntp/ntp.go Outdated
c.cancel()
}

func (c *Checker) GetThreshold() time.Duration {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the reason for this.
Whoever creates the NTP check also knows the threshold. Why should we report it again here?

}

func (c *Checker) GetClockOffset() (time.Duration, error) {
c.lock.RLock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

It can de simplified using defer

util/ntp/ntp.go Outdated
c.lock.RUnlock()

if offset == maxClockOffset {
return 0, errors.New("error on getting clock offset")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please define error type.
That helps to test the package

@ambersun1234 ambersun1234 merged commit cfc28ae into pactus-project:main May 18, 2024
10 checks passed
laosiji-io added a commit to laosiji-io/pactus that referenced this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants