Add a test and fix failing tests

Signed-off-by: Jacinta Ferrant <[email protected]>

Author: jferrant

stacks-node/src/tests/signer/v0.rs

@@ -94,7 +94,8 @@ use stacks_signer::v0::signer_state::{
 use stacks_signer::v0::tests::{
     TEST_IGNORE_ALL_BLOCK_PROPOSALS, TEST_PAUSE_BLOCK_BROADCAST,
     TEST_PIN_SUPPORTED_SIGNER_PROTOCOL_VERSION, TEST_REJECT_ALL_BLOCK_PROPOSAL,
-    TEST_SKIP_BLOCK_BROADCAST, TEST_SKIP_SIGNER_CLEANUP, TEST_STALL_BLOCK_VALIDATION_SUBMISSION,
+    TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST, TEST_SKIP_BLOCK_BROADCAST, TEST_SKIP_SIGNER_CLEANUP,
+    TEST_STALL_BLOCK_VALIDATION_SUBMISSION,
 };
 use stacks_signer::v0::SpawnedSigner;
 use tracing_subscriber::prelude::*;
@@ -1328,6 +1329,37 @@ pub fn wait_for_block_pushed_by_miner_key(
     block.ok_or_else(|| "Failed to find block pushed".to_string())
 }
 
+/// Waits for all of the provided signers to send a pre-commit for a block
+/// with the provided signer signature hash
+pub fn wait_for_block_pre_commits_from_signers(
+    timeout_secs: u64,
+    signer_signature_hash: &Sha512Trunc256Sum,
+    expected_signers: &[StacksPublicKey],
+) -> Result<(), String> {
+    wait_for(timeout_secs, || {
+        let chunks = test_observer::get_stackerdb_chunks()
+            .into_iter()
+            .flat_map(|chunk| chunk.modified_slots)
+            .filter_map(|chunk| {
+                let pk = chunk.recover_pk().expect("Failed to recover pk");
+                if !expected_signers.contains(&pk) {
+                    return None;
+                }
+                let message = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice())
+                    .expect("Failed to deserialize SignerMessage");
+
+                if let SignerMessage::BlockPreCommit(hash) = message {
+                    if hash == *signer_signature_hash {
+                        return Some(pk);
+                    }
+                }
+                None
+            })
+            .collect::<HashSet<_>>();
+        Ok(chunks.len() == expected_signers.len())
+    })
+}
+
 /// Waits for >30% of num_signers block rejection to be observed in the test_observer stackerdb chunks for a block
 /// with the provided signer signature hash
 fn wait_for_block_global_rejection(
@@ -1862,22 +1894,47 @@ fn miner_gather_signatures() {
     // Test prometheus metrics response
     #[cfg(feature = "monitoring_prom")]
     {
+        let min_num_expected = (num_signers * 2) as u64;
         wait_for(30, || {
+            use regex::Regex;
+
             let metrics_response = signer_test.get_signer_metrics();
+            let re_precommits =
+                Regex::new(r#"stacks_signer_block_pre_commits_sent (\d+)"#).unwrap();
+            let re_proposals =
+                Regex::new(r#"stacks_signer_block_proposals_received (\d+)"#).unwrap();
+            let re_responses = Regex::new(
+                r#"stacks_signer_block_responses_sent\{response_type="accepted"\} (\d+)"#,
+            )
+            .unwrap();
 
-            // Because 5 signers are running in the same process, the prometheus metrics
-            // are incremented once for every signer.When booting to Epoch 3.0, the old
-            // miner will attempt to propose a block before its burnchain tip has updated
-            // causing an additional block proposal that gets rejected due to consensus hash
-            // mismatch, hence why we expect 15 rather than just 10 proposals.
-            let expected_result_1 =
-                format!("stacks_signer_block_proposals_received {}", num_signers * 2);
-            let expected_result_2 = format!(
-                "stacks_signer_block_responses_sent{{response_type=\"accepted\"}} {}",
-                num_signers * 2
-            );
-            Ok(metrics_response.contains(&expected_result_1)
-                && metrics_response.contains(&expected_result_2))
+            let precommits = re_precommits
+                .captures(&metrics_response)
+                .and_then(|caps| caps.get(1))
+                .map(|m| m.as_str().parse::<u64>().ok())
+                .flatten();
+
+            let proposals = re_proposals
+                .captures(&metrics_response)
+                .and_then(|caps| caps.get(1))
+                .map(|m| m.as_str().parse::<u64>().ok())
+                .flatten();
+
+            let responses = re_responses
+                .captures(&metrics_response)
+                .and_then(|caps| caps.get(1))
+                .map(|m| m.as_str().parse::<u64>().ok())
+                .flatten();
+
+            if let (Some(proposals), Some(responses), Some(precommits)) =
+                (proposals, responses, precommits)
+            {
+                Ok(proposals >= min_num_expected
+                    && responses >= min_num_expected
+                    && precommits >= min_num_expected)
+            } else {
+                Ok(false)
+            }
         })
         .expect("Failed to advance prometheus metrics");
     }
@@ -9249,7 +9306,7 @@ fn locally_rejected_blocks_overriden_by_global_acceptance() {
 /// The stacks node is then advanced to Epoch 3.0 boundary to allow block signing.
 ///
 /// Test Execution:
-/// The node mines 1 stacks block N (all signers sign it). The subsequent block N+1 is proposed, but <30% accept it. The remaining signers
+/// The node mines 1 stacks block N (all signers sign it). The subsequent block N+1 is proposed, but <30% pre-commit to it. The remaining signers
 /// do not make a decision on the block. A new tenure begins and the miner proposes a new block N+1' which all signers accept.
 ///
 /// Test Assertion:
@@ -9334,7 +9391,7 @@ fn reorg_locally_accepted_blocks_across_tenures_succeeds() {
         .cloned()
         .skip(num_signers * 7 / 10)
         .collect();
-    TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(ignoring_signers.clone());
+    TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.set(ignoring_signers.clone());
     // Clear the stackerdb chunks
     test_observer::clear();
 
@@ -9354,12 +9411,12 @@ fn reorg_locally_accepted_blocks_across_tenures_succeeds() {
         wait_for_block_proposal(30, info_before.stacks_tip_height + 1, &miner_pk)
             .expect("Timed out waiting for block N+1 to be proposed");
     // Make sure that the non ignoring signers do actually accept it though
-    wait_for_block_acceptance_from_signers(
+    wait_for_block_pre_commits_from_signers(
         30,
         &block_n_1_proposal.header.signer_signature_hash(),
         &non_ignoring_signers,
     )
-    .expect("Timed out waiting for block acceptances of N+1");
+    .expect("Timed out waiting for block pre-commits of N+1");
     let info_after = signer_test.get_peer_info();
     assert_eq!(info_after, info_before);
     assert_ne!(
@@ -9402,7 +9459,7 @@ fn reorg_locally_accepted_blocks_across_tenures_succeeds() {
     );
     let info_before = signer_test.get_peer_info();
     test_observer::clear();
-    TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(Vec::new());
+    TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.set(Vec::new());
     TEST_MINE_SKIP.set(false);
 
     let block_n_1_prime =
@@ -9546,7 +9603,7 @@ fn reorg_locally_accepted_blocks_across_tenures_fails() {
         .cloned()
         .skip(num_signers * 7 / 10)
         .collect();
-    TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(ignoring_signers.clone());
+    TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.set(ignoring_signers.clone());
     // Clear the stackerdb chunks
     test_observer::clear();
 
@@ -12740,7 +12797,7 @@ fn injected_signatures_are_ignored_across_boundaries() {
         .collect();
     assert_eq!(ignoring_signers.len(), 3);
     assert_eq!(non_ignoring_signers.len(), 2);
-    TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(ignoring_signers.clone());
+    TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.set(ignoring_signers.clone());
 
     let info_before = signer_test.get_peer_info();
     // submit a tx so that the miner will ATTEMPT to mine a stacks block N
@@ -15529,10 +15586,8 @@ fn mark_miner_as_invalid_if_reorg_is_rejected_v1() {
         .signer_test
         .check_signer_states_reorg(&approving_signers, &rejecting_signers);
 
-    info!("------------------------- Wait for 3 acceptances and 2 rejections -------------------------");
     let signer_signature_hash = block_n_1_prime.header.signer_signature_hash();
-    wait_for_block_acceptance_from_signers(30, &signer_signature_hash, &approving_signers)
-        .expect("Timed out waiting for block acceptance from approving signers");
+    info!("------------------------- Wait for 3 acceptances and 2 rejections of {signer_signature_hash} -------------------------");
     let rejections =
         wait_for_block_rejections_from_signers(30, &signer_signature_hash, &rejecting_signers)
             .expect("Timed out waiting for block rejection from rejecting signers");
@@ -15543,6 +15598,8 @@ fn mark_miner_as_invalid_if_reorg_is_rejected_v1() {
             "Reject reason is not ReorgNotAllowed"
         );
     }
+    wait_for_block_pre_commits_from_signers(30, &signer_signature_hash, &approving_signers)
+        .expect("Timed out waiting for block pre-commits from approving signers");
 
     info!("------------------------- Miner 1 Proposes N+1' Again -------------------------");
     test_observer::clear();
@@ -18077,3 +18134,74 @@ fn bitcoin_reorg_extended_tenure() {
 
     miners.shutdown();
 }
+
+// Basic test to ensure that signers will not issue a signature over a block proposal unless
+// a threshold number of signers have pre-committed to sign.
+#[test]
+#[ignore]
+fn signers_do_not_commit_unless_threshold_precommitted() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    info!("------------------------- Test Setup -------------------------");
+    let num_signers = 20;
+
+    let mut signer_test: SignerTest<SpawnedSigner> = SignerTest::new(num_signers, vec![]);
+    let miner_sk = signer_test.running_nodes.conf.miner.mining_key.unwrap();
+    let miner_pk = StacksPublicKey::from_private(&miner_sk);
+    let all_signers = signer_test.signer_test_pks();
+
+    signer_test.boot_to_epoch_3();
+
+    // Make sure that more than 30% of signers are set to ignore any incoming proposals so that consensus is not reached
+    // on pre-commit round.
+    let ignore_signers: Vec<_> = all_signers
+        .iter()
+        .cloned()
+        .take(all_signers.len() / 2)
+        .collect();
+    let pre_commit_signers: Vec<_> = all_signers
+        .iter()
+        .cloned()
+        .skip(all_signers.len() / 2)
+        .collect();
+    TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(ignore_signers);
+    test_observer::clear();
+    let blocks_before = test_observer::get_mined_nakamoto_blocks().len();
+    let height_before = signer_test.get_peer_info().stacks_tip_height;
+    next_block_and(
+        &mut signer_test.running_nodes.btc_regtest_controller,
+        30,
+        || Ok(test_observer::get_mined_nakamoto_blocks().len() > blocks_before),
+    )
+    .unwrap();
+
+    let proposal = wait_for_block_proposal(30, height_before + 1, &miner_pk)
+        .expect("Timed out waiting for block proposal");
+    let hash = proposal.header.signer_signature_hash();
+    wait_for_block_pre_commits_from_signers(30, &hash, &pre_commit_signers)
+        .expect("Timed out waiting for pre-commits");
+    assert!(
+        wait_for(30, || {
+            for chunk in test_observer::get_stackerdb_chunks()
+                .into_iter()
+                .flat_map(|chunk| chunk.modified_slots)
+            {
+                let message = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice())
+                    .expect("Failed to deserialize SignerMessage");
+                if let SignerMessage::BlockResponse(BlockResponse::Accepted(accepted)) = message {
+                    if accepted.signer_signature_hash == hash {
+                        return Ok(true);
+                    }
+                }
+            }
+            Ok(false)
+        })
+        .is_err(),
+        "Should not have found a single block accept for the block hash {hash}"
+    );
+
+    info!("------------------------- Shutdown -------------------------");
+    signer_test.shutdown();
+}

stacks-signer/src/v0/signer.rs

@@ -908,6 +908,9 @@ impl Signer {
 
     #[cfg(any(test, feature = "testing"))]
     fn send_block_response(&mut self, block: &NakamotoBlock, block_response: BlockResponse) {
+        if self.test_skip_signature_broadcast(&block_response) {
+            return;
+        }
         const NUM_REPEATS: usize = 1;
         let mut count = 0;
         let public_keys = TEST_REPEAT_PROPOSAL_RESPONSE.get();
@@ -1062,7 +1065,7 @@ impl Signer {
         let accepted = self.create_block_acceptance(&block_info.block);
         // have to save the signature _after_ the block info
         self.handle_block_signature(stacks_client, &accepted);
-        self.impl_send_block_response(&block_info.block, accepted.into());
+        self.send_block_response(&block_info.block, accepted.into());
     }
 
     /// Handle block proposal messages submitted to signers stackerdb
@@ -1216,7 +1219,7 @@ impl Signer {
             return;
         };
 
-        self.impl_send_block_response(&block_info.block, block_response);
+        self.send_block_response(&block_info.block, block_response);
     }
 
     /// Handle block response messages from a signer
@@ -1374,7 +1377,7 @@ impl Signer {
                 .insert_block(&block_info)
                 .unwrap_or_else(|e| self.handle_insert_block_error(e));
             self.handle_block_rejection(&block_rejection, sortition_state);
-            self.impl_send_block_response(&block_info.block, block_rejection.into());
+            self.send_block_response(&block_info.block, block_rejection.into());
         } else {
             if let Err(e) = block_info.mark_locally_accepted(false) {
                 if !block_info.has_reached_consensus() {
@@ -1448,7 +1451,7 @@ impl Signer {
             .insert_block(&block_info)
             .unwrap_or_else(|e| self.handle_insert_block_error(e));
         self.handle_block_rejection(&block_rejection, sortition_state);
-        self.impl_send_block_response(&block_info.block, block_rejection.into());
+        self.send_block_response(&block_info.block, block_rejection.into());
     }
 
     /// Handle the block validate response returned from our prior calls to submit a block for validation
@@ -1566,7 +1569,7 @@ impl Signer {
                 warn!("{self}: Failed to mark block as locally rejected: {e:?}");
             }
         };
-        self.impl_send_block_response(&block_info.block, rejection.into());
+        self.send_block_response(&block_info.block, rejection.into());
 
         self.signer_db
             .insert_block(&block_info)

stacks-signer/src/v0/tests.rs

@@ -17,7 +17,7 @@ use std::collections::HashMap;
 use std::sync::LazyLock;
 
 use blockstack_lib::chainstate::nakamoto::NakamotoBlock;
-use libsigner::v0::messages::{BlockRejection, RejectReason};
+use libsigner::v0::messages::{BlockRejection, BlockResponse, RejectReason};
 use libsigner::BlockProposal;
 use stacks_common::types::chainstate::StacksPublicKey;
 use stacks_common::util::get_epoch_time_secs;
@@ -53,6 +53,10 @@ pub static TEST_STALL_BLOCK_VALIDATION_SUBMISSION: LazyLock<TestFlag<bool>> =
 /// A global variable that can be used to prevent signer cleanup
 pub static TEST_SKIP_SIGNER_CLEANUP: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
 
+/// A global variable that can be used to skip signature broadcast if the signer's public key is in the provided list
+pub static TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST: LazyLock<TestFlag<Vec<StacksPublicKey>>> =
+    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 {
@@ -169,4 +173,23 @@ impl Signer {
         }
         self.supported_signer_protocol_version
     }
+
+    /// Skip the block broadcast if the TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST flag is set for the signer
+    pub fn test_skip_signature_broadcast(&self, block_response: &BlockResponse) -> bool {
+        if block_response.as_block_accepted().is_none() {
+            return false;
+        }
+        let hash = block_response.get_signer_signature_hash();
+        let public_keys = TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.get();
+        if public_keys.contains(
+            &stacks_common::types::chainstate::StacksPublicKey::from_private(&self.private_key),
+        ) {
+            warn!(
+                "{self}: Skipping signature broadcast due to testing directive";
+                "signer_signature_hash" => %hash
+            );
+            return true;
+        }
+        false
+    }
 }