Fix: tenure downloader reward set error #6234

stackslib/src/net/download/nakamoto/download_state_machine.rs

@@ -28,8 +28,8 @@ use crate::chainstate::stacks::db::StacksChainState;
 use crate::net::chat::ConversationP2P;
 use crate::net::connection::ConnectionOptions;
 use crate::net::download::nakamoto::{
-    downloader_block_height_to_reward_cycle, AvailableTenures, NakamotoTenureDownloader,
-    NakamotoTenureDownloaderSet, NakamotoUnconfirmedTenureDownloader, TenureStartEnd, WantedTenure,
+    AvailableTenures, NakamotoTenureDownloader, NakamotoTenureDownloaderSet,
+    NakamotoUnconfirmedTenureDownloader, TenureStartEnd, WantedTenure,
 };
 use crate::net::inv::nakamoto::NakamotoTenureInv;
 use crate::net::neighbors::rpc::NeighborRPC;
@@ -122,6 +122,21 @@ impl NakamotoDownloadStateMachine {
         }
     }
 
+    /// Return the reward cycle which could be confirmed by a nakamoto block commit
+    ///  in burn block height `burn_height`.
+    ///
+    /// Nakamoto block commits point at the parent of the tenure that
+    /// starts at `burn_height`.  Therefore, a commit at height N
+    /// could confirm a tenure in the reward cycle active at height
+    /// `N-1`.
+    fn get_confirmable_reward_cycle(
+        pox_constants: &PoxConstants,
+        first_burn_height: u64,
+        burn_height: u64,
+    ) -> Option<u64> {
+        pox_constants.block_height_to_reward_cycle(first_burn_height, burn_height.saturating_sub(1))
+    }
+
     /// Get a range of wanted tenures between two burnchain blocks.
     /// Each wanted tenure's .processed flag will be set to false.
     ///
