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.

Sign up for GitHub

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

fix: accaddr cachefix #15433

Merged
merged 15 commits into from
Mar 31, 2023
Merged

fix: accaddr cachefix #15433

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a81b41b
bugfix for address caching
KyleMoser Mar 9, 2023
9a47a18
bugfix for address caching
KyleMoser Mar 9, 2023
50a1aa2
bugfix for address caching
KyleMoser Mar 9, 2023
fd5e93d
Test case for disabling bech32 cache
KyleMoser Mar 16, 2023
6fe9d1c
bugfix for address caching
KyleMoser Mar 16, 2023
1ec7c3d
Changelog entry
KyleMoser Mar 22, 2023
58c6259
Changelog entry
KyleMoser Mar 22, 2023
7b80e5b
Merge branch 'main' into kamoser/15317-acccache
julienrbrt Mar 22, 2023
ab3c459
mutex for cache enabled flag
KyleMoser Mar 23, 2023
0b9787b
Merge branch 'kamoser/15317-acccache' of ssh://github.com/KyleMoser/c…
KyleMoser Mar 23, 2023
02b5116
synchronization for address cache
KyleMoser Mar 23, 2023
cd44a3d
Merge branch 'main' into kamoser/15317-acccache
tac0turtle Mar 31, 2023
c4b64b5
Merge branch 'main' into kamoser/15317-acccache
julienrbrt Mar 31, 2023
407df3a
better naming and godoc
julienrbrt Mar 31, 2023
df63851
Merge branch 'main' into kamoser/15317-acccache
julienrbrt Mar 31, 2023
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

## [Unreleased]

* (types) [#15433](https://github.com/cosmos/cosmos-sdk/pull/15433) Allow disabling of account address caches (for printing bech32 account addresses).
Copy link
Member

Choose a reason for hiding this comment

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

The changelog should be under the bugfix or improvement section (whichever you think fits best).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it there. Am I finally doing it right? :)


### Features

