fix: don't resend block acceptance if already sent

stacks-common/src/util/macros.rs

@@ -426,8 +426,8 @@ macro_rules! impl_array_newtype {
             }
         }
 
+        #[allow(clippy::non_canonical_clone_impl)]
         impl Clone for $thing {
-            #[allow(clippy::non_canonical_clone_impl)]
             fn clone(&self) -> Self {
                 $thing(self.0.clone())
             }

stacks-node/src/tests/signer/commands/block_wait.rs

@@ -4,7 +4,6 @@ use std::sync::Arc;
 use libsigner::v0::messages::RejectReason;
 use madhouse::{Command, CommandWrapper};
 use proptest::prelude::{Just, Strategy};
-use proptest::prop_oneof;
 use stacks::chainstate::stacks::{TenureChangeCause, TenureChangePayload, TransactionPayload};
 
 use super::context::{SignerTestContext, SignerTestState};

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

@@ -13573,16 +13573,16 @@ fn reorg_attempts_count_towards_miner_validity() {
 ///
 /// Test Execution:
 /// Test validation endpoint is stalled.
-/// The miner proposes a block N.
+/// The miner A proposes a block N.
 /// Block proposals are stalled.
 /// A new tenure is started.
 /// The test waits for reorg_attempts_activity_timeout + 1 second.
-/// The miner proposes a block N'.
+/// The miner B proposes a block N'.
 /// The test waits for block proposal timeout + 1 second.
 /// The validation endpoint is resumed.
 /// The signers accept block N.
 /// The signers reject block N'.
-/// The miner proposes block N+1.
+/// The miner B proposes block N+1.
 /// The signers reject block N+1.
 ///
 /// Test Assertion:
@@ -13669,42 +13669,68 @@ fn reorg_attempts_activity_timeout_exceeded() {
     )
     .expect("Failed to update to Tenure B");
 
+    // Make sure that no subsequent proposal arrives before the block_proposal_timeout is exceeded
+    TEST_BROADCAST_PROPOSAL_STALL.set(vec![miner_pk.clone()]);
+    info!(
+        "------------------------- Wait for block N {} to arrive late  -------------------------",
+        block_proposal_n.header.signer_signature_hash()
+    );
+    // Allow block N validation to finish, but don't broadcast it yet
+    TEST_VALIDATE_STALL.set(false);
+    TEST_PAUSE_BLOCK_BROADCAST.set(true);
+    let reward_cycle = signer_test.get_current_reward_cycle();
+    wait_for_block_global_acceptance_from_signers(
+        30,
+        &block_proposal_n.header.signer_signature_hash(),
+        &signer_test.get_signer_public_keys(reward_cycle),
+    )
+    .expect("Timed out waiting for block proposal N to be globally accepted");
+
     let wait_time = reorg_attempts_activity_timeout.add(Duration::from_secs(1));
     info!("------------------------- Waiting {} Seconds for Reorg Activity Timeout to be Exceeded-------------------------", wait_time.as_secs());
     // Make sure to wait the reorg_attempts_activity_timeout AFTER the block is globally signed over
     // as this is the point where signers start considering from.
-    // Allow incoming mine to propose block N'
     std::thread::sleep(wait_time);
+    test_observer::clear();
+
+    // Allow incoming miner to propose block N'
     TEST_BROADCAST_PROPOSAL_STALL.set(vec![]);
     let block_proposal_n_prime =
         wait_for_block_proposal(30, chain_start.stacks_tip_height + 1, &miner_pk)
             .expect("Failed to get block proposal N'");
-    // Make sure that no subsequent proposal arrives before the block_proposal_timeout is exceeded
+    assert_ne!(block_proposal_n, block_proposal_n_prime);
+
+    // Pause proposals again to avoid any additional proposals
     TEST_BROADCAST_PROPOSAL_STALL.set(vec![miner_pk.clone()]);
-    info!("------------------------- Wait for block N' to arrive late  -------------------------");
-    // Allow block N validation to finish.
-    TEST_VALIDATE_STALL.set(false);
+    // Allow the block broadcast to proceed and then make sure we've advanced to block N
+    TEST_PAUSE_BLOCK_BROADCAST.set(false);
     wait_for(30, || {
         let chain_info = get_chain_info(&signer_test.running_nodes.conf);
         Ok(chain_info.stacks_tip_height > chain_before.stacks_tip_height)
     })
     .expect("Timed out waiting for stacks tip to advance to block N");
     let chain_after = get_chain_info(&signer_test.running_nodes.conf);
-    TEST_VALIDATE_STALL.set(true);
-    // We only need to wait the difference between the two timeouts now since we already slept for a min of reorg_attempts_activity_timeout + 1
-    let wait_time = block_proposal_timeout.saturating_sub(reorg_attempts_activity_timeout);
-    info!("------------------------- Waiting {} Seconds for Miner To be Marked Invalid -------------------------", wait_time.as_secs());
+
+    // We wait the remainder of the block proposal timeout
+    let wait_time = block_proposal_timeout
+        .saturating_sub(reorg_attempts_activity_timeout)
+        .saturating_add(Duration::from_secs(1));
+    info!("------------------------- Waiting {} Seconds for Miner to be Considered Inactive -------------------------", wait_time.as_secs());
     std::thread::sleep(wait_time);
+
+    info!("------------------------- Waiting for Miner To be Marked Invalid -------------------------");
     wait_for_state_machine_update_by_miner_tenure_id(
         30,
         &chain_start.pox_consensus,
         &signer_test.signer_addresses_versions(),
     )
     .expect("Failed to revert back to prior miner's tenure");
-    assert_ne!(block_proposal_n, block_proposal_n_prime);
     let chain_before = chain_after;
-    TEST_VALIDATE_STALL.set(false);
-    info!("------------------------- Wait for Block N' Rejection -------------------------");
+
+    info!(
+        "------------------------- Wait for Block N' {} Rejection -------------------------",
+        block_proposal_n_prime.header.signer_signature_hash()
+    );
     wait_for_block_global_rejection(
         30,
         &block_proposal_n_prime.header.signer_signature_hash(),

stacks-signer/CHANGELOG.md

@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to the versioning scheme outlined in the [README.md](README.md).
 
+## [Unreleased]
+
+### Changed
+
+- Avoid sending duplicate block acceptance messages when additional pre-commits arrive
+
 ## [3.3.0.0.2.0]
 
 ### Added

stacks-signer/src/signerdb.rs

@@ -123,7 +123,9 @@ BlockState {
     /// A threshold number of signers have signed the block
     GloballyAccepted = 3,
     /// A threshold number of signers have rejected the block
-    GloballyRejected = 4
+    GloballyRejected = 4,
+    /// The block is pre-committed by the signer, but not yet signed
+    PreCommitted = 5
 });
 
 impl TryFrom<u8> for BlockState {
@@ -135,6 +137,7 @@ impl TryFrom<u8> for BlockState {
             2 => BlockState::LocallyRejected,
             3 => BlockState::GloballyAccepted,
             4 => BlockState::GloballyRejected,
+            5 => BlockState::PreCommitted,
             _ => return Err("Invalid block state".into()),
         };
         Ok(state)
@@ -149,6 +152,7 @@ impl Display for BlockState {
             BlockState::LocallyRejected => "LocallyRejected",
             BlockState::GloballyAccepted => "GloballyAccepted",
             BlockState::GloballyRejected => "GloballyRejected",
+            BlockState::PreCommitted => "PreCommitted",
         };
         write!(f, "{state}")
     }
@@ -163,6 +167,7 @@ impl TryFrom<&str> for BlockState {
             "LocallyRejected" => BlockState::LocallyRejected,
             "GloballyAccepted" => BlockState::GloballyAccepted,
             "GloballyRejected" => BlockState::GloballyRejected,
+            "PreCommitted" => BlockState::PreCommitted,
             _ => return Err("Unparsable block state".into()),
         };
         Ok(state)
@@ -182,11 +187,11 @@ pub struct BlockInfo {
     pub vote: Option<NakamotoBlockVote>,
     /// Whether the block contents are valid
     pub valid: Option<bool>,
-    /// Whether this block is already being signed over
+    /// Whether this block is already being signed over (pre-committed or signed by self or group)
     pub signed_over: bool,
     /// Time at which the proposal was received by this signer (epoch time in seconds)
     pub proposed_time: u64,
-    /// Time at which the proposal was signed by this signer (epoch time in seconds)
+    /// Time at which the proposal was pre-committed or signed by this signer (epoch time in seconds)
     pub signed_self: Option<u64>,
     /// Time at which the proposal was signed by a threshold in the signer set (epoch time in seconds)
     pub signed_group: Option<u64>,
@@ -253,6 +258,17 @@ impl BlockInfo {
         Ok(())
     }
 
+    /// Mark this block as valid and pre-committed. We set the `signed_self`
+    /// timestamp here because pre-committing to a block implies the same
+    /// behavior as a local acceptance from the signer's perspective.
+    pub fn mark_pre_committed(&mut self) -> Result<(), String> {
+        self.move_to(BlockState::PreCommitted)?;
+        self.valid = Some(true);
+        self.signed_over = true;
+        self.signed_self.get_or_insert(get_epoch_time_secs());
+        Ok(())
+    }
+
     /// Mark this block as valid, signed over, and records a group timestamp in the block info if it wasn't
     ///  already set.
     fn mark_globally_accepted(&mut self) -> Result<(), String> {
@@ -296,6 +312,7 @@ impl BlockInfo {
             ),
             BlockState::GloballyAccepted => !matches!(prev_state, BlockState::GloballyRejected),
             BlockState::GloballyRejected => !matches!(prev_state, BlockState::GloballyAccepted),
+            BlockState::PreCommitted => matches!(prev_state, BlockState::Unprocessed),
         }
     }
 
@@ -323,7 +340,7 @@ impl BlockInfo {
     pub fn is_locally_finalized(&self) -> bool {
         matches!(
             self.state,
-            BlockState::LocallyAccepted | BlockState::LocallyRejected
+            BlockState::PreCommitted | BlockState::LocallyAccepted | BlockState::LocallyRejected
         )
     }
 }
@@ -1182,11 +1199,12 @@ impl SignerDb {
         &self,
         tenure: &ConsensusHash,
     ) -> Result<Option<BlockInfo>, DBError> {
-        let query = "SELECT block_info FROM blocks WHERE consensus_hash = ?1 AND state IN (?2, ?3) ORDER BY stacks_height DESC LIMIT 1";
+        let query = "SELECT block_info FROM blocks WHERE consensus_hash = ?1 AND state IN (?2, ?3, ?4) ORDER BY stacks_height DESC LIMIT 1";
         let args = params![
             tenure,
             &BlockState::GloballyAccepted.to_string(),
-            &BlockState::LocallyAccepted.to_string()
+            &BlockState::LocallyAccepted.to_string(),
+            &BlockState::PreCommitted.to_string(),
         ];
         let result: Option<String> = query_row(&self.db, query, args)?;
 

stacks-signer/src/v0/signer.rs

@@ -1036,6 +1036,15 @@ impl Signer {
             return;
         };
 
+        if block_info.state == BlockState::LocallyAccepted
+            || block_info.state == BlockState::LocallyRejected
+        {
+            debug!(
+                "{self}: Received pre-commit for a block that we have already responded to. Ignoring...",
+            );
+            return;
+        }
+
         if self.signer_db.has_committed(block_hash, stacker_address).inspect_err(|e| warn!("Failed to check if pre-commit message already considered for {stacker_address:?} for {block_hash}: {e}")).unwrap_or(false) {
             debug!("{self}: Already considered pre-commit message from {stacker_address:?} for {block_hash}. Ignoring...");
             return;
@@ -1404,7 +1413,7 @@ impl Signer {
             self.handle_block_rejection(&block_rejection, sortition_state);
             self.send_block_response(&block_info.block, block_rejection.into());
         } else {
-            if let Err(e) = block_info.mark_locally_accepted(false) {
+            if let Err(e) = block_info.mark_pre_committed() {
                 if !block_info.has_reached_consensus() {
                     warn!("{self}: Failed to mark block as locally accepted: {e:?}",);
                     return;

stackslib/src/net/api/postblock_proposal.rs

@@ -193,6 +193,9 @@ impl BlockValidateResponse {
 #[cfg(any(test, feature = "testing"))]
 fn fault_injection_validation_delay() {
     let delay = TEST_VALIDATE_DELAY_DURATION_SECS.get();
+    if delay == 0 {
+        return;
+    }
     warn!("Sleeping for {} seconds to simulate slow processing", delay);
     thread::sleep(Duration::from_secs(delay));
 }