From 3930c67a44220f3d420c129f057721e039813216 Mon Sep 17 00:00:00 2001
From: Leonard Lyubich <leonard@morphbits.io>
Date: Fri, 24 May 2024 03:46:18 +0400
Subject: [PATCH] ir/config: Fix unenforced `morph.consensus.p2p.peers.min`
 config default
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As declared in NeoFS IR configuration, min required number of peer
connections must default to ⌈2/3N-1⌉. Previously, IR app did not set
default value when `p2p` and/or `p2p.peers` sections were omitted, only
when `p2p.peers` is presented and `min` value is missing. This was
incorrect and also unresponsive to the admin: a forgotten value
setting had the effect of no consensus and no new blocks.

This fixes the behavior: now value is recalculated in any setting
without a field. Note that while explicitly specifying zero is
problematic in general, it does not default to preserve admin intent.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
---
 CHANGELOG.md                 |  1 +
 pkg/innerring/config.go      | 14 ++++++---
 pkg/innerring/config_test.go | 61 ++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 48ccc05469..ed5f5368d9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ Changelog for NeoFS Node
 ### Added
 
 ### Fixed
+- Unenforced IR `morph.consensus.p2p.peers.min` config default (#2856)
 
 ### Changed
 
diff --git a/pkg/innerring/config.go b/pkg/innerring/config.go
index c58f1517a8..d0e8ba18f7 100644
--- a/pkg/innerring/config.go
+++ b/pkg/innerring/config.go
@@ -181,6 +181,7 @@ func parseBlockchainConfig(v *viper.Viper, _logger *zap.Logger) (c blockchain.Co
 		}
 	}
 
+	minPeersConfigured := false
 	const p2pSection = cfgPathFSChainLocalConsensus + ".p2p"
 	if v.IsSet(p2pSection) {
 		c.P2P.DialTimeout, err = parseConfigDurationPositive(v, p2pSection+".dial_timeout", "P2P dial timeout")
@@ -202,11 +203,12 @@ func parseBlockchainConfig(v *viper.Viper, _logger *zap.Logger) (c blockchain.Co
 				if !errors.Is(err, errMissingConfig) {
 					return c, err
 				}
-				// we calculate default here since explicit 0 is also a valid setting
-				n := uint64(len(c.Committee))
-				minPeers = n - (n-1)/3 - 1
+				// defaults below
+			} else {
+				c.P2P.MinPeers = uint(minPeers)
+				// zero can be set explicitly, so prevent overriding it
+				minPeersConfigured = true
 			}
-			c.P2P.MinPeers = uint(minPeers)
 			maxPeers, err := parseConfigUint64Range(v, p2pPeersSection+".max", "maximum number of P2P peers", 1, math.MaxInt32)
 			if err != nil && !errors.Is(err, errMissingConfig) {
 				return c, err
@@ -230,6 +232,10 @@ func parseBlockchainConfig(v *viper.Viper, _logger *zap.Logger) (c blockchain.Co
 			}
 		}
 	}
+	if !minPeersConfigured {
+		n := uint(len(c.Committee))
+		c.P2P.MinPeers = n - (n-1)/3 - 1
+	}
 
 	c.SetRolesInGenesis, err = parseConfigBool(v, cfgPathFSChainLocalConsensus+".set_roles_in_genesis", "flag to set roles for the committee in the genesis block")
 	if err != nil && !errors.Is(err, errMissingConfig) {
diff --git a/pkg/innerring/config_test.go b/pkg/innerring/config_test.go
index 926839aae8..a483ff5e41 100644
--- a/pkg/innerring/config_test.go
+++ b/pkg/innerring/config_test.go
@@ -121,6 +121,16 @@ func resetConfig(tb testing.TB, v *viper.Viper, key string) {
 	}
 }
 
+func commiteeN(t testing.TB, n int) []string {
+	res := make([]string, n)
+	for i := range res {
+		k, err := keys.NewPrivateKey()
+		require.NoError(t, err)
+		res[i] = k.PublicKey().StringCompressed()
+	}
+	return res
+}
+
 func TestParseBlockchainConfig(t *testing.T) {
 	fullConfig := true
 	_logger := zap.NewNop()
@@ -143,6 +153,7 @@ func TestParseBlockchainConfig(t *testing.T) {
 			NetworkMagic: 15405,
 			Committee:    validCommittee,
 			Storage:      blockchain.BoltDB("chain.db"),
+			P2P:          blockchain.P2PConfig{MinPeers: 2},
 		}, c)
 	})
 
@@ -570,3 +581,53 @@ fschain_autodeploy: true
 		require.False(t, b)
 	})
 }
+
+func TestP2PMinPeers(t *testing.T) {
+	l := zap.NewNop()
+	assert := func(t testing.TB, v *viper.Viper, exp uint) {
+		c, err := parseBlockchainConfig(v, l)
+		require.NoError(t, err)
+		require.EqualValues(t, exp, c.P2P.MinPeers)
+	}
+
+	v := newValidBlockchainConfig(t, true)
+	v.Set("morph.consensus.p2p.peers.min", 123)
+	assert(t, v, 123)
+
+	t.Run("explicit zero", func(t *testing.T) {
+		v := newValidBlockchainConfig(t, false)
+		v.Set("morph.consensus.p2p.peers.min", 0)
+		assert(t, v, 0)
+	})
+	t.Run("default", func(t *testing.T) {
+		assertDefault := func(t testing.TB, v *viper.Viper) {
+			setCommitteeN := func(n int) {
+				v.Set("morph.consensus.committee", commiteeN(t, n))
+				resetConfig(t, v, "morph.consensus.validators_history") // checked against committee size
+			}
+			setCommitteeN(4)
+			assert(t, v, 2)
+			setCommitteeN(7)
+			assert(t, v, 4)
+			setCommitteeN(21)
+			assert(t, v, 14)
+		}
+		t.Run("missing P2P section", func(t *testing.T) {
+			v := newValidBlockchainConfig(t, true)
+			resetConfig(t, v, "morph.consensus.p2p")
+			assertDefault(t, v)
+		})
+		t.Run("missing peers section", func(t *testing.T) {
+			v := newValidBlockchainConfig(t, true)
+			resetConfig(t, v, "morph.consensus.p2p.peers")
+			require.True(t, v.IsSet("morph.consensus.p2p"))
+			assertDefault(t, v)
+		})
+		t.Run("missing config itself", func(t *testing.T) {
+			v := newValidBlockchainConfig(t, true)
+			resetConfig(t, v, "morph.consensus.p2p.peers.min")
+			require.True(t, v.IsSet("morph.consensus.p2p.peers"))
+			assertDefault(t, v)
+		})
+	})
+}