Call FCU with attribute on non head block

[terencechain]

In the following condition:

99 - 100
   \-- 101

you are a proposer for 102, and 101 arrived on time but never became head. The head during 101 was still 100. Under such conditions, we want to call FCU with head 100 and payload attribute as soon as we process 101 to signal EL client for payload preparation. If we don't do this, then the proposer will wait to call FCU with payload attribute at the start of 102

This PR:

• Reimplement notifyEngineIfChangedHead to forkchoiceUpdateWithExecution. More align on naming
• Refactored all the necessary logic under forkchoice_update_execution.go
• Refactored all the necessary tests under forkchoice_update_execution_test.go
• Added a new test TestService_forkchoiceUpdateWithExecution_SameHeadRootNewProposer

[potuz]

In the following condition, 100 <- 101 <- 102, you are a proposer for 102, and 101 arrived on time but never became head.

This situation can never happen

[terencechain]

In the following condition, 100 <- 101 <- 102, you are a proposer for 102, and 101 arrived on time but never became head.

This situation can never happen

Messed up on the graph. I meant:

100 /--- 101
    \--- 102

[potuz]

In the following condition, 100 <- 101 <- 102, you are a proposer for 102, and 101 arrived on time but never became head.

This situation can never happen

Messed up on the graph. I meant:

100 /--- 101
    \--- 102

This also can't happen :)

[potuz]

Suggested change

	if newHeadRoot == [32]byte{} || s.headRoot() == newHeadRoot && !ok {
	if newHeadRoot == [32]byte{} || s.headRoot() == newHeadRoot && ok {

[potuz]

This adds duplicated logic in a few places. At the very least I remember saveHead where the same check its used. If we are going to have a dedicated method to check if a given block root differs from the stored head, we should move these checks to use this method.

Also, the logic in saveHead is a little safer with respect to the origin Root, so I'll extend slightly this comment. This is what saveHead currently has:

func (s *Service) saveHead(ctx context.Context, newHeadRoot [32]byte, headBlock interfaces.SignedBeaconBlock, headState state.BeaconState) error {
...
	var oldHeadRoot [32]byte
	s.headLock.RLock()
	if s.head == nil {
		oldHeadRoot = s.originBlockRoot
	} else {
		oldHeadRoot = s.head.root
	}
	s.headLock.RUnlock()
	if newHeadRoot == oldHeadRoot {
		return nil
	}

We should move the last check to

if s.isNewHead(newHeadRoot)

But also, we should probably add the checks for head being nil and dealing with the originBlockRoot in this method as well.

[terencechain]

Done. One annoying part is we still need to retrieve oldHeadRoot for logs

[potuz]

This function name is very misleading as it does not get the head state and block, but rather any state and block with the passed block root r.

[potuz]

This will again repeat the check for isNewHead in saveHead when we could have saved the result in the first line of this method. It would be better to change the semanthics of saveHead to not make this check, so that it actually is like the name indicates, a function that saves head, instead of the currently misleading saveHeadIfNew. This would require starting this method with something like

        newHead := s.isNewHead(newHeadRoot)
	if !newHead && !s.isNewProposer() {
		return nil
	}
	....
	if newHead {
		if err := s.saveHead(ctx, newHeadRoot, headBlock, headState); err != nil {
         ...
     }

[potuz]

I know I'm a pain in the butt but can we shift the logic:

return  r != currentHeadRoot || r == [32]byte{}