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

Refactor NTP, support multiple NTP servers #4728

Merged
merged 3 commits into from
Aug 21, 2024
Merged

Refactor NTP, support multiple NTP servers #4728

merged 3 commits into from
Aug 21, 2024

Conversation

GheisMohammadi
Copy link
Contributor

Issue

This PR refactors the NTP functionality to support querying multiple NTP servers and addresses the NTP timeout issue commonly encountered in localnet. The PR also includes minor adjustments to error handling and logging for better troubleshooting and debugging.

Key Changes:

  • Multiple NTP Servers Support:

    • The NTP functionality now accepts a comma-separated list of NTP servers. It adds 0.beevik-ntp.pool.ntp.org to ntp servers as well.
    • It iterates through the provided servers, using the first successful response to determine time accuracy. If all servers fail, detailed error messages are returned.
  • Timeout Issue Fix:

    • Fixed the issue where NTP queries would frequently time out in localnet.
    • Increased the NTP query timeout from 10 seconds to 30 seconds to provide more resilience in network conditions.

@Frozen
Copy link
Contributor

Frozen commented Aug 8, 2024

Why it's better than just use time.Now() in localnet?

@GheisMohammadi
Copy link
Contributor Author

Why it's better than just use time.Now() in localnet?

This PR addresses the issue for all networks and is not touching the logic. However, your suggestion is also great and valuable. I believe it’s acceptable to skip the time accuracy check on the localnet since all nodes are running on the same machine. I will apply this adjustment to the code.

@sophoah sophoah merged commit b33cbc3 into dev Aug 21, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants