config: add Consensus subsection for dBFT config, fix #2677

Old UnlockWallet is still supported and works just fine.

ROADMAP.md

@@ -135,3 +135,13 @@ time-related settings to `Duration` format).
 
 Removal of deprecated P2P related application settings is scheduled for May-June
 2023 (~0.103.0 release).
+
+## Direct UnlockWallet consensus configuration
+
+Top-level UnlockWallet section in ApplicationConfiguration was used as an
+implicit consensus service configuration, now this setting (with Enabled flag)
+is moved into a section of its own (Consensus). Old configurations are still
+supported, but this support will eventually be removed.
+
+Removal of this compatibility code is scheduled for May-June 2023 (~0.103.0
+release).

cli/server/server.go

@@ -376,8 +376,8 @@ func mkOracle(config config.OracleConfiguration, magic netmode.Magic, chain *cor
 	return orc, nil
 }
 
-func mkConsensus(config config.Wallet, tpb time.Duration, chain *core.Blockchain, serv *network.Server, log *zap.Logger) (consensus.Service, error) {
-	if len(config.Path) == 0 {
+func mkConsensus(config config.Consensus, tpb time.Duration, chain *core.Blockchain, serv *network.Server, log *zap.Logger) (consensus.Service, error) {
+	if !config.Enabled {
 		return nil, nil
 	}
 	srv, err := consensus.NewService(consensus.Config{
@@ -387,7 +387,7 @@ func mkConsensus(config config.Wallet, tpb time.Duration, chain *core.Blockchain
 		ProtocolConfiguration: chain.GetConfig(),
 		RequestTx:             serv.RequestTx,
 		StopTxFlow:            serv.StopTxFlow,
-		Wallet:                &config,
+		Wallet:                config.UnlockWallet,
 		TimePerBlock:          tpb,
 	})
 	if err != nil {
@@ -476,7 +476,7 @@ func startServer(ctx *cli.Context) error {
 	if err != nil {
 		return cli.NewExitError(err, 1)
 	}
-	dbftSrv, err := mkConsensus(cfg.ApplicationConfiguration.UnlockWallet, serverConfig.TimePerBlock, chain, serv, log)
+	dbftSrv, err := mkConsensus(cfg.ApplicationConfiguration.Consensus, serverConfig.TimePerBlock, chain, serv, log)
 	if err != nil {
 		return cli.NewExitError(err, 1)
 	}
@@ -609,7 +609,7 @@ Main:
 					serv.DelConsensusService(dbftSrv)
 					dbftSrv.Shutdown()
 				}
-				dbftSrv, err = mkConsensus(cfgnew.ApplicationConfiguration.UnlockWallet, serverConfig.TimePerBlock, chain, serv, log)
+				dbftSrv, err = mkConsensus(cfgnew.ApplicationConfiguration.Consensus, serverConfig.TimePerBlock, chain, serv, log)
 				if err != nil {
 					log.Error("failed to create consensus service", zap.Error(err))
 					break // Whatever happens, I'll leave it all to chance.

docs/consensus.md

@@ -48,9 +48,11 @@ neo-go binary).
 Add the following subsection to `ApplicationConfiguration` section of your
 configuration file (`protocol.mainnet.yml` or `protocol.testnet.yml`):
 ```
-  UnlockWallet:
-    Path: "wallet.json"
-    Password: "welcometotherealworld"
+  Consensus:
+    Enabled: true
+    UnlockWallet:
+      Path: "wallet.json"
+      Password: "welcometotherealworld"
 ```
 where `wallet.json` is a path to your NEP-6 wallet and `welcometotherealworld`
 is a password to your CN key. Run the node in a regular way after that:
@@ -71,6 +73,15 @@ your expectations. Details on various configuration options are provided in the
 [node configuration documentation](node-configuration.md), CLI commands are
 provided in the [CLI documentation](cli.md).
 
+Consensus service can also run in watch-only mode when the node will
+receive/process/log dBFT messages generated by other nodes, but won't be able
+to generate any. It's mostly useful for debugging/monitoring. To enable this
+mode just drop the `UnlockWallet` section from the configuration like this:
+```
+  Consensus:
+    Enabled: true
+```
+
 ### Registration
 
 To register as a candidate, use neo-go as CLI command with an external RPC
@@ -174,8 +185,10 @@ place the corresponding config named `protocol.privnet.yml` there.
 
 2. Edit configuration file for every node.
 Examples can be found at `config/protocol.privnet.docker.one.yml` (`two`, `three` etc.).
-    1. Add `UnlockWallet` section with `Path` and `Password` strings for NEP-6
-       wallet path and the password for the account to be used for the consensus node.
+    1. Add `Consensus` section with `Enabled: true` field and an
+       `UnlockWallet` subsection with `Path` and `Password` strings for NEP-6
+       wallet path and the password for the account to be used for the
+       consensus node.
     2. Make sure that your `MinPeers` setting is equal to
        the number of nodes participating in consensus.
        This requirement is needed for nodes to correctly

docs/node-configuration.md

@@ -37,9 +37,10 @@ node-related settings described in the table below.
 | Prometheus | [Metrics Services Configuration](#Metrics-Services-Configuration) | | Configuration for Prometheus (monitoring system). See the [Metrics Services Configuration](#Metrics-Services-Configuration) section for details |
 | ProtoTickInterval | `int64` | `5` | Duration in seconds between protocol ticks with each connected peer. Warning: this field is deprecated and moved to `P2P` section. |
 | Relay | `bool` | `true` | Determines whether the server is forwarding its inventory. |
+| Consensus | [Consensus Configuration](#Consensus-Configuration) |  | Describes consensus (dBFT) configuration. See the [Consensus Configuration](#Consensus-Configuration) for details. |
 | RPC | [RPC Configuration](#RPC-Configuration) |  | Describes [RPC subsystem](rpc.md) configuration. See the [RPC Configuration](#RPC-Configuration) for details. |
 | StateRoot | [State Root Configuration](#State-Root-Configuration) |  | State root module configuration. See the [State Root Configuration](#State-Root-Configuration) section for details. |
-| UnlockWallet | [Unlock Wallet Configuration](#Unlock-Wallet-Configuration) |  | Node wallet configuration used for consensus (dBFT) operation. See the [Unlock Wallet Configuration](#Unlock-Wallet-Configuration) section for details. |
+| UnlockWallet | [Unlock Wallet Configuration](#Unlock-Wallet-Configuration) |  | Node wallet configuration used for consensus (dBFT) operation. See the [Unlock Wallet Configuration](#Unlock-Wallet-Configuration) section for details. This section is deprecated and replaced by Consensus, it only exists for compatibility with old configuration files, but will be removed in future node versions. |
 
 ### P2P Configuration
 
@@ -293,6 +294,26 @@ where:
   [Unlock Wallet Configuration](#Unlock-Wallet-Configuration) section for
   structure details.
 
+### Consensus Configuration
+
+`Consensus` configuration section describes configuration for dBFT node
+module and has the following structure:
+```
+Consensus:
+  Enabled: false
+  UnlockWallet:
+    Path: "/consensus_node_wallet.json"
+    Password: "pass"
+```
+where:
+- `Enabled` denotes whether dBFT module is active.
+- `UnlockWallet` is a consensus node wallet configuration, see the
+  [Unlock Wallet Configuration](#Unlock-Wallet-Configuration) section for
+  structure details.
+
+Please, refer to the [consensus node documentation](./consensus.md) for more
+details on consensus node setup.
+
 ### Unlock Wallet Configuration
 
 `UnlockWallet` configuration section contains wallet settings and has the following

internal/testcli/executor.go

@@ -158,7 +158,7 @@ func NewTestChain(t *testing.T, f func(*config.Config), run bool) (*core.Blockch
 		ProtocolConfiguration: chain.GetConfig(),
 		RequestTx:             netSrv.RequestTx,
 		StopTxFlow:            netSrv.StopTxFlow,
-		Wallet:                &cfg.ApplicationConfiguration.UnlockWallet,
+		Wallet:                cfg.ApplicationConfiguration.Consensus.UnlockWallet,
 		TimePerBlock:          serverConfig.TimePerBlock,
 	})
 	require.NoError(t, err)

pkg/config/application_config.go

@@ -43,6 +43,7 @@ type ApplicationConfiguration struct {
 	// Deprecated: this option is moved to the P2P section.
 	ProtoTickInterval int64               `yaml:"ProtoTickInterval"`
 	Relay             bool                `yaml:"Relay"`
+	Consensus         Consensus           `yaml:"Consensus"`
 	RPC               RPC                 `yaml:"RPC"`
 	UnlockWallet      Wallet              `yaml:"UnlockWallet"`
 	Oracle            OracleConfiguration `yaml:"Oracle"`

pkg/config/config.go

@@ -42,7 +42,8 @@ func Load(path string, netMode netmode.Magic) (Config, error) {
 	return LoadFile(configPath)
 }
 
-// LoadFile loads config from the provided path.
+// LoadFile loads config from the provided path. It also applies backwards compatibility
+// fixups if necessary.
 func LoadFile(configPath string) (Config, error) {
 	if _, err := os.Stat(configPath); os.IsNotExist(err) {
 		return Config{}, fmt.Errorf("config '%s' doesn't exist", configPath)
@@ -72,6 +73,11 @@ func LoadFile(configPath string) (Config, error) {
 		return Config{}, fmt.Errorf("failed to unmarshal config YAML: %w", err)
 	}
 
+	if len(config.ApplicationConfiguration.UnlockWallet.Path) > 0 && len(config.ApplicationConfiguration.Consensus.UnlockWallet.Path) == 0 {
+		config.ApplicationConfiguration.Consensus.UnlockWallet = config.ApplicationConfiguration.UnlockWallet
+		config.ApplicationConfiguration.Consensus.Enabled = true
+	}
+
 	err = config.ProtocolConfiguration.Validate()
 	if err != nil {
 		return Config{}, err

pkg/config/consensus.go

@@ -0,0 +1,4 @@
+package config
+
+// Consensus contains consensus service configuration.
+type Consensus InternalService

pkg/consensus/consensus.go

@@ -124,8 +124,9 @@ type Config struct {
 	StopTxFlow func()
 	// TimePerBlock is minimal time that should pass before the next block is accepted.
 	TimePerBlock time.Duration
-	// Wallet is a local-node wallet configuration.
-	Wallet *config.Wallet
+	// Wallet is a local-node wallet configuration. If the path is empty, then
+	// no wallet will be initialized and the service will be in watch-only mode.
+	Wallet config.Wallet
 }
 
 // NewService returns a new consensus.Service instance.
@@ -154,21 +155,23 @@ func NewService(cfg Config) (Service, error) {
 
 	var err error
 
-	if srv.wallet, err = wallet.NewWalletFromFile(cfg.Wallet.Path); err != nil {
-		return nil, err
-	}
-
-	// Check that the wallet password is correct for at least one account.
-	var ok bool
-	for _, acc := range srv.wallet.Accounts {
-		err := acc.Decrypt(srv.Config.Wallet.Password, srv.wallet.Scrypt)
-		if err == nil {
-			ok = true
-			break
+	if len(cfg.Wallet.Path) > 0 {
+		if srv.wallet, err = wallet.NewWalletFromFile(cfg.Wallet.Path); err != nil {
+			return nil, err
+		}
+
+		// Check that the wallet password is correct for at least one account.
+		var ok bool
+		for _, acc := range srv.wallet.Accounts {
+			err := acc.Decrypt(srv.Config.Wallet.Password, srv.wallet.Scrypt)
+			if err == nil {
+				ok = true
+				break
+			}
+		}
+		if !ok {
+			return nil, errors.New("no account with provided password was found")
 		}
-	}
-	if !ok {
-		return nil, errors.New("no account with provided password was found")
 	}
 
 	srv.dbft = dbft.New(
@@ -282,7 +285,9 @@ func (s *service) Shutdown() {
 		s.log.Info("stopping consensus service")
 		close(s.quit)
 		<-s.finished
-		s.wallet.Close()
+		if s.wallet != nil {
+			s.wallet.Close()
+		}
 	}
 }
 
@@ -377,24 +382,25 @@ func (s *service) validatePayload(p *Payload) bool {
 }
 
 func (s *service) getKeyPair(pubs []crypto.PublicKey) (int, crypto.PrivateKey, crypto.PublicKey) {
-	for i := range pubs {
-		sh := pubs[i].(*publicKey).GetScriptHash()
-		acc := s.wallet.GetAccount(sh)
-		if acc == nil {
-			continue
-		}
-
-		if !acc.CanSign() {
-			err := acc.Decrypt(s.Config.Wallet.Password, s.wallet.Scrypt)
-			if err != nil {
-				s.log.Fatal("can't unlock account", zap.String("address", address.Uint160ToString(sh)))
-				break
+	if s.wallet != nil {
+		for i := range pubs {
+			sh := pubs[i].(*publicKey).GetScriptHash()
+			acc := s.wallet.GetAccount(sh)
+			if acc == nil {
+				continue
 			}
+
+			if !acc.CanSign() {
+				err := acc.Decrypt(s.Config.Wallet.Password, s.wallet.Scrypt)
+				if err != nil {
+					s.log.Fatal("can't unlock account", zap.String("address", address.Uint160ToString(sh)))
+					break
+				}
+			}
+
+			return i, &privateKey{PrivateKey: acc.PrivateKey()}, &publicKey{PublicKey: acc.PublicKey()}
 		}
-
-		return i, &privateKey{PrivateKey: acc.PrivateKey()}, &publicKey{PublicKey: acc.PublicKey()}
 	}
-
 	return -1, nil, nil
 }
 

pkg/consensus/consensus_test.go

@@ -44,6 +44,24 @@ func TestNewService(t *testing.T) {
 	require.Equal(t, tx, txx[0])
 }
 
+func TestNewWatchingService(t *testing.T) {
+	bc := newTestChain(t, false)
+	srv, err := NewService(Config{
+		Logger:                zaptest.NewLogger(t),
+		Broadcast:             func(*npayload.Extensible) {},
+		Chain:                 bc,
+		ProtocolConfiguration: bc.GetConfig(),
+		RequestTx:             func(...util.Uint256) {},
+		StopTxFlow:            func() {},
+		TimePerBlock:          bc.GetConfig().TimePerBlock,
+		// No wallet provided.
+	})
+	require.NoError(t, err)
+
+	require.NotPanics(t, srv.Start)
+	require.NotPanics(t, srv.Shutdown)
+}
+
 func initServiceNextConsensus(t *testing.T, newAcc *wallet.Account, offset uint32) (*service, *wallet.Account) {
 	acc, err := wallet.NewAccountFromWIF(testchain.WIF(testchain.IDToOrder(0)))
 	require.NoError(t, err)
@@ -481,7 +499,7 @@ func newTestServiceWithChain(t *testing.T, bc *core.Blockchain) *service {
 		RequestTx:             func(...util.Uint256) {},
 		StopTxFlow:            func() {},
 		TimePerBlock:          bc.GetConfig().TimePerBlock,
-		Wallet: &config.Wallet{
+		Wallet: config.Wallet{
 			Path:     "./testdata/wallet1.json",
 			Password: "one",
 		},