incentives: cache top online accounts and use when building AbsentParticipationAccounts by cce · Pull Request #6085 · algorand/go-algorand

cce · 2024-07-26T17:24:51Z

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 to ExpiredParticipationAccounts.

Currently, the list is generated by considering any account touched by a transaction in the current block, since this data is readily available at endOfBlock(). This PR adds a periodically-updated cache of top online accounts to the ledger, to find additional online accounts not mentioned in the current block.

All of these tracked addresses will now be checked for absentee or expired status each round. To get a recent list of top online accounts, this PR uses recent work done by the votersTracker and state proof worker. (Every 256 rounds, the state proof system performs a TopOnlineAccounts query.) This adds access to the votersTracker to fetch the most recent list of top online addresses, and for each address looks up the latest round's data 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

update TestAbsenteeChecks
update TestExpiredAccountGeneration
maybe update TestExpiredAccountGenerationWithDiskFailure?
update TestAbsentTracking
added new TestLatestCompletedVotersUpTo
update TestAbsenteeChallenges
update TestEvalFunctionForExpiredAccounts
disable Payouts for state proof E2E test TestTotalWeightChanges (TODO return later after auto-heartbeating added, to ensure the accounts aren't suspended)
~~update test/e2e-go/features/incentives/suspension_test.go~~ (TODO return later after heartbeats)

…ding AbsentParticipationAccounts

algorandskiy

LGTM.

Just a thought - why not to init top online on startup and then maintain the list in acctonline while processing incoming blocks?

cce · 2024-07-27T00:43:55Z

Just a thought - why not to init top online on startup and then maintain the list in acctonline while processing incoming blocks?

My first approach was to make it a field in the onlineAccounts tracker like the ao.voters sub-tracker, but as I kept cutting it down to MVP and removed its access to the onlineAccounts.deltas, and took it out of being called in onlineAccounts.newBlockImpl(), I moved it out since it had no dependencies left. It felt like I kept adding duplicate state that was already being maintained in onlineAccounts, and duplicate logic for looking it up. So I wanted to try just the absolute minimal approach to start, by relying totally on the onlineAccounts tracker's caching system (which we already know works correctly) rather than add a new cached list of online accounts and delta-processing code..

codecov · 2024-08-09T17:23:41Z

Codecov Report

Attention: Patch coverage is 77.57009% with 24 lines in your changes missing coverage. Please review.

Project coverage is 56.27%. Comparing base (619d257) to head (9d46fa6).
Report is 8 commits behind head on feature/heartbeats.

Files with missing lines	Patch %	Lines
ledger/eval/eval.go	87.27%	4 Missing and 3 partials ⚠️
ledger/ledger.go	75.00%	2 Missing and 2 partials ⚠️
ledger/ledgercore/votersForRound.go	0.00%	4 Missing ⚠️
ledger/tracker.go	0.00%	3 Missing ⚠️
cmd/tealdbg/localLedger.go	0.00%	2 Missing ⚠️
daemon/algod/api/server/v2/dryrun.go	0.00%	2 Missing ⚠️
ledger/ledgercore/accountdata.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##           feature/heartbeats    #6085      +/-   ##
======================================================
+ Coverage               56.22%   56.27%   +0.05%     
======================================================
  Files                     494      494              
  Lines                   69954    70040      +86     
======================================================
+ Hits                    39330    39416      +86     
+ Misses                  27947    27944       -3     
- Partials                 2677     2680       +3

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

…break TestAbsenteeChecks

…andidates

Co-authored-by: John Jannotti <jannotti@gmail.com>

…andidates

…ineAccountsList

gmalouf · 2024-10-08T19:07:33Z

 	// test same scenario on double ledger
 	t.Run("DoubleLedger", func(t *testing.T) {
-		m := newDoubleLedgerAcctModel(t, protocol.ConsensusFuture, true)
+		m := newDoubleLedgerAcctModel(t, protocol.ConsensusV39, true) // TODO simulate heartbeats


Why not keep this on future?

It fails because heartbeats aren't implemented, but proposers aren't being set, so the big accounts are challenged and kicked offline, and all the stake numbers don't match the test expectations. I could have tried to fix this by ensuring all the test accounts show up as proposers as often as necessary to avoid suspension, but I thought maybe it would be better to see after heartbeats were implemented whether that would make the tests pass without as much modification.

gmalouf · 2024-10-08T19:11:48Z

 	// lookup retrieves agreement data about an address, querying the ledger if necessary.
 	lookupAgreement(basics.Address) (basics.OnlineAccountData, error)
 	onlineStake() (basics.MicroAlgos, error)
+	knockOfflineCandidates() (map[basics.Address]basics.OnlineAccountData, error)


Maybe a NIT: should we actually call this top online accounts or similar naming? It's very clear from comments that's what we are requesting, more a debate over if the name should be based on what it's sourced from vs the use-case we have for this atm.

It is a potentially stale list of top online accounts, if new accounts appeared online in the last 256 rounds (since the last state proof) they wouldn't appear. So the word "candidates" was intended to make it seem a little less definitive that this was the complete list of top online accounts for the round... but happy to pick any other name, I wasn't particularly happy with this name.

This is already being used in a method JJ called "generateKnockOfflineAccountsList" in #5757 which is where the "knockOffline" part came from.

gmalouf · 2024-10-08T19:18:53Z

 	a := require.New(fixtures.SynchronizedTest(t))

 	consensusParams := getDefaultStateProofConsensusParams()
+	consensusParams.Payouts = config.ProposerPayoutRules{} // TODO re-enable payouts when nodes aren't suspended


How are we tracking these?

I guess I should make an issue to address the "update this test once heartbeats are implemented" TODOs in this PR

jannotti

I think we will not have the top online cache before the first state proof, right? Maybe it would make sense to seed it during genesis (since the onlince accounts are listed out for us in the genesis file, I think). That could avoid special cases in the tests.

jannotti · 2024-10-09T18:00:41Z

-func (eval *BlockEvaluator) endOfBlock() error {
+// When generating a block, participating addresses are passed to prevent a
+// proposer from suspending itself.
+func (eval *BlockEvaluator) endOfBlock(participating ...basics.Address) error {


Why ...basics.Address instead of []basics.Address? I assume callers always have a slice, as opposed to call sites with, say, 5 explicit arguments.

it's true, this is just me optimizing for a smaller diff, to not change other endOfBlock callers, but the idea is to pass a slice — can change

jannotti · 2024-10-09T18:04:01Z

+		IncentiveEligible     bool // currently unused below, but may be needed in the future
+	}
+	candidates := make(map[basics.Address]candidateData)
+	partAddrs := util.MakeSet(participating...)


Do we do anything else with this slice? Maybe we should push the Set type up through the callers, so that it is built as a Set when it is first created to pass to endOfBlock?

It's used in GenerateBlock while making a map of end-of-block account state for participating addresses, to include in the UnfinishedBlock ... if we pushed it up to GenerateBlock then it could protect against looking up the same participating address twice, if duplicate addresses were passed to GenerateBlock.

gmalouf · 2024-10-15T18:27:42Z

+	if maxSuspensions > 0 {
+		knockOfflineCandidates, err := eval.state.knockOfflineCandidates()
+		if err != nil {
+			// Log an error and keep going; generating lists of absent and expired


So this implies some nodes can "choose" not to search for absent/expired accounts.

yes, when generating a block it is not required they put any accounts in the {Absent,Expired}ParticipationAccounts block headers, but if they are in the list, validation rules require that the accounts are actually absent or expired.

gmalouf · 2024-10-15T18:29:21Z

+
+	// Now, check these candidate accounts to see if they are expired or absent.
+	for accountAddr, acctData := range candidates {
+		if acctData.MicroAlgosWithRewards.IsZero() {


100% of time, zero balance implies being closed?

Yes, that's correct, my understanding is currently the only way you can have a zero balance at the end of the round is if your account has been closed.

gmalouf · 2024-10-15T18:30:42Z

+//
+// This function is passed a list of participating addresses so a node will not
+// propose a block that suspends or expires itself.
+func (eval *BlockEvaluator) generateKnockOfflineAccountsList(participating []basics.Address) {


participating is really "participating accounts excluding any I host"

here, the "participating" argument is the accounts that the node hosts.

gmalouf

I'm good in general, a few small comments.

gmalouf · 2024-10-15T18:33:29Z

-		vb := l.endBlock(t, blkEval)
+		vb := l.endBlock(t, blkEval, recvAddr)
 		blkEval = l.nextBlock(t)
+		//require.Empty(t, vb.Block().ExpiredParticipationAccounts)


why is this added commented out?

The test sets up a bunch of participating accounts that are separate from the ones that I'm interested in, and they do expire (before they didn't because they weren't noticed), but in a separate branch I was working on updating this

gmalouf · 2024-10-15T18:35:23Z

 	challenge := byte(0)
-	for i := uint64(0); i < uint64(1210); i++ { // A bit past one grace period (200) past challenge at 1000.
-		vb := l.endBlock(t, blkEval)
+	for i := uint64(0); i < uint64(1200); i++ { // Just before first suspension at 1171


Would this not go past first suspension - why 1200?

it's based on the values are set for certain accounts initializing LastHeartbeat/LastProposed earlier in the test

gmalouf · 2024-10-15T18:36:45Z

-	}
-
-	st := txn.Sign(keys[0])
-	err = eval.Transaction(st, transactions.ApplyData{})


Why remove all of these eval.Transaction calls?

you no longer need to send transactions to cause GenerateBlock/BlockEvaluator to "notice" an account is expired or not participating

gmalouf · 2024-10-15T18:41:51Z

+	}
+
+	// fetch fresh data up to this round from online account cache. These accounts should all
+	// be in cache, as long as proto.StateProofTopVoters < onlineAccountsCacheMaxSize.


This feels like a condition to call out in the consensus file.

added TestOnlineAccountsCacheSizeBiggerThanStateProofTopVoters

cce · 2024-10-30T18:24:14Z

Merging this into feature/heartbeats branch now to follow with two follow-up PRs for addressing PR feedback here:

branch of changes to address @jannotti's feedback "Maybe it would make sense to seed it during genesis (since the online accounts are listed out for us in the genesis file, I think). That could avoid special cases in the tests"
branch of changes to make BlockEvaluator.endOfBlock(participating) a non-optional argument. Updates tests to require Proposer is set "realistically" to a participating account that is not the fee sink or rewards pool address in test ledger implementations that generate blocks, like evalTestLedger.endBlock() and simple/DoubleLedger.

algorandskiy · 2024-10-30T21:04:30Z

 // key material deleted. If it is only suspended, the key material will remain.
-func (eval *BlockEvaluator) generateKnockOfflineAccountsList() {
+//
+// Different ndoes may propose different list of addresses based on node state.


ndoes -> nodes

algorandskiy · 2024-10-30T21:32:25Z

+			candidates[accountAddr] = candidateData{
+				VoteLastValid:         acctData.VoteLastValid,
+				VoteID:                acctData.VoteID,
+				Status:                basics.Online, // from lookupOnlineAccountData, which only returns online accounts


I guess need a test to enforce knockOfflineCandidates -> lookupOnlineAccountData control flow

algorandskiy · 2024-10-30T21:36:27Z

+	for addr := range voters.AddrToPos {
+		data, err := l.acctsOnline.lookupOnlineAccountData(rnd, addr)
+		if err != nil {
+			continue // skip missing / not online accounts


why would voters ever return non-online account?

the voters are only calculating Top N every 256 rounds, so if a lookup for the current round (for the cached addr from the last state proof interval) being requested is that the account was closed/deleted, you could hit an error here.

I should add a comment and write a test exercising this case, realizing it is kind of complicated now after writing it out

cce requested review from algorandskiy and jannotti July 26, 2024 17:25

algorandskiy added the New Feature label Jul 26, 2024

periodically track top N online accounts in Ledger, and use when buil…

21db44d

…ding AbsentParticipationAccounts

cce force-pushed the track-incentive-candidates branch from 975ddb4 to 21db44d Compare July 26, 2024 17:35

cce commented Jul 26, 2024

View reviewed changes

Comment thread ledger/ledgercore/onlineacct.go Outdated

run make msgp

e968515

algorandskiy reviewed Jul 26, 2024

View reviewed changes

jannotti reviewed Jul 31, 2024

View reviewed changes

Comment thread cmd/tealdbg/localLedger.go Outdated

Comment thread ledger/eval/eval.go Outdated

Comment thread ledger/eval/prefetcher/prefetcher_alignment_test.go Outdated

Comment thread ledger/toponline.go Outdated

cce added 2 commits August 9, 2024 10:15

update votersTracker

8587b28

CR fixes

38d4b8d

cce commented Aug 9, 2024

View reviewed changes

Comment thread ledger/eval/eval.go

cce added 2 commits August 9, 2024 10:58

rename and simplify

9740ddc

add LastProposed/LastHeartbeat to BaseOnlineAccountData handling

b8b9673

cce mentioned this pull request Aug 9, 2024

refactor: remove ledgercore.OnlineAccountData #5965

Merged

cce added 2 commits August 9, 2024 11:43

fill out LastHeartbeat/LastProposed from algorand#5965

0b7fbad

fix remaining tests

b2f1130

cce added 2 commits August 9, 2024 14:40

fix missing AccountData => OnlineAccountData transformation function …

5242990

…break TestAbsenteeChecks

Merge remote-tracking branch 'upstream/master' into track-incentive-c…

0baf81f

…andidates

jannotti reviewed Aug 20, 2024

View reviewed changes

Comment thread ledger/eval/eval.go

Comment thread ledger/eval/eval.go

Comment thread ledger/ledger.go Outdated

Update ledger/ledger.go

be464cf

Co-authored-by: John Jannotti <jannotti@gmail.com>

cce requested a review from algorandskiy August 21, 2024 15:22

cce commented Aug 21, 2024

View reviewed changes

Comment thread ledger/ledger.go Outdated

Update ledger/ledger.go

97d0bcf

algorandskiy reviewed Aug 21, 2024

View reviewed changes

Comment thread ledger/eval/eval.go

cce added 2 commits August 28, 2024 09:16

Merge remote-tracking branch 'upstream/master' into track-incentive-c…

55d5068

…andidates

update TestAbsenteeChecks

01b150a

cce force-pushed the track-incentive-candidates branch from f5b42d4 to 01b150a Compare September 18, 2024 15:26

cce added 2 commits September 18, 2024 13:37

also consider candidates for ExpiredParticipationAccounts

c24e809

update TestExpiredAccountGeneration

c252d95

gmalouf assigned cce Sep 18, 2024

cce added 6 commits September 19, 2024 13:37

Fix TestAbsentTracking

bb82a97

add TestLatestCompletedVotersUpTo

04c0a84

don't propose to suspend yourself, and update TestAbsenteeChallenges

c41a49a

Update TestExpiredAccountGeneration

e476730

update TestAbsenteeChecks for proposer change

9115aae

update TestTotalWeightChanges and update comment on generateKnockOffl…

23b9996

…ineAccountsList

cce marked this pull request as ready for review October 4, 2024 17:31

cce added 2 commits October 4, 2024 14:07

update TestEvalFunctionForExpiredAccounts

9bc39aa

update TestOnlineActModel*

c558d59

cce force-pushed the track-incentive-candidates branch from 1c4c898 to c558d59 Compare October 4, 2024 18:40

gmalouf requested review from algorandskiy and jannotti October 4, 2024 19:18

gmalouf reviewed Oct 8, 2024

View reviewed changes

jannotti reviewed Oct 9, 2024

View reviewed changes

gmalouf reviewed Oct 15, 2024

View reviewed changes

gmalouf approved these changes Oct 15, 2024

View reviewed changes

cce changed the base branch from master to feature/heartbeats October 30, 2024 14:39

cce force-pushed the feature/heartbeats branch from eff5fb4 to 8b6c443 Compare October 30, 2024 14:41

cce force-pushed the track-incentive-candidates branch from a843630 to c558d59 Compare October 30, 2024 14:43

add TestOnlineAccountsCacheSizeBiggerThanStateProofTopVoters

9d46fa6

cce merged commit 28338ff into algorand:feature/heartbeats Oct 30, 2024

cce deleted the track-incentive-candidates branch October 30, 2024 19:41

algorandskiy reviewed Oct 30, 2024

View reviewed changes

cce mentioned this pull request Oct 31, 2024

incentives: handle round 0 "top voters" lookups for easier testing #6159

Merged

Conversation

cce commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

Uh oh!

algorandskiy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cce commented Jul 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cce Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cce Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmalouf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cce Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cce commented Jul 26, 2024 •

edited

Loading

algorandskiy left a comment •

edited

Loading

cce commented Jul 27, 2024 •

edited

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading

cce Oct 10, 2024 •

edited

Loading

cce Oct 10, 2024 •

edited

Loading

cce Oct 30, 2024 •

edited

Loading

cce Oct 31, 2024 •

edited

Loading