fix(bft): correctly drain channels events

[gfanton]

addresses #1320

This PR does two things:

• It prevents deadlocks in state_test by triggering a panic if GetRoundState becomes blocked. In the event of any panic, it will attempt to drain all relevant channels to identify which events have not been drained.
• It fixes tests that have been failing randomly on the CI.

---

for now, you can checkout this branch, it will fail locally (using @jaekwon trick)

There are still some tests that I was unable to resolve. I have marked these with // XXX(FIXME) in tm2/pkg/bft/consensus/state_test.go:

• TestStateFullRound1
• TestWaitingTimeoutProposeOnNewRound
• TestStateEnterProposeYesPrivValidator
• TestStartNextHeightCorrectly

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests
• Added new benchmarks to generated graphs, if any. More info here.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.03%. Comparing base (ccc6d5b) to head (783c214).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1515      +/-   ##
==========================================
- Coverage   49.04%   49.03%   -0.01%     
==========================================
  Files         576      576              
  Lines       77556    77556              
==========================================
- Hits        38035    38033       -2     
  Misses      36439    36439              
- Partials     3082     3084       +2     

Flag	Coverage Δ
tm2	54.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jaekwon]

Thank you for doing this work.

[moul]

@gfanton what's the status of this PR?

[gfanton]

@moul

Current state is:

• The PR causes the BFT test to fail early and shows the drained channel to aid debugging.
• I've fixed some tests by rearranging the event order.
• Unfotunatly, rearranging event consumption will not resolve the last 3 failing test. Or at last i wasn't able to do so.
• I still believe the root issue lies not in the event system (even if there are rooms for improvement), but in how we test event ordering.

What we can do:

• The core problem, to me, is that in an event bus system, a subscription is meant to be consumed. The test is actually leaving subscriptions on hold, which ultimately leads to the test getting stuck. This is especially true for a system that is meant to have no buffer size for sync reasons.
• I still believe that using a single routine to queue events inside the tests can be a good idea. Even if the routine is asynchronous, events will be stored in the queue in the correct order.
• Another potential solution could be to make subscriptions cancelable. By doing so, we can simply cancel previously stuck subscription. However, I'm still not sure about this, as we may still encounter a problem where a necessary subscription is stuck and cannot be canceled.

EDIT: Got another idea but I'm not really liking it as it looks a bit messy: The main deadlock issue come from the method GetRoundState, which uses cs.mutex. If we replace it with getRoundStateUnsafe (which retrieves the round state without locking) for these tests only, it should actually fix all the BFT tests. It can make sense, as I mentioned in my comments, that event bus subscription is meant to be consumed. If we consume events synchronously, it also makes sense to use some tricks to avoid deadlocks in this particular case.
Some tests would probably need to be updated as we need to ensure that RoundState is correctly updated before being retrieved.

• preview

My preference is still to have an event queue as it looks way cleaner to me.

[moul]

Let's talk about merging the current state to enhance global efficiency in the upcoming review meeting. We can then collaborate with @jaekwon to resolve the remaining cases.

[thehowl]

Review Meeting: @gfanton to do a cleanup / take another look, @jaekwon to review.

[gfanton]

TL;DR: Reverting ensureGetRoundState to GetRoundState by eliminating the delay should resolve the last tests (even if technically they are still racy), boosting CI success rate to nearly 100%.
After merging, we should monitor consensus/state_test.go in future PRs to handle any remaining flaky tests.

Next Review Meeting Note: I need a final check on f17c9b5. If the commit looks good, the PR is ready for me to merge.

[zivkovicmilos]

You know I'm not a fan of these sorts of fixes, especially when they use the hidden jutsu of reflect :)

I think it's finally time to put the flaky CI to rest (at least most of it), so I'm willing to move on this fix 👏

It is worth noting that we have fixed all of these issues in the libtm testing suite, so even these tests are just here temporarily