Skip to content
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

Use is_trusted bool in insert_shreds() instead manually adjusting root #34010

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Nov 9, 2023

Problem

The test_duplicate_with_pruned_ancestor test needs to get around a
limitation where the shreds with a parent older than the latest root are
discarded. The previous approach manually adjusted the root value in the
blockstore; this is not ideal in that it is fiddling with the inner
working of Blockstore.

Summary of Changes

So, use the is_trusted argument in Blockstore::insert_shreds(); setting
is_trusted=true bypasses the sanity checks (including the parent >=
latest root check).

@steviez steviez force-pushed the lc_rm_bstore_root_hack branch from 5b2984e to bb2bfc7 Compare November 9, 2023 20:32
Steven Czabaniuk added 2 commits November 9, 2023 14:35
The test_duplicate_with_pruned_ancestor test needs to get around a
limitation where the shreds with a parent older than the latest root are
discarded. The previous approach manually adjusted the root value in the
blockstore; this is not ideal in that it is fiddling with the inner
working of Blockstore.

So, use the is_trusted argument in Blockstore::insert_shreds(); setting
is_trusted=true bypasses the sanity checks (including the parent >=
latest root check).
@steviez steviez force-pushed the lc_rm_bstore_root_hack branch from bb2bfc7 to f423e5d Compare November 9, 2023 20:35
@steviez steviez marked this pull request as ready for review November 9, 2023 21:31
@steviez steviez requested a review from AshwinSekar November 9, 2023 21:31
AshwinSekar
AshwinSekar previously approved these changes Nov 9, 2023
Copy link
Contributor

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this, a much better solution than my hack! just a minor nit on a comment

local-cluster/tests/local_cluster.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #34010 (142e35c) into master (28e08ac) will increase coverage by 0.0%.
Report is 3 commits behind head on master.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #34010   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         811      811           
  Lines      219427   219424    -3     
=======================================
+ Hits       179769   179771    +2     
+ Misses      39658    39653    -5     

@steviez
Copy link
Contributor Author

steviez commented Nov 10, 2023

I was initially a little concerned in that one of the local-cluster CI steps took 3 retries to succeed. However, looking at the logs for those 3 failures, all 3 failed at the same spot on a different test:

thread 'test_optimistic_confirmation_violation_without_tower' panicked at local-cluster/tests/local_cluster.rs:3382:13:

Violation expected because of removed persisted tower!

test test_optimistic_confirmation_violation_without_tower has been running for over 60 seconds

test test_optimistic_confirmation_violation_without_tower ... FAILED

This is not the test whose behavior I altered; test_optimistic_confirmation_violation_without_tower does call copy_block() as well, but it has a value of false set which matches the pre-existing behavior. Namely, this false matches the previously hard-coded false in copy_blocks().

So, it would appear that this test was previously flaky, and my PR does not alter the flaky test so I think I can ok to submit. Do you agree with my conclusion @AshwinSekar; also CC @yihau as I know you've been keeping an eye on flaky tests

@AshwinSekar
Copy link
Contributor

yeah that's a known flaky test. i was unable to reproduce it locally and then it quickly got buried in my todo. I'll take a look again at it next week #29221

@steviez steviez merged commit 1057ba8 into solana-labs:master Nov 10, 2023
17 checks passed
@steviez steviez deleted the lc_rm_bstore_root_hack branch November 10, 2023 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants