fix(chain): prefer block producer-matching parent in skip approval resolution

chain/client/src/client.rs

@@ -2172,6 +2172,41 @@ impl Client {
     /// * `approval` - the approval to be collected
     /// * `approval_type`  - whether the approval was just produced by us (in which case skip validation,
     ///                      only check whether we are the next block producer and store in Doomslug)
+    /// Resolve the parent block hash for a skip approval.
+    ///
+    /// Multiple blocks can exist at `parent_height` when there are forks
+    /// (e.g. around an epoch boundary). Each fork block may belong to a
+    /// different epoch with different block-producer assignments for
+    /// `target_height`. We prefer the parent whose epoch makes us the block
+    /// producer so that the approval is not silently dropped later.
+    ///
+    /// Returns `None` when no blocks exist at `parent_height`.
+    fn resolve_skip_parent(
+        &self,
+        parent_height: BlockHeight,
+        target_height: BlockHeight,
+        my_account_id: Option<&AccountId>,
+    ) -> Option<CryptoHash> {
+        let hashes = self.chain.chain_store().get_all_block_hashes_by_height(parent_height);
+        let mut first = None;
+        let chosen = hashes.values().flatten().find(|hash| {
+            if first.is_none() {
+                first = Some(**hash);
+            }
+            my_account_id.is_some_and(|id| {
+                self.epoch_manager
+                    .get_epoch_id_from_prev_block(hash)
+                    .and_then(|epoch_id| {
+                        self.epoch_manager.get_block_producer(&epoch_id, target_height)
+                    })
+                    .ok()
+                    .as_ref()
+                    == Some(id)
+            })
+        });
+        chosen.copied().or(first)
+    }
+
     pub fn collect_block_approval(&mut self, approval: &Approval, approval_type: ApprovalType) {
         let Approval { inner, account_id, target_height, signature } = approval;
         tracing::debug!(target: "client",
@@ -2180,29 +2215,27 @@ impl Client {
             target_height=target_height,
             approval_type=?approval_type,
             "collect_block_approval");
+        let signer = self.validator_signer.get();
         let parent_hash = match inner {
             ApprovalInner::Endorsement(parent_hash) => *parent_hash,
             ApprovalInner::Skip(parent_height) => {
-                {
-                    let hashes =
-                        self.chain.chain_store().get_all_block_hashes_by_height(*parent_height);
-                    // If there is more than one block at the height, all of them will be
-                    // eligible to build the next block on, so we just pick one.
-                    let hash = hashes.values().flatten().next();
-                    match hash {
-                        Some(hash) => *hash,
-                        None => {
-                            self.handle_process_approval_error(
-                                approval,
-                                approval_type,
-                                true,
-                                near_chain::Error::DBNotFoundErr(format!(
-                                    "Cannot find any block on height {}",
-                                    parent_height
-                                )),
-                            );
-                            return;
-                        }
+                match self.resolve_skip_parent(
+                    *parent_height,
+                    *target_height,
+                    signer.as_ref().map(|s| s.validator_id()),
+                ) {
+                    Some(hash) => hash,
+                    None => {
+                        self.handle_process_approval_error(
+                            approval,
+                            approval_type,
+                            true,
+                            near_chain::Error::DBNotFoundErr(format!(
+                                "cannot find any block on height {}",
+                                parent_height
+                            )),
+                        );
+                        return;
                     }
                 }
             }
@@ -2250,7 +2283,6 @@ impl Client {
             }
         }
 
-        let signer = self.validator_signer.get();
         let is_block_producer =
             match self.epoch_manager.get_block_producer(&next_block_epoch_id, *target_height) {
                 Err(_) => false,

test-loop-tests/src/tests/doomslug.rs

@@ -0,0 +1,120 @@
+use crate::setup::builder::TestLoopBuilder;
+use near_async::time::Duration;
+use near_client::client_actor::AdvProduceBlockHeightSelection;
+use near_o11y::testonly::init_test_logger;
+use near_primitives::block::{Approval, ApprovalInner, ApprovalType};
+use near_primitives::hash::CryptoHash;
+use near_primitives::test_utils::create_test_signer;
+
+/// Regression test ensuring that `collect_block_approval`, when multiple blocks
+/// exist at a skip approval's `parent_height`, prefers a parent hash under
+/// which the current node is the block producer for `target_height`, so the
+/// approval is not silently dropped.
+///
+/// This test runs in multiple trials because the pre-fix behavior was
+/// non-deterministic: an arbitrary hash was picked from `HashMap` iteration
+/// order, which accidentally matched the current node's producer schedule in
+/// roughly half the cases. Each trial:
+///   - advance to the first block of a new epoch (fork_height),
+///   - fork: produce a skip block at fork_height whose parent is at
+///     fork_height - 2, so the skip block stays in the *previous* epoch while
+///     the canonical block at fork_height is in the new epoch,
+///   - scan for a target_height > fork_height where the two epochs disagree
+///     on the block producer, choosing the producer under the skip epoch as
+///     fork_producer (the only node that has both sibling blocks in its
+///     store — others reject the adversarial skip for NotEnoughApprovals),
+///   - inject a self-Skip(fork_height) approval into fork_producer and
+///     assert it reaches doomslug.
+#[test]
+fn test_skip_approval_prefers_producer_matching_parent() {
+    init_test_logger();
+
+    let epoch_length = 10;
+    let num_validators = 2;
+    let mut env =
+        TestLoopBuilder::new().validators(num_validators, 0).epoch_length(epoch_length).build();
+
+    // Advance past the first couple of epochs — they can share rng seeds and
+    // produce the same block-producer schedule, making "different epoch"
+    // forks indistinguishable.
+    env.validator_runner().run_until_new_epoch();
+    env.validator_runner().run_until_new_epoch();
+
+    // 4 trials leave a ~6% false-pass probability of the prior ~50%
+    //     non-deterministic behavior (HashMap iteration order).
+    let mut need_success = 4;
+    let mut iter = 0;
+    while need_success > 0 {
+        iter += 1;
+        assert!(iter <= 20, "ran out of iterations without finding enough testable boundaries");
+
+        env.validator_runner().run_until_new_epoch();
+        let fork_height = env.validator().head().height;
+        let canonical_epoch = env.validator().head().epoch_id;
+
+        let prev_block_hash = env
+            .validator()
+            .client()
+            .chain
+            .chain_store()
+            .get_block_hash_by_height(fork_height - 2)
+            .unwrap();
+        let epoch_manager = env.validator().client().epoch_manager.clone();
+        let skip_epoch = epoch_manager.get_epoch_id_from_prev_block(&prev_block_hash).unwrap();
+        if skip_epoch == canonical_epoch {
+            tracing::warn!(fork_height, "no epoch boundary, skipping trial");
+            continue;
+        }
+
+        let fork_producer = epoch_manager.get_block_producer(&skip_epoch, fork_height).unwrap();
+        let Some(target_height) = (fork_height + 2..fork_height + 50).find_map(|target_height| {
+            let p_canonical =
+                epoch_manager.get_block_producer(&canonical_epoch, target_height).ok()?;
+            let p_skip = epoch_manager.get_block_producer(&skip_epoch, target_height).ok()?;
+            (p_canonical != p_skip && p_skip == fork_producer).then_some(target_height)
+        }) else {
+            tracing::warn!(fork_height, "no usable target_height, skipping trial");
+            continue;
+        };
+
+        // Produce a skip block at fork_height whose parent is at fork_height - 2,
+        // creating a sibling of the canonical block at fork_height in a
+        // different epoch.
+        env.node_for_account_mut(&fork_producer).client_actor().adv_produce_blocks_on(
+            1,
+            true,
+            AdvProduceBlockHeightSelection::SelectedHeightOnSelectedBlock {
+                produced_block_height: fork_height,
+                base_block_height: fork_height - 2,
+            },
+        );
+
+        // Wait for the fork block to land in fork_producer's chain store.
+        // `adv_produce_blocks_on` schedules async block processing, so the
+        // fork is not visible immediately. `get_all_block_hashes_by_height`
+        // returns a map keyed by epoch id, so `keys().len() >= 2` confirms
+        // the two siblings are in two different epochs.
+        env.runner_for_account(&fork_producer).run_until(
+            |node| {
+                let blocks =
+                    node.client().chain.chain_store().get_all_block_hashes_by_height(fork_height);
+                blocks.keys().len() >= 2
+            },
+            Duration::seconds(10),
+        );
+
+        let signer = create_test_signer(fork_producer.as_str());
+        let approval = Approval::new(CryptoHash::default(), fork_height, target_height, &signer);
+        assert!(matches!(approval.inner, ApprovalInner::Skip(_)));
+
+        let mut node = env.node_for_account_mut(&fork_producer);
+        let client = &mut node.client_actor().client;
+        client.collect_block_approval(&approval, ApprovalType::SelfApproval);
+        assert!(
+            !client.doomslug.approval_status_at_height(&target_height).approvals.is_empty(),
+            "approval should reach doomslug at fork_height {fork_height}",
+        );
+
+        need_success -= 1;
+    }
+}

test-loop-tests/src/tests/mod.rs

@@ -17,6 +17,8 @@ mod contract_distribution_simple;
 mod create_delete_account;
 mod cross_shard_tx;
 mod deterministic_account_id;
+#[cfg(feature = "test_features")]
+mod doomslug;
 mod early_prepare_transactions;
 #[cfg(feature = "test_features")]
 mod eth_implicit_global_contract;