Better `head` object coupling for chain service #4869

terencechain · 2020-02-14T21:11:55Z

Problem statement: There's too many variables that defines head for chain service.
Example in initial chain info:

s.headState, _ = s.beaconDB.State(ctx, bytesutil.ToBytes32(finalized.Root))
s.headBlock, _ = s.beaconDB.Block(ctx, bytesutil.ToBytes32(finalized.Root))
if s.headBlock != nil && s.headBlock.Block != nil {
	s.headSlot = s.headBlock.Block.Slot
}
s.canonicalRoots[s.headSlot] = finalized.Root

A problem could be, if headSlot changes itself, then it changes head root while head state and head block remain its own version 😢

To make it better, we define a head object:

// This defines the current chain service's view of head.
type head struct {
	slot  uint64 // current head slot.
	root  [32]byte // current head root.
	block *ethpb.SignedBeaconBlock // current head block.
	state *state.BeaconState // current head state.
}

To update the head object:

// This sets head view object which is used to track the head slot, root, block and state.
func (s *Service) setHead(slot uint64, root [32]byte, block *ethpb.SignedBeaconBlock, state *state.BeaconState) {
	s.headLock.Lock()
	defer s.headLock.Unlock()

	s.head = &head{
		slot:  slot,
		root:  root,
		block: block,
		state: state,
	}
}

To retrieve from the head object, it's copy by default:

// This returns the head block.
// It does a full copy on head block for immutability.
func (s *Service) headBlock() *ethpb.SignedBeaconBlock {
	s.headLock.RLock()
	defer s.headLock.RUnlock()

	return stateTrie.CopySignedBeaconBlock(s.head.block)
}

And now to initialize chain info:

s.setHead(finalizedState.Slot(), bytesutil.ToBytes32(finalized.Root), finalizedBlock, finalizedState)

This PR also cleaned up general head code by moving function to its proper location. Will list out those changes via inline comments

terencechain · 2020-02-14T21:15:54Z

beacon-chain/blockchain/head.go

