ADR 101: Refactor height check-related logic and tests (cometbft#1271)

* state: Invert logic of whether retain heights are set to be more readable Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor findMinRetainHeight logic for clarity and to respect config Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Rename findMinRetainHeight to findMinBlockRetainHeight for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Rename Pruner.SetApplicationRetainHeight to SetApplicationBlockRetainHeight for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Rename pruner SetCompanionRetainHeight to SetCompanionBlockRetainHeight for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable name Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - rename local method name for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - remove redundant condition Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - use helper instead of redefining logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable names Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable names Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor Pruner.SetApplicationBlockRetainHeight logic The application retain height should be set regardless of what the companion retain height is. Pruning should take place based on whichever of the two values is lower. There is no need to consider the companion retain height in this logic. Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor Pruner.SetCompanionBlockRetainHeight logic Similar refactor to that done for Pruner.SetApplicationBlockRetainHeight. There is no need to consider the application retain height during this function call. Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor Pruner.SetABCIResRetainHeight logic Aligns the logic of this method similar to that in the other retain height setter methods. Also fixes a logic bug where setting the retain height would skip updating the metrics. Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable names Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Simplify pruner logic assuming retain heights are always set We can simplify the pruner logic if we assume that the initial retain heights are always set on node startup. Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Tell pruner whether companion is enabled Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Expand error message detail Signed-off-by: Thane Thomson <connect@thanethomson.com> * state+store: Align tests with new logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Refactor - shorten local variable names Signed-off-by: Thane Thomson <connect@thanethomson.com> * rpc/grpc: Return trace IDs in error responses from pruning service Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Refactor/expand pruning service initialization logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Minor cosmetic tweaks to gRPC tests Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Enable data companion pruning Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Refactor - extract pruner creation as function Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Only enable companion-based pruning on full nodes and validators Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Only start ABCI results pruning routine when data companion is enabled Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Fix TestMinRetainHeight logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Fix companion retain height initialization logic Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Expand testing framework Allows explicit control over whether or not pruning should take a data companion into account for full nodes or validators in our E2E framework. This allows greater flexibility in the E2E framework to test nodes both with and without data companion-related functionality. Expands our standard CI manifest to expect data companions attached to two of the nodes. Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Rename background routines Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Apply change from code review Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com>
osmosis-labs · Jan 24, 2024 · d736369 · d736369
1 parent bc6e5f2
commit d736369
Show file tree

Hide file tree

Showing 13 changed files with 335 additions and 230 deletions.
diff --git a/consensus/state.go b/consensus/state.go
@@ -1722,20 +1722,19 @@ func (cs *State) finalizeCommit(height int64) {
 	fail.Fail() // XXX
 
 	if retainHeight > 0 && cs.blockExec.Pruner() != nil {
-		err := cs.blockExec.Pruner().SetApplicationRetainHeight(retainHeight)
+		err := cs.blockExec.Pruner().SetApplicationBlockRetainHeight(retainHeight)
 		if err != nil {
 			logger.Error("failed to prune blocks", "retain_height", retainHeight, "err", err)
 		} else {
 			logger.Error("Failed to set application retain height", "retainHeight", retainHeight, "err", err)
 		}
 	}
+
 	// Prune old heights, if requested by ABCI app.
-	if retainHeight > 0 {
-		pruned, err := cs.pruneBlocks(retainHeight)
+	if retainHeight > 0 && blockExec.pruner != nil {
+		err := blockExec.pruner.SetApplicationBlockRetainHeight(retainHeight)
 		if err != nil {
-			logger.Error("failed to prune blocks", "retain_height", retainHeight, "err", err)
-		} else {
-			logger.Debug("pruned blocks", "pruned", pruned, "retain_height", retainHeight)
+			blockExec.logger.Error("Failed to set application block retain height", "retainHeight", retainHeight, "err", err)
 		}
 	}
 

diff --git a/node/node.go b/node/node.go
@@ -954,26 +954,16 @@ func NewNodeWithContext(ctx context.Context,
 		return nil, err
 	}
 
-	err = initApplicationRetainHeight(stateStore)
-	if err != nil {
-		return nil, err
-	}
-
-	err = initCompanionBlockRetainHeight(
+	pruner, err := createPruner(
+		config,
 		stateStore,
-		config.Storage.Pruning.DataCompanion.Enabled,
-		config.Storage.Pruning.DataCompanion.InitialBlockRetainHeight,
+		blockStore,
+		smMetrics,
+		logger.With("module", "state"),
 	)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create pruner: %w", err)
 	}
-	pruner := sm.NewPruner(
-		stateStore,
-		blockStore,
-		logger,
-		sm.WithPrunerInterval(config.Storage.Pruning.Interval),
-		sm.WithPrunerMetrics(smMetrics),
-	)
 
 	// make block executor for consensus and blocksync reactors to execute blocks
 	blockExec := sm.NewBlockExecutor(
@@ -1686,6 +1676,40 @@ func splitAndTrimEmpty(s, sep, cutset string) []string {
 // Set the initial application retain height to 0 to avoid the data companion pruning blocks
 // before the application indicates it is ok
 // We set this to 0 only if the retain height was not set before by the application
+func createPruner(
+	config *cfg.Config,
+	stateStore sm.Store,
+	blockStore *store.BlockStore,
+	metrics *sm.Metrics,
+	logger log.Logger,
+) (*sm.Pruner, error) {
+	if err := initApplicationRetainHeight(stateStore); err != nil {
+		return nil, err
+	}
+
+	prunerOpts := []sm.PrunerOption{
+		sm.WithPrunerInterval(config.Storage.Pruning.Interval),
+		sm.WithPrunerMetrics(metrics),
+	}
+
+	if config.Storage.Pruning.DataCompanion.Enabled {
+		err := initCompanionRetainHeights(
+			stateStore,
+			config.Storage.Pruning.DataCompanion.InitialBlockRetainHeight,
+			config.Storage.Pruning.DataCompanion.InitialBlockResultsRetainHeight,
+		)
+		if err != nil {
+			return nil, err
+		}
+		prunerOpts = append(prunerOpts, sm.WithPrunerCompanionEnabled())
+	}
+
+	return sm.NewPruner(stateStore, blockStore, logger, prunerOpts...), nil
+}
+
+// Set the initial application retain height to 0 to avoid the data companion
+// pruning blocks before the application indicates it is OK. We set this to 0
+// only if the retain height was not set before by the application.
 func initApplicationRetainHeight(stateStore sm.Store) error {
 	if _, err := stateStore.GetApplicationRetainHeight(); err != nil {
 		if errors.Is(err, sm.ErrKeyNotFound) {
@@ -1696,27 +1720,27 @@ func initApplicationRetainHeight(stateStore sm.Store) error {
 	return nil
 }
 
-func initCompanionBlockRetainHeight(stateStore sm.Store, companionEnabled bool, initialRetainHeight int64) error {
-	if _, err := stateStore.GetCompanionBlockRetainHeight(); err != nil {
-		// If the data companion block retain height has not yet been set in
-		// the database
-		if errors.Is(err, sm.ErrKeyNotFound) {
-			if companionEnabled && initialRetainHeight > 0 {
-				// This will set the data companion retain height into the
-				// database. We bypass the sanity checks by
-				// pruner.SetCompanionBlockRetainHeight. These checks do not
-				// allow a retain height below the current blockstore height or
-				// above the blockstore height  to be set. But this is a retain
-				// height that can be set before the chain starts to indicate
-				// potentially that no pruning should be done before the data
-				// companion comes online.
-				err = stateStore.SaveCompanionBlockRetainHeight(initialRetainHeight)
-				if err != nil {
-					return fmt.Errorf("failed to set initial data companion block retain height: %w", err)
-				}
-			}
-		} else {
-			return fmt.Errorf("failed to obtain companion retain height: %w", err)
+// Sets the data companion retain heights if one of two possible conditions is
+// met:
+// 1. One or more of the retain heights has not yet been set.
+// 2. One or more of the retain heights is currently 0.
+func initCompanionRetainHeights(stateStore sm.Store, initBlockRH, initBlockResultsRH int64) error {
+	curBlockRH, err := stateStore.GetCompanionBlockRetainHeight()
+	if err != nil && !errors.Is(err, sm.ErrKeyNotFound) {
+		return fmt.Errorf("failed to obtain companion block retain height: %w", err)
+	}
+	if curBlockRH == 0 {
+		if err := stateStore.SaveCompanionBlockRetainHeight(initBlockRH); err != nil {
+			return fmt.Errorf("failed to set initial data companion block retain height: %w", err)
+		}
+	}
+	curBlockResultsRH, err := stateStore.GetABCIResRetainHeight()
+	if err != nil && !errors.Is(err, sm.ErrKeyNotFound) {
+		return fmt.Errorf("failed to obtain companion block results retain height: %w", err)
+	}
+	if curBlockResultsRH == 0 {
+		if err := stateStore.SaveABCIResRetainHeight(initBlockResultsRH); err != nil {
+			return fmt.Errorf("failed to set initial data companion block results retain height: %w", err)
 		}
 	}
 	return nil

diff --git a/state/errors.go b/state/errors.go
@@ -52,6 +52,11 @@ type (
 		Height int64
 	}
 
+	ErrPrunerFailedToGetRetainHeight struct {
+		Which string
+		Err   error
+	}
+
 	ErrPrunerFailedToLoadState struct {
 		Err error
 	}
@@ -119,6 +124,14 @@ func (e ErrNoABCIResponsesForHeight) Error() string {
 
 var ErrABCIResponsesNotPersisted = errors.New("node is not persisting abci responses")
 
+func (e ErrPrunerFailedToGetRetainHeight) Error() string {
+	return fmt.Sprintf("pruner failed to get existing %s retain height: %s", e.Which, e.Err.Error())
+}
+
+func (e ErrPrunerFailedToGetRetainHeight) Unwrap() error {
+	return e.Err
+}
+
 func (e ErrPrunerFailedToLoadState) Error() string {
 	return fmt.Sprintf("failed to load state, cannot prune: %s", e.Err.Error())
 }

diff --git a/state/export_test.go b/state/export_test.go
@@ -46,10 +46,11 @@ func SaveValidatorsInfo(db dbm.DB, height, lastHeightChanged int64, valSet *type
 	return stateStore.saveValidatorsInfo(height, lastHeightChanged, valSet)
 }
 
-// FindMinRetainHeight is an alias for the private findMinRetainHeight method
-// in pruner.go, exported exclusively and expicitly for testing.
+// FindMinBlockRetainHeight is an alias for the private
+// findMinBlockRetainHeight method in pruner.go, exported exclusively and
+// expicitly for testing.
 func (p *Pruner) FindMinRetainHeight() int64 {
-	return p.findMinRetainHeight()
+	return p.findMinBlockRetainHeight()
 }
 
 func (p *Pruner) PruneABCIResToRetainHeight(lastRetainHeight int64) int64 {