Merge pull request #5662 from stacks-network/fix/signers-verify-reward-cycle

Do not accept signatures, block proposals, or block responses for blocks from different reward cycles

Author: kantai

.github/workflows/bitcoin-tests.yml

@@ -136,6 +136,9 @@ jobs:
           - tests::signer::v0::block_proposal_max_age_rejections
           - tests::signer::v0::global_acceptance_depends_on_block_announcement
           - tests::signer::v0::no_reorg_due_to_successive_block_validation_ok
+          - tests::signer::v0::incoming_signers_ignore_block_proposals
+          - tests::signer::v0::outgoing_signers_ignore_block_proposals
+          - tests::signer::v0::injected_signatures_are_ignored_across_boundaries
           - tests::nakamoto_integrations::burn_ops_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state

stacks-signer/src/runloop.rs

@@ -26,6 +26,8 @@ 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};
+#[cfg(any(test, feature = "testing"))]
+use crate::v0::tests::TEST_SKIP_SIGNER_CLEANUP;
 use crate::Signer as SignerTrait;
 
 #[derive(thiserror::Error, Debug)]
@@ -46,6 +48,8 @@ pub struct StateInfo {
     pub runloop_state: State,
     /// the current reward cycle info
     pub reward_cycle_info: Option<RewardCycleInfo>,
+    /// The current running signers reward cycles
+    pub running_signers: Vec<u64>,
 }
 
 /// The signer result that can be sent across threads
