Merge pull request #6347 from Jiloc/fix/health-v3-use-canonical-stacks-height-header

fix: `/v3/health` use canonical stacks height header

Author: Jiloc

docs/rpc/components/examples/node-health.example.json

@@ -1,5 +1,6 @@
 {
   "difference_from_max_peer": 0,
+  "node_stacks_tip_height": 12345,
   "max_stacks_height_of_neighbors": 12345,
-  "node_stacks_tip_height": 12345
+  "max_stacks_neighbor_address": "127.0.0.1:8080"
 }

docs/rpc/components/schemas/get-health.schema.yaml

@@ -16,4 +16,7 @@ properties:
     type: integer
     minimum: 0
     description: The current Stacks tip height of this node
+  max_stacks_neighbor_address:
+    type: [string, "null"]
+    description: The address of the most advanced peer
 description: Health information about the node's synchronization status

docs/rpc/openapi.yaml

@@ -1295,24 +1295,11 @@ paths:
         - `difference_from_max_peer`: The difference in Stacks height between this node and its most advanced peer.
         - `max_stacks_height_of_neighbors`: The maximum Stacks height observed among the node"s connected peers.
         - `node_stacks_tip_height`: The current Stacks tip height of this node.
+        - `max_stacks_neighbor_address`: The address of the most advanced peer. Null if no peer data is available.
       tags:
         - Info
       security: []
       operationId: getNodeHealth
-      parameters:
-        - in: query
-          name: neighbors
-          description: |
-            Specifies the set of peers to use for health checks.
-            - `initial` (default): Use only the initial bootstrap peers.
-            - `all`: Use all connected peers.
-          required: false
-          schema:
-            type: string
-            enum:
-              - initial
-              - all
-            default: initial
       responses:
         "200":
           description: Success
@@ -1323,11 +1310,8 @@ paths:
               example:
                 $ref: "./components/examples/node-health.example.json"
         "400":
-          description: Bad request, such as an invalid `neighbors` query parameter.
           $ref: "#/components/responses/BadRequest"
         "500":
-          description: |
-            Failed to query for health (e.g., no data or no valid peers to query from).
           $ref: "#/components/responses/InternalServerError"
 
   /v2/attachments/{hash}:

stacks-node/src/tests/nakamoto_integrations.rs

@@ -126,7 +126,7 @@ use crate::operations::BurnchainOpSigner;
 use crate::run_loop::boot_nakamoto;
 use crate::tests::neon_integrations::{
     call_read_only, get_account, get_account_result, get_chain_info_opt, get_chain_info_result,
-    get_neighbors, get_pox_info, get_sortition_info, next_block_and_wait,
+    get_neighbors, get_node_health, get_pox_info, get_sortition_info, next_block_and_wait,
     run_until_burnchain_height, submit_tx, submit_tx_fallible, test_observer, wait_for_runloop,
 };
 use crate::tests::signer::SignerTest;
