-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
local-cluster: fix flaky optimistic_confirmation tests #35356
Conversation
a07a57a
to
e667cfb
Compare
e667cfb
to
cb6d6cd
Compare
This makes sense, we thought all these blocks were truncated from the ledger by the blockstore purge from all the validators after they're shut down: solana/local-cluster/tests/local_cluster.rs Line 3297 in cb6d6cd
Is the culprit because validator Edit: Oh I see, we ensure |
local-cluster/tests/local_cluster.rs
Outdated
sleep(Duration::from_millis(100)); | ||
|
||
if let Some((last_vote, _)) = last_vote_in_tower(&val_c_ledger_path, &validator_c_pubkey) { | ||
if let Some((latest_vote, _)) = last_vote_in_tower(&val_c_ledger_path, &validator_c_pubkey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: latest_vote -> newest_vote
local-cluster/tests/local_cluster.rs
Outdated
@@ -3172,29 +3172,36 @@ fn do_test_optimistic_confirmation_violation_with_or_without_tower(with_tower: b | |||
// below only for slots <= `next_slot_on_a`, validator A will not know how it's last vote chains | |||
// to the other forks, and may violate switching proofs on restart. | |||
let mut default_config = ValidatorConfig::default_for_test(); | |||
// Split leader schedule 50-50 between validators B and C, don't give validator A any slots because | |||
// Ensure B can make leader blocks up till the fork slot, and give remaining to C, don't give validator A any slots because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: and give the remaining slots to C
local-cluster/tests/local_cluster.rs
Outdated
]; | ||
// Trick C into not producing any blocks, in case it takes too long to kill it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: in case
C takes a long time to kill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Just some nits
beef260
to
e8b716f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #35356 +/- ##
========================================
Coverage 81.7% 81.7%
========================================
Files 834 834
Lines 224299 224767 +468
========================================
+ Hits 183361 183815 +454
- Misses 40938 40952 +14 |
Problem
Summary of Changes
Use a custom leader schedule such that C never produces blocks after restart and B only produces blocks up until the initial fork
Fixes #29221
Fixes #34102
Fixes #33669