Fix capitulate_miner_view so don't flip flop between multiple potential miners

Signed-off-by: Jacinta Ferrant <[email protected]>

Author: jferrant

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) {
-        global_eval.insert_update(address, new_update.clone());
+    // Let's update 40 percent of other signers to some new miner key
+    for address in addresses.iter().take(4) {
+        global_eval.insert_update(address.clone(), 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"
     );
 }

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)]