@@ -2426,10 +2426,11 @@ fn mine_multiple_per_tenure_integration() {
 /// It starts in Epoch 2.0, mines with `neon_node` to Epoch 3.0, and then switches
 ///  to Nakamoto operation (activating pox-4 by submitting a stack-stx tx). The BootLoop
 ///  struct handles the epoch-2/3 tear-down and spin-up.
-/// This test makes three assertions:
+/// This test makes four assertions:
 ///  * 15 tenures are mined after 3.0 starts
 ///  * Each tenure has 6 blocks (the coinbase block and 5 interim blocks)
 ///  * Both nodes see the same chainstate at the end of the test
+///  * Both nodes have the same `PeerNetwork::highest_stacks_height_of_neighbors`
 fn multiple_miners() {
     if env::var("BITCOIND_TEST") != Ok("1".into()) {
         return;
@@ -2657,6 +2658,19 @@ fn multiple_miners() {
     info!("Peer height information"; "peer_1" => peer_1_height, "peer_2" => peer_2_height);
     assert_eq!(peer_1_height, peer_2_height);
 
+    // check that the `ConversationHttp::chat` was called and updated
+    // `PeerNetwork::highest_stacks_height_of_neighbors`
+    wait_for(20, || {
+        let health_node_1 = get_node_health(&naka_conf);
+        let health_node_2 = get_node_health(&conf_node_2);
+        info!("Peer health information"; "peer_1" => ?health_node_1, "peer_2" => ?health_node_2);
+        Ok(
+            health_node_1.max_stacks_height_of_neighbors == peer_2_height
+                && health_node_2.max_stacks_height_of_neighbors == peer_1_height,
+        )
+    })
+    .unwrap();
+
     assert!(tip.anchored_header.as_stacks_nakamoto().is_some());
     assert_eq!(
         tip.stacks_block_height,

stacks-node/src/tests/neon_integrations.rs

@@ -55,6 +55,7 @@ use stacks::core::{
 };
 use stacks::net::api::getaccount::AccountEntryResponse;
 use stacks::net::api::getcontractsrc::ContractSrcResponse;
+use stacks::net::api::gethealth::RPCGetHealthResponse;
 use stacks::net::api::getinfo::RPCPeerInfoData;
 use stacks::net::api::getpoxinfo::RPCPoxInfoData;
 use stacks::net::api::getsortition::SortitionInfo;
@@ -1010,6 +1011,18 @@ pub fn get_block(http_origin: &str, block_id: &StacksBlockId) -> Option<StacksBl
     }
 }
 
+pub fn get_node_health(conf: &Config) -> RPCGetHealthResponse {
+    let http_origin = format!("http://{}", &conf.node.rpc_bind);
+    let client = reqwest::blocking::Client::new();
+    let path = format!("{http_origin}/v3/health");
+    client
+        .get(&path)
+        .send()
+        .unwrap()
+        .json::<RPCGetHealthResponse>()
+        .unwrap()
+}
+
 pub fn get_chain_info_result(conf: &Config) -> Result<RPCPeerInfoData, reqwest::Error> {
     let http_origin = format!("http://{}", &conf.node.rpc_bind);
     let client = reqwest::blocking::Client::new();

stackslib/src/net/api/gethealth.rs

@@ -14,22 +14,15 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::fmt;
-use std::str::FromStr;
-
 use regex::{Captures, Regex};
 use stacks_common::types::net::PeerHost;
-use stacks_common::types::StacksEpochId;
 
-use crate::net::db::PeerDB;
 use crate::net::http::{
     parse_json, Error, HttpRequest, HttpRequestContents, HttpRequestPreamble, HttpResponse,
     HttpResponseContents, HttpResponsePayload, HttpResponsePreamble, HttpServerError,
 };
 use crate::net::httpcore::{RPCRequestHandler, StacksHttpRequest, StacksHttpResponse};
-use crate::net::{
-    infer_initial_burnchain_block_download, Error as NetError, NeighborAddress, StacksNodeState,
-};
+use crate::net::{Error as NetError, StacksNodeState};
 
 /// The response for the GET /v3/health endpoint
 /// This endpoint returns the difference in height between the node and its most advanced neighbor
@@ -42,57 +35,19 @@ pub struct RPCGetHealthResponse {
     pub difference_from_max_peer: u64,
     /// the max height of the node's most advanced neighbor
     pub max_stacks_height_of_neighbors: u64,
+    /// the address of the node's most advanced neighbor
+    pub max_stacks_neighbor_address: Option<String>,
     /// the height of this node
     pub node_stacks_tip_height: u64,
 }
 
-const NEIGHBORS_SCOPE_PARAM_NAME: &str = "neighbors";
-
-#[derive(Clone, Debug, PartialEq)]
-pub enum NeighborsScope {
-    Initial,
-    All,
-}
-
-impl FromStr for NeighborsScope {
-    type Err = crate::net::http::Error;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        match s {
-            "initial" => Ok(NeighborsScope::Initial),
-            "all" => Ok(NeighborsScope::All),
-            _ => Err(crate::net::http::Error::Http(
-                400,
-                format!(
-                    "Invalid `neighbors` query parameter: `{}`, allowed values are `initial` or `all`",
-                    s
-                ),
-            )),
-        }
-    }
-}
-
-impl fmt::Display for NeighborsScope {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let s = match self {
-            NeighborsScope::Initial => "initial",
-            NeighborsScope::All => "all",
-        };
-        write!(f, "{s}")
-    }
-}
-
 #[derive(Clone)]
 /// Empty request handler for the GET /v3/health endpoint
-pub struct RPCGetHealthRequestHandler {
-    neighbors_scope: Option<NeighborsScope>,
-}
+pub struct RPCGetHealthRequestHandler {}
 
 impl RPCGetHealthRequestHandler {
     pub fn new() -> Self {
-        Self {
-            neighbors_scope: None,
-        }
+        Self {}
     }
 }
 
@@ -125,12 +80,7 @@ impl HttpRequest for RPCGetHealthRequestHandler {
             ));
         }
 
-        let req_contents = HttpRequestContents::new().query_string(query);
-        if let Some(scope) = req_contents.get_query_arg(NEIGHBORS_SCOPE_PARAM_NAME) {
-            self.neighbors_scope = Some(scope.parse()?);
-        }
-
-        Ok(req_contents)
+        Ok(HttpRequestContents::new().query_string(query))
     }
 }
 
