Merge branch 'develop' into feat/rpc-health-v3

Author: Jiloc

CHANGELOG.md

@@ -11,6 +11,10 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 
 - Added a new RPC endpoint `/v3/health` to query the node's health status. The endpoint returns a 200 status code with relevant synchronization information (including the node's current Stacks tip height, the maximum Stacks tip height among its neighbors, and the difference between these two). A user can use the `difference_from_max_peer` value to decide what is a good threshold for them before considering the node out of sync. The endpoint returns a 500 status code if the query cannot retrieve viable data.
 
+## [3.1.0.0.11]
+
+- Hotfix for p2p stack misbehavior in mempool syncing conditions
+
 ## [3.1.0.0.10]
 
 ### Added

stacks-signer/src/chainstate.rs

@@ -389,9 +389,9 @@ impl SortitionsView {
             )?;
         } else {
             // check if the new block confirms the last block in the current tenure
-            let confirms_latest_in_tenure =
-                Self::confirms_latest_block_in_same_tenure(block, signer_db)
-                    .map_err(SignerChainstateError::from)?;
+            let confirms_latest_in_tenure = self
+                .confirms_latest_block_in_same_tenure(block, signer_db, client)
+                .map_err(SignerChainstateError::from)?;
             if !confirms_latest_in_tenure {
                 return Err(RejectReason::InvalidParentBlock);
             }
@@ -562,8 +562,7 @@ impl SortitionsView {
         Ok(true)
     }
 
-    /// Get the last block from the given tenure
-    /// Returns the last locally accepted block if it is not timed out, otherwise it will return the last globally accepted block.
+    /// Get the last locally signed block from the given tenure if it has not timed out
     pub fn get_tenure_last_block_info(
         consensus_hash: &ConsensusHash,
         signer_db: &SignerDb,
@@ -573,43 +572,45 @@ impl SortitionsView {
         let last_locally_accepted_block = signer_db
             .get_last_accepted_block(consensus_hash)
             .map_err(|e| ClientError::InvalidResponse(e.to_string()))?;
+        let Some(local_info) = last_locally_accepted_block else {
+            return Ok(None);
+        };
 
-        if let Some(local_info) = last_locally_accepted_block {
-            if let Some(signed_over_time) = local_info.signed_self {
-                if signed_over_time.saturating_add(tenure_last_block_proposal_timeout.as_secs())
-                    > get_epoch_time_secs()
-                {
-                    // The last locally accepted block is not timed out, return it
-                    return Ok(Some(local_info));
-                }
-            }
+        let Some(signed_over_time) = local_info.signed_self else {
+            return Ok(None);
+        };
+
+        if signed_over_time.saturating_add(tenure_last_block_proposal_timeout.as_secs())
+            > get_epoch_time_secs()
+        {
+            // The last locally accepted block is not timed out, return it
+            Ok(Some(local_info))
+        } else {
+            // The last locally accepted block is timed out
+            Ok(None)
         }
-        // The last locally accepted block is timed out, get the last globally accepted block
-        signer_db
-            .get_last_globally_accepted_block(consensus_hash)
-            .map_err(|e| ClientError::InvalidResponse(e.to_string()))
     }
 
-    /// Check if the tenure change block confirms the expected parent block
-    /// (i.e., the last locally accepted block in the parent tenure, or if that block is timed out, the last globally accepted block in the parent tenure)
-    /// It checks the local DB first, and if the block is not present in the local DB, it asks the
-    /// Stacks node for the highest processed block header in the given tenure (and then caches it
-    /// in the DB).
+    /// Check whether or not `block` is higher than the highest block in `tenure_id`.
+    ///  returns `Ok(true)` if `block` is higher, `Ok(false)` if not.
     ///
-    /// The rationale here is that the signer DB can be out-of-sync with the node.  For example,
-    /// the signer may have been added to an already-running node.
-    pub fn check_tenure_change_confirms_parent(
-        tenure_change: &TenureChangePayload,
+    /// If we can't look up `tenure_id`, assume `block` is higher.
+    /// This assumption is safe because this proposal ultimately must be passed
+    /// to the `stacks-node` for proposal processing: so, if we pass the block
+    /// height check here, we are relying on the `stacks-node` proposal endpoint
+    /// to do the validation on the chainstate data that it has.
+    ///
+    /// This updates the activity timer for the miner of `block`.
+    pub fn check_latest_block_in_tenure(
+        tenure_id: &ConsensusHash,
         block: &NakamotoBlock,
         signer_db: &mut SignerDb,
         client: &StacksClient,
         tenure_last_block_proposal_timeout: Duration,
         reorg_attempts_activity_timeout: Duration,
     ) -> Result<bool, ClientError> {
-        // If the tenure change block confirms the expected parent block, it should confirm at least one more block than the last accepted block in the parent tenure.
-        // NOTE: returns the locally accepted block if it is not timed out, otherwise it will return the last globally accepted block.
         let last_block_info = Self::get_tenure_last_block_info(
-            &tenure_change.prev_tenure_consensus_hash,
+            tenure_id,
             signer_db,
             tenure_last_block_proposal_timeout,
         )?;
@@ -636,7 +637,7 @@ impl SortitionsView {
                     // to give the miner some extra buffer time to wait for its chain tip to advance
                     // The miner may just be slow, so count this invalid block proposal towards valid miner activity.
                     if let Err(e) = signer_db.update_last_activity_time(
-                        &tenure_change.tenure_consensus_hash,
+                        &block.header.consensus_hash,
                         get_epoch_time_secs(),
                     ) {
                         warn!("Failed to update last activity time: {e}");
@@ -646,16 +647,16 @@ impl SortitionsView {
             }
         }
 
-        let tip = match client.get_tenure_tip(&tenure_change.prev_tenure_consensus_hash) {
+        let tip = match client.get_tenure_tip(tenure_id) {
             Ok(tip) => tip,
             Err(e) => {
                 warn!(
-                    "Miner block proposal contains a tenure change, but failed to fetch the tenure tip for the parent tenure: {e:?}. Considering proposal invalid.";
+                    "Failed to fetch the tenure tip for the parent tenure: {e:?}. Assuming proposal is higher than the parent tenure for now.";
                     "proposed_block_consensus_hash" => %block.header.consensus_hash,
                     "signer_signature_hash" => %block.header.signer_signature_hash(),
-                    "parent_tenure" => %tenure_change.prev_tenure_consensus_hash,
+                    "parent_tenure" => %tenure_id,
                 );
-                return Ok(false);
+                return Ok(true);
             }
         };
         if let Some(nakamoto_tip) = tip.as_stacks_nakamoto() {
@@ -673,19 +674,33 @@ impl SortitionsView {
                 }
             }
         }
-        let tip_height = tip.height();
-        if block.header.chain_length > tip_height {
-            Ok(true)
-        } else {
-            warn!(
-                "Miner's block proposal does not confirm as many blocks as we expect";
-                "proposed_block_consensus_hash" => %block.header.consensus_hash,
-                "signer_signature_hash" => %block.header.signer_signature_hash(),
-                "proposed_chain_length" => block.header.chain_length,
-                "expected_at_least" => tip_height + 1,
-            );
-            Ok(false)
-        }
+        Ok(tip.height() < block.header.chain_length)
+    }
+
+    /// Check if the tenure change block confirms the expected parent block
+    /// (i.e., the last locally accepted block in the parent tenure, or if that block is timed out, the last globally accepted block in the parent tenure)
+    /// It checks the local DB first, and if the block is not present in the local DB, it asks the
+    /// Stacks node for the highest processed block header in the given tenure (and then caches it
+    /// in the DB).
+    ///
+    /// The rationale here is that the signer DB can be out-of-sync with the node.  For example,
+    /// the signer may have been added to an already-running node.
+    pub fn check_tenure_change_confirms_parent(
+        tenure_change: &TenureChangePayload,
+        block: &NakamotoBlock,
+        signer_db: &mut SignerDb,
+        client: &StacksClient,
+        tenure_last_block_proposal_timeout: Duration,
+        reorg_attempts_activity_timeout: Duration,
+    ) -> Result<bool, ClientError> {
+        Self::check_latest_block_in_tenure(
+            &tenure_change.prev_tenure_consensus_hash,
+            block,
+            signer_db,
+            client,
+            tenure_last_block_proposal_timeout,
+            reorg_attempts_activity_timeout,
+        )
     }
 
     /// in tenure changes, we need to check:
@@ -742,33 +757,19 @@ impl SortitionsView {
     }
 
     fn confirms_latest_block_in_same_tenure(
+        &self,
         block: &NakamotoBlock,
-        signer_db: &SignerDb,
+        signer_db: &mut SignerDb,
+        client: &StacksClient,
     ) -> Result<bool, ClientError> {
-        let Some(last_known_block) = signer_db
-            .get_last_accepted_block(&block.header.consensus_hash)
-            .map_err(|e| ClientError::InvalidResponse(e.to_string()))?
-        else {
-            info!(
-                "Have no accepted blocks in the tenure, assuming block confirmation is correct";
-                "proposed_block_consensus_hash" => %block.header.consensus_hash,
-                "signer_signature_hash" => %block.header.signer_signature_hash(),
-                "proposed_block_height" => block.header.chain_length,
-            );
-            return Ok(true);
-        };
-        if block.header.chain_length > last_known_block.block.header.chain_length {
-            Ok(true)
-        } else {
-            warn!(
-                "Miner's block proposal does not confirm as many blocks as we expect";
-                "proposed_block_consensus_hash" => %block.header.consensus_hash,
-                "signer_signature_hash" => %block.header.signer_signature_hash(),
-                "proposed_chain_length" => block.header.chain_length,
-                "expected_at_least" => last_known_block.block.header.chain_length + 1,
-            );
-            Ok(false)
-        }
+        Self::check_latest_block_in_tenure(
+            &block.header.consensus_hash,
+            block,
+            signer_db,
+            client,
+            self.config.tenure_last_block_proposal_timeout,
+            self.config.reorg_attempts_activity_timeout,
+        )
     }
 
     /// Fetch a new view of the recent sortitions

stacks-signer/src/lib.rs

@@ -53,6 +53,7 @@ use chainstate::SortitionsView;
 use config::GlobalConfig;
 use libsigner::{SignerEvent, SignerEventReceiver, SignerEventTrait, VERSION_STRING};
 use runloop::SignerResult;
+use signerdb::BlockInfo;
 use stacks_common::{info, warn};
 use v0::signer_state::LocalStateMachine;
 
@@ -81,6 +82,8 @@ pub trait Signer<T: SignerEventTrait>: Debug + Display {
     fn get_local_state_machine(&self) -> &LocalStateMachine;
     /// Get the number of pending block proposals
     fn get_pending_proposals_count(&self) -> u64;
+    /// Get canonical block according to this signer's db
+    fn get_canonical_tip(&self) -> Option<BlockInfo>;
 }
 
 /// A wrapper around the running signer type for the signer

stacks-signer/src/runloop.rs

@@ -25,6 +25,7 @@ use stacks_common::{debug, error, info, warn};
 use crate::chainstate::SortitionsView;
 use crate::client::{retry_with_exponential_backoff, ClientError, StacksClient};
 use crate::config::{GlobalConfig, SignerConfig, SignerConfigMode};
+use crate::signerdb::BlockInfo;
 use crate::v0::signer_state::LocalStateMachine;
 #[cfg(any(test, feature = "testing"))]
 use crate::v0::tests::TEST_SKIP_SIGNER_CLEANUP;
@@ -59,6 +60,9 @@ pub struct StateInfo {
     pub signer_state_machines: Vec<(u64, Option<LocalStateMachine>)>,
     /// The number of pending block proposals for this signer
     pub pending_proposals_count: u64,
+    /// The canonical tip block info according to the running signers
+    /// as a pair of (reward-cycle, block-info)
+    pub signer_canonical_tips: Vec<(u64, Option<BlockInfo>)>,
 }
 
 /// The signer result that can be sent across threads
@@ -544,6 +548,16 @@ impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug>
                         }
                     })
                     .unwrap_or(0),
+                signer_canonical_tips: self
+                    .stacks_signers
+                    .iter()
+                    .map(|(reward_cycle, signer)| {
+                        let ConfiguredSigner::RegisteredSigner(ref signer) = signer else {
+                            return (*reward_cycle, None);
+                        };
+                        (*reward_cycle, signer.get_canonical_tip())
+                    })
+                    .collect(),
             };
             info!("Signer status check requested: {state_info:?}");
 

stacks-signer/src/signerdb.rs

@@ -98,7 +98,7 @@ impl FromRow<BurnBlockInfo> for BurnBlockInfo {
     }
 }
 
-#[derive(Serialize, Deserialize, Debug, PartialEq, Default)]
+#[derive(Serialize, Deserialize, Debug, PartialEq, Default, Clone)]
 /// Store extra version-specific info in `BlockInfo`
 pub enum ExtraBlockInfo {
     #[default]
@@ -167,7 +167,7 @@ impl TryFrom<&str> for BlockState {
 }
 
 /// Additional Info about a proposed block
-#[derive(Serialize, Deserialize, Debug, PartialEq)]
+#[derive(Serialize, Deserialize, Debug, PartialEq, Clone)]
 pub struct BlockInfo {
     /// The block we are considering
     pub block: NakamotoBlock,