Merge pull request #8446 from yyforyongyu/validate-deprecation

lnd: add warning logs if deprecated config options are used

config.go

@@ -346,7 +346,7 @@ type Config struct {
 	BlockingProfile int `long:"blockingprofile" description:"Used to enable a blocking profile to be served on the profiling port. This takes a value from 0 to 1, with 1 including every blocking event, and 0 including no events."`
 	MutexProfile    int `long:"mutexprofile" description:"Used to Enable a mutex profile to be served on the profiling port. This takes a value from 0 to 1, with 1 including every mutex event, and 0 including no events."`
 
-	UnsafeDisconnect   bool   `long:"unsafe-disconnect" description:"DEPRECATED: Allows the rpcserver to intentionally disconnect from peers with open channels. THIS FLAG WILL BE REMOVED IN 0.10.0"`
+	UnsafeDisconnect   bool   `long:"unsafe-disconnect" description:"DEPRECATED: Allows the rpcserver to intentionally disconnect from peers with open channels. THIS FLAG WILL BE REMOVED IN 0.10.0" hidden:"true"`
 	UnsafeReplay       bool   `long:"unsafe-replay" description:"Causes a link to replay the adds on its commitment txn after starting up, this enables testing of the sphinx replay logic."`
 	MaxPendingChannels int    `long:"maxpendingchannels" description:"The maximum number of incoming pending channels permitted per peer."`
 	BackupFilePath     string `long:"backupfilepath" description:"The target location of the channel backup file"`
@@ -576,7 +576,6 @@ func DefaultConfig() Config {
 			UserAgentVersion: neutrino.UserAgentVersion,
 		},
 		BlockCacheSize:     defaultBlockCacheSize,
-		UnsafeDisconnect:   true,
 		MaxPendingChannels: lncfg.DefaultMaxPendingChannels,
 		NoSeedBackup:       defaultNoSeedBackup,
 		MinBackoff:         defaultMinBackoff,
@@ -813,6 +812,9 @@ func LoadConfig(interceptor signal.Interceptor) (*Config, error) {
 		ltndLog.Warnf("%v", configFileError)
 	}
 
+	// Finally, log warnings for deprecated config options if they are set.
+	logWarningsForDeprecation(*cleanCfg)
+
 	return cleanCfg, nil
 }
 
@@ -2112,12 +2114,20 @@ func checkEstimateMode(estimateMode string) error {
 		bitcoindEstimateModes[:])
 }
 
