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

SNTP Clock issues #4207

Closed
HowardHinnant opened this issue Jun 14, 2022 · 6 comments · Fixed by #4628
Closed

SNTP Clock issues #4207

HowardHinnant opened this issue Jun 14, 2022 · 6 comments · Fixed by #4628

Comments

@HowardHinnant
Copy link
Contributor

Issue Description

The purpose of this issue is to provide a place to discuss a couple of issues surrounding the time keeping used to create ledger close times.

Currently rippled communicates with an NTP server to get the correct time, and this time serves as the basis for determining ledger close times. Which NTP server is used is configurable in rippled.cfg under the [sntp-servers] tag where one or more servers can be listed, and one will be chosen for the job. It might be possible, however remotely, for a malicious actor to intercept communications between a node in the XRP network and its NTP server, and shift the network time of the node away from its peers.

  1. Should a node use only one NTP server? We could, for example, query three or more servers, eliminate any reporting a time sufficiently far from the mean, and average the rest for the current time.
  2. Should we be using NTP servers at all? We could just query the OS for the current time (e.g. std::chrono::system_clock::now()) and leave it up to server operators to keep their machines temporarily synced. This would allow us to remove the code we currently have that reaches out to NTP servers, and remove the associated config tag.
  3. Is the status quo the best option and thus we should leave well enough alone?
@greg7mdp
Copy link
Contributor

Maybe we could check (on linux/macos) whether ntpd is running, and if it is, just use the system's time (assuming ntpd will do a better job than we could avoiding drift). If ntpd is not running, or on windows, revert to querying NTP servers ourselves.

@HowardHinnant
Copy link
Contributor Author

Doesn't Windows also normally have an NTP server it is running?

@nbougalis
Copy link
Contributor

Maybe we could check (on linux/macos) whether ntpd is running, and if it is, just use the system's time (assuming ntpd will do a better job than we could avoiding drift). If ntpd is not running, or on windows, revert to querying NTP servers ourselves.

I think that if the decision is to rely on an external process (like chrony, ntpd or even sntpd) then we explicitly document a dependency on having time synchronization software running and then rely on admins ensuring that the environment inside which the daemon runs is properly configured.

I wouldn't have code that's mostly dead but spring to life in misconfigured systems; that's generally a recipe for disaster.

Considering that rippled refuses to establish peer connections if the clock drift between the server and its peer exceeds 20 seconds, I think that hosts without synchronized clocks that are wildly off would generally find themselves unable to connect (a message is logged) and would clue the admin in.

Long story short, I'm fine with removing this. Hey @JoelKatz, what's your take? We've been using this code since... must be at least 1998? Are you OK if it goes the way of the dodo and rippled requires the underlying system to have a synchronized clock?

@JoelKatz
Copy link
Collaborator

I have no objection to removing it.

@MarkusTeufelberger
Copy link
Collaborator

I also would prefer removing the NTP stuff from the code base. It is a nice safety net in some edge cases, but it masks operator errors and could lead to unexpected situations. It also seems to have very little impact on the protocol etc. anyways.

HowardHinnant added a commit to HowardHinnant/rippled that referenced this issue Aug 18, 2022
Fixes: XRPLF#4207

Make SNTPClock a simple wrapper around abstract_clock<system_clock>
@HowardHinnant
Copy link
Contributor Author

HowardHinnant commented Aug 18, 2022

Here is a patch that removes the SNTP code for review: HowardHinnant@6c239bd

HowardHinnant added a commit to HowardHinnant/rippled that referenced this issue Aug 18, 2022
Fixes: XRPLF#4207

Make SNTPClock a simple wrapper around abstract_clock<system_clock>
@intelliot intelliot linked a pull request Aug 29, 2023 that will close this issue
7 tasks
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 a pull request may close this issue.

9 participants
@JoelKatz @MarkusTeufelberger @greg7mdp @nbougalis @HowardHinnant and others