Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eval: Prefetching for heartbeat transactions #6182

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

jannotti
Copy link
Contributor

Add prefetching for the HbAddress of a Heartbeat transaction.

Where else might we need to notice that there's a new way for an account to appear in the new heartbeat txn type?

@jannotti jannotti changed the title Prefetching for heartbeat transactions Eval: Prefetching for heartbeat transactions Nov 27, 2024
@jannotti jannotti self-assigned this Nov 27, 2024
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 56.38%. Comparing base (de1c241) to head (41ca53d).
Report is 1 commits behind head on feature/heartbeats.

Files with missing lines Patch % Lines
data/transactions/transaction.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           feature/heartbeats    #6182      +/-   ##
======================================================
+ Coverage               56.35%   56.38%   +0.02%     
======================================================
  Files                     497      497              
  Lines                   70231    70235       +4     
======================================================
+ Hits                    39579    39600      +21     
+ Misses                  27949    27934      -15     
+ Partials                 2703     2701       -2     

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

@jannotti jannotti requested a review from gmalouf November 27, 2024 19:30
@gmalouf gmalouf requested a review from cce December 2, 2024 15:45
MicroAlgos: basics.MicroAlgos{Raw: 100_000},
},
VotingData: basics.VotingData{
VoteID: otss.OneTimeSignatureVerifier,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused but then realized you only need a single votingdata field set to validate a heartbeat txn — not even votefirstvalid / votelastvalid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you have a comment in apply/heartbeat.go to discuss why you are checking the current partkey, but maybe the current partkey should still be valid within some check on current round against VoteFirst/LastValid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true right now. Let me think through whether it's correct.

First, heartbeat always uses the current VoteID, not one from 320 rounds ago. That seemed right to me, but I have to look more closely at how the accountManager.Keys() method works. I need to ensure that if the service wants to heartbeat in round 10,000 if gets the current key, not the one for 9680.

If I keyreg and change my keys in round 10,000, in round 10,100 agreement will still be using my old keys, but I'm saying heartbeat should use new. Weird, but is that more or less weird than using my old keys in apply.Heartbeat? Usually, we don't lookback 320 rounds in apply.

I'm beginning to lean toward changing to use the old keys. In that case, it's ok to add the VoteFirst and VoteLast checks, I think, though it should not matter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also wondering if you are in the "expired, but still online" state, can you keep sending free heartbeats as much as you want. though we will have auto-population of the ExpiredParticipationAccounts header too soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're concerned that if we don't check VoteFirst/VoteLast that we could get an expired account keeping himself online while his keys are already expired? I suppose that's true. I think I'm leaning toward changing the keys that are used during apply.Heartbeat for more consistency, and adding the VoteFirst/VoteLast checks. They won't be a problem if we're using the old keys.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! eval has access to 320 lookback keys for the voting/online stake, as well as for generating Expired/AbsentParticipationAccounts lists, so adding it to apply doesn't seem that bad. Also having the heartbeat service and agreement service in parity on what keys to grab from the accountmanager seems good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make that change in it's own PR, it'll probably take a while to get tests operating properly again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, SGTM!

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prefetcher update looks correct to me.

@gmalouf gmalouf requested a review from cce December 3, 2024 16:19
@jannotti jannotti merged commit d2954e5 into algorand:feature/heartbeats Dec 3, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants