diff --git a/stacks-common/src/util/macros.rs b/stacks-common/src/util/macros.rs index cbf602cfa1..ac3d6f94ab 100644 --- a/stacks-common/src/util/macros.rs +++ b/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()) } diff --git a/stacks-node/src/tests/signer/commands/block_wait.rs b/stacks-node/src/tests/signer/commands/block_wait.rs index 04e9b9f2b7..7706055a43 100644 --- a/stacks-node/src/tests/signer/commands/block_wait.rs +++ b/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}; diff --git a/stacks-node/src/tests/signer/v0.rs b/stacks-node/src/tests/signer/v0.rs index 06556bc586..95eb97524c 100644 --- a/stacks-node/src/tests/signer/v0.rs +++ b/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(), diff --git a/stacks-signer/CHANGELOG.md b/stacks-signer/CHANGELOG.md index 709abc6a24..632a924a8d 100644 --- a/stacks-signer/CHANGELOG.md +++ b/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 diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index 98b6b557d3..c6b34c216c 100644 --- a/stacks-signer/src/signerdb.rs +++ b/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 for BlockState { @@ -135,6 +137,7 @@ impl TryFrom 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, /// Whether the block contents are valid pub valid: Option, - /// 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, /// Time at which the proposal was signed by a threshold in the signer set (epoch time in seconds) pub signed_group: Option, @@ -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, 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 = query_row(&self.db, query, args)?; diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index fb40e39b2b..e5ad0e7b0d 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/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; diff --git a/stackslib/src/net/api/postblock_proposal.rs b/stackslib/src/net/api/postblock_proposal.rs index 431af42351..042cbc08dc 100644 --- a/stackslib/src/net/api/postblock_proposal.rs +++ b/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)); }