Merge pull request #5108 from jbencin/test/duplicate-signer

test(signer): Add test with duplicate signer private key

Author: jferrant

.github/workflows/bitcoin-tests.yml

@@ -99,6 +99,7 @@ jobs:
           - tests::signer::v0::reloads_signer_set_in
           - tests::signer::v0::signers_broadcast_signed_blocks
           - tests::signer::v0::min_gap_between_blocks
+          - tests::signer::v0::duplicate_signers
           - tests::nakamoto_integrations::stack_stx_burn_op_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state

stacks-common/src/util/secp256k1.rs

@@ -38,7 +38,7 @@ use crate::util::hash::{hex_bytes, to_hex};
 // per-thread Secp256k1 context
 thread_local!(static _secp256k1: Secp256k1<secp256k1::All> = Secp256k1::new());
 
-#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)]
+#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize, Hash)]
 pub struct Secp256k1PublicKey {
     // serde is broken for secp256k1, so do it ourselves
     #[serde(

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

@@ -574,7 +574,7 @@ pub mod test_observer {
     pub fn contains_burn_block_range(range: impl RangeBounds<u64>) -> Result<(), String> {
         // Get set of all burn block heights
         let burn_block_heights = get_blocks()
-            .iter()
+            .into_iter()
             .map(|x| x.get("burn_block_height").unwrap().as_u64().unwrap())
             .collect::<HashSet<_>>();
 

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

@@ -125,7 +125,8 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
             wait_on_signers,
             |_| {},
             |_| {},
-            &[],
+            None,
+            None,
         )
     }
 
@@ -138,12 +139,19 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
         wait_on_signers: Option<Duration>,
         mut signer_config_modifier: F,
         mut node_config_modifier: G,
-        btc_miner_pubkeys: &[Secp256k1PublicKey],
+        btc_miner_pubkeys: Option<Vec<Secp256k1PublicKey>>,
+        signer_stacks_private_keys: Option<Vec<StacksPrivateKey>>,
     ) -> Self {
         // Generate Signer Data
-        let signer_stacks_private_keys = (0..num_signers)
-            .map(|_| StacksPrivateKey::new())
-            .collect::<Vec<StacksPrivateKey>>();
+        let signer_stacks_private_keys = signer_stacks_private_keys
+            .inspect(|keys| {
+                assert_eq!(
+                    keys.len(),
+                    num_signers,
+                    "Number of private keys does not match number of signers"
+                )
+            })
+            .unwrap_or_else(|| (0..num_signers).map(|_| StacksPrivateKey::new()).collect());
 
         let (mut naka_conf, _miner_account) = naka_neon_integration_conf(None);
 
@@ -159,11 +167,8 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
         // That's the kind of thing an idiot would have on his luggage!
         let password = "12345";
         naka_conf.connection_options.auth_token = Some(password.to_string());
-        if let Some(wait_on_signers) = wait_on_signers {
-            naka_conf.miner.wait_on_signers = wait_on_signers;
-        } else {
-            naka_conf.miner.wait_on_signers = Duration::from_secs(10);
-        }
+        naka_conf.miner.wait_on_signers =
+            wait_on_signers.unwrap_or_else(|| Duration::from_secs(10));
         let run_stamp = rand::random();
 
         // Setup the signer and coordinator configurations
@@ -195,19 +200,20 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
             .collect();
 
         // Setup the nodes and deploy the contract to it
-        let btc_miner_pubkeys = if btc_miner_pubkeys.is_empty() {
-            let pk = Secp256k1PublicKey::from_hex(
-                naka_conf
-                    .burnchain
-                    .local_mining_public_key
-                    .as_ref()
-                    .unwrap(),
-            )
-            .unwrap();
-            vec![pk]
-        } else {
-            btc_miner_pubkeys.to_vec()
-        };
+        let btc_miner_pubkeys = btc_miner_pubkeys
+            .filter(|keys| !keys.is_empty())
+            .unwrap_or_else(|| {
+                let pk = Secp256k1PublicKey::from_hex(
+                    naka_conf
+                        .burnchain
+                        .local_mining_public_key
+                        .as_ref()
+                        .unwrap(),
+                )
+                .unwrap();
+                vec![pk]
+            });
+
         let node = setup_stx_btc_node(
             naka_conf,
             &signer_stacks_private_keys,
@@ -236,10 +242,10 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
                 continue;
             }
             let port = 3000 + signer_ix;
-            let endpoint = format!("http://localhost:{}", port);
+            let endpoint = format!("http://localhost:{port}");
             let path = format!("{endpoint}/status");
 
-            debug!("Issue status request to {}", &path);
+            debug!("Issue status request to {path}");
             let client = reqwest::blocking::Client::new();
             let response = client
                 .get(path)

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

@@ -321,6 +321,7 @@ impl SignerTest<SpawnedSigner> {
         // Verify that the signers signed the proposed block
         let mut signer_index = 0;
         let mut signature_index = 0;
+        let mut signing_keys = HashSet::new();
         let validated = loop {
             // Since we've already checked `signature.len()`, this means we've
             //  validated all the signatures in this loop
@@ -331,6 +332,9 @@ impl SignerTest<SpawnedSigner> {
                 error!("Failed to validate the mined nakamoto block: ran out of signers to try to validate signatures");
                 break false;
             };
+            if !signing_keys.insert(signer.signing_key) {
+                panic!("Duplicate signing key detected: {:?}", signer.signing_key);
+            }
             let stacks_public_key = Secp256k1PublicKey::from_slice(signer.signing_key.as_slice())
                 .expect("Failed to convert signing key to StacksPublicKey");
             let valid = stacks_public_key
@@ -488,11 +492,7 @@ fn block_proposal_rejection() {
     while !found_signer_signature_hash_1 && !found_signer_signature_hash_2 {
         std::thread::sleep(Duration::from_secs(1));
         let chunks = test_observer::get_stackerdb_chunks();
-        for chunk in chunks
-            .into_iter()
-            .map(|chunk| chunk.modified_slots)
-            .flatten()
-        {
+        for chunk in chunks.into_iter().flat_map(|chunk| chunk.modified_slots) {
             let Ok(message) = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice())
             else {
                 continue;
@@ -796,7 +796,8 @@ fn reloads_signer_set_in() {
         Some(Duration::from_secs(15)),
         |_config| {},
         |_| {},
-        &[],
+        None,
+        None,
     );
 
     setup_epoch_3_reward_set(
@@ -925,7 +926,8 @@ fn forked_tenure_testing(
             config.broadcast_signed_blocks = false;
         },
         |_| {},
-        &[],
+        None,
+        None,
     );
     let http_origin = format!("http://{}", &signer_test.running_nodes.conf.node.rpc_bind);
 
@@ -1429,7 +1431,8 @@ fn multiple_miners() {
                 false
             })
         },
-        &[btc_miner_1_pk.clone(), btc_miner_2_pk.clone()],
+        Some(vec![btc_miner_1_pk.clone(), btc_miner_2_pk.clone()]),
+        None,
     );
     let conf = signer_test.running_nodes.conf.clone();
     let mut conf_node_2 = conf.clone();
@@ -1694,7 +1697,8 @@ fn miner_forking() {
                 false
             })
         },
-        &[btc_miner_1_pk.clone(), btc_miner_2_pk.clone()],
+        Some(vec![btc_miner_1_pk.clone(), btc_miner_2_pk.clone()]),
+        None,
     );
     let conf = signer_test.running_nodes.conf.clone();
     let mut conf_node_2 = conf.clone();
@@ -2274,7 +2278,8 @@ fn empty_sortition() {
             config.block_proposal_timeout = block_proposal_timeout;
         },
         |_| {},
-        &[],
+        None,
+        None,
     );
     let http_origin = format!("http://{}", &signer_test.running_nodes.conf.node.rpc_bind);
     let short_timeout = Duration::from_secs(20);
