Skip to content

Commit

Permalink
Make HashKind an explicit concept
Browse files Browse the repository at this point in the history
  • Loading branch information
@michaeldiamant
michaeldiamant committed Nov 17, 2022
1 parent 1bf1861 commit aeb4f19
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 43 deletions.
4 changes: 2 additions & 2 deletions catchup/catchpointService.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,10 +344,10 @@ func (cs *CatchpointCatchupService) updateVerifiedCounts(hashes [][]byte) {
cs.statsMu.Lock()
defer cs.statsMu.Unlock()

addedTrieAccountHashes := uint64(0)
addedTrieAccountHashes := uint64(0) // Accounts include accounts + creatables (assets + apps)
addedTrieKVHashes := uint64(0)
for _, hash := range hashes {
if hash[4] == 3 { // KV
if hash[ledger.HashKindEncodingIndex] == byte(ledger.KV) {
addedTrieKVHashes++
} else {
addedTrieAccountHashes++
Expand Down
21 changes: 5 additions & 16 deletions ledger/accountdb.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -478,15 +478,10 @@ func prepareNormalizedBalancesV6(bals []encodedBalanceRecordV6, proto config.Con
if err != nil {
return nil, err
}
var ctype basics.CreatableType
if resData.IsAsset() {
ctype = basics.AssetCreatable
} else if resData.IsApp() {
ctype = basics.AppCreatable
} else {
err = fmt.Errorf("unknown creatable for addr %s, aidx %d, data %v", balance.Address.String(), cidx, resData)
normalizedAccountBalances[i].accountHashes[curHashIdx], err = resourcesHashBuilderV6(resData, balance.Address, basics.CreatableIndex(cidx), resData.UpdateRound, res)
if err != nil {
return nil, err
}
normalizedAccountBalances[i].accountHashes[curHashIdx] = resourcesHashBuilderV6(balance.Address, basics.CreatableIndex(cidx), ctype, resData.UpdateRound, res)
normalizedAccountBalances[i].resources[basics.CreatableIndex(cidx)] = resData
normalizedAccountBalances[i].encodedResources[basics.CreatableIndex(cidx)] = res
curHashIdx++
Expand Down Expand Up @@ -4821,16 +4816,10 @@ func (iterator *orderedAccountsIter) Next(ctx context.Context) (acct []accountAd
resCb := func(addr basics.Address, cidx basics.CreatableIndex, resData *resourcesData, encodedResourceData []byte, lastResource bool) error {
var err error
if resData != nil {
var ctype basics.CreatableType
if resData.IsAsset() {
ctype = basics.AssetCreatable
} else if resData.IsApp() {
ctype = basics.AppCreatable
} else {
err = fmt.Errorf("unknown creatable for addr %s, aidx %d, data %v", addr.String(), cidx, resData)
hash, err := resourcesHashBuilderV6(*resData, addr, cidx, resData.UpdateRound, encodedResourceData)
if err != nil {
return err
}
hash := resourcesHashBuilderV6(addr, cidx, ctype, resData.UpdateRound, encodedResourceData)
_, err = iterator.insertStmt.ExecContext(ctx, lastAddrID, hash)
}
return err
Expand Down
68 changes: 44 additions & 24 deletions ledger/catchpointtracker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,15 +971,10 @@ func (ct *catchpointTracker) accountsUpdateBalances(accountsDeltas compactAccoun
resDelta := resourcesDeltas.getByIdx(i)
addr := resDelta.address
if !resDelta.oldResource.data.IsEmpty() {
var ctype basics.CreatableType
if resDelta.oldResource.data.IsAsset() {
ctype = basics.AssetCreatable
} else if resDelta.oldResource.data.IsApp() {
ctype = basics.AppCreatable
} else {
return fmt.Errorf("unknown old creatable for addr %s (%d), aidx %d, data %v", addr.String(), resDelta.oldResource.addrid, resDelta.oldResource.aidx, resDelta.oldResource.data)
deleteHash, err := resourcesHashBuilderV6(resDelta.oldResource.data, addr, resDelta.oldResource.aidx, resDelta.oldResource.data.UpdateRound, protocol.Encode(&resDelta.oldResource.data))
if err != nil {
return err
}
deleteHash := resourcesHashBuilderV6(addr, resDelta.oldResource.aidx, ctype, uint64(resDelta.oldResource.data.UpdateRound), protocol.Encode(&resDelta.oldResource.data))
deleted, err = ct.balancesTrie.Delete(deleteHash)
if err != nil {
return fmt.Errorf("failed to delete resource hash '%s' from merkle trie for account %v: %w", hex.EncodeToString(deleteHash), addr, err)
Expand All @@ -992,15 +987,10 @@ func (ct *catchpointTracker) accountsUpdateBalances(accountsDeltas compactAccoun
}

if !resDelta.newResource.IsEmpty() {
var ctype basics.CreatableType
if resDelta.newResource.IsAsset() {
ctype = basics.AssetCreatable
} else if resDelta.newResource.IsApp() {
ctype = basics.AppCreatable
} else {
return fmt.Errorf("unknown new creatable for addr %s, aidx %d, data %v", addr.String(), resDelta.oldResource.aidx, resDelta.newResource)
addHash, err := resourcesHashBuilderV6(resDelta.newResource, addr, resDelta.oldResource.aidx, resDelta.newResource.UpdateRound, protocol.Encode(&resDelta.newResource))
if err != nil {
return err
}
addHash := resourcesHashBuilderV6(addr, resDelta.oldResource.aidx, ctype, uint64(resDelta.newResource.UpdateRound), protocol.Encode(&resDelta.newResource))
added, err = ct.balancesTrie.Add(addHash)
if err != nil {
return fmt.Errorf("attempted to add duplicate resource hash '%s' to merkle trie for account %v: %w", hex.EncodeToString(addHash), addr, err)
Expand Down Expand Up @@ -1418,7 +1408,7 @@ func removeSingleCatchpointFileFromDisk(dbDirectory, fileToDelete string) (err e
return nil
}

func hashBufV6(affinity uint64, kind byte) []byte {
func hashBufV6(affinity uint64, kind HashKind) []byte {
hash := make([]byte, 4+crypto.DigestSize)
// write out the lowest 32 bits of the affinity value. This should improve
// the caching of the trie by allowing recent updates to be in-cache, and
Expand All @@ -1427,7 +1417,7 @@ func hashBufV6(affinity uint64, kind byte) []byte {
// the following takes the prefix & 255 -> hash[i]
hash[i] = byte(prefix)
}
hash[4] = kind
hash[HashKindEncodingIndex] = byte(kind)
return hash
}

Expand All @@ -1444,7 +1434,7 @@ func accountHashBuilderV6(addr basics.Address, accountData *baseAccountData, enc
if hashIntPrefix == 0 {
hashIntPrefix = accountData.RewardsBase
}
hash := hashBufV6(hashIntPrefix, 0) // 0 indicates an account
hash := hashBufV6(hashIntPrefix, Account)
// write out the lowest 32 bits of the reward base. This should improve the caching of the trie by allowing
// recent updated to be in-cache, and "older" nodes will be left alone.

Expand All @@ -1455,21 +1445,51 @@ func accountHashBuilderV6(addr basics.Address, accountData *baseAccountData, enc
return finishV6(hash, prehash)
}

// accountHashBuilderV6 calculates the hash key used for the trie by combining the account address and the account data
func resourcesHashBuilderV6(addr basics.Address, cidx basics.CreatableIndex, ctype basics.CreatableType, updateRound uint64, encodedResourceData []byte) []byte {
hash := hashBufV6(updateRound, byte(ctype+1)) // one or two ( asset / application ) so we could differentiate the hashes.
// HashKind enumerates the possible data types hashed into a catchpoint merkle
// trie. Each merkle trie hash includes the HashKind byte at a known-offset.
// By encoding HashKind at a known-offset, it's possible for hash readers to
// disambiguate the hashed resource.
//go:generate stringer -type=HashKind
type HashKind byte

const (
Account HashKind = iota
Asset
App
KV
)

const HashKindEncodingIndex = 4

func creatableHashKindFromResourcesData(rd resourcesData, a basics.Address, ci basics.CreatableIndex) (HashKind, error) {
if rd.IsAsset() {
return Asset, nil
} else if rd.IsApp() {
return App, nil
}
return Account, fmt.Errorf("unknown creatable for addr %s, aidx %d, data %v", a.String(), ci, rd)
}

// resourcesHashBuilderV6 calculates the hash key used for the trie by combining the creatable's resource data and its index
func resourcesHashBuilderV6(rd resourcesData, addr basics.Address, cidx basics.CreatableIndex, updateRound uint64, encodedResourceData []byte) ([]byte, error) {
hk, err := creatableHashKindFromResourcesData(rd, addr, cidx)
if err != nil {
return nil, err
}

hash := hashBufV6(updateRound, hk)

prehash := make([]byte, 8+crypto.DigestSize+len(encodedResourceData))
copy(prehash[:], addr[:])
binary.LittleEndian.PutUint64(prehash[crypto.DigestSize:], uint64(cidx))
copy(prehash[crypto.DigestSize+8:], encodedResourceData[:])

return finishV6(hash, prehash)
return finishV6(hash, prehash), nil
}

// kvHashBuilderV6 calculates the hash key used for the trie by combining the key and value
func kvHashBuilderV6(key string, value []byte) []byte {
hash := hashBufV6(0, 3) // 3 indicates a kv pair
hash := hashBufV6(0, KV)

prehash := make([]byte, len(key)+len(value))
copy(prehash[:], key)
Expand Down
2 changes: 1 addition & 1 deletion ledger/catchupaccessor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ func (c *catchpointCatchupAccessorImpl) BuildMerkleTrie(ctx context.Context, pro
var added bool
added, err = trie.Add(hash)
if !added {
return fmt.Errorf("CatchpointCatchupAccessorImpl::BuildMerkleTrie: The provided catchpoint file contained the same account more than once. hash '%s'", hex.EncodeToString(hash))
return fmt.Errorf("CatchpointCatchupAccessorImpl::BuildMerkleTrie: The provided catchpoint file contained the same account more than once. hash = '%s' hash kind = %s", hex.EncodeToString(hash), HashKind(hash[HashKindEncodingIndex]))

This comment has been minimized.

Sign in to view

Copy link
@jannotti

jannotti Nov 17, 2022

Contributor

I like the HashKind change except: It probably should be hashKind (and the constants also not exported), and I'd feel better about it if it didn't use iota, and used direct assignment of the existing numbers. I assume you got it right, but since they are a sort of external interface, I'd rather be explicit. (But I could easily be convinced that, having confirmed that they are the same, it's now nicer with iota)

This comment has been minimized.

Sign in to view

Copy link
@michaeldiamant

michaeldiamant Nov 17, 2022

Author Contributor

@jannotti Responding inline:

It probably should be hashKind (and the constants also not exported),

I switched to hashKind, but KV is referenced in another package, so I think the values should still be exported.

if it didn't use iota, and used direct assignment of the existing numbers.

  • I had thought assignment as shown is less error prone. Ultimately, I don't feel strongly and if after the note you still prefer manual assignment, I'll do it.
  • Surprisingly, I don't think there's a test in place to catch changing an existing ordinal value. I'll add one if I don't find something.
}
if err != nil {
return
Expand Down
26 changes: 26 additions & 0 deletions ledger/hashkind_string.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit aeb4f19

Please sign in to comment.