Merge pull request #5674 from stacks-network/fix/bitcoind-forking-test-signerdb-state

Fix/bitcoind forking test signerdb state

Author: kantai

stacks-signer/src/chainstate.rs

@@ -539,7 +539,7 @@ impl SortitionsView {
     ///
     /// 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.
-    fn check_tenure_change_confirms_parent(
+    pub fn check_tenure_change_confirms_parent(
         tenure_change: &TenureChangePayload,
         block: &NakamotoBlock,
         signer_db: &mut SignerDb,

stacks-signer/src/signerdb.rs

@@ -733,18 +733,6 @@ impl SignerDb {
         try_deserialize(result)
     }
 
-    /// Return the last accepted block the signer (highest stacks height). It will tie break a match based on which was more recently signed.
-    pub fn get_signer_last_accepted_block(&self) -> Result<Option<BlockInfo>, DBError> {
-        let query = "SELECT block_info FROM blocks WHERE state IN (?1, ?2) ORDER BY stacks_height DESC, signed_group DESC, signed_self DESC LIMIT 1";
-        let args = params![
-            &BlockState::GloballyAccepted.to_string(),
-            &BlockState::LocallyAccepted.to_string()
-        ];
-        let result: Option<String> = query_row(&self.db, query, args)?;
-
-        try_deserialize(result)
-    }
-
     /// Return the last accepted block in a tenure (identified by its consensus hash).
     pub fn get_last_accepted_block(
         &self,
@@ -1769,69 +1757,4 @@ mod tests {
                 < block_infos[0].proposed_time
         );
     }
-
-    #[test]
-    fn signer_last_accepted_block() {
-        let db_path = tmp_db_path();
-        let mut db = SignerDb::new(db_path).expect("Failed to create signer db");
-
-        let (mut block_info_1, _block_proposal_1) = create_block_override(|b| {
-            b.block.header.miner_signature = MessageSignature([0x01; 65]);
-            b.block.header.chain_length = 1;
-            b.burn_height = 1;
-        });
-
-        let (mut block_info_2, _block_proposal_2) = create_block_override(|b| {
-            b.block.header.miner_signature = MessageSignature([0x02; 65]);
-            b.block.header.chain_length = 2;
-            b.burn_height = 1;
-        });
-
-        let (mut block_info_3, _block_proposal_3) = create_block_override(|b| {
-            b.block.header.miner_signature = MessageSignature([0x02; 65]);
-            b.block.header.chain_length = 2;
-            b.burn_height = 4;
-        });
-        block_info_3
-            .mark_locally_accepted(false)
-            .expect("Failed to mark block as locally accepted");
-
-        db.insert_block(&block_info_1)
-            .expect("Unable to insert block into db");
-        db.insert_block(&block_info_2)
-            .expect("Unable to insert block into db");
-
-        assert!(db.get_signer_last_accepted_block().unwrap().is_none());
-
-        block_info_1
-            .mark_globally_accepted()
-            .expect("Failed to mark block as globally accepted");
-        db.insert_block(&block_info_1)
-            .expect("Unable to insert block into db");
-
-        assert_eq!(
-            db.get_signer_last_accepted_block().unwrap().unwrap(),
-            block_info_1
-        );
-
-        block_info_2
-            .mark_globally_accepted()
-            .expect("Failed to mark block as globally accepted");
-        block_info_2.signed_self = Some(get_epoch_time_secs());
-        db.insert_block(&block_info_2)
-            .expect("Unable to insert block into db");
-
-        assert_eq!(
-            db.get_signer_last_accepted_block().unwrap().unwrap(),
-            block_info_2
-        );
-
-        db.insert_block(&block_info_3)
-            .expect("Unable to insert block into db");
-
-        assert_eq!(
-            db.get_signer_last_accepted_block().unwrap().unwrap(),
-            block_info_3
-        );
-    }
 }

stacks-signer/src/v0/signer.rs

@@ -582,60 +582,80 @@ impl Signer {
         }
     }
 
-    /// WARNING: Do NOT call this function PRIOR to check_proposal or block_proposal validation succeeds.
+    /// WARNING: This is an incomplete check. Do NOT call this function PRIOR to check_proposal or block_proposal validation succeeds.
     ///
     /// Re-verify a block's chain length against the last signed block within signerdb.
     /// This is required in case a block has been approved since the initial checks of the block validation endpoint.
     fn check_block_against_signer_db_state(
-        &self,
+        &mut self,
+        stacks_client: &StacksClient,
         proposed_block: &NakamotoBlock,
     ) -> Option<BlockResponse> {
         let signer_signature_hash = proposed_block.header.signer_signature_hash();
         let proposed_block_consensus_hash = proposed_block.header.consensus_hash;
+        // If this is a tenure change block, ensure that it confirms the correct number of blocks from the parent tenure.
+        if let Some(tenure_change) = proposed_block.get_tenure_change_tx_payload() {
+            // Ensure that the tenure change block confirms the expected parent block
+            match SortitionsView::check_tenure_change_confirms_parent(
+                tenure_change,
+                proposed_block,
+                &mut self.signer_db,
+                stacks_client,
+                self.proposal_config.tenure_last_block_proposal_timeout,
+            ) {
+                Ok(true) => {}
+                Ok(false) => {
+                    return Some(
+                        self.create_block_rejection(
+                            RejectCode::SortitionViewMismatch,
+                            proposed_block,
+                        ),
+                    )
+                }
+                Err(e) => {
+                    warn!("{self}: Error checking block proposal: {e}";
+                        "signer_sighash" => %signer_signature_hash,
+                        "block_id" => %proposed_block.block_id()
+                    );
+                    return Some(
+                        self.create_block_rejection(RejectCode::ConnectivityIssues, proposed_block),
+                    );
+                }
+            }
+        }
 
-        match self.signer_db.get_signer_last_accepted_block() {
+        // Ensure that the block is the last block in the chain of its current tenure.
+        match self
+            .signer_db
+            .get_last_accepted_block(&proposed_block_consensus_hash)
+        {
             Ok(Some(last_block_info)) => {
                 if proposed_block.header.chain_length <= last_block_info.block.header.chain_length {
-                    // We do not allow reorgs at any time within the same consensus hash OR of globally accepted blocks
-                    let non_reorgable_block = last_block_info.block.header.consensus_hash
-                        == proposed_block_consensus_hash
-                        || last_block_info.state == BlockState::GloballyAccepted;
-                    // Is the reorg timeout requirement exceeded?
-                    let reorg_timeout_exceeded = last_block_info
-                        .signed_self
-                        .map(|signed_over_time| {
-                            signed_over_time.saturating_add(
-                                self.proposal_config
-                                    .tenure_last_block_proposal_timeout
-                                    .as_secs(),
-                            ) <= get_epoch_time_secs()
-                        })
-                        .unwrap_or(false);
-                    if non_reorgable_block || !reorg_timeout_exceeded {
-                        warn!(
-                            "Miner's block proposal does not confirm as many blocks as we expect";
-                            "proposed_block_consensus_hash" => %proposed_block_consensus_hash,
-                            "proposed_block_signer_sighash" => %signer_signature_hash,
-                            "proposed_chain_length" => proposed_block.header.chain_length,
-                            "expected_at_least" => last_block_info.block.header.chain_length + 1,
-                        );
-                        return Some(self.create_block_rejection(
-                            RejectCode::SortitionViewMismatch,
-                            proposed_block,
-                        ));
-                    }
+                    warn!(
+                        "Miner's block proposal does not confirm as many blocks as we expect";
+                        "proposed_block_consensus_hash" => %proposed_block_consensus_hash,
+                        "proposed_block_signer_sighash" => %signer_signature_hash,
+                        "proposed_chain_length" => proposed_block.header.chain_length,
+                        "expected_at_least" => last_block_info.block.header.chain_length + 1,
+                    );
+                    return Some(self.create_block_rejection(
+                        RejectCode::SortitionViewMismatch,
+                        proposed_block,
+                    ));
                 }
-                None
             }
-            Ok(_) => None,
+            Ok(_) => {}
             Err(e) => {
                 warn!("{self}: Failed to check block against signer db: {e}";
                     "signer_sighash" => %signer_signature_hash,
                     "block_id" => %proposed_block.block_id()
                 );
-                Some(self.create_block_rejection(RejectCode::ConnectivityIssues, proposed_block))
+                return Some(
+                    self.create_block_rejection(RejectCode::ConnectivityIssues, proposed_block),
+                );
             }
         }
+        None
     }
 
     /// Handle the block validate ok response. Returns our block response if we have one
@@ -684,7 +704,9 @@ impl Signer {
             }
         };
 
-        if let Some(block_response) = self.check_block_against_signer_db_state(&block_info.block) {
+        if let Some(block_response) =
+            self.check_block_against_signer_db_state(stacks_client, &block_info.block)
+        {
             // The signer db state has changed. We no longer view this block as valid. Override the validation response.
             if let Err(e) = block_info.mark_locally_rejected() {
                 if !block_info.has_reached_consensus() {