diff --git a/CHANGELOG.md b/CHANGELOG.md index bc8c0e1f92..f6625ccd7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE ### Added +- Added `SignerMessage::PreBlockCommit` for 2-phase commit block signing - Added field `vm_error` to EventObserver transaction outputs - Added new `ValidateRejectCode` values to the `/v3/block_proposal` endpoint - Added `StateMachineUpdateContent::V1` to support a vector of `StacksTransaction` expected to be replayed in subsequent Stacks blocks diff --git a/libsigner/src/v0/messages.rs b/libsigner/src/v0/messages.rs index ccd098a8b3..2e6924a955 100644 --- a/libsigner/src/v0/messages.rs +++ b/libsigner/src/v0/messages.rs @@ -89,7 +89,9 @@ MessageSlotID { /// Block Response message from signers BlockResponse = 1, /// Signer State Machine Update - StateMachineUpdate = 2 + StateMachineUpdate = 2, + /// Block Pre-commit message from signers before they commit to a block response + BlockPreCommit = 3 }); define_u8_enum!( @@ -132,7 +134,9 @@ SignerMessageTypePrefix { /// Mock block message from Epoch 2.5 miners MockBlock = 5, /// State machine update - StateMachineUpdate = 6 + StateMachineUpdate = 6, + /// Block Pre-commit message + BlockPreCommit = 7 }); #[cfg_attr(test, mutants::skip)] @@ -155,7 +159,7 @@ impl MessageSlotID { #[cfg_attr(test, mutants::skip)] impl Display for MessageSlotID { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{:?}({})", self, self.to_u8()) + write!(f, "{self:?}({})", self.to_u8()) } } @@ -179,6 +183,7 @@ impl From<&SignerMessage> for SignerMessageTypePrefix { SignerMessage::MockSignature(_) => SignerMessageTypePrefix::MockSignature, SignerMessage::MockBlock(_) => SignerMessageTypePrefix::MockBlock, SignerMessage::StateMachineUpdate(_) => SignerMessageTypePrefix::StateMachineUpdate, + SignerMessage::BlockPreCommit(_) => SignerMessageTypePrefix::BlockPreCommit, } } } @@ -200,6 +205,8 @@ pub enum SignerMessage { MockBlock(MockBlock), /// A state machine update StateMachineUpdate(StateMachineUpdate), + /// The pre commit message from signers for other signers to observe + BlockPreCommit(Sha512Trunc256Sum), } impl SignerMessage { @@ -215,6 +222,7 @@ impl SignerMessage { | Self::MockBlock(_) => None, Self::BlockResponse(_) | Self::MockSignature(_) => Some(MessageSlotID::BlockResponse), // Mock signature uses the same slot as block response since its exclusively for epoch 2.5 testing Self::StateMachineUpdate(_) => Some(MessageSlotID::StateMachineUpdate), + Self::BlockPreCommit(_) => Some(MessageSlotID::BlockPreCommit), } } } @@ -234,6 +242,9 @@ impl StacksMessageCodec for SignerMessage { SignerMessage::StateMachineUpdate(state_machine_update) => { state_machine_update.consensus_serialize(fd) } + SignerMessage::BlockPreCommit(signer_signature_hash) => { + signer_signature_hash.consensus_serialize(fd) + } }?; Ok(()) } @@ -271,6 +282,10 @@ impl StacksMessageCodec for SignerMessage { let state_machine_update = StacksMessageCodec::consensus_deserialize(fd)?; SignerMessage::StateMachineUpdate(state_machine_update) } + SignerMessageTypePrefix::BlockPreCommit => { + let signer_signature_hash = StacksMessageCodec::consensus_deserialize(fd)?; + SignerMessage::BlockPreCommit(signer_signature_hash) + } }; Ok(message) } diff --git a/stacks-signer/CHANGELOG.md b/stacks-signer/CHANGELOG.md index 33679040ec..c78a500fbc 100644 --- a/stacks-signer/CHANGELOG.md +++ b/stacks-signer/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE ## [Unreleased] +### Added + +- Added `SignerMessage::BlockPreCommit` message handling; signers now collect until a threshold is reached before issuing a block signature, implementing a proper 2-phase commit. + ### Changed - Upgraded `SUPPORTED_SIGNER_PROTOCOL_VERSION` to 1 diff --git a/stacks-signer/src/monitoring/mod.rs b/stacks-signer/src/monitoring/mod.rs index dabf529b6b..b92ec981b5 100644 --- a/stacks-signer/src/monitoring/mod.rs +++ b/stacks-signer/src/monitoring/mod.rs @@ -86,6 +86,11 @@ pub mod actions { BLOCK_RESPONSES_SENT.with_label_values(&[label_value]).inc(); } + /// Increment the block pre-commit sent counter + pub fn increment_block_pre_commits_sent() { + BLOCK_PRE_COMMITS_SENT.inc(); + } + /// Increment the number of block proposals received pub fn increment_block_proposals_received() { BLOCK_PROPOSALS_RECEIVED.inc(); @@ -203,6 +208,9 @@ pub mod actions { /// Increment the block responses sent counter pub fn increment_block_responses_sent(_accepted: bool) {} + /// Increment the block pre-commits sent counter + pub fn increment_block_pre_commits_sent() {} + /// Increment the number of block proposals received pub fn increment_block_proposals_received() {} diff --git a/stacks-signer/src/monitoring/prometheus.rs b/stacks-signer/src/monitoring/prometheus.rs index 2114b7f4cd..4d755b5170 100644 --- a/stacks-signer/src/monitoring/prometheus.rs +++ b/stacks-signer/src/monitoring/prometheus.rs @@ -43,6 +43,11 @@ lazy_static! { &["response_type"] ) .unwrap(); + pub static ref BLOCK_PRE_COMMITS_SENT: IntCounter = register_int_counter!(opts!( + "stacks_signer_block_pre_commits_sent", + "The number of block pre-commits sent by the signer" + )) + .unwrap(); pub static ref BLOCK_PROPOSALS_RECEIVED: IntCounter = register_int_counter!(opts!( "stacks_signer_block_proposals_received", "The number of block proposals received by the signer" diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index 5de793edc0..b408be9826 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -532,6 +532,18 @@ CREATE TABLE IF NOT EXISTS signer_state_machine_updates ( PRIMARY KEY (signer_addr, reward_cycle) ) STRICT;"#; +static CREATE_BLOCK_PRE_COMMITS_TABLE: &str = r#" +CREATE TABLE IF NOT EXISTS block_pre_commits ( + -- The block sighash commits to all of the stacks and burnchain state as of its parent, + -- as well as the tenure itself so there's no need to include the reward cycle. Just + -- the sighash is sufficient to uniquely identify the block across all burnchain, PoX, + -- and stacks forks. + signer_signature_hash TEXT NOT NULL, + -- signer address committing to sign the block + signer_addr TEXT NOT NULL, + PRIMARY KEY (signer_signature_hash, signer_addr) +) STRICT;"#; + static SCHEMA_1: &[&str] = &[ DROP_SCHEMA_0, CREATE_DB_CONFIG, @@ -613,9 +625,14 @@ static SCHEMA_11: &[&str] = &[ "INSERT INTO db_config (version) VALUES (11);", ]; +static SCHEMA_12: &[&str] = &[ + CREATE_BLOCK_PRE_COMMITS_TABLE, + "INSERT INTO db_config (version) VALUES (12);", +]; + impl SignerDb { /// The current schema version used in this build of the signer binary. - pub const SCHEMA_VERSION: u32 = 11; + pub const SCHEMA_VERSION: u32 = 12; /// Create a new `SignerState` instance. /// This will create a new SQLite database at the given path @@ -799,6 +816,20 @@ impl SignerDb { Ok(()) } + /// Migrate from schema 11 to schema 12 + fn schema_12_migration(tx: &Transaction) -> Result<(), DBError> { + if Self::get_schema_version(tx)? >= 12 { + // no migration necessary + return Ok(()); + } + + for statement in SCHEMA_12.iter() { + tx.execute_batch(statement)?; + } + + Ok(()) + } + /// Register custom scalar functions used by the database fn register_scalar_functions(&self) -> Result<(), DBError> { // Register helper function for determining if a block is a tenure change transaction @@ -843,7 +874,8 @@ impl SignerDb { 8 => Self::schema_9_migration(&sql_tx)?, 9 => Self::schema_10_migration(&sql_tx)?, 10 => Self::schema_11_migration(&sql_tx)?, - 11 => break, + 11 => Self::schema_12_migration(&sql_tx)?, + 12 => break, x => return Err(DBError::Other(format!( "Database schema is newer than supported by this binary. Expected version = {}, Database version = {x}", Self::SCHEMA_VERSION, @@ -1428,6 +1460,39 @@ impl SignerDb { None => Ok(0), } } + + /// Record an observed block pre commit + pub fn add_block_pre_commit( + &self, + block_sighash: &Sha512Trunc256Sum, + address: &StacksAddress, + ) -> Result<(), DBError> { + let qry = "INSERT OR REPLACE INTO block_pre_commits (signer_signature_hash, signer_addr) VALUES (?1, ?2);"; + let args = params![block_sighash, address.to_string()]; + + debug!("Inserting block pre commit."; + "signer_signature_hash" => %block_sighash, + "signer_addr" => %address); + + self.db.execute(qry, args)?; + Ok(()) + } + + /// Get all pre committers for a block + pub fn get_block_pre_committers( + &self, + block_sighash: &Sha512Trunc256Sum, + ) -> Result, DBError> { + let qry = "SELECT signer_addr FROM block_pre_commits WHERE signer_signature_hash = ?1"; + let args = params![block_sighash]; + let addrs_txt: Vec = query_rows(&self.db, qry, args)?; + + let res: Result, _> = addrs_txt + .into_iter() + .map(|addr| StacksAddress::from_string(&addr).ok_or(DBError::Corruption)) + .collect(); + res + } } fn try_deserialize(s: Option) -> Result, DBError> @@ -2619,4 +2684,49 @@ pub mod tests { "latency between updates should be 10 second" ); } + + #[test] + fn insert_and_get_state_block_pre_commits() { + let db_path = tmp_db_path(); + let db = SignerDb::new(db_path).expect("Failed to create signer db"); + let block_sighash1 = Sha512Trunc256Sum([1u8; 32]); + let address1 = StacksAddress::p2pkh( + false, + &StacksPublicKey::from_private(&StacksPrivateKey::random()), + ); + let block_sighash2 = Sha512Trunc256Sum([2u8; 32]); + let address2 = StacksAddress::p2pkh( + false, + &StacksPublicKey::from_private(&StacksPrivateKey::random()), + ); + let address3 = StacksAddress::p2pkh( + false, + &StacksPublicKey::from_private(&StacksPrivateKey::random()), + ); + assert!(db + .get_block_pre_committers(&block_sighash1) + .unwrap() + .is_empty()); + + db.add_block_pre_commit(&block_sighash1, &address1).unwrap(); + assert_eq!( + db.get_block_pre_committers(&block_sighash1).unwrap(), + vec![address1] + ); + + db.add_block_pre_commit(&block_sighash1, &address2).unwrap(); + let commits = db.get_block_pre_committers(&block_sighash1).unwrap(); + assert_eq!(commits.len(), 2); + assert!(commits.contains(&address2)); + assert!(commits.contains(&address1)); + + db.add_block_pre_commit(&block_sighash2, &address3).unwrap(); + let commits = db.get_block_pre_committers(&block_sighash1).unwrap(); + assert_eq!(commits.len(), 2); + assert!(commits.contains(&address2)); + assert!(commits.contains(&address1)); + let commits = db.get_block_pre_committers(&block_sighash2).unwrap(); + assert_eq!(commits.len(), 1); + assert!(commits.contains(&address3)); + } } diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index 1080b0543a..41ec6eda48 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -410,6 +410,11 @@ impl Signer { ), SignerMessage::StateMachineUpdate(update) => self .handle_state_machine_update(signer_public_key, update, received_time), + SignerMessage::BlockPreCommit(hash) => self.handle_block_pre_commit( + stacks_client, + &StacksAddress::p2pkh(self.mainnet, signer_public_key), + hash, + ), _ => {} } } @@ -665,11 +670,11 @@ impl Signer { /// The actual `send_block_response` implementation. Declared so that we do /// not need to duplicate in testing. - fn impl_send_block_response( - &mut self, - block: Option<&NakamotoBlock>, - block_response: BlockResponse, - ) { + fn impl_send_block_response(&mut self, block: &NakamotoBlock, block_response: BlockResponse) { + #[cfg(any(test, feature = "testing"))] + if self.test_skip_signature_broadcast(&block_response) { + return; + } info!( "{self}: Broadcasting a block response to stacks node: {block_response:?}"; ); @@ -686,9 +691,7 @@ impl Signer { ); } crate::monitoring::actions::increment_block_responses_sent(accepted); - if let Some(block) = block { - crate::monitoring::actions::record_block_response_latency(block); - } + crate::monitoring::actions::record_block_response_latency(block); } Err(e) => { warn!("{self}: Failed to send block response to stacker-db: {e:?}",); @@ -697,11 +700,7 @@ impl Signer { } #[cfg(any(test, feature = "testing"))] - fn send_block_response( - &mut self, - block: Option<&NakamotoBlock>, - block_response: BlockResponse, - ) { + fn send_block_response(&mut self, block: &NakamotoBlock, block_response: BlockResponse) { const NUM_REPEATS: usize = 1; let mut count = 0; let public_keys = TEST_REPEAT_PROPOSAL_RESPONSE.get(); @@ -719,14 +718,34 @@ impl Signer { } #[cfg(not(any(test, feature = "testing")))] - fn send_block_response( - &mut self, - block: Option<&NakamotoBlock>, - block_response: BlockResponse, - ) { + fn send_block_response(&mut self, block: &NakamotoBlock, block_response: BlockResponse) { self.impl_send_block_response(block, block_response) } + /// Send a pre block commit message to signers to indicate that we will be signing the proposed block + fn send_block_pre_commit(&mut self, block_hash: Sha512Trunc256Sum) { + info!( + "{self}: Broadcasting a block pre-commit to stacks node: {block_hash:?}"; + ); + match self + .stackerdb + .send_message_with_retry(SignerMessage::BlockPreCommit(block_hash)) + { + Ok(ack) => { + if !ack.accepted { + warn!( + "{self}: Block pre-commit not accepted by stacker-db: {:?}", + ack.reason + ); + } + crate::monitoring::actions::increment_block_pre_commits_sent(); + } + Err(e) => { + warn!("{self}: Failed to send block pre-commit to stacker-db: {e:?}",); + } + } + } + /// Handle signer state update message fn handle_state_machine_update( &mut self, @@ -758,6 +777,95 @@ impl Signer { ); } + /// Handle pre-commit message from another signer + fn handle_block_pre_commit( + &mut self, + stacks_client: &StacksClient, + stacker_address: &StacksAddress, + block_hash: &Sha512Trunc256Sum, + ) { + debug!( + "{self}: Received a pre-commit from signer ({stacker_address:?}) for block ({block_hash})", + ); + let Some(mut block_info) = self.block_lookup_by_reward_cycle(block_hash) else { + debug!( + "{self}: Received block commit for a block we have not seen before. Ignoring..." + ); + return; + }; + if block_info.has_reached_consensus() { + debug!("{self}: Received block pre-commit for a block that is already marked as {}. Ignoring...", block_info.state); + return; + }; + // Make sure the sender is part of our signing set + let is_valid_sender = self.signer_addresses.iter().any(|addr| { + // it only matters that the address hash bytes match + stacker_address.bytes() == addr.bytes() + }); + + if !is_valid_sender { + debug!("{self}: Receive a pre-commit message from an unknown sender {stacker_address:?}. Will not store."); + return; + } + + // commit message is from a valid sender! store it + self.signer_db + .add_block_pre_commit(block_hash, stacker_address) + .unwrap_or_else(|_| panic!("{self}: Failed to save block pre-commit")); + + // do we have enough pre-commits to reach consensus? + // i.e. is the threshold reached? + let committers = self + .signer_db + .get_block_pre_committers(block_hash) + .unwrap_or_else(|_| panic!("{self}: Failed to load block commits")); + + let commit_weight = self.compute_signature_signing_weight(committers.iter()); + let total_weight = self.compute_signature_total_weight(); + + let min_weight = NakamotoBlockHeader::compute_voting_weight_threshold(total_weight) + .unwrap_or_else(|_| { + panic!("{self}: Failed to compute threshold weight for {total_weight}") + }); + + if min_weight > commit_weight { + debug!( + "{self}: Not enough pre-committed to block {block_hash} (have {commit_weight}, need at least {min_weight}/{total_weight})" + ); + return; + } + + // have enough commits, so maybe we should actually broadcast our signature... + if block_info.valid == Some(false) { + // We already marked this block as invalid. We should not do anything further as we do not change our votes on rejected blocks. + debug!( + "{self}: Enough pre-committed to block {block_hash}, but we do not view the block as valid. Doing nothing." + ); + return; + } + if block_info.signed_self.is_some() { + debug!("{self}: Already pre-committed and signed block {block_hash}. Will not broadcast again"); + // We already marked the block as locally accepted and signed over it. No need to sign again. + return; + } + // It is only considered globally accepted IFF we receive a new block event confirming it OR see the chain tip of the node advance to it. + if let Err(e) = block_info.mark_locally_accepted(false) { + if !block_info.has_reached_consensus() { + warn!("{self}: Failed to mark block as locally accepted: {e:?}",); + } + } + + self.signer_db + .insert_block(&block_info) + .unwrap_or_else(|e| self.handle_insert_block_error(e)); + let block_response = self.create_block_acceptance(&block_info.block); + // have to save the signature _after_ the block info + if let Some(accepted) = block_response.as_block_accepted() { + self.handle_block_signature(stacks_client, accepted); + }; + self.impl_send_block_response(&block_info.block, block_response); + } + /// Handle block proposal messages submitted to signers stackerdb fn handle_block_proposal( &mut self, @@ -854,7 +962,7 @@ impl Signer { if let Some(block_response) = block_response { // We know proposal is invalid. Send rejection message, do not do further validation and do not store it. - self.send_block_response(Some(&block_info.block), block_response); + self.send_block_response(&block_info.block, block_response); } else { // Just in case check if the last block validation submission timed out. self.check_submitted_block_proposal(); @@ -908,7 +1016,7 @@ impl Signer { return; }; - self.impl_send_block_response(Some(&block_info.block), block_response); + self.impl_send_block_response(&block_info.block, block_response); } /// Handle block response messages from a signer @@ -1009,12 +1117,12 @@ impl Signer { None } - /// Handle the block validate ok response. Returns our block response if we have one + /// Handle the block validate ok response. fn handle_block_validate_ok( &mut self, stacks_client: &StacksClient, block_validate_ok: &BlockValidateOk, - ) -> Option { + ) { crate::monitoring::actions::increment_block_validation_responses(true); let signer_signature_hash = block_validate_ok.signer_signature_hash; if self @@ -1027,12 +1135,12 @@ impl Signer { // For mutability reasons, we need to take the block_info out of the map and add it back after processing let Some(mut block_info) = self.block_lookup_by_reward_cycle(&signer_signature_hash) else { // We have not seen this block before. Why are we getting a response for it? - debug!("{self}: Received a block validate response for a block we have not seen before. Ignoring..."); - return None; + debug!("{self}: Received block validate response for a block we have not seen before. Ignoring..."); + return; }; if block_info.is_locally_finalized() { debug!("{self}: Received block validation for a block that is already marked as {}. Ignoring...", block_info.state); - return None; + return; } if let Some(block_response) = @@ -1044,19 +1152,12 @@ impl Signer { warn!("{self}: Failed to mark block as locally rejected: {e:?}"); } }; - self.impl_send_block_response(Some(&block_info.block), block_response); + self.impl_send_block_response(&block_info.block, block_response); self.signer_db .insert_block(&block_info) .unwrap_or_else(|e| self.handle_insert_block_error(e)); - None } else { - if let Err(e) = block_info.mark_locally_accepted(false) { - if !block_info.has_reached_consensus() { - warn!("{self}: Failed to mark block as locally accepted: {e:?}",); - return None; - } - block_info.signed_self.get_or_insert(get_epoch_time_secs()); - } + block_info.valid = Some(true); // Record the block validation time but do not consider stx transfers or boot contract calls block_info.validation_time_ms = if block_validate_ok.cost.is_zero() { Some(0) @@ -1067,19 +1168,19 @@ impl Signer { self.signer_db .insert_block(&block_info) .unwrap_or_else(|e| self.handle_insert_block_error(e)); - let block_response = self.create_block_acceptance(&block_info.block); + self.send_block_pre_commit(signer_signature_hash); // have to save the signature _after_ the block info - self.handle_block_signature(stacks_client, block_response.as_block_accepted()?); - Some(block_response) + let address = self.stacks_address; + self.handle_block_pre_commit(stacks_client, &address, &signer_signature_hash); } } - /// Handle the block validate reject response. Returns our block response if we have one + /// Handle the block validate reject response. fn handle_block_validate_reject( &mut self, block_validate_reject: &BlockValidateReject, sortition_state: &mut Option, - ) -> Option { + ) { crate::monitoring::actions::increment_block_validation_responses(false); let signer_signature_hash = block_validate_reject.signer_signature_hash; if self @@ -1092,16 +1193,16 @@ impl Signer { let Some(mut block_info) = self.block_lookup_by_reward_cycle(&signer_signature_hash) else { // We have not seen this block before. Why are we getting a response for it? debug!("{self}: Received a block validate response for a block we have not seen before. Ignoring..."); - return None; + return; }; if block_info.is_locally_finalized() { debug!("{self}: Received block validation for a block that is already marked as {}. Ignoring...", block_info.state); - return None; + return; } if let Err(e) = block_info.mark_locally_rejected() { if !block_info.has_reached_consensus() { warn!("{self}: Failed to mark block as locally rejected: {e:?}",); - return None; + return; } } let block_rejection = BlockRejection::from_validate_rejection( @@ -1122,7 +1223,8 @@ impl Signer { .insert_block(&block_info) .unwrap_or_else(|e| self.handle_insert_block_error(e)); self.handle_block_rejection(&block_rejection, sortition_state); - Some(BlockResponse::Rejected(block_rejection)) + let response = BlockResponse::Rejected(block_rejection); + self.impl_send_block_response(&block_info.block, response); } /// Handle the block validate response returned from our prior calls to submit a block for validation @@ -1133,15 +1235,15 @@ impl Signer { sortition_state: &mut Option, ) { info!("{self}: Received a block validate response: {block_validate_response:?}"); - let block_response = match block_validate_response { + match block_validate_response { BlockValidateResponse::Ok(block_validate_ok) => { crate::monitoring::actions::record_block_validation_latency( block_validate_ok.validation_time_ms, ); - self.handle_block_validate_ok(stacks_client, block_validate_ok) + self.handle_block_validate_ok(stacks_client, block_validate_ok); } BlockValidateResponse::Reject(block_validate_reject) => { - self.handle_block_validate_reject(block_validate_reject, sortition_state) + self.handle_block_validate_reject(block_validate_reject, sortition_state); } }; // Remove this block validation from the pending table @@ -1149,16 +1251,6 @@ impl Signer { self.signer_db .remove_pending_block_validation(&signer_sig_hash) .unwrap_or_else(|e| warn!("{self}: Failed to remove pending block validation: {e:?}")); - - if let Some(response) = block_response { - let block = self - .signer_db - .block_lookup(&signer_sig_hash) - .unwrap_or_default() - .map(|info| info.block); - self.impl_send_block_response(block.as_ref(), response); - }; - // Check if there is a pending block validation that we need to submit to the node self.check_pending_block_validations(stacks_client); } @@ -1249,7 +1341,7 @@ impl Signer { warn!("{self}: Failed to mark block as locally rejected: {e:?}"); } }; - self.impl_send_block_response(Some(&block_info.block), rejection); + self.impl_send_block_response(&block_info.block, rejection); self.signer_db .insert_block(&block_info) @@ -1437,10 +1529,9 @@ impl Signer { return; }; + let stacker_address = StacksAddress::p2pkh(self.mainnet, &public_key); // authenticate the signature -- it must be signed by one of the stacking set let is_valid_sig = self.signer_addresses.iter().any(|addr| { - let stacker_address = StacksAddress::p2pkh(self.mainnet, &public_key); - // it only matters that the address hash bytes match stacker_address.bytes() == addr.bytes() }); @@ -1454,7 +1545,10 @@ impl Signer { self.signer_db .add_block_signature(block_hash, signature) .unwrap_or_else(|_| panic!("{self}: Failed to save block signature")); - + // If this isn't our own signature, try treating it as a pre-commit in case the caller is running an outdated version + if stacker_address != self.stacks_address { + self.handle_block_pre_commit(stacks_client, &stacker_address, block_hash); + } // do we have enough signatures to broadcast? // i.e. is the threshold reached? let signatures = self diff --git a/stacks-signer/src/v0/tests.rs b/stacks-signer/src/v0/tests.rs index 9580468c9a..169f42b528 100644 --- a/stacks-signer/src/v0/tests.rs +++ b/stacks-signer/src/v0/tests.rs @@ -41,6 +41,10 @@ pub static TEST_REJECT_ALL_BLOCK_PROPOSAL: LazyLock>> = LazyLock::new(TestFlag::default); +/// A global variable that can be used to skip signature broadcast if the signer's public key is in the provided list +pub static TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST: LazyLock>> = + LazyLock::new(TestFlag::default); + /// A global variable that can be used to pause broadcasting the block to the network pub static TEST_PAUSE_BLOCK_BROADCAST: LazyLock> = LazyLock::new(TestFlag::default); @@ -77,6 +81,25 @@ impl Signer { false } + /// Skip the block broadcast if the TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST flag is set for the signer + pub fn test_skip_signature_broadcast(&self, block_response: &BlockResponse) -> bool { + if block_response.as_block_accepted().is_none() { + return false; + } + let hash = block_response.get_signer_signature_hash(); + let public_keys = TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.get(); + if public_keys.contains( + &stacks_common::types::chainstate::StacksPublicKey::from_private(&self.private_key), + ) { + warn!( + "{self}: Skipping signature broadcast due to testing directive"; + "signer_signature_hash" => %hash + ); + return true; + } + false + } + /// Reject block proposals if the TEST_REJECT_ALL_BLOCK_PROPOSAL flag is set for the signer's public key pub fn test_reject_block_proposal( &mut self, diff --git a/testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs b/testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs index b0a317c9e1..b2120b88e9 100644 --- a/testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs +++ b/testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs @@ -448,6 +448,9 @@ impl StackerDBListener { SignerMessageV0::StateMachineUpdate(_) => { debug!("Received state machine update message. Ignoring."); } + SignerMessageV0::BlockPreCommit(_) => { + debug!("Received block pre commit message. Ignorning."); + } }; } } diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 2a5fd3bfd5..970966628e 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -84,7 +84,8 @@ use stacks_signer::v0::signer_state::SUPPORTED_SIGNER_PROTOCOL_VERSION; use stacks_signer::v0::tests::{ TEST_IGNORE_ALL_BLOCK_PROPOSALS, TEST_PAUSE_BLOCK_BROADCAST, TEST_PIN_SUPPORTED_SIGNER_PROTOCOL_VERSION, TEST_REJECT_ALL_BLOCK_PROPOSAL, - TEST_SKIP_BLOCK_BROADCAST, TEST_SKIP_SIGNER_CLEANUP, TEST_STALL_BLOCK_VALIDATION_SUBMISSION, + TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST, TEST_SKIP_BLOCK_BROADCAST, TEST_SKIP_SIGNER_CLEANUP, + TEST_STALL_BLOCK_VALIDATION_SUBMISSION, }; use stacks_signer::v0::SpawnedSigner; use tracing_subscriber::prelude::*; @@ -1389,6 +1390,37 @@ pub fn wait_for_block_acceptance_from_signers( Ok(result) } +/// Waits for all of the provided signers to send a pre-commit for a block +/// with the provided signer signature hash +pub fn wait_for_block_pre_commits_from_signers( + timeout_secs: u64, + signer_signature_hash: &Sha512Trunc256Sum, + expected_signers: &[StacksPublicKey], +) -> Result<(), String> { + wait_for(timeout_secs, || { + let chunks = test_observer::get_stackerdb_chunks() + .into_iter() + .flat_map(|chunk| chunk.modified_slots) + .filter_map(|chunk| { + let pk = chunk.recover_pk().expect("Failed to recover pk"); + if !expected_signers.contains(&pk) { + return None; + } + let message = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice()) + .expect("Failed to deserialize SignerMessage"); + + if let SignerMessage::BlockPreCommit(hash) = message { + if hash == *signer_signature_hash { + return Some(pk); + } + } + None + }) + .collect::>(); + Ok(chunks.len() == expected_signers.len()) + }) +} + /// Waits for all of the provided signers to send a rejection for a block /// with the provided signer signature hash pub fn wait_for_block_rejections_from_signers( @@ -6537,7 +6569,7 @@ fn reorg_locally_accepted_blocks_across_tenures_succeeds() { .cloned() .skip(num_signers * 7 / 10) .collect(); - TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(ignoring_signers.clone()); + TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.set(ignoring_signers.clone()); // Clear the stackerdb chunks test_observer::clear(); @@ -6557,12 +6589,12 @@ fn reorg_locally_accepted_blocks_across_tenures_succeeds() { wait_for_block_proposal(30, info_before.stacks_tip_height + 1, &miner_pk) .expect("Timed out waiting for block N+1 to be proposed"); // Make sure that the non ignoring signers do actually accept it though - wait_for_block_acceptance_from_signers( + wait_for_block_pre_commits_from_signers( 30, &block_n_1_proposal.header.signer_signature_hash(), &non_ignoring_signers, ) - .expect("Timed out waiting for block acceptances of N+1"); + .expect("Timed out waiting for block pre-commits of N+1"); let info_after = signer_test.get_peer_info(); assert_eq!(info_after, info_before); assert_ne!( @@ -6593,7 +6625,7 @@ fn reorg_locally_accepted_blocks_across_tenures_succeeds() { "------------------------- Mine Nakamoto Block N+1' in Tenure B -------------------------" ); let info_before = signer_test.get_peer_info(); - TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(Vec::new()); + TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.set(Vec::new()); let block_n_1_prime = wait_for_block_pushed_by_miner_key(30, info_before.stacks_tip_height + 1, &miner_pk) @@ -6707,7 +6739,7 @@ fn reorg_locally_accepted_blocks_across_tenures_fails() { .cloned() .skip(num_signers * 7 / 10) .collect(); - TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(ignoring_signers.clone()); + TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.set(ignoring_signers.clone()); // Clear the stackerdb chunks test_observer::clear(); @@ -9791,7 +9823,7 @@ fn injected_signatures_are_ignored_across_boundaries() { .collect(); assert_eq!(ignoring_signers.len(), 3); assert_eq!(non_ignoring_signers.len(), 2); - TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(ignoring_signers.clone()); + TEST_SIGNERS_SKIP_SIGNATURE_BROADCAST.set(ignoring_signers.clone()); let info_before = signer_test.get_peer_info(); // submit a tx so that the miner will ATTEMPT to mine a stacks block N @@ -12467,10 +12499,8 @@ fn mark_miner_as_invalid_if_reorg_is_rejected() { .signer_test .check_signer_states_reorg(&[], &all_signers); - info!("------------------------- Wait for 3 acceptances and 2 rejections -------------------------"); let signer_signature_hash = block_n_1_prime.header.signer_signature_hash(); - wait_for_block_acceptance_from_signers(30, &signer_signature_hash, &approving_signers) - .expect("Timed out waiting for block acceptance from approving signers"); + info!("------------------------- Wait for 3 acceptances and 2 rejections of {signer_signature_hash} -------------------------"); let rejections = wait_for_block_rejections_from_signers(30, &signer_signature_hash, &rejecting_signers) .expect("Timed out waiting for block rejection from rejecting signers"); @@ -12481,6 +12511,8 @@ fn mark_miner_as_invalid_if_reorg_is_rejected() { "Reject reason is not ReorgNotAllowed" ); } + wait_for_block_pre_commits_from_signers(30, &signer_signature_hash, &approving_signers) + .expect("Timed out waiting for block pre-commits from approving signers"); info!("------------------------- Miner 1 Proposes N+1' Again -------------------------"); test_observer::clear(); @@ -14600,3 +14632,74 @@ fn rollover_signer_protocol_version() { signer_test.shutdown(); } + +// Basic test to ensure that signers will not issue a signature over a block proposal unless +// a threshold number of signers have pre-committed to sign. +#[test] +#[ignore] +fn signers_do_not_commit_unless_threshold_precommitted() { + if env::var("BITCOIND_TEST") != Ok("1".into()) { + return; + } + + info!("------------------------- Test Setup -------------------------"); + let num_signers = 20; + + let mut signer_test: SignerTest = SignerTest::new(num_signers, vec![]); + let miner_sk = signer_test.running_nodes.conf.miner.mining_key.unwrap(); + let miner_pk = StacksPublicKey::from_private(&miner_sk); + let all_signers = signer_test.signer_test_pks(); + + signer_test.boot_to_epoch_3(); + + // Make sure that more than 30% of signers are set to ignore any incoming proposals so that consensus is not reached + // on pre-commit round. + let ignore_signers: Vec<_> = all_signers + .iter() + .cloned() + .take(all_signers.len() / 2) + .collect(); + let pre_commit_signers: Vec<_> = all_signers + .iter() + .cloned() + .skip(all_signers.len() / 2) + .collect(); + TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(ignore_signers); + test_observer::clear(); + let blocks_before = test_observer::get_mined_nakamoto_blocks().len(); + let height_before = signer_test.get_peer_info().stacks_tip_height; + next_block_and( + &mut signer_test.running_nodes.btc_regtest_controller, + 30, + || Ok(test_observer::get_mined_nakamoto_blocks().len() > blocks_before), + ) + .unwrap(); + + let proposal = wait_for_block_proposal(30, height_before + 1, &miner_pk) + .expect("Timed out waiting for block proposal"); + let hash = proposal.header.signer_signature_hash(); + wait_for_block_pre_commits_from_signers(30, &hash, &pre_commit_signers) + .expect("Timed out waiting for pre-commits"); + assert!( + wait_for(30, || { + for chunk in test_observer::get_stackerdb_chunks() + .into_iter() + .flat_map(|chunk| chunk.modified_slots) + { + let message = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice()) + .expect("Failed to deserialize SignerMessage"); + if let SignerMessage::BlockResponse(BlockResponse::Accepted(accepted)) = message { + if accepted.signer_signature_hash == hash { + return Ok(true); + } + } + } + Ok(false) + }) + .is_err(), + "Should not have found a single block accept for the block hash {hash}" + ); + + info!("------------------------- Shutdown -------------------------"); + signer_test.shutdown(); +}