Ledger refactoring changes #870

tolikzinovyev · 2022-02-07T22:20:19Z

Summary

Ledger refactoring changes.

Closes https://github.com/algorand/go-algorand-internal/issues/1867.

Test Plan

Relying on existing tests. Some of them were changed. There are also tests for the migration.

The import validator caught up to the latest round on mainnet, testnet and betanet successfully.

codecov-commenter · 2022-02-08T03:58:33Z

Codecov Report

Merging #870 (d2677c5) into develop (e96032f) will increase coverage by 1.09%.
The diff coverage is 73.10%.

@@             Coverage Diff             @@
##           develop     #870      +/-   ##
===========================================
+ Coverage    58.54%   59.63%   +1.09%     
===========================================
  Files           37       38       +1     
  Lines         4407     4685     +278     
===========================================
+ Hits          2580     2794     +214     
- Misses        1514     1562      +48     
- Partials       313      329      +16

Impacted Files	Coverage Δ
cmd/block-generator/generator/generate.go	`58.92% <ø> (+0.16%)`	⬆️
idb/postgres/internal/encoding/types.go	`66.66% <ø> (ø)`
idb/postgres/postgres_migrations.go	`31.19% <5.00%> (-3.92%)`	⬇️
...gres/internal/migrations/convert_account_data/m.go	`63.15% <63.15%> (ø)`
idb/postgres/postgres.go	`50.31% <67.62%> (+1.06%)`	⬆️
...ernal/ledger_for_evaluator/ledger_for_evaluator.go	`76.28% <81.48%> (+9.14%)`	⬆️
idb/postgres/internal/encoding/encoding.go	`78.91% <87.50%> (+3.35%)`	⬆️
idb/postgres/internal/writer/writer.go	`75.40% <100.00%> (+0.20%)`	⬆️
util/util.go	`57.14% <0.00%> (-1.20%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e96032f...d2677c5. Read the comment docs.

shiqizng · 2022-02-15T19:03:19Z

cmd/import-validator/core/service.go

+		// Make a copy of `AppLocalState` to avoid modifying ledger's storage.
+		appLocalState := new(basics.AppLocalState)
+		*appLocalState = *r.AppLocalState
+		appLocalState.KeyValue = nil


why does global state and local state need to be set to nil?

go-algorand code can return both empty and nil maps, but indexer code only returns nil maps. So here if the map is empty, we set it to nil, so that comparison doesn't fail.

shiqizng · 2022-02-15T19:51:46Z

idb/postgres/internal/encoding/encoding.go

+	}
+}
+
+func unconvertTrimmedLcAccountData(ba baseAccountData) ledgercore.AccountData {


maybe rename these to baseAccountToTrimmedAccount and trimmedLcAccountToBaseAccount?

The current name is consistent with other similar functions here. But I could rename baseAccountData.

shiqizng · 2022-02-15T20:07:00Z

idb/postgres/internal/encoding/types.go

+	TotalAssetParams           uint64        `codec:"f"`
+	TotalAssets                uint64        `codec:"g"`
+	TotalAppParams             uint64        `codec:"h"`
+	TotalAppLocalStates        uint64        `codec:"i"`


do we need to give better key names?

Not necessarily. It's good for space efficiency, and this is how it's going to be done in go-algorand. https://github.com/algorand/go-algorand/blob/fcd06e94f60d2141df0cd11ff6e3fb0518483cdc/ledger/accountdb.go#L1079

Some of these fields are being renamed (or maybe just auth-addr). Not sure if anyone is querying these directly, but it could potentially matter to some users. The fact that the names are equivalent to algod makes this seem a little better, but it does seem unusual that the data is basically unusable without serializing it to a go object.

It's only auth-addr. Are you proposing anything?

while the space efficiency in go-algorand is impressive, I think it would be better for the indexer to have user friendly names for things.. these will show up as SQL query JSON names right?

basically, shouldn't we have backwards compatibility with the codec names in basics.AccountData? onl, vote, voteFst, voteLst, etc..?

Yes, for instance, it would show up in the postgres query log. In this PR that only applies to auth-addr.

And yes, if someone had their own query to get any of those other renamed fields, those queries would break (that's what I was trying to get at in my first comment)

I noticed that flipside crypto lets you query JSON keys inside the accountdata blob. names like onl, vote, voteFst, voteLst are being renamed here, more than auth-addr, right? @barnjamin

Yes thats correct

shiqizng · 2022-02-15T20:16:59Z

idb/postgres/internal/ledger_for_evaluator/ledger_for_evaluator.go

+func (l LedgerForEvaluator) LookupWithoutRewards(addresses map[basics.Address]struct{}) (map[basics.Address]*ledgercore.AccountData, error) {
+	addressesArr := make([]basics.Address, 0, len(addresses))
+	for address := range addresses {
+		addressesArr = append(addressesArr, address)
 	}


do we need to make a copy here?

Yes because iterating over a map is non-determinstic. So, I first copy the addresses to an array.

I think you need to copy the address object into the array, since "address" is reused for each value in the range.

addressesArr := make([]basics.Address, len(addresses)) i := 0 for address := range addresses { copy(addressesArr[i][:], address) i++ }

basics.Address is a fixed size byte array. It is copied by value.

shiqizng · 2022-02-15T21:06:55Z

idb/postgres/internal/ledger_for_evaluator/ledger_for_evaluator.go

-		}
+	err := row.Scan(&creatorAddr, &params)
+	if err == pgx.ErrNoRows {
+		return basics.Address{}, basics.AssetParams{}, false, nil


return err here?

If the asset doesn't exist, there is no error. However, the third return value ("exists") is false.

Does this case get triggered when creating an asset/holding?

can you explain?

I was wondering when you would expect to see ErrNoRows. If it isn't expected it could be an error like Shiqi was suggesting.

Suppose the evaluator wants asset holding of an account. But it can only request asset holding together with asset params for the account. In many cases, asset params will not be created by the given account.

winder

I haven't reviewed the migration code yet, but wanted to share my first round of comments.

Overall it seems to make sense, but I wasn't expecting the data structures to be so complicated. In a way, it feels like we're going to have the same sort of issues we had with the "apply" functions, but now it's complexity for running "eval".

I did have some specific requests in the comments.

winder · 2022-02-17T16:29:42Z

cmd/import-validator/README.md

@@ -1,14 +1,14 @@
 # Import validation tool


Is this tool able to upgrade the DB from a previous release, or does it need to be restarted from the beginning?

It can upgrade the DBs (indexer and go-algorand). It uses public "open db" functions that will run necessary migrations.

winder · 2022-02-17T17:43:41Z

idb/postgres/internal/encoding/types.go

@@ -106,3 +106,29 @@ type specialAddresses struct {
 	FeeSinkOverride     crypto.Digest `codec:"FeeSink"`
 	RewardsPoolOverride crypto.Digest `codec:"RewardsPool"`
 }
+
+type baseOnlineAccountData struct {


Is there a type that we can embed here?

The pattern used previously implicitly supports new fields. With this pattern, it would be easy to forget about a new field and accidentally drop that data.

No unfortunately. The new approach in go-algorand is, ledgercore.AccountData doesn't have codec tags, and there is baseAccountData similar to this one.

Could we make them public in go-algorand?

hrmm. would it help to add codec tags to ledgercore.AccountData and ledgercore.VotingData that match the ones in basics.AccountData?

winder · 2022-02-17T17:50:01Z

idb/postgres/internal/encoding/types.go

+	TotalAssetParams           uint64        `codec:"f"`
+	TotalAssets                uint64        `codec:"g"`
+	TotalAppParams             uint64        `codec:"h"`
+	TotalAppLocalStates        uint64        `codec:"i"`


Some of these fields are being renamed (or maybe just auth-addr). Not sure if anyone is querying these directly, but it could potentially matter to some users. The fact that the names are equivalent to algod makes this seem a little better, but it does seem unusual that the data is basically unusable without serializing it to a go object.

winder · 2022-02-17T19:02:22Z

idb/postgres/internal/ledger_for_evaluator/ledger_for_evaluator.go

+		"WHERE index = $1 AND NOT deleted",
+	appParamsStmtName: "SELECT creator, params FROM app WHERE index = $1 AND NOT deleted",
+	appLocalStatesStmtName: "SELECT localstate FROM account_app " +
+		"WHERE addr = $1 AND app = $2 AND NOT deleted",


Just checking my understanding here. Previously we query for all of the holdings according to an address, now the eval function gives a list of required IDs and they are fetched one by one as needed?

Yes, the eval function gives a list of (address, creatable).

winder · 2022-02-17T19:05:22Z

idb/postgres/internal/ledger_for_evaluator/ledger_for_evaluator.go

+func (l LedgerForEvaluator) LookupWithoutRewards(addresses map[basics.Address]struct{}) (map[basics.Address]*ledgercore.AccountData, error) {
+	addressesArr := make([]basics.Address, 0, len(addresses))
+	for address := range addresses {
+		addressesArr = append(addressesArr, address)
 	}


I think you need to copy the address object into the array, since "address" is reused for each value in the range.

addressesArr := make([]basics.Address, len(addresses)) i := 0 for address := range addresses { copy(addressesArr[i][:], address) i++ }

winder · 2022-02-17T19:31:04Z

idb/postgres/internal/ledger_for_evaluator/ledger_for_evaluator.go

-		if accountData != nil {
-			existingAddresses = append(existingAddresses, address)
+// LookupResources is part of go-algorand's indexerLedgerForEval interface.
+func (l LedgerForEvaluator) LookupResources(input map[basics.Address]map[ledger.Creatable]struct{}) (map[basics.Address]map[ledger.Creatable]ledgercore.AccountResource, error) {


This whole function is very hard to follow with the large data structures mixed with SQL queries that lookup the data.

Could you update the function comment to summarize what's happening, and add some comments to the different sections?

winder · 2022-02-17T19:31:50Z

idb/postgres/internal/ledger_for_evaluator/ledger_for_evaluator.go

+	appLocalStatesReq := make([]AddrID, 0, len(input))
+	appParamsReq := make([]basics.CreatableIndex, 0, len(input))
+	appParamsToAddresses := make(map[basics.CreatableIndex]map[basics.Address]struct{})
+


Is this whole loop / data structure created to facilitate the batch query and results processing?

Yes, we need to construct arrays because iterating over maps is non-deterministic.

winder · 2022-02-17T19:32:59Z

idb/postgres/internal/ledger_for_evaluator/ledger_for_evaluator_test.go

+			RewardedMicroAlgos: basics.MicroAlgos{Raw: 6},
+			AuthAddr:           test.AccountA,
+		},
+		VotingData: ledgercore.VotingData{


May as well add a state proof key in here.

winder · 2022-02-17T21:17:37Z

idb/postgres/postgres.go

+// `resourcesReq` respectively for the given transaction.
+func addToAccountsResourcesRequest(stxnad *transactions.SignedTxnWithAD, assetCreators map[basics.AssetIndex]ledger.FoundAddress, appCreators map[basics.AppIndex]ledger.FoundAddress, addressesReq map[basics.Address]struct{}, resourcesReq map[basics.Address]map[ledger.Creatable]struct{}) {
+	lookupResourcesReq :=
+		func(addr basics.Address) map[ledger.Creatable]struct{} {


This helper seems to be used identically everywhere:

lookupResourcesReq(txn.Sender)[creatable] = struct{}{}

How about changing the signature to setResourcesReq(txn.Sender, creatable) and skip the map index / struct{}{}?

good idea, done

winder · 2022-02-17T21:23:30Z

idb/postgres/postgres.go

+
+// Add requests for account data and account resources to `addressesReq` and
+// `resourcesReq` respectively for the given transaction.
+func addToAccountsResourcesRequest(stxnad *transactions.SignedTxnWithAD, assetCreators map[basics.AssetIndex]ledger.FoundAddress, appCreators map[basics.AppIndex]ledger.FoundAddress, addressesReq map[basics.Address]struct{}, resourcesReq map[basics.Address]map[ledger.Creatable]struct{}) {


Are any of these helpers postgres specific, or is this pulling types out of transactions and building up accounting data structures?

We should pull all of the accounting logic into a separate package so that it's clear what is for the DB and what is dealing with go-algorand accounting details.

I moved some of it to accounting/.

tolikzinovyev · 2022-02-18T19:50:35Z

I haven't reviewed the migration code yet, but wanted to share my first round of comments.

Overall it seems to make sense, but I wasn't expecting the data structures to be so complicated. In a way, it feels like we're going to have the same sort of issues we had with the "apply" functions, but now it's complexity for running "eval".

I did have some specific requests in the comments.

Thanks for the review! What complexity exactly are you referring to?

winder · 2022-02-18T21:24:28Z

I haven't reviewed the migration code yet, but wanted to share my first round of comments.
Overall it seems to make sense, but I wasn't expecting the data structures to be so complicated. In a way, it feels like we're going to have the same sort of issues we had with the "apply" functions, but now it's complexity for running "eval".
I did have some specific requests in the comments.

Thanks for the review! What complexity exactly are you referring to?

Working with the map[addr]map[creatable]struct{} data structures. I can see how the existing patterns were applied pretty directly to the new data structures, but it's complicated.

winder · 2022-02-18T22:05:15Z

idb/postgres/postgres.go

+// Preload account data and account resources.
+func prepareAccountsResources(l *ledger_for_evaluator.LedgerForEvaluator, payset transactions.Payset, assetCreators map[basics.AssetIndex]ledger.FoundAddress, appCreators map[basics.AppIndex]ledger.FoundAddress) (map[basics.Address]*ledgercore.AccountData, map[basics.Address]map[ledger.Creatable]ledgercore.AccountResource, error) {
+	addressesReq, resourcesReq :=
+		accounting.MakePreloadAccountsResourcesRequest(payset, assetCreators, appCreators)


ty, this makes it a lot easier to read the code 👍

winder

Looks good!

Adds new REST endpoints and "exclude" parameter for account information endpoint, similar to algorand/go-algorand#3542. Relies on #870 Update endpoint: GET /v2/accounts/{address} Supports new query parameter “exclude”, which can be a combination of all, created-apps, created-assets, apps-local-state, assets Removes empty “creator” string from AssetHolding type — was not implemented (removed associated TODO) Update endpoint: GET /v2/assets/{asset-id}/balances Remove "round" query parameter — I noticed support is not implemented, and the parameter was being ignored New endpoints: GET /v2/accounts/{address}/created-apps GET /v2/accounts/{address}/created-assets GET /v2/accounts/{address}/assets GET /v2/accounts/{address}/apps-local-state Supported query parameters: asset-id / application-id (look up a single asset), include-all (include deleted), limit, next (for pagination)

tolikzinovyev added the Team Carbon-11 label Feb 7, 2022

tolikzinovyev self-assigned this Feb 7, 2022

tolikzinovyev force-pushed the tolik/unlimited-assets-import branch 3 times, most recently from b75d368 to 70b5450 Compare February 11, 2022 20:34

tolikzinovyev marked this pull request as ready for review February 11, 2022 20:35

tolikzinovyev requested a review from winder February 11, 2022 20:35

shiqizng reviewed Feb 15, 2022

View reviewed changes

cce changed the base branch from develop to feature/unlimited-assets February 17, 2022 16:02

winder suggested changes Feb 17, 2022

View reviewed changes

cce mentioned this pull request Feb 18, 2022

REST API changes for unlimited assets #872

Merged

tolikzinovyev added 6 commits February 18, 2022 12:41

Ledger refactoring changes.

96d35a2

Move some of the preloading logic to accounting/.

1163471

Use setResourcesReq().

0ccc27d

Some renaming.

1515970

Add comments to ledger_for_evaluator.LookupResources().

cde056a

Encode state proof id.

2f706ce

tolikzinovyev force-pushed the tolik/unlimited-assets-import branch from 70b5450 to 2f706ce Compare February 18, 2022 19:49

tolikzinovyev requested a review from winder February 18, 2022 19:50

winder reviewed Feb 18, 2022

View reviewed changes

winder approved these changes Feb 22, 2022

View reviewed changes

tolikzinovyev merged commit ae96643 into feature/unlimited-assets Feb 22, 2022

tolikzinovyev deleted the tolik/unlimited-assets-import branch February 22, 2022 21:00

cce mentioned this pull request Feb 25, 2022

Feature: Add support for unlimited assets. #900

Merged

Ledger refactoring changes #870

Ledger refactoring changes #870

Conversation

tolikzinovyev commented Feb 7, 2022 • edited Loading

Summary

Test Plan

codecov-commenter commented Feb 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolikzinovyev commented Feb 18, 2022

winder commented Feb 18, 2022

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

tolikzinovyev commented Feb 7, 2022 •

edited

Loading

codecov-commenter commented Feb 8, 2022 •

edited

Loading