+// This gets called to update canonical root mapping. It does not save head block
+// root in DB. With the inception of inital-sync-cache-state flag, it uses finalized
+// check point as anchors to resume sync therefore head is no longer needed to be saved on per slot basis.
+func (s *Service) saveHeadNoDB(ctx context.Context, b *ethpb.SignedBeaconBlock, r [32]byte) error {


this is an old function, I moved it from service.go to here, it makes more sense here

terencechain · 2020-02-14T21:16:14Z

beacon-chain/blockchain/head.go

+}
+
+// This sets head view object which is used to track the head slot, root, block and state.
+func (s *Service) setHead(slot uint64, root [32]byte, block *ethpb.SignedBeaconBlock, state *state.BeaconState) {


start review here lines 145 to 205

terencechain · 2020-02-14T21:16:53Z

beacon-chain/blockchain/info.go

-const latestSlotCount = 10
-
-// HeadsHandler is a handler to serve /heads page in metrics.
-func (s *Service) HeadsHandler(w http.ResponseWriter, _ *http.Request) {


This was not used at all. Removing it because it was too much work to maintain

…prysm into better-head-obj

prestonvanloon · 2020-02-14T23:19:47Z

beacon-chain/blockchain/head.go

+	s.headLock.RLock()
+	defer s.headLock.RUnlock()
+
+	return !(s.head == nil) && !(s.head.state == nil)


Suggested change

return !(s.head == nil) && !(s.head.state == nil)

return s.head != nil && s.head.state != nil

codecov · 2020-02-14T23:35:55Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a26da31). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #4869   +/-   ##
=========================================
  Coverage          ?   44.34%           
=========================================
  Files             ?      203           
  Lines             ?    15205           
  Branches          ?        0           
=========================================
  Hits              ?     6742           
  Misses            ?     7382           
  Partials          ?     1081

nisdas · 2020-02-14T23:56:41Z

beacon-chain/blockchain/head.go

+}
+
+// This sets head view object which is used to track the head slot, root, block and state.
+func (s *Service) setHead(slot uint64, root [32]byte, block *ethpb.SignedBeaconBlock, state *state.BeaconState) {


we dont need a slot param, cant we just get it from the block ?

nisdas · 2020-02-14T23:57:00Z

beacon-chain/blockchain/head.go

+		slot:  slot,
+		root:  root,
+		block: block,
+		state: state,


please copy the state

I also copied the block as well

nisdas · 2020-02-14T23:57:46Z

beacon-chain/blockchain/head.go

+	root := make([]byte, 32)
+	copy(root, s.head.root[:])
+
+	return bytesutil.ToBytes32(root)


roots are 32 byte arrays, so they are passed by value. You can just do

return s.head.root

…o better-head-obj

terencechain · 2020-02-15T18:38:23Z

validator/client/validator_metrics.go

@@ -29,8 +29,7 @@ var validatorBalancesGaugeVec = promauto.NewGaugeVec(
 // and penalties over time, percentage gain/loss, and gives the end user a better idea
 // of how the validator performs with respect to the rest.
 func (v *validator) LogValidatorGainsAndLosses(ctx context.Context, slot uint64) error {
-
-	if slot%params.BeaconConfig().SlotsPerEpoch != 0 || slot < params.BeaconConfig().SlotsPerEpoch {
+	if slot%params.BeaconConfig().SlotsPerEpoch != 0 || slot <= params.BeaconConfig().SlotsPerEpoch {


This was a bug, caught it via new test here

rauljordan · 2020-02-15T18:39:58Z

beacon-chain/blockchain/chain_info.go

 		return &ethpb.Checkpoint{Root: params.BeaconConfig().ZeroHash[:]}
 	}
-	cpt := s.headState.FinalizedCheckpoint()
+
+	cpt := state.CopyCheckpoint(s.finalizedCheckpt)


Can you move this beneath the equality check and just compare s.finalizedCheckpt.Root in bytes.Equal? Might save some copies

Good suggestion. Done

rauljordan · 2020-02-15T18:40:06Z

beacon-chain/blockchain/chain_info.go

 		return &ethpb.Checkpoint{Root: params.BeaconConfig().ZeroHash[:]}
 	}

-	cpt := s.headState.CurrentJustifiedCheckpoint()
+	cpt := state.CopyCheckpoint(s.justifiedCheckpt)


rauljordan · 2020-02-15T18:40:12Z

beacon-chain/blockchain/chain_info.go

 		return &ethpb.Checkpoint{Root: params.BeaconConfig().ZeroHash[:]}
 	}

-	cpt := s.headState.PreviousJustifiedCheckpoint()
+	cpt := state.CopyCheckpoint(s.prevJustifiedCheckpt)


rauljordan · 2020-02-15T18:40:39Z

beacon-chain/blockchain/chain_info.go

 	}

-	return s.headState.Copy(), nil
+	headState, err := s.beaconDB.HeadState(ctx)


Update cache if we have a cache miss?

Not here. That's what the PR aim to avoid. Update individual head field. If there's a miss setHead will be used during init

…prysm into better-head-obj

* Done * Fixed lock * Fixed all the tests * Comments * Fixed more tests * Merge branch 'master' into better-head-obj * Fixed more tests * Merge branch 'better-head-obj' of git+ssh://github.com/prysmaticlabs/prysm into better-head-obj * Prestons feedback & fixed test * Nishant's feedback * Participation edge case * Gaz * Merge branch 'master' into better-head-obj * Merge branch 'master' of git+ssh://github.com/prysmaticlabs/prysm into better-head-obj * Raul's feedback * Merge branch 'better-head-obj' of git+ssh://github.com/prysmaticlabs/prysm into better-head-obj

terencechain added 4 commits February 14, 2020 11:32

Done

f95e8f9

Fixed lock

5b4a4af

Fixed all the tests

c1ab82f

Comments

eee115d

terencechain self-assigned this Feb 14, 2020

Fixed more tests

6152f51

terencechain added the Enhancement New feature or request label Feb 14, 2020

terencechain commented Feb 14, 2020

View reviewed changes

terencechain added 3 commits February 14, 2020 13:17

Merge branch 'master' into better-head-obj

74fc8fa

Fixed more tests

9d0d403

Merge branch 'better-head-obj' of git+ssh://github.com/prysmaticlabs/…

eb0749f

…prysm into better-head-obj

prestonvanloon reviewed Feb 14, 2020

View reviewed changes

Prestons feedback & fixed test

c9c3a83

nisdas reviewed Feb 14, 2020

View reviewed changes

terencechain added 3 commits February 14, 2020 16:26

Nishant's feedback

235b07a

Participation edge case

54c5c4d

Gaz

b7f1019

terencechain added the Ready For Review A pull request ready for code review label Feb 15, 2020

terencechain added 2 commits February 15, 2020 10:34

Merge branch 'master' into better-head-obj

fd639de

Merge branch 'master' of git+ssh://github.com/prysmaticlabs/prysm int…

a26da31

…o better-head-obj

terencechain commented Feb 15, 2020

View reviewed changes

rauljordan reviewed Feb 15, 2020

View reviewed changes

terencechain added 2 commits February 15, 2020 11:46

Raul's feedback

21d394b

Merge branch 'better-head-obj' of git+ssh://github.com/prysmaticlabs/…

91ab903

…prysm into better-head-obj

rauljordan approved these changes Feb 15, 2020

View reviewed changes

terencechain added the OK to merge label Feb 15, 2020

prylabs-bulldozer bot merged commit 456ac5f into master Feb 15, 2020

delete-merged-branch bot deleted the better-head-obj branch February 15, 2020 18:57

prestonvanloon mentioned this pull request Feb 16, 2020

Fix Head References #4852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better `head` object coupling for chain service #4869

Better `head` object coupling for chain service #4869

terencechain commented Feb 14, 2020 •

edited

terencechain Feb 14, 2020

terencechain Feb 14, 2020

terencechain Feb 14, 2020

prestonvanloon Feb 14, 2020

terencechain Feb 14, 2020

codecov bot commented Feb 14, 2020 •

edited

nisdas Feb 14, 2020

nisdas Feb 14, 2020

terencechain Feb 15, 2020

nisdas Feb 14, 2020

terencechain Feb 15, 2020

rauljordan Feb 15, 2020

terencechain Feb 15, 2020

rauljordan Feb 15, 2020

rauljordan Feb 15, 2020

rauljordan Feb 15, 2020

terencechain Feb 15, 2020

	return !(s.head == nil) && !(s.head.state == nil)
	return s.head != nil && s.head.state != nil

Better head object coupling for chain service #4869

Better head object coupling for chain service #4869

Conversation

terencechain commented Feb 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Better `head` object coupling for chain service #4869

Better `head` object coupling for chain service #4869

terencechain commented Feb 14, 2020 •

edited

codecov bot commented Feb 14, 2020 •

edited