Feat/signer two phase commit

CHANGELOG.md

@@ -69,6 +69,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

libsigner/src/v0/messages.rs

@@ -71,7 +71,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!(
@@ -114,7 +116,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)]
@@ -137,7 +141,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())
     }
 }
 
@@ -161,6 +165,7 @@ impl From<&SignerMessage> for SignerMessageTypePrefix {
             SignerMessage::MockSignature(_) => SignerMessageTypePrefix::MockSignature,
             SignerMessage::MockBlock(_) => SignerMessageTypePrefix::MockBlock,
             SignerMessage::StateMachineUpdate(_) => SignerMessageTypePrefix::StateMachineUpdate,
+            SignerMessage::BlockPreCommit(_) => SignerMessageTypePrefix::BlockPreCommit,
         }
     }
 }
@@ -182,6 +187,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 {
@@ -197,6 +204,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),
         }
     }
 }
@@ -216,6 +224,9 @@ impl StacksMessageCodec for SignerMessage {
             SignerMessage::StateMachineUpdate(state_machine_update) => {
                 state_machine_update.consensus_serialize(fd)
             }
+            SignerMessage::BlockPreCommit(block_pre_commit) => {
+                block_pre_commit.consensus_serialize(fd)
+            }
         }?;
         Ok(())
     }
@@ -253,6 +264,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)
     }
@@ -2606,4 +2621,14 @@ mod test {
 
         assert_eq!(signer_message, signer_message_deserialized);
     }
+
+    #[test]
+    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::<SignerMessage, _>(&mut &serialized_pre_commit[..])
+                .expect("Failed to deserialize pre-commit");
+        assert_eq!(pre_commit, deserialized_pre_commit);
+    }
 }

stacks-node/src/nakamoto_node/stackerdb_listener.rs

@@ -500,6 +500,9 @@ impl StackerDBListener {
                     SignerMessageV0::StateMachineUpdate(update) => {
                         self.update_global_state_evaluator(&signer_pubkey, update);
                     }
+                    SignerMessageV0::BlockPreCommit(_) => {
+                        debug!("Received block pre commit message. Ignoring.");
+                    }
                 };
             }
         }

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

@@ -94,7 +94,8 @@ use stacks_signer::v0::signer_state::{
 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::*;
@@ -1491,6 +1492,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::<HashSet<_>>();
+        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(
@@ -9054,7 +9086,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();
 
@@ -9074,12 +9106,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!(
@@ -9121,9 +9153,7 @@ fn reorg_locally_accepted_blocks_across_tenures_succeeds() {
         info_before.stacks_tip_height + 1
     );
     let info_before = signer_test.get_peer_info();
-    test_observer::clear();
-    TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(Vec::new());
-    TEST_MINE_SKIP.set(false);
+    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)
@@ -9266,7 +9296,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();
 
@@ -12438,7 +12468,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
@@ -15227,10 +15257,8 @@ fn mark_miner_as_invalid_if_reorg_is_rejected_v1() {
         .signer_test
         .check_signer_states_reorg(&approving_signers, &rejecting_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");
@@ -15241,6 +15269,8 @@ fn mark_miner_as_invalid_if_reorg_is_rejected_v1() {
             "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();
@@ -17774,3 +17804,74 @@ fn bitcoin_reorg_extended_tenure() {
 
     miners.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<SpawnedSigner> = 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();
+}

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]
 

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 {

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() {}
 

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"