Merge pull request #5686 from stacks-network/fix/mark-any-miners-proposal-towards-activity

[Signer] Make any miner's block proposal that passes initial checks count towards miner activity/validity

Author: hstove

.github/workflows/bitcoin-tests.yml

@@ -141,6 +141,8 @@ jobs:
           - 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::signer::v0::block_proposal_timeout
+          - tests::signer::v0::rejected_blocks_count_towards_miner_validity
           - tests::nakamoto_integrations::burn_ops_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state

stacks-signer/src/chainstate.rs

@@ -89,10 +89,8 @@ impl SortitionState {
         if self.miner_status != SortitionMinerStatus::Valid {
             return Ok(false);
         }
-        // if we've already signed a block in this tenure, the miner can't have timed out.
-        let has_blocks = signer_db
-            .get_last_signed_block_in_tenure(&self.consensus_hash)?
-            .is_some();
+        // if we've already seen a proposed block from this miner. It cannot have timed out.
+        let has_blocks = signer_db.has_proposed_block_in_tenure(&self.consensus_hash)?;
         if has_blocks {
             return Ok(false);
         }

stacks-signer/src/signerdb.rs

@@ -746,15 +746,13 @@ impl SignerDb {
         try_deserialize(result)
     }
 
-    /// Return the last signed block in a tenure (identified by its consensus hash)
-    pub fn get_last_signed_block_in_tenure(
-        &self,
-        tenure: &ConsensusHash,
-    ) -> Result<Option<BlockInfo>, DBError> {
-        let query = "SELECT block_info FROM blocks WHERE consensus_hash = ? AND signed_over = 1 ORDER BY stacks_height DESC LIMIT 1";
+    /// Return whether a block proposal has been stored for a tenure (identified by its consensus hash)
+    /// Does not consider the block's state.
+    pub fn has_proposed_block_in_tenure(&self, tenure: &ConsensusHash) -> Result<bool, DBError> {
+        let query = "SELECT block_info FROM blocks WHERE consensus_hash = ? LIMIT 1";
         let result: Option<String> = query_row(&self.db, query, [tenure])?;
 
-        try_deserialize(result)
+        Ok(result.is_some())
     }
 
     /// Return the first signed block in a tenure (identified by its consensus hash)
@@ -1904,4 +1902,28 @@ mod tests {
         let pendings = db.get_all_pending_block_validations().unwrap();
         assert_eq!(pendings.len(), 0);
     }
+
+    #[test]
+    fn has_proposed_block() {
+        let db_path = tmp_db_path();
+        let consensus_hash_1 = ConsensusHash([0x01; 20]);
+        let consensus_hash_2 = ConsensusHash([0x02; 20]);
+        let mut db = SignerDb::new(db_path).expect("Failed to create signer db");
+        let (mut block_info, _) = create_block_override(|b| {
+            b.block.header.consensus_hash = consensus_hash_1;
+            b.block.header.chain_length = 1;
+        });
+
+        assert!(!db.has_proposed_block_in_tenure(&consensus_hash_1).unwrap());
+        assert!(!db.has_proposed_block_in_tenure(&consensus_hash_2).unwrap());
+
+        db.insert_block(&block_info).unwrap();
+
+        block_info.block.header.chain_length = 2;
+
+        db.insert_block(&block_info).unwrap();
+
+        assert!(db.has_proposed_block_in_tenure(&consensus_hash_1).unwrap());
+        assert!(!db.has_proposed_block_in_tenure(&consensus_hash_2).unwrap());
+    }
 }

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

@@ -13,6 +13,8 @@
 //
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#[cfg(test)]
+use std::sync::LazyLock;
 use std::thread;
 use std::thread::JoinHandle;
 use std::time::{Duration, Instant};
@@ -42,6 +44,8 @@ use stacks::net::stackerdb::StackerDBs;
 use stacks::net::{NakamotoBlocksData, StacksMessageType};
 use stacks::util::get_epoch_time_secs;
 use stacks::util::secp256k1::MessageSignature;
+#[cfg(test)]
+use stacks::util::tests::TestFlag;
 use stacks_common::types::chainstate::{StacksAddress, StacksBlockId};
 use stacks_common::types::{PrivateKey, StacksEpochId};
 use stacks_common::util::vrf::VRFProof;
@@ -55,9 +59,11 @@ use crate::run_loop::nakamoto::Globals;
 use crate::run_loop::RegisteredKey;
 
 #[cfg(test)]
-pub static TEST_MINE_STALL: std::sync::Mutex<Option<bool>> = std::sync::Mutex::new(None);
+/// Test flag to stall the miner thread
+pub static TEST_MINE_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
 #[cfg(test)]
-pub static TEST_BROADCAST_STALL: std::sync::Mutex<Option<bool>> = std::sync::Mutex::new(None);
+/// Test flag to stall block proposal broadcasting
+pub static TEST_BROADCAST_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
 #[cfg(test)]
 pub static TEST_BLOCK_ANNOUNCE_STALL: std::sync::Mutex<Option<bool>> = std::sync::Mutex::new(None);
 #[cfg(test)]
@@ -195,15 +201,15 @@ impl BlockMinerThread {
 
     #[cfg(test)]
     fn fault_injection_block_broadcast_stall(new_block: &NakamotoBlock) {
-        if *TEST_BROADCAST_STALL.lock().unwrap() == Some(true) {
+        if TEST_BROADCAST_STALL.get() {
             // Do an extra check just so we don't log EVERY time.
             warn!("Fault injection: 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) {
+            while TEST_BROADCAST_STALL.get() {
                 std::thread::sleep(std::time::Duration::from_millis(10));
             }
             info!("Fault injection: Broadcasting is no longer stalled due to testing directive.";
@@ -356,10 +362,10 @@ impl BlockMinerThread {
         reward_set: &RewardSet,
     ) -> Result<(), NakamotoNodeError> {
         #[cfg(test)]
-        if *TEST_MINE_STALL.lock().unwrap() == Some(true) {
+        if TEST_MINE_STALL.get() {
             // Do an extra check just so we don't log EVERY time.
             warn!("Mining is stalled due to testing directive");
-            while *TEST_MINE_STALL.lock().unwrap() == Some(true) {
+            while TEST_MINE_STALL.get() {
                 std::thread::sleep(std::time::Duration::from_millis(10));
             }
             warn!("Mining is no longer stalled due to testing directive. Continuing...");

testnet/stacks-node/src/tests/nakamoto_integrations.rs

@@ -4992,7 +4992,7 @@ fn forked_tenure_is_ignored() {
 
     // For the next tenure, submit the commit op but do not allow any stacks blocks to be broadcasted.
     // Stall the miner thread; only wait until the number of submitted commits increases.
-    TEST_BROADCAST_STALL.lock().unwrap().replace(true);
+    TEST_BROADCAST_STALL.set(true);
     TEST_BLOCK_ANNOUNCE_STALL.lock().unwrap().replace(true);
     let blocks_before = mined_blocks.load(Ordering::SeqCst);
     let commits_before = commits_submitted.load(Ordering::SeqCst);
@@ -5010,7 +5010,7 @@ fn forked_tenure_is_ignored() {
     // Unpause the broadcast of Tenure B's block, do not submit commits, and do not allow blocks to
     // be processed
     test_skip_commit_op.set(true);
-    TEST_BROADCAST_STALL.lock().unwrap().replace(false);
+    TEST_BROADCAST_STALL.set(false);
 
     // Wait for a stacks block to be broadcasted.
     // However, it will not be processed.
@@ -6101,7 +6101,7 @@ fn clarity_burn_state() {
             result.expect_result_ok().expect("Read-only call failed");
 
             // Pause mining to prevent the stacks block from being mined before the tenure change is processed
-            TEST_MINE_STALL.lock().unwrap().replace(true);
+            TEST_MINE_STALL.set(true);
             // Submit a tx for the next block (the next block will be a new tenure, so the burn block height will increment)
             let call_tx = tests::make_contract_call(
                 &sender_sk,
@@ -6126,7 +6126,7 @@ fn clarity_burn_state() {
             Ok(commits_submitted.load(Ordering::SeqCst) > commits_before)
         })
         .unwrap();
-        TEST_MINE_STALL.lock().unwrap().replace(false);
+        TEST_MINE_STALL.set(false);
         wait_for(20, || {
             Ok(coord_channel
                 .lock()
@@ -10409,7 +10409,7 @@ fn clarity_cost_spend_down() {
             .expect("Mutex poisoned")
             .get_stacks_blocks_processed();
         // Pause mining so we can add all our transactions to the mempool at once.
-        TEST_MINE_STALL.lock().unwrap().replace(true);
+        TEST_MINE_STALL.set(true);
         let mut submitted_txs = vec![];
         for _nmb_tx in 0..nmb_txs_per_signer {
             for sender_sk in sender_sks.iter() {
@@ -10438,7 +10438,7 @@ fn clarity_cost_spend_down() {
                 }
             }
         }
-        TEST_MINE_STALL.lock().unwrap().replace(false);
+        TEST_MINE_STALL.set(false);
         wait_for(120, || {
             let blocks_processed = coord_channel
                 .lock()