refactor(cosmovisor)!: re-implement core logic to address reliability issues

tools/cosmovisor/TEST_SCENARIOS.md

@@ -0,0 +1,17 @@
+- basic: start node, get upgrade-info.json, upgrade
+- manual upgrade added while running:
+  - start node
+  - get upgrade-info.json.batch
+  - restart with halt height
+  - reach halt height
+  - upgrade
+- manual upgrade added while running but get upgrade-info.json:
+  - start node
+  - get upgrade-info.json.batch
+  - restart with halt height
+  - get upgrade-info.json
+  - upgrade
+  - restart with halt height
+  - reach halt height
+  - upgrade
+- start with halt height

tools/cosmovisor/args.go

@@ -1,7 +1,7 @@
 package cosmovisor
 
 import (
-	"encoding/json"
+	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -12,6 +12,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/cosmos/gogoproto/jsonpb"
 	"github.com/pelletier/go-toml/v2"
 	"github.com/spf13/viper"
 
@@ -71,9 +72,6 @@ type Config struct {
 	TimeFormatLogs           string        `toml:"cosmovisor_timeformat_logs" mapstructure:"cosmovisor_timeformat_logs" default:"kitchen"`
 	CustomPreUpgrade         string        `toml:"cosmovisor_custom_preupgrade" mapstructure:"cosmovisor_custom_preupgrade" default:""`
 	DisableRecase            bool          `toml:"cosmovisor_disable_recase" mapstructure:"cosmovisor_disable_recase" default:"false"`
-
-	// currently running upgrade
-	currentUpgrade upgradetypes.Plan
 }
 
 // Root returns the root directory where all info lives
@@ -107,9 +105,14 @@ func (cfg *Config) BaseUpgradeDir() string {
 	return filepath.Join(cfg.Root(), upgradesDir)
 }
 
+// UpgradeInfoDir is the directory where upgrade-info.json is expected to be created by `x/upgrade/keeper`.
+func (cfg *Config) UpgradeInfoDir() string {
+	return filepath.Join(cfg.Home, "data")
+}
+
 // UpgradeInfoFilePath is the expected upgrade-info filename created by `x/upgrade/keeper`.
 func (cfg *Config) UpgradeInfoFilePath() string {
-	return filepath.Join(cfg.Home, "data", upgradetypes.UpgradeInfoFilename)
+	return filepath.Join(cfg.UpgradeInfoDir(), upgradetypes.UpgradeInfoFilename)
 }
 
 // UpgradeInfoBatchFilePath is the same as UpgradeInfoFilePath but with a batch suffix.
@@ -408,7 +411,6 @@ func (cfg *Config) SetCurrentUpgrade(u upgradetypes.Plan) (rerr error) {
 		return fmt.Errorf("creating current symlink: %w", err)
 	}
 
-	cfg.currentUpgrade = u
 	f, err := os.Create(filepath.Join(cfg.Root(), upgrade, upgradetypes.UpgradeInfoFilename))
 	if err != nil {
 		return err
@@ -420,39 +422,63 @@ func (cfg *Config) SetCurrentUpgrade(u upgradetypes.Plan) (rerr error) {
 		}
 	}()
 
-	bz, err := json.Marshal(u)
+	out, err := (&jsonpb.Marshaler{}).MarshalToString(&u)
 	if err != nil {
 		return err
 	}
-	_, err = f.Write(bz)
+	_, err = f.Write([]byte(out))
 	return err
 }
 
 // UpgradeInfo returns the current upgrade info
 func (cfg *Config) UpgradeInfo() (upgradetypes.Plan, error) {
-	if cfg.currentUpgrade.Name != "" {
-		return cfg.currentUpgrade, nil
-	}
-
-	filename := filepath.Join(cfg.Root(), currentLink, upgradetypes.UpgradeInfoFilename)
+	filename := cfg.UpgradeInfoFilePath()
+	fmt.Printf("Reading upgrade info from %q\n", filename)
 	_, err := os.Lstat(filename)
-	var u upgradetypes.Plan
 	var bz []byte
 	if err != nil { // no current directory
-		goto returnError
+		return upgradetypes.Plan{}, fmt.Errorf("failed to read %q: %w", filename, err)
 	}
 	if bz, err = os.ReadFile(filename); err != nil {
-		goto returnError
+		return upgradetypes.Plan{}, fmt.Errorf("failed to read %q: %w", filename, err)
 	}
-	if err = json.Unmarshal(bz, &u); err != nil {
-		goto returnError
+	return cfg.ParseUpgradeInfo(bz)
+}
+
+func (cfg *Config) ParseUpgradeInfo(bz []byte) (upgradetypes.Plan, error) {
+	var upgradePlan upgradetypes.Plan
+	if err := jsonpb.Unmarshal(bytes.NewReader(bz), &upgradePlan); err != nil {
+		return upgradetypes.Plan{}, fmt.Errorf("error unmarshalling upgrade info: %w", err)
+	}
+	if err := upgradePlan.ValidateBasic(); err != nil {
+		return upgradetypes.Plan{}, fmt.Errorf("upgrade info failed validation upgrade inof: %w", err)
 	}
-	cfg.currentUpgrade = u
-	return cfg.currentUpgrade, nil
+	if !cfg.DisableRecase {
+		upgradePlan.Name = strings.ToLower(upgradePlan.Name)
+	}
+	return upgradePlan, nil
+}
+
+const LastKnownHeightFile = ".last_known_height"
+
+func (cfg Config) ReadLastKnownHeight() uint64 {
+	filename := filepath.Join(cfg.UpgradeInfoDir(), LastKnownHeightFile)
+	bz, err := os.ReadFile(filename)
+	if err != nil {
+		return 0
+	}
+
+	h, err := strconv.ParseUint(string(bz), 10, 64)
+	if err != nil {
+		return 0
+	}
+
+	return h
+}
 
-returnError:
-	cfg.currentUpgrade.Name = "_"
-	return cfg.currentUpgrade, fmt.Errorf("failed to read %q: %w", filename, err)
+func (cfg Config) WriteLastKnownHeight(height uint64) error {
+	filename := filepath.Join(cfg.UpgradeInfoDir(), LastKnownHeightFile)
+	return os.WriteFile(filename, []byte(strconv.FormatUint(height, 10)), 0644)
 }
 
 // BooleanOption checks and validate env option
@@ -575,7 +601,7 @@ func (cfg Config) DetailString() string {
 	var sb strings.Builder
 	sb.WriteString("Configurable Values:\n")
 	for _, kv := range configEntries {
-		fmt.Fprintf(&sb, "  %s: %s\n", kv.name, kv.value)
+		_, _ = fmt.Fprintf(&sb, "  %s: %s\n", kv.name, kv.value)
 	}
 	sb.WriteString("Derived Values:\n")
 	dnl := 0
@@ -586,7 +612,7 @@ func (cfg Config) DetailString() string {
 	}
 	dFmt := fmt.Sprintf("  %%%ds: %%s\n", dnl)
 	for _, kv := range derivedEntries {
-		fmt.Fprintf(&sb, dFmt, kv.name, kv.value)
+		_, _ = fmt.Fprintf(&sb, dFmt, kv.name, kv.value)
 	}
 	return sb.String()
 }
@@ -614,7 +640,9 @@ func (cfg Config) Export() (string, error) {
 	// convert the time value to its format option
 	cfg.TimeFormatLogs = ValueToTimeFormatOption(cfg.TimeFormatLogs)
 
-	defer file.Close()
+	defer func(file *os.File) {
+		_ = file.Close()
+	}(file)
 
 	// write the configuration to the file
 	err = toml.NewEncoder(file).Encode(cfg)

tools/cosmovisor/cmd/cosmovisor/add_upgrade.go

@@ -1,7 +1,6 @@
 package main
 
 import (
-	"encoding/json"
 	"fmt"
 	"os"
 	"path"
@@ -10,8 +9,6 @@ import (
 	"github.com/spf13/cobra"
 
 	"cosmossdk.io/tools/cosmovisor"
-
-	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )
 
 func NewAddUpgradeCmd() *cobra.Command {
@@ -23,14 +20,15 @@ func NewAddUpgradeCmd() *cobra.Command {
 		RunE:         addUpgradeCmd,
 	}
 
-	addUpgrade.Flags().Bool(cosmovisor.FlagForce, false, "overwrite existing upgrade binary / upgrade-info.json file")
+	addUpgrade.Flags().Bool(cosmovisor.FlagForce, false, "overwrite existing upgrade binary and plan with the same name")
 	addUpgrade.Flags().Int64(cosmovisor.FlagUpgradeHeight, 0, "define a height at which to upgrade the binary automatically (without governance proposal)")
 
 	return addUpgrade
 }
 
 // addUpgrade adds upgrade info to manifest
-func addUpgrade(cfg *cosmovisor.Config, force bool, upgradeHeight int64, upgradeName, executablePath, upgradeInfoPath string) error {
+// TODO batch-upgrade and add-upgrade should write to the same batch file
+func addUpgrade(cfg *cosmovisor.Config, force bool, upgradeHeight int64, upgradeName, executablePath string) error {
 	logger := cfg.Logger(os.Stdout)
 
 	if !cfg.DisableRecase {
@@ -65,22 +63,19 @@ func addUpgrade(cfg *cosmovisor.Config, force bool, upgradeHeight int64, upgrade
 	logger.Info(fmt.Sprintf("Upgrade binary located at %s", cfg.UpgradeBin(upgradeName)))
 
 	if upgradeHeight > 0 {
-		plan := upgradetypes.Plan{Name: upgradeName, Height: upgradeHeight}
-		if err := plan.ValidateBasic(); err != nil {
-			panic(fmt.Errorf("something is wrong with cosmovisor: %w", err))
+		plan := &cosmovisor.ManualUpgradePlan{
+			Name:   upgradeName,
+			Height: upgradeHeight,
 		}
-
-		// create upgrade-info.json file
-		planData, err := json.Marshal(plan)
-		if err != nil {
-			return fmt.Errorf("failed to marshal upgrade plan: %w", err)
+		if err := plan.ValidateBasic(); err != nil {
+			panic(fmt.Errorf("invalid manual upgrade plan: %w", err))
 		}
 
-		if err := saveOrAbort(upgradeInfoPath, planData, force); err != nil {
-			return err
+		if err := cosmovisor.AddManualUpgrade(cfg, plan, force); err != nil {
+			panic(fmt.Errorf("failed to add manual upgrade: %w", err))
 		}
 
-		logger.Info(fmt.Sprintf("%s created, %s upgrade binary will switch at height %d", upgradeInfoPath, upgradeName, upgradeHeight))
+		logger.Info(fmt.Sprintf("added manual upgrade, node will be set to halt at height %d, and binary for upgrade %q will be activated", upgradeHeight, upgradeName))
 	}
 
 	return nil
@@ -119,7 +114,7 @@ func addUpgradeCmd(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("failed to get upgrade-height flag: %w", err)
 	}
 
-	return addUpgrade(cfg, force, upgradeHeight, upgradeName, executablePath, cfg.UpgradeInfoFilePath())
+	return addUpgrade(cfg, force, upgradeHeight, upgradeName, executablePath)
 }
 
 // saveOrAbort saves data to path or aborts if file exists and force is false

tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go

@@ -2,7 +2,6 @@ package main
 
 import (
 	"encoding/csv"
-	"encoding/json"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -49,7 +48,7 @@ cosmovisor add-batch-upgrade --upgrade-file /path/to/batch_upgrade.csv`,
 
 // addBatchUpgrade takes in multiple specified upgrades and creates a single
 // batch upgrade file out of them
-func addBatchUpgrade(cmd *cobra.Command, args []string) error {
+func addBatchUpgrade(cmd *cobra.Command, _ []string) error {
 	cfg, err := getConfigFromCmd(cmd)
 	if err != nil {
 		return err
@@ -71,7 +70,6 @@ func addBatchUpgrade(cmd *cobra.Command, args []string) error {
 
 // processUpgradeList takes in a list of upgrades and creates a batch upgrade file
 func processUpgradeList(cfg *cosmovisor.Config, upgradeList [][]string) error {
-	upgradeInfoPaths := []string{}
 	for i, upgrade := range upgradeList {
 		if len(upgrade) != 3 {
 			return fmt.Errorf("argument at position %d (%s) is invalid", i, upgrade)
@@ -82,42 +80,12 @@ func processUpgradeList(cfg *cosmovisor.Config, upgradeList [][]string) error {
 		if err != nil {
 			return fmt.Errorf("upgrade height at position %d (%s) is invalid", i, upgrade[2])
 		}
-		upgradeInfoPath := cfg.UpgradeInfoFilePath() + "." + upgradeName
-		upgradeInfoPaths = append(upgradeInfoPaths, upgradeInfoPath)
-		if err := addUpgrade(cfg, true, upgradeHeight, upgradeName, upgradePath, upgradeInfoPath); err != nil {
-			return err
-		}
-	}
-
-	var allData []json.RawMessage
-	for _, uip := range upgradeInfoPaths {
-		fileData, err := os.ReadFile(uip)
-		if err != nil {
-			return fmt.Errorf("error reading file %s: %w", uip, err)
-		}
 
-		// Verify it's valid JSON
-		var jsonData json.RawMessage
-		if err := json.Unmarshal(fileData, &jsonData); err != nil {
-			return fmt.Errorf("error parsing JSON from file %s: %w", uip, err)
+		// we use the same logic as the add-upgrade command here, appending to any existing manual upgrade data
+		if err := addUpgrade(cfg, true, upgradeHeight, upgradeName, upgradePath); err != nil {
+			return err
 		}
-
-		// Add to our slice
-		allData = append(allData, jsonData)
 	}
-
-	// Marshal the combined data
-	batchData, err := json.MarshalIndent(allData, "", "  ")
-	if err != nil {
-		return fmt.Errorf("error marshaling combined JSON: %w", err)
-	}
-
-	// Write to output file
-	err = os.WriteFile(cfg.UpgradeInfoBatchFilePath(), batchData, 0o600)
-	if err != nil {
-		return fmt.Errorf("error writing combined JSON to file: %w", err)
-	}
-
 	return nil
 }
 
@@ -127,7 +95,9 @@ func processUpgradeFile(cfg *cosmovisor.Config, upgradeFile string) error {
 	if err != nil {
 		return fmt.Errorf("error opening upgrade CSV file %s: %w", upgradeFile, err)
 	}
-	defer file.Close()
+	defer func(file *os.File) {
+		_ = file.Close()
+	}(file)
 
 	r := csv.NewReader(file)
 	r.FieldsPerRecord = 3

tools/cosmovisor/cmd/cosmovisor/init.go

@@ -10,6 +10,7 @@ import (
 	"github.com/spf13/cobra"
 
 	"cosmossdk.io/log"
+
 	"cosmossdk.io/tools/cosmovisor"
 
 	"github.com/cosmos/cosmos-sdk/x/upgrade/plan"

tools/cosmovisor/cmd/cosmovisor/main.go

@@ -3,10 +3,13 @@ package main
 import (
 	"context"
 	"os"
+	"os/signal"
+	"syscall"
 )
 
 func main() {
-	if err := NewRootCmd().ExecuteContext(context.Background()); err != nil {
+	ctx, _ := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
+	if err := NewRootCmd().ExecuteContext(ctx); err != nil {
 		os.Exit(1)
 	}
 }