Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

ledger: increase locks granularity in lookupWithoutRewards #5637

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions ledger/acctupdates.go
Original file line number Diff line number Diff line change
Expand Up @@ -1321,23 +1321,29 @@
return
}

deltas := au.deltas[:offset]
rewardsVersion = au.versions[offset]
rewardsLevel = au.roundTotals[offset].RewardsLevel

// check if we've had this address modified in the past rounds. ( i.e. if it's in the deltas )
macct, indeltas := au.accounts[addr]
if synchronized {
au.accountsMu.RUnlock()
needUnlock = false
}

if indeltas {
// Check if this is the most recent round, in which case, we can
// use a cache of the most recent account state.
if offset == uint64(len(au.deltas)) {
if offset == uint64(currentDeltaLen) {
return macct.data, rnd, rewardsVersion, rewardsLevel, nil
}
// the account appears in the deltas, but we don't know if it appears in the
// delta range of [0..offset-1], so we'll need to check. Walk deltas
// backwards to ensure that later updates take priority if present.
for offset > 0 {
offset--
d, ok := au.deltas[offset].Accts.GetData(addr)
d, ok := deltas[offset].Accts.GetData(addr)
if ok {
// the returned validThrough here is not optimal, but it still correct. We could get a more accurate value by scanning
// the deltas forward, but this would be time consuming loop, which might not pay off.
Expand All @@ -1352,17 +1358,27 @@
rnd = currentDbRound + basics.Round(currentDeltaLen)
}

if synchronized {
au.accountsMu.RLock()
needUnlock = true
}
// check the baseAccounts -
if macct, has := au.baseAccounts.read(addr); has {
// there is a chance the db advanced between deltas check and the baseAccounts check
// ensure the round requested is still in the db
if macct.Round > rnd {
return ledgercore.AccountData{}, 0, "", 0, &RoundOffsetError{rnd, macct.Round}

Check warning on line 1370 in ledger/acctupdates.go

View check run for this annotation

Codecov / codecov/patch

ledger/acctupdates.go#L1370

Added line #L1370 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think it is worth taking an optimistic (instead of preventing an error here with locks), and returning an error when the database advances?
There are operation that need to get canceled because an error is returned from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only actual for rest api since txpool and block validator need the latest state of account to validate.

}

// we don't technically need this, since it's already in the baseAccounts, however, writing this over
// would ensure that we promote this field.
au.baseAccounts.writePending(macct)
return macct.AccountData.GetLedgerCoreAccountData(), rnd, rewardsVersion, rewardsLevel, nil
}

// check baseAccoiunts again to see if it does not exist
// check baseAccounts again to see if it does not exist
if au.baseAccounts.readNotFound(addr) {
// it seems the account doesnt exist
// it seems the account does not exist
return ledgercore.AccountData{}, rnd, rewardsVersion, rewardsLevel, nil
}

Expand Down
4 changes: 2 additions & 2 deletions ledger/acctupdates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ func testAcctUpdatesUpdatesCorrectness(t *testing.T, cfg config.Local) {
fromAccountDataOld, validThrough, err := au.LookupWithoutRewards(i-1, fromAccount)
require.NoError(t, err)
require.Equal(t, i-1, validThrough)
require.Equalf(t, moneyAccountsExpectedAmounts[i-1][j], fromAccountDataOld.MicroAlgos.Raw, "Account index : %d\nRound number : %d", j, i)
require.Equalf(t, int(moneyAccountsExpectedAmounts[i-1][j]), int(fromAccountDataOld.MicroAlgos.Raw), "Account index : %d (%s)\nRound number : %d (%d)", j, moneyAccounts[j], i, i-1)

fromAccountDataNew := fromAccountDataOld

Expand Down Expand Up @@ -869,7 +869,7 @@ func testAcctUpdatesUpdatesCorrectness(t *testing.T, cfg config.Local) {
require.NoError(t, err)
require.GreaterOrEqual(t, int64(validThrough), int64(basics.Round(checkRound-uint64(testback))))
// if we received no error, we want to make sure the reported amount is correct.
require.Equalf(t, moneyAccountsExpectedAmounts[checkRound-uint64(testback)][j], acct.MicroAlgos.Raw, "Account index : %d\nRound number : %d", j, checkRound)
require.Equalf(t, int(moneyAccountsExpectedAmounts[checkRound-uint64(testback)][j]), int(acct.MicroAlgos.Raw), "Account index : %d (%s)\nRound number : %d (%d)", j, moneyAccounts[j], checkRound, checkRound-uint64(testback))
testback++
j--
}
Expand Down
Loading