Merge pull request #5860 from stacks-network/fix/signerdb-pubkey

Fix: signer calculates miner pk with block header

Author: kantai

libsigner/src/events.rs

@@ -188,8 +188,7 @@ impl StacksMessageCodec for BlockProposalData {
 pub enum SignerEvent<T: SignerEventTrait> {
     /// A miner sent a message over .miners
     /// The `Vec<T>` will contain any signer messages made by the miner.
-    /// The `StacksPublicKey` is the message sender's public key.
-    MinerMessages(Vec<T>, StacksPublicKey),
+    MinerMessages(Vec<T>),
     /// The signer messages for other signers and miners to observe
     /// The u32 is the signer set to which the message belongs (either 0 or 1)
     SignerMessages(u32, Vec<T>),
@@ -513,20 +512,13 @@ impl<T: SignerEventTrait> TryFrom<StackerDBChunksEvent> for SignerEvent<T> {
             && event.contract_id.is_boot()
         {
             let mut messages = vec![];
-            let mut miner_pk = None;
             for chunk in event.modified_slots {
                 let Ok(msg) = T::consensus_deserialize(&mut chunk.data.as_slice()) else {
                     continue;
                 };
-
-                miner_pk = Some(chunk.recover_pk().map_err(|e| {
-                    EventError::MalformedRequest(format!(
-                        "Failed to recover PK from StackerDB chunk: {e}"
-                    ))
-                })?);
                 messages.push(msg);
             }
-            SignerEvent::MinerMessages(messages, miner_pk.ok_or(EventError::EmptyChunksEvent)?)
+            SignerEvent::MinerMessages(messages)
         } else if event.contract_id.name.starts_with(SIGNERS_NAME) && event.contract_id.is_boot() {
             let Some((signer_set, _)) =
                 get_signers_db_signer_set_message_id(event.contract_id.name.as_str())

stacks-signer/CHANGELOG.md

@@ -20,6 +20,10 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 - Add new reject codes to the signer response for better visibility into why a block was rejected.
 - When allowing a reorg within the `reorg_attempts_activity_timeout_ms`, the signer will now watch the responses from other signers and if >30% of them reject this reorg attempt, then the signer will mark the miner as invalid, reject further attempts to reorg and allow the previous miner to extend their tenure.
 
+### Fixed
+
+- The signer runloop no longer relies on pubkey reports from the SignerDB event system. This previously led to improper proposal rejections via #5858. 
+
 ## [3.1.0.0.5.0]
 
 ### Added

stacks-signer/src/v0/signer.rs

@@ -184,7 +184,7 @@ impl SignerTrait<SignerMessage> for Signer {
                     self.handle_block_response(stacks_client, block_response, sortition_state);
                 }
             }
-            SignerEvent::MinerMessages(messages, miner_pubkey) => {
+            SignerEvent::MinerMessages(messages) => {
                 debug!(
                     "{self}: Received {} messages from the miner",
                     messages.len();
@@ -196,11 +196,19 @@ impl SignerTrait<SignerMessage> for Signer {
                             if self.test_ignore_all_block_proposals(block_proposal) {
                                 continue;
                             }
+                            let Some(miner_pubkey) = block_proposal.block.header.recover_miner_pk()
+                            else {
+                                warn!("{self}: Failed to recover miner pubkey";
+                                      "signer_sighash" => %block_proposal.block.header.signer_signature_hash(),
+                                      "consensus_hash" => %block_proposal.block.header.consensus_hash);
+                                continue;
+                            };
+
                             self.handle_block_proposal(
                                 stacks_client,
                                 sortition_state,
                                 block_proposal,
-                                miner_pubkey,
+                                &miner_pubkey,
                             );
                         }
                         SignerMessage::BlockPushed(b) => {

testnet/stacks-node/src/event_dispatcher.rs

@@ -82,14 +82,14 @@ lazy_static! {
 }
 
 #[derive(Debug, Clone)]
-struct EventObserver {
+pub struct EventObserver {
     /// Path to the database where pending payloads are stored. If `None`, then
     /// the database is not used and events are not recoverable across restarts.
-    db_path: Option<PathBuf>,
+    pub db_path: Option<PathBuf>,
     /// URL to which events will be sent
-    endpoint: String,
+    pub endpoint: String,
     /// Timeout for sending events to this observer
-    timeout: Duration,
+    pub timeout: Duration,
 }
 
 struct ReceiptPayloadInfo<'a> {
@@ -770,7 +770,7 @@ impl EventObserver {
         self.send_payload(payload, PATH_MINED_NAKAMOTO_BLOCK);
     }
 
-    fn send_stackerdb_chunks(&self, payload: &serde_json::Value) {
+    pub fn send_stackerdb_chunks(&self, payload: &serde_json::Value) {
         self.send_payload(payload, PATH_STACKERDB_CHUNKS);
     }
 

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

@@ -77,7 +77,9 @@ use tracing_subscriber::prelude::*;
 use tracing_subscriber::{fmt, EnvFilter};
 
 use super::SignerTest;
-use crate::event_dispatcher::{MinedNakamotoBlockEvent, TEST_SKIP_BLOCK_ANNOUNCEMENT};
+use crate::event_dispatcher::{
+    EventObserver, MinedNakamotoBlockEvent, TEST_SKIP_BLOCK_ANNOUNCEMENT,
+};
 use crate::nakamoto_node::miner::{
     TEST_BLOCK_ANNOUNCE_STALL, TEST_BROADCAST_PROPOSAL_STALL, TEST_MINE_STALL,
     TEST_P2P_BROADCAST_STALL,
@@ -396,6 +398,10 @@ impl SignerTest<SpawnedSigner> {
         }
     }
 
+    fn get_miner_key(&self) -> &Secp256k1PrivateKey {
+        self.running_nodes.conf.miner.mining_key.as_ref().unwrap()
+    }
+
     /// Propose a block to the signers
     fn propose_block(&mut self, block: NakamotoBlock, timeout: Duration) {
         let miners_contract_id = boot_code_id(MINERS_NAME, false);
@@ -407,6 +413,9 @@ impl SignerTest<SpawnedSigner> {
             .get_headers_height();
         let reward_cycle = self.get_current_reward_cycle();
         let signer_signature_hash = block.header.signer_signature_hash();
+        let signed_by = block.header.recover_miner_pk().expect(
+            "FATAL: signer tests should only propose blocks that have been signed by the signer test miner. Otherwise, signers won't even consider them via this channel."
+        );
         let message = SignerMessage::BlockProposal(BlockProposal {
             block,
             burn_height,
@@ -419,6 +428,9 @@ impl SignerTest<SpawnedSigner> {
             .miner
             .mining_key
             .expect("No mining key");
+        assert_eq!(signed_by, Secp256k1PublicKey::from_private(&miner_sk),
+                   "signer tests should only propose blocks that have been signed by the signer test miner. Otherwise, signers won't even consider them via this channel.");
+
         // Submit the block proposal to the miner's slot
         let mut accepted = false;
         let mut version = 0;
@@ -1256,6 +1268,10 @@ fn block_proposal_rejection() {
 
     // First propose a block to the signers that does not have the correct consensus hash or BitVec. This should be rejected BEFORE
     // the block is submitted to the node for validation.
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let block_signer_signature_hash_1 = block.header.signer_signature_hash();
     signer_test.propose_block(block.clone(), short_timeout);
 
@@ -1268,6 +1284,10 @@ fn block_proposal_rejection() {
     block.header.consensus_hash = view.cur_sortition.consensus_hash;
     block.header.chain_length = 35; // We have mined 35 blocks so far.
 
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let block_signer_signature_hash_2 = block.header.signer_signature_hash();
     signer_test.propose_block(block, short_timeout);
 
@@ -1430,6 +1450,130 @@ fn mine_2_nakamoto_reward_cycles() {
     signer_test.shutdown();
 }
 
+#[test]
+#[ignore]
+/// This test is a regression test for issue #5858 in which the signer runloop
+///  used the signature from the stackerdb to determine the miner public key.
+/// This does not work in cases where events get coalesced. The fix was to use
+///  the signature in the proposal's block header instead.
+///
+/// This test covers the regression by adding a thread that interposes on the
+///  stackerdb events sent to the test signers and mutating the signatures
+///  so that the stackerdb chunks are signed by the wrong signer. After the
+///  fix to #5848, signers are resilient to this behavior because they check
+///  the signature on the block proposal (not the chunk).
+fn regr_use_block_header_pk() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    tracing_subscriber::registry()
+        .with(fmt::layer())
+        .with(EnvFilter::from_default_env())
+        .init();
+
+    info!("------------------------- Test Setup -------------------------");
+    let num_signers = 5;
+    let signer_listeners: Mutex<Vec<String>> = Mutex::default();
+    let mut signer_test: SignerTest<SpawnedSigner> = SignerTest::new_with_config_modifications(
+        num_signers,
+        vec![],
+        |_| {},
+        |node_config| {
+            node_config.events_observers = node_config
+                .events_observers
+                .clone()
+                .into_iter()
+                .map(|mut event_observer| {
+                    if event_observer
+                        .endpoint
+                        .ends_with(&test_observer::EVENT_OBSERVER_PORT.to_string())
+                    {
+                        event_observer
+                    } else if event_observer
+                        .events_keys
+                        .contains(&EventKeyType::StackerDBChunks)
+                    {
+                        event_observer
+                            .events_keys
+                            .retain(|key| *key != EventKeyType::StackerDBChunks);
+                        let mut listeners_lock = signer_listeners.lock().unwrap();
+                        listeners_lock.push(event_observer.endpoint.clone());
+                        event_observer
+                    } else {
+                        event_observer
+                    }
+                })
+                .collect();
+        },
+        None,
+        None,
+    );
+
+    let signer_listeners: Vec<_> = signer_listeners
+        .lock()
+        .unwrap()
+        .drain(..)
+        .map(|endpoint| EventObserver {
+            endpoint,
+            db_path: None,
+            timeout: Duration::from_secs(120),
+        })
+        .collect();
+
+    let bad_signer = Secp256k1PrivateKey::from_seed(&[0xde, 0xad, 0xbe, 0xef]);
+    let bad_signer_pk = Secp256k1PublicKey::from_private(&bad_signer);
+
+    let broadcast_thread_stopper = Arc::new(AtomicBool::new(true));
+    let broadcast_thread_flag = broadcast_thread_stopper.clone();
+    let broadcast_thread = thread::Builder::new()
+        .name("rebroadcast-thread".into())
+        .spawn(move || {
+            let mut last_sent = 0;
+            while broadcast_thread_flag.load(Ordering::SeqCst) {
+                thread::sleep(Duration::from_secs(1));
+                let mut signerdb_chunks = test_observer::get_stackerdb_chunks();
+                if last_sent >= signerdb_chunks.len() {
+                    continue;
+                }
+                let mut to_send = signerdb_chunks.split_off(last_sent);
+                last_sent = signerdb_chunks.len();
+                for event in to_send.iter_mut() {
+                    // mutilate the signature
+                    event.modified_slots.iter_mut().for_each(|chunk_data| {
+                        let pk = chunk_data.recover_pk().unwrap();
+                        assert_ne!(pk, bad_signer_pk);
+                        chunk_data.sign(&bad_signer).unwrap();
+                    });
+
+                    let payload = serde_json::to_value(event).unwrap();
+                    for signer_listener in signer_listeners.iter() {
+                        signer_listener.send_stackerdb_chunks(&payload);
+                    }
+                }
+            }
+        })
+        .unwrap();
+
+    let timeout = Duration::from_secs(200);
+    signer_test.boot_to_epoch_3();
+
+    let prior_stacks_height = signer_test.get_peer_info().stacks_tip_height;
+
+    let tenures_to_mine = 2;
+    for _i in 0..tenures_to_mine {
+        signer_test.mine_nakamoto_block(timeout, false);
+    }
+
+    let current_stacks_height = signer_test.get_peer_info().stacks_tip_height;
+
+    assert!(current_stacks_height >= prior_stacks_height + tenures_to_mine);
+
+    broadcast_thread_stopper.store(false, Ordering::SeqCst);
+    broadcast_thread.join().unwrap();
+    signer_test.shutdown();
+}
+
 #[test]
 #[ignore]
 fn forked_tenure_invalid() {
@@ -7067,6 +7211,10 @@ fn block_validation_response_timeout() {
     block.header.consensus_hash = view.cur_sortition.consensus_hash;
     block.header.chain_length = info_before.stacks_tip_height + 1;
 
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let block_signer_signature_hash_1 = block.header.signer_signature_hash();
     signer_test.propose_block(block, timeout);
 
@@ -7352,6 +7500,10 @@ fn block_validation_pending_table() {
     block.header.pox_treatment = BitVec::ones(1).unwrap();
     block.header.consensus_hash = view.cur_sortition.consensus_hash;
     block.header.chain_length = peer_info.stacks_tip_height + 1;
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let block_signer_signature_hash = block.header.signer_signature_hash();
     signer_test.propose_block(block.clone(), short_timeout);
 
@@ -8009,11 +8161,19 @@ fn block_proposal_max_age_rejections() {
             .block_proposal_max_age_secs
             .saturating_add(1),
     );
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let block_signer_signature_hash_1 = block.header.signer_signature_hash();
     signer_test.propose_block(block.clone(), short_timeout);
 
     // Next propose a recent invalid block
     block.header.timestamp = get_epoch_time_secs();
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let block_signer_signature_hash_2 = block.header.signer_signature_hash();
     signer_test.propose_block(block, short_timeout);
 
@@ -8614,6 +8774,10 @@ fn incoming_signers_ignore_block_proposals() {
         txs: vec![],
     };
     block.header.timestamp = get_epoch_time_secs();
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let signer_signature_hash_1 = block.header.signer_signature_hash();
 
     info!("------------------------- Test Attempt to Mine Invalid Block {signer_signature_hash_1} -------------------------");
@@ -8632,6 +8796,10 @@ fn incoming_signers_ignore_block_proposals() {
     block.header.consensus_hash = view.cur_sortition.consensus_hash;
     block.header.chain_length =
         get_chain_info(&signer_test.running_nodes.conf).stacks_tip_height + 1;
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let signer_signature_hash_2 = block.header.signer_signature_hash();
 
     info!("------------------------- Test Attempt to Mine Invalid Block {signer_signature_hash_2} -------------------------");
@@ -8800,6 +8968,10 @@ fn outgoing_signers_ignore_block_proposals() {
     block.header.consensus_hash = view.cur_sortition.consensus_hash;
     block.header.chain_length =
         get_chain_info(&signer_test.running_nodes.conf).stacks_tip_height + 1;
+    block
+        .header
+        .sign_miner(signer_test.get_miner_key())
+        .unwrap();
     let signer_signature_hash = block.header.signer_signature_hash();
 
     info!("------------------------- Test Attempt to Mine Invalid Block {signer_signature_hash} -------------------------");