Merge pull request #4695 from stacks-network/chore/remove-block-proposal-message

Remove redundant block proposal message and fix tests

Author: wileyj

libsigner/src/events.rs

@@ -68,14 +68,9 @@ pub struct BlockProposalSigners {
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub enum SignerEvent {
     /// A miner sent a message over .miners
-    /// The `Vec<BlockProposalSigners>` will contain any block proposals made by the miner during this StackerDB event.
     /// The `Vec<SignerMessage>` will contain any signer WSTS messages made by the miner while acting as a coordinator.
-    /// The `Option<StacksPublicKey>` will contain the message sender's public key if either of the vecs is non-empty.
-    MinerMessages(
-        Vec<BlockProposalSigners>,
-        Vec<SignerMessage>,
-        Option<StacksPublicKey>,
-    ),
+    /// The `Option<StacksPublicKey>` will contain the message sender's public key if the vec is non-empty.
+    MinerMessages(Vec<SignerMessage>, Option<StacksPublicKey>),
     /// 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<SignerMessage>),
@@ -417,7 +412,6 @@ impl TryFrom<StackerDBChunksEvent> for SignerEvent {
         let signer_event = if event.contract_id.name.as_str() == MINERS_NAME
             && event.contract_id.is_boot()
         {
-            let mut blocks = vec![];
             let mut messages = vec![];
             let mut miner_pk = None;
             for chunk in event.modified_slots {
@@ -426,28 +420,13 @@ impl TryFrom<StackerDBChunksEvent> for SignerEvent {
                         "Failed to recover PK from StackerDB chunk: {e}"
                     ))
                 })?);
-                if chunk.slot_id % MINER_SLOT_COUNT == 0 {
-                    // block
-                    let Ok(block) =
-                        BlockProposalSigners::consensus_deserialize(&mut chunk.data.as_slice())
-                    else {
-                        continue;
-                    };
-                    blocks.push(block);
-                } else if chunk.slot_id % MINER_SLOT_COUNT == 1 {
-                    // message
-                    let Ok(msg) = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice())
-                    else {
-                        continue;
-                    };
-                    messages.push(msg);
-                } else {
-                    return Err(EventError::UnrecognizedEvent(
-                        "Unrecognized slot_id for miners contract".into(),
-                    ));
+                let Ok(msg) = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice())
+                else {
+                    continue;
                 };
+                messages.push(msg);
             }
-            SignerEvent::MinerMessages(blocks, messages, miner_pk)
+            SignerEvent::MinerMessages(messages, miner_pk)
         } 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/src/main.rs

@@ -32,11 +32,13 @@ use std::path::{Path, PathBuf};
 use std::sync::mpsc::{channel, Receiver, Sender};
 use std::time::Duration;
 
-use blockstack_lib::chainstate::nakamoto::NakamotoBlock;
 use blockstack_lib::util_lib::signed_structured_data::pox4::make_pox_4_signer_key_signature;
 use clap::Parser;
 use clarity::vm::types::QualifiedContractIdentifier;
-use libsigner::{RunningSigner, Signer, SignerEventReceiver, SignerSession, StackerDBSession};
+use libsigner::{
+    BlockProposalSigners, RunningSigner, Signer, SignerEventReceiver, SignerSession,
+    StackerDBSession,
+};
 use libstackerdb::StackerDBChunkData;
 use slog::{slog_debug, slog_error, slog_info};
 use stacks_common::codec::read_next;
