Fix test to confirm reorg accross boundaries is possible if block not announced by the node

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

Author: jferrant

stacks-common/src/util/mod.rs

@@ -35,6 +35,30 @@ use std::path::Path;
 use std::time::{SystemTime, UNIX_EPOCH};
 use std::{error, fmt, thread, time};
 
+#[cfg(any(test, feature = "testing"))]
+#[derive(Clone)]
+pub struct TestFlag<T>(pub std::sync::Arc<std::sync::Mutex<Option<T>>>);
+
+#[cfg(any(test, feature = "testing"))]
+impl<T: Default + Clone> Default for TestFlag<T> {
+    fn default() -> Self {
+        Self(std::sync::Arc::new(std::sync::Mutex::new(None)))
+    }
+}
+
+#[cfg(any(test, feature = "testing"))]
+impl<T: Default + Clone> TestFlag<T> {
+    /// Set the test flag to the given value
+    pub fn set(&self, value: T) {
+        *self.0.lock().unwrap() = Some(value);
+    }
+
+    /// Get the test flag value. Defaults otherwise.
+    pub fn get(&self) -> T {
+        self.0.lock().unwrap().clone().unwrap_or_default().clone()
+    }
+}
+
 pub fn get_epoch_time_secs() -> u64 {
     let start = SystemTime::now();
     let since_the_epoch = start

stacks-signer/src/v0/signer.rs

@@ -26,6 +26,8 @@ use clarity::types::chainstate::StacksPrivateKey;
 use clarity::types::{PrivateKey, StacksEpochId};
 use clarity::util::hash::MerkleHashFunc;
 use clarity::util::secp256k1::Secp256k1PublicKey;
+#[cfg(any(test, feature = "testing"))]
+use lazy_static::lazy_static;
 use libsigner::v0::messages::{
     BlockAccepted, BlockRejection, BlockResponse, MessageSlotID, MockProposal, MockSignature,
     RejectCode, SignerMessage,
@@ -35,6 +37,8 @@ use slog::{slog_debug, slog_error, slog_info, slog_warn};
 use stacks_common::types::chainstate::StacksAddress;
 use stacks_common::util::get_epoch_time_secs;
 use stacks_common::util::secp256k1::MessageSignature;
+#[cfg(any(test, feature = "testing"))]
+use stacks_common::util::TestFlag;
 use stacks_common::{debug, error, info, warn};
 
 use crate::chainstate::{ProposalEvalConfig, SortitionsView};
@@ -45,29 +49,28 @@ use crate::signerdb::{BlockInfo, BlockState, SignerDb};
 use crate::Signer as SignerTrait;
 
 #[cfg(any(test, feature = "testing"))]
-/// A global variable that can be used to reject all block proposals if the signer's public key is in the provided list
-pub static TEST_REJECT_ALL_BLOCK_PROPOSAL: std::sync::Mutex<
-    Option<Vec<stacks_common::types::chainstate::StacksPublicKey>>,
-> = std::sync::Mutex::new(None);
-
-#[cfg(any(test, feature = "testing"))]
-/// A global variable that can be used to ignore block proposals if the signer's public key is in the provided list
-pub static TEST_IGNORE_ALL_BLOCK_PROPOSALS: std::sync::Mutex<
-    Option<Vec<stacks_common::types::chainstate::StacksPublicKey>>,
-> = std::sync::Mutex::new(None);
+lazy_static! {
+    /// A global variable that can be used to reject all block proposals if the signer's public key is in the provided list
+    pub static ref TEST_REJECT_ALL_BLOCK_PROPOSAL: TestFlag<Vec<stacks_common::types::chainstate::StacksPublicKey>> = TestFlag::default();
+}
 
 #[cfg(any(test, feature = "testing"))]
-/// Pause the block broadcast
-pub static TEST_PAUSE_BLOCK_BROADCAST: std::sync::Mutex<Option<bool>> = std::sync::Mutex::new(None);
+lazy_static! {
+    /// A global variable that can be used to ignore block proposals if the signer's public key is in the provided list
+    pub static ref TEST_IGNORE_ALL_BLOCK_PROPOSALS: TestFlag<Vec<stacks_common::types::chainstate::StacksPublicKey>> = TestFlag::default();
+}
 
 #[cfg(any(test, feature = "testing"))]
-/// Skip broadcasting the block to the network
-pub static TEST_SKIP_BLOCK_BROADCAST: std::sync::Mutex<Option<bool>> = std::sync::Mutex::new(None);
+lazy_static! {
+    /// Pause the block broadcast
+    pub static ref TEST_PAUSE_BLOCK_BROADCAST: TestFlag<bool> = TestFlag::default();
+}
 
 #[cfg(any(test, feature = "testing"))]
-/// Skip any block responses from other signers
-pub static TEST_IGNORE_BLOCK_RESPONSES: std::sync::Mutex<Option<bool>> =
-    std::sync::Mutex::new(None);
+lazy_static! {
+    /// Skip broadcasting the block to the network
+    pub static ref TEST_SKIP_BLOCK_BROADCAST: TestFlag<bool> = TestFlag::default();
+}
 
 /// The stacks signer registered for the reward cycle
 #[derive(Debug)]
@@ -174,9 +177,8 @@ impl SignerTrait<SignerMessage> for Signer {
                     match message {
                         SignerMessage::BlockProposal(block_proposal) => {
                             #[cfg(any(test, feature = "testing"))]
-                            if let Some(public_keys) =
-                                &*TEST_IGNORE_ALL_BLOCK_PROPOSALS.lock().unwrap()
                             {
+                                let public_keys = TEST_IGNORE_ALL_BLOCK_PROPOSALS.get();
                                 if public_keys.contains(
                                     &stacks_common::types::chainstate::StacksPublicKey::from_private(
                                         &self.private_key,
@@ -405,8 +407,10 @@ impl Signer {
             "burn_height" => block_proposal.burn_height,
         );
         crate::monitoring::increment_block_proposals_received();
-        #[allow(unused_mut)]
+        #[cfg(any(test, feature = "testing"))]
         let mut block_info = BlockInfo::from(block_proposal.clone());
+        #[cfg(not(any(test, feature = "testing")))]
+        let block_info = BlockInfo::from(block_proposal.clone());
 
         // Get sortition view if we don't have it
         if sortition_state.is_none() {
@@ -538,10 +542,6 @@ impl Signer {
         stacks_client: &StacksClient,
         block_response: &BlockResponse,
     ) {
-        #[cfg(any(test, feature = "testing"))]
-        if self.test_ignore_block_responses(block_response) {
-            return;
-        }
         match block_response {
             BlockResponse::Accepted(accepted) => {
                 self.handle_block_signature(stacks_client, accepted);
@@ -1071,7 +1071,7 @@ impl Signer {
 
     #[cfg(any(test, feature = "testing"))]
     fn test_skip_block_broadcast(&self, block: &NakamotoBlock) -> bool {
-        if *TEST_SKIP_BLOCK_BROADCAST.lock().unwrap() == Some(true) {
+        if TEST_SKIP_BLOCK_BROADCAST.get() {
             let block_hash = block.header.signer_signature_hash();
             warn!(
                 "{self}: Skipping block broadcast due to testing directive";
@@ -1099,9 +1099,7 @@ impl Signer {
         block_info: &mut BlockInfo,
         block_response: Option<BlockResponse>,
     ) -> Option<BlockResponse> {
-        let Some(public_keys) = &*TEST_REJECT_ALL_BLOCK_PROPOSAL.lock().unwrap() else {
-            return block_response;
-        };
+        let public_keys = TEST_REJECT_ALL_BLOCK_PROPOSAL.get();
         if public_keys.contains(
             &stacks_common::types::chainstate::StacksPublicKey::from_private(&self.private_key),
         ) {
@@ -1126,31 +1124,19 @@ impl Signer {
                 self.mainnet,
             ))
         } else {
-            None
-        }
-    }
-
-    #[cfg(any(test, feature = "testing"))]
-    fn test_ignore_block_responses(&self, block_response: &BlockResponse) -> bool {
-        if *TEST_IGNORE_BLOCK_RESPONSES.lock().unwrap() == Some(true) {
-            warn!(
-                "{self}: Ignoring block response due to testing directive";
-                "block_response" => %block_response
-            );
-            return true;
+            block_response
         }
-        false
     }
 
     #[cfg(any(test, feature = "testing"))]
     fn test_pause_block_broadcast(&self, block_info: &BlockInfo) {
-        if *TEST_PAUSE_BLOCK_BROADCAST.lock().unwrap() == Some(true) {
+        if TEST_PAUSE_BLOCK_BROADCAST.get() {
             // Do an extra check just so we don't log EVERY time.
             warn!("{self}: Block broadcast is stalled due to testing directive.";
                 "block_id" => %block_info.block.block_id(),
                 "height" => block_info.block.header.chain_length,
             );
-            while *TEST_PAUSE_BLOCK_BROADCAST.lock().unwrap() == Some(true) {
+            while TEST_PAUSE_BLOCK_BROADCAST.get() {
                 std::thread::sleep(std::time::Duration::from_millis(10));
             }
             info!("{self}: Block validation is no longer stalled due to testing directive.";

testnet/stacks-node/src/event_dispatcher.rs

@@ -26,6 +26,8 @@ use clarity::vm::analysis::contract_interface_builder::build_contract_interface;
 use clarity::vm::costs::ExecutionCost;
 use clarity::vm::events::{FTEventType, NFTEventType, STXEventType};
 use clarity::vm::types::{AssetIdentifier, QualifiedContractIdentifier, Value};
+#[cfg(any(test, feature = "testing"))]
+use lazy_static::lazy_static;
 use rand::Rng;
 use rusqlite::{params, Connection};
 use serde_json::json;
@@ -59,6 +61,8 @@ use stacks::net::http::HttpRequestContents;
 use stacks::net::httpcore::{send_http_request, StacksHttpRequest};
 use stacks::net::stackerdb::StackerDBEventDispatcher;
 use stacks::util::hash::to_hex;
+#[cfg(any(test, feature = "testing"))]
+use stacks::util::TestFlag;
 use stacks::util_lib::db::Error as db_error;
 use stacks_common::bitvec::BitVec;
 use stacks_common::codec::StacksMessageCodec;
@@ -71,8 +75,10 @@ use url::Url;
 use super::config::{EventKeyType, EventObserverConfig};
 
 #[cfg(any(test, feature = "testing"))]
-pub static TEST_SKIP_BLOCK_ANNOUNCEMENT: std::sync::Mutex<Option<bool>> =
-    std::sync::Mutex::new(None);
+lazy_static! {
+    /// Do not announce a signed/mined block to the network when set to true.
+    pub static ref TEST_SKIP_BLOCK_ANNOUNCEMENT: TestFlag<bool> = TestFlag::default();
+}
 
 #[derive(Debug, Clone)]
 struct EventObserver {
@@ -1706,7 +1712,7 @@ impl EventDispatcher {
 
 #[cfg(any(test, feature = "testing"))]
 fn test_skip_block_announcement(block: &StacksBlockEventData) -> bool {
-    if *TEST_SKIP_BLOCK_ANNOUNCEMENT.lock().unwrap() == Some(true) {
+    if TEST_SKIP_BLOCK_ANNOUNCEMENT.get() {
         warn!(
             "Skipping new block announcement due to testing directive";
             "block_hash" => %block.block_hash

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

@@ -20,6 +20,8 @@ use std::sync::{Arc, Mutex};
 use std::time::Duration;
 
 use hashbrown::{HashMap, HashSet};
+#[cfg(any(test, feature = "testing"))]
+use lazy_static::lazy_static;
 use libsigner::v0::messages::{
     BlockAccepted, BlockResponse, MinerSlotID, SignerMessage as SignerMessageV0,
 };
@@ -37,6 +39,8 @@ use stacks::net::stackerdb::StackerDBs;
 use stacks::types::PublicKey;
 use stacks::util::hash::MerkleHashFunc;
 use stacks::util::secp256k1::MessageSignature;
+#[cfg(any(test, feature = "testing"))]
+use stacks::util::TestFlag;
 use stacks::util_lib::boot::boot_code_id;
 use stacks_common::bitvec::BitVec;
 use stacks_common::codec::StacksMessageCodec;
@@ -47,10 +51,12 @@ use crate::event_dispatcher::StackerDBChannel;
 use crate::neon::Counters;
 use crate::Config;
 
-/// Fault injection flag to prevent the miner from seeing enough signer signatures.
-/// Used to test that the signers will broadcast a block if it gets enough signatures
-#[cfg(test)]
-pub static TEST_IGNORE_SIGNERS: std::sync::Mutex<Option<bool>> = std::sync::Mutex::new(None);
+#[cfg(any(test, feature = "testing"))]
+lazy_static! {
+    /// Fault injection flag to prevent the miner from seeing enough signer signatures.
+    /// Used to test that the signers will broadcast a block if it gets enough signatures
+    pub static ref TEST_IGNORE_SIGNERS: TestFlag<bool> = TestFlag::default();
+}
 
 /// How long should the coordinator poll on the event receiver before
 /// waking up to check timeouts?
@@ -256,10 +262,7 @@ impl SignCoordinator {
     /// Do we ignore signer signatures?
     #[cfg(test)]
     fn fault_injection_ignore_signatures() -> bool {
-        if *TEST_IGNORE_SIGNERS.lock().unwrap() == Some(true) {
-            return true;
-        }
-        false
+        TEST_IGNORE_SIGNERS.get()
     }
 
     #[cfg(not(test))]

testnet/stacks-node/src/run_loop/neon.rs

@@ -21,6 +21,8 @@ use stacks::chainstate::stacks::db::{ChainStateBootData, StacksChainState};
 use stacks::chainstate::stacks::miner::{signal_mining_blocked, signal_mining_ready, MinerStatus};
 use stacks::core::StacksEpochId;
 use stacks::net::atlas::{AtlasConfig, AtlasDB, Attachment};
+#[cfg(test)]
+use stacks::util::TestFlag;
 use stacks::util_lib::db::Error as db_error;
 use stacks_common::deps_common::ctrlc as termination;
 use stacks_common::deps_common::ctrlc::SignalId;
@@ -82,30 +84,6 @@ impl std::ops::Deref for RunLoopCounter {
     }
 }
 
-#[cfg(test)]
-#[derive(Clone)]
-pub struct TestFlag(pub Arc<std::sync::Mutex<Option<bool>>>);
-
-#[cfg(test)]
-impl Default for TestFlag {
-    fn default() -> Self {
-        Self(Arc::new(std::sync::Mutex::new(None)))
-    }
-}
-
-#[cfg(test)]
-impl TestFlag {
-    /// Set the test flag to the given value
-    pub fn set(&self, value: bool) {
-        *self.0.lock().unwrap() = Some(value);
-    }
-
-    /// Get the test flag value. Defaults to false if the flag is not set.
-    pub fn get(&self) -> bool {
-        self.0.lock().unwrap().unwrap_or(false)
-    }
-}
-
 #[derive(Clone, Default)]
 pub struct Counters {
     pub blocks_processed: RunLoopCounter,
@@ -123,7 +101,7 @@ pub struct Counters {
     pub naka_signer_pushed_blocks: RunLoopCounter,
 
     #[cfg(test)]
-    pub naka_skip_commit_op: TestFlag,
+    pub naka_skip_commit_op: TestFlag<bool>,
 }
 
 impl Counters {

testnet/stacks-node/src/tests/signer/mod.rs

@@ -49,6 +49,7 @@ use stacks::types::chainstate::{StacksAddress, StacksPublicKey};
 use stacks::types::PublicKey;
 use stacks::util::hash::MerkleHashFunc;
 use stacks::util::secp256k1::{MessageSignature, Secp256k1PublicKey};
+use stacks::util::TestFlag;
 use stacks_common::codec::StacksMessageCodec;
 use stacks_common::consts::SIGNER_SLOTS_PER_USER;
 use stacks_common::types::StacksEpochId;
@@ -60,7 +61,7 @@ use stacks_signer::{Signer, SpawnedSigner};
 
 use super::nakamoto_integrations::{check_nakamoto_empty_block_heuristics, wait_for};
 use crate::config::{Config as NeonConfig, EventKeyType, EventObserverConfig, InitialBalance};
-use crate::neon::{Counters, TestFlag};
+use crate::neon::Counters;
 use crate::run_loop::boot_nakamoto;
 use crate::tests::bitcoin_regtest::BitcoinCoreController;
 use crate::tests::nakamoto_integrations::{
@@ -88,7 +89,7 @@ pub struct RunningNodes {
     pub nakamoto_blocks_mined: Arc<AtomicU64>,
     pub nakamoto_blocks_rejected: Arc<AtomicU64>,
     pub nakamoto_blocks_signer_pushed: Arc<AtomicU64>,
-    pub nakamoto_test_skip_commit_op: TestFlag,
+    pub nakamoto_test_skip_commit_op: TestFlag<bool>,
     pub coord_channel: Arc<Mutex<CoordinatorChannels>>,
     pub conf: NeonConfig,
 }