Fix test_optimistic_confirmation_violation_without_tower()

[carllin]

Problem

test_optimistic_confirmation_violation_without_tower is flaky with failure message: "Violation expected because of removed persisted tower!". Example of failure case:

[2020-10-20T22:12:25.389384948Z INFO  local_cluster] collected validator C's votes: {29, 30, 31, 32}
[2020-10-20T22:12:36.152101878Z INFO  local_cluster] Observed A's votes on: [
    26, 26, 26, 26, 26, 26, 26, 26, 26, 33, 33, 33, 33, 33, 33, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 35, 44, 44, 44, 44, 44, 45, 45, 45, 45, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 52, 52, 52, 52, 53, 53, 53, 53, 53, 53, 53, 53, 53, 53]

A's vote on 33 is a descendant of C's block 32, but the test doesn't recognize this.

Summary of Changes

Check blockstore for descendants check

Context #10718 #12350

[t-nelson]

Nice!

[t-nelson]

Suggested change

	// C can avoid NoPropagatedConfirmation erorrs and continue to generate blocks
	// 2) Provide gosisp discovery for `A` when it restarts because `A` will restart
	// at a different gossip port than the entrypoint saved in C's gosisp table
	// C can avoid NoPropagatedConfirmation errors and continue to generate blocks
	// 2) Provide gossip discovery for `A` when it restarts because `A` will restart
	// at a different gossip port than the entrypoint saved in C's gossip table

You still blaming the keyboard? 😆

[carllin]

Oh, that's just how I thought those were words were spelled, totally on purpose

Pull request has been modified.

[codecov]

Codecov Report

Merging #13043 into master will increase coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #13043   +/-   ##
=======================================
  Coverage    82.1%    82.1%           
=======================================
  Files         366      366           
  Lines       86101    86101           
=======================================
+ Hits        70742    70749    +7     
+ Misses      15359    15352    -7     

[ryoqun]

TIL

[ryoqun]

well, this only breaks from the newly-added inner for loop, maybe break 'outer or use if ancestors.any(|a| votes_on_c_fork.contains(a)) { bad_vote_detected = true; break}.

I'd prefer the latter. Nested loops are hard to manage as this bug illustrates. ;)

I think the outer loop will wait for the timeout needlessly currently with the new code.

[ryoqun]

@carllin Hooray! Thanks for debugging the propagation error and actually helping to fix the flaky test_optimistic_confirmation_violation_without_tower. I owe you.

[ryoqun]

@ryoqun ryoqun added the v1.4 label 14 seconds ago

Adding this, because this fluky test is currently affected on v1.4 branch as well