@@ -145,9 +95,7 @@ fn create_error_response(
 
 impl RPCRequestHandler for RPCGetHealthRequestHandler {
     /// Reset internal state
-    fn restart(&mut self) {
-        self.neighbors_scope = None;
-    }
+    fn restart(&mut self) {}
 
     /// Make the response
     fn try_handle_request(
@@ -156,97 +104,30 @@ impl RPCRequestHandler for RPCGetHealthRequestHandler {
         _contents: HttpRequestContents,
         node: &mut StacksNodeState,
     ) -> Result<(HttpResponsePreamble, HttpResponseContents), NetError> {
-        let neighbors_scope = self
-            .neighbors_scope
-            .take()
-            .unwrap_or(NeighborsScope::Initial);
-        let use_all_neighbors = neighbors_scope == NeighborsScope::All;
-
-        node.with_node_state(|network, _sortdb, _chainstate, _mempool, _rpc_args| {
-            let current_epoch = network.get_current_epoch();
-
-            let neighbors_arg = if use_all_neighbors {
-                None
-            } else {
-                let initial_neighbors = PeerDB::get_valid_initial_neighbors(
-                    network.peerdb.conn(),
-                    network.local_peer.network_id,
-                    current_epoch.network_epoch,
-                    network.peer_version,
-                    network.chain_view.burn_block_height,
+        let ((max_stacks_neighbor_address, max_stacks_height_of_neighbors), node_stacks_tip_height) =
+            node.with_node_state(|network, _sortdb, _chainstate, _mempool, _rpc_args| {
+                (
+                    network
+                        .highest_stacks_neighbor
+                        .map(|(addr, height)| (Some(addr.to_string()), height))
+                        .unwrap_or((None, 0)),
+                    network.stacks_tip.height,
                 )
-                .map_err(NetError::from)?;
-
-                if initial_neighbors.is_empty() {
-                    return create_error_response(
-                        &preamble,
-                        "No viable bootstrap peers found, unable to determine health",
-                    );
-                }
-                Some(initial_neighbors)
-            };
-
-            let peer_max_stacks_height_opt = {
-                if current_epoch.epoch_id < StacksEpochId::Epoch30 {
-                    // When the node enters Epoch 3.0, ibd is not accurate. In nakamoto it's always set to false.
-                    // See the implementation of `RunLoop::start` in `stacks-node/src/run_loop/nakamoto.rs`,
-                    // specifically the section and comment where `let ibd = false`, for details.
-                    let ibd = infer_initial_burnchain_block_download(
-                        &network.burnchain,
-                        network.burnchain_tip.block_height,
-                        network.chain_view.burn_block_height,
-                    );
-
-                    // get max block height amongst bootstrap nodes
-                    match network.inv_state.as_ref() {
-                        Some(inv_state) => {
-                            inv_state.get_max_stacks_height_of_neighbors(neighbors_arg.as_deref(), ibd)
-                        }
-                        None => {
-                            return create_error_response(
-                                &preamble,
-                                "Peer inventory state (Epoch 2.x) not found, unable to determine health.",
-                            );
-                        }
-                    }
-                } else {
-                    let neighbors_arg: Option<Vec<NeighborAddress>> = neighbors_arg.as_ref().map(|v| v.iter().map(NeighborAddress::from_neighbor).collect());
-                    match network.block_downloader_nakamoto.as_ref() {
-                        Some(block_downloader_nakamoto) => {
-                            block_downloader_nakamoto.get_max_stacks_height_of_neighbors(neighbors_arg.as_deref())
-                        }
-                        None => {
-                            return create_error_response(
-                                &preamble,
-                                "Nakamoto block downloader not found (Epoch 3.0+), unable to determine health.",
-                            );
-                        }
-                    }
-                }
-            };
-
-            match peer_max_stacks_height_opt {
-                Some(max_stacks_height_of_neighbors) => {
-                    // There could be a edge case where our node is ahead of all peers.
-                    let node_stacks_tip_height = network.stacks_tip.height;
-                    let difference_from_max_peer =
-                        max_stacks_height_of_neighbors.saturating_sub(node_stacks_tip_height);
-
-                    let preamble = HttpResponsePreamble::ok_json(&preamble);
-                    let data = RPCGetHealthResponse {
-                        difference_from_max_peer,
-                        max_stacks_height_of_neighbors,
-                        node_stacks_tip_height,
-                    };
-                    let body = HttpResponseContents::try_from_json(&data)?;
-                    Ok((preamble, body))
-                }
-                None => create_error_response(
-                    &preamble,
-                    "Couldn't obtain stats on any bootstrap peers, unable to determine health.",
-                ),
-            }
-        })
+            });
+
+        // There could be a edge case where our node is ahead of all peers.
+        let difference_from_max_peer =
+            max_stacks_height_of_neighbors.saturating_sub(node_stacks_tip_height);
+
+        let preamble = HttpResponsePreamble::ok_json(&preamble);
+        let data = RPCGetHealthResponse {
+            difference_from_max_peer,
+            max_stacks_height_of_neighbors,
+            max_stacks_neighbor_address,
+            node_stacks_tip_height,
+        };
+        let body = HttpResponseContents::try_from_json(&data)?;
+        Ok((preamble, body))
     }
 }
 
@@ -263,15 +144,12 @@ impl HttpResponse for RPCGetHealthRequestHandler {
 }
 
 impl StacksHttpRequest {
-    pub fn new_gethealth(host: PeerHost, neighbors_scope: NeighborsScope) -> StacksHttpRequest {
+    pub fn new_gethealth(host: PeerHost) -> StacksHttpRequest {
         StacksHttpRequest::new_for_peer(
             host,
             "GET".into(),
             "/v3/health".into(),
-            HttpRequestContents::new().query_arg(
-                NEIGHBORS_SCOPE_PARAM_NAME.into(),
-                neighbors_scope.to_string(),
-            ),
+            HttpRequestContents::new(),
         )
         .expect("FATAL: failed to construct request from infallible data")
     }