From 3611b8f5aea95143119fa2d82c643fa5a02cc57a Mon Sep 17 00:00:00 2001 From: Divma Date: Mon, 7 Nov 2022 06:48:34 +0000 Subject: [PATCH] Blocklookup data inconsistencies (#3677) Closes #3649 Add a regression test for the data inconsistency, catching the problem in https://github.com/sigp/lighthouse/pull/3677/commits/31e88c5533be9cf25571dd5ffbdf6e0bdc26f060 [here](https://github.com/sigp/lighthouse/actions/runs/3379894044/jobs/5612044797#step:6:2043). When a chain is sent for processing, move it to a separate collection and now the test works, yay! na --- .../network/src/sync/block_lookups/tests.rs | 97 ++++++++++++++++--- 1 file changed, 85 insertions(+), 12 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 47b1830b1bf..06dfdcdf41c 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -255,7 +255,7 @@ fn test_single_block_lookup_becomes_parent_request() { assert_eq!(bl.single_block_lookups.len(), 0); rig.expect_parent_request(); rig.expect_empty_network(); - assert_eq!(bl.parent_queue.len(), 1); + assert_eq!(bl.parent_lookups.len(), 1); } #[test] @@ -283,7 +283,7 @@ fn test_parent_lookup_happy_path() { was_non_empty: true, }; bl.parent_chain_processed(chain_hash, process_result, &mut cx); - assert_eq!(bl.parent_queue.len(), 0); + assert_eq!(bl.parent_lookups.len(), 0); } #[test] @@ -320,7 +320,7 @@ fn test_parent_lookup_wrong_response() { was_non_empty: true, }; bl.parent_chain_processed(chain_hash, process_result, &mut cx); - assert_eq!(bl.parent_queue.len(), 0); + assert_eq!(bl.parent_lookups.len(), 0); } #[test] @@ -352,7 +352,7 @@ fn test_parent_lookup_empty_response() { was_non_empty: true, }; bl.parent_chain_processed(chain_hash, process_result, &mut cx); - assert_eq!(bl.parent_queue.len(), 0); + assert_eq!(bl.parent_lookups.len(), 0); } #[test] @@ -383,7 +383,7 @@ fn test_parent_lookup_rpc_failure() { was_non_empty: true, }; bl.parent_chain_processed(chain_hash, process_result, &mut cx); - assert_eq!(bl.parent_queue.len(), 0); + assert_eq!(bl.parent_lookups.len(), 0); } #[test] @@ -415,11 +415,11 @@ fn test_parent_lookup_too_many_attempts() { } } if i < parent_lookup::PARENT_FAIL_TOLERANCE { - assert_eq!(bl.parent_queue[0].failed_attempts(), dbg!(i)); + assert_eq!(bl.parent_lookups[0].failed_attempts(), dbg!(i)); } } - assert_eq!(bl.parent_queue.len(), 0); + assert_eq!(bl.parent_lookups.len(), 0); } #[test] @@ -446,11 +446,11 @@ fn test_parent_lookup_too_many_download_attempts_no_blacklist() { rig.expect_penalty(); } if i < parent_lookup::PARENT_FAIL_TOLERANCE { - assert_eq!(bl.parent_queue[0].failed_attempts(), dbg!(i)); + assert_eq!(bl.parent_lookups[0].failed_attempts(), dbg!(i)); } } - assert_eq!(bl.parent_queue.len(), 0); + assert_eq!(bl.parent_lookups.len(), 0); assert!(!bl.failed_chains.contains(&block_hash)); assert!(!bl.failed_chains.contains(&parent.canonical_root())); } @@ -487,7 +487,7 @@ fn test_parent_lookup_too_many_processing_attempts_must_blacklist() { } assert!(bl.failed_chains.contains(&block_hash)); - assert_eq!(bl.parent_queue.len(), 0); + assert_eq!(bl.parent_lookups.len(), 0); } #[test] @@ -541,7 +541,7 @@ fn test_parent_lookup_disconnection() { &mut cx, ); bl.peer_disconnected(&peer_id, &mut cx); - assert!(bl.parent_queue.is_empty()); + assert!(bl.parent_lookups.is_empty()); } #[test] @@ -594,5 +594,78 @@ fn test_parent_lookup_ignored_response() { // Return an Ignored result. The request should be dropped bl.parent_block_processed(chain_hash, BlockProcessResult::Ignored, &mut cx); rig.expect_empty_network(); - assert_eq!(bl.parent_queue.len(), 0); + assert_eq!(bl.parent_lookups.len(), 0); +} + +/// This is a regression test. +#[test] +fn test_same_chain_race_condition() { + let (mut bl, mut cx, mut rig) = TestRig::test_setup(Some(Level::Debug)); + + #[track_caller] + fn parent_lookups_consistency(bl: &BlockLookups) { + let hashes: Vec<_> = bl + .parent_lookups + .iter() + .map(|req| req.chain_hash()) + .collect(); + let expected = hashes.len(); + assert_eq!( + expected, + hashes + .into_iter() + .collect::>() + .len(), + "duplicated chain hashes in parent queue" + ) + } + // if we use one or two blocks it will match on the hash or the parent hash, so make a longer + // chain. + let depth = 4; + let mut blocks = Vec::>>::with_capacity(depth); + while blocks.len() < depth { + let parent = blocks + .last() + .map(|b| b.canonical_root()) + .unwrap_or_else(Hash256::random); + let block = Arc::new(rig.block_with_parent(parent)); + blocks.push(block); + } + + let peer_id = PeerId::random(); + let trigger_block = blocks.pop().unwrap(); + let chain_hash = trigger_block.canonical_root(); + bl.search_parent(chain_hash, trigger_block.clone(), peer_id, &mut cx); + + for (i, block) in blocks.into_iter().rev().enumerate() { + let id = rig.expect_parent_request(); + // the block + bl.parent_lookup_response(id, peer_id, Some(block.clone()), D, &mut cx); + // the stream termination + bl.parent_lookup_response(id, peer_id, None, D, &mut cx); + // the processing request + rig.expect_block_process(); + // the processing result + if i + 2 == depth { + // one block was removed + bl.parent_block_processed(chain_hash, BlockError::BlockIsAlreadyKnown.into(), &mut cx) + } else { + bl.parent_block_processed(chain_hash, BlockError::ParentUnknown(block).into(), &mut cx) + } + parent_lookups_consistency(&bl) + } + + // Processing succeeds, now the rest of the chain should be sent for processing. + rig.expect_parent_chain_process(); + + // Try to get this block again while the chain is being processed. We should not request it again. + let peer_id = PeerId::random(); + bl.search_parent(chain_hash, trigger_block, peer_id, &mut cx); + parent_lookups_consistency(&bl); + + let process_result = BatchProcessResult::Success { + was_non_empty: true, + }; + bl.parent_chain_processed(chain_hash, process_result, &mut cx); + assert_eq!(bl.parent_lookups.len(), 0); }