From eb0bd19f52394835698dce08dff570a94b224756 Mon Sep 17 00:00:00 2001 From: terence tsao Date: Sun, 4 Oct 2020 15:52:54 -0700 Subject: [PATCH 1/3] Refactor head info for better usages for lock --- beacon-chain/blockchain/chain_info.go | 32 +++++++++++++++++---- beacon-chain/blockchain/chain_info_test.go | 6 ++-- beacon-chain/blockchain/head.go | 33 +++++++++------------- beacon-chain/blockchain/info.go | 11 ++++++-- beacon-chain/blockchain/receive_block.go | 6 ++-- beacon-chain/blockchain/service_test.go | 4 ++- 6 files changed, 58 insertions(+), 34 deletions(-) diff --git a/beacon-chain/blockchain/chain_info.go b/beacon-chain/blockchain/chain_info.go index 9748ed20af96..55941cd82dce 100644 --- a/beacon-chain/blockchain/chain_info.go +++ b/beacon-chain/blockchain/chain_info.go @@ -97,7 +97,10 @@ func (s *Service) PreviousJustifiedCheckpt() *ethpb.Checkpoint { // HeadSlot returns the slot of the head of the chain. func (s *Service) HeadSlot() uint64 { - if !s.HasHeadState() { + s.headLock.RLock() + defer s.headLock.RUnlock() + + if !s.hasHeadState() { return 0 } @@ -106,6 +109,9 @@ func (s *Service) HeadSlot() uint64 { // HeadRoot returns the root of the head of the chain. func (s *Service) HeadRoot(ctx context.Context) ([]byte, error) { + s.headLock.RLock() + defer s.headLock.RUnlock() + if s.headRoot() != params.BeaconConfig().ZeroHash { r := s.headRoot() return r[:], nil @@ -131,7 +137,10 @@ func (s *Service) HeadRoot(ctx context.Context) ([]byte, error) { // If the head is nil from service struct, // it will attempt to get the head block from DB. func (s *Service) HeadBlock(ctx context.Context) (*ethpb.SignedBeaconBlock, error) { - if s.HasHeadState() { + s.headLock.RLock() + defer s.headLock.RUnlock() + + if s.hasHeadState() { return s.headBlock(), nil } @@ -144,8 +153,10 @@ func (s *Service) HeadBlock(ctx context.Context) (*ethpb.SignedBeaconBlock, erro func (s *Service) HeadState(ctx context.Context) (*state.BeaconState, error) { ctx, span := trace.StartSpan(ctx, "blockChain.HeadState") defer span.End() + s.headLock.RLock() + defer s.headLock.RUnlock() - ok := s.HasHeadState() + ok := s.hasHeadState() span.AddAttributes(trace.BoolAttribute("cache_hit", ok)) if ok { @@ -157,7 +168,10 @@ func (s *Service) HeadState(ctx context.Context) (*state.BeaconState, error) { // HeadValidatorsIndices returns a list of active validator indices from the head view of a given epoch. func (s *Service) HeadValidatorsIndices(ctx context.Context, epoch uint64) ([]uint64, error) { - if !s.HasHeadState() { + s.headLock.RLock() + defer s.headLock.RUnlock() + + if !s.hasHeadState() { return []uint64{}, nil } return helpers.ActiveValidatorIndices(s.headState(ctx), epoch) @@ -165,7 +179,10 @@ func (s *Service) HeadValidatorsIndices(ctx context.Context, epoch uint64) ([]ui // HeadSeed returns the seed from the head view of a given epoch. func (s *Service) HeadSeed(ctx context.Context, epoch uint64) ([32]byte, error) { - if !s.HasHeadState() { + s.headLock.RLock() + defer s.headLock.RUnlock() + + if !s.hasHeadState() { return [32]byte{}, nil } @@ -174,7 +191,10 @@ func (s *Service) HeadSeed(ctx context.Context, epoch uint64) ([32]byte, error) // HeadGenesisValidatorRoot returns genesis validator root of the head state. func (s *Service) HeadGenesisValidatorRoot() [32]byte { - if !s.HasHeadState() { + s.headLock.RLock() + defer s.headLock.RUnlock() + + if !s.hasHeadState() { return [32]byte{} } diff --git a/beacon-chain/blockchain/chain_info_test.go b/beacon-chain/blockchain/chain_info_test.go index 0b4f3aac8936..d58f4f938998 100644 --- a/beacon-chain/blockchain/chain_info_test.go +++ b/beacon-chain/blockchain/chain_info_test.go @@ -103,13 +103,15 @@ func TestHeadSlot_CanRetrieve(t *testing.T) { s, err := state.InitializeFromProto(&pb.BeaconState{}) require.NoError(t, err) c.head = &head{slot: 100, state: s} - assert.Equal(t, uint64(100), c.headSlot()) + assert.Equal(t, uint64(100), c.HeadSlot()) } func TestHeadRoot_CanRetrieve(t *testing.T) { c := &Service{} c.head = &head{root: [32]byte{'A'}} - assert.Equal(t, [32]byte{'A'}, c.headRoot()) + r, err := c.HeadRoot(context.Background()) + require.NoError(t, err) + assert.Equal(t, [32]byte{'A'}, r) } func TestHeadBlock_CanRetrieve(t *testing.T) { diff --git a/beacon-chain/blockchain/head.go b/beacon-chain/blockchain/head.go index c84040068e08..1bba5afd062e 100644 --- a/beacon-chain/blockchain/head.go +++ b/beacon-chain/blockchain/head.go @@ -72,7 +72,11 @@ func (s *Service) saveHead(ctx context.Context, headRoot [32]byte) error { defer span.End() // Do nothing if head hasn't changed. - if headRoot == s.headRoot() { + r, err := s.HeadRoot(ctx) + if err != nil { + return err + } + if headRoot == bytesutil.ToBytes32(r) { return nil } @@ -101,16 +105,17 @@ func (s *Service) saveHead(ctx context.Context, headRoot [32]byte) error { } // A chain re-org occurred, so we fire an event notifying the rest of the services. - if bytesutil.ToBytes32(newHeadBlock.Block.ParentRoot) != s.headRoot() { + headSlot := s.HeadSlot() + if bytesutil.ToBytes32(newHeadBlock.Block.ParentRoot) != bytesutil.ToBytes32(r) { log.WithFields(logrus.Fields{ "newSlot": fmt.Sprintf("%d", newHeadBlock.Block.Slot), - "oldSlot": fmt.Sprintf("%d", s.headSlot()), + "oldSlot": fmt.Sprintf("%d", headSlot), }).Debug("Chain reorg occurred") s.stateNotifier.StateFeed().Send(&feed.Event{ Type: statefeed.Reorg, Data: &statefeed.ReorgData{ NewSlot: newHeadBlock.Block.Slot, - OldSlot: s.headSlot(), + OldSlot: headSlot, }, }) @@ -179,52 +184,42 @@ func (s *Service) setHeadInitialSync(root [32]byte, block *ethpb.SignedBeaconBlo } // This returns the head slot. +// This is a lock free version. func (s *Service) headSlot() uint64 { - s.headLock.RLock() - defer s.headLock.RUnlock() - return s.head.slot } // This returns the head root. // It does a full copy on head root for immutability. +// This is a lock free version. func (s *Service) headRoot() [32]byte { if s.head == nil { return params.BeaconConfig().ZeroHash } - s.headLock.RLock() - defer s.headLock.RUnlock() - return s.head.root } // This returns the head block. // It does a full copy on head block for immutability. +// This is a lock free version. func (s *Service) headBlock() *ethpb.SignedBeaconBlock { - s.headLock.RLock() - defer s.headLock.RUnlock() - return stateTrie.CopySignedBeaconBlock(s.head.block) } // This returns the head state. // It does a full copy on head state for immutability. +// This is a lock free version. func (s *Service) headState(ctx context.Context) *stateTrie.BeaconState { ctx, span := trace.StartSpan(ctx, "blockChain.headState") defer span.End() - s.headLock.RLock() - defer s.headLock.RUnlock() - return s.head.state.Copy() } // This returns the genesis validator root of the head state. +// This is a lock free version. func (s *Service) headGenesisValidatorRoot() [32]byte { - s.headLock.RLock() - defer s.headLock.RUnlock() - return bytesutil.ToBytes32(s.head.state.GenesisValidatorRoot()) } diff --git a/beacon-chain/blockchain/info.go b/beacon-chain/blockchain/info.go index 132b18a388da..796909e93d45 100644 --- a/beacon-chain/blockchain/info.go +++ b/beacon-chain/blockchain/info.go @@ -34,7 +34,12 @@ const template = ` // TreeHandler is a handler to serve /tree page in metrics. func (s *Service) TreeHandler(w http.ResponseWriter, r *http.Request) { - if s.headState(r.Context()) == nil { + headState, err := s.HeadState(r.Context()) + if err != nil { + log.WithError(err).Error("Could not get head state") + return + } + if headState == nil { if _, err := w.Write([]byte("Unavailable during initial syncing")); err != nil { log.WithError(err).Error("Failed to render p2p info page") } @@ -47,7 +52,7 @@ func (s *Service) TreeHandler(w http.ResponseWriter, r *http.Request) { graph.Attr("labeljust", "l") dotNodes := make([]*dot.Node, len(nodes)) - avgBalance := uint64(averageBalance(s.headState(r.Context()).Balances())) + avgBalance := uint64(averageBalance(headState.Balances())) for i := len(nodes) - 1; i >= 0; i-- { // Construct label for each node. @@ -63,7 +68,7 @@ func (s *Service) TreeHandler(w http.ResponseWriter, r *http.Request) { dotN = graph.Node(index).Box().Attr("label", label) } - if nodes[i].Slot() == s.headSlot() && + if nodes[i].Slot() == s.HeadSlot() && nodes[i].BestDescendant() == ^uint64(0) && nodes[i].Parent() != ^uint64(0) { dotN = dotN.Attr("color", "green") diff --git a/beacon-chain/blockchain/receive_block.go b/beacon-chain/blockchain/receive_block.go index b5e0abbb9e00..a6751cd8f641 100644 --- a/beacon-chain/blockchain/receive_block.go +++ b/beacon-chain/blockchain/receive_block.go @@ -59,7 +59,7 @@ func (s *Service) ReceiveBlock(ctx context.Context, block *ethpb.SignedBeaconBlo } // Reports on block and fork choice metrics. - reportSlotMetrics(blockCopy.Block.Slot, s.headSlot(), s.CurrentSlot(), s.finalizedCheckpt) + reportSlotMetrics(blockCopy.Block.Slot, s.HeadSlot(), s.CurrentSlot(), s.finalizedCheckpt) // Log block sync status. logBlockSyncStatus(blockCopy.Block, blockRoot, s.finalizedCheckpt) @@ -95,7 +95,7 @@ func (s *Service) ReceiveBlockInitialSync(ctx context.Context, block *ethpb.Sign }) // Reports on blockCopy and fork choice metrics. - reportSlotMetrics(blockCopy.Block.Slot, s.headSlot(), s.CurrentSlot(), s.finalizedCheckpt) + reportSlotMetrics(blockCopy.Block.Slot, s.HeadSlot(), s.CurrentSlot(), s.finalizedCheckpt) // Log state transition data. log.WithFields(logrus.Fields{ @@ -139,7 +139,7 @@ func (s *Service) ReceiveBlockBatch(ctx context.Context, blocks []*ethpb.SignedB }) // Reports on blockCopy and fork choice metrics. - reportSlotMetrics(blockCopy.Block.Slot, s.headSlot(), s.CurrentSlot(), s.finalizedCheckpt) + reportSlotMetrics(blockCopy.Block.Slot, s.HeadSlot(), s.CurrentSlot(), s.finalizedCheckpt) } if err := s.VerifyWeakSubjectivityRoot(s.ctx); err != nil { diff --git a/beacon-chain/blockchain/service_test.go b/beacon-chain/blockchain/service_test.go index 12d709e4e66d..5cec79a1bbdd 100644 --- a/beacon-chain/blockchain/service_test.go +++ b/beacon-chain/blockchain/service_test.go @@ -218,7 +218,9 @@ func TestChainService_InitializeBeaconChain(t *testing.T) { if headBlk == nil { t.Error("Head state can't be nil after initialize beacon chain") } - if bc.headRoot() == params.BeaconConfig().ZeroHash { + r, err := bc.HeadRoot(ctx) + require.NoError(t, err) + if bytesutil.ToBytes32(r) == params.BeaconConfig().ZeroHash { t.Error("Canonical root for slot 0 can't be zeros after initialize beacon chain") } } From de74872e4dcdecdf1cd43c8f0d4751c466df6071 Mon Sep 17 00:00:00 2001 From: terence tsao Date: Mon, 5 Oct 2020 07:59:41 -0700 Subject: [PATCH 2/3] Fix headroot test to use [32]byte{} --- beacon-chain/blockchain/chain_info_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon-chain/blockchain/chain_info_test.go b/beacon-chain/blockchain/chain_info_test.go index d58f4f938998..f1a6614584bd 100644 --- a/beacon-chain/blockchain/chain_info_test.go +++ b/beacon-chain/blockchain/chain_info_test.go @@ -111,7 +111,7 @@ func TestHeadRoot_CanRetrieve(t *testing.T) { c.head = &head{root: [32]byte{'A'}} r, err := c.HeadRoot(context.Background()) require.NoError(t, err) - assert.Equal(t, [32]byte{'A'}, r) + assert.Equal(t, [32]byte{'A'}, bytesutil.ToBytes32(r)) } func TestHeadBlock_CanRetrieve(t *testing.T) { From d9a40495f0daf98d1bd31701ea25d2fe2ae67895 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Tue, 6 Oct 2020 11:17:07 +0300 Subject: [PATCH 3/3] go fmt: issue introduced in #7429 --- beacon-chain/main.go | 2 +- slasher/main.go | 2 +- validator/main.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/beacon-chain/main.go b/beacon-chain/main.go index 419b95ca03ff..2a86253f01b2 100644 --- a/beacon-chain/main.go +++ b/beacon-chain/main.go @@ -19,9 +19,9 @@ import ( _ "github.com/prysmaticlabs/prysm/shared/maxprocs" "github.com/prysmaticlabs/prysm/shared/version" "github.com/sirupsen/logrus" - "github.com/wercker/journalhook" "github.com/urfave/cli/v2" "github.com/urfave/cli/v2/altsrc" + "github.com/wercker/journalhook" prefixed "github.com/x-cray/logrus-prefixed-formatter" ) diff --git a/slasher/main.go b/slasher/main.go index 3199e786d864..e85fd1b927dd 100644 --- a/slasher/main.go +++ b/slasher/main.go @@ -17,9 +17,9 @@ import ( "github.com/prysmaticlabs/prysm/slasher/flags" "github.com/prysmaticlabs/prysm/slasher/node" "github.com/sirupsen/logrus" - "github.com/wercker/journalhook" "github.com/urfave/cli/v2" "github.com/urfave/cli/v2/altsrc" + "github.com/wercker/journalhook" prefixed "github.com/x-cray/logrus-prefixed-formatter" ) diff --git a/validator/main.go b/validator/main.go index dc4b45cd6379..5fb5debe14fb 100644 --- a/validator/main.go +++ b/validator/main.go @@ -27,9 +27,9 @@ import ( "github.com/prysmaticlabs/prysm/validator/flags" "github.com/prysmaticlabs/prysm/validator/node" "github.com/sirupsen/logrus" - "github.com/wercker/journalhook" "github.com/urfave/cli/v2" "github.com/urfave/cli/v2/altsrc" + "github.com/wercker/journalhook" prefixed "github.com/x-cray/logrus-prefixed-formatter" "google.golang.org/grpc" )