test: add integration tests for signer validation of reorgs

.github/workflows/bitcoin-tests.yml

@@ -87,6 +87,8 @@ jobs:
           - tests::signer::v0::miner_gather_signatures
           - tests::signer::v0::mine_2_nakamoto_reward_cycles
           - tests::signer::v0::end_of_tenure
+          - tests::signer::v0::forked_tenure_okay
+          - tests::signer::v0::forked_tenure_invalid
           - tests::nakamoto_integrations::stack_stx_burn_op_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state

stacks-signer/src/chainstate.rs

@@ -34,22 +34,10 @@ use crate::signerdb::SignerDb;
 pub enum SignerChainstateError {
     /// Error resulting from database interactions
     #[error("Database error: {0}")]
-    DBError(DBError),
+    DBError(#[from] DBError),
     /// Error resulting from crate::client interactions
     #[error("Client error: {0}")]
-    ClientError(ClientError),
-}
-
-impl From<ClientError> for SignerChainstateError {
-    fn from(value: ClientError) -> Self {
-        Self::ClientError(value)
-    }
-}
-
-impl From<DBError> for SignerChainstateError {
-    fn from(value: DBError) -> Self {
-        Self::DBError(value)
-    }
+    ClientError(#[from] ClientError),
 }
 
 /// Captures this signer's current view of a sortition's miner.
@@ -93,8 +81,8 @@ pub struct SortitionState {
 /// Captures the configuration settings used by the signer when evaluating block proposals.
 #[derive(Debug, Clone)]
 pub struct ProposalEvalConfig {
-    /// How much time between the first block proposal in a tenure and the next bitcoin block
-    ///  must pass before a subsequent miner isn't allowed to reorg the tenure
+    /// How much time must pass between the first block proposal in a tenure and the next bitcoin block
+    ///  before a subsequent miner isn't allowed to reorg the tenure
     pub first_proposal_burn_block_timing: Duration,
 }
 
@@ -323,6 +311,9 @@ impl SortitionsView {
             "Most recent miner's tenure does not build off the prior sortition, checking if this is valid behavior";
             "proposed_block_consensus_hash" => %block.header.consensus_hash,
             "proposed_block_signer_sighash" => %block.header.signer_signature_hash(),
+            "sortition_state.consensus_hash" => %sortition_state.consensus_hash,
+            "sortition_state.prior_sortition" => %sortition_state.prior_sortition,
+            "sortition_state.parent_tenure_id" => %sortition_state.parent_tenure_id,
         );
 
         let tenures_reorged = client.get_tenure_forking_info(
@@ -367,8 +358,14 @@ impl SortitionsView {
                     sortition_state_received_time
                 {
                     // how long was there between when the proposal was received and the next sortition started?
-                    let proposal_to_sortition = sortition_state_received_time
-                        .saturating_sub(local_block_info.proposed_time);
+                    let proposal_to_sortition = if let Some(signed_at) =
+                        local_block_info.signed_self
+                    {
+                        sortition_state_received_time.saturating_sub(signed_at)
+                    } else {
+                        info!("We did not sign over the reorged tenure's first block, considering it as a late-arriving proposal");
+                        0
+                    };
                     if Duration::from_secs(proposal_to_sortition)
                         <= *first_proposal_burn_block_timing
                     {

stacks-signer/src/config.rs

@@ -151,8 +151,8 @@ pub struct SignerConfig {
     pub max_tx_fee_ustx: Option<u64>,
     /// The path to the signer's database file
     pub db_path: PathBuf,
-    /// How much time between the first block proposal in a tenure and the next bitcoin block
-    ///  must pass before a subsequent miner isn't allowed to reorg the tenure
+    /// How much time must pass between the first block proposal in a tenure and the next bitcoin block
+    ///  before a subsequent miner isn't allowed to reorg the tenure
     pub first_proposal_burn_block_timing: Duration,
 }
 
@@ -233,8 +233,8 @@ struct RawConfigFile {
     pub db_path: String,
     /// Metrics endpoint
     pub metrics_endpoint: Option<String>,
-    /// How much time between the first block proposal in a tenure and the next bitcoin block
-    ///  must pass before a subsequent miner isn't allowed to reorg the tenure
+    /// How much time must pass between the first block proposal in a tenure and the next bitcoin block
+    ///  before a subsequent miner isn't allowed to reorg the tenure
     pub first_proposal_burn_block_timing_secs: Option<u64>,
 }
 

stacks-signer/src/signerdb.rs

@@ -83,6 +83,16 @@ impl BlockInfo {
         block_info
     }
 
+    /// Mark this block as valid, signed over, and record a timestamp in the block info if it wasn't
+    ///  already set.
+    pub fn mark_signed_and_valid(&mut self) {
+        self.valid = Some(true);
+        self.signed_over = true;
+        if self.signed_self.is_none() {
+            self.signed_self = Some(get_epoch_time_secs());
+        }
+    }
+
     /// Return the block's signer signature hash
     pub fn signer_signature_hash(&self) -> Sha512Trunc256Sum {
         self.block.header.signer_signature_hash()

stacks-signer/src/v0/signer.rs

@@ -409,7 +409,7 @@ impl Signer {
                         return;
                     }
                 };
-                block_info.valid = Some(true);
+                block_info.mark_signed_and_valid();
                 let signature = self
                     .private_key
                     .sign(&signer_signature_hash.0)

stackslib/src/chainstate/nakamoto/miner.rs

@@ -216,6 +216,11 @@ impl NakamotoBlockBuilder {
                 tenure_id_consensus_hash.clone(),
                 parent_stacks_header.index_block_hash(),
                 bitvec_len,
+                parent_stacks_header
+                    .anchored_header
+                    .as_stacks_nakamoto()
+                    .map(|b| b.timestamp)
+                    .unwrap_or(0),
             ),
         })
     }

stackslib/src/chainstate/nakamoto/mod.rs

@@ -635,6 +635,7 @@ impl NakamotoBlockHeader {
         consensus_hash: ConsensusHash,
         parent_block_id: StacksBlockId,
         bitvec_len: u16,
+        parent_timestamp: u64,
     ) -> NakamotoBlockHeader {
         NakamotoBlockHeader {
             version: NAKAMOTO_BLOCK_VERSION,
@@ -644,7 +645,7 @@ impl NakamotoBlockHeader {
             parent_block_id,
             tx_merkle_root: Sha512Trunc256Sum([0u8; 32]),
             state_index_root: TrieHash([0u8; 32]),
-            timestamp: get_epoch_time_secs(),
+            timestamp: std::cmp::max(parent_timestamp, get_epoch_time_secs()),
             miner_signature: MessageSignature::empty(),
             signer_signature: vec![],
             pox_treatment: BitVec::ones(bitvec_len)

stackslib/src/net/api/getsortition.rs

@@ -247,7 +247,7 @@ impl RPCRequestHandler for GetSortitionHandler {
                         stacks_parent_sn.consensus_hash.clone()
                     } else {
                         // we actually need to perform the marf lookup
-                        let last_sortition = handle.get_last_snapshot_with_sortition(stacks_parent_sn.block_height)?;
+                        let last_sortition = handle.get_last_snapshot_with_sortition(sortition_sn.block_height.saturating_sub(1))?;
                         last_sortition.consensus_hash
                     };
 

testnet/stacks-node/src/nakamoto_node/miner.rs

@@ -243,6 +243,27 @@ impl BlockMinerThread {
             };
 
             if let Some(mut new_block) = new_block {
+                #[cfg(test)]
+                {
+                    if *TEST_BROADCAST_STALL.lock().unwrap() == Some(true) {
+                        // Do an extra check just so we don't log EVERY time.
+                        warn!("Broadcasting is stalled due to testing directive.";
+                                  "stacks_block_id" => %new_block.block_id(),
+                                  "stacks_block_hash" => %new_block.header.block_hash(),
+                                  "height" => new_block.header.chain_length,
+                                  "consensus_hash" => %new_block.header.consensus_hash
+                        );
+                        while *TEST_BROADCAST_STALL.lock().unwrap() == Some(true) {
+                            std::thread::sleep(std::time::Duration::from_millis(10));
+                        }
+                        info!("Broadcasting is no longer stalled due to testing directive.";
+                              "block_id" => %new_block.block_id(),
+                              "height" => new_block.header.chain_length,
+                              "consensus_hash" => %new_block.header.consensus_hash
+                        );
+                    }
+                }
+
                 let (reward_set, signer_signature) = match self.gather_signatures(
                     &mut new_block,
                     self.burn_block.block_height,
@@ -656,26 +677,6 @@ impl BlockMinerThread {
         reward_set: RewardSet,
         stackerdbs: &StackerDBs,
     ) -> Result<(), NakamotoNodeError> {
-        #[cfg(test)]
-        {
-            if *TEST_BROADCAST_STALL.lock().unwrap() == Some(true) {
-                // Do an extra check just so we don't log EVERY time.
-                warn!("Broadcasting is stalled due to testing directive.";
-                    "stacks_block_id" => %block.block_id(),
-                    "stacks_block_hash" => %block.header.block_hash(),
-                    "height" => block.header.chain_length,
-                    "consensus_hash" => %block.header.consensus_hash
-                );
-                while *TEST_BROADCAST_STALL.lock().unwrap() == Some(true) {
-                    std::thread::sleep(std::time::Duration::from_millis(10));
-                }
-                info!("Broadcasting is no longer stalled due to testing directive.";
-                    "block_id" => %block.block_id(),
-                    "height" => block.header.chain_length,
-                    "consensus_hash" => %block.header.consensus_hash
-                );
-            }
-        }
         let mut chain_state = neon_node::open_chainstate_with_faults(&self.config)
             .expect("FATAL: could not open chainstate DB");
         let sort_db = SortitionDB::open(
@@ -1014,7 +1015,6 @@ impl BlockMinerThread {
                 ChainstateError::NoTransactionsToMine,
             ));
         }
-
         let mining_key = self.keychain.get_nakamoto_sk();
         let miner_signature = mining_key
             .sign(block.header.miner_signature_hash().as_bytes())
@@ -1028,6 +1028,7 @@ impl BlockMinerThread {
             block.txs.len();
             "signer_sighash" => %block.header.signer_signature_hash(),
             "consensus_hash" => %block.header.consensus_hash,
+            "timestamp" => block.header.timestamp,
         );
 
         self.event_dispatcher.process_mined_nakamoto_block_event(

testnet/stacks-node/src/tests/signer/mod.rs

@@ -104,6 +104,15 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
         num_signers: usize,
         initial_balances: Vec<(StacksAddress, u64)>,
         wait_on_signers: Option<Duration>,
+    ) -> Self {
+        Self::new_with_config_modifications(num_signers, initial_balances, wait_on_signers, |_| {})
+    }
+
+    fn new_with_config_modifications<F: Fn(&mut SignerConfig) -> ()>(
+        num_signers: usize,
+        initial_balances: Vec<(StacksAddress, u64)>,
+        wait_on_signers: Option<Duration>,
+        modifier: F,
     ) -> Self {
         // Generate Signer Data
         let signer_stacks_private_keys = (0..num_signers)
@@ -148,8 +157,9 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
             .into_iter()
             .map(|i| {
                 info!("spawning signer");
-                let signer_config =
+                let mut signer_config =
                     SignerConfig::load_from_str(&signer_configs[i as usize]).unwrap();
+                modifier(&mut signer_config);
                 SpawnedSigner::new(signer_config)
             })
             .collect();
@@ -520,11 +530,10 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
         self.running_nodes
             .run_loop_stopper
             .store(false, Ordering::SeqCst);
-        // Stop the signers before the node to prevent hanging
+        self.running_nodes.run_loop_thread.join().unwrap();
         for signer in self.spawned_signers {
             assert!(signer.stop().is_none());
         }
-        self.running_nodes.run_loop_thread.join().unwrap();
     }
 }