@@ -220,7 +235,13 @@ impl NakamotoDownloadStateMachine {
         sortdb: &SortitionDB,
         loaded_so_far: &[WantedTenure],
     ) -> Result<Vec<WantedTenure>, NetError> {
-        let tip_rc = downloader_block_height_to_reward_cycle(
+        // we want tenures that are *confirmable* from the current sortition tip.
+        //  any miner commitment chosen in the sortition tip confirms a tenure with a lower
+        //  block height, so only consider the reward cycle at height - 1.
+        // *Note: its possible that a the second or later sortition of a RC also confirms a tenure
+        //   in the previous RC, but for the purposes of the wanted tenures calculations, we only
+        //   are loading up wanted tenures in the current RC.
+        let tip_rc = Self::get_confirmable_reward_cycle(
             &sortdb.pox_constants,
             sortdb.first_block_height,
             tip.block_height,
@@ -234,21 +255,17 @@ impl NakamotoDownloadStateMachine {
         } else if let Some(last_tip) = last_tip.as_ref() {
             last_tip.block_height.saturating_add(1)
         } else {
-            // careful -- need .saturating_sub(1) since this calculation puts the reward cycle start at
-            // block height 1 mod reward cycle len, but we really want 0 mod reward cycle len
             sortdb
                 .pox_constants
-                .reward_cycle_to_block_height(sortdb.first_block_height, tip_rc)
-                .saturating_sub(1)
+                .nakamoto_first_block_of_cycle(sortdb.first_block_height, tip_rc)
         };
 
         // be extra careful with last_block_height -- we not only account for the above, but also
         // we need to account for the fact that `load_wanted_tenures` does not load the sortition
         // of the last block height (but we want this!)
         let last_block_height = sortdb
             .pox_constants
-            .reward_cycle_to_block_height(sortdb.first_block_height, tip_rc.saturating_add(1))
-            .saturating_sub(1)
+            .nakamoto_first_block_of_cycle(sortdb.first_block_height, tip_rc.saturating_add(1))
             .min(tip.block_height.saturating_add(1));
 
         debug!(
@@ -395,7 +412,7 @@ impl NakamotoDownloadStateMachine {
                 .expect("FATAL: usize cannot support reward cycle length")
         {
             // this is the first-ever pass, so load up the last full reward cycle
-            let prev_sort_rc = downloader_block_height_to_reward_cycle(
+            let prev_sort_rc = Self::get_confirmable_reward_cycle(
                 &sortdb.pox_constants,
                 sortdb.first_block_height,
                 sort_tip.block_height,
@@ -419,7 +436,7 @@ impl NakamotoDownloadStateMachine {
         }
         if self.wanted_tenures.is_empty() {
             // this is the first-ever pass, so load up the current reward cycle
-            let sort_rc = downloader_block_height_to_reward_cycle(
+            let sort_rc = Self::get_confirmable_reward_cycle(
                 &sortdb.pox_constants,
                 sortdb.first_block_height,
                 sort_tip.block_height,
@@ -472,7 +489,7 @@ impl NakamotoDownloadStateMachine {
         self.initialize_wanted_tenures(sort_tip, sortdb)?;
         let last_sort_height_opt = self.last_sort_tip.as_ref().map(|sn| sn.block_height);
         let last_sort_height = last_sort_height_opt.unwrap_or(sort_tip.block_height);
-        let sort_rc = downloader_block_height_to_reward_cycle(
+        let sort_rc = Self::get_confirmable_reward_cycle(
             &sortdb.pox_constants,
             sortdb.first_block_height,
             last_sort_height,

stackslib/src/net/download/nakamoto/mod.rs

@@ -114,7 +114,6 @@ use std::collections::HashMap;
 use stacks_common::types::chainstate::ConsensusHash;
 use stacks_common::types::StacksEpochId;
 
-use crate::burnchains::PoxConstants;
 use crate::chainstate::burn::db::sortdb::SortitionDB;
 use crate::chainstate::nakamoto::NakamotoBlock;
 use crate::chainstate::stacks::db::StacksChainState;
@@ -139,14 +138,6 @@ pub use crate::net::download::nakamoto::tenure_downloader_unconfirmed::{
     NakamotoUnconfirmedDownloadState, NakamotoUnconfirmedTenureDownloader,
 };
 
-pub fn downloader_block_height_to_reward_cycle(
-    pox_constants: &PoxConstants,
-    first_block_height: u64,
-    block_height: u64,
-) -> Option<u64> {
-    pox_constants.block_height_to_reward_cycle(first_block_height, block_height.saturating_sub(1))
-}
-
 impl PeerNetwork {
     /// Set up the Nakamoto block downloader
     pub fn init_nakamoto_block_downloader(&mut self) {

stackslib/src/net/download/nakamoto/tenure_downloader_unconfirmed.rs

@@ -26,9 +26,7 @@ use crate::chainstate::nakamoto::{NakamotoBlock, NakamotoChainState};
 use crate::chainstate::stacks::boot::RewardSet;
 use crate::chainstate::stacks::db::StacksChainState;
 use crate::net::api::gettenureinfo::RPCGetTenureInfo;
-use crate::net::download::nakamoto::{
-    downloader_block_height_to_reward_cycle, NakamotoTenureDownloader,
-};
+use crate::net::download::nakamoto::NakamotoTenureDownloader;
 use crate::net::httpcore::{StacksHttpRequest, StacksHttpResponse};
 use crate::net::neighbors::rpc::NeighborRPC;
 use crate::net::p2p::{CurrentRewardSet, DropReason, DropSource, PeerNetwork};
@@ -162,8 +160,11 @@ impl NakamotoUnconfirmedTenureDownloader {
             return Err(NetError::InvalidState);
         }
 
-        debug!("Got tenure info {:?}", remote_tenure_tip);
-        debug!("Local sortition tip is {}", &local_sort_tip.consensus_hash);
+        debug!(
+            "Got tenure info";
+            "remote_tenure_tip" => ?remote_tenure_tip,
+            "local_sortition_tip" => %local_sort_tip.consensus_hash
+        );
 
         // authenticate consensus hashes against canonical chain history
         let local_tenure_sn = SortitionDB::get_block_snapshot_consensus(
@@ -286,19 +287,24 @@ impl NakamotoUnconfirmedTenureDownloader {
             return Ok(());
         }
 
+        debug!(
+            "TenureDownloaderUnconfirmed not finished";
+            "tenure_burn_ht" => local_tenure_sn.block_height,
+            "parent_tenure_burn_ht" => parent_local_tenure_sn.block_height
+        );
+
         // we're not finished
-        let tenure_rc = downloader_block_height_to_reward_cycle(
-            &sortdb.pox_constants,
-            sortdb.first_block_height,
-            local_tenure_sn.block_height,
-        )
-        .expect("FATAL: sortition from before system start");
-        let parent_tenure_rc = downloader_block_height_to_reward_cycle(
-            &sortdb.pox_constants,
-            sortdb.first_block_height,
-            parent_local_tenure_sn.block_height,
-        )
-        .expect("FATAL: sortition from before system start");
+        let tenure_rc = sortdb
+            .pox_constants
+            .block_height_to_reward_cycle(sortdb.first_block_height, local_tenure_sn.block_height)
+            .expect("FATAL: sortition from before system start");
+        let parent_tenure_rc = sortdb
+            .pox_constants
+            .block_height_to_reward_cycle(
+                sortdb.first_block_height,
+                parent_local_tenure_sn.block_height,
+            )
+            .expect("FATAL: sortition from before system start");
 
         // get reward set info for the unconfirmed tenure and highest-complete tenure sortitions
         let Some(Some(confirmed_reward_set)) = current_reward_sets
@@ -697,10 +703,12 @@ impl NakamotoUnconfirmedTenureDownloader {
             return Err(NetError::InvalidState);
         };
 
-        debug!(
-            "Create downloader for highest complete tenure {} known by {}",
-            &tenure_tip.parent_consensus_hash, &self.naddr,
+        info!(
+            "Create highest confirmed downloader from unconfirmed";
+            "confirmed_tenure" => %tenure_tip.parent_consensus_hash,
+            "neighbor" => %self.naddr,
         );
+
         let ntd = NakamotoTenureDownloader::new(
             tenure_tip.parent_consensus_hash.clone(),
             tenure_tip.consensus_hash.clone(),