@@ -421,6 +425,11 @@ impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug> RunLo
     }
 
     fn cleanup_stale_signers(&mut self, current_reward_cycle: u64) {
+        #[cfg(any(test, feature = "testing"))]
+        if TEST_SKIP_SIGNER_CLEANUP.get() {
+            warn!("Skipping signer cleanup due to testing directive.");
+            return;
+        }
         let mut to_delete = Vec::new();
         for (idx, signer) in &mut self.stacks_signers {
             let reward_cycle = signer.reward_cycle();
@@ -467,6 +476,11 @@ impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug>
             if let Err(e) = res.send(vec![StateInfo {
                 runloop_state: self.state,
                 reward_cycle_info: self.current_reward_cycle_info,
+                running_signers: self
+                    .stacks_signers
+                    .values()
+                    .map(|s| s.reward_cycle())
+                    .collect(),
             }
             .into()])
             {

stacks-signer/src/signerdb.rs

@@ -960,22 +960,6 @@ impl SignerDb {
         Ok(Some(broadcasted))
     }
 
-    /// Get the current state of a given block in the database
-    pub fn get_block_state(
-        &self,
-        block_sighash: &Sha512Trunc256Sum,
-    ) -> Result<Option<BlockState>, DBError> {
-        let qry = "SELECT state FROM blocks WHERE signer_signature_hash = ?1 LIMIT 1";
-        let args = params![block_sighash];
-        let state_opt: Option<String> = query_row(&self.db, qry, args)?;
-        let Some(state) = state_opt else {
-            return Ok(None);
-        };
-        Ok(Some(
-            BlockState::try_from(state.as_str()).map_err(|_| DBError::Corruption)?,
-        ))
-    }
-
     /// Return the start time (epoch time in seconds) and the processing time in milliseconds of the tenure (idenfitied by consensus_hash).
     fn get_tenure_times(&self, tenure: &ConsensusHash) -> Result<(u64, u64), DBError> {
         let query = "SELECT tenure_change, proposed_time, validation_time_ms FROM blocks WHERE consensus_hash = ?1 AND state = ?2 ORDER BY stacks_height DESC";
@@ -1133,13 +1117,6 @@ mod tests {
             .expect("Unable to get block from db");
 
         assert_eq!(BlockInfo::from(block_proposal_2.clone()), block_info);
-        // test getting the block state
-        let block_state = db
-            .get_block_state(&block_proposal_1.block.header.signer_signature_hash())
-            .unwrap()
-            .expect("Unable to get block state from db");
-
-        assert_eq!(block_state, BlockInfo::from(block_proposal_1.clone()).state);
     }
 
     #[test]

stacks-signer/src/v0/signer.rs

@@ -24,7 +24,7 @@ use blockstack_lib::net::api::postblock_proposal::{
 use blockstack_lib::util_lib::db::Error as DBError;
 use clarity::types::chainstate::StacksPrivateKey;
 use clarity::types::{PrivateKey, StacksEpochId};
-use clarity::util::hash::MerkleHashFunc;
+use clarity::util::hash::{MerkleHashFunc, Sha512Trunc256Sum};
 use clarity::util::secp256k1::Secp256k1PublicKey;
 use libsigner::v0::messages::{
     BlockAccepted, BlockRejection, BlockResponse, MessageSlotID, MockProposal, MockSignature,
@@ -128,6 +128,17 @@ impl SignerTrait<SignerMessage> for Signer {
             debug!("{self}: No event received");
             return;
         };
+        if self.reward_cycle > current_reward_cycle
+            && !matches!(
+                event,
+                SignerEvent::StatusCheck | SignerEvent::NewBurnBlock { .. }
+            )
+        {
+            // The reward cycle has not yet started for this signer instance
+            // Do not process any events other than status checks or new burn blocks
+            debug!("{self}: Signer reward cycle has not yet started. Ignoring event.");
+            return;
+        }
         match event {
             SignerEvent::BlockValidationResponse(block_validate_response) => {
                 debug!("{self}: Received a block proposal result from the stacks node...");
@@ -444,11 +455,7 @@ impl Signer {
         // TODO: should add a check to ignore an old burn block height if we know its outdated. Would require us to store the burn block height we last saw on the side.
         //  the signer needs to be able to determine whether or not the block they're about to sign would conflict with an already-signed Stacks block
         let signer_signature_hash = block_proposal.block.header.signer_signature_hash();
-        if let Some(block_info) = self
-            .signer_db
-            .block_lookup(&signer_signature_hash)
-            .expect("Failed to connect to signer DB")
-        {
+        if let Some(block_info) = self.block_lookup_by_reward_cycle(&signer_signature_hash) {
             let Some(block_response) = self.determine_response(&block_info) else {
                 // We are still waiting for a response for this block. Do nothing.
                 debug!("{self}: Received a block proposal for a block we are already validating.";
@@ -677,32 +684,15 @@ impl Signer {
             self.submitted_block_proposal = None;
         }
         // For mutability reasons, we need to take the block_info out of the map and add it back after processing
-        let mut block_info = match self.signer_db.block_lookup(&signer_signature_hash) {
-            Ok(Some(block_info)) => {
-                if block_info.reward_cycle != self.reward_cycle {
-                    // We are not signing for this reward cycle. Ignore the block.
-                    debug!(
-                        "{self}: Received a block validation response for a different reward cycle. Ignore it.";
-                        "requested_reward_cycle" => block_info.reward_cycle,
-                    );
-                    return None;
-                }
-                if block_info.is_locally_finalized() {
-                    debug!("{self}: Received block validation for a block that is already marked as {}. Ignoring...", block_info.state);
-                    return None;
-                }
-                block_info
-            }
-            Ok(None) => {
-                // We have not seen this block before. Why are we getting a response for it?
-                debug!("{self}: Received a block validate response for a block we have not seen before. Ignoring...");
-                return None;
-            }
-            Err(e) => {
-                error!("{self}: Failed to lookup block in signer db: {e:?}",);
-                return None;
-            }
+        let Some(mut block_info) = self.block_lookup_by_reward_cycle(&signer_signature_hash) else {
+            // We have not seen this block before. Why are we getting a response for it?
+            debug!("{self}: Received a block validate response for a block we have not seen before. Ignoring...");
+            return None;
         };
+        if block_info.is_locally_finalized() {
+            debug!("{self}: Received block validation for a block that is already marked as {}. Ignoring...", block_info.state);
+            return None;
+        }
 
         if let Some(block_response) =
             self.check_block_against_signer_db_state(stacks_client, &block_info.block)
@@ -772,32 +762,15 @@ impl Signer {
         {
             self.submitted_block_proposal = None;
         }
-        let mut block_info = match self.signer_db.block_lookup(&signer_signature_hash) {
-            Ok(Some(block_info)) => {
-                if block_info.reward_cycle != self.reward_cycle {
-                    // We are not signing for this reward cycle. Ignore the block.
-                    debug!(
-                        "{self}: Received a block validation response for a different reward cycle. Ignore it.";
-                        "requested_reward_cycle" => block_info.reward_cycle,
-                    );
-                    return None;
-                }
-                if block_info.is_locally_finalized() {
-                    debug!("{self}: Received block validation for a block that is already marked as {}. Ignoring...", block_info.state);
-                    return None;
-                }
-                block_info
-            }
-            Ok(None) => {
-                // We have not seen this block before. Why are we getting a response for it?
-                debug!("{self}: Received a block validate response for a block we have not seen before. Ignoring...");
-                return None;
-            }
-            Err(e) => {
-                error!("{self}: Failed to lookup block in signer db: {e:?}");
-                return None;
-            }
+        let Some(mut block_info) = self.block_lookup_by_reward_cycle(&signer_signature_hash) else {
+            // We have not seen this block before. Why are we getting a response for it?
+            debug!("{self}: Received a block validate response for a block we have not seen before. Ignoring...");
+            return None;
         };
+        if block_info.is_locally_finalized() {
+            debug!("{self}: Received block validation for a block that is already marked as {}. Ignoring...", block_info.state);
+            return None;
+        }
         if let Err(e) = block_info.mark_locally_rejected() {
             if !block_info.has_reached_consensus() {
                 warn!("{self}: Failed to mark block as locally rejected: {e:?}",);
@@ -873,9 +846,7 @@ impl Signer {
         // For mutability reasons, we need to take the block_info out of the map and add it back after processing
         let mut block_info = match self.signer_db.block_lookup(&signature_sighash) {
             Ok(Some(block_info)) => {
-                if block_info.state == BlockState::GloballyRejected
-                    || block_info.state == BlockState::GloballyAccepted
-                {
+                if block_info.has_reached_consensus() {
                     // The block has already reached consensus.
                     return;
                 }
@@ -952,25 +923,16 @@ impl Signer {
         let block_hash = &rejection.signer_signature_hash;
         let signature = &rejection.signature;
 
-        let mut block_info = match self.signer_db.block_lookup(block_hash) {
-            Ok(Some(block_info)) => {
-                if block_info.state == BlockState::GloballyRejected
-                    || block_info.state == BlockState::GloballyAccepted
-                {
-                    debug!("{self}: Received block rejection for a block that is already marked as {}. Ignoring...", block_info.state);
-                    return;
-                }
-                block_info
-            }
-            Ok(None) => {
-                debug!("{self}: Received block rejection for a block we have not seen before. Ignoring...");
-                return;
-            }
-            Err(e) => {
-                warn!("{self}: Failed to load block state: {e:?}",);
-                return;
-            }
+        let Some(mut block_info) = self.block_lookup_by_reward_cycle(block_hash) else {
+            debug!(
+                "{self}: Received block rejection for a block we have not seen before. Ignoring..."
+            );
+            return;
         };
+        if block_info.has_reached_consensus() {
+            debug!("{self}: Received block rejection for a block that is already marked as {}. Ignoring...", block_info.state);
+            return;
+        }
 
         // recover public key
         let Ok(public_key) = rejection.recover_public_key() else {
@@ -1056,23 +1018,15 @@ impl Signer {
             "{self}: Received a block-accept signature: ({block_hash}, {signature}, {})",
             metadata.server_version
         );
-
-        // Have we already processed this block?
-        match self.signer_db.get_block_state(block_hash) {
-            Ok(Some(state)) => {
-                if state == BlockState::GloballyAccepted || state == BlockState::GloballyRejected {
-                    debug!("{self}: Received block signature for a block that is already marked as {}. Ignoring...", state);
-                    return;
-                }
-            }
-            Ok(None) => {
-                debug!("{self}: Received block signature for a block we have not seen before. Ignoring...");
-                return;
-            }
-            Err(e) => {
-                warn!("{self}: Failed to load block state: {e:?}",);
-                return;
-            }
+        let Some(mut block_info) = self.block_lookup_by_reward_cycle(block_hash) else {
+            debug!(
+                "{self}: Received block signature for a block we have not seen before. Ignoring..."
+            );
+            return;
+        };
+        if block_info.has_reached_consensus() {
+            debug!("{self}: Received block signature for a block that is already marked as {}. Ignoring...", block_info.state);
+            return;
         }
 
         // recover public key
@@ -1140,12 +1094,6 @@ impl Signer {
         }
 
         // have enough signatures to broadcast!
-        let Ok(Some(mut block_info)) = self.signer_db.block_lookup(block_hash).inspect_err(|e| {
-            warn!("{self}: Failed to load block {block_hash}: {e:?})");
-        }) else {
-            warn!("{self}: No such block {block_hash}");
-            return;
-        };
         // move block to LOCALLY accepted state.
         // It is only considered globally accepted IFF we receive a new block event confirming it OR see the chain tip of the node advance to it.
         if let Err(e) = block_info.mark_locally_accepted(true) {
@@ -1227,4 +1175,24 @@ impl Signer {
         error!("{self}: Failed to insert block into signer-db: {e:?}");
         panic!("{self} Failed to write block to signerdb: {e}");
     }
+
+    /// Helper for getting the block info from the db while accommodating for reward cycle
+    pub fn block_lookup_by_reward_cycle(
+        &self,
+        block_hash: &Sha512Trunc256Sum,
+    ) -> Option<BlockInfo> {
+        let block_info = self
+            .signer_db
+            .block_lookup(block_hash)
+            .inspect_err(|e| {
+                error!("{self}: Failed to lookup block hash {block_hash} in signer db: {e:?}");
+            })
+            .ok()
+            .flatten()?;
+        if block_info.reward_cycle == self.reward_cycle {
+            Some(block_info)
+        } else {
+            None
+        }
+    }
 }

stacks-signer/src/v0/tests.rs

@@ -45,6 +45,9 @@ pub static TEST_SKIP_BLOCK_BROADCAST: LazyLock<TestFlag<bool>> = LazyLock::new(T
 pub static TEST_STALL_BLOCK_VALIDATION_SUBMISSION: LazyLock<TestFlag<bool>> =
     LazyLock::new(TestFlag::default);
 
+/// A global variable that can be used to prevent signer cleanup
+pub static TEST_SKIP_SIGNER_CLEANUP: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
+
 impl Signer {
     /// Skip the block broadcast if the TEST_SKIP_BLOCK_BROADCAST flag is set
     pub fn test_skip_block_broadcast(&self, block: &NakamotoBlock) -> bool {