Fix fork race condition in optimistic violation tower tests #19192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
stale
bot
added
the
stale
AshwinSekar
removed
the
stale
|
Not stale, this commit fixes the original issue however it exposed another flakiness in the test which i am currently trying to address. |
AshwinSekar
force-pushed
the
flake
branch
2 times, most recently
from
caacb47 to
7da0c51
Compare
carllin
reviewed
Aug 25, 2021
local-cluster/tests/local_cluster.rs
Outdated
Comment on lines
2802
to
2824
| let b_blockstore = open_blockstore(&val_b_ledger_path); | ||
| let a_blockstore = open_blockstore(&val_a_ledger_path); | ||
|
|
||
| // Find latest vote in B, and wait for it to reach blockstore | ||
| let (b_last_vote, _) = last_vote_in_tower(&val_b_ledger_path, &validator_b_pubkey).unwrap(); | ||
| while !b_blockstore.is_full(b_last_vote) { | ||
| sleep(Duration::from_millis(100)); | ||
| } | ||
|
|
||
| // Now we copy these blocks to A | ||
| for slot in | ||
| std::iter::once(b_last_vote).chain(AncestorIterator::new(b_last_vote, &b_blockstore)) | ||
| { | ||
| let slot_meta = b_blockstore.meta(slot).unwrap().unwrap(); | ||
| assert!(slot_meta.is_full()); | ||
|
|
||
| let shreds = b_blockstore.get_data_shreds_for_slot(slot, 0).unwrap(); | ||
| a_blockstore.insert_shreds(shreds, None, false).unwrap(); | ||
|
|
||
| let a_meta = a_blockstore.meta(slot).unwrap().unwrap(); | ||
| assert!(a_meta.is_full()); | ||
| assert_eq!(a_meta.last_index, slot_meta.last_index); | ||
| } |
There was a problem hiding this comment.
might be good to extract this copying logic into a resuable function shared with the other test that does this same thing 😃
Codecov Report
@@ Coverage Diff @@
## master #19192 +/- ##
=========================================
- Coverage 82.7% 82.7% -0.1%
=========================================
Files 459 459
Lines 130822 130822
=========================================
- Hits 108306 108286 -20
- Misses 22516 22536 +20 |
carllin
reviewed
Aug 26, 2021
local-cluster/tests/local_cluster.rs
Outdated
| @@ -2649,6 +2641,21 @@ fn last_vote_in_tower(tower_path: &Path, node_pubkey: &Pubkey) -> Option<(Slot, | |||
| restore_tower(tower_path, node_pubkey).map(|tower| tower.last_voted_slot_hash().unwrap()) | |||
| } | |||
|
|
|||
| // Fetches the last vote in the tower, blocking until it has also appeared in blockstore. | |||
| // Fails if tower is empty | |||
| fn last_vote_in_tower_wait(tower_path: &Path, node_pubkey: &Pubkey) -> Slot { | |||
There was a problem hiding this comment.
nit: rename to wait_for_last_vote_in_tower_to_land_in_ledger
There was a problem hiding this comment.
rename tower_path -> ledger_path
carllin
previously approved these changes
Aug 26, 2021
steviez
pushed a commit
to steviez/solana
that referenced
this pull request
Aug 27, 2021
steviez
pushed a commit
that referenced
this pull request
Aug 27, 2021
carllin
pushed a commit
that referenced
this pull request
Dec 7, 2021
* Fix fork race condition in optimistic violation tower tests * clippy * pr comments
mergify bot
added a commit
that referenced
this pull request
Dec 7, 2021
* Fix fork race condition in optimistic violation tower tests (#19192) * Fix fork race condition in optimistic violation tower tests * clippy * pr comments * Fixup flaky tests (#21617) * Fixup flaky tests * Fixup listeners Co-authored-by: Ashwin Sekar <[email protected]> Co-authored-by: carllin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The optimistic violation tower tests warmup by letting two validators run for a pre determined # of slots. Because of the stake weighting, this warmup fails if there is ever a fork.
Summary of Changes
Instead run only a single validator and manually create the ledger for the other validator. Then run the other validator in order for it to vote and create the correct tower. This achieves the same warmup result as the previous version while guaranteeing there will be no fork.
Fixes #19017