Merge pull request #6147 from jferrant/bug/capitulate-viewpoint-equality-bugs

jferrant · web-flow · commit c09f17eb56ce · 2025-05-27T19:22:09.000Z
Fix capitulate_miner_view so don't flip flop between multiple miners
diff --git a/stacks-signer/src/tests/signer_state.rs b/stacks-signer/src/tests/signer_state.rs
@@ -13,9 +13,11 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 use std::collections::HashMap;
+use std::time::SystemTime;
 
 use clarity::types::chainstate::{
-    ConsensusHash, StacksAddress, StacksBlockId, StacksPrivateKey, StacksPublicKey,
+    BurnchainHeaderHash, ConsensusHash, StacksAddress, StacksBlockId, StacksPrivateKey,
+    StacksPublicKey,
 };
 use clarity::util::hash::Hash160;
 use clarity::util::secp256k1::MessageSignature;
@@ -32,7 +34,7 @@ use crate::v0::signer_state::LocalStateMachine;
 #[test]
 fn check_capitulate_miner_view() {
     let mut address_weights = HashMap::new();
-    for _ in 0..5 {
+    for _ in 0..10 {
         let stacks_address = StacksAddress::p2pkh(
             false,
             &StacksPublicKey::from_private(&StacksPrivateKey::random()),
@@ -43,17 +45,23 @@ fn check_capitulate_miner_view() {
     let active_signer_protocol_version = 0;
     let local_supported_signer_protocol_version = 0;
     let burn_block = ConsensusHash([0x55; 20]);
-    let burn_block_height = 100;
     let parent_tenure_id = ConsensusHash([0x22; 20]);
     let parent_tenure_last_block = StacksBlockId([0x33; 32]);
     let parent_tenure_last_block_height = 1;
+
+    let old_miner_tenure_id = ConsensusHash([0x01; 20]);
+    let new_miner_tenure_id = ConsensusHash([0x00; 20]);
+
+    let burn_block_height = 100;
+
     let old_miner = StateMachineUpdateMinerState::ActiveMiner {
         current_miner_pkh: Hash160([0xab; 20]),
-        tenure_id: ConsensusHash([0x44; 20]),
+        tenure_id: old_miner_tenure_id,
         parent_tenure_id,
         parent_tenure_last_block,
         parent_tenure_last_block_height,
     };
+    // Make sure the old update still has the newer burn block height
     let old_update = StateMachineUpdateMessage::new(
         active_signer_protocol_version,
         local_supported_signer_protocol_version,
@@ -86,35 +94,21 @@ fn check_capitulate_miner_view() {
             StateMachineUpdateContent::V0 {
                 burn_block,
                 burn_block_height,
-                current_miner,
+                ..
             },
         ..
     } = local_update.clone()
     else {
         panic!("Unexpected state machine update message version");
     };
     // Let's create a new miner view
-    let new_tenure_id = ConsensusHash([0x00; 20]);
-
-    let db_path = tmp_db_path();
-    let mut db = SignerDb::new(db_path).expect("Failed to create signer db");
-    let (mut block_info_1, _block_proposal) = create_block_override(|b| {
-        b.block.header.consensus_hash = new_tenure_id;
-        b.block.header.miner_signature = MessageSignature([0x01; 65]);
-        b.block.header.chain_length = 1;
-        b.burn_height = burn_block_height;
-    });
-
-    db.insert_block(&block_info_1).unwrap();
     let new_miner = StateMachineUpdateMinerState::ActiveMiner {
         current_miner_pkh: Hash160([0x00; 20]),
-        tenure_id: new_tenure_id,
+        tenure_id: new_miner_tenure_id,
         parent_tenure_id,
         parent_tenure_last_block,
         parent_tenure_last_block_height,
     };
-
-    // Let's update only our own view: the evaluator will tell me to revert my viewpoint to the old miner
     let new_update = StateMachineUpdateMessage::new(
         active_signer_protocol_version,
         local_supported_signer_protocol_version,
@@ -126,6 +120,43 @@ fn check_capitulate_miner_view() {
     )
     .unwrap();
 
+    // Update the database to have both the burn blocks corresponding to the tenure ids
+    // and both tenures have a locally accepted block
+    let db_path = tmp_db_path();
+    let mut db = SignerDb::new(db_path).expect("Failed to create signer db");
+    // Make sure both burn block corresponding to the tenure id's exist in our DB.
+    db.insert_burn_block(
+        &BurnchainHeaderHash([0u8; 32]),
+        &old_miner_tenure_id,
+        burn_block_height.saturating_sub(1),
+        &SystemTime::now(),
+        &BurnchainHeaderHash([1u8; 32]),
+    )
+    .unwrap();
+    db.insert_burn_block(
+        &BurnchainHeaderHash([0u8; 32]),
+        &new_miner_tenure_id,
+        burn_block_height,
+        &SystemTime::now(),
+        &BurnchainHeaderHash([1u8; 32]),
+    )
+    .unwrap();
+    let (mut block_info_1, _block_proposal) = create_block_override(|b| {
+        b.block.header.consensus_hash = old_miner_tenure_id;
+        b.block.header.miner_signature = MessageSignature([0x02; 65]);
+        b.block.header.chain_length = 1;
+        b.burn_height = burn_block_height.saturating_sub(1);
+    });
+    db.insert_block(&block_info_1).unwrap();
+
+    let (mut block_info_2, _block_proposal) = create_block_override(|b| {
+        b.block.header.consensus_hash = new_miner_tenure_id;
+        b.block.header.miner_signature = MessageSignature([0x01; 65]);
+        b.block.header.chain_length = 1;
+        b.burn_height = burn_block_height;
+    });
+    db.insert_block(&block_info_2).unwrap();
+
     let signer_state_machine = SignerStateMachine {
         burn_block,
         burn_block_height,
@@ -135,35 +166,43 @@ fn check_capitulate_miner_view() {
     };
 
     let mut local_state_machine = LocalStateMachine::Initialized(signer_state_machine.clone());
-    assert_eq!(
-        local_state_machine
-            .capitulate_miner_view(&mut global_eval, &mut db, local_address, &new_update)
-            .unwrap(),
-        current_miner
-    );
 
-    // Let's set a blocking minority to this different view: evaluator should see no global blocks for the blocking majority and return none
-    // I.e. only if the blocking minority is attempting to reject an reorg should it take priority over the rest.
-    // Let's update 1 other signer to some new miner key (60 percent)
-    for address in addresses.into_iter().skip(1).take(1) {
+    // Let's update 40 percent of other signers to some new miner key
+    for address in addresses.into_iter().take(4) {
         global_eval.insert_update(address, new_update.clone());
     }
+    // Miner view should be None as we can't find consensus on a single miner
     assert!(
         local_state_machine
             .capitulate_miner_view(&mut global_eval, &mut db, local_address, &new_update)
             .is_none(),
-        "Evaluator should have been unable to determine a majority view and return none"
+        "Evaluator should have told me to capitulate to the old miner"
     );
 
+    // Mark the old miner's block as globally accepted
     db.mark_block_globally_accepted(&mut block_info_1).unwrap();
-
     db.insert_block(&block_info_1).unwrap();
 
-    // Now that the blocking minority references a tenure which would actually get reorged, lets capitulate to their view
+    // Miner view should stay as the old miner as it has a globally accepted block and 60% consider it valid.
+    assert_eq!(
+        local_state_machine
+            .capitulate_miner_view(&mut global_eval, &mut db, local_address, &new_update)
+            .unwrap(),
+        old_miner,
+        "Evaluator should have told me to capitulate to the old miner"
+    );
+
+    // Now that we have a globally approved block for the new miner
+    db.mark_block_globally_accepted(&mut block_info_2).unwrap();
+    db.insert_block(&block_info_2).unwrap();
+
+    // Now that the blocking minority references a tenure which would actually get reorged, lets capitulate to the NEW view
+    // even though both the old and new signer have > 30% approval (it has a higher burn block).
     assert_eq!(
         local_state_machine
             .capitulate_miner_view(&mut global_eval, &mut db, local_address, &new_update)
             .unwrap(),
-        new_miner
+        new_miner,
+        "Evaluator should have told me to capitulate to the new miner"
     );
 }
diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs
@@ -255,6 +255,22 @@ impl SignerTrait<SignerMessage> for Signer {
         _res: &Sender<SignerResult>,
         current_reward_cycle: u64,
     ) {
+        self.check_submitted_block_proposal();
+        self.check_pending_block_validations(stacks_client);
+
+        let mut prior_state = self.local_state_machine.clone();
+        if self.reward_cycle <= current_reward_cycle {
+            self.local_state_machine.handle_pending_update(&self.signer_db, stacks_client, &self.proposal_config)
+                .unwrap_or_else(|e| error!("{self}: failed to update local state machine for pending update"; "err" => ?e));
+        }
+
+        if prior_state != self.local_state_machine {
+            let version = self.get_signer_protocol_version();
+            self.local_state_machine
+                .send_signer_update_message(&mut self.stackerdb, version);
+            prior_state = self.local_state_machine.clone();
+        }
+
         let event_parity = match event {
             // Block proposal events do have reward cycles, but each proposal has its own cycle,
             //  and the vec could be heterogeneous, so, don't differentiate.
@@ -272,8 +288,6 @@ impl SignerTrait<SignerMessage> for Signer {
         if event_parity == Some(other_signer_parity) {
             return;
         }
-        self.check_submitted_block_proposal();
-        self.check_pending_block_validations(stacks_client);
         debug!("{self}: Processing event: {event:?}");
         let Some(event) = event else {
             // No event. Do nothing.
@@ -292,12 +306,6 @@ impl SignerTrait<SignerMessage> for Signer {
             return;
         }
 
-        let prior_state = self.local_state_machine.clone();
-        if self.reward_cycle <= current_reward_cycle {
-            self.local_state_machine.handle_pending_update(&self.signer_db, stacks_client, &self.proposal_config)
-                .unwrap_or_else(|e| error!("{self}: failed to update local state machine for pending update"; "err" => ?e));
-        }
-
         self.handle_event_match(stacks_client, sortition_state, event, current_reward_cycle);
 
         self.check_submitted_block_proposal();
@@ -1374,7 +1382,7 @@ impl Signer {
             // Not enough rejection signatures to make a decision
             return;
         }
-        debug!("{self}: {total_reject_weight}/{total_weight} signers voted to reject the block {block_hash}");
+        info!("{self}: {total_reject_weight}/{total_weight} signers voted to reject the block {block_hash}");
         if let Err(e) = self.signer_db.mark_block_globally_rejected(&mut block_info) {
             warn!("{self}: Failed to mark block as globally rejected: {e:?}",);
         }
diff --git a/stacks-signer/src/v0/signer_state.rs b/stacks-signer/src/v0/signer_state.rs
@@ -695,24 +695,33 @@ impl LocalStateMachine {
         local_address: StacksAddress,
         local_update: &StateMachineUpdateMessage,
     ) -> Option<StateMachineUpdateMinerState> {
+        // First always make sure we consider our own viewpoint
+        eval.insert_update(local_address, local_update.clone());
+
+        // Determine the current burn block from the local update
         let current_burn_block = match local_update.content {
             StateMachineUpdateContent::V0 { burn_block, .. }
             | StateMachineUpdateContent::V1 { burn_block, .. } => burn_block,
         };
-        eval.insert_update(local_address, local_update.clone());
+
+        // Determine the global burn view
         let (global_burn_view, _) = eval.determine_global_burn_view()?;
         if current_burn_block != global_burn_view {
+            // We don't have the majority's burn block yet...will have to wait
             crate::monitoring::actions::increment_signer_agreement_state_conflict(
                 crate::monitoring::SignerAgreementStateConflict::BurnBlockDelay,
             );
             return None;
         }
-        let mut current_miners = HashMap::new();
+
+        let mut miners = HashMap::new();
+        let mut potential_matches = Vec::new();
+
         for (address, update) in &eval.address_updates {
             let Some(weight) = eval.address_weights.get(address) else {
                 continue;
             };
-            let (burn_block, current_miner) = match &update.content {
+            let (burn_block, miner_state) = match &update.content {
                 StateMachineUpdateContent::V0 {
                     burn_block,
                     current_miner,
@@ -724,31 +733,48 @@ impl LocalStateMachine {
                     ..
                 } => (burn_block, current_miner),
             };
-
             if *burn_block != global_burn_view {
                 continue;
             }
-
-            let StateMachineUpdateMinerState::ActiveMiner { tenure_id, .. } = current_miner else {
+            let StateMachineUpdateMinerState::ActiveMiner { tenure_id, .. } = miner_state else {
+                // Only consider potential active miners
                 continue;
             };
 
-            let entry = current_miners.entry(current_miner).or_insert_with(|| 0);
+            let entry = miners.entry(miner_state).or_insert(0);
             *entry += weight;
+            if *entry <= eval.total_weight * 3 / 10 {
+                // We don't even see a blocking minority threshold. Ignore.
+                continue;
+            }
 
-            if *entry >= eval.total_weight * 3 / 10 {
-                let nmb_blocks = signerdb
-                    .get_globally_accepted_block_count_in_tenure(tenure_id)
-                    .unwrap_or(0);
-                if nmb_blocks > 0 || eval.reached_agreement(*entry) {
-                    return Some(current_miner.clone());
+            let nmb_blocks = signerdb
+                .get_globally_accepted_block_count_in_tenure(tenure_id)
+                .unwrap_or(0);
+            if nmb_blocks == 0 && !eval.reached_agreement(*entry) {
+                continue;
+            }
+
+            match signerdb.get_burn_block_by_ch(tenure_id) {
+                Ok(block) => {
+                    potential_matches.push((block.block_height, miner_state.clone()));
+                }
+                Err(e) => {
+                    warn!("Error retrieving burn block for consensus_hash {tenure_id} from signerdb: {e}");
                 }
             }
         }
-        crate::monitoring::actions::increment_signer_agreement_state_conflict(
-            crate::monitoring::SignerAgreementStateConflict::MinerView,
-        );
-        None
+
+        potential_matches.sort_by_key(|(block_height, _)| *block_height);
+
+        let new_miner = potential_matches.last().map(|(_, miner)| miner.clone());
+        if new_miner.is_none() {
+            crate::monitoring::actions::increment_signer_agreement_state_conflict(
+                crate::monitoring::SignerAgreementStateConflict::MinerView,
+            );
+        }
+
+        new_miner
     }
 
     #[allow(unused_variables)]
diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs
