From 1e4dc3f3486ec60557284602d60651ceef00c9e7 Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Tue, 4 Jun 2024 16:33:30 +0100 Subject: [PATCH 1/9] feat: do not connect to peers that have a different major version --- cmd/thor/utils.go | 2 +- comm/comm_test.go | 6 ---- comm/communicator.go | 33 +++++++++++++++++++++- comm/communicator_test.go | 59 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 8 deletions(-) delete mode 100644 comm/comm_test.go create mode 100644 comm/communicator_test.go diff --git a/cmd/thor/utils.go b/cmd/thor/utils.go index 89ac0212e..ca7c93ebe 100644 --- a/cmd/thor/utils.go +++ b/cmd/thor/utils.go @@ -461,7 +461,7 @@ func newP2PCommunicator(ctx *cli.Context, repo *chain.Repository, txPool *txpool } return p2p.New( - comm.New(repo, txPool), + comm.New(repo, txPool, version), key, instanceDir, userNAT, diff --git a/comm/comm_test.go b/comm/comm_test.go deleted file mode 100644 index a5a8d2471..000000000 --- a/comm/comm_test.go +++ /dev/null @@ -1,6 +0,0 @@ -// Copyright (c) 2018 The VeChainThor developers - -// Distributed under the GNU Lesser General Public License v3.0 software license, see the accompanying -// file LICENSE or - -package comm_test diff --git a/comm/communicator.go b/comm/communicator.go index 574d53278..9bc37b3f9 100644 --- a/comm/communicator.go +++ b/comm/communicator.go @@ -9,7 +9,9 @@ import ( "context" "fmt" "math" + "regexp" "sort" + "strings" "sync" "time" @@ -41,10 +43,11 @@ type Communicator struct { feedScope event.SubscriptionScope goes co.Goes onceSynced sync.Once + version string } // New create a new Communicator instance. -func New(repo *chain.Repository, txPool *txpool.TxPool) *Communicator { +func New(repo *chain.Repository, txPool *txpool.TxPool, version string) *Communicator { ctx, cancel := context.WithCancel(context.Background()) return &Communicator{ repo: repo, @@ -54,6 +57,7 @@ func New(repo *chain.Repository, txPool *txpool.TxPool) *Communicator { peerSet: newPeerSet(), syncedCh: make(chan struct{}), announcementCh: make(chan *announcement), + version: version, } } @@ -161,6 +165,11 @@ type txsToSync struct { } func (c *Communicator) servePeer(p *p2p.Peer, rw p2p.MsgReadWriter) error { + if !sameMajor(c.version, p.Name()) { + log.Warn("peer version mismatch", "name", p.Name(), "address", p.RemoteAddr().String(), "id", p.ID().String()) + return nil + } + peer := newPeer(p, rw) c.goes.Go(func() { c.runPeer(peer) @@ -283,3 +292,25 @@ func (c *Communicator) PeersStats() []*PeerStats { }) return stats } + +// SameMajor returns true if the peer has the same major version +func sameMajor(appVersion, peerName string) bool { + versionRegex := regexp.MustCompile(`\d+\.\d+\.\d+`) + // Got a bad app version, so accept any peer + if appVersion == "" || !versionRegex.MatchString(appVersion) { + log.Info("bad app version", "appVersion", appVersion) + return true + } + + // Extract the semantic version from the name + peerVersion := versionRegex.FindString(peerName) + if peerVersion == "" { + return false + } + + peerMajorVersion := strings.Split(peerVersion, ".")[0] + givenMajorVersion := strings.Split(appVersion, ".")[0] + + // Compare the major versions + return peerMajorVersion == givenMajorVersion +} diff --git a/comm/communicator_test.go b/comm/communicator_test.go new file mode 100644 index 000000000..d26fb2e2d --- /dev/null +++ b/comm/communicator_test.go @@ -0,0 +1,59 @@ +// Copyright (c) 2024 The VeChainThor developers + +// Distributed under the GNU Lesser General Public License v3.0 software license, see the accompanying +// file LICENSE or +package comm + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSameMajor(t *testing.T) { + testCases := []struct { + id string + appVersion string + peerName string + expected bool + }{ + { + id: "only-major-same", + appVersion: "2.0.0", + peerName: "thor/v2.1.1-88c7c86-release/linux/go1.21.9", + expected: true, + }, + { + id: "only-major-different", + appVersion: "2.1.1", + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: false, + }, + { + id: "app-version-empty", + appVersion: "", + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: true, + }, + { + id: "exact-match", + appVersion: "1.1.1", + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: true, + }, + { + id: "bad-app-version", + // bad app version, so accept any peer + appVersion: "bad.bad.bad", + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.id, func(t *testing.T) { + result := sameMajor(tc.appVersion, tc.peerName) + assert.Equal(t, tc.expected, result) + }) + } +} From 351cba2ff65b0626419ef2cd6dcde17cdc4f086b Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Tue, 4 Jun 2024 16:34:50 +0100 Subject: [PATCH 2/9] fix: bad comment --- comm/communicator.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/comm/communicator.go b/comm/communicator.go index 9bc37b3f9..0289e82e5 100644 --- a/comm/communicator.go +++ b/comm/communicator.go @@ -296,9 +296,8 @@ func (c *Communicator) PeersStats() []*PeerStats { // SameMajor returns true if the peer has the same major version func sameMajor(appVersion, peerName string) bool { versionRegex := regexp.MustCompile(`\d+\.\d+\.\d+`) - // Got a bad app version, so accept any peer if appVersion == "" || !versionRegex.MatchString(appVersion) { - log.Info("bad app version", "appVersion", appVersion) + // Got a bad app version, so accept any peer return true } From de35fa36bc59d13484bb21a333484e952592e028 Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Tue, 4 Jun 2024 16:38:26 +0100 Subject: [PATCH 3/9] fix: bad comment --- comm/communicator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/comm/communicator.go b/comm/communicator.go index 0289e82e5..a823c46d4 100644 --- a/comm/communicator.go +++ b/comm/communicator.go @@ -293,7 +293,7 @@ func (c *Communicator) PeersStats() []*PeerStats { return stats } -// SameMajor returns true if the peer has the same major version +// sameMajor returns true if the peer has the same major version func sameMajor(appVersion, peerName string) bool { versionRegex := regexp.MustCompile(`\d+\.\d+\.\d+`) if appVersion == "" || !versionRegex.MatchString(appVersion) { From b436b841bff1c0c11a5f0175c931285ac6db1263 Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Tue, 4 Jun 2024 16:47:59 +0100 Subject: [PATCH 4/9] fix: unit test --- api/node/node_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/node/node_test.go b/api/node/node_test.go index 90857d0d0..0cb904c9d 100644 --- a/api/node/node_test.go +++ b/api/node/node_test.go @@ -49,7 +49,7 @@ func initCommServer(t *testing.T) { Limit: 10000, LimitPerAccount: 16, MaxLifetime: 10 * time.Minute, - })) + }), "2.0.0") router := mux.NewRouter() node.New(comm).Mount(router, "/node") ts = httptest.NewServer(router) From 0e8ad07fd2635e40f74d3c93c883b29b8235b1c6 Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Tue, 4 Jun 2024 17:19:51 +0100 Subject: [PATCH 5/9] refactor: disconnect in p2psrv --- api/node/node_test.go | 2 +- cmd/thor/main.go | 2 +- cmd/thor/p2p/p2p.go | 2 +- cmd/thor/utils.go | 2 +- comm/communicator.go | 32 +-------------------- comm/communicator_test.go | 59 --------------------------------------- p2psrv/server.go | 42 ++++++++++++++++++++++++++-- p2psrv/server_test.go | 54 +++++++++++++++++++++++++++++++++-- 8 files changed, 97 insertions(+), 98 deletions(-) delete mode 100644 comm/communicator_test.go diff --git a/api/node/node_test.go b/api/node/node_test.go index 0cb904c9d..90857d0d0 100644 --- a/api/node/node_test.go +++ b/api/node/node_test.go @@ -49,7 +49,7 @@ func initCommServer(t *testing.T) { Limit: 10000, LimitPerAccount: 16, MaxLifetime: 10 * time.Minute, - }), "2.0.0") + })) router := mux.NewRouter() node.New(comm).Mount(router, "/node") ts = httptest.NewServer(router) diff --git a/cmd/thor/main.go b/cmd/thor/main.go index 00cb44977..4d4b18b00 100644 --- a/cmd/thor/main.go +++ b/cmd/thor/main.go @@ -39,7 +39,7 @@ import ( ) var ( - version string + version string = "2.1.1" gitCommit string gitTag string copyrightYear string diff --git a/cmd/thor/p2p/p2p.go b/cmd/thor/p2p/p2p.go index 86f067b39..e41802253 100644 --- a/cmd/thor/p2p/p2p.go +++ b/cmd/thor/p2p/p2p.go @@ -78,7 +78,7 @@ func New( return &P2P{ comm: communicator, - p2pSrv: p2psrv.New(opts), + p2pSrv: p2psrv.New(opts, version), peersCachePath: peersCachePath, enode: fmt.Sprintf("enode://%x@[extip]:%v", discover.PubkeyID(&privateKey.PublicKey).Bytes(), listenPort), } diff --git a/cmd/thor/utils.go b/cmd/thor/utils.go index ca7c93ebe..89ac0212e 100644 --- a/cmd/thor/utils.go +++ b/cmd/thor/utils.go @@ -461,7 +461,7 @@ func newP2PCommunicator(ctx *cli.Context, repo *chain.Repository, txPool *txpool } return p2p.New( - comm.New(repo, txPool, version), + comm.New(repo, txPool), key, instanceDir, userNAT, diff --git a/comm/communicator.go b/comm/communicator.go index a823c46d4..574d53278 100644 --- a/comm/communicator.go +++ b/comm/communicator.go @@ -9,9 +9,7 @@ import ( "context" "fmt" "math" - "regexp" "sort" - "strings" "sync" "time" @@ -43,11 +41,10 @@ type Communicator struct { feedScope event.SubscriptionScope goes co.Goes onceSynced sync.Once - version string } // New create a new Communicator instance. -func New(repo *chain.Repository, txPool *txpool.TxPool, version string) *Communicator { +func New(repo *chain.Repository, txPool *txpool.TxPool) *Communicator { ctx, cancel := context.WithCancel(context.Background()) return &Communicator{ repo: repo, @@ -57,7 +54,6 @@ func New(repo *chain.Repository, txPool *txpool.TxPool, version string) *Communi peerSet: newPeerSet(), syncedCh: make(chan struct{}), announcementCh: make(chan *announcement), - version: version, } } @@ -165,11 +161,6 @@ type txsToSync struct { } func (c *Communicator) servePeer(p *p2p.Peer, rw p2p.MsgReadWriter) error { - if !sameMajor(c.version, p.Name()) { - log.Warn("peer version mismatch", "name", p.Name(), "address", p.RemoteAddr().String(), "id", p.ID().String()) - return nil - } - peer := newPeer(p, rw) c.goes.Go(func() { c.runPeer(peer) @@ -292,24 +283,3 @@ func (c *Communicator) PeersStats() []*PeerStats { }) return stats } - -// sameMajor returns true if the peer has the same major version -func sameMajor(appVersion, peerName string) bool { - versionRegex := regexp.MustCompile(`\d+\.\d+\.\d+`) - if appVersion == "" || !versionRegex.MatchString(appVersion) { - // Got a bad app version, so accept any peer - return true - } - - // Extract the semantic version from the name - peerVersion := versionRegex.FindString(peerName) - if peerVersion == "" { - return false - } - - peerMajorVersion := strings.Split(peerVersion, ".")[0] - givenMajorVersion := strings.Split(appVersion, ".")[0] - - // Compare the major versions - return peerMajorVersion == givenMajorVersion -} diff --git a/comm/communicator_test.go b/comm/communicator_test.go deleted file mode 100644 index d26fb2e2d..000000000 --- a/comm/communicator_test.go +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright (c) 2024 The VeChainThor developers - -// Distributed under the GNU Lesser General Public License v3.0 software license, see the accompanying -// file LICENSE or -package comm - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestSameMajor(t *testing.T) { - testCases := []struct { - id string - appVersion string - peerName string - expected bool - }{ - { - id: "only-major-same", - appVersion: "2.0.0", - peerName: "thor/v2.1.1-88c7c86-release/linux/go1.21.9", - expected: true, - }, - { - id: "only-major-different", - appVersion: "2.1.1", - peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", - expected: false, - }, - { - id: "app-version-empty", - appVersion: "", - peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", - expected: true, - }, - { - id: "exact-match", - appVersion: "1.1.1", - peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", - expected: true, - }, - { - id: "bad-app-version", - // bad app version, so accept any peer - appVersion: "bad.bad.bad", - peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", - expected: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.id, func(t *testing.T) { - result := sameMajor(tc.appVersion, tc.peerName) - assert.Equal(t, tc.expected, result) - }) - } -} diff --git a/p2psrv/server.go b/p2psrv/server.go index ff64e2ed8..e18243d20 100644 --- a/p2psrv/server.go +++ b/p2psrv/server.go @@ -10,6 +10,8 @@ import ( "errors" "math" "net" + "regexp" + "strings" "time" "github.com/ethereum/go-ethereum/common/mclock" @@ -36,10 +38,11 @@ type Server struct { knownNodes *cache.PrioCache discoveredNodes *cache.RandCache dialingNodes *nodeMap + version string } // New create a p2p server. -func New(opts *Options) *Server { +func New(opts *Options, version string) *Server { knownNodes := cache.NewPrioCache(5) discoveredNodes := cache.NewRandCache(128) for _, node := range opts.KnownNodes { @@ -67,6 +70,7 @@ func New(opts *Options) *Server { knownNodes: knownNodes, discoveredNodes: discoveredNodes, dialingNodes: newNodeMap(), + version: version, } } @@ -86,7 +90,20 @@ func (s *Server) Start(protocols []*p2p.Protocol, topic discv5.Topic) error { if peer.Inbound() { dir = "inbound" } - log := log.New("peer", peer, "dir", dir) + + if !sameMajor(s.version, peer.Name()) { + log.Warn("peer version mismatch", + "name", peer.Name(), + "address", peer.RemoteAddr().String(), + "id", peer.ID().String()) + + peer.Disconnect(p2p.DiscIncompatibleVersion) + s.knownNodes.Remove(peer.ID()) + s.dialingNodes.Remove(peer.ID()) + return nil + } + + log := log.New("peer", peer, "dir", dir, "name", peer.Name()) log.Debug("peer connected") metricConnectedPeers().Add(1) @@ -374,3 +391,24 @@ func (s *Server) fetchBootstrap() { func (s *Server) Options() *Options { return s.opts } + +// sameMajor returns true if the peer has the same major version +func sameMajor(appVersion, peerName string) bool { + versionRegex := regexp.MustCompile(`\d+\.\d+\.\d+`) + if appVersion == "" || !versionRegex.MatchString(appVersion) { + // Got a bad app version, so accept any peer + return true + } + + // Extract the semantic version from the name + peerVersion := versionRegex.FindString(peerName) + if peerVersion == "" { + return false + } + + peerMajorVersion := strings.Split(peerVersion, ".")[0] + givenMajorVersion := strings.Split(appVersion, ".")[0] + + // Compare the major versions + return peerMajorVersion == givenMajorVersion +} diff --git a/p2psrv/server_test.go b/p2psrv/server_test.go index 5cdc45d12..1708376ab 100644 --- a/p2psrv/server_test.go +++ b/p2psrv/server_test.go @@ -13,6 +13,8 @@ import ( "github.com/stretchr/testify/assert" ) +const version = "2.1.1-88c7c86-release" + func TestNewServer(t *testing.T) { privateKey, err := crypto.GenerateKey() if err != nil { @@ -31,7 +33,7 @@ func TestNewServer(t *testing.T) { KnownNodes: Nodes{node}, } - server := New(opts) + server := New(opts, version) assert.Equal(t, "testNode", server.opts.Name) assert.Equal(t, privateKey, server.opts.PrivateKey) @@ -62,7 +64,7 @@ func TestNewServerConnectOnly(t *testing.T) { KnownNodes: Nodes{knownNode}, } - server := New(opts) + server := New(opts, version) assert.Equal(t, "testNode", server.opts.Name) assert.Equal(t, privateKey, server.opts.PrivateKey) @@ -75,3 +77,51 @@ func TestNewServerConnectOnly(t *testing.T) { assert.True(t, server.discoveredNodes.Contains(knownNode.ID)) assert.True(t, server.knownNodes.Contains(knownNode.ID)) } + +func TestSameMajor(t *testing.T) { + testCases := []struct { + id string + appVersion string + peerName string + expected bool + }{ + { + id: "only-major-same", + appVersion: "2.0.0-88c7c86-release", + peerName: "thor/v2.1.1-88c7c86-release/linux/go1.21.9", + expected: true, + }, + { + id: "only-major-different", + appVersion: "2.1.1-88c7c86-release", + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: false, + }, + { + id: "app-version-empty", + appVersion: "", + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: true, + }, + { + id: "exact-match", + appVersion: "1.1.1-88c7c86-release", + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: true, + }, + { + id: "bad-app-version", + // bad app version, so accept any peer + appVersion: "bad.bad.bad-88c7c86-release", + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.id, func(t *testing.T) { + result := sameMajor(tc.appVersion, tc.peerName) + assert.Equal(t, tc.expected, result) + }) + } +} From bd5313418eebdf338aafc1c3e632fc1a3f39c045 Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Tue, 4 Jun 2024 17:20:32 +0100 Subject: [PATCH 6/9] fix: revert main --- cmd/thor/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/thor/main.go b/cmd/thor/main.go index 4d4b18b00..00cb44977 100644 --- a/cmd/thor/main.go +++ b/cmd/thor/main.go @@ -39,7 +39,7 @@ import ( ) var ( - version string = "2.1.1" + version string gitCommit string gitTag string copyrightYear string From 25c7a8b090eae1cf74841d70e1b0121688856374 Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Tue, 4 Jun 2024 17:40:59 +0100 Subject: [PATCH 7/9] declare regex as global var --- p2psrv/server.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/p2psrv/server.go b/p2psrv/server.go index e18243d20..ea9b2b13e 100644 --- a/p2psrv/server.go +++ b/p2psrv/server.go @@ -392,9 +392,10 @@ func (s *Server) Options() *Options { return s.opts } +var versionRegex = regexp.MustCompile(`\d+\.\d+\.\d+`) + // sameMajor returns true if the peer has the same major version func sameMajor(appVersion, peerName string) bool { - versionRegex := regexp.MustCompile(`\d+\.\d+\.\d+`) if appVersion == "" || !versionRegex.MatchString(appVersion) { // Got a bad app version, so accept any peer return true From 4ce5e4832f477c3e96c17e5c1dd7428398129785 Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Wed, 5 Jun 2024 08:29:37 +0100 Subject: [PATCH 8/9] fix: explicity define v1 nodes --- cmd/thor/p2p/p2p.go | 2 +- p2psrv/server.go | 26 ++++++++--------------- p2psrv/server_test.go | 49 ++++++++++++------------------------------- 3 files changed, 23 insertions(+), 54 deletions(-) diff --git a/cmd/thor/p2p/p2p.go b/cmd/thor/p2p/p2p.go index e41802253..86f067b39 100644 --- a/cmd/thor/p2p/p2p.go +++ b/cmd/thor/p2p/p2p.go @@ -78,7 +78,7 @@ func New( return &P2P{ comm: communicator, - p2pSrv: p2psrv.New(opts, version), + p2pSrv: p2psrv.New(opts), peersCachePath: peersCachePath, enode: fmt.Sprintf("enode://%x@[extip]:%v", discover.PubkeyID(&privateKey.PublicKey).Bytes(), listenPort), } diff --git a/p2psrv/server.go b/p2psrv/server.go index ea9b2b13e..040bbb3c3 100644 --- a/p2psrv/server.go +++ b/p2psrv/server.go @@ -38,11 +38,10 @@ type Server struct { knownNodes *cache.PrioCache discoveredNodes *cache.RandCache dialingNodes *nodeMap - version string } // New create a p2p server. -func New(opts *Options, version string) *Server { +func New(opts *Options) *Server { knownNodes := cache.NewPrioCache(5) discoveredNodes := cache.NewRandCache(128) for _, node := range opts.KnownNodes { @@ -70,7 +69,6 @@ func New(opts *Options, version string) *Server { knownNodes: knownNodes, discoveredNodes: discoveredNodes, dialingNodes: newNodeMap(), - version: version, } } @@ -91,8 +89,8 @@ func (s *Server) Start(protocols []*p2p.Protocol, topic discv5.Topic) error { dir = "inbound" } - if !sameMajor(s.version, peer.Name()) { - log.Warn("peer version mismatch", + if isIncompatible(peer.Name()) { + log.Warn("incompatible peer version", "name", peer.Name(), "address", peer.RemoteAddr().String(), "id", peer.ID().String()) @@ -392,24 +390,18 @@ func (s *Server) Options() *Options { return s.opts } -var versionRegex = regexp.MustCompile(`\d+\.\d+\.\d+`) - -// sameMajor returns true if the peer has the same major version -func sameMajor(appVersion, peerName string) bool { - if appVersion == "" || !versionRegex.MatchString(appVersion) { - // Got a bad app version, so accept any peer - return true - } +var versionRegex = regexp.MustCompile(`v\d+\.\d+\.\d+`) +// isIncompatible checks if the peer is running an incompatible version. +func isIncompatible(peerName string) bool { // Extract the semantic version from the name peerVersion := versionRegex.FindString(peerName) if peerVersion == "" { return false } - peerMajorVersion := strings.Split(peerVersion, ".")[0] - givenMajorVersion := strings.Split(appVersion, ".")[0] + major := strings.Split(peerVersion, ".")[0] - // Compare the major versions - return peerMajorVersion == givenMajorVersion + // Only v1 nodes are incompatible for now + return major == "v1" } diff --git a/p2psrv/server_test.go b/p2psrv/server_test.go index 1708376ab..bd87e2798 100644 --- a/p2psrv/server_test.go +++ b/p2psrv/server_test.go @@ -13,8 +13,6 @@ import ( "github.com/stretchr/testify/assert" ) -const version = "2.1.1-88c7c86-release" - func TestNewServer(t *testing.T) { privateKey, err := crypto.GenerateKey() if err != nil { @@ -33,7 +31,7 @@ func TestNewServer(t *testing.T) { KnownNodes: Nodes{node}, } - server := New(opts, version) + server := New(opts) assert.Equal(t, "testNode", server.opts.Name) assert.Equal(t, privateKey, server.opts.PrivateKey) @@ -64,7 +62,7 @@ func TestNewServerConnectOnly(t *testing.T) { KnownNodes: Nodes{knownNode}, } - server := New(opts, version) + server := New(opts) assert.Equal(t, "testNode", server.opts.Name) assert.Equal(t, privateKey, server.opts.PrivateKey) @@ -78,49 +76,28 @@ func TestNewServerConnectOnly(t *testing.T) { assert.True(t, server.knownNodes.Contains(knownNode.ID)) } -func TestSameMajor(t *testing.T) { +func TestIsIncompatible(t *testing.T) { testCases := []struct { - id string - appVersion string - peerName string - expected bool + peerName string + expected bool }{ { - id: "only-major-same", - appVersion: "2.0.0-88c7c86-release", - peerName: "thor/v2.1.1-88c7c86-release/linux/go1.21.9", - expected: true, - }, - { - id: "only-major-different", - appVersion: "2.1.1-88c7c86-release", - peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", - expected: false, - }, - { - id: "app-version-empty", - appVersion: "", - peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", - expected: true, + peerName: "thor/v2.1.1-88c7c86-release/linux/go1.21.9", + expected: false, }, { - id: "exact-match", - appVersion: "1.1.1-88c7c86-release", - peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", - expected: true, + peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", + expected: true, }, { - id: "bad-app-version", - // bad app version, so accept any peer - appVersion: "bad.bad.bad-88c7c86-release", - peerName: "thor/v1.1.1-88c7c86-release/linux/go1.21.9", - expected: true, + peerName: "thor/v3.1.1-88c7c86-release/linux/go1.21.9", + expected: false, }, } for _, tc := range testCases { - t.Run(tc.id, func(t *testing.T) { - result := sameMajor(tc.appVersion, tc.peerName) + t.Run(tc.peerName, func(t *testing.T) { + result := isIncompatible(tc.peerName) assert.Equal(t, tc.expected, result) }) } From c297567be0eac4a05118c5512e4dcbeaa20e53e2 Mon Sep 17 00:00:00 2001 From: Darren Kelly Date: Wed, 5 Jun 2024 08:31:02 +0100 Subject: [PATCH 9/9] fix: add tests --- p2psrv/server.go | 2 +- p2psrv/server_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/p2psrv/server.go b/p2psrv/server.go index 040bbb3c3..188fd2ce6 100644 --- a/p2psrv/server.go +++ b/p2psrv/server.go @@ -397,7 +397,7 @@ func isIncompatible(peerName string) bool { // Extract the semantic version from the name peerVersion := versionRegex.FindString(peerName) if peerVersion == "" { - return false + return true } major := strings.Split(peerVersion, ".")[0] diff --git a/p2psrv/server_test.go b/p2psrv/server_test.go index bd87e2798..4566c5dcf 100644 --- a/p2psrv/server_test.go +++ b/p2psrv/server_test.go @@ -93,6 +93,14 @@ func TestIsIncompatible(t *testing.T) { peerName: "thor/v3.1.1-88c7c86-release/linux/go1.21.9", expected: false, }, + { + peerName: "", + expected: true, + }, + { + peerName: "thor/v1.bad.v-88c7c86-release/linux/go1.21.9", + expected: true, + }, } for _, tc := range testCases {