Merge pull request #5497 from stacks-network/bug/validation-queue-race-condition

Fix validation queue race condition in block approval vs validaiton submission

Author: jferrant

.github/workflows/bitcoin-tests.yml

@@ -135,6 +135,7 @@ jobs:
           - tests::signer::v0::tenure_extend_after_bad_commit
           - 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::nakamoto_integrations::burn_ops_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state

libsigner/src/v0/messages.rs

@@ -686,6 +686,22 @@ impl BlockResponse {
             BlockResponse::Rejected(rejection) => rejection.signer_signature_hash,
         }
     }
+
+    /// Get the block accept data from the block response
+    pub fn as_block_accepted(&self) -> Option<&BlockAccepted> {
+        match self {
+            BlockResponse::Accepted(accepted) => Some(accepted),
+            _ => None,
+        }
+    }
+
+    /// Get the block accept data from the block response
+    pub fn as_block_rejection(&self) -> Option<&BlockRejection> {
+        match self {
+            BlockResponse::Rejected(rejection) => Some(rejection),
+            _ => None,
+        }
+    }
 }
 
 impl StacksMessageCodec for BlockResponse {

stacks-signer/src/chainstate.rs

@@ -505,7 +505,7 @@ impl SortitionsView {
 
     /// Get the last block from the given tenure
     /// Returns the last locally accepted block if it is not timed out, otherwise it will return the last globally accepted block.
-    fn get_tenure_last_block_info(
+    pub fn get_tenure_last_block_info(
         consensus_hash: &ConsensusHash,
         signer_db: &SignerDb,
         tenure_last_block_proposal_timeout: Duration,
@@ -517,7 +517,7 @@ impl SortitionsView {
 
         if let Some(local_info) = last_locally_accepted_block {
             if let Some(signed_over_time) = local_info.signed_self {
-                if signed_over_time + tenure_last_block_proposal_timeout.as_secs()
+                if signed_over_time.saturating_add(tenure_last_block_proposal_timeout.as_secs())
                     > get_epoch_time_secs()
                 {
                     // The last locally accepted block is not timed out, return it

stacks-signer/src/signerdb.rs

@@ -733,6 +733,18 @@ 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,
@@ -1757,4 +1769,69 @@ 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/tests/chainstate.rs

@@ -452,8 +452,12 @@ fn check_sortition_timeout() {
     fs::create_dir_all(signer_db_dir).unwrap();
     let mut signer_db = SignerDb::new(signer_db_path).unwrap();
 
+    let block_sk = StacksPrivateKey::from_seed(&[0, 1]);
+    let block_pk = StacksPublicKey::from_private(&block_sk);
+    let block_pkh = Hash160::from_node_public_key(&block_pk);
+
     let mut sortition = SortitionState {
-        miner_pkh: Hash160([0; 20]),
+        miner_pkh: block_pkh,
         miner_pubkey: None,
         prior_sortition: ConsensusHash([0; 20]),
         parent_tenure_id: ConsensusHash([0; 20]),