cleanup: remove redundant method LookupAccountDataByAddress #5216

icorderi · 2023-03-17T16:09:56Z

Summary

LookupAccountDataByAddress was only used in one place and was repeating the same logic as in LookupAccount.
The difference between the two was that LookupAccountDataByAddress returned raw bytes instead of decoding the account data.

LookupAccountDataByAddress was removed from the interface and the call site was changed to used LookupAccount instead.

Test Plan

Existing tests.

…ethod.

codecov · 2023-03-17T16:34:29Z

Codecov Report

Merging #5216 (ab2e201) into master (ca04d83) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

❗ Current head ab2e201 differs from pull request most recent head 7933c7d. Consider uploading reports for the commit 7933c7d to get more accurate results

@@            Coverage Diff             @@
##           master    #5216      +/-   ##
==========================================
- Coverage   53.56%   53.55%   -0.02%     
==========================================
  Files         441      441              
  Lines       55098    55079      -19     
==========================================
- Hits        29515    29497      -18     
+ Misses      23298    23292       -6     
- Partials     2285     2290       +5

Impacted Files	Coverage Δ
ledger/store/trackerdb/sqlitedriver/accountsV2.go	`13.79% <ø> (+0.24%)`	⬆️
ledger/acctdeltas.go	`79.53% <75.00%> (+0.33%)`	⬆️

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

gmalouf

LGTM - easy to follow thinking, clean.

algorandskiy · 2023-03-17T19:56:57Z

ledger/acctdeltas.go

-		switch err {
-		case nil:
-			if len(acctDataBuf) > 0 {
-				persistedAcctData := &trackerdb.PersistedAccountData{Addr: addr, Ref: ref}


there is still a difference. this code does not set Round but LookupAccount does set. I did not check if this is important or not, but I suggest to either check or clear it here

The Round is the round of the db at the point the "old value" was read.
The old code wasn't setting it because it wasn't querying it, since it didn't seem to care about what round the old value was from. LookupAccount reads the round, because it's part of the data structure being read, and it might not be used by this caller, but it is used by others.

This can't possible break code, because if any code was attempting to get meaning from the Round field in this "old value" entries, they all had Round(0), implying that all this old values were read at genesis, which is flat out wrong.

a problem could be in a code that uses Round=0 as some indication similarly to rowid=0. I'm pretty sure there is no such code, but have you checked? The PR description claimed the code is equivalent but it is not for this case and for the empty data as discussed in a thread below.

Hello, isn't the account data hashed by the catchpoint tracker, so wouldn't populating it with a new nonzero round number change that hash?

You are right Round might affect MT hash but we've checked all possible code paths and returning Round=0 not possible now.

Catchpoint tracker's AccountHashBuilderV6 uses the BaseAccountData.UpdateRound value, the round number of the change to this account. This is not the same thing as the PersistedAccountData.Round value which is the current round that the DB was synced up to at the time of the SQL query lookup.

algorandskiy · 2023-03-21T17:07:40Z

I think we need a test confirming the loadOld clients do not use Rounds in their logic before merging it.

ledger/acctdeltas.go

jsgranados · 2023-03-21T20:26:49Z

Ignacio and Pavel to follow up

ledger/acctdeltas.go

cce

Agreed with Pavel as we both can recall subtle unexpected changes to resourcesLoadOld and accountsLoadOld leading to serious bugs before, this PR would feel safer if it either (1) exactly matched what the behavior of old code paths it is replacing does, or (2) had some kind of test to show the 3 different cases it is merging into 1 have the same impact to commitRound.

algorandskiy · 2023-04-05T19:17:38Z

I reexamined code paths and it appears even an empty/nil accounts in accountbase would not cause any problem for resources because resources update would resolve address to rowid but such loaded account (really rowid only) does not appears in modification lists => no writes, no caching issues.

It might cause a problem with transferring to an empty account since the new code would assume there is no such account and transfer cannot be recorded.

The PR to clean up possible empty accounts appears to be a fix.

algorandskiy · 2023-04-06T00:25:48Z

Here is a demo making an empty account and transferring to it. DB will fail to commit b/c of unique constraint error master...algorandskiy:go-algorand:pavel/empty-acct-test-demo

Here is a demo making an empty account and transferring to it. DB will fail to commit b/c of unique constraint error master...algorandskiy:go-algorand:pavel/empty-acct-test-demo

algorandskiy

Please change the address logging %v -> %s - we use canonical string addresses almost everywhere outside the code.
LGTM. The reason why I approve this is the resources migration converted all possible old empty/nil accounts, and no such records should be in the DB (that existed at the very early days).

ledger/store/trackerdb/sqlitedriver/sql.go

…aseAccountData

algorandskiy · 2023-04-06T20:31:07Z

@cce please take another look.
There are two write paths for accountbase: accountsNewRound and catchpoints. The former one serialized data. The latter writes pre-serialized data but it decodes before, so no empty values possible.

ledger/store/trackerdb/data_test.go

cce · 2023-06-02T19:00:20Z

ledger/acctdeltas.go

 			// unexpected error - let the caller know that we couldn't complete the operation.
 			return err
 		}
+		// update the account
+		a.updateOld(idx, data)


The only remaining change I see between the old code and this new version is that we didn't use to fill in oldAcct.Round (the PersistedAccountData.Round field) and now we are filling it in with the Round of the oldAcct.

Before the old code would only fill in Addr, Ref, and AccountData but Round was not available from the old LookupAccountDataByAddress query so was not set for oldAcct.

I can't find anywhere where oldAcct.Round is used though ... oldAcct is being used for Ref (to set on newAcct's Ref) and in the onlineAccountsNewRoundImpl there is a check for oldAcct.AccountData != newAcct but Round does not seem used.

