fix(dapi): truthful state sync status, health height field collision, client crash on absent sections

packages/js-dapi-client/lib/methods/platform/getStatus/GetStatusResponse.js

@@ -5,14 +5,32 @@ const TimeStatus = require('./TimeStatus');
 const StateSyncStatus = require('./StateSyncStatus');
 const NetworkStatus = require('./NetworkStatus');
 
+/**
+ * Read a value out of an optional protobuf sub-message.
+ *
+ * Every message-typed field in `GetStatusResponseV0` is optional on the wire, and DAPI
+ * omits whole sections when the underlying data is unavailable: `state_sync` is absent
+ * unless the node is state syncing, `chain`/`node`/`network` are absent when Tenderdash
+ * is unreachable, and `protocol.drive` is absent when Drive is unreachable. The generated
+ * getters return `undefined` for an absent sub-message, so they must never be chained
+ * without a guard.
+ *
+ * @param {object|undefined} message - protobuf sub-message, possibly absent
+ * @param {function(object): *} read - reader invoked when the sub-message is present
+ * @returns {*} the read value, or `undefined` when the sub-message is absent
+ */
+function readOptional(message, read) {
+  return message ? read(message) : undefined;
+}
+
 class GetStatusResponse {
   /**
    * @param {VersionStatus} version - status versions
-   * @param {NodeStatus} node - node status
-   * @param {ChainStatus} chain - chain status
-   * @param {NetworkStatus} network - network status
-   * @param {StateSyncStatus} stateSync - state sync status
-   * @param {TimeStatus} time - time status
+   * @param {NodeStatus|null} node - node status, null if unavailable
+   * @param {ChainStatus|null} chain - chain status, null if unavailable
+   * @param {NetworkStatus|null} network - network status, null if unavailable
+   * @param {StateSyncStatus|null} stateSync - state sync status, null if not state syncing
+   * @param {TimeStatus|null} time - time status, null if unavailable
    */
   constructor(version, node, chain, network, stateSync, time) {
     this.version = version;
@@ -31,35 +49,35 @@ class GetStatusResponse {
   }
 
   /**
-   * @returns {NodeStatus} node info status
+   * @returns {NodeStatus|null} node info status, null if unavailable
    */
   getNodeStatus() {
     return this.node;
   }
 
   /**
-   * @returns {ChainStatus} chain status
+   * @returns {ChainStatus|null} chain status, null if unavailable
    */
   getChainStatus() {
     return this.chain;
   }
 
   /**
-   * @returns {NetworkStatus} network status
+   * @returns {NetworkStatus|null} network status, null if unavailable
    */
   getNetworkStatus() {
     return this.network;
   }
 
   /**
-   * @returns {StateSyncStatus} state sync status
+   * @returns {StateSyncStatus|null} state sync status, null if not state syncing
    */
   getStateSyncStatus() {
     return this.stateSync;
   }
 
   /**
-   * @returns {TimeStatus} time status
+   * @returns {TimeStatus|null} time status, null if unavailable
    */
   getTimeStatus() {
     return this.time;
@@ -72,85 +90,75 @@ class GetStatusResponse {
   static createFromProto(proto) {
     const v0 = proto.getV0();
 
-    const dapiVersion = v0.getVersion().getSoftware().getDapi();
-    const driveVersion = v0.getVersion().getSoftware().getDrive();
-    const tenderdashVersion = v0.getVersion().getSoftware().getTenderdash();
-    const tenderdashP2pProtocol = v0.getVersion().getProtocol().getTenderdash().getP2p();
-    const tenderdashBlockProtocol = v0.getVersion().getProtocol().getTenderdash().getBlock();
-    const driveCurrentProtocol = v0.getVersion().getProtocol().getDrive().getCurrent();
-    const driveLatestProtocol = v0.getVersion().getProtocol().getDrive().getLatest();
-    const driveNextEpochProtocol = v0.getVersion().getProtocol().getDrive().getNextEpoch();
+    const versionProto = v0.getVersion();
+    const softwareProto = readOptional(versionProto, (v) => v.getSoftware());
+    const protocolProto = readOptional(versionProto, (v) => v.getProtocol());
+    const tenderdashProtocolProto = readOptional(protocolProto, (p) => p.getTenderdash());
+    const driveProtocolProto = readOptional(protocolProto, (p) => p.getDrive());
 
     const version = new VersionStatus(
-      dapiVersion,
-      driveVersion,
-      tenderdashVersion,
-      tenderdashP2pProtocol,
-      tenderdashBlockProtocol,
-      driveCurrentProtocol,
-      driveLatestProtocol,
-      driveNextEpochProtocol,
-    );
-
-    const nodeId = Buffer.from(v0.getNode().getId()).toString('hex');
-    const proTxHash = Buffer.from(v0.getNode().getProTxHash()).toString('hex');
-
-    const node = new NodeStatus(nodeId, proTxHash);
-
-    const catchingUp = v0.getChain().getCatchingUp();
-    const latestBlockHash = Buffer.from(v0.getChain().getLatestBlockHash()).toString('hex');
-    const latestAppHash = Buffer.from(v0.getChain().getLatestAppHash()).toString('hex');
-    const latestBlockHeight = BigInt(v0.getChain().getLatestBlockHeight());
-    const earliestBlockHash = Buffer.from(v0.getChain().getEarliestBlockHash()).toString('hex');
-    const earliestAppHash = Buffer.from(v0.getChain().getEarliestAppHash()).toString('hex');
-    const earliestBlockHeight = BigInt(v0.getChain().getEarliestBlockHeight());
-    const maxPeerBlockHeight = BigInt(v0.getChain().getMaxPeerBlockHeight());
-    const coreChainLockedHeight = v0.getChain().getCoreChainLockedHeight();
-
-    const chain = new ChainStatus(
-      catchingUp,
-      latestBlockHash,
-      latestAppHash,
-      latestBlockHeight,
-      earliestBlockHash,
-      earliestAppHash,
-      earliestBlockHeight,
-      maxPeerBlockHeight,
-      coreChainLockedHeight,
+      readOptional(softwareProto, (s) => s.getDapi()),
+      readOptional(softwareProto, (s) => s.getDrive()),
+      readOptional(softwareProto, (s) => s.getTenderdash()),
+      readOptional(tenderdashProtocolProto, (t) => t.getP2p()),
+      readOptional(tenderdashProtocolProto, (t) => t.getBlock()),
+      readOptional(driveProtocolProto, (d) => d.getCurrent()),
+      readOptional(driveProtocolProto, (d) => d.getLatest()),
+      readOptional(driveProtocolProto, (d) => d.getNextEpoch()),
     );
 
-    const chainId = v0.getNetwork().getChainId();
-    const peersCount = v0.getNetwork().getPeersCount();
-    const isListening = v0.getNetwork().getListening();
-
-    const network = new NetworkStatus(chainId, peersCount, isListening);
-
-    const totalSyncedTime = BigInt(v0.getStateSync().getTotalSyncedTime());
-    const remainingTime = BigInt(v0.getStateSync().getRemainingTime());
-    const totalSnapshots = v0.getStateSync().getTotalSnapshots();
-    const chunkProcessAverageTime = BigInt(v0.getStateSync().getChunkProcessAvgTime());
-    const snapshotHeight = BigInt(v0.getStateSync().getSnapshotHeight());
-    const snapshotChunksCount = BigInt(v0.getStateSync().getSnapshotChunksCount());
-    const backfilledBlocks = BigInt(v0.getStateSync().getBackfilledBlocks());
-    const backfillBlocksTotal = BigInt(v0.getStateSync().getBackfillBlocksTotal());
-
-    const stateSync = new StateSyncStatus(
-      totalSyncedTime,
-      remainingTime,
-      totalSnapshots,
-      chunkProcessAverageTime,
-      snapshotHeight,
-      snapshotChunksCount,
-      backfilledBlocks,
-      backfillBlocksTotal,
-    );
-
-    const local = BigInt(v0.getTime().getLocal());
-    const block = BigInt(v0.getTime().getBlock());
-    const genesis = BigInt(v0.getTime().getGenesis());
-    const epoch = v0.getTime().getEpoch();
-
-    const time = new TimeStatus(local, block, genesis, epoch);
+    const nodeProto = v0.getNode();
+
+    const node = nodeProto ? new NodeStatus(
+      Buffer.from(nodeProto.getId()).toString('hex'),
+      Buffer.from(nodeProto.getProTxHash()).toString('hex'),
+    ) : null;
+
+    const chainProto = v0.getChain();
+
+    const chain = chainProto ? new ChainStatus(
+      chainProto.getCatchingUp(),
+      Buffer.from(chainProto.getLatestBlockHash()).toString('hex'),
+      Buffer.from(chainProto.getLatestAppHash()).toString('hex'),
+      BigInt(chainProto.getLatestBlockHeight()),
+      Buffer.from(chainProto.getEarliestBlockHash()).toString('hex'),
+      Buffer.from(chainProto.getEarliestAppHash()).toString('hex'),
+      BigInt(chainProto.getEarliestBlockHeight()),
+      BigInt(chainProto.getMaxPeerBlockHeight()),
+      chainProto.getCoreChainLockedHeight(),
+    ) : null;
+
+    const networkProto = v0.getNetwork();
+
+    const network = networkProto ? new NetworkStatus(
+      networkProto.getChainId(),
+      networkProto.getPeersCount(),
+      networkProto.getListening(),
+    ) : null;
+
+    // DAPI omits the whole state sync section on nodes that are not state syncing,
+    // so an absent section means "no state sync information", not "all zeroes".
+    const stateSyncProto = v0.getStateSync();
+
+    const stateSync = stateSyncProto ? new StateSyncStatus(
+      BigInt(stateSyncProto.getTotalSyncedTime()),
+      BigInt(stateSyncProto.getRemainingTime()),
+      stateSyncProto.getTotalSnapshots(),
+      BigInt(stateSyncProto.getChunkProcessAvgTime()),
+      BigInt(stateSyncProto.getSnapshotHeight()),
+      BigInt(stateSyncProto.getSnapshotChunksCount()),
+      BigInt(stateSyncProto.getBackfilledBlocks()),
+      BigInt(stateSyncProto.getBackfillBlocksTotal()),
+    ) : null;
+
+    const timeProto = v0.getTime();
+
+    const time = timeProto ? new TimeStatus(
+      BigInt(timeProto.getLocal()),
+      BigInt(timeProto.getBlock()),
+      BigInt(timeProto.getGenesis()),
+      timeProto.getEpoch(),
+    ) : null;
 
     return new GetStatusResponse(
       version,

packages/js-dapi-client/test/unit/methods/platform/getStatus/GetStatusResponse.spec.js

@@ -349,4 +349,51 @@ describe('GetStatusResponse', () => {
     expect(timeStatus.getGenesisTime()).to.equal(BigInt(statusFixture.time.genesis));
     expect(timeStatus.getEpochNumber()).to.equal(statusFixture.time.epoch);
   });
+
+  it('should create an instance from proto without state sync section', () => {
+    // DAPI omits the state sync section on nodes that never state synced
+    proto.getV0().clearStateSync();
+
+    getStatusResponse = GetStatusResponseClass.createFromProto(proto);
+
+    expect(getStatusResponse.getStateSyncStatus()).to.equal(null);
+
+    // the rest of the response must still be parsed
+    expect(getStatusResponse.getChainStatus()).to.be.an.instanceOf(ChainStatus);
+    expect(getStatusResponse.getChainStatus().getLatestBlockHeight())
+      .to.equal(BigInt(statusFixture.chain.latestBlockHeight));
+    expect(getStatusResponse.getVersionStatus()).to.be.an.instanceOf(VersionStatus);
+    expect(getStatusResponse.getTimeStatus()).to.be.an.instanceOf(TimeStatus);
+  });
+
+  it('should create an instance from proto with every optional section absent', () => {
+    const v0 = proto.getV0();
+
+    v0.clearNode();
+    v0.clearChain();
+    v0.clearNetwork();
+    v0.clearStateSync();
+    v0.getVersion().getProtocol().clearDrive();
+    v0.getVersion().getProtocol().clearTenderdash();
+    v0.getVersion().clearSoftware();
+
+    getStatusResponse = GetStatusResponseClass.createFromProto(proto);
+
+    expect(getStatusResponse.getNodeStatus()).to.equal(null);
+    expect(getStatusResponse.getChainStatus()).to.equal(null);
+    expect(getStatusResponse.getNetworkStatus()).to.equal(null);
+    expect(getStatusResponse.getStateSyncStatus()).to.equal(null);
+
+    const versionStatus = getStatusResponse.getVersionStatus();
+    expect(versionStatus).to.be.an.instanceOf(VersionStatus);
+    expect(versionStatus.getDapiVersion()).to.equal(undefined);
+    expect(versionStatus.getDriveVersion()).to.be.null();
+    expect(versionStatus.getTenderdashVersion()).to.be.null();
+    expect(versionStatus.getDriveCurrentProtocol()).to.equal(undefined);
+    expect(versionStatus.getTenderdashP2pProtocol()).to.equal(undefined);
+
+    const timeStatus = getStatusResponse.getTimeStatus();
+    expect(timeStatus).to.be.an.instanceOf(TimeStatus);
+    expect(timeStatus.getLocalTime()).to.equal(BigInt(statusFixture.time.local));
+  });
 });

packages/rs-dapi/src/server/metrics.rs

@@ -112,7 +112,7 @@ async fn handle_health(State(state): State<MetricsAppState>) -> impl axum::respo
             true,
             CoreRpcCheck {
                 status: "ok".into(),
-                latest_block_height: Some(height),
+                core_block_height: Some(height),
                 error: None,
             },
         ),
@@ -122,7 +122,7 @@ async fn handle_health(State(state): State<MetricsAppState>) -> impl axum::respo
                 false,
                 CoreRpcCheck {
                     status: "error".into(),
-                    latest_block_height: None,
+                    core_block_height: None,
                     error: Some(health_error_label(&err).to_string()),
                 },
             )
@@ -131,7 +131,7 @@ async fn handle_health(State(state): State<MetricsAppState>) -> impl axum::respo
             false,
             CoreRpcCheck {
                 status: "error".into(),
-                latest_block_height: None,
+                core_block_height: None,
                 error: Some("timeout".into()),
             },
         ),
@@ -213,8 +213,13 @@ struct PlatformChecks {
 #[derive(Serialize)]
 struct CoreRpcCheck {
     status: String,
-    #[serde(rename = "latestBlockHeight", skip_serializing_if = "Option::is_none")]
-    latest_block_height: Option<u32>,
+    /// Dash Core chain height from `getblockcount`.
+    ///
+    /// Deliberately not called `latestBlockHeight`: that name is the *Platform* chain height
+    /// in `getStatus`'s `chain` section, and reporting a Core height under the same name here
+    /// has been mistaken for a wildly-ahead Platform height on freshly joined nodes.
+    #[serde(rename = "coreBlockHeight", skip_serializing_if = "Option::is_none")]
+    core_block_height: Option<u32>,
     #[serde(skip_serializing_if = "Option::is_none")]
     error: Option<String>,
 }
@@ -304,3 +309,24 @@ impl From<bool> for ComponentCheck {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// The Core RPC health check reports a Dash Core height. Naming it `latestBlockHeight`
+    /// collides with the Platform height of the same name in `getStatus`'s `chain` section,
+    /// and operators have read the Core height as a Platform height because of it.
+    #[test]
+    fn core_rpc_health_check_does_not_reuse_the_platform_height_field_name() {
+        let json = serde_json::to_string(&CoreRpcCheck {
+            status: "ok".into(),
+            core_block_height: Some(8036),
+            error: None,
+        })
+        .expect("serialize core rpc check");
+
+        assert!(json.contains(r#""coreBlockHeight":8036"#), "{json}");
+        assert!(!json.contains("latestBlockHeight"), "{json}");
+    }
+}