From a2902e871971d33bbb372a153bfa92111ad4123f Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Fri, 8 Jul 2022 20:10:21 -0400 Subject: [PATCH 1/8] wip: do not break prefetching on error --- ledger/internal/eval.go | 70 ++++---- ledger/internal/prefetcher/prefetcher.go | 164 +++++++++++------- .../prefetcher/prefetcher_alignment_test.go | 21 +-- ledger/internal/prefetcher/prefetcher_test.go | 122 ++++++++----- 4 files changed, 229 insertions(+), 148 deletions(-) diff --git a/ledger/internal/eval.go b/ledger/internal/eval.go index 1944c4a69a..6891bc2c65 100644 --- a/ledger/internal/eval.go +++ b/ledger/internal/eval.go @@ -1549,45 +1549,47 @@ transactionGroupLoop: if !ok { break transactionGroupLoop } else if txgroup.Err != nil { - return ledgercore.StateDelta{}, txgroup.Err + logging.Base().Errorf("evaluation error: %w", txgroup.Err) } - for _, br := range txgroup.Accounts { - if _, have := base.accounts[*br.Address]; !have { - base.accounts[*br.Address] = *br.Data - } - } - for _, lr := range txgroup.Resources { - if lr.Address == nil { - // we attempted to look for the creator, and failed. - base.creators[creatable{cindex: lr.CreatableIndex, ctype: lr.CreatableType}] = - foundAddress{exists: false} - continue - } - if lr.CreatableType == basics.AssetCreatable { - if lr.Resource.AssetHolding != nil { - base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true} - } else { - base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{exists: false} + if txgroup.Err == nil { + for _, br := range txgroup.Accounts { + if _, have := base.accounts[*br.Address]; !have { + base.accounts[*br.Address] = *br.Data } - if lr.Resource.AssetParams != nil { - base.assetParams[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetParams{value: *lr.Resource.AssetParams, exists: true} - base.creators[creatable{cindex: lr.CreatableIndex, ctype: basics.AssetCreatable}] = foundAddress{address: *lr.Address, exists: true} - } else { - base.assetParams[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetParams{exists: false} - - } - } else { - if lr.Resource.AppLocalState != nil { - base.appLocalStates[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppLocalState{value: *lr.Resource.AppLocalState, exists: true} - } else { - base.appLocalStates[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppLocalState{exists: false} + } + for _, lr := range txgroup.Resources { + if lr.Address == nil { + // we attempted to look for the creator, and failed. + base.creators[creatable{cindex: lr.CreatableIndex, ctype: lr.CreatableType}] = + foundAddress{exists: false} + continue } - if lr.Resource.AppParams != nil { - base.appParams[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppParams{value: *lr.Resource.AppParams, exists: true} - base.creators[creatable{cindex: lr.CreatableIndex, ctype: basics.AppCreatable}] = foundAddress{address: *lr.Address, exists: true} + if lr.CreatableType == basics.AssetCreatable { + if lr.Resource.AssetHolding != nil { + base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true} + } else { + base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{exists: false} + } + if lr.Resource.AssetParams != nil { + base.assetParams[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetParams{value: *lr.Resource.AssetParams, exists: true} + base.creators[creatable{cindex: lr.CreatableIndex, ctype: basics.AssetCreatable}] = foundAddress{address: *lr.Address, exists: true} + } else { + base.assetParams[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetParams{exists: false} + + } } else { - base.appParams[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppParams{exists: false} + if lr.Resource.AppLocalState != nil { + base.appLocalStates[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppLocalState{value: *lr.Resource.AppLocalState, exists: true} + } else { + base.appLocalStates[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppLocalState{exists: false} + } + if lr.Resource.AppParams != nil { + base.appParams[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppParams{value: *lr.Resource.AppParams, exists: true} + base.creators[creatable{cindex: lr.CreatableIndex, ctype: basics.AppCreatable}] = foundAddress{address: *lr.Address, exists: true} + } else { + base.appParams[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppParams{exists: false} + } } } } diff --git a/ledger/internal/prefetcher/prefetcher.go b/ledger/internal/prefetcher/prefetcher.go index 82e0d830c1..44c46e0bac 100644 --- a/ledger/internal/prefetcher/prefetcher.go +++ b/ledger/internal/prefetcher/prefetcher.go @@ -76,7 +76,7 @@ type LoadedTransactionGroup struct { type accountPrefetcher struct { ledger Ledger rnd basics.Round - groups [][]transactions.SignedTxnWithAD + txnGroups [][]transactions.SignedTxnWithAD feeSinkAddr basics.Address consensusParams config.ConsensusParams outChan chan LoadedTransactionGroup @@ -84,14 +84,14 @@ type accountPrefetcher struct { // PrefetchAccounts loads the account data for the provided transaction group list. It also loads the feeSink account and add it to the first returned transaction group. // The order of the transaction groups returned by the channel is identical to the one in the input array. -func PrefetchAccounts(ctx context.Context, l Ledger, rnd basics.Round, groups [][]transactions.SignedTxnWithAD, feeSinkAddr basics.Address, consensusParams config.ConsensusParams) <-chan LoadedTransactionGroup { +func PrefetchAccounts(ctx context.Context, l Ledger, rnd basics.Round, txnGroups [][]transactions.SignedTxnWithAD, feeSinkAddr basics.Address, consensusParams config.ConsensusParams) <-chan LoadedTransactionGroup { prefetcher := &accountPrefetcher{ ledger: l, rnd: rnd, - groups: groups, + txnGroups: txnGroups, feeSinkAddr: feeSinkAddr, consensusParams: consensusParams, - outChan: make(chan LoadedTransactionGroup, len(groups)), + outChan: make(chan LoadedTransactionGroup, len(txnGroups)), } go prefetcher.prefetch(ctx) @@ -117,6 +117,9 @@ type groupTask struct { resources []LoadedResourcesEntry // resourcesCount is the number of resources that nees to be loaded per transaction group resourcesCount int + + // error while processing this group task + err *GroupTaskError } // preloaderTask manage the loading of a single element, whether it's a resource or an account address. @@ -128,9 +131,9 @@ type preloaderTask struct { // resource type creatableType basics.CreatableType // a list of transaction group tasks that depends on this address or resource - groups []*groupTask + groupTasks []*groupTask // a list of indices into the groupTask.balances or groupTask.resources where the address would be stored - groupIndices []int + groupTasksIndices []int } // preloaderTaskQueue is a dynamic linked list of enqueued entries, optimized for non-syncronized insertion and @@ -198,18 +201,18 @@ func loadAccountsAddAccountTask(addr *basics.Address, wt *groupTask, accountTask } if task, have := accountTasks[*addr]; !have { task := &preloaderTask{ - address: addr, - groups: make([]*groupTask, 1, 4), - groupIndices: make([]int, 1, 4), + address: addr, + groupTasks: make([]*groupTask, 1, 4), + groupTasksIndices: make([]int, 1, 4), } - task.groups[0] = wt - task.groupIndices[0] = wt.balancesCount + task.groupTasks[0] = wt + task.groupTasksIndices[0] = wt.balancesCount accountTasks[*addr] = task queue.enqueue(task) } else { - task.groups = append(task.groups, wt) - task.groupIndices = append(task.groupIndices, wt.balancesCount) + task.groupTasks = append(task.groupTasks, wt) + task.groupTasksIndices = append(task.groupTasksIndices, wt.balancesCount) } wt.balancesCount++ } @@ -226,20 +229,20 @@ func loadAccountsAddResourceTask(addr *basics.Address, cidx basics.CreatableInde } if task, have := resourceTasks[key]; !have { task := &preloaderTask{ - address: addr, - groups: make([]*groupTask, 1, 4), - groupIndices: make([]int, 1, 4), - creatableIndex: cidx, - creatableType: ctype, + address: addr, + groupTasks: make([]*groupTask, 1, 4), + groupTasksIndices: make([]int, 1, 4), + creatableIndex: cidx, + creatableType: ctype, } - task.groups[0] = wt - task.groupIndices[0] = wt.resourcesCount + task.groupTasks[0] = wt + task.groupTasksIndices[0] = wt.resourcesCount resourceTasks[key] = task queue.enqueue(task) } else { - task.groups = append(task.groups, wt) - task.groupIndices = append(task.groupIndices, wt.resourcesCount) + task.groupTasks = append(task.groupTasks, wt) + task.groupTasksIndices = append(task.groupTasksIndices, wt.resourcesCount) } wt.resourcesCount++ } @@ -250,6 +253,7 @@ func loadAccountsAddResourceTask(addr *basics.Address, cidx basics.CreatableInde func (p *accountPrefetcher) prefetch(ctx context.Context) { defer close(p.outChan) accountTasks := make(map[basics.Address]*preloaderTask) + resourceTasks := make(map[accountCreatableKey]*preloaderTask) var maxTxnGroupEntries int if p.consensusParams.Application { @@ -260,21 +264,21 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { maxTxnGroupEntries = p.consensusParams.MaxTxGroupSize * 8 } - tasksQueue := allocPreloaderQueue(len(p.groups), maxTxnGroupEntries) + tasksQueue := allocPreloaderQueue(len(p.txnGroups), maxTxnGroupEntries) // totalBalances counts the total number of balances over all the transaction groups totalBalances := 0 totalResources := 0 - groupsReady := make([]groupTask, len(p.groups)) + groupsReady := make([]groupTask, len(p.txnGroups)) // Add fee sink to the first group - if len(p.groups) > 0 { + if len(p.txnGroups) > 0 { // the feeSinkAddr is known to be non-empty feeSinkPreloader := &preloaderTask{ - address: &p.feeSinkAddr, - groups: []*groupTask{&groupsReady[0]}, - groupIndices: []int{0}, + address: &p.feeSinkAddr, + groupTasks: []*groupTask{&groupsReady[0]}, + groupTasksIndices: []int{0}, } groupsReady[0].balancesCount = 1 accountTasks[p.feeSinkAddr] = feeSinkPreloader @@ -283,21 +287,60 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { // iterate over the transaction groups and add all their account addresses to the list queue := &tasksQueue - for i := range p.groups { + for i := range p.txnGroups { task := &groupsReady[i] - for j := range p.groups[i] { - stxn := &p.groups[i][j] + for j := range p.txnGroups[i] { + stxn := &p.txnGroups[i][j] switch stxn.Txn.Type { case protocol.PaymentTx: loadAccountsAddAccountTask(&stxn.Txn.Receiver, task, accountTasks, queue) loadAccountsAddAccountTask(&stxn.Txn.CloseRemainderTo, task, accountTasks, queue) case protocol.AssetConfigTx: + loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.ConfigAsset), basics.AssetCreatable, task, resourceTasks, queue) case protocol.AssetTransferTx: + if !stxn.Txn.AssetSender.IsZero() { + loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + loadAccountsAddResourceTask(&stxn.Txn.AssetSender, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + } else { + loadAccountsAddResourceTask(&stxn.Txn.Sender, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + if stxn.Txn.AssetAmount == 0 && (stxn.Txn.AssetReceiver == stxn.Txn.Sender) { + // opt in + loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + } + } + if !stxn.Txn.AssetReceiver.IsZero() { + loadAccountsAddResourceTask(&stxn.Txn.AssetReceiver, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + } + if !stxn.Txn.AssetCloseTo.IsZero() { + loadAccountsAddResourceTask(&stxn.Txn.AssetCloseTo, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + } case protocol.AssetFreezeTx: + if !stxn.Txn.FreezeAccount.IsZero() { + loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.FreezeAsset), basics.AssetCreatable, task, resourceTasks, queue) + loadAccountsAddResourceTask(&stxn.Txn.FreezeAccount, basics.CreatableIndex(stxn.Txn.FreezeAsset), basics.AssetCreatable, task, resourceTasks, queue) + loadAccountsAddAccountTask(&stxn.Txn.FreezeAccount, task, accountTasks, queue) + } case protocol.ApplicationCallTx: + if stxn.Txn.ApplicationID != 0 { + // load the global - so that we'll have the program + loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.ApplicationID), basics.AppCreatable, task, resourceTasks, queue) + // load the local - so that we'll have the local state + // TODO: this is something we need to decide if we want to enable, since not + // every application call would use local storage. + if (stxn.Txn.ApplicationCallTxnFields.OnCompletion == transactions.OptInOC) || + (stxn.Txn.ApplicationCallTxnFields.OnCompletion == transactions.CloseOutOC) || + (stxn.Txn.ApplicationCallTxnFields.OnCompletion == transactions.ClearStateOC) { + loadAccountsAddResourceTask(&stxn.Txn.Sender, basics.CreatableIndex(stxn.Txn.ApplicationID), basics.AppCreatable, task, resourceTasks, queue) + } + } + + // do not preload Txn.ForeignApps, Txn.ForeignAssets, Txn.Accounts + // since they might be non-used arbitrary values + case protocol.StateProofTx: case protocol.KeyRegistrationTx: } + // If you add new addresses here, also add them in getTxnAddresses(). if !stxn.Txn.Sender.IsZero() { loadAccountsAddAccountTask(&stxn.Txn.Sender, task, accountTasks, queue) @@ -356,24 +399,20 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { // iterate on the transaction groups tasks. This array retains the original order. completed := make(map[int64]bool) - for i := int64(0); i < int64(len(p.groups)); { + for i := int64(0); i < int64(len(p.txnGroups)); { wait: incompleteCount := atomic.LoadInt64(&groupsReady[i].incompleteCount) if incompleteCount > 0 || (incompleteCount != dependencyFreeGroup && !completed[i]) { select { case done := <-groupDoneCh: if done.err != nil { - // if there is an error, report the error to the output channel. - p.outChan <- LoadedTransactionGroup{ - Err: &GroupTaskError{ - err: done.err, - GroupIdx: done.groupIdx, - Address: done.task.address, - CreatableIndex: done.task.creatableIndex, - CreatableType: done.task.creatableType, - }, + groupsReady[done.groupIdx].err = &GroupTaskError{ + err: done.err, + GroupIdx: done.groupIdx, + Address: done.task.address, + CreatableIndex: done.task.creatableIndex, + CreatableType: done.task.creatableType, } - return } if done.groupIdx > i { // mark future txn as ready. @@ -388,7 +427,7 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { } } next := i - for ; next < int64(len(p.groups)); next++ { + for ; next < int64(len(p.txnGroups)); next++ { if !completed[next] { if next > i { i = next @@ -402,7 +441,8 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { // if we had no error, write the result to the output channel. // this write will not block since we preallocated enough space on the channel. p.outChan <- LoadedTransactionGroup{ - TxnGroup: p.groups[next], + Err: groupsReady[next].err, + TxnGroup: p.txnGroups[next], Accounts: groupsReady[next].balances, Resources: groupsReady[next].resources, } @@ -460,15 +500,19 @@ func (p *accountPrefetcher) asyncPrefetchRoutine(queue *preloaderTaskQueue, task // if there was an error.. if err != nil { // there was an error loading that entry. - break + for _, wt := range task.groupTasks { + // notify the channel of the error. + wt.markCompletionAcctError(err, task, groupDoneCh) + } + continue } br := LoadedAccountDataEntry{ Address: task.address, Data: &acctData, } // update all the group tasks with the new acquired balance. - for i, wt := range task.groups { - wt.markCompletionAcct(task.groupIndices[i], br, groupDoneCh) + for i, wt := range task.groupTasks { + wt.markCompletionAcct(task.groupTasksIndices[i], br, groupDoneCh) } continue } @@ -479,7 +523,11 @@ func (p *accountPrefetcher) asyncPrefetchRoutine(queue *preloaderTaskQueue, task creator, ok, err = p.ledger.GetCreatorForRound(p.rnd, task.creatableIndex, task.creatableType) if err != nil { // there was an error loading that entry. - break + for _, wt := range task.groupTasks { + // notify the channel of the error. + wt.markCompletionAcctError(err, task, groupDoneCh) + } + continue } if !ok { re := LoadedResourcesEntry{ @@ -487,8 +535,8 @@ func (p *accountPrefetcher) asyncPrefetchRoutine(queue *preloaderTaskQueue, task CreatableType: task.creatableType, } // update all the group tasks with the new acquired balance. - for i, wt := range task.groups { - wt.markCompletionResource(task.groupIndices[i], re, groupDoneCh) + for i, wt := range task.groupTasks { + wt.markCompletionResource(task.groupTasksIndices[i], re, groupDoneCh) } continue } @@ -508,7 +556,11 @@ func (p *accountPrefetcher) asyncPrefetchRoutine(queue *preloaderTaskQueue, task } if err != nil { // there was an error loading that entry. - break + for _, wt := range task.groupTasks { + // notify the channel of the error. + wt.markCompletionAcctError(err, task, groupDoneCh) + } + continue } re := LoadedResourcesEntry{ Resource: &resource, @@ -517,14 +569,8 @@ func (p *accountPrefetcher) asyncPrefetchRoutine(queue *preloaderTaskQueue, task CreatableType: task.creatableType, } // update all the group tasks with the new acquired balance. - for i, wt := range task.groups { - wt.markCompletionResource(task.groupIndices[i], re, groupDoneCh) + for i, wt := range task.groupTasks { + wt.markCompletionResource(task.groupTasksIndices[i], re, groupDoneCh) } } - // if we got here, it means that there was an error. - // in every case we get here, the task is gurenteed to be a non-nil. - for _, wt := range task.groups { - // notify the channel of the error. - wt.markCompletionAcctError(err, task, groupDoneCh) - } } diff --git a/ledger/internal/prefetcher/prefetcher_alignment_test.go b/ledger/internal/prefetcher/prefetcher_alignment_test.go index 05a672d325..5d21ced192 100644 --- a/ledger/internal/prefetcher/prefetcher_alignment_test.go +++ b/ledger/internal/prefetcher/prefetcher_alignment_test.go @@ -1116,10 +1116,9 @@ func TestEvaluatorPrefetcherAlignmentApplicationCallAccountsDeclaration(t *testi requested, prefetched := run(t, l, txn) prefetched.Accounts[rewardsPool()] = struct{}{} - // Loading accounts depends on the smart contract program. Ignore the addresses - // not requested. - requested.Accounts[makeAddress(5)] = struct{}{} - requested.Accounts[makeAddress(3)] = struct{}{} + // Foreign accounts are not loaded, ensure they are not prefetched + require.NotContains(t, prefetched.Accounts, makeAddress(5)) + require.NotContains(t, prefetched.Accounts, makeAddress(3)) require.Equal(t, requested, prefetched) } @@ -1185,10 +1184,9 @@ func TestEvaluatorPrefetcherAlignmentApplicationCallForeignAppsDeclaration(t *te requested, prefetched := run(t, l, txn) prefetched.Accounts[rewardsPool()] = struct{}{} - // Loading foreign apps depends on the smart contract program. Ignore the apps - // not requested. - requested.Creators[creatable{cindex: 6, ctype: basics.AppCreatable}] = struct{}{} - requested.Creators[creatable{cindex: 8, ctype: basics.AppCreatable}] = struct{}{} + // Foreign apps are not loaded, ensure they are not prefetched + require.NotContains(t, prefetched.Creators, creatable{cindex: 6, ctype: basics.AppCreatable}) + require.NotContains(t, prefetched.Creators, creatable{cindex: 8, ctype: basics.AppCreatable}) require.Equal(t, requested, prefetched) } @@ -1254,10 +1252,9 @@ func TestEvaluatorPrefetcherAlignmentApplicationCallForeignAssetsDeclaration(t * requested, prefetched := run(t, l, txn) prefetched.Accounts[rewardsPool()] = struct{}{} - // Loading foreign assets depends on the smart contract program. Ignore the assets - // not requested. - requested.Creators[creatable{cindex: 6, ctype: basics.AssetCreatable}] = struct{}{} - requested.Creators[creatable{cindex: 8, ctype: basics.AssetCreatable}] = struct{}{} + // Foreign apps are not loaded, ensure they are not prefetched + require.NotContains(t, prefetched.Creators, creatable{cindex: 6, ctype: basics.AssetCreatable}) + require.NotContains(t, prefetched.Creators, creatable{cindex: 8, ctype: basics.AssetCreatable}) require.Equal(t, requested, prefetched) } diff --git a/ledger/internal/prefetcher/prefetcher_test.go b/ledger/internal/prefetcher/prefetcher_test.go index 40fe6949be..28b7c6d558 100644 --- a/ledger/internal/prefetcher/prefetcher_test.go +++ b/ledger/internal/prefetcher/prefetcher_test.go @@ -471,38 +471,42 @@ func TestEvaluatorPrefetcher(t *testing.T) { AccountBaseData: ledgercore.AccountBaseData{MicroAlgos: basics.MicroAlgos{Raw: 100000000}}, }, }, - { - Address: makeAddressPtr(4), - Data: &ledgercore.AccountData{ - AccountBaseData: ledgercore.AccountBaseData{MicroAlgos: basics.MicroAlgos{Raw: 0}}, + /* + { + Address: makeAddressPtr(4), + Data: &ledgercore.AccountData{ + AccountBaseData: ledgercore.AccountBaseData{MicroAlgos: basics.MicroAlgos{Raw: 0}}, + }, }, - }, - { - Address: makeAddressPtr(5), - Data: &ledgercore.AccountData{ - AccountBaseData: ledgercore.AccountBaseData{MicroAlgos: basics.MicroAlgos{Raw: 0}}, + { + Address: makeAddressPtr(5), + Data: &ledgercore.AccountData{ + AccountBaseData: ledgercore.AccountBaseData{MicroAlgos: basics.MicroAlgos{Raw: 0}}, + }, }, - }, + */ }, resources: []prefetcher.LoadedResourcesEntry{ - { - Address: makeAddressPtr(2), - CreatableIndex: 1001, - CreatableType: basics.AssetCreatable, - Resource: &ledgercore.AccountResource{}, - }, - { - Address: makeAddressPtr(15), - CreatableIndex: 2001, - CreatableType: basics.AppCreatable, - Resource: &ledgercore.AccountResource{}, - }, - { - Address: nil, - CreatableIndex: 2002, - CreatableType: basics.AppCreatable, - Resource: nil, - }, + /* + { + Address: makeAddressPtr(2), + CreatableIndex: 1001, + CreatableType: basics.AssetCreatable, + Resource: &ledgercore.AccountResource{}, + }, + { + Address: makeAddressPtr(15), + CreatableIndex: 2001, + CreatableType: basics.AppCreatable, + Resource: &ledgercore.AccountResource{}, + }, + { + Address: nil, + CreatableIndex: 2002, + CreatableType: basics.AppCreatable, + Resource: nil, + }, + */ /* - if we'll decide that we want to perfetch the account local state, then this should be enabled. { address: acctAddrPtr(1), @@ -568,10 +572,12 @@ func TestAssetLookupError(t *testing.T) { } errorReceived := false - groups := make([][]transactions.SignedTxnWithAD, 5) - for i := 0; i < 5; i++ { - groups[i] = make([]transactions.SignedTxnWithAD, 2) - for j := 0; j < 2; j++ { + const numGroups = 5 + const txnPerGroup = 2 + groups := make([][]transactions.SignedTxnWithAD, numGroups) + for i := 0; i < numGroups; i++ { + groups[i] = make([]transactions.SignedTxnWithAD, txnPerGroup) + for j := 0; j < txnPerGroup; j++ { groups[i][j].SignedTxn = assetTransferTxn if i == 2 { // force error in asset lookup in the second txn group only @@ -579,8 +585,12 @@ func TestAssetLookupError(t *testing.T) { } } } + preloadedTxnGroupsCh := prefetcher.PrefetchAccounts(context.Background(), ledger, rnd+100, groups, feeSinkAddr, config.Consensus[proto]) + + receivedNumGroups := 0 for loadedTxnGroup := range preloadedTxnGroupsCh { + receivedNumGroups++ if loadedTxnGroup.Err != nil { errorReceived = true require.Equal(t, int64(2), loadedTxnGroup.Err.GroupIdx) @@ -589,8 +599,10 @@ func TestAssetLookupError(t *testing.T) { require.Equal(t, errorTriggerAssetIndex, int(loadedTxnGroup.Err.CreatableIndex)) require.Equal(t, basics.AssetCreatable, loadedTxnGroup.Err.CreatableType) } + require.Equal(t, txnPerGroup, len(loadedTxnGroup.TxnGroup)) } require.True(t, errorReceived) + require.Equal(t, numGroups, receivedNumGroups) } // Test for error from GetCreatorForRound @@ -610,23 +622,33 @@ func TestGetCreatorForRoundError(t *testing.T) { Sender: makeAddress(1), }, AssetConfigTxnFields: transactions.AssetConfigTxnFields{ - ConfigAsset: errorTriggerCreatableIndex, + ConfigAsset: 101, }, }, } + createAssetFailedTxn := createAssetTxn + createAssetFailedTxn.Txn.ConfigAsset = errorTriggerCreatableIndex errorReceived := false - groups := make([][]transactions.SignedTxnWithAD, 5) - for i := 0; i < 5; i++ { - groups[i] = make([]transactions.SignedTxnWithAD, 10) - for j := 0; j < 10; j++ { + const numGroups = 5 + const txnPerGroup = 10 + groups := make([][]transactions.SignedTxnWithAD, numGroups) + for i := 0; i < numGroups; i++ { + groups[i] = make([]transactions.SignedTxnWithAD, txnPerGroup) + for j := 0; j < txnPerGroup; j++ { groups[i][j].SignedTxn = createAssetTxn + // fail only the first txn in the first group + if i == 0 && j == 0 { + groups[i][j].SignedTxn = createAssetFailedTxn + } } } preloadedTxnGroupsCh := prefetcher.PrefetchAccounts(context.Background(), ledger, rnd+100, groups, feeSinkAddr, config.Consensus[proto]) + receivedNumGroups := 0 for loadedTxnGroup := range preloadedTxnGroupsCh { + receivedNumGroups++ if loadedTxnGroup.Err != nil { errorReceived = true require.True(t, errors.Is(loadedTxnGroup.Err, getCreatorError{})) @@ -634,8 +656,10 @@ func TestGetCreatorForRoundError(t *testing.T) { require.Equal(t, errorTriggerCreatableIndex, int(loadedTxnGroup.Err.CreatableIndex)) require.Equal(t, basics.AssetCreatable, loadedTxnGroup.Err.CreatableType) } + require.Equal(t, txnPerGroup, len(loadedTxnGroup.TxnGroup)) } require.True(t, errorReceived) + require.Equal(t, numGroups, receivedNumGroups) } // Test for error from LookupWithoutRewards @@ -658,29 +682,41 @@ func TestLookupWithoutRewards(t *testing.T) { }, }, } + createAssetFailedTxn := createAssetTxn + createAssetFailedTxn.Txn.Sender = makeAddress(10) errorReceived := false - groups := make([][]transactions.SignedTxnWithAD, 5) - for i := 0; i < 5; i++ { - groups[i] = make([]transactions.SignedTxnWithAD, 10) - for j := 0; j < 10; j++ { + const numGroups = 5 + const txnPerGroup = 10 + groups := make([][]transactions.SignedTxnWithAD, numGroups) + for i := 0; i < numGroups; i++ { + groups[i] = make([]transactions.SignedTxnWithAD, txnPerGroup) + for j := 0; j < txnPerGroup; j++ { groups[i][j].SignedTxn = createAssetTxn + // fail only last txn in the first group + if i == 0 && j == txnPerGroup-1 { + groups[i][j].SignedTxn = createAssetFailedTxn + } } } - ledger.errorTriggerAddress[createAssetTxn.Txn.Sender] = true + ledger.errorTriggerAddress[createAssetFailedTxn.Txn.Sender] = true preloadedTxnGroupsCh := prefetcher.PrefetchAccounts(context.Background(), ledger, rnd+100, groups, feeSinkAddr, config.Consensus[proto]) + receivedNumGroups := 0 for loadedTxnGroup := range preloadedTxnGroupsCh { + receivedNumGroups++ if loadedTxnGroup.Err != nil { errorReceived = true require.True(t, errors.Is(loadedTxnGroup.Err, lookupError{})) - require.Equal(t, makeAddress(1), *loadedTxnGroup.Err.Address) + require.Equal(t, makeAddress(10), *loadedTxnGroup.Err.Address) require.Equal(t, 0, int(loadedTxnGroup.Err.CreatableIndex)) require.Equal(t, basics.AssetCreatable, loadedTxnGroup.Err.CreatableType) } + require.Equal(t, txnPerGroup, len(loadedTxnGroup.TxnGroup)) } require.True(t, errorReceived) + require.Equal(t, numGroups, receivedNumGroups) } func TestEvaluatorPrefetcherQueueExpansion(t *testing.T) { From d050b610349700d28f8ee7cf7c2808a3eff47ed6 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Fri, 8 Jul 2022 21:17:54 -0400 Subject: [PATCH 2/8] Add a test for a app transaction with the offending foreign asset field --- .../prefetcher/prefetcher_alignment_test.go | 12 +- ledger/internal/prefetcher/prefetcher_test.go | 2 - .../features/transactions/accountv2_test.go | 315 ++++++++++++++++++ 3 files changed, 316 insertions(+), 13 deletions(-) diff --git a/ledger/internal/prefetcher/prefetcher_alignment_test.go b/ledger/internal/prefetcher/prefetcher_alignment_test.go index 5d21ced192..bf77af27cd 100644 --- a/ledger/internal/prefetcher/prefetcher_alignment_test.go +++ b/ledger/internal/prefetcher/prefetcher_alignment_test.go @@ -395,7 +395,6 @@ func TestEvaluatorPrefetcherAlignmentCreateAsset(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentReconfigAsset(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) addr := makeAddress(1) @@ -448,7 +447,6 @@ func TestEvaluatorPrefetcherAlignmentReconfigAsset(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentAssetOptIn(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) assetID := basics.AssetIndex(5) @@ -504,7 +502,6 @@ func TestEvaluatorPrefetcherAlignmentAssetOptIn(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentAssetTransfer(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) assetID := basics.AssetIndex(5) @@ -571,7 +568,6 @@ func TestEvaluatorPrefetcherAlignmentAssetTransfer(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentAssetClawback(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) assetID := basics.AssetIndex(5) @@ -811,7 +807,6 @@ func TestEvaluatorPrefetcherAlignmentCreateApplication(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentDeleteApplication(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) addr := makeAddress(1) @@ -866,7 +861,6 @@ func TestEvaluatorPrefetcherAlignmentDeleteApplication(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentApplicationOptIn(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) appID := basics.AppIndex(5) @@ -925,7 +919,6 @@ func TestEvaluatorPrefetcherAlignmentApplicationOptIn(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentApplicationCloseOut(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) appID := basics.AppIndex(5) @@ -990,7 +983,6 @@ func TestEvaluatorPrefetcherAlignmentApplicationCloseOut(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentApplicationClearState(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) appID := basics.AppIndex(5) @@ -1055,7 +1047,6 @@ func TestEvaluatorPrefetcherAlignmentApplicationClearState(t *testing.T) { } func TestEvaluatorPrefetcherAlignmentApplicationCallAccountsDeclaration(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) appID := basics.AppIndex(5) @@ -1119,11 +1110,11 @@ func TestEvaluatorPrefetcherAlignmentApplicationCallAccountsDeclaration(t *testi // Foreign accounts are not loaded, ensure they are not prefetched require.NotContains(t, prefetched.Accounts, makeAddress(5)) require.NotContains(t, prefetched.Accounts, makeAddress(3)) + require.Equal(t, requested, prefetched) } func TestEvaluatorPrefetcherAlignmentApplicationCallForeignAppsDeclaration(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) appID := basics.AppIndex(5) @@ -1191,7 +1182,6 @@ func TestEvaluatorPrefetcherAlignmentApplicationCallForeignAppsDeclaration(t *te } func TestEvaluatorPrefetcherAlignmentApplicationCallForeignAssetsDeclaration(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) appID := basics.AppIndex(5) diff --git a/ledger/internal/prefetcher/prefetcher_test.go b/ledger/internal/prefetcher/prefetcher_test.go index 28b7c6d558..8b67c66989 100644 --- a/ledger/internal/prefetcher/prefetcher_test.go +++ b/ledger/internal/prefetcher/prefetcher_test.go @@ -549,7 +549,6 @@ func TestEvaluatorPrefetcher(t *testing.T) { // Test for error from LookupAsset func TestAssetLookupError(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) rnd := basics.Round(5) @@ -607,7 +606,6 @@ func TestAssetLookupError(t *testing.T) { // Test for error from GetCreatorForRound func TestGetCreatorForRoundError(t *testing.T) { - t.Skip("disabled") partitiontest.PartitionTest(t) rnd := basics.Round(5) diff --git a/test/e2e-go/features/transactions/accountv2_test.go b/test/e2e-go/features/transactions/accountv2_test.go index 52b39c9eed..c5e7eda64c 100644 --- a/test/e2e-go/features/transactions/accountv2_test.go +++ b/test/e2e-go/features/transactions/accountv2_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/require" "github.com/algorand/go-algorand/config" + "github.com/algorand/go-algorand/crypto" "github.com/algorand/go-algorand/data/basics" "github.com/algorand/go-algorand/data/transactions" "github.com/algorand/go-algorand/data/transactions/logic" @@ -335,3 +336,317 @@ int 1 // 3 global state update in total, 2 local state updates checkEvalDelta(t, &client, txnRound, txnRound+1, 3, 2) } + +// Add offending asset index greater than uint64 +func TestAccountInformationWithBadAssetIdx(t *testing.T) { + partitiontest.PartitionTest(t) + defer fixtures.ShutdownSynchronizedTest(t) + + t.Parallel() + AccountInformationCheckWithOffendingFields(t, []basics.AssetIndex{12181853637140359511}, nil, nil) +} + +// Add missing asset index +func TestAccountInformationWithMissingAssetIdx(t *testing.T) { + partitiontest.PartitionTest(t) + defer fixtures.ShutdownSynchronizedTest(t) + + AccountInformationCheckWithOffendingFields(t, []basics.AssetIndex{121818}, nil, nil) +} + +// Add offending app index greater than uint64 +func TestAccountInformationWithBadAppIdx(t *testing.T) { + partitiontest.PartitionTest(t) + defer fixtures.ShutdownSynchronizedTest(t) + + AccountInformationCheckWithOffendingFields(t, nil, []basics.AppIndex{12181853637140359511}, nil) +} + +// Add missing app index +func TestAccountInformationWithMissingApp(t *testing.T) { + partitiontest.PartitionTest(t) + defer fixtures.ShutdownSynchronizedTest(t) + + AccountInformationCheckWithOffendingFields(t, nil, []basics.AppIndex{121818}, nil) +} + +// Add missing account address +func TestAccountInformationWithMissingAddress(t *testing.T) { + partitiontest.PartitionTest(t) + defer fixtures.ShutdownSynchronizedTest(t) + + randAddr := basics.Address{} + crypto.RandBytes(randAddr[:]) + AccountInformationCheckWithOffendingFields(t, nil, nil, []basics.Address{randAddr}) +} + +func AccountInformationCheckWithOffendingFields(t *testing.T, + foreignAssets []basics.AssetIndex, + foreignApps []basics.AppIndex, + accounts []basics.Address) { + + a := require.New(fixtures.SynchronizedTest(t)) + + var fixture fixtures.RestClientFixture + proto, ok := config.Consensus[protocol.ConsensusFuture] + a.True(ok) + proto.AgreementFilterTimeoutPeriod0 = 400 * time.Millisecond + proto.AgreementFilterTimeout = 400 * time.Millisecond + fixture.SetConsensus(config.ConsensusProtocols{protocol.ConsensusFuture: proto}) + + fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50EachV26.json")) + defer fixture.Shutdown() + + client := fixture.LibGoalClient + accountList, err := fixture.GetWalletsSortedByBalance() + a.NoError(err) + + creator := accountList[0].Address + wh, err := client.GetUnencryptedWalletHandle() + a.NoError(err) + + user, err := client.GenerateAddress(wh) + a.NoError(err) + + fee := uint64(1000) + + var txn transactions.Transaction + + // Fund the manager, so it can issue transactions later on + txn, err = client.SendPaymentFromUnencryptedWallet(creator, user, fee, 10000000000, nil) + a.NoError(err) + + round, err := client.CurrentRound() + a.NoError(err) + fixture.WaitForConfirmedTxn(round+4, creator, txn.ID().String()) + + // There should be no apps to start with + ad, err := client.AccountData(creator) + a.NoError(err) + a.Zero(len(ad.AppParams)) + + ad, err = client.AccountData(user) + a.NoError(err) + a.Zero(len(ad.AppParams)) + a.Equal(basics.MicroAlgos{Raw: 10000000000}, ad.MicroAlgos) + + counter := `#pragma version 2 +// a simple global and local calls counter app +byte b64 Y291bnRlcg== // counter +dup +app_global_get +int 1 ++ +app_global_put // update the counter +int 0 +int 0 +app_opted_in +bnz opted_in +err +opted_in: +int 0 // account idx for app_local_put +byte b64 Y291bnRlcg== // counter +int 0 +byte b64 Y291bnRlcg== +app_local_get +int 1 // increment ++ +app_local_put +int 1 +` + approvalOps, err := logic.AssembleString(counter) + a.NoError(err) + clearstateOps, err := logic.AssembleString("#pragma version 2\nint 1") + a.NoError(err) + schema := basics.StateSchema{ + NumUint: 1, + } + + // create the app + tx, err := client.MakeUnsignedAppCreateTx( + transactions.OptInOC, approvalOps.Program, clearstateOps.Program, schema, schema, nil, nil, nil, nil, 0) + a.NoError(err) + tx, err = client.FillUnsignedTxTemplate(creator, 0, 0, fee, tx) + a.NoError(err) + wh, err = client.GetUnencryptedWalletHandle() + a.NoError(err) + signedTxn, err := client.SignTransactionWithWallet(wh, nil, tx) + a.NoError(err) + txid, err := client.BroadcastTransaction(signedTxn) + a.NoError(err) + round, err = client.CurrentRound() + a.NoError(err) + // ensure transaction is accepted into a block within 5 rounds. + confirmed := fixture.WaitForAllTxnsToConfirm(round+5, map[string]string{txid: signedTxn.Txn.Sender.String()}) + a.True(confirmed) + + // check creator's balance record for the app entry and the state changes + ad, err = client.AccountData(creator) + a.NoError(err) + a.Equal(1, len(ad.AppParams)) + var appIdx basics.AppIndex + var params basics.AppParams + for i, p := range ad.AppParams { + appIdx = i + params = p + break + } + a.Equal(approvalOps.Program, params.ApprovalProgram) + a.Equal(clearstateOps.Program, params.ClearStateProgram) + a.Equal(schema, params.LocalStateSchema) + a.Equal(schema, params.GlobalStateSchema) + a.Equal(1, len(params.GlobalState)) + value, ok := params.GlobalState["counter"] + a.True(ok) + a.Equal(uint64(1), value.Uint) + + a.Equal(1, len(ad.AppLocalStates)) + state, ok := ad.AppLocalStates[appIdx] + a.True(ok) + a.Equal(schema, state.Schema) + a.Equal(1, len(state.KeyValue)) + value, ok = state.KeyValue["counter"] + a.True(ok) + a.Equal(uint64(1), value.Uint) + + txInfo, err := fixture.LibGoalClient.PendingTransactionInformationV2(txid) + a.NoError(err) + a.NotNil(txInfo.ConfirmedRound) + a.NotZero(*txInfo.ConfirmedRound) + txnRound := *txInfo.ConfirmedRound + + // 1 global state update in total, 1 local state updates + checkEvalDelta(t, &client, txnRound, txnRound+1, 1, 1) + + // call the app + tx, err = client.MakeUnsignedAppOptInTx(uint64(appIdx), nil, nil, nil, nil) + a.NoError(err) + if foreignAssets != nil { + tx.ForeignAssets = foreignAssets + } + if foreignApps != nil { + tx.ForeignApps = foreignApps + } + if accounts != nil { + tx.Accounts = accounts + } + tx, err = client.FillUnsignedTxTemplate(user, 0, 0, fee, tx) + a.NoError(err) + wh, err = client.GetUnencryptedWalletHandle() + a.NoError(err) + signedTxn, err = client.SignTransactionWithWallet(wh, nil, tx) + a.NoError(err) + txid, err = client.BroadcastTransaction(signedTxn) + a.NoError(err) + round, err = client.CurrentRound() + a.NoError(err) + _, err = client.WaitForRound(round + 3) + a.NoError(err) + + // Ensure the txn committed + resp, err := client.GetPendingTransactions(2) + a.NoError(err) + a.Equal(uint64(0), resp.TotalTxns) + txinfo, err := client.TransactionInformation(signedTxn.Txn.Sender.String(), txid) + a.NoError(err) + a.True(txinfo.ConfirmedRound != 0) + + // check creator's balance record for the app entry and the state changes + ad, err = client.AccountData(creator) + a.NoError(err) + a.Equal(1, len(ad.AppParams)) + params, ok = ad.AppParams[appIdx] + a.True(ok) + a.Equal(approvalOps.Program, params.ApprovalProgram) + a.Equal(clearstateOps.Program, params.ClearStateProgram) + a.Equal(schema, params.LocalStateSchema) + a.Equal(schema, params.GlobalStateSchema) + a.Equal(1, len(params.GlobalState)) + value, ok = params.GlobalState["counter"] + a.True(ok) + a.Equal(uint64(2), value.Uint) + + a.Equal(1, len(ad.AppLocalStates)) + state, ok = ad.AppLocalStates[appIdx] + a.True(ok) + a.Equal(schema, state.Schema) + a.Equal(1, len(state.KeyValue)) + value, ok = state.KeyValue["counter"] + a.True(ok) + a.Equal(uint64(1), value.Uint) + + a.Equal(uint64(2), ad.TotalAppSchema.NumUint) + + // check user's balance record for the app entry and the state changes + ad, err = client.AccountData(user) + a.NoError(err) + a.Equal(0, len(ad.AppParams)) + + a.Equal(1, len(ad.AppLocalStates)) + state, ok = ad.AppLocalStates[appIdx] + a.True(ok) + a.Equal(schema, state.Schema) + a.Equal(1, len(state.KeyValue)) + value, ok = state.KeyValue["counter"] + a.True(ok) + a.Equal(uint64(1), value.Uint) + + txInfo, err = fixture.LibGoalClient.PendingTransactionInformationV2(txid) + a.NoError(err) + a.NotNil(txInfo.ConfirmedRound) + a.NotZero(*txInfo.ConfirmedRound) + txnRound = *txInfo.ConfirmedRound + + // 2 global state update in total, 1 local state updates + checkEvalDelta(t, &client, txnRound, txnRound+1, 2, 1) + + a.Equal(basics.MicroAlgos{Raw: 10000000000 - fee}, ad.MicroAlgos) + + app, err := client.ApplicationInformation(uint64(appIdx)) + a.NoError(err) + a.Equal(uint64(appIdx), app.Id) + a.Equal(creator, app.Params.Creator) + + // call the app + tx, err = client.MakeUnsignedAppNoOpTx(uint64(appIdx), nil, nil, nil, nil) + a.NoError(err) + tx, err = client.FillUnsignedTxTemplate(user, 0, 0, fee, tx) + a.NoError(err) + signedTxn, err = client.SignTransactionWithWallet(wh, nil, tx) + a.NoError(err) + txid, err = client.BroadcastTransaction(signedTxn) + a.NoError(err) + for { + round, err = client.CurrentRound() + a.NoError(err) + _, err = client.WaitForRound(round + 1) + a.NoError(err) + // Ensure the txn committed + resp, err = client.GetPendingTransactions(2) + a.NoError(err) + if resp.TotalTxns == 1 { + a.Equal(resp.TruncatedTxns.Transactions[0].TxID, txid) + continue + } + a.Equal(uint64(0), resp.TotalTxns) + break + } + + ad, err = client.AccountData(creator) + a.NoError(err) + a.Equal(1, len(ad.AppParams)) + params, ok = ad.AppParams[appIdx] + a.True(ok) + value, ok = params.GlobalState["counter"] + a.True(ok) + a.Equal(uint64(3), value.Uint) + + txInfo, err = fixture.LibGoalClient.PendingTransactionInformationV2(txid) + a.NoError(err) + a.NotNil(txInfo.ConfirmedRound) + a.NotZero(*txInfo.ConfirmedRound) + txnRound = *txInfo.ConfirmedRound + + // 3 global state update in total, 2 local state updates + checkEvalDelta(t, &client, txnRound, txnRound+1, 3, 2) +} From 7095d5ab877ec2c1b0b636857a7aacb9c2f25ac6 Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 4 Aug 2022 16:36:31 -0400 Subject: [PATCH 3/8] Re-enable some prefetcher tests --- ledger/internal/prefetcher/prefetcher_test.go | 43 ++++++++----------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/ledger/internal/prefetcher/prefetcher_test.go b/ledger/internal/prefetcher/prefetcher_test.go index 8b67c66989..f145bdfdfb 100644 --- a/ledger/internal/prefetcher/prefetcher_test.go +++ b/ledger/internal/prefetcher/prefetcher_test.go @@ -259,7 +259,6 @@ func TestEvaluatorPrefetcher(t *testing.T) { }, { name: "asset config transaction for a non-existing asset", - skip: true, signedTxn: transactions.SignedTxn{ Txn: transactions.Transaction{ Type: protocol.AssetConfigTx, @@ -296,7 +295,6 @@ func TestEvaluatorPrefetcher(t *testing.T) { }, { name: "asset config transaction for an existing asset", - skip: true, signedTxn: transactions.SignedTxn{ Txn: transactions.Transaction{ Type: protocol.AssetConfigTx, @@ -333,7 +331,6 @@ func TestEvaluatorPrefetcher(t *testing.T) { }, { name: "asset transfer transaction", - skip: true, signedTxn: transactions.SignedTxn{ Txn: transactions.Transaction{ Type: protocol.AssetTransferTx, @@ -385,7 +382,6 @@ func TestEvaluatorPrefetcher(t *testing.T) { }, { name: "asset freeze transaction", - skip: true, signedTxn: transactions.SignedTxn{ Txn: transactions.Transaction{ Type: protocol.AssetFreezeTx, @@ -435,7 +431,6 @@ func TestEvaluatorPrefetcher(t *testing.T) { }, { name: "application transaction", - skip: true, signedTxn: transactions.SignedTxn{ Txn: transactions.Transaction{ Type: protocol.ApplicationCallTx, @@ -487,25 +482,25 @@ func TestEvaluatorPrefetcher(t *testing.T) { */ }, resources: []prefetcher.LoadedResourcesEntry{ - /* - { - Address: makeAddressPtr(2), - CreatableIndex: 1001, - CreatableType: basics.AssetCreatable, - Resource: &ledgercore.AccountResource{}, - }, - { - Address: makeAddressPtr(15), - CreatableIndex: 2001, - CreatableType: basics.AppCreatable, - Resource: &ledgercore.AccountResource{}, - }, - { - Address: nil, - CreatableIndex: 2002, - CreatableType: basics.AppCreatable, - Resource: nil, - }, + /* - if we'll decide that we want to perfetch the foreign apps/assets, then this should be enabled + { + Address: makeAddressPtr(2), + CreatableIndex: 1001, + CreatableType: basics.AssetCreatable, + Resource: &ledgercore.AccountResource{}, + }, + { + Address: makeAddressPtr(15), + CreatableIndex: 2001, + CreatableType: basics.AppCreatable, + Resource: &ledgercore.AccountResource{}, + }, + { + Address: nil, + CreatableIndex: 2002, + CreatableType: basics.AppCreatable, + Resource: nil, + }, */ /* - if we'll decide that we want to perfetch the account local state, then this should be enabled. { From 7d1ca4dd47ffb4f7f0751b39e2536ba52139f03a Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 4 Aug 2022 17:51:38 -0400 Subject: [PATCH 4/8] CR: update comment --- ledger/internal/prefetcher/prefetcher.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ledger/internal/prefetcher/prefetcher.go b/ledger/internal/prefetcher/prefetcher.go index 44c46e0bac..a36a59a36f 100644 --- a/ledger/internal/prefetcher/prefetcher.go +++ b/ledger/internal/prefetcher/prefetcher.go @@ -438,7 +438,7 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { delete(completed, next) - // if we had no error, write the result to the output channel. + // write the result to the output channel. // this write will not block since we preallocated enough space on the channel. p.outChan <- LoadedTransactionGroup{ Err: groupsReady[next].err, From 0e1ee7480fe1c310062b6d36ba7aac8f14d08913 Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 15 Sep 2022 16:08:17 -0400 Subject: [PATCH 5/8] CR: readability --- ledger/internal/eval.go | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/ledger/internal/eval.go b/ledger/internal/eval.go index 6891bc2c65..00061d95ff 100644 --- a/ledger/internal/eval.go +++ b/ledger/internal/eval.go @@ -1549,7 +1549,7 @@ transactionGroupLoop: if !ok { break transactionGroupLoop } else if txgroup.Err != nil { - logging.Base().Errorf("evaluation error: %w", txgroup.Err) + logging.Base().Errorf("eval prefetcher error: %w", txgroup.Err) } if txgroup.Err == nil { @@ -1561,34 +1561,44 @@ transactionGroupLoop: for _, lr := range txgroup.Resources { if lr.Address == nil { // we attempted to look for the creator, and failed. - base.creators[creatable{cindex: lr.CreatableIndex, ctype: lr.CreatableType}] = - foundAddress{exists: false} + creatableKey := creatable{cindex: lr.CreatableIndex, ctype: lr.CreatableType} + base.creators[creatableKey] = foundAddress{exists: false} continue } if lr.CreatableType == basics.AssetCreatable { + assetKey := ledgercore.AccountAsset{ + Address: *lr.Address, + Asset: basics.AssetIndex(lr.CreatableIndex), + } + if lr.Resource.AssetHolding != nil { - base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true} + base.assets[assetKey] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true} } else { - base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{exists: false} + base.assets[assetKey] = cachedAssetHolding{exists: false} } if lr.Resource.AssetParams != nil { - base.assetParams[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetParams{value: *lr.Resource.AssetParams, exists: true} - base.creators[creatable{cindex: lr.CreatableIndex, ctype: basics.AssetCreatable}] = foundAddress{address: *lr.Address, exists: true} + creatableKey := creatable{cindex: lr.CreatableIndex, ctype: basics.AssetCreatable} + base.assetParams[assetKey] = cachedAssetParams{value: *lr.Resource.AssetParams, exists: true} + base.creators[creatableKey] = foundAddress{address: *lr.Address, exists: true} } else { - base.assetParams[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetParams{exists: false} - + base.assetParams[assetKey] = cachedAssetParams{exists: false} } } else { + appKey := ledgercore.AccountApp{ + Address: *lr.Address, + App: basics.AppIndex(lr.CreatableIndex), + } if lr.Resource.AppLocalState != nil { - base.appLocalStates[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppLocalState{value: *lr.Resource.AppLocalState, exists: true} + base.appLocalStates[appKey] = cachedAppLocalState{value: *lr.Resource.AppLocalState, exists: true} } else { - base.appLocalStates[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppLocalState{exists: false} + base.appLocalStates[appKey] = cachedAppLocalState{exists: false} } if lr.Resource.AppParams != nil { - base.appParams[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppParams{value: *lr.Resource.AppParams, exists: true} - base.creators[creatable{cindex: lr.CreatableIndex, ctype: basics.AppCreatable}] = foundAddress{address: *lr.Address, exists: true} + creatableKey := creatable{cindex: lr.CreatableIndex, ctype: basics.AppCreatable} + base.appParams[appKey] = cachedAppParams{value: *lr.Resource.AppParams, exists: true} + base.creators[creatableKey] = foundAddress{address: *lr.Address, exists: true} } else { - base.appParams[ledgercore.AccountApp{Address: *lr.Address, App: basics.AppIndex(lr.CreatableIndex)}] = cachedAppParams{exists: false} + base.appParams[appKey] = cachedAppParams{exists: false} } } } From 76b71c92f0c7456683db71de68b1cc58b57077af Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 15 Sep 2022 16:08:45 -0400 Subject: [PATCH 6/8] Do not prefetch zero asset transfers --- ledger/internal/prefetcher/prefetcher.go | 11 ++- .../prefetcher/prefetcher_alignment_test.go | 92 ++++++++++++++++++- 2 files changed, 98 insertions(+), 5 deletions(-) diff --git a/ledger/internal/prefetcher/prefetcher.go b/ledger/internal/prefetcher/prefetcher.go index a36a59a36f..098ac5f193 100644 --- a/ledger/internal/prefetcher/prefetcher.go +++ b/ledger/internal/prefetcher/prefetcher.go @@ -302,14 +302,17 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) loadAccountsAddResourceTask(&stxn.Txn.AssetSender, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) } else { - loadAccountsAddResourceTask(&stxn.Txn.Sender, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) - if stxn.Txn.AssetAmount == 0 && (stxn.Txn.AssetReceiver == stxn.Txn.Sender) { - // opt in + if stxn.Txn.AssetAmount == 0 && (stxn.Txn.AssetReceiver == stxn.Txn.Sender) { // opt in loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) } + if stxn.Txn.AssetAmount != 0 { // zero transfer is noop + loadAccountsAddResourceTask(&stxn.Txn.Sender, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + } } if !stxn.Txn.AssetReceiver.IsZero() { - loadAccountsAddResourceTask(&stxn.Txn.AssetReceiver, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + if stxn.Txn.AssetAmount != 0 { // zero transfer is noop + loadAccountsAddResourceTask(&stxn.Txn.AssetReceiver, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) + } } if !stxn.Txn.AssetCloseTo.IsZero() { loadAccountsAddResourceTask(&stxn.Txn.AssetCloseTo, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) diff --git a/ledger/internal/prefetcher/prefetcher_alignment_test.go b/ledger/internal/prefetcher/prefetcher_alignment_test.go index bf77af27cd..55571ed97a 100644 --- a/ledger/internal/prefetcher/prefetcher_alignment_test.go +++ b/ledger/internal/prefetcher/prefetcher_alignment_test.go @@ -501,7 +501,7 @@ func TestEvaluatorPrefetcherAlignmentAssetOptIn(t *testing.T) { require.Equal(t, requested, prefetched) } -func TestEvaluatorPrefetcherAlignmentAssetTransfer(t *testing.T) { +func TestEvaluatorPrefetcherAlignmentAssetOptInCloseTo(t *testing.T) { partitiontest.PartitionTest(t) assetID := basics.AssetIndex(5) @@ -567,6 +567,96 @@ func TestEvaluatorPrefetcherAlignmentAssetTransfer(t *testing.T) { require.Equal(t, requested, prefetched) } +func TestEvaluatorPrefetcherAlignmentAssetTransfer(t *testing.T) { + partitiontest.PartitionTest(t) + + assetID := basics.AssetIndex(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}, + TotalAssets: 1, + TotalAssetParams: 1, + }, + }, + makeAddress(2): { + AccountBaseData: ledgercore.AccountBaseData{ + MicroAlgos: basics.MicroAlgos{Raw: 1000002}, + }, + }, + makeAddress(3): { + AccountBaseData: ledgercore.AccountBaseData{ + MicroAlgos: basics.MicroAlgos{Raw: 1000003}, + }, + }, + }, + assets: map[basics.Address]map[basics.AssetIndex]ledgercore.AssetResource{ + makeAddress(1): { + assetID: { + AssetParams: &basics.AssetParams{}, + AssetHolding: &basics.AssetHolding{}, + }, + }, + makeAddress(2): { + assetID: { + AssetHolding: &basics.AssetHolding{Amount: 5}, + }, + }, + makeAddress(3): { + assetID: { + AssetHolding: &basics.AssetHolding{}, + }, + }, + }, + creators: map[basics.CreatableIndex]basics.Address{ + basics.CreatableIndex(assetID): makeAddress(1), + }, + } + + txn := transactions.Transaction{ + Type: protocol.AssetTransferTx, + Header: transactions.Header{ + Sender: makeAddress(2), + GenesisHash: genesisHash(), + }, + AssetTransferTxnFields: transactions.AssetTransferTxnFields{ + XferAsset: assetID, + AssetReceiver: makeAddress(3), + AssetAmount: 1, + }, + } + + requested, prefetched := run(t, l, txn) + + prefetched.Accounts[rewardsPool()] = struct{}{} + require.Equal(t, requested, prefetched) + + // zero transfer of any asset + txn = transactions.Transaction{ + Type: protocol.AssetTransferTx, + Header: transactions.Header{ + Sender: makeAddress(1), + GenesisHash: genesisHash(), + }, + AssetTransferTxnFields: transactions.AssetTransferTxnFields{ + XferAsset: assetID + 12345, + AssetReceiver: makeAddress(2), + AssetAmount: 0, + }, + } + + requested, prefetched = run(t, l, txn) + + prefetched.Accounts[rewardsPool()] = struct{}{} + require.Equal(t, requested, prefetched) +} + func TestEvaluatorPrefetcherAlignmentAssetClawback(t *testing.T) { partitiontest.PartitionTest(t) From ee154518836fad38157f19312f32107b415c65b7 Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Fri, 16 Sep 2022 12:19:31 -0400 Subject: [PATCH 7/8] Fix unit test --- ledger/internal/prefetcher/prefetcher.go | 3 +- ledger/internal/prefetcher/prefetcher_test.go | 50 ++++++++++++++++++- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/ledger/internal/prefetcher/prefetcher.go b/ledger/internal/prefetcher/prefetcher.go index 098ac5f193..fcdfacfa12 100644 --- a/ledger/internal/prefetcher/prefetcher.go +++ b/ledger/internal/prefetcher/prefetcher.go @@ -310,7 +310,8 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { } } if !stxn.Txn.AssetReceiver.IsZero() { - if stxn.Txn.AssetAmount != 0 { // zero transfer is noop + if stxn.Txn.AssetAmount != 0 || (stxn.Txn.AssetReceiver == stxn.Txn.Sender) { + // if not zero transfer and not not opt in loadAccountsAddResourceTask(&stxn.Txn.AssetReceiver, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) } } diff --git a/ledger/internal/prefetcher/prefetcher_test.go b/ledger/internal/prefetcher/prefetcher_test.go index f145bdfdfb..555cc8f6d2 100644 --- a/ledger/internal/prefetcher/prefetcher_test.go +++ b/ledger/internal/prefetcher/prefetcher_test.go @@ -339,6 +339,7 @@ func TestEvaluatorPrefetcher(t *testing.T) { }, AssetTransferTxnFields: transactions.AssetTransferTxnFields{ XferAsset: 1001, + AssetAmount: 1, AssetSender: makeAddress(2), AssetReceiver: makeAddress(3), AssetCloseTo: makeAddress(4), @@ -380,6 +381,51 @@ func TestEvaluatorPrefetcher(t *testing.T) { }, }, }, + { + name: "asset transfer transaction zero amount", + signedTxn: transactions.SignedTxn{ + Txn: transactions.Transaction{ + Type: protocol.AssetTransferTx, + Header: transactions.Header{ + Sender: makeAddress(1), + }, + AssetTransferTxnFields: transactions.AssetTransferTxnFields{ + XferAsset: 1001, + AssetSender: makeAddress(2), + AssetReceiver: makeAddress(3), + AssetCloseTo: makeAddress(4), + }, + }, + }, + accounts: []prefetcher.LoadedAccountDataEntry{ + { + Address: &feeSinkAddr, + Data: &ledgercore.AccountData{ + AccountBaseData: ledgercore.AccountBaseData{MicroAlgos: basics.MicroAlgos{Raw: 0}}, + }, + }, + { + Address: makeAddressPtr(1), + Data: &ledgercore.AccountData{ + AccountBaseData: ledgercore.AccountBaseData{MicroAlgos: basics.MicroAlgos{Raw: 100000000}}, + }, + }, + }, + resources: []prefetcher.LoadedResourcesEntry{ + { + Address: makeAddressPtr(2), + CreatableIndex: 1001, + CreatableType: basics.AssetCreatable, + Resource: &ledgercore.AccountResource{}, + }, + { + Address: makeAddressPtr(4), + CreatableIndex: 1001, + CreatableType: basics.AssetCreatable, + Resource: &ledgercore.AccountResource{}, + }, + }, + }, { name: "asset freeze transaction", signedTxn: transactions.SignedTxn{ @@ -482,7 +528,7 @@ func TestEvaluatorPrefetcher(t *testing.T) { */ }, resources: []prefetcher.LoadedResourcesEntry{ - /* - if we'll decide that we want to perfetch the foreign apps/assets, then this should be enabled + /* - if we'll decide that we want to prefetch the foreign apps/assets, then this should be enabled { Address: makeAddressPtr(2), CreatableIndex: 1001, @@ -502,7 +548,7 @@ func TestEvaluatorPrefetcher(t *testing.T) { Resource: nil, }, */ - /* - if we'll decide that we want to perfetch the account local state, then this should be enabled. + /* - if we'll decide that we want to prefetch the account local state, then this should be enabled. { address: acctAddrPtr(1), creatableIndex: 10, From 6bdb584fe9c23d7145cc1cf5537bcc44de08832b Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Tue, 20 Sep 2022 10:23:02 -0400 Subject: [PATCH 8/8] CR feedback --- ledger/internal/prefetcher/prefetcher.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ledger/internal/prefetcher/prefetcher.go b/ledger/internal/prefetcher/prefetcher.go index fcdfacfa12..b7223806ff 100644 --- a/ledger/internal/prefetcher/prefetcher.go +++ b/ledger/internal/prefetcher/prefetcher.go @@ -311,7 +311,7 @@ func (p *accountPrefetcher) prefetch(ctx context.Context) { } if !stxn.Txn.AssetReceiver.IsZero() { if stxn.Txn.AssetAmount != 0 || (stxn.Txn.AssetReceiver == stxn.Txn.Sender) { - // if not zero transfer and not not opt in + // if not zero transfer or opt in then prefetch loadAccountsAddResourceTask(&stxn.Txn.AssetReceiver, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) } }