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.

Sign up for GitHub

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
Merged

Eval: Prefetching for heartbeat transactions #6182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a0abc4
Prefetching for heartbeat transactions
jannotti Nov 27, 2024
521cf5f
Make the pending transaction endpoint notice heartbeats
jannotti Nov 27, 2024
41ca53d
Two small changes I had stashed
jannotti Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion daemon/algod/api/algod.oas2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@
},
"/v2/accounts/{address}": {
"get": {
"description": "Given a specific account public key, this call returns the accounts status, balance and spendable amounts",
"description": "Given a specific account public key, this call returns the account's status, balance and spendable amounts",
"tags": [
"public",
"nonparticipating"
Expand Down
2 changes: 1 addition & 1 deletion daemon/algod/api/algod.oas3.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2978,7 +2978,7 @@
},
"/v2/accounts/{address}": {
"get": {
"description": "Given a specific account public key, this call returns the accounts status, balance and spendable amounts",
"description": "Given a specific account public key, this call returns the account's status, balance and spendable amounts",
"operationId": "AccountInformation",
"parameters": [
{
Expand Down
188 changes: 94 additions & 94 deletions daemon/algod/api/server/v2/generated/nonparticipating/public/routes.go
View file
Open in desktop

Large diffs are not rendered by default.

16 changes: 15 additions & 1 deletion data/transactions/logic/eval_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3218,7 +3218,21 @@ func TestIllegalOp(t *testing.T) {
}
}

func TestShortProgram(t *testing.T) {
func TestShortSimple(t *testing.T) {
partitiontest.PartitionTest(t)

t.Parallel()
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
ops := testProg(t, `int 8; store 7`, v)
testLogicBytes(t, ops.Program[:len(ops.Program)-1], nil,
"program ends short of immediate values",
"program ends short of immediate values")
})
}
}

func TestShortBranch(t *testing.T) {
partitiontest.PartitionTest(t)

t.Parallel()
Expand Down
9 changes: 5 additions & 4 deletions data/transactions/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@

// MatchAddress checks if the transaction touches a given address.
func (tx Transaction) MatchAddress(addr basics.Address, spec SpecialAddresses) bool {
return slices.Contains(tx.RelevantAddrs(spec), addr)
return slices.Contains(tx.relevantAddrs(spec), addr)

Check warning on line 328 in data/transactions/transaction.go

View check run for this annotation

Codecov / codecov/patch

data/transactions/transaction.go#L328 

Added line #L328 was not covered by tests
}

var errKeyregTxnFirstVotingRoundGreaterThanLastVotingRound = errors.New("transaction first voting round need to be less than its last voting round")
Expand Down Expand Up @@ -714,9 +714,8 @@
return tx.LastValid
}

// RelevantAddrs returns the addresses whose balance records this transaction will need to access.
// The header's default is to return just the sender and the fee sink.
func (tx Transaction) RelevantAddrs(spec SpecialAddresses) []basics.Address {
// relevantAddrs returns the addresses whose balance records this transaction will need to access.
func (tx Transaction) relevantAddrs(spec SpecialAddresses) []basics.Address {

Check warning on line 718 in data/transactions/transaction.go

View check run for this annotation

Codecov / codecov/patch

data/transactions/transaction.go#L718 

Added line #L718 was not covered by tests
addrs := []basics.Address{tx.Sender, spec.FeeSink}

switch tx.Type {
Expand All @@ -733,6 +732,8 @@
if !tx.AssetTransferTxnFields.AssetSender.IsZero() {
addrs = append(addrs, tx.AssetTransferTxnFields.AssetSender)
}
case protocol.HeartbeatTx:
addrs = append(addrs, tx.HeartbeatTxnFields.HbAddress)

Check warning on line 736 in data/transactions/transaction.go

View check run for this annotation

Codecov / codecov/patch

data/transactions/transaction.go#L735-L736 

Added lines #L735 - L736 were not covered by tests
}

return addrs
Expand Down
4 changes: 4 additions & 0 deletions ledger/eval/eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1612,6 +1612,10 @@ func (eval *BlockEvaluator) recordProposal() error {
return nil
}

// proposerPayout determines how much the proposer should be paid, assuming it
// gets paid at all. It may not examine the actual proposer because it is
// called before the proposer is known. Agreement might zero out this value
// when the actual proposer is decided, if that proposer is ineligible.
func (eval *BlockEvaluator) proposerPayout() (basics.MicroAlgos, error) {
incentive, _ := basics.NewPercent(eval.proto.Payouts.Percent).DivvyAlgos(eval.block.FeesCollected)
total, o := basics.OAddA(incentive, eval.block.Bonus)
Expand Down
4 changes: 3 additions & 1 deletion ledger/eval/prefetcher/prefetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,9 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) {
// since they might be non-used arbitrary values

case protocol.StateProofTx:
case protocol.KeyRegistrationTx:
case protocol.KeyRegistrationTx: // No extra accounts besides the sender
case protocol.HeartbeatTx:
loadAccountsAddAccountTask(&stxn.Txn.HbAddress, task, accountTasks, queue)
}

// If you add new addresses here, also add them in getTxnAddresses().
Expand Down
52 changes: 52 additions & 0 deletions ledger/eval/prefetcher/prefetcher_alignment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/algorand/go-algorand/crypto/stateproof"
"github.com/algorand/go-algorand/data/basics"
"github.com/algorand/go-algorand/data/bookkeeping"
"github.com/algorand/go-algorand/data/committee"
"github.com/algorand/go-algorand/data/stateproofmsg"
"github.com/algorand/go-algorand/data/transactions"
"github.com/algorand/go-algorand/ledger/eval"
Expand Down Expand Up @@ -1422,3 +1423,54 @@ func TestEvaluatorPrefetcherAlignmentStateProof(t *testing.T) {
prefetched.pretend(rewardsPool())
require.Equal(t, requested, prefetched)
}

func TestEvaluatorPrefetcherAlignmentHeartbeat(t *testing.T) {
partitiontest.PartitionTest(t)

// We need valid part keys to evaluate the Heartbeat.
const kd = 10
firstID := basics.OneTimeIDForRound(0, kd)
otss := crypto.GenerateOneTimeSignatureSecrets(firstID.Batch, 5)

l := &prefetcherAlignmentTestLedger{
balances: map[basics.Address]ledgercore.AccountData{
rewardsPool(): {
AccountBaseData: ledgercore.AccountBaseData{
MicroAlgos: basics.MicroAlgos{Raw: 1234567890},
},
},
makeAddress(1): {
AccountBaseData: ledgercore.AccountBaseData{
MicroAlgos: basics.MicroAlgos{Raw: 1000001},
},
},
makeAddress(2): {
AccountBaseData: ledgercore.AccountBaseData{
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!

},
},
},
}

txn := transactions.Transaction{
Type: protocol.HeartbeatTx,
Header: transactions.Header{
Sender: makeAddress(1),
GenesisHash: genesisHash(),
Fee: basics.Algos(1), // Heartbeat txn is unusual in that it checks fees a bit.
},
HeartbeatTxnFields: transactions.HeartbeatTxnFields{
HbAddress: makeAddress(2),
HbProof: otss.Sign(firstID, committee.Seed(genesisHash())).ToHeartbeatProof(),
HbSeed: committee.Seed(genesisHash()),
},
}

requested, prefetched := run(t, l, txn)

prefetched.pretend(rewardsPool())
require.Equal(t, requested, prefetched)
}
Loading