Merge pull request onflow#5938 from onflow/jord/dynamic-bootstrap-err…

…or-if-snapshot-file-exists

Require either a root snapshot file, or Dynamic Startup flags

cmd/dynamic_startup.go

@@ -47,45 +47,59 @@ func ValidateDynamicStartupFlags(accessPublicKey, accessAddress string, startPha
 // DynamicStartPreInit is the pre-init func that will check if a node has already bootstrapped
 // from a root protocol snapshot. If not attempt to get a protocol snapshot where the following
 // conditions are met.
-// 1. Target epoch < current epoch (in the past), set root snapshot to current snapshot
-// 2. Target epoch == "current", wait until target phase == current phase before setting root snapshot
-// 3. Target epoch > current epoch (in future), wait until target epoch and target phase is reached before
+//  1. Target epoch < current epoch (in the past), set root snapshot to current snapshot
+//  2. Target epoch == "current", wait until target phase == current phase before setting root snapshot
+//  3. Target epoch > current epoch (in future), wait until target epoch and target phase is reached before
+//
 // setting root snapshot
 func DynamicStartPreInit(nodeConfig *NodeConfig) error {
 	ctx := context.Background()
 
 	log := nodeConfig.Logger.With().Str("component", "dynamic-startup").Logger()
 
-	// skip dynamic startup if the protocol state is bootstrapped
+	// CASE 1: The state is already bootstrapped - nothing to do
 	isBootstrapped, err := badgerstate.IsBootstrapped(nodeConfig.DB)
 	if err != nil {
 		return fmt.Errorf("could not check if state is boostrapped: %w", err)
 	}
 	if isBootstrapped {
-		log.Info().Msg("protocol state already bootstrapped, skipping dynamic startup")
+		log.Debug().Msg("protocol state already bootstrapped, skipping dynamic startup")
 		return nil
 	}
 
-	// skip dynamic startup if a root snapshot file is specified - this takes priority
+	// CASE 2: The state is not already bootstrapped.
+	// We will either bootstrap from a file or using Dynamic Startup.
 	rootSnapshotPath := filepath.Join(nodeConfig.BootstrapDir, bootstrap.PathRootProtocolStateSnapshot)
-	if utilsio.FileExists(rootSnapshotPath) {
-		log.Info().
+	rootSnapshotFileExists := utilsio.FileExists(rootSnapshotPath)
+	dynamicStartupFlagsSet := anyDynamicStartupFlagsAreSet(nodeConfig)
+
+	// If the user has provided both a root snapshot file AND dynamic startup specification, return an error.
+	// Previously, the snapshot file would take precedence over the Dynamic Startup flags.
+	// This caused operators to inadvertently bootstrap from an old snapshot file when attempting to use Dynamic Startup.
+	// Therefore, we instead require the operator to explicitly choose one option or the other.
+	if rootSnapshotFileExists && dynamicStartupFlagsSet {
+		return fmt.Errorf("must specify either a root snapshot file (%s) or Dynamic Startup flags (--dynamic-startup-*) but not both", rootSnapshotPath)
+	}
+
+	// CASE 2.1: Use the root snapshot file to bootstrap.
+	if rootSnapshotFileExists {
+		log.Debug().
 			Str("root_snapshot_path", rootSnapshotPath).
 			Msg("protocol state is not bootstrapped, will bootstrap using configured root snapshot file, skipping dynamic startup")
 		return nil
 	}
 
+	// CASE 2.2: Use Dynamic Startup to bootstrap.
+
 	// get flow client with secure client connection to download protocol snapshot from access node
 	config, err := common.NewFlowClientConfig(nodeConfig.DynamicStartupANAddress, nodeConfig.DynamicStartupANPubkey, flow.ZeroID, false)
 	if err != nil {
 		return fmt.Errorf("failed to create flow client config for node dynamic startup pre-init: %w", err)
 	}
-
 	flowClient, err := common.FlowClient(config)
 	if err != nil {
 		return fmt.Errorf("failed to create flow client for node dynamic startup pre-init: %w", err)
 	}
-
 	getSnapshotFunc := func(ctx context.Context) (protocol.Snapshot, error) {
 		return common.GetSnapshot(ctx, flowClient)
 	}
@@ -95,7 +109,6 @@ func DynamicStartPreInit(nodeConfig *NodeConfig) error {
 	if err != nil {
 		return fmt.Errorf("failed to validate flag --dynamic-start-epoch: %w", err)
 	}
-
 	startupPhase := flow.GetEpochPhase(nodeConfig.DynamicStartupEpochPhase)
 
 	// validate the rest of the dynamic startup flags
@@ -121,6 +134,16 @@ func DynamicStartPreInit(nodeConfig *NodeConfig) error {
 	return nil
 }
 
+// anyDynamicStartupFlagsAreSet returns true if either the AN address or AN public key for Dynamic Startup are set.
+// All other Dynamic Startup flags have default values (and aren't required) hence they aren't checked here.
+// Both these flags must be set for Dynamic Startup to occur.
+func anyDynamicStartupFlagsAreSet(config *NodeConfig) bool {
+	if len(config.DynamicStartupANAddress) > 0 || len(config.DynamicStartupANPubkey) > 0 {
+		return true
+	}
+	return false
+}
+
 // validateDynamicStartEpochFlags parse the start epoch flag and return the uin64 value,
 // if epoch = current return the current epoch counter
 func validateDynamicStartEpochFlags(ctx context.Context, getSnapshot common.GetProtocolSnapshot, flagEpoch string) (uint64, error) {

cmd/dynamic_startup_test.go

@@ -29,6 +29,7 @@ func getMockSnapshot(t *testing.T, epochCounter uint64, phase flow.EpochPhase) *
 	snapshot := new(protocolmock.Snapshot)
 	snapshot.On("Epochs").Return(epochQuery)
 	snapshot.On("Phase").Return(phase, nil)
+	snapshot.On("Head").Return(unittest.BlockHeaderFixture(), nil)
 
 	return snapshot
 }

cmd/util/cmd/common/snapshot.go

@@ -9,6 +9,8 @@ import (
 	"github.com/rs/zerolog"
 	"github.com/sethvargo/go-retry"
 
+	"github.com/onflow/flow-go/utils/logging"
+
 	"github.com/onflow/flow-go-sdk/access/grpc"
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/state/protocol"
@@ -88,10 +90,16 @@ func GetSnapshotAtEpochAndPhase(ctx context.Context, log zerolog.Logger, startup
 
 		// check if we are in or past the target epoch and phase
 		if currEpochCounter > startupEpoch || (currEpochCounter == startupEpoch && currEpochPhase >= startupEpochPhase) {
+			head, err := snapshot.Head()
+			if err != nil {
+				return fmt.Errorf("could not get Dynamic Startup snapshot header: %w", err)
+			}
 			log.Info().
-				Dur("time-waiting", time.Since(start)).
-				Uint64("current-epoch", currEpochCounter).
-				Str("current-epoch-phase", currEpochPhase.String()).
+				Dur("time_waiting", time.Since(start)).
+				Uint64("current_epoch", currEpochCounter).
+				Str("current_epoch_phase", currEpochPhase.String()).
+				Hex("finalized_root_block_id", logging.ID(head.ID())).
+				Uint64("finalized_block_height", head.Height).
 				Msg("finished dynamic startup - reached desired epoch and phase")
 
 			return nil