Fix NTP: implement RFC 5905 offset calculation and verify time before warning #2137
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements the RFC 5905 NTP offset calculation, which is a significant improvement over the previous logic that didn't account for network latency. It also adds a check to verify if the system time is actually out of sync before prompting the user, which improves the user experience. The migration to net.Dialer.DialContext is also a good move for context-aware network operations.
I've identified a few areas for improvement, mainly related to code duplication and logging consistency. Please see my detailed comments.
| func (m *ntpManager) getTimeOffset(ctx context.Context) (time.Duration, error) { | ||
| dialer := &net.Dialer{ | ||
| Timeout: ntpDialTimeout, | ||
| } | ||
|
|
||
| for i := range m.pools { | ||
| con, err := net.DialTimeout("udp", m.pools[i], 5*time.Second) //nolint:noctx // TODO: #2006 Use (*net.Dialer).DialContext with (*net.Dialer).Timeout | ||
| conn, err := dialer.DialContext(ctx, "udp", m.pools[i]) | ||
| if err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to connect to hosts %v attempting next", m.pools[i]) | ||
| log.Warnf(log.TimeMgr, "Unable to connect to hosts %v attempting to next", m.pools[i]) | ||
| continue | ||
| } | ||
|
|
||
| if err = con.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to SetDeadline. Error: %s\n", err) | ||
| err = con.Close() | ||
| if err != nil { | ||
| if err = conn.SetDeadline(time.Now().Add(ntpReadWriteTimeout)); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to set deadline on hosts %v. Error %s. attempting to next\n", m.pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // T1: Record time before sending request (origin timestamp) | ||
| t1 := time.Now() | ||
|
|
||
| req := &ntpPacket{Settings: 0x1B} | ||
| if err = binary.Write(con, binary.BigEndian, req); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to write. Error: %s\n", err) | ||
| err = con.Close() | ||
| if err != nil { | ||
| if err = binary.Write(conn, binary.BigEndian, req); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to write to hosts %v. Error %s. Attempting to next\n", m.pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| rsp := &ntpPacket{} | ||
| if err = binary.Read(con, binary.BigEndian, rsp); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to read. Error: %s\n", err) | ||
| err = con.Close() | ||
| if err != nil { | ||
| if err = binary.Read(conn, binary.BigEndian, rsp); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to read from hosts %v. Error: %s. Attempting to next\n", m.pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| secs := float64(rsp.TxTimeSec) - 2208988800 | ||
| nanos := (int64(rsp.TxTimeFrac) * 1e9) >> 32 | ||
| // T4L Record time after receiving response (Destination timestamp) | ||
| t4 := time.Now() | ||
|
|
||
| err = con.Close() | ||
| if err != nil { | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| return time.Unix(int64(secs), nanos) | ||
|
|
||
| // T2: Server receive timestamp (when server received our request) | ||
| t2 := ntpTimestampToTime(rsp.RxTimeSec, rsp.RxTimeFrac) | ||
| // T3: Server transmit timestamp (when server sent our response) | ||
| t3 := ntpTimestampToTime(rsp.TxTimeSec, rsp.TxTimeFrac) | ||
|
|
||
| // RFC 5905 offset calculation: ((T2-T1) + (T3-T4)) / 2 | ||
| // This formula cancels out the network round-trip time | ||
| offset := (t2.Sub(t1) + t3.Sub(t4)) / 2 | ||
| return offset, nil | ||
| } | ||
| log.Warnln(log.TimeMgr, "No valid NTP servers found, using current system time") | ||
| return time.Now().UTC() | ||
| log.Warnln(log.TimeMgr, "No valid NTP servers found") | ||
| return 0, errNoValidNTPServer | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between getTimeOffset and the new CheckNTPOffset function. The logic is almost identical. To improve maintainability and reduce redundancy, you could refactor getTimeOffset to call CheckNTPOffset. This would centralize the NTP offset calculation logic. Note that this would change some log messages to be consistent with CheckNTPOffset.
func (m *ntpManager) getTimeOffset(ctx context.Context) (time.Duration, error) {
return CheckNTPOffset(ctx, m.pools)
}| } | ||
|
|
||
| if err = conn.SetDeadline(time.Now().Add(ntpReadWriteTimeout)); err != nil { | ||
| log.Warnf(log.TimeMgr, "NTP check: Unable to set deadline on %v. Error %s. Attempting next\n", pools[i], err) |
There was a problem hiding this comment.
Several log.Warnf calls in this file have a trailing newline character (\n) in the format string (here and on lines 57, 66, 232, 253, 265, 274). This is inconsistent with other log.Warnf calls that don't have it. log.Warnf does not add a newline by default, so this leads to inconsistent formatting in logs. Please remove the explicit \n for consistency. If a newline is always intended, consider using log.Warnln.
| log.Warnf(log.TimeMgr, "NTP check: Unable to set deadline on %v. Error %s. Attempting next\n", pools[i], err) | |
| log.Warnf(log.TimeMgr, "NTP check: Unable to set deadline on %v. Error %s. Attempting next", pools[i], err) |
|
|
||
| secs := float64(rsp.TxTimeSec) - 2208988800 | ||
| nanos := (int64(rsp.TxTimeFrac) * 1e9) >> 32 | ||
| // T4L Record time after receiving response (Destination timestamp) |
There was a problem hiding this comment.
There's a typo in the comment. T4L should be T4.
// T4: Record time after receiving response (Destination timestamp)There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the NTP time synchronization implementation by properly implementing the RFC 5905 offset calculation formula. Previously, the code incorrectly calculated time difference using T3 - T4 (server transmit time minus client receive time), which included network latency and produced false positives. The fix implements the correct formula ((T2-T1) + (T3-T4)) / 2 which cancels out network round-trip time. Additionally, the PR adds proper time verification before prompting users, ensuring warnings only appear when time is actually out of sync.
Key Changes:
- Implemented RFC 5905 compliant offset calculation using all four NTP timestamps (T1, T2, T3, T4)
- Added CheckNTPOffset function to verify time sync before warning users at startup
- Migrated from deprecated net.DialTimeout to context-aware net.Dialer.DialContext
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| engine/ntp_manager.go | Core fix implementing RFC 5905 offset calculation, added CheckNTPOffset function, refactored time query logic to use proper timestamps and context |
| engine/ntp_manager_test.go | Updated test signatures to use context.Context, removed redundant test code, added Pool configuration to tests |
| engine/engine.go | Added time verification logic before prompting user, only shows warning when time is actually out of sync |
|
|
||
| secs := float64(rsp.TxTimeSec) - 2208988800 | ||
| nanos := (int64(rsp.TxTimeFrac) * 1e9) >> 32 | ||
| // T4L Record time after receiving response (Destination timestamp) |
There was a problem hiding this comment.
Typo in comment: "T4L" should be "T4:"
| // T4L Record time after receiving response (Destination timestamp) | |
| // T4: Record time after receiving response (Destination timestamp) |
| conn, err := dialer.DialContext(ctx, "udp", m.pools[i]) | ||
| if err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to connect to hosts %v attempting next", m.pools[i]) | ||
| log.Warnf(log.TimeMgr, "Unable to connect to hosts %v attempting to next", m.pools[i]) |
There was a problem hiding this comment.
Inconsistent error message formatting: "attempting to next" should be "attempting next" to match the pattern used in CheckNTPOffset function (line 40).
| log.Warnf(log.TimeMgr, "Unable to connect to hosts %v attempting to next", m.pools[i]) | |
| log.Warnf(log.TimeMgr, "Unable to connect to hosts %v. Attempting next\n", m.pools[i]) |
| err = con.Close() | ||
| if err != nil { | ||
| if err = conn.SetDeadline(time.Now().Add(ntpReadWriteTimeout)); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to set deadline on hosts %v. Error %s. attempting to next\n", m.pools[i], err) |
There was a problem hiding this comment.
Inconsistent error message formatting: "attempting to next" should be "attempting next" to match the pattern used in CheckNTPOffset function.
| err = con.Close() | ||
| if err != nil { | ||
| if err = binary.Write(conn, binary.BigEndian, req); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to write to hosts %v. Error %s. Attempting to next\n", m.pools[i], err) |
There was a problem hiding this comment.
Inconsistent error message formatting: "Attempting to next" should be "Attempting next" to match the pattern used in CheckNTPOffset function.
| err = con.Close() | ||
| if err != nil { | ||
| if err = binary.Read(conn, binary.BigEndian, rsp); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to read from hosts %v. Error: %s. Attempting to next\n", m.pools[i], err) |
There was a problem hiding this comment.
Inconsistent error message formatting: "Attempting to next" should be "Attempting next" to match the pattern used in CheckNTPOffset function.
| allowedDiff := *bot.Config.NTPClient.AllowedDifference | ||
| allowedNegDiff := -*bot.Config.NTPClient.AllowedNegativeDifference | ||
| if offset > allowedDiff || offset < allowedNegDiff { | ||
| gctlog.Warnf(gctlog.TimeMgr, "System time offset detected: %v (allowed: +%v / %v)", offset, allowedDiff, allowedNegDiff) | ||
| responseMessage, err := bot.Config.SetNTPCheck(os.Stdin) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to set NTP check: %w", err) | ||
| } | ||
| gctlog.Infoln(gctlog.TimeMgr, responseMessage) | ||
| } else { | ||
| gctlog.Debugf(gctlog.TimeMgr, "System time is in sync (offset: %v)", offset) |
There was a problem hiding this comment.
Potential nil pointer dereference: If CheckNTPConfig() fails to set the default values for AllowedDifference or AllowedNegativeDifference (which could happen in unusual circumstances), dereferencing these pointers on lines 345-346 would cause a panic. Consider adding nil checks before dereferencing, or ensure CheckNTPConfig is called earlier in the config initialization flow to guarantee these values are set.
| allowedDiff := *bot.Config.NTPClient.AllowedDifference | |
| allowedNegDiff := -*bot.Config.NTPClient.AllowedNegativeDifference | |
| if offset > allowedDiff || offset < allowedNegDiff { | |
| gctlog.Warnf(gctlog.TimeMgr, "System time offset detected: %v (allowed: +%v / %v)", offset, allowedDiff, allowedNegDiff) | |
| responseMessage, err := bot.Config.SetNTPCheck(os.Stdin) | |
| if err != nil { | |
| return fmt.Errorf("unable to set NTP check: %w", err) | |
| } | |
| gctlog.Infoln(gctlog.TimeMgr, responseMessage) | |
| } else { | |
| gctlog.Debugf(gctlog.TimeMgr, "System time is in sync (offset: %v)", offset) | |
| if bot.Config.NTPClient.AllowedDifference == nil || bot.Config.NTPClient.AllowedNegativeDifference == nil { | |
| gctlog.Warnf(gctlog.TimeMgr, "NTP client allowed time differences are not configured; skipping offset validation") | |
| } else { | |
| allowedDiff := *bot.Config.NTPClient.AllowedDifference | |
| allowedNegDiff := -*bot.Config.NTPClient.AllowedNegativeDifference | |
| if offset > allowedDiff || offset < allowedNegDiff { | |
| gctlog.Warnf(gctlog.TimeMgr, "System time offset detected: %v (allowed: +%v / %v)", offset, allowedDiff, allowedNegDiff) | |
| responseMessage, err := bot.Config.SetNTPCheck(os.Stdin) | |
| if err != nil { | |
| return fmt.Errorf("unable to set NTP check: %w", err) | |
| } | |
| gctlog.Infoln(gctlog.TimeMgr, responseMessage) | |
| } else { | |
| gctlog.Debugf(gctlog.TimeMgr, "System time is in sync (offset: %v)", offset) | |
| } |
| func CheckNTPOffset(ctx context.Context, pools []string) (time.Duration, error) { | ||
| if len(pools) == 0 { | ||
| return 0, errors.New("no NTP pools configured") | ||
| } | ||
|
|
||
| dialer := &net.Dialer{ | ||
| Timeout: ntpDialTimeout, | ||
| } | ||
|
|
||
| for i := range pools { | ||
| conn, err := dialer.DialContext(ctx, "udp", pools[i]) | ||
| if err != nil { | ||
| log.Warnf(log.TimeMgr, "NTP check: Unable to connect to %v, attempting next", pools[i]) | ||
| continue | ||
| } | ||
|
|
||
| if err = conn.SetDeadline(time.Now().Add(ntpReadWriteTimeout)); err != nil { | ||
| log.Warnf(log.TimeMgr, "NTP check: Unable to set deadline on %v. Error %s. Attempting next\n", pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // T1: Record time before sending request (origin timestamp) | ||
| t1 := time.Now() | ||
|
|
||
| req := &ntpPacket{Settings: 0x1B} | ||
| if err = binary.Write(conn, binary.BigEndian, req); err != nil { | ||
| log.Warnf(log.TimeMgr, "NTP check: Unable to write to %v. Error %s. Attempting next\n", pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| rsp := &ntpPacket{} | ||
| if err = binary.Read(conn, binary.BigEndian, rsp); err != nil { | ||
| log.Warnf(log.TimeMgr, "NTP check: Unable to read from %v. Error: %s. Attempting next\n", pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // T4: Record time after receiving response (Destination timestamp) | ||
| t4 := time.Now() | ||
|
|
||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
|
|
||
| // T2: Server receive timestamp (when server received our request) | ||
| t2 := ntpTimestampToTime(rsp.RxTimeSec, rsp.RxTimeFrac) | ||
| // T3: Server transmit timestamp (when server sent our response) | ||
| t3 := ntpTimestampToTime(rsp.TxTimeSec, rsp.TxTimeFrac) | ||
|
|
||
| // RFC 5905 offset calculation: ((T2-T1) + (T3-T4)) / 2 | ||
| // This formula cancels out the network round-trip time | ||
| offset := (t2.Sub(t1) + t3.Sub(t4)) / 2 | ||
| return offset, nil | ||
| } | ||
| return 0, errNoValidNTPServer | ||
| } |
There was a problem hiding this comment.
The new CheckNTPOffset function (lines 28-91) is a public API function but lacks test coverage. Given that this is a critical function that validates system time synchronization and is called during engine startup, it should have unit tests covering both success and failure scenarios (e.g., no pools configured, connection failures, valid offset calculation).
| func (m *ntpManager) getTimeOffset(ctx context.Context) (time.Duration, error) { | ||
| dialer := &net.Dialer{ | ||
| Timeout: ntpDialTimeout, | ||
| } | ||
|
|
||
| for i := range m.pools { | ||
| con, err := net.DialTimeout("udp", m.pools[i], 5*time.Second) //nolint:noctx // TODO: #2006 Use (*net.Dialer).DialContext with (*net.Dialer).Timeout | ||
| conn, err := dialer.DialContext(ctx, "udp", m.pools[i]) | ||
| if err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to connect to hosts %v attempting next", m.pools[i]) | ||
| log.Warnf(log.TimeMgr, "Unable to connect to hosts %v attempting to next", m.pools[i]) | ||
| continue | ||
| } | ||
|
|
||
| if err = con.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to SetDeadline. Error: %s\n", err) | ||
| err = con.Close() | ||
| if err != nil { | ||
| if err = conn.SetDeadline(time.Now().Add(ntpReadWriteTimeout)); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to set deadline on hosts %v. Error %s. attempting to next\n", m.pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // T1: Record time before sending request (origin timestamp) | ||
| t1 := time.Now() | ||
|
|
||
| req := &ntpPacket{Settings: 0x1B} | ||
| if err = binary.Write(con, binary.BigEndian, req); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to write. Error: %s\n", err) | ||
| err = con.Close() | ||
| if err != nil { | ||
| if err = binary.Write(conn, binary.BigEndian, req); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to write to hosts %v. Error %s. Attempting to next\n", m.pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| rsp := &ntpPacket{} | ||
| if err = binary.Read(con, binary.BigEndian, rsp); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to read. Error: %s\n", err) | ||
| err = con.Close() | ||
| if err != nil { | ||
| if err = binary.Read(conn, binary.BigEndian, rsp); err != nil { | ||
| log.Warnf(log.TimeMgr, "Unable to read from hosts %v. Error: %s. Attempting to next\n", m.pools[i], err) | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| secs := float64(rsp.TxTimeSec) - 2208988800 | ||
| nanos := (int64(rsp.TxTimeFrac) * 1e9) >> 32 | ||
| // T4L Record time after receiving response (Destination timestamp) | ||
| t4 := time.Now() | ||
|
|
||
| err = con.Close() | ||
| if err != nil { | ||
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } | ||
| return time.Unix(int64(secs), nanos) | ||
|
|
||
| // T2: Server receive timestamp (when server received our request) | ||
| t2 := ntpTimestampToTime(rsp.RxTimeSec, rsp.RxTimeFrac) | ||
| // T3: Server transmit timestamp (when server sent our response) | ||
| t3 := ntpTimestampToTime(rsp.TxTimeSec, rsp.TxTimeFrac) | ||
|
|
||
| // RFC 5905 offset calculation: ((T2-T1) + (T3-T4)) / 2 | ||
| // This formula cancels out the network round-trip time | ||
| offset := (t2.Sub(t1) + t3.Sub(t4)) / 2 | ||
| return offset, nil | ||
| } | ||
| log.Warnln(log.TimeMgr, "No valid NTP servers found, using current system time") | ||
| return time.Now().UTC() | ||
| log.Warnln(log.TimeMgr, "No valid NTP servers found") | ||
| return 0, errNoValidNTPServer | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between CheckNTPOffset (lines 28-91) and getTimeOffset (lines 240-300). Both functions perform essentially the same NTP query logic with only minor differences in logging messages. Consider extracting the common NTP query logic into a shared helper function to improve maintainability and reduce duplication.
| func ntpTimestampToTime(seconds, fractional uint32) time.Time { | ||
| unixSeconds := int64(seconds) - ntpEpochOffset | ||
| nanos := (int64(fractional) * 1.e9) >> 32 | ||
| return time.Unix(unixSeconds, nanos) | ||
| } |
There was a problem hiding this comment.
The new ntpTimestampToTime helper function (lines 94-98) lacks test coverage. Given that this function performs time conversion calculations that are critical for accurate NTP offset computation, it should have unit tests to verify correctness of the conversion from NTP timestamp format (seconds + fractional seconds) to time.Time.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2137 +/- ##
==========================================
- Coverage 42.09% 42.05% -0.05%
==========================================
Files 444 444
Lines 143153 143201 +48
==========================================
- Hits 60267 60227 -40
- Misses 75758 75841 +83
- Partials 7128 7133 +5
🚀 New features to boost your workflow:
|
|
Nice man, getting e-jumped by AI these days |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
thrasher-
requested review from
cranktakular,
gloriousCode,
samuael and
shazbert
|
please let me know if its ok, will do the things gemini suggested |
shazbert
left a comment
shazbert
left a comment
There was a problem hiding this comment.
Just did a really brief check of this, tests are failing and some minor critiques I will need to go into the weeds fully to understand what is going on.
| } | ||
|
|
||
| // T1: Record time before sending request (origin timestamp) | ||
| t1 := time.Now() |
There was a problem hiding this comment.
change variable names so we can remove commentary. t1 to origin
| ntpDialTimeout = 5 * time.Second | ||
| ntpReadWriteTimeout = 5 * time.Second |
There was a problem hiding this comment.
suggestion: You can combine these as ntpDefaultTimeout
| dialer := &net.Dialer{ | ||
| Timeout: ntpDialTimeout, | ||
| } | ||
|
|
There was a problem hiding this comment.
| dialer := &net.Dialer{ | |
| Timeout: ntpDialTimeout, | |
| } | |
| dialer := &net.Dialer{Timeout: ntpDialTimeout} |
Squishy squashy
| for i := range pools { | ||
| conn, err := dialer.DialContext(ctx, "udp", pools[i]) | ||
| if err != nil { | ||
| log.Warnf(log.TimeMgr, "NTP check: Unable to connect to %v, attempting next", pools[i]) |
There was a problem hiding this comment.
| log.Warnf(log.TimeMgr, "NTP check: Unable to connect to %v, attempting next", pools[i]) | |
| log.Warnf(log.TimeMgr, "NTP check: Unable to connect to %q, attempting next", pools[i]) |
| continue | ||
| } | ||
|
|
||
| if err = conn.SetDeadline(time.Now().Add(ntpReadWriteTimeout)); err != nil { |
There was a problem hiding this comment.
| if err = conn.SetDeadline(time.Now().Add(ntpReadWriteTimeout)); err != nil { | |
| if err := conn.SetDeadline(time.Now().Add(ntpReadWriteTimeout)); err != nil { |
| // CheckNTPOffset performs a one-time NTP check and returns the time offset. | ||
| // This can be called before the NTP manager is started to verify time sync. | ||
| // It uses the RFC 5905 formula: offset = ((T2-T1) + (T3-T4)) / 2 | ||
| func CheckNTPOffset(ctx context.Context, pools []string) (time.Duration, error) { |
There was a problem hiding this comment.
This needs a dedicated test
| continue | ||
| } | ||
|
|
||
| rsp := &ntpPacket{} |
There was a problem hiding this comment.
This might need some packet validation to make sure its correct. Stratum not zero, the mode is correct, and the TxTime txFractions are not zero.
| if err = conn.Close(); err != nil { | ||
| log.Errorln(log.TimeMgr, err) | ||
| } |
There was a problem hiding this comment.
suggestion: You can always have a sub func then just defer this close which saves on the error pathways, then in the loop just have an error check and continue and/or return value, might be useful for getTimeOffset as well to consolidate some code. Up to you.
| } | ||
|
|
||
| // T4: Record time after receiving response (Destination timestamp) | ||
| t4 := time.Now() |
| } | ||
|
|
||
| // ntpTimestampToTime converts timestamp (seconds and fractional) to time.Time | ||
| func ntpTimestampToTime(seconds, fractional uint32) time.Time { |
There was a problem hiding this comment.
This needs a dedicated test
|
@codex please review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Critiques are good! exactly what I was looking for. Yeah, I remember writing this PR when I was sick but had to know if the concept idea was in the right direction. Will take another attempt by trying to refine this |
3 participants
PR implement RFC 5905 offset calculation and verify time before warning
Before I got
No, the issue is not my system/setup
The 4 timestamps
Original code
The issue
T3 - T4gives us-(network_delay) + offsetIf my clock is perfect (offset = 0), I still get
-50msdue to networkThe current code asks: "Bonjour, what time did the server say it was?"
But that's stale information (packet took time to travel)
By the time you read
T3 = 10:00:00.050, it's already10:00:00.100on your clock. You can't just compare them directly.RFC 5905 compensates for network delay/latency
Where:
RxTimeSec/RxTimeFrac)TxTimeSec/TxTimeFrac)This formula cancels out the network round-trip time, giving an accurate offset measurement.
Also worth to mention
Even when the calculation was fixed, I noticed it didn't actually actually check if the time is out of sync. It seemed like it assumed and asked the user how they want to be notified in the future.
Also
Migrated from net.DialTimeout to net.Dialer.DialContext
Anyways after these changes it all worked. Just gonna do the pull request now (can't wait to do 10 more fixes because the automated tests are gonna complain
Sources to confirm:
https://datatracker.ietf.org/doc/html/rfc5905
GuerrillaNtp https://github.com/robertvazan/guerrillantp
GuerrillaNtp documentation:
{ "Response": { "OriginateTimestamp": "2023-10-27T10:30:00.0000000Z", // T1 "ReceiveTimestamp": "2023-10-27T10:30:00.0100000Z", // T2 "TransmitTimestamp": "2023-10-27T10:30:00.0150000Z" // T3 }, "CorrectionOffset": "00:00:00.0051234", // Calculated using RFC formula "RoundTripTime": "00:00:00.0258765" // (T4-T1) - (T3-T2) }Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.
Checklist