fix: better descendency check

Author: hstove

stacks-signer/src/v0/signer_state.rs

@@ -997,40 +997,20 @@ impl LocalStateMachine {
         replay_state: &ReplayState,
     ) -> Result<Option<ReplayState>, SignerChainstateError> {
         if expected_burn_block.burn_block_height > prior_state_machine.burn_block_height {
-            // prevent too large of a loop
-            if expected_burn_block
-                .burn_block_height
-                .saturating_sub(prior_state_machine.burn_block_height)
-                > 10
-            {
-                return Ok(None);
-            }
-            // are we building on top of this prior tip?
-            let mut parent_burn_block_info =
-                db.get_burn_block_by_ch(&expected_burn_block.consensus_hash)?;
-
-            while parent_burn_block_info.block_height > prior_state_machine.burn_block_height {
-                let Ok(parent_info) =
-                    db.get_burn_block_by_hash(&parent_burn_block_info.parent_burn_block_hash)
-                else {
-                    warn!(
-                        "Failed to get parent burn block info for {}",
-                        parent_burn_block_info.parent_burn_block_hash
-                    );
-                    return Ok(None);
-                };
-                parent_burn_block_info = parent_info;
-            }
-            if parent_burn_block_info.consensus_hash == prior_state_machine.burn_block {
-                // no bitcoin fork, because we're building on the parent
-                return Ok(None);
-            } else {
+            if Self::new_burn_block_fork_descendency_check(
+                db,
+                expected_burn_block,
+                prior_state_machine.burn_block_height,
+                prior_state_machine.burn_block,
+            )? {
                 info!("Detected bitcoin fork - prior tip is not parent of new tip.";
                     "new_tip.burn_block_height" => expected_burn_block.burn_block_height,
                     "new_tip.consensus_hash" => %expected_burn_block.consensus_hash,
                     "prior_tip.burn_block_height" => prior_state_machine.burn_block_height,
                     "prior_tip.consensus_hash" => %prior_state_machine.burn_block,
                 );
+            } else {
+                return Ok(None);
             }
         }
         if expected_burn_block.consensus_hash == prior_state_machine.burn_block {
@@ -1273,4 +1253,53 @@ impl LocalStateMachine {
 
         Ok(new_burn_block.burn_block_height > failsafe_height)
     }
+
+    /// Check if the new burn block is a fork, by checking if the new burn block
+    /// is a descendant of the prior burn block
+    fn new_burn_block_fork_descendency_check(
+        db: &SignerDb,
+        new_burn_block: &NewBurnBlock,
+        prior_burn_block_height: u64,
+        prior_burn_block_ch: ConsensusHash,
+    ) -> Result<bool, SignerChainstateError> {
+        let max_height_delta = 10;
+        let height_delta = match new_burn_block
+            .burn_block_height
+            .checked_sub(prior_burn_block_height)
+        {
+            None | Some(0) => return Ok(false), // same height or older
+            Some(d) if d > max_height_delta => return Ok(false), // too far apart
+            Some(d) => d,
+        };
+
+        let Ok(mut parent_burn_block_info) =
+            db.get_burn_block_by_ch(&new_burn_block.consensus_hash)
+        else {
+            warn!(
+                "Failed to get parent burn block info for {}",
+                new_burn_block.consensus_hash
+            );
+            return Ok(false);
+        };
+
+        for _ in 0..height_delta {
+            if parent_burn_block_info.block_height == prior_burn_block_height {
+                return Ok(parent_burn_block_info.consensus_hash != prior_burn_block_ch);
+            }
+
+            parent_burn_block_info =
+                match db.get_burn_block_by_hash(&parent_burn_block_info.parent_burn_block_hash) {
+                    Ok(bi) => bi,
+                    Err(e) => {
+                        warn!(
+                            "Failed to get parent burn block info for {}. Error: {e}",
+                            parent_burn_block_info.parent_burn_block_hash
+                        );
+                        return Ok(false);
+                    }
+                };
+        }
+
+        Ok(false)
+    }
 }

testnet/stacks-node/src/tests/signer/mod.rs

@@ -325,7 +325,11 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
             let metadata_path = snapshot_path.join("metadata.json");
             if !metadata_path.clone().exists() {
                 warn!("Snapshot metadata file does not exist, not restoring snapshot");
-                return SetupSnapshotResult::NoSnapshot;
+                std::fs::remove_dir_all(snapshot_path.clone()).unwrap();
+                return SetupSnapshotResult::WithSnapshot(SnapshotSetupInfo {
+                    snapshot_path: snapshot_path.clone(),
+                    snapshot_exists: false,
+                });
             }
             let Ok(metadata) = serde_json::from_reader::<_, SnapshotMetadata>(
                 File::open(metadata_path.clone()).unwrap(),

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

@@ -3982,13 +3982,6 @@ fn tx_replay_simple() {
     let _http_origin = format!("http://{}", &conf.node.rpc_bind);
     let btc_controller = &signer_test.running_nodes.btc_regtest_controller;
 
-    let miner_pk = btc_controller
-        .get_mining_pubkey()
-        .as_deref()
-        .map(Secp256k1PublicKey::from_hex)
-        .unwrap()
-        .unwrap();
-
     if signer_test.bootstrap_snapshot() {
         signer_test.shutdown_and_snapshot();
         return;