incentives: cache top online accounts and use when building AbsentParticipationAccounts #6085
Conversation
…ding AbsentParticipationAccounts
cce
force-pushed
the
track-incentive-candidates
branch
from
975ddb4
to
21db44d
Compare
My first approach was to make it a field in the |
| @@ -1647,7 +1701,7 @@ func (eval *BlockEvaluator) generateKnockOfflineAccountsList() { | |||
|
|
|||
| if acctData.Status == basics.Online { | |||
| lastSeen := max(acctData.LastProposed, acctData.LastHeartbeat) | |||
| if isAbsent(eval.state.prevTotals.Online.Money, acctData.MicroAlgos, lastSeen, current) || | |||
| if isAbsent(eval.state.prevTotals.Online.Money, acctData.MicroAlgosWithRewards, lastSeen, current) || | |||
There was a problem hiding this comment.
@jannotti in your earlier version, you were able to use the (ledgercore.AccountData).MicroAlgos value returned from eval.state.mods.Accts.GetData here without calling AccountData.WithUpdatedRewards() because you knew all accounts in mods would be guaranteed to already have their rewards updated?
There was a problem hiding this comment.
I don't recall thinking it through that way, but it seems true.
There was a problem hiding this comment.
Is it worth checking rewardsLevel or anything to make sure they were applied? I imagine all accounts touched should end up having them applied them but maybe there are some that don't?
There was a problem hiding this comment.
We should do a little further checking. I think any account considered touched here will also be updated. I think for weird cases like an axfer from A to B, only A is touched (since its algo balance changes when paying fee). Similar stuff can happen with accounts in app calls. Probably/hopefully eval.state.modifiedAccounts() are exactly the ones that have truly been touched and will therefore have updated balances.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6085 +/- ##
==========================================
- Coverage 55.81% 53.07% -2.74%
==========================================
Files 488 488
Lines 69581 69656 +75
==========================================
- Hits 38838 36973 -1865
- Misses 28049 29925 +1876
- Partials 2694 2758 +64 ☔ View full report in Codecov by Sentry. |
…break TestAbsenteeChecks
| @@ -1647,7 +1701,7 @@ func (eval *BlockEvaluator) generateKnockOfflineAccountsList() { | |||
|
|
|||
| if acctData.Status == basics.Online { | |||
| lastSeen := max(acctData.LastProposed, acctData.LastHeartbeat) | |||
| if isAbsent(eval.state.prevTotals.Online.Money, acctData.MicroAlgos, lastSeen, current) || | |||
| if isAbsent(eval.state.prevTotals.Online.Money, acctData.MicroAlgosWithRewards, lastSeen, current) || | |||
There was a problem hiding this comment.
I don't recall thinking it through that way, but it seems true.
Co-authored-by: John Jannotti <[email protected]>
| // First, ask the ledger for the top N online accounts, with their latest | ||
| // online account data, current up to the previous round. | ||
| if maxSuspensions > 0 { | ||
| knockOfflineCandidates, err := eval.state.knockOfflineCandidates() |
There was a problem hiding this comment.
since this depends on a node state (not chain state) it could produce different results on different nodes - one can return actual voters and other voters for previous period. In this case network would not agree on block.
Even with "if a current period voters not ready then return empty" rule it is the same story, some might have it ready.
To mitigate consider this:
- move the invocation of
generateKnockOfflineAccountsListafter thestateProofVotersAndTotalinendOfBlock - always aim for the current interval, and return empty if not ready.
This would allows us to ensure that a current interval voters are ready, and all nodes use the same input data.
Upd: I realized it is called only in mode=generate, so probably does not matter, but maybe makes sense to comment/document it is expected
cce
force-pushed
the
track-incentive-candidates
branch
from
f5b42d4
to
01b150a
Compare
Summary
In #5757 a mechanism was introduced to suspend "absentee" accounts that don't participate (by making a proposal, or heartbeat as in #5799), by adding a block header
AbsentParticipationAccounts, similar toExpiredParticipationAccounts.Currently, the list is generated by considering any account touched in the current block, since this data is available in memory at
endOfBlock()time. This PR adds a periodically-updated cache of top online accounts to the ledger, for use in considering accounts that are not involved in the current block for absentee or expired status.To get a list of recent top online accounts, this PR grabs the most recently-completed collection of top online accounts compiled by the state proof worker. Every 256 rounds, the state proof system performs a TopOnlineAccounts query, so access was added to get its most recently-updated list of online addresses, and look up their values for the latest round from the online account cache.
LastProposed and LastHeartbeat are added to the online accounts table's DB representation in this PR. This also fixes an issue introduced in #5965 where uses of ledgercore.OnlineAccountData (which didn't have LastHeartbeat/LastProposed fields) were replaced by basics.OnlineAccountData (which did) and ended up with those fields not being set in a couple of conversions from AccountData.
Test Plan