Merge #4165

4165: Cleanup stop control component r=janezpodhostnik a=janezpodhostnik

I was trying to extract this change 
https://github.com/onflow/flow-go/pull/3736/files#diff-c495742b9ab8ee8b5b050d102767ba106b2fda1cbf4f1c0a49acee333a118c0fR107 out of this PR #3736, but I wasn't quite sure what was going on, so I did some cleaning as well.

- changed some naming
- changed formatting
- added todos to look into in the future

I think they might be related to these tests being disabled:
```
func Test_OnlyHeadOfTheQueueIsExecuted(t *testing.T) {
	unittest.SkipUnless(t, unittest.TEST_FLAKY, "To be fixed later")
```
```
func TestBlocksArentExecutedMultipleTimes_multipleBlockEnqueue(t *testing.T) {
	unittest.SkipUnless(t, unittest.TEST_TODO, "broken test")
```

I'll take a look at that next.

Co-authored-by: Janez Podhostnik <[email protected]>

Author: bors[bot]

engine/execution/ingestion/stop_control.go

@@ -17,45 +17,55 @@ import (
 // StopControl follows states described in StopState
 type StopControl struct {
 	sync.RWMutex
-	// desired stop height, the first value new version should be used, so this height WON'T
-	// be executed
-	height uint64
+	// desired stopHeight, the first value new version should be used,
+	// so this height WON'T be executed
+	stopHeight uint64
 
-	// if the node should crash or just pause after reaching stop height
-	crash              bool
+	// if the node should crash or just pause after reaching stopHeight
+	crash bool
+
+	// This is the block ID of the block that should be executed last.
 	stopAfterExecuting flow.Identifier
 
 	log   zerolog.Logger
 	state StopControlState
 
-	// used to prevent setting stop height to block which has already been executed
+	// used to prevent setting stopHeight to block which has already been executed
 	highestExecutingHeight uint64
 }
 
 type StopControlState byte
 
 const (
-	// StopControlOff default state, envisioned to be used most of the time. Stopping module is simply off,
-	// blocks will be processed "as usual".
+	// StopControlOff default state, envisioned to be used most of the time.
+	// Stopping module is simply off, blocks will be processed "as usual".
 	StopControlOff StopControlState = iota
 
-	// StopControlSet means stop height is set but not reached yet, and nothing related to stopping happened yet.
+	// StopControlSet means stopHeight is set but not reached yet,
+	// and nothing related to stopping happened yet.
 	// We could still go back to StopControlOff or progress to StopControlCommenced.
 	StopControlSet
 
-	// StopControlCommenced indicates that stopping process has commenced and no parameters can be changed anymore.
-	// For example, blocks at or above stop height has been received, but finalization didn't reach stop height yet.
+	// StopControlCommenced indicates that stopping process has commenced
+	// and no parameters can be changed anymore.
+	// For example, blocks at or above stopHeight has been received,
+	// but finalization didn't reach stopHeight yet.
 	// It can only progress to StopControlPaused
 	StopControlCommenced
 
-	// StopControlPaused means EN has stopped processing blocks. It can happen by reaching the set stopping `height`, or
+	// StopControlPaused means EN has stopped processing blocks.
+	// It can happen by reaching the set stopping `stopHeight`, or
 	// if the node was started in pause mode.
 	// It is a final state and cannot be changed
 	StopControlPaused
 )
 
 // NewStopControl creates new empty NewStopControl
-func NewStopControl(log zerolog.Logger, paused bool, lastExecutedHeight uint64) *StopControl {
+func NewStopControl(
+	log zerolog.Logger,
+	paused bool,
+	lastExecutedHeight uint64,
+) *StopControl {
 	state := StopControlOff
 	if paused {
 		state = StopControlPaused
@@ -82,61 +92,84 @@ func (s *StopControl) IsPaused() bool {
 	return s.state == StopControlPaused
 }
 
-// SetStopHeight sets new stop height and crash mode, and return old values:
-//   - height
+// SetStopHeight sets new stopHeight and crash mode, and return old values:
+//   - stopHeight
 //   - crash
 //
 // Returns error if the stopping process has already commenced, new values will be rejected.
-func (s *StopControl) SetStopHeight(height uint64, crash bool) (uint64, bool, error) {
+func (s *StopControl) SetStopHeight(
+	height uint64,
+	crash bool,
+) (uint64, bool, error) {
 	s.Lock()
 	defer s.Unlock()
 
-	oldHeight := s.height
+	oldHeight := s.stopHeight
 	oldCrash := s.crash
 
 	if s.state == StopControlCommenced {
-		return oldHeight, oldCrash, fmt.Errorf("cannot update stop height, stopping commenced for height %d with crash=%t", oldHeight, oldCrash)
+		return oldHeight,
+			oldCrash,
+			fmt.Errorf(
+				"cannot update stopHeight, "+
+					"stopping commenced for stopHeight %d with crash=%t",
+				oldHeight,
+				oldCrash,
+			)
 	}
 
 	if s.state == StopControlPaused {
-		return oldHeight, oldCrash, fmt.Errorf("cannot update stop height, already paused")
+		return oldHeight,
+			oldCrash,
+			fmt.Errorf("cannot update stopHeight, already paused")
 	}
 
-	// +1 because we track last executing height, so +1 is the lowest possible block to stop
-	if height <= s.highestExecutingHeight+1 {
-		return oldHeight, oldCrash, fmt.Errorf("cannot update stop height, given height %d at or below last executed %d", height, s.highestExecutingHeight)
+	// cannot set stopHeight to block which is already executing
+	// so the lowest possible stopHeight is highestExecutingHeight+1
+	if height <= s.highestExecutingHeight {
+		return oldHeight,
+			oldCrash,
+			fmt.Errorf(
+				"cannot update stopHeight, "+
+					"given stopHeight %d below or equal to highest executing height %d",
+				height,
+				s.highestExecutingHeight,
+			)
 	}
 
 	s.log.Info().
-		Int8("previous_state", int8(s.state)).Int8("new_state", int8(StopControlSet)).
-		Uint64("height", height).Bool("crash", crash).
-		Uint64("old_height", oldHeight).Bool("old_crash", oldCrash).Msg("new stop height set")
+		Int8("previous_state", int8(s.state)).
+		Int8("new_state", int8(StopControlSet)).
+		Uint64("stopHeight", height).
+		Bool("crash", crash).
+		Uint64("old_height", oldHeight).
+		Bool("old_crash", oldCrash).
+		Msg("new stopHeight set")
 
 	s.state = StopControlSet
 
-	s.height = height
+	s.stopHeight = height
 	s.crash = crash
 	s.stopAfterExecuting = flow.ZeroID
 
 	return oldHeight, oldCrash, nil
 }
 
 // GetStopHeight returns:
-//   - height
+//   - stopHeight
 //   - crash
 //
 // Values are undefined if they were not previously set
 func (s *StopControl) GetStopHeight() (uint64, bool) {
 	s.RLock()
 	defer s.RUnlock()
 
-	return s.height, s.crash
+	return s.stopHeight, s.crash
 }
 
 // blockProcessable should be called when new block is processable.
 // It returns boolean indicating if the block should be processed.
 func (s *StopControl) blockProcessable(b *flow.Header) bool {
-
 	s.Lock()
 	defer s.Unlock()
 
@@ -148,9 +181,19 @@ func (s *StopControl) blockProcessable(b *flow.Header) bool {
 		return false
 	}
 
-	// skips blocks at or above requested stop height
-	if b.Height >= s.height {
-		s.log.Warn().Int8("previous_state", int8(s.state)).Int8("new_state", int8(StopControlCommenced)).Msgf("Skipping execution of %s at height %d because stop has been requested at height %d", b.ID(), b.Height, s.height)
+	// skips blocks at or above requested stopHeight
+	if b.Height >= s.stopHeight {
+		s.log.Warn().
+			Int8("previous_state", int8(s.state)).
+			Int8("new_state", int8(StopControlCommenced)).
+			Msgf(
+				"Skipping execution of %s at height %d"+
+					" because stop has been requested at height %d",
+				b.ID(),
+				b.Height,
+				s.stopHeight,
+			)
+
 		s.state = StopControlCommenced // if block was skipped, move into commenced state
 		return false
 	}
@@ -159,7 +202,11 @@ func (s *StopControl) blockProcessable(b *flow.Header) bool {
 }
 
 // blockFinalized should be called when a block is marked as finalized
-func (s *StopControl) blockFinalized(ctx context.Context, execState state.ReadOnlyExecutionState, h *flow.Header) {
+func (s *StopControl) blockFinalized(
+	ctx context.Context,
+	execState state.ReadOnlyExecutionState,
+	h *flow.Header,
+) {
 
 	s.Lock()
 	defer s.Unlock()
@@ -168,29 +215,38 @@ func (s *StopControl) blockFinalized(ctx context.Context, execState state.ReadOn
 		return
 	}
 
-	// Once finalization reached stop height we can be sure no other fork will be valid at this height,
+	// Once finalization reached stopHeight we can be sure no other fork will be valid at this height,
 	// if this block's parent has been executed, we are safe to stop or crash.
 	// This will happen during normal execution, where blocks are executed before they are finalized.
 	// However, it is possible that EN block computation progress can fall behind. In this case,
-	// we want to crash only after the execution reached the stop height.
-	if h.Height == s.height {
+	// we want to crash only after the execution reached the stopHeight.
+	if h.Height == s.stopHeight {
 
 		executed, err := state.IsBlockExecuted(ctx, execState, h.ParentID)
 		if err != nil {
 			// any error here would indicate unexpected storage error, so we crash the node
-			s.log.Fatal().Err(err).Str("block_id", h.ID().String()).Msg("failed to check if the block has been executed")
+			// TODO: what if the error is due to the node being stopped?
+			// i.e. context cancelled?
+			s.log.Fatal().
+				Err(err).
+				Str("block_id", h.ID().String()).
+				Msg("failed to check if the block has been executed")
 			return
 		}
 
 		if executed {
 			s.stopExecution()
 		} else {
 			s.stopAfterExecuting = h.ParentID
-			s.log.Info().Msgf("Node scheduled to stop executing after executing block %s at height %d", s.stopAfterExecuting.String(), h.Height-1)
+			s.log.Info().
+				Msgf(
+					"Node scheduled to stop executing"+
+						" after executing block %s at height %d",
+					s.stopAfterExecuting.String(),
+					h.Height-1,
+				)
 		}
-
 	}
-
 }
 
 // blockExecuted should be called after a block has finished execution
@@ -203,37 +259,61 @@ func (s *StopControl) blockExecuted(h *flow.Header) {
 	}
 
 	if s.stopAfterExecuting == h.ID() {
-		// double check. Even if requested stop height has been changed multiple times,
+		// double check. Even if requested stopHeight has been changed multiple times,
 		// as long as it matches this block we are safe to terminate
-
-		if h.Height == s.height-1 {
+		if h.Height == s.stopHeight-1 {
 			s.stopExecution()
 		} else {
-			s.log.Warn().Msgf("Inconsistent stopping state. Scheduled to stop after executing block ID %s and height %d, but this block has a height %d. ",
-				h.ID().String(), s.height-1, h.Height)
+			s.log.Warn().
+				Msgf(
+					"Inconsistent stopping state. "+
+						"Scheduled to stop after executing block ID %s and height %d, "+
+						"but this block has a height %d. ",
+					h.ID().String(),
+					s.stopHeight-1,
+					h.Height,
+				)
 		}
 	}
 }
 
 func (s *StopControl) stopExecution() {
 	if s.crash {
-		s.log.Fatal().Msgf("Crashing as finalization reached requested stop height %d and the highest executed block is (%d - 1)", s.height, s.height)
-	} else {
-		s.log.Debug().Int8("previous_state", int8(s.state)).Int8("new_state", int8(StopControlPaused)).Msg("StopControl state transition")
-		s.state = StopControlPaused
-		s.log.Warn().Msgf("Pausing execution as finalization reached requested stop height %d", s.height)
+		s.log.Fatal().Msgf(
+			"Crashing as finalization reached requested "+
+				"stop height %d and the highest executed block is (%d - 1)",
+			s.stopHeight,
+			s.stopHeight,
+		)
+		return
 	}
+
+	s.log.Debug().
+		Int8("previous_state", int8(s.state)).
+		Int8("new_state", int8(StopControlPaused)).
+		Msg("StopControl state transition")
+
+	s.state = StopControlPaused
+
+	s.log.Warn().Msgf(
+		"Pausing execution as finalization reached "+
+			"the requested stop height %d",
+		s.stopHeight,
+	)
+
 }
 
-// executingBlockHeight should be called while execution of height starts, used for internal tracking of the minimum
-// possible value of height
+// executingBlockHeight should be called while execution of height starts,
+// used for internal tracking of the minimum possible value of stopHeight
 func (s *StopControl) executingBlockHeight(height uint64) {
+	// TODO: should we lock here?
+
 	if s.state == StopControlPaused {
 		return
 	}
 
-	// updating the highest executing height, which will be used to reject setting stop height that
-	// is too low.
+	// updating the highest executing height, which will be used to reject setting
+	// stopHeight that is too low.
 	if height > s.highestExecutingHeight {
 		s.highestExecutingHeight = height
 	}