@@ -2455,7 +2460,8 @@ fn mock_sign_epoch_25() {
                 }
             }
         },
-        &[],
+        None,
+        None,
     );
 
     let epochs = signer_test
@@ -2659,7 +2665,8 @@ fn signer_set_rollover() {
             }
             naka_conf.node.rpc_bind = rpc_bind.clone();
         },
-        &[],
+        None,
+        None,
     );
     assert_eq!(
         new_spawned_signers[0].config.node_host,
@@ -2873,7 +2880,8 @@ fn min_gap_between_blocks() {
         |config| {
             config.miner.min_time_between_blocks_ms = time_between_blocks_ms;
         },
-        &[],
+        None,
+        None,
     );
 
     let http_origin = format!("http://{}", &signer_test.running_nodes.conf.node.rpc_bind);
@@ -2945,3 +2953,108 @@ fn min_gap_between_blocks() {
 
     signer_test.shutdown();
 }
+
+#[test]
+#[ignore]
+/// Test scenario where there are duplicate signers with the same private key
+/// First submitted signature should take precedence
+fn duplicate_signers() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    tracing_subscriber::registry()
+        .with(fmt::layer())
+        .with(EnvFilter::from_default_env())
+        .init();
+
+    // Disable p2p broadcast of the nakamoto blocks, so that we rely
+    //  on the signer's using StackerDB to get pushed blocks
+    *nakamoto_node::miner::TEST_SKIP_P2P_BROADCAST
+        .lock()
+        .unwrap() = Some(true);
+
+    info!("------------------------- Test Setup -------------------------");
+    let num_signers = 5;
+    let mut signer_stacks_private_keys = (0..num_signers)
+        .map(|_| StacksPrivateKey::new())
+        .collect::<Vec<_>>();
+
+    // First two signers have same private key
+    signer_stacks_private_keys[1] = signer_stacks_private_keys[0];
+    let unique_signers = num_signers - 1;
+    let duplicate_pubkey = Secp256k1PublicKey::from_private(&signer_stacks_private_keys[0]);
+    let duplicate_pubkey_from_copy =
+        Secp256k1PublicKey::from_private(&signer_stacks_private_keys[1]);
+    assert_eq!(
+        duplicate_pubkey, duplicate_pubkey_from_copy,
+        "Recovered pubkeys don't match"
+    );
+
+    let mut signer_test: SignerTest<SpawnedSigner> = SignerTest::new_with_config_modifications(
+        num_signers,
+        vec![],
+        None,
+        |_| {},
+        |_| {},
+        None,
+        Some(signer_stacks_private_keys),
+    );
+
+    signer_test.boot_to_epoch_3();
+    let timeout = Duration::from_secs(30);
+
+    info!("------------------------- Try mining one block -------------------------");
+
+    signer_test.mine_and_verify_confirmed_naka_block(timeout, num_signers);
+
+    info!("------------------------- Read all `BlockResponse::Accepted` messages -------------------------");
+
+    let mut signer_accepted_responses = vec![];
+    let start_polling = Instant::now();
+    while start_polling.elapsed() <= timeout {
+        std::thread::sleep(Duration::from_secs(1));
+        let messages = test_observer::get_stackerdb_chunks()
+            .into_iter()
+            .flat_map(|chunk| chunk.modified_slots)
+            .filter_map(|chunk| {
+                SignerMessage::consensus_deserialize(&mut chunk.data.as_slice()).ok()
+            })
+            .filter_map(|message| match message {
+                SignerMessage::BlockResponse(BlockResponse::Accepted(m)) => {
+                    info!("Message(accepted): {message:?}");
+                    Some(m)
+                }
+                _ => {
+                    debug!("Message(ignored): {message:?}");
+                    None
+                }
+            });
+        signer_accepted_responses.extend(messages);
+    }
+
+    info!("------------------------- Assert there are {unique_signers} unique signatures and recovered pubkeys -------------------------");
+
+    // Pick a message hash
+    let (selected_sighash, _) = signer_accepted_responses
+        .iter()
+        .min_by_key(|(sighash, _)| *sighash)
+        .copied()
+        .expect("No `BlockResponse::Accepted` messages recieved");
+
+    // Filter only resonses for selected block and collect unique pubkeys and signatures
+    let (pubkeys, signatures): (HashSet<_>, HashSet<_>) = signer_accepted_responses
+        .into_iter()
+        .filter(|(hash, _)| *hash == selected_sighash)
+        .map(|(msg, sig)| {
+            let pubkey = Secp256k1PublicKey::recover_to_pubkey(msg.bits(), &sig)
+                .expect("Failed to recover pubkey");
+            (pubkey, sig)
+        })
+        .unzip();
+
+    assert_eq!(pubkeys.len(), unique_signers);
+    assert_eq!(signatures.len(), unique_signers);
+
+    signer_test.shutdown();
+}