diff --git a/chain/client/src/client.rs b/chain/client/src/client.rs index f3733caf0ae..67d4dc609a8 100644 --- a/chain/client/src/client.rs +++ b/chain/client/src/client.rs @@ -2302,6 +2302,44 @@ impl Client { } } + /// 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 { + let hashes = self.chain.chain_store().get_all_block_hashes_by_height(parent_height); + let mut iter = hashes.values().flatten().copied(); + let first = iter.next()?; + let Some(my_account_id) = my_account_id else { + return Some(first); + }; + let is_producer = |hash: &CryptoHash| { + 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(my_account_id) + }; + if is_producer(&first) { + Some(first) + } else { + Some(iter.find(is_producer).unwrap_or(first)) + } + } + /// Collects block approvals. /// /// We send the approval to doomslug given the epoch of the current tip iff: @@ -2322,29 +2360,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; } } } @@ -2392,7 +2428,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, diff --git a/test-loop-tests/src/tests/doomslug.rs b/test-loop-tests/src/tests/doomslug.rs new file mode 100644 index 00000000000..80df4ea92e4 --- /dev/null +++ b/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; + } +} diff --git a/test-loop-tests/src/tests/mod.rs b/test-loop-tests/src/tests/mod.rs index d95365be290..909722d260b 100644 --- a/test-loop-tests/src/tests/mod.rs +++ b/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;