tests: reenable TestVotersReloadFromDiskPassRecoveryPeriod #5496
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5496 +/- ##
==========================================
+ Coverage 55.66% 55.70% +0.04%
==========================================
Files 446 446
Lines 63128 63144 +16
==========================================
+ Hits 35138 35174 +36
+ Misses 25612 25603 -9
+ Partials 2378 2367 -11
... and 9 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Failed against the current master : c223dbc |
|
This is error tells voters failed to build a tree at round 752 = 256 * 3 - 16 because trackers advanced. This should not happen and appears to be a real bug. Good catch. |
|
The test is using in-memory sqlite and the issue most likely caused by db access congestion Kind of weird, voters should prevent trackers from advancing if lagging behind. |
|
Narrowed the issue down to |
|
@icorderi |
| } | ||
| var roundOffsetError *RoundOffsetError | ||
| if !errors.As(err, &roundOffsetError) { | ||
| return ledgercore.OnlineRoundParamsData{}, err |
There was a problem hiding this comment.
The onlineTotalsEx version of this logs the error but doesn't return, then falls through to doing LookupOnlineRoundParams.. slightly inconsistent
There was a problem hiding this comment.
yes, I noticed that as well, if I remember right, it was some temporal thing while we do not have stateproof recoverability. I guess it is safe to remove, but I'll double check
| @@ -569,8 +589,12 @@ func (ao *onlineAccounts) onlineTotalsEx(rnd basics.Round) (basics.MicroAlgos, e | |||
| ao.log.Errorf("onlineTotals error: %v", err) | |||
| } | |||
|
|
|||
| totalsOnline, err = ao.accountsq.LookupOnlineTotalsHistory(rnd) | |||
| return totalsOnline, err | |||
| roundParams, err := ao.accountsq.LookupOnlineRoundParams(rnd) | |||
There was a problem hiding this comment.
There's an argument that onlineTotalsEx can just be a wrapper around roundParamsEx now and not have this RoundOffsetError-checking logic implemented twice, but I don't feel strongly about it
There was a problem hiding this comment.
It makes you wonder, do we even need the onlineTotalsEx() method at all — it is only called from TopOnlineAccounts.
There was a problem hiding this comment.
I guess the difference is onlineTotalsEx() calls onlineTotals() which acquires the accountsMu.RLock itself, but roundParamsEx does not. But TopOnlineAccounts could acquire the RLock too or pass a synchronized=false parameter when calling roundParamsEx or something.
| @@ -556,6 +556,26 @@ func (ao *onlineAccounts) onlineCirculation(rnd basics.Round, voteRnd basics.Rou | |||
| return totalStake, nil | |||
| } | |||
|
|
|||
| // roundsParamsEx return the round params for the given round for extended rounds range | |||
| // by looking into DB as needed | |||
| // locking semantics: requires accountsMu.RLock() | |||
There was a problem hiding this comment.
This comment is ambiguous: does it lock accountsMu or requires accountsMu to be held before calling this function?
Summary
Fix
onlineAcctsExpiredByRoundand allow it to lookup DB for round params data (proto + totals) similarly to how it works for online accounts: this info is available in DB for cases when trackers DB advanced but voters preserve longer online accounts history.Reenable TestVotersReloadFromDiskPassRecoveryPeriod test after this fix.
Test Plan
Locally the test appears to be working fine.
Repro: