From 157c84cc4de28f02341c09ae2a754d8b197b6994 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 31 Jan 2024 14:58:10 +0800 Subject: [PATCH 1/4] multi: log warnings if deprecated config options are used This commit adds warning logs if the user uses deprecated config options, paving the way for future removal. --- config.go | 41 ++++++++++++++++++++++++++++++++++++----- config_test.go | 8 +++++++- lncfg/chain.go | 2 +- rpcserver.go | 2 +- 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/config.go b/config.go index e89f37baeb..d510b7cf56 100644 --- a/config.go +++ b/config.go @@ -813,6 +813,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 +2115,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 +2170,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 +2185,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 +2220,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) + } } diff --git a/config_test.go b/config_test.go index 1f1740a0db..0c82db28d4 100644 --- a/config_test.go +++ b/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( diff --git a/lncfg/chain.go b/lncfg/chain.go index 6452363363..e84cadb7b8 100644 --- a/lncfg/chain.go +++ b/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"` diff --git a/rpcserver.go b/rpcserver.go index acce910df2..25140c4d04 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -3054,7 +3054,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) From 78d552de22531259ebf499d834438c44fbf8b3fd Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 31 Jan 2024 15:05:50 +0800 Subject: [PATCH 2/4] config+rpcserver: hide deprecated option `unsafe-disconnect` Also fix a case where this deprecated flag is used. We will always bypass the active channels check when `DisconnectPeer` because `!r.cfg.UnsafeDisconnect` is always false. --- config.go | 3 +-- rpcserver.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/config.go b/config.go index d510b7cf56..ec6220b9b4 100644 --- a/config.go +++ b/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, diff --git a/rpcserver.go b/rpcserver.go index 25140c4d04..95bdd39bf3 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1747,7 +1747,7 @@ 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 { + if len(nodeChannels) > 0 { return nil, fmt.Errorf("cannot disconnect from peer(%x), "+ "all active channels with the peer need to be closed "+ "first", pubKeyBytes) From 9adec89fa8c15811525d23add62e0c4c214e31ac Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 31 Jan 2024 15:14:31 +0800 Subject: [PATCH 3/4] lncfg: hide deprecated `routing.assumechanvalid` and `neutrino.feeurl` --- docs/release-notes/release-notes-0.18.0.md | 3 +++ lncfg/neutrino.go | 2 +- lncfg/routing.go | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/release-notes/release-notes-0.18.0.md b/docs/release-notes/release-notes-0.18.0.md index f95d4ec9e3..5942520a3a 100644 --- a/docs/release-notes/release-notes-0.18.0.md +++ b/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) diff --git a/lncfg/neutrino.go b/lncfg/neutrino.go index 638c8457df..09a66312aa 100644 --- a/lncfg/neutrino.go +++ b/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"` diff --git a/lncfg/routing.go b/lncfg/routing.go index 47d321fd97..16620329e9 100644 --- a/lncfg/routing.go +++ b/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."` } From 1057eb729de883882d3d0ef57ecace51f5881ca3 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 31 Jan 2024 17:13:38 +0800 Subject: [PATCH 4/4] multi: add `unsafedisconnect` as a dev config This commit adds a new dev config `unsafedisconnect` as we sometimes want to disconnect nodes in our itests. --- lncfg/dev.go | 11 +++++++++++ lncfg/dev_integration.go | 6 ++++++ lntest/node/config.go | 4 +++- rpcserver.go | 22 +++++++++++++++++----- 4 files changed, 37 insertions(+), 6 deletions(-) diff --git a/lncfg/dev.go b/lncfg/dev.go index 0bc305cff0..7eb2958242 100644 --- a/lncfg/dev.go +++ b/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 diff --git a/lncfg/dev_integration.go b/lncfg/dev_integration.go index 526c86b6aa..86a4af275e 100644 --- a/lncfg/dev_integration.go +++ b/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 +} diff --git a/lntest/node/config.go b/lntest/node/config.go index 855325ebc9..449ad2fadc 100644 --- a/lntest/node/config.go +++ b/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...) diff --git a/rpcserver.go b/rpcserver.go index 95bdd39bf3..370dfd9920 100644 --- a/rpcserver.go +++ b/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 { - 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) }