Agreed, old Round is not used anywhere

cce

After clearing up the comments and adding the extra err return case to LookupAccount ... the only remaining change I see between the old code and this new version is that we didn't used to fill in oldAcct.Round (the PersistedAccountData.Round field) and now we are filling it in with the Round of the oldAcct (the old code would only fill in Addr, Ref, and AccountData).

I can't find anywhere where oldAcct.Round is used though ... oldAcct is being used for Ref (to set on newAcct's Ref) and in the onlineAccountsNewRoundImpl there is a check for oldAcct.AccountData != newAcct but Round does not seem used.

related: unneeded PR #5267

bbroder-algo · 2023-06-02T19:24:57Z

what about here:
https://github.com/icorderi/go-algorand/blob/refactor/remove-LookupAccountDataByAddress/ledger/store/trackerdb/hashing.go#L131

algorandskiy · 2023-06-02T19:26:45Z

You mean MT deleting here, right?

		delta := accountsDeltas.getByIdx(i)
		if !delta.oldAcct.AccountData.IsEmpty() {
			deleteHash := trackerdb.AccountHashBuilderV6(delta.address, &delta.oldAcct.AccountData, protocol.Encode(&delta.oldAcct.AccountData))
			deleted, err := ct.balancesTrie.Delete(deleteHash)

bbroder-algo · 2023-06-02T19:31:21Z

confirmed that newAcct/oldAcct hashing is using BaseOnlineAccountData part of PersistedOnlineAccountData and not the round.

cleanup: remove redundant method LookupAccountDataByAddress

a6ff0f7

icorderi added Team Carbon-11 Skip-Release-Notes labels Mar 17, 2023

icorderi self-assigned this Mar 17, 2023

refactor: upgrade MakeAccountsOptimizedReader from a testing only m…

ab2e201

…ethod.

icorderi requested review from AlgoAxel and algorandskiy March 17, 2023 17:18

AlgoAxel previously approved these changes Mar 17, 2023

View reviewed changes

gmalouf previously approved these changes Mar 17, 2023

View reviewed changes

algorandskiy reviewed Mar 17, 2023

View reviewed changes

cce reviewed Mar 21, 2023

View reviewed changes

ledger/acctdeltas.go Show resolved Hide resolved

cce reviewed Mar 21, 2023

View reviewed changes

ledger/acctdeltas.go Show resolved Hide resolved

cce requested changes Mar 21, 2023

View reviewed changes

algorandskiy previously approved these changes Apr 5, 2023

View reviewed changes

algorandskiy self-requested a review April 5, 2023 20:02

cce mentioned this pull request Apr 6, 2023

ledger: Add LookupOldAccount to AccountsReader interface #5267

Closed

icorderi dismissed stale reviews from gmalouf and AlgoAxel via 0a0e779 April 6, 2023 19:54

algorandskiy reviewed Apr 6, 2023

View reviewed changes

ledger/store/trackerdb/sqlitedriver/sql.go Outdated Show resolved Hide resolved

misc: log if we run into an empty account

2a65b33

icorderi force-pushed the refactor/remove-LookupAccountDataByAddress branch from 0a0e779 to 2a65b33 Compare April 6, 2023 20:01

Add a test ensuring nil/empty value cannot be decoded into an empty B…

4b667dd

…aseAccountData

algorandskiy previously approved these changes Apr 6, 2023

View reviewed changes

icorderi requested a review from cce April 6, 2023 20:50

cce reviewed Apr 12, 2023

View reviewed changes

ledger/store/trackerdb/data_test.go Show resolved Hide resolved

fix linter

10187d4

algorandskiy dismissed their stale review via 10187d4 April 18, 2023 19:31

algorandskiy previously approved these changes Apr 18, 2023

View reviewed changes

algorandskiy mentioned this pull request Apr 18, 2023

feat: remove any potential accounts with 0-byte len #5258

Closed

review: error on empty account lookup

7933c7d

algorandskiy dismissed their stale review via 7933c7d April 18, 2023 20:43

algorandskiy requested review from cce, algorandskiy and AlgoAxel April 18, 2023 20:44

algorandskiy approved these changes May 8, 2023

View reviewed changes

icorderi mentioned this pull request May 9, 2023

kv: add StorageEngine config #5369

Merged

cce reviewed Jun 2, 2023

View reviewed changes

cce approved these changes Jun 2, 2023

View reviewed changes

bbroder-algo merged commit 7d8525e into algorand:master Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup: remove redundant method LookupAccountDataByAddress #5216

cleanup: remove redundant method LookupAccountDataByAddress #5216

icorderi commented Mar 17, 2023

codecov bot commented Mar 17, 2023 •

edited

Loading

gmalouf left a comment

algorandskiy Mar 17, 2023

icorderi Mar 21, 2023

algorandskiy Mar 22, 2023

bbroder-algo May 9, 2023

algorandskiy Jun 1, 2023

cce Jun 2, 2023

algorandskiy commented Mar 21, 2023 •

edited

Loading

jsgranados commented Mar 21, 2023 •

edited

Loading

cce left a comment

algorandskiy commented Apr 5, 2023 •

edited

Loading

algorandskiy commented Apr 6, 2023

algorandskiy left a comment

algorandskiy commented Apr 6, 2023

cce Jun 2, 2023 •

edited

Loading

algorandskiy Jun 2, 2023

cce left a comment •

edited

Loading

bbroder-algo commented Jun 2, 2023

algorandskiy commented Jun 2, 2023

bbroder-algo commented Jun 2, 2023

cleanup: remove redundant method LookupAccountDataByAddress #5216

cleanup: remove redundant method LookupAccountDataByAddress #5216

Conversation

icorderi commented Mar 17, 2023

Summary

Test Plan

codecov bot commented Mar 17, 2023 • edited Loading

Codecov Report

gmalouf left a comment

Choose a reason for hiding this comment

algorandskiy Mar 17, 2023

Choose a reason for hiding this comment

icorderi Mar 21, 2023

Choose a reason for hiding this comment

algorandskiy Mar 22, 2023

Choose a reason for hiding this comment

bbroder-algo May 9, 2023

Choose a reason for hiding this comment

algorandskiy Jun 1, 2023

Choose a reason for hiding this comment

cce Jun 2, 2023

Choose a reason for hiding this comment

algorandskiy commented Mar 21, 2023 • edited Loading

jsgranados commented Mar 21, 2023 • edited Loading

cce left a comment

Choose a reason for hiding this comment

algorandskiy commented Apr 5, 2023 • edited Loading

algorandskiy commented Apr 6, 2023

algorandskiy left a comment

Choose a reason for hiding this comment

algorandskiy commented Apr 6, 2023

cce Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

algorandskiy Jun 2, 2023

Choose a reason for hiding this comment

cce left a comment • edited Loading

Choose a reason for hiding this comment

bbroder-algo commented Jun 2, 2023

algorandskiy commented Jun 2, 2023

bbroder-algo commented Jun 2, 2023

codecov bot commented Mar 17, 2023 •

edited

Loading

algorandskiy commented Mar 21, 2023 •

edited

Loading

jsgranados commented Mar 21, 2023 •

edited

Loading

algorandskiy commented Apr 5, 2023 •

edited

Loading

cce Jun 2, 2023 •

edited

Loading

cce left a comment •

edited

Loading