From 05285b2af9dedada1ac6c35106025f766509bc44 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 5 May 2025 17:48:00 -0700 Subject: [PATCH 01/12] WIP: 2-phase commit block signing Signed-off-by: Jacinta Ferrant --- CHANGELOG.md | 1 + libsigner/src/v0/messages.rs | 21 ++- stacks-signer/CHANGELOG.md | 4 + stacks-signer/src/monitoring/mod.rs | 8 + stacks-signer/src/monitoring/prometheus.rs | 5 + stacks-signer/src/signerdb.rs | 115 +++++++++++- stacks-signer/src/v0/signer.rs | 170 ++++++++++++++---- .../src/nakamoto_node/stackerdb_listener.rs | 3 + 8 files changed, 290 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 760519eb76..947c6d5afa 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 27029241ff..487b319349 100644 --- a/stacks-signer/src/monitoring/mod.rs +++ b/stacks-signer/src/monitoring/mod.rs @@ -71,6 +71,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(); @@ -173,6 +178,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 be9f89191e..1c8123f066 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 48b891f03f..1906bd2ceb 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, @@ -1412,6 +1444,39 @@ impl SignerDb { } Ok(result) } + + /// 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> @@ -2515,4 +2580,50 @@ pub mod tests { assert_eq!(updates.get(&address_2), None); assert_eq!(updates.get(&address_3), Some(&update_3)); } + + #[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(); + assert_eq!( + db.get_block_pre_committers(&block_sighash1).unwrap(), + vec![address2, address1] + ); + + db.add_block_pre_commit(&block_sighash2, &address3).unwrap(); + assert_eq!( + db.get_block_pre_committers(&block_sighash1).unwrap(), + vec![address2, address1] + ); + assert_eq!( + db.get_block_pre_committers(&block_sighash2).unwrap(), + vec![address3] + ); + } } diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index 58e720282e..0f763333b7 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, + ), _ => {} } } @@ -716,6 +721,30 @@ impl Signer { self.impl_send_block_response(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, @@ -746,6 +775,92 @@ 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 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 committed to block {block_hash}, but we do not view the block as valid. Doing nothing." + ); + 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:?}",); + } else { + block_info.signed_self.get_or_insert(get_epoch_time_secs()); + } + } + + 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.send_block_response(block_response); + } + /// Handle block proposal messages submitted to signers stackerdb fn handle_block_proposal( &mut self, @@ -997,12 +1112,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 @@ -1016,11 +1131,11 @@ 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 Some(block_response) = @@ -1036,15 +1151,8 @@ impl Signer { 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) @@ -1055,19 +1163,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 @@ -1080,16 +1188,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( @@ -1110,7 +1218,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.send_block_response(response); } /// Handle the block validate response returned from our prior calls to submit a block for validation @@ -1121,15 +1230,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 @@ -1137,11 +1246,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 { - self.impl_send_block_response(response); - }; - // Check if there is a pending block validation that we need to submit to the node self.check_pending_block_validations(stacks_client); } @@ -1420,10 +1524,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() }); @@ -1437,7 +1540,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/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."); + } }; } } From 0c6a83ee8639ce1158697737606fba9d188ba487 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 6 May 2025 16:39:24 -0700 Subject: [PATCH 02/12] Use impl_send_block_response where appropro Signed-off-by: Jacinta Ferrant --- stacks-signer/src/v0/signer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index c8fe7577a5..936fa26a8c 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -1217,7 +1217,7 @@ impl Signer { .unwrap_or_else(|e| self.handle_insert_block_error(e)); self.handle_block_rejection(&block_rejection, sortition_state); let response = BlockResponse::Rejected(block_rejection); - self.send_block_response(&block_info.block, response); + 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 From 0aacdaacb957ef90530ce296b319c78eaebb719a Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 7 May 2025 07:32:13 -0700 Subject: [PATCH 03/12] Do not continually sign the same block Signed-off-by: Jacinta Ferrant --- stacks-signer/src/v0/signer.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index 936fa26a8c..e640e6fb10 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -839,12 +839,14 @@ impl Signer { ); return; } + if block_info.signed_self.is_some() { + // 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:?}",); - } else { - block_info.signed_self.get_or_insert(get_epoch_time_secs()); } } From 90ff69ee797f46b1977bfd698547b29798f352c0 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 7 May 2025 11:58:01 -0700 Subject: [PATCH 04/12] Cleanup tests Signed-off-by: Jacinta Ferrant --- stacks-signer/src/v0/signer.rs | 11 +++-- stacks-signer/src/v0/tests.rs | 23 ++++++++++ testnet/stacks-node/src/tests/signer/v0.rs | 50 ++++++++++++++++++---- 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index e640e6fb10..41ec6eda48 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -671,6 +671,10 @@ 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: &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:?}"; ); @@ -826,7 +830,7 @@ impl Signer { if min_weight > commit_weight { debug!( - "{self}: Not enough committed to block {block_hash} (have {commit_weight}, need at least {min_weight}/{total_weight})" + "{self}: Not enough pre-committed to block {block_hash} (have {commit_weight}, need at least {min_weight}/{total_weight})" ); return; } @@ -835,11 +839,12 @@ impl Signer { 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 committed to block {block_hash}, but we do not view the block as valid. Doing nothing." + "{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; } @@ -1130,7 +1135,7 @@ 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..."); + debug!("{self}: Received block validate response for a block we have not seen before. Ignoring..."); return; }; if block_info.is_locally_finalized() { 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/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 2a5fd3bfd5..bac8a0c76e 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(); @@ -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(); From 802d051907377287b37e2bfd725b8055b6aa2c0a Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 7 May 2025 13:21:43 -0700 Subject: [PATCH 05/12] Fix injected_signatures_are_ignored_across_boundaries Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/tests/signer/v0.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index bac8a0c76e..3f0141b9a9 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -9823,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 From 6ac758f3e69b7c702095ff618b776c0551280c35 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 7 May 2025 13:47:02 -0700 Subject: [PATCH 06/12] Add a pre commit specific test Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/tests/signer/v0.rs | 71 ++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 3f0141b9a9..970966628e 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -14632,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(); +} From 653c86454b93586a5dba433a99262b64715fcf3d Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 7 May 2025 14:46:19 -0700 Subject: [PATCH 07/12] Fix db test to be order resistent Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signerdb.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index cdf6f36455..b408be9826 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -2715,19 +2715,18 @@ pub mod tests { ); db.add_block_pre_commit(&block_sighash1, &address2).unwrap(); - assert_eq!( - db.get_block_pre_committers(&block_sighash1).unwrap(), - vec![address2, address1] - ); + 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(); - assert_eq!( - db.get_block_pre_committers(&block_sighash1).unwrap(), - vec![address2, address1] - ); - assert_eq!( - db.get_block_pre_committers(&block_sighash2).unwrap(), - vec![address3] - ); + 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)); } } From 3ae5af949c0e0b8baf715f256fce644fd99dd1b1 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 30 Jun 2025 12:21:36 -0700 Subject: [PATCH 08/12] Add signature across BlockPreCommit instead of using stackerdb slot to get signer address Signed-off-by: Jacinta Ferrant --- libsigner/src/v0/messages.rs | 112 +++++++++++++++++- stacks-signer/src/v0/signer.rs | 36 ++++-- .../src/nakamoto_node/stackerdb_listener.rs | 2 +- testnet/stacks-node/src/tests/signer/v0.rs | 4 +- 4 files changed, 137 insertions(+), 17 deletions(-) diff --git a/libsigner/src/v0/messages.rs b/libsigner/src/v0/messages.rs index 2e6924a955..55e086dfda 100644 --- a/libsigner/src/v0/messages.rs +++ b/libsigner/src/v0/messages.rs @@ -206,7 +206,7 @@ pub enum SignerMessage { /// A state machine update StateMachineUpdate(StateMachineUpdate), /// The pre commit message from signers for other signers to observe - BlockPreCommit(Sha512Trunc256Sum), + BlockPreCommit(BlockPreCommit), } impl SignerMessage { @@ -242,8 +242,8 @@ 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) + SignerMessage::BlockPreCommit(block_pre_commit) => { + block_pre_commit.consensus_serialize(fd) } }?; Ok(()) @@ -807,6 +807,93 @@ impl StacksMessageCodec for StateMachineUpdate { } } +/// A block pre commit message +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct BlockPreCommit { + /// The signer signature hash of the block being committed to + pub signer_signature_hash: Sha512Trunc256Sum, + /// The miner's signature across the BlockPreCommit message + pub signature: MessageSignature, +} + +impl BlockPreCommit { + /// create a new unsigned block pre commit message + pub fn new_unsigned(signer_signature_hash: Sha512Trunc256Sum) -> Self { + Self { + signer_signature_hash, + signature: MessageSignature::empty(), + } + } + + /// Create a new signed block pre commit message using the provided private key + pub fn new_signed( + signer_signature_hash: Sha512Trunc256Sum, + private_key: &StacksPrivateKey, + mainnet: bool, + ) -> Result { + let mut pre_commit = Self::new_unsigned(signer_signature_hash); + pre_commit.sign(private_key, mainnet)?; + Ok(pre_commit) + } + + /// Create a hash across the BlockPreCommit data. Note it cannot simply sign the signer_signature_hash directly + /// as this could be added prematurely to a NakamotoBlock + pub fn hash(&self, mainnet: bool) -> Sha256Sum { + let chain_id = if mainnet { + CHAIN_ID_MAINNET + } else { + CHAIN_ID_TESTNET + }; + let domain_tuple = make_structured_data_domain("block-pre-commit", "1.0.0", chain_id); + let data = Value::buff_from(self.signer_signature_hash.as_bytes().into()).unwrap(); + structured_data_message_hash(data, domain_tuple) + } + + /// Sign the BlockPreCommit and set the internal signature field + pub fn sign(&mut self, private_key: &StacksPrivateKey, mainnet: bool) -> Result<(), String> { + let signature_hash = self.hash(mainnet); + self.signature = private_key.sign(signature_hash.as_bytes())?; + Ok(()) + } + + /// Verify the block pre commit against the provided public key + pub fn verify(&self, public_key: &StacksPublicKey, mainnet: bool) -> Result { + if self.signature == MessageSignature::empty() { + return Ok(false); + } + let signature_hash = self.hash(mainnet); + public_key + .verify(&signature_hash.0, &self.signature) + .map_err(|e| e.to_string()) + } + + /// Recover the public key from the BlockPreCommit + pub fn recover_public_key(&self, mainnet: bool) -> Result { + if self.signature == MessageSignature::empty() { + return Err("No signature to recover public key from"); + } + let signature_hash = self.hash(mainnet); + StacksPublicKey::recover_to_pubkey(signature_hash.as_bytes(), &self.signature) + } +} + +impl StacksMessageCodec for BlockPreCommit { + fn consensus_serialize(&self, fd: &mut W) -> Result<(), CodecError> { + self.signer_signature_hash.consensus_serialize(fd)?; + self.signature.consensus_serialize(fd)?; + Ok(()) + } + + fn consensus_deserialize(fd: &mut R) -> Result { + let signer_signature_hash = Sha512Trunc256Sum::consensus_deserialize(fd)?; + let signature = MessageSignature::consensus_deserialize(fd)?; + Ok(Self { + signer_signature_hash, + signature, + }) + } +} + define_u8_enum!( /// Enum representing the reject code type prefix RejectCodeTypePrefix { @@ -1762,6 +1849,12 @@ impl From for SignerMessage { } } +impl From for SignerMessage { + fn from(block_pre_commit: BlockPreCommit) -> Self { + Self::BlockPreCommit(block_pre_commit) + } +} + #[cfg(test)] mod test { use blockstack_lib::chainstate::nakamoto::NakamotoBlockHeader; @@ -2448,4 +2541,17 @@ mod test { assert_eq!(signer_message, signer_message_deserialized); } + + #[test] + fn serde_block_pre_commit() { + let mut pre_commit = BlockPreCommit::new_unsigned(Sha512Trunc256Sum([0u8; 32])); + pre_commit + .sign(&StacksPrivateKey::random(), false) + .expect("Failed to sign pre-commit"); + let serialized_pre_commit = pre_commit.serialize_to_vec(); + let deserialized_pre_commit = + read_next::(&mut &serialized_pre_commit[..]) + .expect("Failed to deserialize pre-commit"); + assert_eq!(pre_commit, deserialized_pre_commit); + } } diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index 67bed84da2..74f8fa4eda 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -34,8 +34,8 @@ use clarity::util::sleep_ms; #[cfg(any(test, feature = "testing"))] use clarity::util::tests::TestFlag; use libsigner::v0::messages::{ - BlockAccepted, BlockRejection, BlockResponse, MessageSlotID, MockProposal, MockSignature, - RejectReason, RejectReasonPrefix, SignerMessage, StateMachineUpdate, + BlockAccepted, BlockPreCommit, BlockRejection, BlockResponse, MessageSlotID, MockProposal, + MockSignature, RejectReason, RejectReasonPrefix, SignerMessage, StateMachineUpdate, }; use libsigner::v0::signer_state::GlobalStateEvaluator; use libsigner::{BlockProposal, SignerEvent}; @@ -441,11 +441,16 @@ impl Signer { received_time, sortition_state, ), - SignerMessage::BlockPreCommit(hash) => self.handle_block_pre_commit( - stacks_client, - &StacksAddress::p2pkh(self.mainnet, signer_public_key), - hash, - ), + SignerMessage::BlockPreCommit(block_pre_commit) => { + if let Ok(pubkey) = block_pre_commit.recover_public_key(self.mainnet).inspect_err(|e| warn!("Unable to recover public key from block pre commit {block_pre_commit:?}: {e}")) { + let stacker_address = StacksAddress::p2pkh(self.mainnet, &pubkey); + self.handle_block_pre_commit( + stacks_client, + &stacker_address, + &block_pre_commit.signer_signature_hash + ) + } + }, _ => {} } } @@ -764,13 +769,13 @@ impl Signer { } /// 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) { + fn send_block_pre_commit(&mut self, block_pre_commit: BlockPreCommit) { info!( - "{self}: Broadcasting a block pre-commit to stacks node: {block_hash:?}"; + "{self}: Broadcasting a block pre-commit to stacks node: {block_pre_commit:?}"; ); match self .stackerdb - .send_message_with_retry(SignerMessage::BlockPreCommit(block_hash)) + .send_message_with_retry::(block_pre_commit.into()) { Ok(ack) => { if !ack.accepted { @@ -1228,7 +1233,16 @@ impl Signer { self.signer_db .insert_block(&block_info) .unwrap_or_else(|e| self.handle_insert_block_error(e)); - self.send_block_pre_commit(signer_signature_hash); + if let Ok(block_pre_commit) = BlockPreCommit::new_signed( + signer_signature_hash, + &self.private_key, + self.mainnet, + ) + .inspect_err(|e| { + warn!("Unable to create signed block pre-commit for {signer_signature_hash}: {e}") + }) { + self.send_block_pre_commit(block_pre_commit); + }; // have to save the signature _after_ the block info let address = self.stacks_address; self.handle_block_pre_commit(stacks_client, &address, &signer_signature_hash); diff --git a/testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs b/testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs index 517fb5750d..4f06b27796 100644 --- a/testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs +++ b/testnet/stacks-node/src/nakamoto_node/stackerdb_listener.rs @@ -501,7 +501,7 @@ impl StackerDBListener { self.update_global_state_evaluator(&signer_pubkey, update); } SignerMessageV0::BlockPreCommit(_) => { - debug!("Received block pre commit message. Ignorning."); + debug!("Received block pre commit message. Ignoring."); } }; } diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 2af80cff45..35fec0f525 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -1457,8 +1457,8 @@ pub fn wait_for_block_pre_commits_from_signers( 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 { + if let SignerMessage::BlockPreCommit(block_pre_commit) = message { + if block_pre_commit.signer_signature_hash == *signer_signature_hash { return Some(pk); } } From 77a0ec43d1d9f0686e9f6d7aa2435e4804048b56 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 10 Jul 2025 10:42:55 -0700 Subject: [PATCH 09/12] Remove unnecessary signature across BlockCommit Signed-off-by: Jacinta Ferrant --- libsigner/src/v0/messages.rs | 104 +-------------------- stacks-signer/src/v0/signer.rs | 40 +++----- testnet/stacks-node/src/tests/signer/v0.rs | 4 +- 3 files changed, 21 insertions(+), 127 deletions(-) diff --git a/libsigner/src/v0/messages.rs b/libsigner/src/v0/messages.rs index 9c0791d7e4..77cc98239c 100644 --- a/libsigner/src/v0/messages.rs +++ b/libsigner/src/v0/messages.rs @@ -188,7 +188,7 @@ pub enum SignerMessage { /// A state machine update StateMachineUpdate(StateMachineUpdate), /// The pre commit message from signers for other signers to observe - BlockPreCommit(BlockPreCommit), + BlockPreCommit(Sha512Trunc256Sum), } impl SignerMessage { @@ -857,93 +857,6 @@ impl StacksMessageCodec for StateMachineUpdate { } } -/// A block pre commit message -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct BlockPreCommit { - /// The signer signature hash of the block being committed to - pub signer_signature_hash: Sha512Trunc256Sum, - /// The miner's signature across the BlockPreCommit message - pub signature: MessageSignature, -} - -impl BlockPreCommit { - /// create a new unsigned block pre commit message - pub fn new_unsigned(signer_signature_hash: Sha512Trunc256Sum) -> Self { - Self { - signer_signature_hash, - signature: MessageSignature::empty(), - } - } - - /// Create a new signed block pre commit message using the provided private key - pub fn new_signed( - signer_signature_hash: Sha512Trunc256Sum, - private_key: &StacksPrivateKey, - mainnet: bool, - ) -> Result { - let mut pre_commit = Self::new_unsigned(signer_signature_hash); - pre_commit.sign(private_key, mainnet)?; - Ok(pre_commit) - } - - /// Create a hash across the BlockPreCommit data. Note it cannot simply sign the signer_signature_hash directly - /// as this could be added prematurely to a NakamotoBlock - pub fn hash(&self, mainnet: bool) -> Sha256Sum { - let chain_id = if mainnet { - CHAIN_ID_MAINNET - } else { - CHAIN_ID_TESTNET - }; - let domain_tuple = make_structured_data_domain("block-pre-commit", "1.0.0", chain_id); - let data = Value::buff_from(self.signer_signature_hash.as_bytes().into()).unwrap(); - structured_data_message_hash(data, domain_tuple) - } - - /// Sign the BlockPreCommit and set the internal signature field - pub fn sign(&mut self, private_key: &StacksPrivateKey, mainnet: bool) -> Result<(), String> { - let signature_hash = self.hash(mainnet); - self.signature = private_key.sign(signature_hash.as_bytes())?; - Ok(()) - } - - /// Verify the block pre commit against the provided public key - pub fn verify(&self, public_key: &StacksPublicKey, mainnet: bool) -> Result { - if self.signature == MessageSignature::empty() { - return Ok(false); - } - let signature_hash = self.hash(mainnet); - public_key - .verify(&signature_hash.0, &self.signature) - .map_err(|e| e.to_string()) - } - - /// Recover the public key from the BlockPreCommit - pub fn recover_public_key(&self, mainnet: bool) -> Result { - if self.signature == MessageSignature::empty() { - return Err("No signature to recover public key from"); - } - let signature_hash = self.hash(mainnet); - StacksPublicKey::recover_to_pubkey(signature_hash.as_bytes(), &self.signature) - } -} - -impl StacksMessageCodec for BlockPreCommit { - fn consensus_serialize(&self, fd: &mut W) -> Result<(), CodecError> { - self.signer_signature_hash.consensus_serialize(fd)?; - self.signature.consensus_serialize(fd)?; - Ok(()) - } - - fn consensus_deserialize(fd: &mut R) -> Result { - let signer_signature_hash = Sha512Trunc256Sum::consensus_deserialize(fd)?; - let signature = MessageSignature::consensus_deserialize(fd)?; - Ok(Self { - signer_signature_hash, - signature, - }) - } -} - define_u8_enum!( /// Enum representing the reject code type prefix RejectCodeTypePrefix { @@ -1953,12 +1866,6 @@ impl From for SignerMessage { } } -impl From for SignerMessage { - fn from(block_pre_commit: BlockPreCommit) -> Self { - Self::BlockPreCommit(block_pre_commit) - } -} - #[cfg(test)] mod test { use blockstack_lib::chainstate::nakamoto::NakamotoBlockHeader; @@ -2716,14 +2623,11 @@ mod test { } #[test] - fn serde_block_pre_commit() { - let mut pre_commit = BlockPreCommit::new_unsigned(Sha512Trunc256Sum([0u8; 32])); - pre_commit - .sign(&StacksPrivateKey::random(), false) - .expect("Failed to sign pre-commit"); + fn serde_block_signer_message_pre_commit() { + let pre_commit = SignerMessage::BlockPreCommit(Sha512Trunc256Sum([0u8; 32])); let serialized_pre_commit = pre_commit.serialize_to_vec(); let deserialized_pre_commit = - read_next::(&mut &serialized_pre_commit[..]) + read_next::(&mut &serialized_pre_commit[..]) .expect("Failed to deserialize pre-commit"); assert_eq!(pre_commit, deserialized_pre_commit); } diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index ef12b5d7ed..3a45136a97 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -34,8 +34,8 @@ use clarity::util::sleep_ms; #[cfg(any(test, feature = "testing"))] use clarity::util::tests::TestFlag; use libsigner::v0::messages::{ - BlockAccepted, BlockPreCommit, BlockRejection, BlockResponse, MessageSlotID, MockProposal, - MockSignature, RejectReason, RejectReasonPrefix, SignerMessage, StateMachineUpdate, + BlockAccepted, BlockRejection, BlockResponse, MessageSlotID, MockProposal, MockSignature, + RejectReason, RejectReasonPrefix, SignerMessage, StateMachineUpdate, }; use libsigner::v0::signer_state::GlobalStateEvaluator; use libsigner::{BlockProposal, SignerEvent}; @@ -468,16 +468,15 @@ impl Signer { ), SignerMessage::StateMachineUpdate(update) => self .handle_state_machine_update(signer_public_key, update, received_time), - SignerMessage::BlockPreCommit(block_pre_commit) => { - if let Ok(pubkey) = block_pre_commit.recover_public_key(self.mainnet).inspect_err(|e| warn!("Unable to recover public key from block pre commit {block_pre_commit:?}: {e}")) { - let stacker_address = StacksAddress::p2pkh(self.mainnet, &pubkey); - self.handle_block_pre_commit( - stacks_client, - &stacker_address, - &block_pre_commit.signer_signature_hash - ) - } - }, + SignerMessage::BlockPreCommit(signer_signature_hash) => { + let stacker_address = + StacksAddress::p2pkh(self.mainnet, signer_public_key); + self.handle_block_pre_commit( + stacks_client, + &stacker_address, + signer_signature_hash, + ) + } _ => {} } } @@ -879,13 +878,13 @@ impl Signer { } /// 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_pre_commit: BlockPreCommit) { + fn send_block_pre_commit(&mut self, signer_signature_hash: Sha512Trunc256Sum) { info!( - "{self}: Broadcasting a block pre-commit to stacks node: {block_pre_commit:?}"; + "{self}: Broadcasting a block pre-commit to stacks node for {signer_signature_hash}"; ); match self .stackerdb - .send_message_with_retry::(block_pre_commit.into()) + .send_message_with_retry(SignerMessage::BlockPreCommit(signer_signature_hash)) { Ok(ack) => { if !ack.accepted { @@ -1327,16 +1326,7 @@ impl Signer { self.signer_db .insert_block(&block_info) .unwrap_or_else(|e| self.handle_insert_block_error(e)); - if let Ok(block_pre_commit) = BlockPreCommit::new_signed( - signer_signature_hash, - &self.private_key, - self.mainnet, - ) - .inspect_err(|e| { - warn!("Unable to create signed block pre-commit for {signer_signature_hash}: {e}") - }) { - self.send_block_pre_commit(block_pre_commit); - }; + self.send_block_pre_commit(signer_signature_hash); // have to save the signature _after_ the block info let address = self.stacks_address; self.handle_block_pre_commit(stacks_client, &address, &signer_signature_hash); diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index d88e8d9e20..7e91028b38 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -1511,8 +1511,8 @@ pub fn wait_for_block_pre_commits_from_signers( let message = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice()) .expect("Failed to deserialize SignerMessage"); - if let SignerMessage::BlockPreCommit(block_pre_commit) = message { - if block_pre_commit.signer_signature_hash == *signer_signature_hash { + if let SignerMessage::BlockPreCommit(hash) = message { + if hash == *signer_signature_hash { return Some(pk); } } From 11e1da1c3767e714a6acb168f21d044a656b9bc9 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 10 Jul 2025 12:25:24 -0700 Subject: [PATCH 10/12] Fix changelog Signed-off-by: Jacinta Ferrant --- stacks-signer/CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/stacks-signer/CHANGELOG.md b/stacks-signer/CHANGELOG.md index 5d0302cbb3..c2504a3e12 100644 --- a/stacks-signer/CHANGELOG.md +++ b/stacks-signer/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE - Added `info` logs to the signer to provide more visibility into the block approval/rejection status - Introduced `capitulate_miner_view_timeout_secs`: the duration (in seconds) for the signer to wait between updating the local state machine viewpoint and capitulating to other signers' miner views. - Added codepath to enable signers to evaluate block proposals and miner activity against global signer state for improved consistency and correctness. Currently feature gated behind the `SUPPORTED_SIGNER_PROTOCOL_VERSION` +- Added `SignerMessage::BlockPreCommit` message handling; signers now collect until a threshold is reached before issuing a block signature, implementing a proper 2-phase commit. ## [3.1.0.0.13.0] @@ -41,10 +42,6 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE ## [3.1.0.0.9.0] -### 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 From 119e37c5118b58c8d183a8ffe2e752ccfe2f90b9 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 10 Jul 2025 14:43:41 -0700 Subject: [PATCH 11/12] Mark a block as local accepted if pre committed to Signed-off-by: Jacinta Ferrant --- stacks-signer/src/chainstate/mod.rs | 2 +- stacks-signer/src/v0/signer.rs | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/stacks-signer/src/chainstate/mod.rs b/stacks-signer/src/chainstate/mod.rs index 9db0b511ef..1edbf3293c 100644 --- a/stacks-signer/src/chainstate/mod.rs +++ b/stacks-signer/src/chainstate/mod.rs @@ -317,7 +317,7 @@ impl SortitionData { if let Some(info) = last_block_info { // N.B. this block might not be the last globally accepted block across the network; - // it's just the highest one in this tenure that we knnge: &TenureChangePow about. If this given block is + // it's just the highest one in this tenure that we know about. If this given block is // no higher than it, then it's definitely no higher than the last globally accepted // block across the network, so we can do an early rejection here. if block.header.chain_length <= info.block.header.chain_length { diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index 3a45136a97..9cf522bcf6 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -989,16 +989,12 @@ impl Signer { ); 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:?}",); } + block_info.signed_self.get_or_insert(get_epoch_time_secs()); } self.signer_db @@ -1315,7 +1311,13 @@ impl Signer { .insert_block(&block_info) .unwrap_or_else(|e| self.handle_insert_block_error(e)); } else { - block_info.valid = Some(true); + 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; + } + block_info.signed_self.get_or_insert(get_epoch_time_secs()); + } // 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) From cf079275de8944224e657012fa85bfc68228083e Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 10 Jul 2025 18:24:57 -0700 Subject: [PATCH 12/12] Do not needlessly reconsider pre-commits Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signerdb.rs | 30 ++++++++++++++++++++++++++++++ stacks-signer/src/v0/signer.rs | 4 ++++ 2 files changed, 34 insertions(+) diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index 469812b3f1..d5b25a1995 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -1767,6 +1767,29 @@ impl SignerDb { Ok(()) } + /// Check if the given address has already committed to sign the block identified by block_sighash + pub fn has_committed( + &self, + block_sighash: &Sha512Trunc256Sum, + address: &StacksAddress, + ) -> Result { + let qry_check = " + SELECT 1 FROM block_pre_commits + WHERE signer_signature_hash = ?1 AND signer_addr = ?2 + LIMIT 1;"; + + let exists: Option = self + .db + .query_row( + qry_check, + params![block_sighash, address.to_string()], + |row| row.get(0), + ) + .optional()?; + + Ok(exists.is_some()) + } + /// Get all pre committers for a block pub fn get_block_pre_committers( &self, @@ -3254,5 +3277,12 @@ pub mod tests { let commits = db.get_block_pre_committers(&block_sighash2).unwrap(); assert_eq!(commits.len(), 1); assert!(commits.contains(&address3)); + + assert!(db.has_committed(&block_sighash1, &address1).unwrap()); + assert!(db.has_committed(&block_sighash1, &address2).unwrap()); + assert!(!db.has_committed(&block_sighash1, &address3).unwrap()); + assert!(!db.has_committed(&block_sighash2, &address1).unwrap()); + assert!(!db.has_committed(&block_sighash2, &address2).unwrap()); + assert!(db.has_committed(&block_sighash2, &address3).unwrap()); } } diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index 9cf522bcf6..d2dca9f46d 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -954,6 +954,10 @@ impl Signer { 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; + } // commit message is from a valid sender! store it self.signer_db .add_block_pre_commit(block_hash, stacker_address)