@@ -208,15 +210,16 @@ fn handle_dkg(args: RunDkgArgs) {
 fn handle_sign(args: SignArgs) {
     debug!("Signing message...");
     let spawned_signer = spawn_running_signer(&args.config);
-    let Some(block) = read_next::<NakamotoBlock, _>(&mut &args.data[..]).ok() else {
-        error!("Unable to parse provided message as a NakamotoBlock.");
+    let Some(block_proposal) = read_next::<BlockProposalSigners, _>(&mut &args.data[..]).ok()
+    else {
+        error!("Unable to parse provided message as a BlockProposalSigners.");
         spawned_signer.running_signer.stop();
         return;
     };
     let sign_command = RunLoopCommand {
         reward_cycle: args.reward_cycle,
         command: SignerCommand::Sign {
-            block,
+            block_proposal,
             is_taproot: false,
             merkle_root: None,
         },
@@ -230,8 +233,9 @@ fn handle_sign(args: SignArgs) {
 fn handle_dkg_sign(args: SignArgs) {
     debug!("Running DKG and signing message...");
     let spawned_signer = spawn_running_signer(&args.config);
-    let Some(block) = read_next::<NakamotoBlock, _>(&mut &args.data[..]).ok() else {
-        error!("Unable to parse provided message as a NakamotoBlock.");
+    let Some(block_proposal) = read_next::<BlockProposalSigners, _>(&mut &args.data[..]).ok()
+    else {
+        error!("Unable to parse provided message as a BlockProposalSigners.");
         spawned_signer.running_signer.stop();
         return;
     };
@@ -242,7 +246,7 @@ fn handle_dkg_sign(args: SignArgs) {
     let sign_command = RunLoopCommand {
         reward_cycle: args.reward_cycle,
         command: SignerCommand::Sign {
-            block,
+            block_proposal,
             is_taproot: false,
             merkle_root: None,
         },

stacks-signer/src/signer.rs

@@ -72,6 +72,10 @@ impl std::fmt::Display for SignerSlotID {
 pub struct BlockInfo {
     /// The block we are considering
     pub block: NakamotoBlock,
+    /// The burn block height at which the block was proposed
+    pub burn_block_height: u64,
+    /// The reward cycle the block belongs to
+    pub reward_cycle: u64,
     /// Our vote on the block if we have one yet
     pub vote: Option<NakamotoBlockVote>,
     /// Whether the block contents are valid
@@ -82,27 +86,29 @@ pub struct BlockInfo {
     pub signed_over: bool,
 }
 
-impl BlockInfo {
-    /// Create a new BlockInfo
-    pub const fn new(block: NakamotoBlock) -> Self {
+impl From<BlockProposalSigners> for BlockInfo {
+    fn from(value: BlockProposalSigners) -> Self {
         Self {
-            block,
+            block: value.block,
+            burn_block_height: value.burn_height,
+            reward_cycle: value.reward_cycle,
             vote: None,
             valid: None,
             nonce_request: None,
             signed_over: false,
         }
     }
-
+}
+impl BlockInfo {
     /// Create a new BlockInfo with an associated nonce request packet
-    pub const fn new_with_request(block: NakamotoBlock, nonce_request: NonceRequest) -> Self {
-        Self {
-            block,
-            vote: None,
-            valid: None,
-            nonce_request: Some(nonce_request),
-            signed_over: true,
-        }
+    pub fn new_with_request(
+        block_proposal: BlockProposalSigners,
+        nonce_request: NonceRequest,
+    ) -> Self {
+        let mut block_info = BlockInfo::from(block_proposal);
+        block_info.nonce_request = Some(nonce_request);
+        block_info.signed_over = true;
+        block_info
     }
 
     /// Return the block's signer signature hash
@@ -119,7 +125,7 @@ pub enum Command {
     /// Sign a message
     Sign {
         /// The block to sign over
-        block: NakamotoBlock,
+        block_proposal: BlockProposalSigners,
         /// Whether to make a taproot signature
         is_taproot: bool,
         /// Taproot merkle root
@@ -399,31 +405,31 @@ impl Signer {
                 }
             }
             Command::Sign {
-                block,
+                block_proposal,
                 is_taproot,
                 merkle_root,
             } => {
                 if self.approved_aggregate_public_key.is_none() {
                     debug!("{self}: Cannot sign a block without an approved aggregate public key. Ignore it.");
                     return;
                 }
-                let signer_signature_hash = block.header.signer_signature_hash();
+                let signer_signature_hash = block_proposal.block.header.signer_signature_hash();
                 let mut block_info = self
                     .signer_db
                     .block_lookup(self.reward_cycle, &signer_signature_hash)
-                    .unwrap_or_else(|_| Some(BlockInfo::new(block.clone())))
-                    .unwrap_or_else(|| BlockInfo::new(block.clone()));
+                    .unwrap_or_else(|_| Some(BlockInfo::from(block_proposal.clone())))
+                    .unwrap_or_else(|| BlockInfo::from(block_proposal.clone()));
                 if block_info.signed_over {
                     debug!("{self}: Received a sign command for a block we are already signing over. Ignore it.");
                     return;
                 }
                 info!("{self}: Signing block";
-                         "block_consensus_hash" => %block.header.consensus_hash,
-                         "block_height" => block.header.chain_length,
-                         "pre_sign_block_id" => %block.block_id(),
+                         "block_consensus_hash" => %block_proposal.block.header.consensus_hash,
+                         "block_height" => block_proposal.block.header.chain_length,
+                         "pre_sign_block_id" => %block_proposal.block.block_id(),
                 );
                 match self.coordinator.start_signing_round(
-                    &block.serialize_to_vec(),
+                    &block_proposal.serialize_to_vec(),
                     *is_taproot,
                     *merkle_root,
                 ) {
@@ -432,7 +438,7 @@ impl Signer {
                         debug!("{self}: ACK: {ack:?}",);
                         block_info.signed_over = true;
                         self.signer_db
-                            .insert_block(self.reward_cycle, &block_info)
+                            .insert_block(&block_info)
                             .unwrap_or_else(|e| {
                                 error!("{self}: Failed to insert block in DB: {e:?}");
                             });
@@ -517,7 +523,7 @@ impl Signer {
                 let is_valid = self.verify_block_transactions(stacks_client, &block_info.block);
                 block_info.valid = Some(is_valid);
                 self.signer_db
-                    .insert_block(self.reward_cycle, &block_info)
+                    .insert_block(&block_info)
                     .unwrap_or_else(|_| panic!("{self}: Failed to insert block in DB"));
                 info!(
                     "{self}: Treating block validation for block {} as valid: {:?}",
@@ -574,7 +580,7 @@ impl Signer {
             "signed_over" => block_info.signed_over,
         );
         self.signer_db
-            .insert_block(self.reward_cycle, &block_info)
+            .insert_block(&block_info)
             .unwrap_or_else(|_| panic!("{self}: Failed to insert block in DB"));
     }
 
@@ -607,63 +613,6 @@ impl Signer {
         self.handle_packets(stacks_client, res, &packets, current_reward_cycle);
     }
 
-    /// Handle proposed blocks submitted by the miners to stackerdb
-    fn handle_proposed_blocks(
-        &mut self,
-        stacks_client: &StacksClient,
-        proposals: &[BlockProposalSigners],
-    ) {
-        for proposal in proposals {
-            if proposal.reward_cycle != self.reward_cycle {
-                debug!(
-                    "{self}: Received proposal for block outside of my reward cycle, ignoring.";
-                    "proposal_reward_cycle" => proposal.reward_cycle,
-                    "proposal_burn_height" => proposal.burn_height,
-                );
-                continue;
-            }
-            let sig_hash = proposal.block.header.signer_signature_hash();
-            match self.signer_db.block_lookup(self.reward_cycle, &sig_hash) {
-                Ok(Some(block)) => {
-                    debug!(
-                        "{self}: Received proposal for block already known, ignoring new proposal.";
-                        "signer_sighash" => %sig_hash,
-                        "proposal_burn_height" => proposal.burn_height,
-                        "vote" => ?block.vote.as_ref().map(|v| {
-                            if v.rejected {
-                                "REJECT"
-                            } else {
-                                "ACCEPT"
-                            }
-                        }),
-                        "signed_over" => block.signed_over,
-                    );
-                    continue;
-                }
-                Ok(None) => {
-                    // Store the block in our cache
-                    self.signer_db
-                        .insert_block(self.reward_cycle, &BlockInfo::new(proposal.block.clone()))
-                        .unwrap_or_else(|e| {
-                            error!("{self}: Failed to insert block in DB: {e:?}");
-                        });
-                    // Submit the block for validation
-                    stacks_client
-                        .submit_block_for_validation(proposal.block.clone())
-                        .unwrap_or_else(|e| {
-                            warn!("{self}: Failed to submit block for validation: {e:?}");
-                        });
-                }
-                Err(e) => {
-                    error!(
-                        "{self}: Failed to lookup block in DB: {e:?}. Dropping proposal request."
-                    );
-                    continue;
-                }
-            }
-        }
-    }
-
     /// Helper function for determining if the provided message is a DKG specific message
     fn is_dkg_message(msg: &Message) -> bool {
         matches!(
@@ -808,26 +757,35 @@ impl Signer {
         stacks_client: &StacksClient,
         nonce_request: &mut NonceRequest,
     ) -> Option<BlockInfo> {
-        let Some(block) =
-            NakamotoBlock::consensus_deserialize(&mut nonce_request.message.as_slice()).ok()
+        let Some(block_proposal) =
+            BlockProposalSigners::consensus_deserialize(&mut nonce_request.message.as_slice()).ok()
         else {
-            // We currently reject anything that is not a block
+            // We currently reject anything that is not a valid block proposal
             warn!("{self}: Received a nonce request for an unknown message stream. Reject it.",);
             return None;
         };
-        let signer_signature_hash = block.header.signer_signature_hash();
+        if block_proposal.reward_cycle != self.reward_cycle {
+            // We are not signing for this reward cycle. Reject the block
+            warn!(
+                "{self}: Received a nonce request for a different reward cycle. Reject it.";
+                "requested_reward_cycle" => block_proposal.reward_cycle,
+            );
+            return None;
+        }
+        // TODO: could add a check to ignore an old burn block height if we know its oudated. Would require us to store the burn block height we last saw on the side.
+        let signer_signature_hash = block_proposal.block.header.signer_signature_hash();
         let Some(mut block_info) = self
             .signer_db
             .block_lookup(self.reward_cycle, &signer_signature_hash)
             .expect("Failed to connect to signer DB")
         else {
             debug!(
-                "{self}: We have received a block sign request for a block we have not seen before. Cache the nonce request and submit the block for validation...";
-                "signer_sighash" => %block.header.signer_signature_hash(),
+                "{self}: received a nonce request for a new block. Submit block for validation. ";
+                "signer_sighash" => %signer_signature_hash,
             );
-            let block_info = BlockInfo::new_with_request(block.clone(), nonce_request.clone());
+            let block_info = BlockInfo::new_with_request(block_proposal, nonce_request.clone());
             stacks_client
-                .submit_block_for_validation(block)
+                .submit_block_for_validation(block_info.block.clone())
                 .unwrap_or_else(|e| {
                     warn!("{self}: Failed to submit block for validation: {e:?}",);
                 });
@@ -1000,7 +958,7 @@ impl Signer {
                         return None;
                     };
                     self.signer_db
-                        .insert_block(self.reward_cycle, &updated_block_info)
+                        .insert_block(&updated_block_info)
                         .unwrap_or_else(|_| panic!("{self}: Failed to insert block in DB"));
                     let process_request = updated_block_info.vote.is_some();
                     if !process_request {
@@ -1533,7 +1491,7 @@ impl Signer {
                 );
                 self.handle_signer_messages(stacks_client, res, messages, current_reward_cycle);
             }
-            Some(SignerEvent::MinerMessages(blocks, messages, miner_key)) => {
+            Some(SignerEvent::MinerMessages(messages, miner_key)) => {
                 if let Some(miner_key) = miner_key {
                     let miner_key = PublicKey::try_from(miner_key.to_bytes_compressed().as_slice())
                         .expect("FATAL: could not convert from StacksPublicKey to PublicKey");
@@ -1545,13 +1503,11 @@ impl Signer {
                     return Ok(());
                 }
                 debug!(
-                    "{self}: Received {} block proposals and {} messages from the miner",
-                    blocks.len(),
+                    "{self}: Received {} messages from the miner",
                     messages.len();
                     "miner_key" => ?miner_key,
                 );
                 self.handle_signer_messages(stacks_client, res, messages, current_reward_cycle);
-                self.handle_proposed_blocks(stacks_client, blocks);
             }
             Some(SignerEvent::StatusCheck) => {
                 debug!("{self}: Received a status check event.")