Cleanup comments and remove unnecessary clone

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

Author: jferrant

stacks-signer/src/config.rs

@@ -161,7 +161,7 @@ pub struct GlobalConfig {
     pub block_proposal_timeout: Duration,
     /// An optional custom Chain ID
     pub chain_id: Option<u32>,
-    /// How long to wait for a response from a block proposal validation response from the node
+    /// How long to wait in for a response from a block proposal validation response from the node
     /// before marking that block as invalid and rejecting it
     pub block_proposal_validation_timeout: Duration,
 }
@@ -193,8 +193,8 @@ struct RawConfigFile {
     pub block_proposal_timeout_ms: Option<u64>,
     /// An optional custom Chain ID
     pub chain_id: Option<u32>,
-    /// How long to wait for a response from a block proposal validation response from the node
-    /// before marking that block as invalid and rejecting it in milliseconds.
+    /// How long to wait n milliseconds for a response from a block proposal validation response from the node
+    /// before marking that block as invalid and rejecting it
     pub block_proposal_validation_timeout_ms: Option<u64>,
 }
 

stacks-signer/src/v0/signer.rs

@@ -670,17 +670,15 @@ impl Signer {
     /// Check the current tracked submitted block proposal to see if it has timed out.
     /// Broadcasts a rejection and marks the block locally rejected if it has.
     fn check_submitted_block_proposal(&mut self) {
-        let Some((block_proposal, block_submission)) = self.submitted_block_proposal.clone() else {
+        let Some((block_proposal, block_submission)) = self.submitted_block_proposal.take() else {
             // Nothing to check.
             return;
         };
         if block_submission.elapsed() < self.block_proposal_validation_timeout {
-            // Not expired yet.
+            // Not expired yet. Put it back!
+            self.submitted_block_proposal = Some((block_proposal, block_submission));
             return;
         }
-        // Let us immediately flush, even if we later encounter an error broadcasting our responses.
-        // We should still attempt to handle a new proposal at this point.
-        self.submitted_block_proposal = None;
         let signature_sighash = block_proposal.block.header.signer_signature_hash();
         // For mutability reasons, we need to take the block_info out of the map and add it back after processing
         let mut block_info = match self
@@ -710,6 +708,8 @@ impl Signer {
                 return;
             }
         };
+        // We cannot determine the validity of the block, but we have not reached consensus on it yet.
+        // Reject it so we aren't holding up the network because of our inaction.
         warn!(
             "{self}: Failed to receive block validation response within {} ms. Rejecting block.", self.block_proposal_validation_timeout.as_millis();
             "signer_sighash" => %signature_sighash,
@@ -721,7 +721,6 @@ impl Signer {
             &self.private_key,
             self.mainnet,
         );
-        // We know proposal is invalid. Send rejection message, do not do further validation
         if let Err(e) = block_info.mark_locally_rejected() {
             warn!("{self}: Failed to mark block as locally rejected: {e:?}",);
         };
@@ -855,6 +854,7 @@ impl Signer {
             .map(|(proposal, _)| &proposal.block.header.signer_signature_hash() == block_hash)
             .unwrap_or(false)
         {
+            // Consensus reached! No longer bother tracking its validation submission to the node as we are too late to participate in the decision anyway.
             self.submitted_block_proposal = None;
         }
     }
@@ -1005,6 +1005,7 @@ impl Signer {
             .map(|(proposal, _)| &proposal.block.header.signer_signature_hash() == block_hash)
             .unwrap_or(false)
         {
+            // Consensus reached! No longer bother tracking its validation submission to the node as we are too late to participate in the decision anyway.
             self.submitted_block_proposal = None;
         }
     }

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

@@ -5608,7 +5608,9 @@ fn multiple_miners_with_custom_chain_id() {
     signer_test.shutdown();
 }
 
-// Teimout a block validation response
+// Ensures that a signer will issue ConnectivityIssues rejections if a block submission
+// times out. Also ensures that no other proposal gets submitted for validation if we
+// are already waiting for a block submission response.
 #[test]
 #[ignore]
 fn block_validation_response_timeout() {
@@ -5732,11 +5734,12 @@ fn block_validation_response_timeout() {
             };
             // We are waiting for the original block proposal which will have a diff signature to our
             // second proposed block.
-            if signer_signature_hash != block_signer_signature_hash_1 {
+            assert_ne!(
+                signer_signature_hash, block_signer_signature_hash_1,
+                "Received a rejection for the wrong block"
+            );
+            if matches!(reason_code, RejectCode::ConnectivityIssues) {
                 rejected_signers.push(signature);
-                if matches!(reason_code, RejectCode::ConnectivityIssues) {
-                    break;
-                }
             }
         }
         assert!(