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

Switch to lazy state balance cache #9822

Merged
merged 34 commits into from
Nov 19, 2021
Merged

Switch to lazy state balance cache #9822

Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
594a853
quick lazy balance cache proof of concept
kasey Oct 25, 2021
628d2f3
WIP refactoring to use lazy cache
kasey Oct 26, 2021
f29d0d8
updating tests to use functional opts
kasey Oct 26, 2021
3c71522
updating the rest of the tests, all passing
kasey Oct 27, 2021
5499b9f
use mock stategen where possible
kasey Oct 27, 2021
aa9004b
rename test opt method for clear link
kasey Oct 27, 2021
5832ae4
Update beacon-chain/blockchain/process_block.go
kasey Oct 27, 2021
ca992c9
test assumption that zerohash is in db
kasey Oct 28, 2021
151283d
remove unused MockDB (mocking stategen instead)
kasey Nov 1, 2021
95da658
Merge branch 'lazy-cache-poc' of github.com:prysmaticlabs/prysm into …
kasey Nov 1, 2021
9e9622d
fix cache bug, switch to sync.Mutex
kasey Nov 1, 2021
e308933
improve test coverage for the state cache
kasey Nov 2, 2021
b5f56b4
uncomment failing genesis test for discussion
kasey Nov 2, 2021
54768dc
Merge branch 'develop' into lazy-cache-poc
kasey Nov 4, 2021
748bfd3
gofmt
kasey Nov 9, 2021
067eba9
remove unused Service struct member
kasey Nov 9, 2021
a18f5b6
cleanup unused func input
kasey Nov 9, 2021
868bc0e
combining type declaration in signature
kasey Nov 9, 2021
a89e0f5
don't export the state cache constructor
kasey Nov 9, 2021
ff41c28
work around blockchain deps w/ new file
kasey Nov 9, 2021
54607c3
gofmt
kasey Nov 9, 2021
6fbb6f6
remove intentionally failing test
kasey Nov 10, 2021
3acc951
Merge branch 'develop' into lazy-cache-poc
kasey Nov 10, 2021
5451054
fixed error introduced by develop refresh
kasey Nov 10, 2021
a4a00f2
fix import ordering
kasey Nov 10, 2021
63bf9f9
appease deepsource
kasey Nov 10, 2021
aededf8
remove unused function
kasey Nov 10, 2021
4af1335
Merge branch 'develop' into lazy-cache-poc
terencechain Nov 10, 2021
44eebee
Merge branch 'develop' into lazy-cache-poc
kasey Nov 17, 2021
fa5f07e
godoc comments on new requires/assert
kasey Nov 17, 2021
56e9445
defensive constructor per terence's PR comment
kasey Nov 17, 2021
bf8f9b6
more differentiated balance cache metric names
kasey Nov 17, 2021
75b6473
Merge branch 'develop' into lazy-cache-poc
rauljordan Nov 19, 2021
e4da535
Merge refs/heads/develop into lazy-cache-poc
prylabs-bulldozer[bot] Nov 19, 2021
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
3 changes: 3 additions & 0 deletions beacon-chain/blockchain/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ go_library(
"receive_attestation.go",
"receive_block.go",
"service.go",
"state_balance_cache.go",
"weak_subjectivity_checks.go",
],
importpath = "github.com/prysmaticlabs/prysm/beacon-chain/blockchain",
Expand Down Expand Up @@ -95,6 +96,7 @@ go_test(
"init_test.go",
"log_test.go",
"metrics_test.go",
"mock_test.go",
"process_attestation_test.go",
"process_block_test.go",
"receive_attestation_test.go",
Expand Down Expand Up @@ -141,6 +143,7 @@ go_test(
"chain_info_norace_test.go",
"checktags_test.go",
"init_test.go",
"mock_test.go",
"receive_block_test.go",
"service_norace_test.go",
],
Expand Down
55 changes: 0 additions & 55 deletions beacon-chain/blockchain/head.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/prysmaticlabs/prysm/beacon-chain/core/feed"
statefeed "github.com/prysmaticlabs/prysm/beacon-chain/core/feed/state"
"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
"github.com/prysmaticlabs/prysm/beacon-chain/core/time"
"github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/protoarray"
"github.com/prysmaticlabs/prysm/beacon-chain/state"
"github.com/prysmaticlabs/prysm/config/features"
Expand Down Expand Up @@ -42,9 +41,6 @@ func (s *Service) updateHead(ctx context.Context, balances []uint64) error {
// ensure head gets its best justified info.
if s.bestJustifiedCheckpt.Epoch > s.justifiedCheckpt.Epoch {
s.justifiedCheckpt = s.bestJustifiedCheckpt
if err := s.cacheJustifiedStateBalances(ctx, bytesutil.ToBytes32(s.justifiedCheckpt.Root)); err != nil {
return err
}
}

// Get head from the fork choice service.
Expand Down Expand Up @@ -273,57 +269,6 @@ func (s *Service) hasHeadState() bool {
return s.head != nil && s.head.state != nil
}

// This caches justified state balances to be used for fork choice.
func (s *Service) cacheJustifiedStateBalances(ctx context.Context, justifiedRoot [32]byte) error {
if err := s.cfg.BeaconDB.SaveBlocks(ctx, s.getInitSyncBlocks()); err != nil {
return err
}

s.clearInitSyncBlocks()

var justifiedState state.BeaconState
var err error
if justifiedRoot == s.genesisRoot {
Copy link
Member

Choose a reason for hiding this comment

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

Is this condition necessary in the new scheme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we have a cache miss and go back to stategen for the lookup, it will attempt to read the state from the db. If the root is equal to genesisRoot, I believe that we can assume it will be present in the database. correct? In other words I believe that get state by root using the genesis root is equivalent to calling db.GenesisState.

Copy link
Member

Choose a reason for hiding this comment

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

My instinct says this exists because for some reason at genesis both justifiedRoot and s.genesisRoot can be 0x000....

To be safe, maybe we should apply ensureRootNotZeros to c.stateGen.StateByRoot(ctx, justifiedRoot) in get ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot this detail -- stategen.StateByRoot internally checks for the zero hash and in that case loads from the DB:
https://github.com/prysmaticlabs/prysm/blob/develop/beacon-chain/state/stategen/getter.go#L45

Copy link
Member

Choose a reason for hiding this comment

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

Ah perfect. Thanks!

justifiedState, err = s.cfg.BeaconDB.GenesisState(ctx)
if err != nil {
return err
}
} else {
justifiedState, err = s.cfg.StateGen.StateByRoot(ctx, justifiedRoot)
if err != nil {
return err
}
}
if justifiedState == nil || justifiedState.IsNil() {
return errors.New("justified state can't be nil")
}

epoch := time.CurrentEpoch(justifiedState)

justifiedBalances := make([]uint64, justifiedState.NumValidators())
if err := justifiedState.ReadFromEveryValidator(func(idx int, val state.ReadOnlyValidator) error {
if helpers.IsActiveValidatorUsingTrie(val, epoch) {
justifiedBalances[idx] = val.EffectiveBalance()
} else {
justifiedBalances[idx] = 0
}
return nil
}); err != nil {
return err
}

s.justifiedBalancesLock.Lock()
defer s.justifiedBalancesLock.Unlock()
s.justifiedBalances = justifiedBalances
return nil
}

func (s *Service) getJustifiedBalances() []uint64 {
s.justifiedBalancesLock.RLock()
defer s.justifiedBalancesLock.RUnlock()
return s.justifiedBalances
}

// Notifies a common event feed of a new chain head event. Called right after a new
// chain head is determined, set, and saved to disk.
func (s *Service) notifyNewHeadEvent(
Expand Down
6 changes: 4 additions & 2 deletions beacon-chain/blockchain/head_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,15 @@ func TestSaveHead_Different_Reorg(t *testing.T) {
func TestCacheJustifiedStateBalances_CanCache(t *testing.T) {
beaconDB := testDB.SetupDB(t)
service := setupBeaconChain(t, beaconDB)
ctx := context.Background()

state, _ := util.DeterministicGenesisState(t, 100)
r := [32]byte{'a'}
require.NoError(t, service.cfg.BeaconDB.SaveStateSummary(context.Background(), &ethpb.StateSummary{Root: r[:]}))
require.NoError(t, service.cfg.BeaconDB.SaveState(context.Background(), state, r))
require.NoError(t, service.cacheJustifiedStateBalances(context.Background(), r))
require.DeepEqual(t, service.getJustifiedBalances(), state.Balances(), "Incorrect justified balances")
balances, err := service.justifiedBalances.get(ctx, r)
require.NoError(t, err)
require.DeepEqual(t, balances, state.Balances(), "Incorrect justified balances")
}

func TestUpdateHead_MissingJustifiedRoot(t *testing.T) {
Expand Down
20 changes: 10 additions & 10 deletions beacon-chain/blockchain/info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,18 @@ func TestService_TreeHandler(t *testing.T) {
headState, err := util.NewBeaconState()
require.NoError(t, err)
require.NoError(t, headState.SetBalances([]uint64{params.BeaconConfig().GweiPerEth}))
cfg := &config{
BeaconDB: beaconDB,
ForkChoiceStore: protoarray.New(
0, // justifiedEpoch
0, // finalizedEpoch
[32]byte{'a'},
),
StateGen: stategen.New(beaconDB),
fcs := protoarray.New(
0, // justifiedEpoch
0, // finalizedEpoch
[32]byte{'a'},
)
opts := []Option{
WithDatabase(beaconDB),
WithStateGen(stategen.New(beaconDB)),
WithForkChoiceStore(fcs),
}
s, err := NewService(ctx)
s, err := NewService(ctx, opts...)
require.NoError(t, err)
s.cfg = cfg
require.NoError(t, s.cfg.ForkChoiceStore.ProcessBlock(ctx, 0, [32]byte{'a'}, [32]byte{'g'}, [32]byte{'c'}, 0, 0))
require.NoError(t, s.cfg.ForkChoiceStore.ProcessBlock(ctx, 1, [32]byte{'b'}, [32]byte{'a'}, [32]byte{'c'}, 0, 0))
s.setHead([32]byte{'a'}, wrapper.WrappedPhase0SignedBeaconBlock(util.NewBeaconBlock()), headState)
Expand Down
8 changes: 8 additions & 0 deletions beacon-chain/blockchain/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,14 @@ var (
Name: "sync_head_state_hit",
Help: "The number of sync head state requests that are present in the cache.",
})
stateBalanceCacheHit = promauto.NewCounter(prometheus.CounterOpts{
Name: "balance_cache_hit",
Copy link
Member

Choose a reason for hiding this comment

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

there names are too similar to the other metrics like total_effective_balance_cache_hit

Copy link
Member

Choose a reason for hiding this comment

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

why dont define these metrics closer to its usages inside state_balance_cache.go?

Copy link
Contributor Author

@kasey kasey Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

why dont define these metrics closer to its usages

I'm following the conventions of the blockchain package here, all the prom metric values are defined in metrics.go.

there names are too similar to the other metrics

how about state_balance_cache_(hit|miss)?

Copy link
Member

Choose a reason for hiding this comment

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

for some reason i thought the cache resides in cache pkg, it makes sense that the metric is here, i dont want to go into another debate where the cache should live ha!

state_balance_cache is fine, justified_state_balance_cache is also fine

Help: "Count the number of state balance cache hits.",
})
stateBalanceCacheMiss = promauto.NewCounter(prometheus.CounterOpts{
Name: "balance_cache_miss",
Help: "Count the number of state balance cache hits.",
})
)

// reportSlotMetrics reports slot related metrics.
Expand Down
50 changes: 50 additions & 0 deletions beacon-chain/blockchain/mock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package blockchain

import (
"context"
"errors"
"testing"

testDB "github.com/prysmaticlabs/prysm/beacon-chain/db/testing"
"github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/protoarray"
"github.com/prysmaticlabs/prysm/beacon-chain/state"
"github.com/prysmaticlabs/prysm/beacon-chain/state/stategen"
)

func testServiceOptsWithDB(t *testing.T) []Option {
beaconDB := testDB.SetupDB(t)
fcs := protoarray.New(0, 0, [32]byte{'a'})
return []Option{
WithDatabase(beaconDB),
WithStateGen(stategen.New(beaconDB)),
WithForkChoiceStore(fcs),
}
}

// warning: only use these opts when you are certain there are no db calls
// in your code path. this is a lightweight way to satisfy the stategen/beacondb
// initialization requirements w/o the overhead of db init.
func testServiceOptsNoDB() []Option {
return []Option{
withStateBalanceCache(satisfactoryStateBalanceCache()),
}
}

type mockStateByRooter struct {
state state.BeaconState
err error
}

var _ stateByRooter = &mockStateByRooter{}

func (m mockStateByRooter) StateByRoot(_ context.Context, _ [32]byte) (state.BeaconState, error) {
return m.state, m.err
}

// returns an instance of the state balance cache that can be used
// to satisfy the requirement for one in NewService, but which will
// always return an error if used.
func satisfactoryStateBalanceCache() *stateBalanceCache {
err := errors.New("satisfactoryStateBalanceCache doesn't perform real caching")
return &stateBalanceCache{stateGen: mockStateByRooter{err: err}}
}
7 changes: 7 additions & 0 deletions beacon-chain/blockchain/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,13 @@ func WithSlasherAttestationsFeed(f *event.Feed) Option {
}
}

func withStateBalanceCache(c *stateBalanceCache) Option {
return func(s *Service) error {
s.justifiedBalances = c
return nil
}
}

// WithFinalizedStateAtStartUp to store finalized state at start up.
func WithFinalizedStateAtStartUp(st state.BeaconState) Option {
return func(s *Service) error {
Expand Down
Loading