chore: fix p2p handling of unsolicited nakamoto blocks by loading the reward set for nakamoto prepare phases eagerly, and pass the stacks tip height via NetworkResult to the relayer so it can update prometheus

Author: jcnelson

stackslib/src/chainstate/nakamoto/mod.rs

@@ -328,7 +328,7 @@ pub trait StacksDBIndexed {
     fn get(&mut self, tip: &StacksBlockId, key: &str) -> Result<Option<String>, DBError>;
     fn sqlite(&self) -> &Connection;
 
-    /// Get the ancestor block hash given a height
+    /// Get the ancestor block hash given a coinbase height
     fn get_ancestor_block_id(
         &mut self,
         coinbase_height: u64,

stackslib/src/net/mod.rs

@@ -1515,6 +1515,10 @@ pub struct NetworkResult {
     pub num_connected_peers: usize,
     /// The observed burnchain height
     pub burn_height: u64,
+    /// The observed stacks coinbase height
+    pub coinbase_height: u64,
+    /// The observed stacks tip height (different in Nakamoto from coinbase height)
+    pub stacks_tip_height: u64,
     /// The consensus hash of the stacks tip (prefixed `rc_` for historical reasons)
     pub rc_consensus_hash: ConsensusHash,
     /// The current StackerDB configs
@@ -1529,6 +1533,8 @@ impl NetworkResult {
         num_download_passes: u64,
         num_connected_peers: usize,
         burn_height: u64,
+        coinbase_height: u64,
+        stacks_tip_height: u64,
         rc_consensus_hash: ConsensusHash,
         stacker_db_configs: HashMap<QualifiedContractIdentifier, StackerDBConfig>,
     ) -> NetworkResult {
@@ -1557,6 +1563,8 @@ impl NetworkResult {
             num_download_passes: num_download_passes,
             num_connected_peers,
             burn_height,
+            coinbase_height,
+            stacks_tip_height,
             rc_consensus_hash,
             stacker_db_configs,
         }
@@ -3416,8 +3424,6 @@ pub mod test {
             let mut stacks_node = self.stacks_node.take().unwrap();
             let indexer = BitcoinIndexer::new_unit_test(&self.config.burnchain.working_dir);
 
-            let old_tip = self.network.stacks_tip.clone();
-
             self.network
                 .refresh_burnchain_view(&indexer, &sortdb, &mut stacks_node.chainstate, false)
                 .unwrap();
@@ -3426,6 +3432,28 @@ pub mod test {
             self.stacks_node = Some(stacks_node);
         }
 
+        pub fn refresh_reward_cycles(&mut self) {
+            let sortdb = self.sortdb.take().unwrap();
+            let mut stacks_node = self.stacks_node.take().unwrap();
+
+            let tip = SortitionDB::get_canonical_burn_chain_tip(sortdb.conn()).unwrap();
+            let tip_block_id = self.network.stacks_tip.block_id();
+            let tip_height = self.network.stacks_tip.height;
+
+            self.network
+                .refresh_reward_cycles(
+                    &sortdb,
+                    &mut stacks_node.chainstate,
+                    &tip,
+                    &tip_block_id,
+                    tip_height,
+                )
+                .unwrap();
+
+            self.sortdb = Some(sortdb);
+            self.stacks_node = Some(stacks_node);
+        }
+
         pub fn for_each_convo_p2p<F, R>(&mut self, mut f: F) -> Vec<Result<R, net_error>>
         where
             F: FnMut(usize, &mut ConversationP2P) -> Result<R, net_error>,

stackslib/src/net/p2p.rs

@@ -4295,38 +4295,55 @@ impl PeerNetwork {
         }
     }
 
-    /// Refresh our view of the last three reward cycles
-    /// This ensures that the PeerNetwork has cached copies of the reward cycle data (including the
-    /// signing set) for the current, previous, and previous-previous reward cycles.  This data is
-    /// in turn consumed by the Nakamoto block downloader, which must validate blocks signed from
-    /// any of these reward cycles.
-    #[cfg_attr(test, mutants::skip)]
-    fn refresh_reward_cycles(
-        &mut self,
+    /// Determine if we need to invalidate a given cached reward set.
+    ///
+    /// In Epoch 2, this requires checking the first sortition in the start of the reward set's
+    /// reward phase.
+    ///
+    /// In Nakamoto, this requires checking the anchor block in the prepare phase for the upcoming
+    /// reward phase.
+    fn check_reload_cached_reward_set(
+        &self,
         sortdb: &SortitionDB,
-        chainstate: &mut StacksChainState,
+        chainstate: &StacksChainState,
+        rc: u64,
         tip_sn: &BlockSnapshot,
         tip_block_id: &StacksBlockId,
-    ) -> Result<(), net_error> {
-        let cur_rc = self
-            .burnchain
-            .block_height_to_reward_cycle(tip_sn.block_height)
-            .expect("FATAL: sortition from before system start");
-
-        let prev_rc = cur_rc.saturating_sub(1);
-        let prev_prev_rc = prev_rc.saturating_sub(1);
-        let ih = sortdb.index_handle(&tip_sn.sortition_id);
-
-        for rc in [cur_rc, prev_rc, prev_prev_rc] {
-            debug!("Refresh reward cycle info for cycle {}", rc);
+        tip_height: u64,
+    ) -> Result<bool, net_error> {
+        let epoch = self.get_epoch_at_burn_height(tip_sn.block_height);
+        if epoch.epoch_id >= StacksEpochId::Epoch30 {
+            // epoch 3, where there are no forks except from bugs or burnchain reorgs.
+            // invalidate reward cycles on burnchain or stacks reorg, should they ever happen
+            let reorg = Self::is_reorg(Some(&self.burnchain_tip), tip_sn, sortdb)
+                || Self::is_nakamoto_reorg(
+                    &self.stacks_tip.block_id(),
+                    self.stacks_tip.height,
+                    tip_block_id,
+                    tip_height,
+                    chainstate,
+                );
+            if reorg {
+                info!("Burnchain or Stacks reorg detected; will invalidate cached reward set for cycle {rc}");
+            }
+            return Ok(reorg);
+        } else {
+            // epoch 2
             // NOTE: + 1 needed because the sortition db indexes anchor blocks at index height 1,
             // not 0
+            let ih = sortdb.index_handle(&tip_sn.sortition_id);
             let rc_start_height = self.burnchain.nakamoto_first_block_of_cycle(rc) + 1;
             let Some(ancestor_sort_id) =
                 get_ancestor_sort_id(&ih, rc_start_height, &tip_sn.sortition_id)?
             else {
-                // reward cycle is too far back for there to be an ancestor
-                continue;
+                // reward cycle is too far back for there to be an ancestor, so no need to
+                // reload
+                test_debug!(
+                    "No ancestor sortition ID off of {} (height {}) at {rc_start_height})",
+                    &tip_sn.sortition_id,
+                    tip_sn.block_height
+                );
+                return Ok(false);
             };
             let ancestor_ih = sortdb.index_handle(&ancestor_sort_id);
             let anchor_hash_opt = ancestor_ih.get_last_anchor_block_hash()?;
@@ -4340,12 +4357,53 @@ impl PeerNetwork {
                         || cached_rc_info.anchor_block_hash == *anchor_hash
                     {
                         // cached reward set data is still valid
-                        continue;
+                        test_debug!("Cached reward cycle {rc} is still valid");
+                        return Ok(false);
                     }
                 }
             }
+        }
+
+        Ok(true)
+    }
 
-            debug!("Load reward cycle info for cycle {}", rc);
+    /// Refresh our view of the last three reward cycles
+    /// This ensures that the PeerNetwork has cached copies of the reward cycle data (including the
+    /// signing set) for the current, previous, and previous-previous reward cycles.  This data is
+    /// in turn consumed by the Nakamoto block downloader, which must validate blocks signed from
+    /// any of these reward cycles.
+    #[cfg_attr(test, mutants::skip)]
+    pub fn refresh_reward_cycles(
+        &mut self,
+        sortdb: &SortitionDB,
+        chainstate: &mut StacksChainState,
+        tip_sn: &BlockSnapshot,
+        tip_block_id: &StacksBlockId,
+        tip_height: u64,
+    ) -> Result<(), net_error> {
+        let cur_rc = self
+            .burnchain
+            .block_height_to_reward_cycle(tip_sn.block_height)
+            .expect("FATAL: sortition from before system start");
+
+        let prev_rc = cur_rc.saturating_sub(1);
+        let prev_prev_rc = prev_rc.saturating_sub(1);
+
+        for rc in [cur_rc, prev_rc, prev_prev_rc] {
+            debug!("Refresh reward cycle info for cycle {}", rc);
+            if self.current_reward_sets.contains_key(&rc)
+                && !self.check_reload_cached_reward_set(
+                    sortdb,
+                    chainstate,
+                    rc,
+                    tip_sn,
+                    tip_block_id,
+                    tip_height,
+                )?
+            {
+                continue;
+            }
+            debug!("Refresh reward cycle info for cycle {rc}");
             let Some((reward_set_info, anchor_block_header)) = load_nakamoto_reward_set(
                 rc,
                 &tip_sn.sortition_id,
@@ -4452,6 +4510,7 @@ impl PeerNetwork {
                 chainstate,
                 &canonical_sn,
                 &new_stacks_tip_block_id,
+                stacks_tip_height,
             )?;
         }
 
@@ -4649,7 +4708,7 @@ impl PeerNetwork {
             debug!(
                 "{:?}: handle unsolicited stacks messages: tenure changed {} != {}, {} buffered",
                 self.get_local_peer(),
-                &self.burnchain_tip.consensus_hash,
+                &self.stacks_tip.consensus_hash,
                 &canonical_sn.consensus_hash,
                 self.pending_stacks_messages
                     .iter()
@@ -4751,7 +4810,6 @@ impl PeerNetwork {
             ibd,
             true,
         );
-
         let unhandled_messages =
             self.handle_unsolicited_stacks_messages(chainstate, unhandled_messages, true);
 
@@ -4998,7 +5056,7 @@ impl PeerNetwork {
         Ok(())
     }
 
-    /// Static helper to check to see if there has been a reorg
+    /// Static helper to check to see if there has been a burnchain reorg
     pub fn is_reorg(
         last_sort_tip: Option<&BlockSnapshot>,
         sort_tip: &BlockSnapshot,
@@ -5021,15 +5079,15 @@ impl PeerNetwork {
         {
             // current and previous sortition tips are at the same height, but represent different
             // blocks.
-            debug!(
-                "Reorg detected at burn height {}: {} != {}",
+            info!(
+                "Burnchain reorg detected at burn height {}: {} != {}",
                 sort_tip.block_height, &last_sort_tip.consensus_hash, &sort_tip.consensus_hash
             );
             return true;
         }
 
         // It will never be the case that the last and current tip have different heights, but the
-        // smae consensus hash.  If they have the same height, then we would have already returned
+        // same consensus hash.  If they have the same height, then we would have already returned
         // since we've handled both the == and != cases for their consensus hashes.  So if we reach
         // this point, the heights and consensus hashes are not equal.  We only need to check that
         // last_sort_tip is an ancestor of sort_tip
@@ -5061,6 +5119,60 @@ impl PeerNetwork {
         false
     }
 
+    /// Static helper to check to see if there has been a Nakamoto reorg.
+    /// Return true if there's a Nakamoto reorg
+    /// Return false otherwise.
+    pub fn is_nakamoto_reorg(
+        last_stacks_tip: &StacksBlockId,
+        last_stacks_tip_height: u64,
+        stacks_tip: &StacksBlockId,
+        stacks_tip_height: u64,
+        chainstate: &StacksChainState,
+    ) -> bool {
+        if last_stacks_tip == stacks_tip {
+            // same tip
+            return false;
+        }
+
+        if last_stacks_tip_height == stacks_tip_height && last_stacks_tip != stacks_tip {
+            // last block is a sibling
+            info!(
+                "Stacks reorg detected at stacks height {last_stacks_tip_height}: {last_stacks_tip} != {stacks_tip}",
+            );
+            return true;
+        }
+
+        if stacks_tip_height < last_stacks_tip_height {
+            info!(
+                "Stacks reorg (chain shrink) detected at stacks height {last_stacks_tip_height}: {last_stacks_tip} != {stacks_tip}",
+            );
+            return true;
+        }
+
+        // It will never be the case that the last and current tip have different heights, but the
+        // same block ID.  If they have the same height, then we would have already returned
+        // since we've handled both the == and != cases for their block IDs.  So if we reach
+        // this point, the heights and block IDs are not equal.  We only need to check that
+        // last_stacks_tip is an ancestor of stacks_tip
+
+        let mut cursor = stacks_tip.clone();
+        for _ in last_stacks_tip_height..stacks_tip_height {
+            let Ok(Some(parent_id)) =
+                NakamotoChainState::get_nakamoto_parent_block_id(chainstate.db(), &cursor)
+            else {
+                error!("Failed to load parent id of {cursor}");
+                return true;
+            };
+            cursor = parent_id;
+        }
+
+        debug!("is_nakamoto_reorg check";
+               "parent_id" => %cursor,
+               "last_stacks_tip" => %last_stacks_tip);
+
+        cursor != *last_stacks_tip
+    }
+
     /// Log our neighbors.
     /// Used for testing and debuggin
     fn log_neighbors(&mut self) {
@@ -5143,13 +5255,19 @@ impl PeerNetwork {
                 }
             };
 
+        test_debug!(
+            "unsolicited_buffered_messages = {:?}",
+            &unsolicited_buffered_messages
+        );
         let mut network_result = NetworkResult::new(
             self.stacks_tip.block_id(),
             self.num_state_machine_passes,
             self.num_inv_sync_passes,
             self.num_downloader_passes,
             self.peers.len(),
             self.chain_view.burn_block_height,
+            self.stacks_tip.coinbase_height,
+            self.stacks_tip.height,
             self.chain_view.rc_consensus_hash.clone(),
             self.get_stacker_db_configs_owned(),
         );

stackslib/src/net/relay.rs

@@ -2322,8 +2322,6 @@ impl Relayer {
             event_observer,
         )?;
 
-        update_stacks_tip_height(chain_height as i64);
-
         Ok(ret)
     }
 
@@ -3034,6 +3032,10 @@ impl Relayer {
             event_observer.map(|obs| obs.as_stackerdb_event_dispatcher()),
         )?;
 
+        update_stacks_tip_height(
+            i64::try_from(network_result.stacks_tip_height).unwrap_or(i64::MAX),
+        );
+
         let receipts = ProcessedNetReceipts {
             mempool_txs_added,
             processed_unconfirmed_state,