* (x/bank) [#15265](https://github.com/cosmos/cosmos-sdk/pull/15265) Update keeper interface to include `GetAllDenomMetaData`.
Expand Down
55 changes: 39 additions & 16 deletions types/address.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a changelog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks. Missed that in the contributor guide, my bad.

Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ var (
consAddrCache *simplelru.LRU
valAddrMu sync.Mutex
valAddrCache *simplelru.LRU

isCachingEnabled bool
)

// sentinel errors
Expand All @@ -94,6 +96,8 @@ var (

func init() {
var err error
isCachingEnabled = true

// in total the cache size is 61k entries. Key is 32 bytes and value is around 50-70 bytes.
// That will make around 92 * 61k * 2 (LRU) bytes ~ 11 MB
if accAddrCache, err = simplelru.NewLRU(60000, nil); err != nil {
Expand All @@ -107,6 +111,11 @@ func init() {
}
}

// By default, caches are enabled. This enables or disables accAddrCache, consAddrCache, and valAddrCache.
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
func SetAddrCacheEnabled(enabled bool) {
isCachingEnabled = enabled
}
Copy link

Choose a reason for hiding this comment

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

It's unsafe to mutate a global variable like this without synchronization (e.g. a mutex).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only meant to be used once on client startup. If I put a mutex in SetAddrCacheEnabled it should also be locked when checking isCachingEnabled, meaning every time an address bech32 conversion happens.

Copy link

Choose a reason for hiding this comment

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

You're correct that both reads and writes need to be synchronized. You might consider atomic.Bool as an alternative.

var cachingEnabled atomic.Bool

func SetCachingEnabled(b bool) { cachingEnabled.Store(b) }
func GetCachingEnabled() bool { return cachingEnabled.Load() }


// Address is a common interface for different types of addresses used by the SDK
type Address interface {
Equals(Address) bool
Expand Down Expand Up @@ -287,11 +296,15 @@ func (aa AccAddress) String() string {
}

key := conv.UnsafeBytesToStr(aa)
accAddrMu.Lock()
defer accAddrMu.Unlock()
addr, ok := accAddrCache.Get(key)
if ok {
return addr.(string)

if isCachingEnabled {
accAddrMu.Lock()
defer accAddrMu.Unlock()

addr, ok := accAddrCache.Get(key)
if ok {
return addr.(string)
}
}
return cacheBech32Addr(GetConfig().GetBech32AccountAddrPrefix(), aa, accAddrCache, key)
}
Expand Down Expand Up @@ -437,11 +450,15 @@ func (va ValAddress) String() string {
}

key := conv.UnsafeBytesToStr(va)
valAddrMu.Lock()
defer valAddrMu.Unlock()
addr, ok := valAddrCache.Get(key)
if ok {
return addr.(string)

if isCachingEnabled {
valAddrMu.Lock()
defer valAddrMu.Unlock()

addr, ok := valAddrCache.Get(key)
if ok {
return addr.(string)
}
}
return cacheBech32Addr(GetConfig().GetBech32ValidatorAddrPrefix(), va, valAddrCache, key)
}
Expand Down Expand Up @@ -592,11 +609,15 @@ func (ca ConsAddress) String() string {
}

key := conv.UnsafeBytesToStr(ca)
consAddrMu.Lock()
defer consAddrMu.Unlock()
addr, ok := consAddrCache.Get(key)
if ok {
return addr.(string)

if isCachingEnabled {
consAddrMu.Lock()
defer consAddrMu.Unlock()

addr, ok := consAddrCache.Get(key)
if ok {
return addr.(string)
}
}
return cacheBech32Addr(GetConfig().GetBech32ConsensusAddrPrefix(), ca, consAddrCache, key)
}
Expand Down Expand Up @@ -676,6 +697,8 @@ func cacheBech32Addr(prefix string, addr []byte, cache *simplelru.LRU, cacheKey
if err != nil {
panic(err)
}
cache.Add(cacheKey, bech32Addr)
if isCachingEnabled {
cache.Add(cacheKey, bech32Addr)
}
return bech32Addr
}
71 changes: 71 additions & 0 deletions types/address_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,77 @@ func (s *addressTestSuite) TestRandBech32AccAddrConsistency() {
s.Require().Equal(types.ErrEmptyHexAddress, err)
}

// Test that the account address cache ignores the bech32 prefix setting, retrieving bech32 addresses from the cache.
// This will cause the AccAddress.String() to print out unexpected prefixes if the config was changed between bech32 lookups.
// See https://github.com/cosmos/cosmos-sdk/issues/15317.
func (s *addressTestSuite) TestAddrCache() {
// Use a random key
pubBz := make([]byte, ed25519.PubKeySize)
pub := &ed25519.PubKey{Key: pubBz}
rand.Read(pub.Key)

// Set SDK bech32 prefixes to 'osmo'
prefix := "osmo"
conf := types.GetConfig()
conf.SetBech32PrefixForAccount(prefix, prefix+"pub")
conf.SetBech32PrefixForValidator(prefix+"valoper", prefix+"valoperpub")
conf.SetBech32PrefixForConsensusNode(prefix+"valcons", prefix+"valconspub")

acc := types.AccAddress(pub.Address())
osmoAddrBech32 := acc.String()

// Set SDK bech32 to 'cosmos'
prefix = "cosmos"
conf.SetBech32PrefixForAccount(prefix, prefix+"pub")
conf.SetBech32PrefixForValidator(prefix+"valoper", prefix+"valoperpub")
conf.SetBech32PrefixForConsensusNode(prefix+"valcons", prefix+"valconspub")

// We name this 'addrCosmos' to prove a point, but the bech32 address will still begin with 'osmo' due to the cache behavior.
addrCosmos := types.AccAddress(pub.Address())
cosmosAddrBech32 := addrCosmos.String()

// The default behavior will retrieve the bech32 address from the cache, ignoring the bech32 prefix change.
s.Require().Equal(osmoAddrBech32, cosmosAddrBech32)
s.Require().True(strings.HasPrefix(osmoAddrBech32, "osmo"))
s.Require().True(strings.HasPrefix(cosmosAddrBech32, "osmo"))
}

// Test that the bech32 prefix is respected when the address cache is disabled.
// This causes AccAddress.String() to print out the expected prefixes if the config is changed between bech32 lookups.
// See https://github.com/cosmos/cosmos-sdk/issues/15317.
func (s *addressTestSuite) TestAddrCacheDisabled() {
types.SetAddrCacheEnabled(false)

// Use a random key
pubBz := make([]byte, ed25519.PubKeySize)
pub := &ed25519.PubKey{Key: pubBz}
rand.Read(pub.Key)

// Set SDK bech32 prefixes to 'osmo'
prefix := "osmo"
conf := types.GetConfig()
conf.SetBech32PrefixForAccount(prefix, prefix+"pub")
conf.SetBech32PrefixForValidator(prefix+"valoper", prefix+"valoperpub")
conf.SetBech32PrefixForConsensusNode(prefix+"valcons", prefix+"valconspub")

acc := types.AccAddress(pub.Address())
osmoAddrBech32 := acc.String()

// Set SDK bech32 to 'cosmos'
prefix = "cosmos"
conf.SetBech32PrefixForAccount(prefix, prefix+"pub")
conf.SetBech32PrefixForValidator(prefix+"valoper", prefix+"valoperpub")
conf.SetBech32PrefixForConsensusNode(prefix+"valcons", prefix+"valconspub")

addrCosmos := types.AccAddress(pub.Address())
cosmosAddrBech32 := addrCosmos.String()

// retrieve the bech32 address from the cache, respecting the bech32 prefix change.
s.Require().NotEqual(osmoAddrBech32, cosmosAddrBech32)
s.Require().True(strings.HasPrefix(osmoAddrBech32, "osmo"))
s.Require().True(strings.HasPrefix(cosmosAddrBech32, "cosmos"))
}

func (s *addressTestSuite) TestValAddr() {
pubBz := make([]byte, ed25519.PubKeySize)
pub := &ed25519.PubKey{Key: pubBz}
Expand Down