Merge pull request #1213 from input-output-hk/fix/ddw-518-improve-ntp…

…-check-handling-on-system-wake-up-event [DDW-518, DDW-519, DDW-520, DDW-521] Improve NTP check handling on system wake-up event
input-output-hk · Dec 10, 2018 · 84c180b · 84c180b
2 parents 1f615ed + 68c6434
commit 84c180b
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 47 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,7 +17,7 @@ Changelog
 - Implemented support for Cardano node "structured logging" ([PR 1092](https://github.com/input-output-hk/daedalus/pull/1092), [PR 1122](https://github.com/input-output-hk/daedalus/pull/1122))
 - Implemented the IPC driven Cardano node / Daedalus communication ([PR 1075](https://github.com/input-output-hk/daedalus/pull/1075), [PR 1107](https://github.com/input-output-hk/daedalus/pull/1107), [PR 1109](https://github.com/input-output-hk/daedalus/pull/1109), [PR 1115](https://github.com/input-output-hk/daedalus/pull/1115), [PR 1118](https://github.com/input-output-hk/daedalus/pull/1118), [PR 1119](https://github.com/input-output-hk/daedalus/pull/1119), [PR 1162](https://github.com/input-output-hk/daedalus/pull/1162))
 - Improved the loading UX ([PR 723](https://github.com/input-output-hk/daedalus/pull/723))
-- Improved the NTP check handling ([PR 1086](https://github.com/input-output-hk/daedalus/pull/1086), [PR 1149](https://github.com/input-output-hk/daedalus/pull/1149), [PR 1158](https://github.com/input-output-hk/daedalus/pull/1158), [PR 1194](https://github.com/input-output-hk/daedalus/pull/1194))
+- Improved the NTP check handling ([PR 1086](https://github.com/input-output-hk/daedalus/pull/1086), [PR 1149](https://github.com/input-output-hk/daedalus/pull/1149), [PR 1158](https://github.com/input-output-hk/daedalus/pull/1158), [PR 1194](https://github.com/input-output-hk/daedalus/pull/1194), [PR 1213](https://github.com/input-output-hk/daedalus/pull/1213))
 - Improved the transaction details text selection ([PR 1073](https://github.com/input-output-hk/daedalus/pull/1073), [PR 1095](https://github.com/input-output-hk/daedalus/pull/1095))
 - Integrated Cardano V1 API endpoints ([PR 1018](https://github.com/input-output-hk/daedalus/pull/1018), [PR 1031](https://github.com/input-output-hk/daedalus/pull/1031), [PR 1037](https://github.com/input-output-hk/daedalus/pull/1037), [PR 1042](https://github.com/input-output-hk/daedalus/pull/1042), [PR 1045](https://github.com/input-output-hk/daedalus/pull/1045), [PR 1070](https://github.com/input-output-hk/daedalus/pull/1070), [PR 1078](https://github.com/input-output-hk/daedalus/pull/1078), [PR 1079](https://github.com/input-output-hk/daedalus/pull/1079), [PR 1080](https://github.com/input-output-hk/daedalus/pull/1080), [PR 1088](https://github.com/input-output-hk/daedalus/pull/1088))
 - Refactored and improved `NetworkStatus` store to use V1 API data ([PR 1081](https://github.com/input-output-hk/daedalus/pull/1081))

diff --git a/source/renderer/app/components/status/NetworkStatus.js b/source/renderer/app/components/status/NetworkStatus.js
@@ -41,7 +41,6 @@ type Props = {
   isSystemTimeIgnored: boolean,
   isSystemTimeCorrect: boolean,
   isForceCheckingNodeTime: boolean,
-  isSystemTimeChanged: boolean,
   mostRecentBlockTimestamp: number,
   localBlockHeight: number,
   networkBlockHeight: number,
@@ -119,7 +118,7 @@ export default class NetworkStatus extends Component<Props, State> {
       cardanoNodeState, isNodeResponding, isNodeSubscribed, isNodeSyncing, isNodeInSync,
       isNodeTimeCorrect, isConnected, isSynced, syncPercentage, hasBeenConnected,
       localTimeDifference, isSystemTimeCorrect, isForceCheckingNodeTime,
-      isSystemTimeChanged, mostRecentBlockTimestamp, localBlockHeight, networkBlockHeight,
+      mostRecentBlockTimestamp, localBlockHeight, networkBlockHeight,
       onForceCheckLocalTimeDifference, onClose, nodeConnectionError, isSystemTimeIgnored,
       onOpenExternalLink,
     } = this.props;
@@ -234,12 +233,6 @@ export default class NetworkStatus extends Component<Props, State> {
                   {isSystemTimeIgnored ? 'YES' : 'NO'}
                 </td>
               </tr>
-              <tr>
-                <td>isSystemTimeChanged:</td>
-                <td className={this.getClass(!isSystemTimeChanged)}>
-                  {isSystemTimeChanged ? 'YES' : 'NO'}
-                </td>
-              </tr>
               <tr>
                 <td>isForceCheckingNodeTime:</td>
                 <td>

diff --git a/source/renderer/app/config/timingConfig.js b/source/renderer/app/config/timingConfig.js
@@ -10,4 +10,4 @@ export const ALLOWED_TIME_DIFFERENCE = 15 * 1000000; // 15 seconds | unit: micro
 export const MAX_ALLOWED_STALL_DURATION = 2 * 60 * 1000; // 2 minutes | unit: milliseconds
 export const NETWORK_STATUS_REQUEST_TIMEOUT = 30 * 1000; // 30 seconds | unit: milliseconds
 export const NETWORK_STATUS_POLL_INTERVAL = 2000; // 2 seconds | unit: milliseconds
-export const SYSTEM_TIME_POLL_INTERVAL = 1000; // 1 second | unit: milliseconds
+export const NTP_FORCE_CHECK_POLL_INTERVAL = 30 * 60 * 1000; // 30 minutes | unit: milliseconds
diff --git a/source/renderer/app/containers/status/NetworkStatusPage.js b/source/renderer/app/containers/status/NetworkStatusPage.js
@@ -24,7 +24,7 @@ export default class NetworkStatusPage extends Component<InjectedProps> {
       // Application state
       isConnected, isSynced, syncPercentage, hasBeenConnected,
       localTimeDifference, isSystemTimeCorrect, forceCheckTimeDifferenceRequest,
-      forceCheckLocalTimeDifference, isSystemTimeChanged, getNetworkStatusRequest,
+      forceCheckLocalTimeDifference, getNetworkStatusRequest,
       localBlockHeight, networkBlockHeight, mostRecentBlockTimestamp, restartNode,
       isSystemTimeIgnored,
     } = stores.networkStatus;
@@ -45,7 +45,6 @@ export default class NetworkStatusPage extends Component<InjectedProps> {
           isSystemTimeCorrect={isSystemTimeCorrect}
           isForceCheckingNodeTime={forceCheckTimeDifferenceRequest.isExecuting}
           isSystemTimeIgnored={isSystemTimeIgnored}
-          isSystemTimeChanged={isSystemTimeChanged}
           mostRecentBlockTimestamp={mostRecentBlockTimestamp}
           nodeConnectionError={
             getNetworkStatusRequest.error || forceCheckTimeDifferenceRequest.error

diff --git a/source/renderer/app/stores/NetworkStatusStore.js b/source/renderer/app/stores/NetworkStatusStore.js
@@ -9,7 +9,7 @@ import {
   MAX_ALLOWED_STALL_DURATION,
   NETWORK_STATUS_REQUEST_TIMEOUT,
   NETWORK_STATUS_POLL_INTERVAL,
-  SYSTEM_TIME_POLL_INTERVAL,
+  NTP_FORCE_CHECK_POLL_INTERVAL,
 } from '../config/timingConfig';
 import { UNSYNCED_BLOCKS_ALLOWED } from '../config/numbersConfig';
 import { Logger } from '../../../common/logging';
@@ -39,10 +39,9 @@ export default class NetworkStatusStore extends Store {
   // Initialize store properties
   _startTime = Date.now();
   _tlsConfig: ?TlsConfig = null;
-  _systemTime = Date.now();
   _networkStatus = NETWORK_STATUS.CONNECTING;
   _networkStatusPollingInterval: ?number = null;
-  _systemTimeChangeCheckPollingInterval: ?number = null;
+  _forceCheckTimeDifferencePollingInterval: ?number = null;
 
   // Initialize store observables
 
@@ -64,7 +63,6 @@ export default class NetworkStatusStore extends Store {
   @observable mostRecentBlockTimestamp = 0; // milliseconds
   @observable localTimeDifference: ?number = 0; // microseconds
   @observable isSystemTimeIgnored = false; // Tracks if NTP time checks are ignored
-  @observable isSystemTimeChanged = false; // Tracks system time change event
   @observable getNetworkStatusRequest: Request<GetNetworkStatusResponse> = new Request(
     this.api.ada.getNetworkStatus
   );
@@ -86,17 +84,16 @@ export default class NetworkStatusStore extends Store {
 
     this.registerReactions([
       this._updateNetworkStatusWhenDisconnected,
-      this._updateLocalTimeDifferenceWhenSystemTimeChanged,
     ]);
 
     // Setup network status polling interval
     this._networkStatusPollingInterval = setInterval(
       this._updateNetworkStatus, NETWORK_STATUS_POLL_INTERVAL
     );
 
-    // Setup system time change polling interval
-    this._systemTimeChangeCheckPollingInterval = setInterval(
-      this._updateSystemTime, SYSTEM_TIME_POLL_INTERVAL
+    // Forced time difference check polling interval
+    this._forceCheckTimeDifferencePollingInterval = setInterval(
+      this._forceCheckTimeDifference, NTP_FORCE_CHECK_POLL_INTERVAL
     );
   }
 
@@ -116,8 +113,8 @@ export default class NetworkStatusStore extends Store {
     if (this._networkStatusPollingInterval) {
       clearInterval(this._networkStatusPollingInterval);
     }
-    if (this._systemTimeChangeCheckPollingInterval) {
-      clearInterval(this._systemTimeChangeCheckPollingInterval);
+    if (this._forceCheckTimeDifferencePollingInterval) {
+      clearInterval(this._forceCheckTimeDifferencePollingInterval);
     }
 
     // Save current state into the cache
@@ -128,15 +125,8 @@ export default class NetworkStatusStore extends Store {
     };
   }
 
-  _updateNetworkStatusWhenDisconnected = async () => {
-    if (!this.isConnected) await this._updateNetworkStatus();
-  };
-
-  _updateLocalTimeDifferenceWhenSystemTimeChanged = async () => {
-    if (this.isSystemTimeChanged) {
-      Logger.debug('System time change detected');
-      await this._updateNetworkStatus({ force_ntp_check: true });
-    }
+  _updateNetworkStatusWhenDisconnected = () => {
+    if (!this.isConnected) this._updateNetworkStatus();
   };
 
   _getStartupTimeDelta() {
@@ -178,30 +168,28 @@ export default class NetworkStatusStore extends Store {
     switch (state) {
       case CardanoNodeStates.STARTING: break;
       case CardanoNodeStates.RUNNING: this._requestTlsConfig(); break;
+      case CardanoNodeStates.STOPPING:
+      case CardanoNodeStates.EXITING:
+      case CardanoNodeStates.UPDATING:
+        runInAction('reset _tlsConfig', () => this._tlsConfig = null);
+        this._setDisconnected(wasConnected);
+        break;
       default: this._setDisconnected(wasConnected);
     }
-    runInAction('setting cardanoNodeState', () => {
-      this.cardanoNodeState = state;
-    });
+    runInAction('setting cardanoNodeState', () => this.cardanoNodeState = state);
     return Promise.resolve();
   };
 
+  _forceCheckTimeDifference = () => {
+    this._updateNetworkStatus({ force_ntp_check: true });
+  };
+
   // DEFINE ACTIONS
   @action initialize() {
     super.initialize();
     if (cachedState !== null) Object.assign(this, cachedState);
   }
 
-  @action _updateSystemTime = () => {
-    // In order to detect system time changes (e.g. user updates machine's date/time)
-    // we are checking current system time and comparing the difference to the last known value.
-    // If the difference is larger than the polling interval plus a safety margin of 100%
-    // we can be sure the user has update the time.
-    const systemTimeDifference = Math.abs(moment(Date.now()).diff(moment(this._systemTime)));
-    this.isSystemTimeChanged = Math.floor(systemTimeDifference / SYSTEM_TIME_POLL_INTERVAL) > 1;
-    this._systemTime = Date.now();
-  };
-
   @action _updateNetworkStatus = async (queryParams?: NodeQueryParams) => {
     // In case we haven't received TLS config we shouldn't trigger any API calls
     if (!this._tlsConfig) return;
@@ -222,7 +210,9 @@ export default class NetworkStatusStore extends Store {
     if (isForcedTimeDifferenceCheck) {
       // Set most recent block timestamp into the future as a guard
       // against system time changes - e.g. if system time was set into the past
-      this.mostRecentBlockTimestamp = Date.now() + NETWORK_STATUS_REQUEST_TIMEOUT;
+      runInAction('update mostRecentBlockTimestamp', () => {
+        this.mostRecentBlockTimestamp = Date.now() + NETWORK_STATUS_REQUEST_TIMEOUT;
+      });
     }
 
     // Record connection status before running network status call
@@ -236,6 +226,13 @@ export default class NetworkStatusStore extends Store {
         networkStatus = await this.getNetworkStatusRequest.execute().promise;
       }
 
+      // In case we no longer have TLS config we ignore all API call responses
+      // as this means we are in the Cardano shutdown (stopping|exiting|updating) sequence
+      if (!this._tlsConfig) {
+        Logger.debug('Ignoring NetworkStatusRequest result during Cardano shutdown sequence...');
+        return;
+      }
+
       const {
         subscriptionStatus,
         syncProgress,
@@ -363,12 +360,10 @@ export default class NetworkStatusStore extends Store {
   };
 
   @action _setDisconnected = (wasConnected: boolean) => {
-    this.cardanoNodeState = null;
     this.isNodeResponding = false;
     this.isNodeSubscribed = false;
     this.isNodeSyncing = false;
     this.isNodeInSync = false;
-    this._tlsConfig = null;
     if (wasConnected) {
       if (!this.hasBeenConnected) {
         runInAction('update hasBeenConnected', () => this.hasBeenConnected = true);