From b8e3790600633043bfe1cfade1f802b18e18257b Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Mon, 14 Nov 2022 19:07:25 +0900 Subject: [PATCH 1/6] Return error --- p2p/transport.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/p2p/transport.go b/p2p/transport.go index 7c2c0493d..2165d28bf 100644 --- a/p2p/transport.go +++ b/p2p/transport.go @@ -265,13 +265,16 @@ func (mt *MultiplexTransport) Listen(addr NetAddress) error { // NOTE: NodeInfo must be of type DefaultNodeInfo else channels won't be updated // This is a bit messy at the moment but is cleaned up in the following version // when NodeInfo changes from an interface to a concrete type -func (mt *MultiplexTransport) AddChannel(chID byte) { - if ni, ok := mt.nodeInfo.(DefaultNodeInfo); ok { - if !ni.HasChannel(chID) { - ni.Channels = append(ni.Channels, chID) - } - mt.nodeInfo = ni +func (mt *MultiplexTransport) AddChannel(chID byte) error { + ni, ok := mt.nodeInfo.(DefaultNodeInfo) + if !ok { + return fmt.Errorf("nodeInfo type: %T is not supported", mt.nodeInfo) + } + if !ni.HasChannel(chID) { + ni.Channels = append(ni.Channels, chID) } + mt.nodeInfo = ni + return nil } func (mt *MultiplexTransport) acceptPeers() { From d825b8163bcff0b61799163ba420f48663d923a2 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Mon, 14 Nov 2022 19:16:46 +0900 Subject: [PATCH 2/6] add no lint tags --- node/node.go | 2 +- p2p/transport_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/node/node.go b/node/node.go index dcb1736cd..d5c98371c 100644 --- a/node/node.go +++ b/node/node.go @@ -198,7 +198,7 @@ func CustomReactors(reactors map[string]p2p.Reactor) Option { for _, chDesc := range reactor.GetChannels() { if !ni.HasChannel(chDesc.ID) { ni.Channels = append(ni.Channels, chDesc.ID) - n.transport.AddChannel(chDesc.ID) + n.transport.AddChannel(chDesc.ID) //nolint:errcheck } } n.nodeInfo = ni diff --git a/p2p/transport_test.go b/p2p/transport_test.go index 789ed81c9..5bf855cda 100644 --- a/p2p/transport_test.go +++ b/p2p/transport_test.go @@ -630,7 +630,7 @@ func TestTransportAddChannel(t *testing.T) { ) testChannel := byte(0x01) - mt.AddChannel(testChannel) + mt.AddChannel(testChannel) //nolint:errcheck if !mt.nodeInfo.(DefaultNodeInfo).HasChannel(testChannel) { t.Errorf("missing added channel %v. Got %v", testChannel, mt.nodeInfo.(DefaultNodeInfo).Channels) } From c8094e2ae60bb3efa72814831084ba3daf697bf3 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Tue, 15 Nov 2022 14:58:50 +0900 Subject: [PATCH 3/6] Add error handling --- node/node.go | 5 ++++- p2p/transport_test.go | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/node/node.go b/node/node.go index d5c98371c..5452bee1d 100644 --- a/node/node.go +++ b/node/node.go @@ -198,7 +198,10 @@ func CustomReactors(reactors map[string]p2p.Reactor) Option { for _, chDesc := range reactor.GetChannels() { if !ni.HasChannel(chDesc.ID) { ni.Channels = append(ni.Channels, chDesc.ID) - n.transport.AddChannel(chDesc.ID) //nolint:errcheck + err := n.transport.AddChannel(chDesc.ID) + if err != nil { + n.Logger.Debug("AddChannel failed", "err", err) + } } } n.nodeInfo = ni diff --git a/p2p/transport_test.go b/p2p/transport_test.go index 5bf855cda..80a497d6e 100644 --- a/p2p/transport_test.go +++ b/p2p/transport_test.go @@ -14,6 +14,7 @@ import ( "github.com/line/ostracon/libs/protoio" "github.com/line/ostracon/p2p/conn" tmp2p "github.com/line/ostracon/proto/ostracon/p2p" + "github.com/stretchr/testify/require" ) var defaultNodeName = "host_peer" @@ -630,7 +631,8 @@ func TestTransportAddChannel(t *testing.T) { ) testChannel := byte(0x01) - mt.AddChannel(testChannel) //nolint:errcheck + err := mt.AddChannel(testChannel) + require.NoError(t, err) if !mt.nodeInfo.(DefaultNodeInfo).HasChannel(testChannel) { t.Errorf("missing added channel %v. Got %v", testChannel, mt.nodeInfo.(DefaultNodeInfo).Channels) } From a6a45fd476f93961caa725ab220bee91d2238cd5 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Wed, 16 Nov 2022 18:48:31 +0900 Subject: [PATCH 4/6] Remove logging that never happens --- node/node.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/node/node.go b/node/node.go index 5452bee1d..dcb1736cd 100644 --- a/node/node.go +++ b/node/node.go @@ -198,10 +198,7 @@ func CustomReactors(reactors map[string]p2p.Reactor) Option { for _, chDesc := range reactor.GetChannels() { if !ni.HasChannel(chDesc.ID) { ni.Channels = append(ni.Channels, chDesc.ID) - err := n.transport.AddChannel(chDesc.ID) - if err != nil { - n.Logger.Debug("AddChannel failed", "err", err) - } + n.transport.AddChannel(chDesc.ID) } } n.nodeInfo = ni From 1ed0a49dd7fce1a976e2f1c217858d2c74654d57 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Wed, 16 Nov 2022 18:51:19 +0900 Subject: [PATCH 5/6] Add no lint tags --- node/node.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/node.go b/node/node.go index dcb1736cd..d5c98371c 100644 --- a/node/node.go +++ b/node/node.go @@ -198,7 +198,7 @@ func CustomReactors(reactors map[string]p2p.Reactor) Option { for _, chDesc := range reactor.GetChannels() { if !ni.HasChannel(chDesc.ID) { ni.Channels = append(ni.Channels, chDesc.ID) - n.transport.AddChannel(chDesc.ID) + n.transport.AddChannel(chDesc.ID) //nolint:errcheck } } n.nodeInfo = ni From dfcf497ce3468e8a9e8567aa1fdbbc425c4e9b22 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Wed, 16 Nov 2022 19:26:58 +0900 Subject: [PATCH 6/6] Add logging --- node/node.go | 5 ++- node/node_test.go | 66 ++++++++++++++++++++++++++++++ p2p/mocks/node_info.go | 93 ++++++++++++++++++++++++++++++++++++++++++ p2p/mocks/peer.go | 17 +++++++- p2p/node_info.go | 2 + 5 files changed, 181 insertions(+), 2 deletions(-) create mode 100644 p2p/mocks/node_info.go diff --git a/node/node.go b/node/node.go index d5c98371c..5452bee1d 100644 --- a/node/node.go +++ b/node/node.go @@ -198,7 +198,10 @@ func CustomReactors(reactors map[string]p2p.Reactor) Option { for _, chDesc := range reactor.GetChannels() { if !ni.HasChannel(chDesc.ID) { ni.Channels = append(ni.Channels, chDesc.ID) - n.transport.AddChannel(chDesc.ID) //nolint:errcheck + err := n.transport.AddChannel(chDesc.ID) + if err != nil { + n.Logger.Debug("AddChannel failed", "err", err) + } } } n.nodeInfo = ni diff --git a/node/node_test.go b/node/node_test.go index 8c619347e..ea09909c8 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -27,6 +27,7 @@ import ( "github.com/line/ostracon/p2p" "github.com/line/ostracon/p2p/conn" p2pmock "github.com/line/ostracon/p2p/mock" + p2pmocks "github.com/line/ostracon/p2p/mocks" "github.com/line/ostracon/privval" "github.com/line/ostracon/proxy" sm "github.com/line/ostracon/state" @@ -578,3 +579,68 @@ func state(nVals int, height int64) (sm.State, dbm.DB, []types.PrivValidator) { } return s, stateDB, privVals } + +func TestNodeInvalidNodeInfoCustomReactors(t *testing.T) { + config := cfg.ResetTestRoot("node_new_node_custom_reactors_test") + defer os.RemoveAll(config.RootDir) + + cr := p2pmock.NewReactor() + cr.Channels = []*conn.ChannelDescriptor{ + { + ID: byte(0x31), + Priority: 5, + SendQueueCapacity: 100, + RecvMessageCapacity: 100, + }, + } + customBlockchainReactor := p2pmock.NewReactor() + + nodeKey, err := p2p.LoadOrGenNodeKey(config.NodeKeyFile()) + require.NoError(t, err) + + pvKey, _ := privval.LoadOrGenFilePV(config.PrivValidatorKeyFile(), config.PrivValidatorStateFile(), + config.PrivValidatorKeyType()) + _, err = NewInvalidNode(config, + pvKey, + nodeKey, + proxy.DefaultClientCreator(config.ProxyApp, config.ABCI, config.DBDir()), + DefaultGenesisDocProviderFunc(config), + DefaultDBProvider, + DefaultMetricsProvider(config.Instrumentation), + log.TestingLogger(), + CustomReactors(map[string]p2p.Reactor{"FOO": cr, "BLOCKCHAIN": customBlockchainReactor}), + ) + require.NoError(t, err) +} + +func NewInvalidNode(config *cfg.Config, + privValidator types.PrivValidator, + nodeKey *p2p.NodeKey, + clientCreator proxy.ClientCreator, + genesisDocProvider GenesisDocProvider, + dbProvider DBProvider, + metricsProvider MetricsProvider, + logger log.Logger, + options ...Option) (*Node, error) { + n, err := NewNode(config, + privValidator, + nodeKey, + clientCreator, + genesisDocProvider, + dbProvider, + metricsProvider, + logger, + ) + if err != nil { + return nil, err + } + + transport, _ := createTransport(config, &p2pmocks.NodeInfo{}, nodeKey, n.proxyApp) + n.transport = transport + + for _, option := range options { + option(n) + } + + return n, nil +} diff --git a/p2p/mocks/node_info.go b/p2p/mocks/node_info.go new file mode 100644 index 000000000..4ec0a5d62 --- /dev/null +++ b/p2p/mocks/node_info.go @@ -0,0 +1,93 @@ +// Code generated by mockery v2.15.0. DO NOT EDIT. + +package mocks + +import ( + p2p "github.com/line/ostracon/p2p" + mock "github.com/stretchr/testify/mock" +) + +// NodeInfo is an autogenerated mock type for the NodeInfo type +type NodeInfo struct { + mock.Mock +} + +// CompatibleWith provides a mock function with given fields: other +func (_m *NodeInfo) CompatibleWith(other p2p.NodeInfo) error { + ret := _m.Called(other) + + var r0 error + if rf, ok := ret.Get(0).(func(p2p.NodeInfo) error); ok { + r0 = rf(other) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// ID provides a mock function with given fields: +func (_m *NodeInfo) ID() p2p.ID { + ret := _m.Called() + + var r0 p2p.ID + if rf, ok := ret.Get(0).(func() p2p.ID); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(p2p.ID) + } + + return r0 +} + +// NetAddress provides a mock function with given fields: +func (_m *NodeInfo) NetAddress() (*p2p.NetAddress, error) { + ret := _m.Called() + + var r0 *p2p.NetAddress + if rf, ok := ret.Get(0).(func() *p2p.NetAddress); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*p2p.NetAddress) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// Validate provides a mock function with given fields: +func (_m *NodeInfo) Validate() error { + ret := _m.Called() + + var r0 error + if rf, ok := ret.Get(0).(func() error); ok { + r0 = rf() + } else { + r0 = ret.Error(0) + } + + return r0 +} + +type mockConstructorTestingTNewNodeInfo interface { + mock.TestingT + Cleanup(func()) +} + +// NewNodeInfo creates a new instance of NodeInfo. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewNodeInfo(t mockConstructorTestingTNewNodeInfo) *NodeInfo { + mock := &NodeInfo{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/p2p/mocks/peer.go b/p2p/mocks/peer.go index 8a5e02c5b..838f3614f 100644 --- a/p2p/mocks/peer.go +++ b/p2p/mocks/peer.go @@ -1,4 +1,4 @@ -// Code generated by mockery 2.9.0. DO NOT EDIT. +// Code generated by mockery v2.15.0. DO NOT EDIT. package mocks @@ -329,3 +329,18 @@ func (_m *Peer) TrySend(_a0 byte, _a1 []byte) bool { return r0 } + +type mockConstructorTestingTNewPeer interface { + mock.TestingT + Cleanup(func()) +} + +// NewPeer creates a new instance of Peer. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewPeer(t mockConstructorTestingTNewPeer) *Peer { + mock := &Peer{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/p2p/node_info.go b/p2p/node_info.go index 3f9183e9d..5eb30d927 100644 --- a/p2p/node_info.go +++ b/p2p/node_info.go @@ -12,6 +12,8 @@ import ( "github.com/line/ostracon/version" ) +//go:generate mockery --case underscore --name NodeInfo + const ( maxNodeInfoSize = 10240 // 10KB maxNumChannels = 16 // plenty of room for upgrades, for now