-// configToFlatMap converts the given config struct into a flat map of key/value
-// pairs using the dot notation we are used to from the config file or command
-// line flags.
-func configToFlatMap(cfg Config) (map[string]string, error) {
+// configToFlatMap converts the given config struct into a flat map of
+// key/value pairs using the dot notation we are used to from the config file
+// or command line flags. It also returns a map containing deprecated config
+// options.
+func configToFlatMap(cfg Config) (map[string]string,
+	map[string]struct{}, error) {
+
 	result := make(map[string]string)
 
+	// deprecated stores a map of deprecated options found in the config
+	// that are set by the users. A config option is considered as
+	// deprecated if it has a `hidden` flag.
+	deprecated := make(map[string]struct{})
+
 	// redact is the helper function that redacts sensitive values like
 	// passwords.
 	redact := func(key, value string) string {
@@ -2159,6 +2169,8 @@ func configToFlatMap(cfg Config) (map[string]string, error) {
 			longName := fieldType.Tag.Get("long")
 			namespace := fieldType.Tag.Get("namespace")
 			group := fieldType.Tag.Get("group")
+			hidden := fieldType.Tag.Get("hidden")
+
 			switch {
 			// We have a long name defined, this is a config value.
 			case longName != "":
@@ -2172,6 +2184,11 @@ func configToFlatMap(cfg Config) (map[string]string, error) {
 					"%v", field.Interface(),
 				))
 
+				// If there's a hidden flag, it's deprecated.
+				if hidden == "true" && !field.IsZero() {
+					deprecated[key] = struct{}{}
+				}
+
 			// We have no long name but a namespace, this is a
 			// nested struct.
 			case longName == "" && namespace != "":
@@ -2202,5 +2219,18 @@ func configToFlatMap(cfg Config) (map[string]string, error) {
 	// Turn the whole config struct into a flat map.
 	printConfig(reflect.ValueOf(cfg), "")
 
-	return result, nil
+	return result, deprecated, nil
+}
+
+// logWarningsForDeprecation logs a warning if a deprecated config option is
+// set.
+func logWarningsForDeprecation(cfg Config) {
+	_, deprecated, err := configToFlatMap(cfg)
+	if err != nil {
+		ltndLog.Errorf("Convert configs to map: %v", err)
+	}
+
+	for k := range deprecated {
+		ltndLog.Warnf("Config '%s' is deprecated, please remove it", k)
+	}
 }

config_test.go

@@ -24,9 +24,15 @@ func TestConfigToFlatMap(t *testing.T) {
 	cfg.DB.Etcd.Pass = testPassword
 	cfg.DB.Postgres.Dsn = testPassword
 
-	result, err := configToFlatMap(cfg)
+	// Set a deprecated field.
+	cfg.Bitcoin.Active = true
+
+	result, deprecated, err := configToFlatMap(cfg)
 	require.NoError(t, err)
 
+	// Check that the deprecated option has been parsed out.
+	require.Contains(t, deprecated, "bitcoin.active")
+
 	// Pick a couple of random values to check.
 	require.Equal(t, DefaultLndDir, result["lnddir"])
 	require.Equal(

docs/release-notes/release-notes-0.18.0.md

@@ -172,6 +172,9 @@
   contract court logs in case of timed-out htlcs in order to easily spot dust
   outputs.
 
+* [Add warning logs](https://github.com/lightningnetwork/lnd/pull/8446) during
+  startup when deprecated config options are used.
+
 ## RPC Updates
 
 * [Deprecated](https://github.com/lightningnetwork/lnd/pull/7175)

lncfg/chain.go

@@ -10,7 +10,7 @@ import (
 //
 //nolint:lll
 type Chain struct {
-	Active   bool   `long:"active" description:"DEPRECATED: If the chain should be active or not. This field is now ignored since only the Bitcoin chain is supported"`
+	Active   bool   `long:"active" description:"DEPRECATED: If the chain should be active or not. This field is now ignored since only the Bitcoin chain is supported" hidden:"true"`
 	ChainDir string `long:"chaindir" description:"The directory to store the chain's data within."`
 
 	Node string `long:"node" description:"The blockchain interface to use." choice:"btcd" choice:"bitcoind" choice:"neutrino" choice:"nochainbackend"`

lncfg/dev.go

@@ -24,6 +24,17 @@ func (d *DevConfig) ChannelReadyWait() time.Duration {
 	return 0
 }
 
+// GetUnsafeDisconnect returns the config value, which is always true for
+// production build.
+//
+// TODO(yy): this is a temporary solution to allow users to reconnect peers to
+// trigger a reestablishiment for the active channels. Once a new dedicated RPC
+// is added to realize that functionality, this function should return false to
+// forbidden disconnecting peers while there are active channels.
+func (d *DevConfig) GetUnsafeDisconnect() bool {
+	return true
+}
+
 // GetReservationTimeout returns the config value for `ReservationTimeout`.
 func (d *DevConfig) GetReservationTimeout() time.Duration {
 	return DefaultReservationTimeout

lncfg/dev_integration.go

@@ -22,6 +22,7 @@ type DevConfig struct {
 	ProcessChannelReadyWait time.Duration `long:"processchannelreadywait" description:"Time to sleep before processing remote node's channel_ready message."`
 	ReservationTimeout      time.Duration `long:"reservationtimeout" description:"The maximum time we keep a pending channel open flow in memory."`
 	ZombieSweeperInterval   time.Duration `long:"zombiesweeperinterval" description:"The time interval at which channel opening flows are evaluated for zombie status."`
+	UnsafeDisconnect        bool          `long:"unsafedisconnect" description:"Allows the rpcserver to intentionally disconnect from peers with open channels."`
 }
 
 // ChannelReadyWait returns the config value `ProcessChannelReadyWait`.
@@ -46,3 +47,8 @@ func (d *DevConfig) GetZombieSweeperInterval() time.Duration {
 
 	return d.ZombieSweeperInterval
 }
+
+// ChannelReadyWait returns the config value `UnsafeDisconnect`.
+func (d *DevConfig) GetUnsafeDisconnect() bool {
+	return d.UnsafeDisconnect
+}

lncfg/neutrino.go

@@ -12,7 +12,7 @@ type Neutrino struct {
 	MaxPeers           int           `long:"maxpeers" description:"Max number of inbound and outbound peers"`
 	BanDuration        time.Duration `long:"banduration" description:"How long to ban misbehaving peers.  Valid time units are {s, m, h}.  Minimum 1 second"`
 	BanThreshold       uint32        `long:"banthreshold" description:"Maximum allowed ban score before disconnecting and banning misbehaving peers."`
-	FeeURL             string        `long:"feeurl" description:"DEPRECATED: Use top level 'feeurl' option. Optional URL for fee estimation. If a URL is not specified, static fees will be used for estimation."`
+	FeeURL             string        `long:"feeurl" description:"DEPRECATED: Use top level 'feeurl' option. Optional URL for fee estimation. If a URL is not specified, static fees will be used for estimation." hidden:"true"`
 	AssertFilterHeader string        `long:"assertfilterheader" description:"Optional filter header in height:hash format to assert the state of neutrino's filter header chain on startup. If the assertion does not hold, then the filter header chain will be re-synced from the genesis block."`
 	UserAgentName      string        `long:"useragentname" description:"Used to help identify ourselves to other bitcoin peers"`
 	UserAgentVersion   string        `long:"useragentversion" description:"Used to help identify ourselves to other bitcoin peers"`

lncfg/routing.go

@@ -4,7 +4,7 @@ package lncfg
 //
 //nolint:lll
 type Routing struct {
-	AssumeChannelValid bool `long:"assumechanvalid" description:"Skip checking channel spentness during graph validation. This speedup comes at the risk of using an unvalidated view of the network for routing. (default: false)"`
+	AssumeChannelValid bool `long:"assumechanvalid" description:"DEPRECATED: Skip checking channel spentness during graph validation. This speedup comes at the risk of using an unvalidated view of the network for routing. (default: false)" hidden:"true"`
 
 	StrictZombiePruning bool `long:"strictgraphpruning" description:"If true, then the graph will be pruned more aggressively for zombies. In practice this means that edges with a single stale edge will be considered a zombie."`
 }

lntest/node/config.go

@@ -206,7 +206,6 @@ func (cfg *BaseNodeConfig) GenArgs() []string {
 	args = append(args, backendArgs...)
 
 	nodeArgs := []string{
-		"--bitcoin.active",
 		"--nobootstrap",
 		"--debuglevel=debug,DISC=trace",
 		"--bitcoin.defaultchanconfs=1",
@@ -241,6 +240,9 @@ func (cfg *BaseNodeConfig) GenArgs() []string {
 		// Speed up the tests for bitcoind backend.
 		"--bitcoind.blockpollinginterval=100ms",
 		"--bitcoind.txpollinginterval=100ms",
+
+		// Allow unsafe disconnect in itest.
+		"--dev.unsafedisconnect",
 	}
 
 	args = append(args, nodeArgs...)

rpcserver.go

@@ -1747,15 +1747,27 @@ func (r *rpcServer) DisconnectPeer(ctx context.Context,
 	// In order to avoid erroneously disconnecting from a peer that we have
 	// an active channel with, if we have any channels active with this
 	// peer, then we'll disallow disconnecting from them.
-	if len(nodeChannels) > 0 && !r.cfg.UnsafeDisconnect {
-		return nil, fmt.Errorf("cannot disconnect from peer(%x), "+
-			"all active channels with the peer need to be closed "+
-			"first", pubKeyBytes)
+	if len(nodeChannels) != 0 {
+		// If we are not in a dev environment or the configed dev value
+		// `unsafedisconnect` is false, we return an error since there
+		// are active channels.
+		if !r.cfg.Dev.GetUnsafeDisconnect() {
+			return nil, fmt.Errorf("cannot disconnect from "+
+				"peer(%x), still has %d active channels",
+				pubKeyBytes, len(nodeChannels))
+		}
+
+		// We are in a dev environment, print a warning log and
+		// disconnect.
+		rpcsLog.Warnf("UnsafeDisconnect mode, disconnecting from "+
+			"peer(%x) while there are %d active channels",
+			pubKeyBytes, len(nodeChannels))
 	}
 
 	// With all initial validation complete, we'll now request that the
 	// server disconnects from the peer.
-	if err := r.server.DisconnectPeer(peerPubKey); err != nil {
+	err = r.server.DisconnectPeer(peerPubKey)
+	if err != nil {
 		return nil, fmt.Errorf("unable to disconnect peer: %v", err)
 	}
 
@@ -3054,7 +3066,7 @@ func (r *rpcServer) GetInfo(_ context.Context,
 func (r *rpcServer) GetDebugInfo(_ context.Context,
 	_ *lnrpc.GetDebugInfoRequest) (*lnrpc.GetDebugInfoResponse, error) {
 
-	flatConfig, err := configToFlatMap(*r.cfg)
+	flatConfig, _, err := configToFlatMap(*r.cfg)
 	if err != nil {
 		return nil, fmt.Errorf("error converting config to flat map: "+
 			"%w", err)