Khalil/5732 Adjust Blocktime Controller

[kc1116]

trafficstars

This PR adjusts the Blocktime controller and adds processing of the EpochExtended and EpochRecovered events.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 63.29114% with 29 lines in your changes missing coverage. Please review.

Project coverage is 41.71%. Comparing base (e833847) to head (711452c).

Files	Patch %	Lines
...sensus/hotstuff/cruisectl/block_time_controller.go	64.10%	18 Missing and 10 partials :warning:
utils/unittest/mocks/epoch_query.go	0.00%	1 Missing :warning:

Additional details and impacted files

@@                  Coverage Diff                  @@
##           feature/efm-recovery    #6102   +/-   ##
=====================================================
  Coverage                 41.70%   41.71%           
=====================================================
  Files                      1976     1976           
  Lines                    139361   139331   -30     
=====================================================
- Hits                      58123    58121    -2     
+ Misses                    75176    75152   -24     
+ Partials                   6062     6058    -4     

Flag	Coverage Δ
unittests	41.71% <63.29%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[kc1116]

@jordanschalm relevant changes

• Event Handling https://github.com/onflow/flow-go/pull/6102/commits/f8440429ab4e52bec455ed6336aa7be442ef496b
• Initializing in an epoch with extensions https://github.com/onflow/flow-go/pull/6102/commits/7be849dabb04f5c223e65f979605ca4dcfd9d349
• Change to EpochCommitted events if phase == flow.EpochPhaseCommitted https://github.com/onflow/flow-go/pull/6102/commits/e16a282e51cd1c429acd1a1bd165d86513d9c6f8

[AlexHentschel]

I noticed that we duplicate notable amounts of code:

• checks that the epochInfo values are not nil in the initialization and epoch transition
• handling errors from querying the values in the initialization and update

I feel we could consolidate this

1. by splitting the BlockTimeController.epochInfo into the current and next epoch:

   currentEpochTiming epochTiming
   nextEpochTiming    *epochTiming
   

2. the struct epochTiming would then just describe a single epoch:

   // epochTiming encapsulates the timing information of one specific epoch:
   type epochTiming struct {
   	curEpochFirstView      uint64 // first view of the epoch's view range
   	curEpochFinalView      uint64 // last view of the epoch's view range
   	curEpochTargetDuration uint64 // desired total duration of the epoch in seconds
   	curEpochTargetEndTime  uint64 // target end time of the epoch, represented as Unix Time [seconds]
   }
   
   // newEpochTiming queries the timing information from the given `epoch` and returns it as a new `epochTiming` instance.
   func newEpochTiming(epoch protocol.Epoch) (*epochTiming, error) {
   	curEpochFirstView, err := epoch.FirstView()
   	if err != nil {
   		return nil, fmt.Errorf("could not retrieve epoch's first view: %w", err)
   	}
   	curEpochFinalView, err := epoch.FinalView()
   	if err != nil {
   		return nil, fmt.Errorf("could not retrieve epoch's final view: %w", err)
   	}
   	curEpochTargetDuration, err := epoch.TargetDuration()
   	if err != nil {
   		return nil, fmt.Errorf("could not retrieve epoch's target duration: %w", err)
   	}
   	curEpochTargetEndTime, err := epoch.TargetEndTime()
   	if err != nil {
   		return nil, fmt.Errorf("could not retrieve epoch's target end time: %w", err)
   	}
   
   	return &epochTiming{
   		curEpochFirstView:      curEpochFirstView,
   		curEpochFinalView:      curEpochFinalView,
   		curEpochTargetDuration: curEpochTargetDuration,
   		curEpochTargetEndTime:  curEpochTargetEndTime,
   	}, nil
   }
   
   // isFollowedBy determines whether nextEpoch is indeed the direct successor of the receiver,
   // based on the view ranges of both epochs.
   func (et *epochTiming) isFollowedBy(nextEpoch *epochTiming) bool {
   	return et.curEpochFinalView+1 == nextEpoch.curEpochFirstView
   }
   

3. I would really like a sanity check:

   • here, we just assume that we haven't missed anyting and there are no bugs anywhere that could compromise the correctness: https://github.com/onflow/flow-go/blob/3c7d837f5cf62d401c7e9c894a7608e6e7a1f7ad/consensus/hotstuff/cruisectl/block_time_controller.go#L350
   • However, each epoch specifies its first view, so we can actually check.

   Therefore, I included the method epochTiming.isFollowedBy. With the sanity check the method BlockTimeController.initEpochInfo would then look like (a lot more concise in my opinion):

   // initEpochInfo initializes the epochInfo state upon component startup.
   // No errors are expected during normal operation.
   func (ctl *BlockTimeController) initEpochInfo() error {
   	finalSnapshot := ctl.state.Final()
   
   	currentEpochTiming, err := newEpochTiming(finalSnapshot.Epochs().Current())
   	if err != nil {
   		return fmt.Errorf("failed to retrieve the current epoch's timing information: %w", err)
   	}
   	ctl.currentEpochTiming = *currentEpochTiming
   
   	phase, err := finalSnapshot.EpochPhase()
   	if err != nil {
   		return fmt.Errorf("could not check snapshot phase: %w", err)
   	}
   	if phase == flow.EpochPhaseCommitted {
   		ctl.nextEpochTiming, err = newEpochTiming(finalSnapshot.Epochs().Next())
   		if err != nil {
   			return fmt.Errorf("failed to retrieve the next epoch's timing information: %w", err)
   		}
   		if !currentEpochTiming.isFollowedBy(ctl.nextEpochTiming) {
   			return fmt.Errorf("failed to retrieve the next epoch's timing information: %w", err)
   		}
   	}
   
   	return nil
   }
   

4. similarly for processEpochCommittedPhaseStarted:

   // processEpochCommittedPhaseStarted processes the EpochCommittedPhaseStarted notification, which
   // the consensus component emits when we finalize the first block of the Epoch Committed phase.
   // Specifically, we memorize the next epoch's timing information in the BlockTimeController.
   // No errors are expected during normal operation.
   func (ctl *BlockTimeController) processEpochCommittedPhaseStarted(first *flow.Header) error {
   	snapshot := ctl.state.AtHeight(first.Height)
   	ctl.nextEpochTiming, err = newEpochTiming(snapshot.Epochs().Next())
   	if err != nil {
   		return fmt.Errorf("failed to retrieve the next epoch's timing information: %w", err)
   	}
   	if !ctl.currentEpochTiming.isFollowedBy(ctl.nextEpochTiming) {
   		return fmt.Errorf("failed to retrieve the next epoch's timing information: %w", err)
   	}
   	return nil
   }
   

[kc1116]

@AlexHentschel Refactor epochInfo into currentEpochTiming and *nextEpochTiming https://github.com/onflow/flow-go/pull/6102/commits/84fb5a20eb255d197ed7d2baccf1627dc5f596cd