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(utils): use NTP timestamps to avoid sync problems #883

Merged
merged 1 commit into from
Nov 18, 2019

Conversation

lrubiorod
Copy link
Contributor

@lrubiorod lrubiorod commented Nov 14, 2019

Close #874

util/Cargo.toml Outdated Show resolved Hide resolved
util/Cargo.toml Outdated Show resolved Hide resolved
util/src/timestamp.rs Outdated Show resolved Hide resolved
util/src/timestamp.rs Outdated Show resolved Hide resolved
util/src/timestamp.rs Outdated Show resolved Hide resolved
@lrubiorod lrubiorod force-pushed the ntp branch 3 times, most recently from 2c4dae4 to e7d6168 Compare November 14, 2019 14:13
@tmpolaczyk
Copy link
Contributor

I think it would be better to move the NTP updating mechanism outside of the get_timestamp function. The main issue with the current approach is the timeout: a call to get_timestamp can block for up to 5 seconds.

We could have a periodic callback, for example in EpochManager, which would call update_global_timestamp. This way all the calls to get_timestamp have more or less the same overhead. It would be really nice if that function was async, so that it doesn't block the executor thread, but the NTP crate used here does not allow that. I don't know if there are other crates with that feature, but we could create a new issue for replacing the NTP request with an async one.

@lrubiorod lrubiorod force-pushed the ntp branch 2 times, most recently from eecffe4 to 39f428d Compare November 14, 2019 17:21
config/src/config.rs Outdated Show resolved Hide resolved
@lrubiorod lrubiorod force-pushed the ntp branch 3 times, most recently from a1585b2 to 4026853 Compare November 15, 2019 15:06
util/src/timestamp.rs Outdated Show resolved Hide resolved
@lrubiorod lrubiorod force-pushed the ntp branch 2 times, most recently from 7eccc41 to 0fcea21 Compare November 15, 2019 16:21
@@ -276,6 +281,17 @@ pub struct Mining {
pub enabled: bool,
}

/// Ntp-related configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Ntp-related configuration
/// NTP-related configuration

And we should probably explain somewhere what is NTP and why it's useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in witnet.io documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

config/src/config.rs Outdated Show resolved Hide resolved
config/src/config.rs Outdated Show resolved Hide resolved
config/src/defaults.rs Outdated Show resolved Hide resolved
util/src/timestamp.rs Outdated Show resolved Hide resolved
@lrubiorod lrubiorod force-pushed the ntp branch 4 times, most recently from 60c8d37 to eec7e62 Compare November 18, 2019 13:49
config/src/config.rs Outdated Show resolved Hide resolved
util/Cargo.toml Outdated Show resolved Hide resolved
@lrubiorod lrubiorod merged commit 7d126fc into witnet:master Nov 18, 2019
@lrubiorod lrubiorod deleted the ntp branch November 18, 2019 16:30
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.

Implement NTP (Network Time Protocol) synchronization
2 participants