From 466651854f8157d227f4b88b81aebd1d774bb819 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 3 Nov 2021 16:58:03 -0400 Subject: [PATCH 01/59] add lots of logs, comment out maintainTransactionPool for now --- dot/core/service.go | 48 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 289881e8dc..4fc36c7dc2 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -314,7 +314,11 @@ func (s *Service) handleBlocksAsync() { prev := s.blockState.BestBlockHash() select { - case block := <-s.blockAddCh: + case block, ok := <-s.blockAddCh: + if !ok { + return + } + if block == nil { continue } @@ -323,8 +327,7 @@ func (s *Service) handleBlocksAsync() { logger.Warn("failed to re-add transactions to chain upon re-org", "error", err) } - s.maintainTransactionPool(block) - + //s.maintainTransactionPool(block) case <-s.ctx.Done(): return } @@ -335,40 +338,66 @@ func (s *Service) handleBlocksAsync() { // previous chain head). If there is a re-org, it moves the transactions that were included on the previous // chain back into the transaction pool. func (s *Service) handleChainReorg(prev, curr common.Hash) error { + logger.Trace("handleChainReorg", "prev", prev, "curr", curr) + ancestor, err := s.blockState.HighestCommonAncestor(prev, curr) if err != nil { return err } + logger.Trace("got HighestCommonAncestor", "ancestor", ancestor) + // if the highest common ancestor of the previous chain head and current chain head is the previous chain head, // then the current chain head is the descendant of the previous and thus are on the same chain if ancestor == prev { return nil } + logger.Trace("getting subchain...") + subchain, err := s.blockState.SubChain(ancestor, prev) if err != nil { return err } + logger.Trace("got subchain") + // subchain contains the ancestor as well so we need to remove it. if len(subchain) > 0 { subchain = subchain[1:] + } else { + return nil } + logger.Trace("getting runtime") + // Check transaction validation on the best block. rt, err := s.blockState.GetRuntime(nil) if err != nil { return err } + if rt == nil { + panic("runtime is nil!") + } + + logger.Trace("got runtime") + // for each block in the previous chain, re-add its extrinsics back into the pool for _, hash := range subchain { + logger.Trace("handling subchain block", "hash", hash) + body, err := s.blockState.GetBlockBody(hash) if err != nil { continue } + if body == nil { + panic("body is nil!") + } + + logger.Trace("handling block body...", "body", body) + for _, ext := range *body { logger.Trace("validating transaction on re-org chain", "extrinsic", ext) encExt, err := scale.Marshal(ext) @@ -378,11 +407,12 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { // decode extrinsic and make sure it's not an inherent. decExt := &types.ExtrinsicData{} - err = decExt.DecodeVersion(encExt) - if err != nil { - return err + if err = decExt.DecodeVersion(encExt); err != nil { + continue } + logger.Trace("decoded ext version", decExt) + // Inherent are not signed. if !decExt.IsSigned() { continue @@ -391,12 +421,16 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { externalExt := types.Extrinsic(append([]byte{byte(types.TxnExternal)}, encExt...)) txv, err := rt.ValidateTransaction(externalExt) if err != nil { - logger.Info("failed to validate transaction", "error", err, "extrinsic", ext) + logger.Debug("failed to validate transaction", "error", err, "extrinsic", ext) continue } + logger.Trace("validated tx", "ext", ext) + vtx := transaction.NewValidTransaction(encExt, txv) s.transactionState.AddToPool(vtx) + + logger.Trace("added tx to pool") } } From 06928251f7b99c984406c4ae7ba396ec27843ba1 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 3 Nov 2021 19:18:43 -0400 Subject: [PATCH 02/59] update blocktree.HighestCommonAncestor to return err if ancestor is nil --- lib/blocktree/blocktree.go | 10 +++++++++- lib/blocktree/errors.go | 3 +++ lib/blocktree/node.go | 19 +++++++++++++------ 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/lib/blocktree/blocktree.go b/lib/blocktree/blocktree.go index db5f8066d2..0790a4555f 100644 --- a/lib/blocktree/blocktree.go +++ b/lib/blocktree/blocktree.go @@ -338,12 +338,20 @@ func (bt *BlockTree) HighestCommonAncestor(a, b Hash) (Hash, error) { if an == nil { return common.Hash{}, ErrNodeNotFound } + bn := bt.getNode(b) if bn == nil { return common.Hash{}, ErrNodeNotFound } - return an.highestCommonAncestor(bn).hash, nil + ancestor := an.highestCommonAncestor(bn) + if ancestor == nil { + // this case shouldn't happen - any two nodes in the blocktree must + // have a common ancestor, the lowest of which is the root node + return common.Hash{}, ErrNoCommonAncestor + } + + return ancestor.hash, nil } // GetAllBlocks returns all the blocks in the tree diff --git a/lib/blocktree/errors.go b/lib/blocktree/errors.go index 57a3be78d2..d7ed98d04b 100644 --- a/lib/blocktree/errors.go +++ b/lib/blocktree/errors.go @@ -38,5 +38,8 @@ var ( // ErrNumLowerThanRoot is returned when attempting to get a hash by number that is lower than the root node ErrNumLowerThanRoot = errors.New("cannot find node with number lower than root node") + // ErrNoCommonAncestor is returned when a common ancestor cannot be found between two nodes + ErrNoCommonAncestor = errors.New("no common ancestor between two nodes") + errUnexpectedNumber = errors.New("block number is not parent number + 1") ) diff --git a/lib/blocktree/node.go b/lib/blocktree/node.go index 1222e14a98..309ba9177c 100644 --- a/lib/blocktree/node.go +++ b/lib/blocktree/node.go @@ -55,17 +55,24 @@ func (n *node) createTree(tree gotree.Tree) { // getNode recursively searches for a node with a given hash func (n *node) getNode(h common.Hash) *node { + if n == nil { + return nil + } + if n.hash == h { return n - } else if len(n.children) == 0 { + } + + if len(n.children) == 0 { return nil - } else { - for _, child := range n.children { - if n := child.getNode(h); n != nil { - return n - } + } + + for _, child := range n.children { + if n := child.getNode(h); n != nil { + return n } } + return nil } From 71f2937a3c88dc23f98dbff28035d5bb3501d279 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 4 Nov 2021 12:17:13 -0400 Subject: [PATCH 03/59] increase first block timeout --- lib/babe/babe.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/babe/babe.go b/lib/babe/babe.go index bc09fe3ca9..f8de51a7d7 100644 --- a/lib/babe/babe.go +++ b/lib/babe/babe.go @@ -215,7 +215,7 @@ func (b *Service) waitForFirstBlock() error { ch := b.blockState.GetImportedBlockNotifierChannel() defer b.blockState.FreeImportedBlockNotifierChannel(ch) - const firstBlockTimeout = time.Minute + const firstBlockTimeout = time.Minute * 5 timer := time.NewTimer(firstBlockTimeout) cleanup := func() { if !timer.Stop() { From 2950c17a33b5ed6bc611e02a87ab4f5ac919477c Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 4 Nov 2021 12:25:12 -0400 Subject: [PATCH 04/59] close and set stream to nil if err on message send --- dot/network/notifications.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 7ba62745e7..3fc2f20e6c 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -349,7 +349,9 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc err := s.host.writeToStream(hsData.stream, msg) if err != nil { - logger.Debug("failed to send message to peer", "peer", peer, "error", err) + logger.Debug("failed to send message to peer", "protocol", info.protocolID, "peer", peer, "error", err) + _ = hsData.stream.Close() + hsData.stream = nil } } From 45973447257ad78c804aa9ce7b8a401e63eae003 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 4 Nov 2021 14:26:20 -0400 Subject: [PATCH 05/59] close stream if failed to decode msg --- dot/network/notifications.go | 3 +++ dot/network/service.go | 1 + 2 files changed, 4 insertions(+) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 3fc2f20e6c..493026846c 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -352,7 +352,10 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc logger.Debug("failed to send message to peer", "protocol", info.protocolID, "peer", peer, "error", err) _ = hsData.stream.Close() hsData.stream = nil + return } + + logger.Trace("successfully sent message", "protocol", info.protocolID, "peer", peer, "message", msg) } // broadcastExcluding sends a message to each connected peer except the given peer, diff --git a/dot/network/service.go b/dot/network/service.go index c192486859..93f057672c 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -587,6 +587,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) if err != nil { logger.Trace("failed to decode message from peer", "id", stream.ID(), "protocol", stream.Protocol(), "err", err) + _ = stream.Close() continue } From d87397995b2dd3b67d56aea87e1732b76c841a74 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 4 Nov 2021 19:13:37 -0400 Subject: [PATCH 06/59] add block announce and grandpa stream count metrics --- dot/core/service.go | 28 +++------------------------- dot/network/service.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 4fc36c7dc2..18f0730fa9 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -327,7 +327,7 @@ func (s *Service) handleBlocksAsync() { logger.Warn("failed to re-add transactions to chain upon re-org", "error", err) } - //s.maintainTransactionPool(block) + s.maintainTransactionPool(block) case <-s.ctx.Done(): return } @@ -338,30 +338,22 @@ func (s *Service) handleBlocksAsync() { // previous chain head). If there is a re-org, it moves the transactions that were included on the previous // chain back into the transaction pool. func (s *Service) handleChainReorg(prev, curr common.Hash) error { - logger.Trace("handleChainReorg", "prev", prev, "curr", curr) - ancestor, err := s.blockState.HighestCommonAncestor(prev, curr) if err != nil { return err } - logger.Trace("got HighestCommonAncestor", "ancestor", ancestor) - // if the highest common ancestor of the previous chain head and current chain head is the previous chain head, // then the current chain head is the descendant of the previous and thus are on the same chain if ancestor == prev { return nil } - logger.Trace("getting subchain...") - subchain, err := s.blockState.SubChain(ancestor, prev) if err != nil { return err } - logger.Trace("got subchain") - // subchain contains the ancestor as well so we need to remove it. if len(subchain) > 0 { subchain = subchain[1:] @@ -369,8 +361,6 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { return nil } - logger.Trace("getting runtime") - // Check transaction validation on the best block. rt, err := s.blockState.GetRuntime(nil) if err != nil { @@ -378,26 +368,20 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { } if rt == nil { - panic("runtime is nil!") + return ErrNilRuntime } - logger.Trace("got runtime") - // for each block in the previous chain, re-add its extrinsics back into the pool for _, hash := range subchain { - logger.Trace("handling subchain block", "hash", hash) - body, err := s.blockState.GetBlockBody(hash) if err != nil { continue } if body == nil { - panic("body is nil!") + continue } - logger.Trace("handling block body...", "body", body) - for _, ext := range *body { logger.Trace("validating transaction on re-org chain", "extrinsic", ext) encExt, err := scale.Marshal(ext) @@ -411,8 +395,6 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { continue } - logger.Trace("decoded ext version", decExt) - // Inherent are not signed. if !decExt.IsSigned() { continue @@ -425,12 +407,8 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { continue } - logger.Trace("validated tx", "ext", ext) - vtx := transaction.NewValidTransaction(encExt, txv) s.transactionState.AddToPool(vtx) - - logger.Trace("added tx to pool") } } diff --git a/dot/network/service.go b/dot/network/service.go index 93f057672c..a150311f14 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -289,11 +289,20 @@ func (s *Service) collectNetworkMetrics() { totalConn := metrics.GetOrRegisterGauge("network/node/totalConnection", metrics.DefaultRegistry) networkLatency := metrics.GetOrRegisterGauge("network/node/latency", metrics.DefaultRegistry) syncedBlocks := metrics.GetOrRegisterGauge("service/blocks/sync", metrics.DefaultRegistry) + numInboundBlockAnnounceStreams := metrics.GetOrRegisterGauge("network/streams/block_announce/inbound", metrics.DefaultRegistry) + numOutboundBlockAnnounceStreams := metrics.GetOrRegisterGauge("network/streams/block_announce/outbound", metrics.DefaultRegistry) + numInboundGrandpaStreams := metrics.GetOrRegisterGauge("network/streams/grandpa/inbound", metrics.DefaultRegistry) + numOutboundGrandpaStreams := metrics.GetOrRegisterGauge("network/streams/grandpa/outbound", metrics.DefaultRegistry) peerCount.Update(int64(s.host.peerCount())) totalConn.Update(int64(len(s.host.h.Network().Conns()))) networkLatency.Update(int64(s.host.h.Peerstore().LatencyEWMA(s.host.id()))) + numInboundBlockAnnounceStreams.Update(s.getNumStreams(BlockAnnounceMsgType, true)) + numOutboundBlockAnnounceStreams.Update(s.getNumStreams(BlockAnnounceMsgType, false)) + numInboundGrandpaStreams.Update(s.getNumStreams(ConsensusMsgType, false)) + numOutboundGrandpaStreams.Update(s.getNumStreams(ConsensusMsgType, true)) + num, err := s.blockState.BestBlockNumber() if err != nil { syncedBlocks.Update(0) @@ -305,6 +314,35 @@ func (s *Service) collectNetworkMetrics() { } } +func (s *Service) getNumStreams(protocolID byte, inbound bool) int64 { + np, has := s.notificationsProtocols[protocolID] + if !has { + return 0 + } + + var hsData *sync.Map + if inbound { + hsData = np.inboundHandshakeData + } else { + hsData = np.outboundHandshakeData + } + + var count int64 + hsData.Range(func(_, data interface{}) bool { + if data == nil { + return true + } + + if data.(*handshakeData).stream != nil { + count++ + } + + return true + }) + + return count +} + func (s *Service) logPeerCount() { ticker := time.NewTicker(time.Second * 30) defer ticker.Stop() From 07ed9079a6384ea6e93b997e4c18a2f62f326633 Mon Sep 17 00:00:00 2001 From: noot Date: Fri, 5 Nov 2021 11:10:28 -0400 Subject: [PATCH 07/59] fix getNumStreams interface conv --- dot/network/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/network/service.go b/dot/network/service.go index a150311f14..08b0831037 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -333,7 +333,7 @@ func (s *Service) getNumStreams(protocolID byte, inbound bool) int64 { return true } - if data.(*handshakeData).stream != nil { + if data.(handshakeData).stream != nil { count++ } From bed53dc16703dfcd88e84f60aef38a336a356561 Mon Sep 17 00:00:00 2001 From: noot Date: Fri, 5 Nov 2021 11:16:55 -0400 Subject: [PATCH 08/59] check error in sendData --- dot/network/notifications.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 493026846c..671437b123 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -18,10 +18,12 @@ package network import ( "errors" + "io" "reflect" "sync" "time" + "github.com/libp2p/go-libp2p-core/mux" libp2pnetwork "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" "github.com/libp2p/go-libp2p-core/protocol" @@ -350,8 +352,14 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc err := s.host.writeToStream(hsData.stream, msg) if err != nil { logger.Debug("failed to send message to peer", "protocol", info.protocolID, "peer", peer, "error", err) - _ = hsData.stream.Close() - hsData.stream = nil + + // the stream was closed or reset, close it on our end and delete it from our peer's data + if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { + s := hsData.stream + hsData.stream = nil + _ = s.Close() + } + return } From 39de2d7cef054a221f876d7ed52a88e96111bd73 Mon Sep 17 00:00:00 2001 From: noot Date: Fri, 5 Nov 2021 11:43:25 -0400 Subject: [PATCH 09/59] add nil checks to HighestCommonAncestor, increase BABE first block timeout --- lib/babe/babe.go | 2 +- lib/blocktree/blocktree.go | 10 +++++++++- lib/blocktree/errors.go | 3 +++ lib/blocktree/node.go | 19 +++++++++++++------ 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/lib/babe/babe.go b/lib/babe/babe.go index bc09fe3ca9..f8de51a7d7 100644 --- a/lib/babe/babe.go +++ b/lib/babe/babe.go @@ -215,7 +215,7 @@ func (b *Service) waitForFirstBlock() error { ch := b.blockState.GetImportedBlockNotifierChannel() defer b.blockState.FreeImportedBlockNotifierChannel(ch) - const firstBlockTimeout = time.Minute + const firstBlockTimeout = time.Minute * 5 timer := time.NewTimer(firstBlockTimeout) cleanup := func() { if !timer.Stop() { diff --git a/lib/blocktree/blocktree.go b/lib/blocktree/blocktree.go index db5f8066d2..0790a4555f 100644 --- a/lib/blocktree/blocktree.go +++ b/lib/blocktree/blocktree.go @@ -338,12 +338,20 @@ func (bt *BlockTree) HighestCommonAncestor(a, b Hash) (Hash, error) { if an == nil { return common.Hash{}, ErrNodeNotFound } + bn := bt.getNode(b) if bn == nil { return common.Hash{}, ErrNodeNotFound } - return an.highestCommonAncestor(bn).hash, nil + ancestor := an.highestCommonAncestor(bn) + if ancestor == nil { + // this case shouldn't happen - any two nodes in the blocktree must + // have a common ancestor, the lowest of which is the root node + return common.Hash{}, ErrNoCommonAncestor + } + + return ancestor.hash, nil } // GetAllBlocks returns all the blocks in the tree diff --git a/lib/blocktree/errors.go b/lib/blocktree/errors.go index 57a3be78d2..d7ed98d04b 100644 --- a/lib/blocktree/errors.go +++ b/lib/blocktree/errors.go @@ -38,5 +38,8 @@ var ( // ErrNumLowerThanRoot is returned when attempting to get a hash by number that is lower than the root node ErrNumLowerThanRoot = errors.New("cannot find node with number lower than root node") + // ErrNoCommonAncestor is returned when a common ancestor cannot be found between two nodes + ErrNoCommonAncestor = errors.New("no common ancestor between two nodes") + errUnexpectedNumber = errors.New("block number is not parent number + 1") ) diff --git a/lib/blocktree/node.go b/lib/blocktree/node.go index 1222e14a98..309ba9177c 100644 --- a/lib/blocktree/node.go +++ b/lib/blocktree/node.go @@ -55,17 +55,24 @@ func (n *node) createTree(tree gotree.Tree) { // getNode recursively searches for a node with a given hash func (n *node) getNode(h common.Hash) *node { + if n == nil { + return nil + } + if n.hash == h { return n - } else if len(n.children) == 0 { + } + + if len(n.children) == 0 { return nil - } else { - for _, child := range n.children { - if n := child.getNode(h); n != nil { - return n - } + } + + for _, child := range n.children { + if n := child.getNode(h); n != nil { + return n } } + return nil } From a07d83dfdae39ec41c0e677dccd23d3db3645929 Mon Sep 17 00:00:00 2001 From: noot Date: Fri, 5 Nov 2021 11:43:53 -0400 Subject: [PATCH 10/59] add nil checks to core service --- dot/core/service.go | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index f79dd56f5e..18f0730fa9 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -314,7 +314,11 @@ func (s *Service) handleBlocksAsync() { prev := s.blockState.BestBlockHash() select { - case block := <-s.blockAddCh: + case block, ok := <-s.blockAddCh: + if !ok { + return + } + if block == nil { continue } @@ -353,6 +357,8 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { // subchain contains the ancestor as well so we need to remove it. if len(subchain) > 0 { subchain = subchain[1:] + } else { + return nil } // Check transaction validation on the best block. @@ -361,6 +367,10 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { return err } + if rt == nil { + return ErrNilRuntime + } + // for each block in the previous chain, re-add its extrinsics back into the pool for _, hash := range subchain { body, err := s.blockState.GetBlockBody(hash) @@ -368,6 +378,10 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { continue } + if body == nil { + continue + } + for _, ext := range *body { logger.Trace("validating transaction on re-org chain", "extrinsic", ext) encExt, err := scale.Marshal(ext) @@ -377,9 +391,8 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { // decode extrinsic and make sure it's not an inherent. decExt := &types.ExtrinsicData{} - err = decExt.DecodeVersion(encExt) - if err != nil { - return err + if err = decExt.DecodeVersion(encExt); err != nil { + continue } // Inherent are not signed. @@ -390,7 +403,7 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { externalExt := types.Extrinsic(append([]byte{byte(types.TxnExternal)}, encExt...)) txv, err := rt.ValidateTransaction(externalExt) if err != nil { - logger.Info("failed to validate transaction", "error", err, "extrinsic", ext) + logger.Debug("failed to validate transaction", "error", err, "extrinsic", ext) continue } From 178f727bb09bff7a13391a9d97564ff0596243ac Mon Sep 17 00:00:00 2001 From: noot Date: Mon, 8 Nov 2021 09:44:52 -0500 Subject: [PATCH 11/59] address comments --- dot/core/service.go | 6 +----- lib/blocktree/blocktree.go | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 18f0730fa9..faa738f082 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -374,11 +374,7 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { // for each block in the previous chain, re-add its extrinsics back into the pool for _, hash := range subchain { body, err := s.blockState.GetBlockBody(hash) - if err != nil { - continue - } - - if body == nil { + if err != nil || body == nil { continue } diff --git a/lib/blocktree/blocktree.go b/lib/blocktree/blocktree.go index 0790a4555f..d7e807488d 100644 --- a/lib/blocktree/blocktree.go +++ b/lib/blocktree/blocktree.go @@ -348,7 +348,7 @@ func (bt *BlockTree) HighestCommonAncestor(a, b Hash) (Hash, error) { if ancestor == nil { // this case shouldn't happen - any two nodes in the blocktree must // have a common ancestor, the lowest of which is the root node - return common.Hash{}, ErrNoCommonAncestor + return common.Hash{}, fmt.Errorf("%w: %s and %s", ErrNoCommonAncestor, a, b) } return ancestor.hash, nil From f335df9de5b5e1b527b43253fff510237af73301 Mon Sep 17 00:00:00 2001 From: noot Date: Mon, 8 Nov 2021 14:34:07 -0500 Subject: [PATCH 12/59] update blockState.AddBlockToBlockTree to also store in unfinalised block map --- dot/state/block.go | 11 ++++------- dot/state/block_finalisation.go | 4 +++- dot/sync/chain_processor.go | 2 +- dot/sync/interface.go | 2 +- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/dot/state/block.go b/dot/state/block.go index 09632c733a..d21108647e 100644 --- a/dot/state/block.go +++ b/dot/state/block.go @@ -221,10 +221,6 @@ func (bs *BlockState) getAndDeleteUnfinalisedBlock(hash common.Hash) (*types.Blo return block.(*types.Block), true } -func (bs *BlockState) deleteUnfinalisedBlock(hash common.Hash) { - bs.unfinalisedBlocks.Delete(hash) -} - // HasHeader returns if the db contains a header with the given hash func (bs *BlockState) HasHeader(hash common.Hash) (bool, error) { if bs.hasUnfinalisedBlock(hash) { @@ -434,16 +430,17 @@ func (bs *BlockState) AddBlockWithArrivalTime(block *types.Block, arrivalTime ti // AddBlockToBlockTree adds the given block to the blocktree. It does not write it to the database. // TODO: remove this func and usage from sync (after sync refactor?) -func (bs *BlockState) AddBlockToBlockTree(header *types.Header) error { +func (bs *BlockState) AddBlockToBlockTree(block *types.Block) error { bs.Lock() defer bs.Unlock() - arrivalTime, err := bs.GetArrivalTime(header.Hash()) + arrivalTime, err := bs.GetArrivalTime(block.Header.Hash()) if err != nil { arrivalTime = time.Now() } - return bs.bt.AddBlock(header, arrivalTime) + bs.storeUnfinalisedBlock(block) + return bs.bt.AddBlock(&block.Header, arrivalTime) } // GetAllBlocksAtNumber returns all unfinalised blocks with the given number diff --git a/dot/state/block_finalisation.go b/dot/state/block_finalisation.go index 2e85ac37d4..e53b86d8be 100644 --- a/dot/state/block_finalisation.go +++ b/dot/state/block_finalisation.go @@ -164,6 +164,7 @@ func (bs *BlockState) SetFinalisedHash(hash common.Hash, round, setID uint64) er continue } + // blocks were deleted from the unfinalisedBlockMap in `handleFinalisedBlock()` logger.Trace("pruned block", "hash", hash, "number", block.Header.Number) go func(header *types.Header) { @@ -252,7 +253,8 @@ func (bs *BlockState) handleFinalisedBlock(curr common.Hash) error { return err } - bs.deleteUnfinalisedBlock(hash) + // the block will be deleted from the unfinalisedBlockMap in the pruning loop + // in `SetFinalisedHash()`, which calls this function } return batch.Flush() diff --git a/dot/sync/chain_processor.go b/dot/sync/chain_processor.go index bc7972ea45..7216e8ba4b 100644 --- a/dot/sync/chain_processor.go +++ b/dot/sync/chain_processor.go @@ -130,7 +130,7 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error { logger.Debug("skipping block, already have", "hash", bd.Hash, "number", block.Header.Number) - err = s.blockState.AddBlockToBlockTree(&block.Header) + err = s.blockState.AddBlockToBlockTree(block) if errors.Is(err, blocktree.ErrBlockExists) { return nil } else if err != nil { diff --git a/dot/sync/interface.go b/dot/sync/interface.go index a9746523c3..94988562c4 100644 --- a/dot/sync/interface.go +++ b/dot/sync/interface.go @@ -50,7 +50,7 @@ type BlockState interface { GetJustification(common.Hash) ([]byte, error) SetJustification(hash common.Hash, data []byte) error SetFinalisedHash(hash common.Hash, round, setID uint64) error - AddBlockToBlockTree(header *types.Header) error + AddBlockToBlockTree(block *types.Block) error GetHashByNumber(*big.Int) (common.Hash, error) GetBlockByHash(common.Hash) (*types.Block, error) GetRuntime(*common.Hash) (runtime.Instance, error) From 67f59cc7dd16dd039ac423a3687580a244ab43de Mon Sep 17 00:00:00 2001 From: noot Date: Mon, 8 Nov 2021 14:39:32 -0500 Subject: [PATCH 13/59] fix tests --- dot/state/block_test.go | 5 ++++- dot/sync/mocks/block_state.go | 10 +++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/dot/state/block_test.go b/dot/state/block_test.go index e6469531e3..bf27a2bf61 100644 --- a/dot/state/block_test.go +++ b/dot/state/block_test.go @@ -470,7 +470,10 @@ func TestAddBlockToBlockTree(t *testing.T) { err := bs.setArrivalTime(header.Hash(), time.Now()) require.NoError(t, err) - err = bs.AddBlockToBlockTree(header) + err = bs.AddBlockToBlockTree(&types.Block{ + Header: *header, + Body: types.Body{}, + }) require.NoError(t, err) require.Equal(t, bs.BestBlockHash(), header.Hash()) } diff --git a/dot/sync/mocks/block_state.go b/dot/sync/mocks/block_state.go index 2a3dcd60bc..a8ef319dc5 100644 --- a/dot/sync/mocks/block_state.go +++ b/dot/sync/mocks/block_state.go @@ -32,13 +32,13 @@ func (_m *BlockState) AddBlock(_a0 *types.Block) error { return r0 } -// AddBlockToBlockTree provides a mock function with given fields: header -func (_m *BlockState) AddBlockToBlockTree(header *types.Header) error { - ret := _m.Called(header) +// AddBlockToBlockTree provides a mock function with given fields: block +func (_m *BlockState) AddBlockToBlockTree(block *types.Block) error { + ret := _m.Called(block) var r0 error - if rf, ok := ret.Get(0).(func(*types.Header) error); ok { - r0 = rf(header) + if rf, ok := ret.Get(0).(func(*types.Block) error); ok { + r0 = rf(block) } else { r0 = ret.Error(0) } From 941f954a76b4614fb15f020f0d86116d1df6c5be Mon Sep 17 00:00:00 2001 From: noot Date: Mon, 8 Nov 2021 14:47:44 -0500 Subject: [PATCH 14/59] cleanup --- dot/state/block_finalisation.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/dot/state/block_finalisation.go b/dot/state/block_finalisation.go index e53b86d8be..473a9674c1 100644 --- a/dot/state/block_finalisation.go +++ b/dot/state/block_finalisation.go @@ -164,7 +164,6 @@ func (bs *BlockState) SetFinalisedHash(hash common.Hash, round, setID uint64) er continue } - // blocks were deleted from the unfinalisedBlockMap in `handleFinalisedBlock()` logger.Trace("pruned block", "hash", hash, "number", block.Header.Number) go func(header *types.Header) { @@ -195,7 +194,6 @@ func (bs *BlockState) SetFinalisedHash(hash common.Hash, round, setID uint64) er return fmt.Errorf("could not send 'notify.finalized' telemetry message, error: %s", err) } - // return bs.setHighestRoundAndSetID(round, setID) bs.lastFinalised = hash return nil } From d9bdff469fdb44601394486cd8bb12bc07121dc9 Mon Sep 17 00:00:00 2001 From: noot Date: Mon, 8 Nov 2021 14:57:14 -0500 Subject: [PATCH 15/59] add logs for when body isn't in response --- dot/sync/chain_sync.go | 2 +- dot/sync/message.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dot/sync/chain_sync.go b/dot/sync/chain_sync.go index e7d4d5c9b4..6f30f3595e 100644 --- a/dot/sync/chain_sync.go +++ b/dot/sync/chain_sync.go @@ -813,7 +813,7 @@ func validateBlockData(req *network.BlockRequestMessage, bd *types.BlockData) er } if (requestedData&network.RequestedDataBody>>1) == 1 && bd.Body == nil { - return errNilBodyInResponse + return fmt.Errorf("%w: hash=%s", errNilBodyInResponse, bd.Hash) } return nil diff --git a/dot/sync/message.go b/dot/sync/message.go index 5b3d324895..985d074b65 100644 --- a/dot/sync/message.go +++ b/dot/sync/message.go @@ -406,14 +406,14 @@ func (s *Service) getBlockData(hash common.Hash, requestedData byte) (*types.Blo if (requestedData & network.RequestedDataHeader) == 1 { blockData.Header, err = s.blockState.GetHeader(hash) if err != nil { - logger.Debug("failed to get header for block", "hash", hash, "error", err) + logger.Warn("failed to get header for block", "hash", hash, "error", err) } } if (requestedData&network.RequestedDataBody)>>1 == 1 { blockData.Body, err = s.blockState.GetBlockBody(hash) if err != nil { - logger.Debug("failed to get body for block", "hash", hash, "error", err) + logger.Warn("failed to get body for block", "hash", hash, "error", err) } } From 00091c5ee1fd357b890b830d8f968e1b3b1aae06 Mon Sep 17 00:00:00 2001 From: noot Date: Tue, 9 Nov 2021 12:40:29 -0500 Subject: [PATCH 16/59] cleanup --- dot/core/service.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/dot/core/service.go b/dot/core/service.go index 0f036ec6ca..faa738f082 100644 --- a/dot/core/service.go +++ b/dot/core/service.go @@ -378,10 +378,6 @@ func (s *Service) handleChainReorg(prev, curr common.Hash) error { continue } - if body == nil { - continue - } - for _, ext := range *body { logger.Trace("validating transaction on re-org chain", "extrinsic", ext) encExt, err := scale.Marshal(ext) From 577ff25603db2899b62d6f3c8ba0214a115c62e2 Mon Sep 17 00:00:00 2001 From: noot Date: Tue, 9 Nov 2021 12:41:57 -0500 Subject: [PATCH 17/59] delete hsData on stream close --- dot/network/notifications.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index cfb6b8c590..40628ab28b 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -373,9 +373,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // the stream was closed or reset, close it on our end and delete it from our peer's data if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { - s := hsData.stream - hsData.stream = nil - _ = s.Close() + _ = hsData.stream.Close() + info.outboundHandshakeData.Delete(peer) } return From 178434ec280e6f1a396bb6283bc444d5a6a12699 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 18:23:42 -0500 Subject: [PATCH 18/59] cleanup logic in createNotificationsMessageHandler --- dot/network/notifications.go | 44 ++++++++++++++++++++++++++---------- dot/network/service.go | 3 +-- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 40628ab28b..7987141c7a 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -18,6 +18,7 @@ package network import ( "errors" + "fmt" "io" "reflect" "sync" @@ -157,6 +158,8 @@ func createDecoder(info *notificationsProtocol, handshakeDecoder HandshakeDecode } func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, messageHandler NotificationsMessageHandler, batchHandler NotificationsMessageBatchHandler) messageHandler { + // if this function returns an error, it should *always* delete the inbound handshake data + // associated with it, if it exists. return func(stream libp2pnetwork.Stream, m Message) error { if m == nil || info == nil || info.handshakeValidator == nil || messageHandler == nil { return nil @@ -196,6 +199,7 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, err := info.handshakeValidator(peer, hs) if err != nil { logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) + info.inboundHandshakeData.Delete(peer) return errCannotValidateHandshake } @@ -206,14 +210,17 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, resp, err := info.getHandshake() if err != nil { logger.Warn("failed to get handshake", "protocol", info.protocolID, "error", err) + info.inboundHandshakeData.Delete(peer) return err } err = s.host.writeToStream(stream, resp) if err != nil { logger.Trace("failed to send handshake", "protocol", info.protocolID, "peer", peer, "error", err) + info.inboundHandshakeData.Delete(peer) return err } + logger.Trace("receiver: sent handshake", "protocol", info.protocolID, "peer", peer) } @@ -233,6 +240,7 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, if batchHandler != nil { msgs, err = batchHandler(peer, msg) if err != nil { + info.inboundHandshakeData.Delete(peer) return err } @@ -240,8 +248,10 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, } else { propagate, err = messageHandler(peer, msg) if err != nil { + info.inboundHandshakeData.Delete(peer) return err } + msgs = append(msgs, &BatchMessage{ msg: msg, peer: peer, @@ -279,34 +289,43 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc return } + if info.handshakeValidator == nil { + logger.Warn("handshakeValidator is not set for protocol!", "protocol", info.protocolID) + return + } + hsData, has := info.getOutboundHandshakeData(peer) - if has && !hsData.validated { + if !has { + hsData = newHandshakeData(false, false, nil) + } + + hsData.Lock() + defer hsData.Unlock() + + if has && hsData.received && !hsData.validated { // peer has sent us an invalid handshake in the past, ignore + logger.Warn("peer sent us invalid handshake before, ignoring...", "peer", peer, "protocol", info.protocolID) return } - if !has || !hsData.received || hsData.stream == nil { + if !has || !hsData.received /*|| hsData.stream == nil*/ { if !has { hsData = newHandshakeData(false, false, nil) + } else if has && hsData.stream == nil { + panic(fmt.Sprintf("stream is nil, but it shouldn't be!! %s", info.protocolID)) } - hsData.Lock() - defer hsData.Unlock() - logger.Trace("sending outbound handshake", "protocol", info.protocolID, "peer", peer, "message", hs) stream, err := s.host.send(peer, info.protocolID, hs) if err != nil { logger.Trace("failed to send message to peer", "peer", peer, "error", err) + _ = stream.Close() return } hsData.stream = stream info.outboundHandshakeData.Store(peer, hsData) - if info.handshakeValidator == nil { - return - } - hsTimer := time.NewTimer(handshakeTimeout) var hs Handshake @@ -334,10 +353,11 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc hsData.received = true } - err = info.handshakeValidator(peer, hs) - if err != nil { + if err = info.handshakeValidator(peer, hs); err != nil { logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) hsData.validated = false + hsData.stream = nil + _ = stream.Close() info.outboundHandshakeData.Store(peer, hsData) return } @@ -365,7 +385,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc } // we've completed the handshake with the peer, send message directly - logger.Trace("sending message", "protocol", info.protocolID, "peer", peer, "message", msg) + logger.Debug("sending message", "protocol", info.protocolID, "peer", peer, "message", msg) err := s.host.writeToStream(hsData.stream, msg) if err != nil { diff --git a/dot/network/service.go b/dot/network/service.go index d600002a3b..2c9e5a1e3c 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -639,8 +639,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder "msg", msg.String(), ) - err = handler(stream, msg) - if err != nil { + if err = handler(stream, msg); err != nil { logger.Trace("failed to handle message from stream", "id", stream.ID(), "message", msg, "error", err) _ = stream.Close() return From c6a946573777cc3241733013e51a1efe372400c9 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 18:48:45 -0500 Subject: [PATCH 19/59] update readStream to use closeInboundStream --- dot/network/notifications.go | 10 ++-------- dot/network/service.go | 25 +++++++++++++++++++++---- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 7987141c7a..926ea69a13 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -158,8 +158,6 @@ func createDecoder(info *notificationsProtocol, handshakeDecoder HandshakeDecode } func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, messageHandler NotificationsMessageHandler, batchHandler NotificationsMessageBatchHandler) messageHandler { - // if this function returns an error, it should *always* delete the inbound handshake data - // associated with it, if it exists. return func(stream libp2pnetwork.Stream, m Message) error { if m == nil || info == nil || info.handshakeValidator == nil || messageHandler == nil { return nil @@ -199,7 +197,6 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, err := info.handshakeValidator(peer, hs) if err != nil { logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) - info.inboundHandshakeData.Delete(peer) return errCannotValidateHandshake } @@ -210,14 +207,12 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, resp, err := info.getHandshake() if err != nil { logger.Warn("failed to get handshake", "protocol", info.protocolID, "error", err) - info.inboundHandshakeData.Delete(peer) return err } err = s.host.writeToStream(stream, resp) if err != nil { logger.Trace("failed to send handshake", "protocol", info.protocolID, "peer", peer, "error", err) - info.inboundHandshakeData.Delete(peer) return err } @@ -240,7 +235,6 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, if batchHandler != nil { msgs, err = batchHandler(peer, msg) if err != nil { - info.inboundHandshakeData.Delete(peer) return err } @@ -248,7 +242,6 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, } else { propagate, err = messageHandler(peer, msg) if err != nil { - info.inboundHandshakeData.Delete(peer) return err } @@ -308,7 +301,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc return } - if !has || !hsData.received /*|| hsData.stream == nil*/ { + if !has || !hsData.received || hsData.stream == nil { if !has { hsData = newHandshakeData(false, false, nil) } else if has && hsData.stream == nil { @@ -320,6 +313,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc if err != nil { logger.Trace("failed to send message to peer", "peer", peer, "error", err) _ = stream.Close() + info.outboundHandshakeData.Delete(peer) return } diff --git a/dot/network/service.go b/dot/network/service.go index 2c9e5a1e3c..5371db093c 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -618,7 +618,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder return } else if err != nil { logger.Trace("failed to read from stream", "id", stream.ID(), "peer", stream.Conn().RemotePeer(), "protocol", stream.Protocol(), "error", err) - _ = stream.Close() + s.closeInboundStream(stream) return } @@ -628,8 +628,8 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) if err != nil { logger.Trace("failed to decode message from peer", "id", stream.ID(), "protocol", stream.Protocol(), "err", err) - _ = stream.Close() - continue + s.closeInboundStream(stream) + return } logger.Trace( @@ -641,7 +641,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder if err = handler(stream, msg); err != nil { logger.Trace("failed to handle message from stream", "id", stream.ID(), "message", msg, "error", err) - _ = stream.Close() + s.closeInboundStream(stream) return } @@ -649,6 +649,23 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder } } +func (s *Service) closeInboundStream(stream libp2pnetwork.Stream) { + protocolID := stream.Protocol() + + s.notificationsMu.Lock() + defer s.notificationsMu.Unlock() + + for _, prtl := range s.notificationsProtocols { + if prtl.protocolID != protocolID { + continue + } + + prtl.inboundHandshakeData.Delete(stream.Conn().RemotePeer()) + } + + _ = stream.Close() +} + func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error { defer func() { _ = stream.Close() From 3eb1b8e2370ce0e5a114ee543fedb6b39fc1dd45 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 19:04:27 -0500 Subject: [PATCH 20/59] treat io.EOF the same as other errs in readStream --- dot/network/notifications.go | 2 +- dot/network/service.go | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 926ea69a13..6abe22293d 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -222,7 +222,7 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, return nil } - logger.Trace("received message on notifications sub-protocol", "protocol", info.protocolID, + logger.Debug("received message on notifications sub-protocol", "protocol", info.protocolID, "message", msg, "peer", stream.Conn().RemotePeer(), ) diff --git a/dot/network/service.go b/dot/network/service.go index 5371db093c..4b2b8af09d 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "io" "math/big" "os" "sync" @@ -614,9 +613,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder for { tot, err := readStream(stream, msgBytes[:]) - if errors.Is(err, io.EOF) { - return - } else if err != nil { + if err != nil { logger.Trace("failed to read from stream", "id", stream.ID(), "peer", stream.Conn().RemotePeer(), "protocol", stream.Protocol(), "error", err) s.closeInboundStream(stream) return From dea40af1541e616285da433f703eea726c8c4260 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 19:08:04 -0500 Subject: [PATCH 21/59] update streamManager to use closeInboundStream func --- dot/network/service.go | 4 ++-- dot/network/stream_manager.go | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/dot/network/service.go b/dot/network/service.go index 4b2b8af09d..31213d3b9c 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -176,11 +176,11 @@ func NewService(cfg *Config) (*Service, error) { telemetryInterval: cfg.telemetryInterval, closeCh: make(chan struct{}), bufPool: bufPool, - streamManager: newStreamManager(ctx), blockResponseBuf: make([]byte, maxBlockResponseSize), batchSize: 100, } - + + network.streamManager = newStreamManager(ctx, network.closeInboundStream) return network, err } diff --git a/dot/network/stream_manager.go b/dot/network/stream_manager.go index 6755f5c3da..b0008cad3b 100644 --- a/dot/network/stream_manager.go +++ b/dot/network/stream_manager.go @@ -10,6 +10,8 @@ import ( var cleanupStreamInterval = time.Minute +type inboundStreamCloseFunc func(stream network.Stream) + type streamData struct { lastReceivedMessage time.Time stream network.Stream @@ -21,12 +23,14 @@ type streamData struct { type streamManager struct { ctx context.Context streamDataMap *sync.Map //map[string]*streamData + closeFunc inboundStreamCloseFunc } -func newStreamManager(ctx context.Context) *streamManager { +func newStreamManager(ctx context.Context, closeFunc inboundStreamCloseFunc) *streamManager { return &streamManager{ ctx: ctx, streamDataMap: new(sync.Map), + closeFunc: closeFunc, } } @@ -53,7 +57,7 @@ func (sm *streamManager) cleanupStreams() { stream := sdata.stream if time.Since(lastReceived) > cleanupStreamInterval { - _ = stream.Close() + sm.closeFunc(stream) sm.streamDataMap.Delete(id) } From d9f7e364aadaf4afafbf3a9cfe0c0553cf28d53e Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 19:36:11 -0500 Subject: [PATCH 22/59] remove gossip module --- dot/network/gossip.go | 49 ------------- dot/network/gossip_test.go | 125 ---------------------------------- dot/network/notifications.go | 5 +- dot/network/service.go | 4 +- dot/network/stream_manager.go | 4 +- 5 files changed, 4 insertions(+), 183 deletions(-) delete mode 100644 dot/network/gossip.go delete mode 100644 dot/network/gossip_test.go diff --git a/dot/network/gossip.go b/dot/network/gossip.go deleted file mode 100644 index 9f306151d4..0000000000 --- a/dot/network/gossip.go +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright 2019 ChainSafe Systems (ON) Corp. -// This file is part of gossamer. -// -// The gossamer library is free software: you can redistribute it and/or modify -// it under the terms of the GNU Lesser General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// The gossamer library is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Lesser General Public License for more detailg. -// -// You should have received a copy of the GNU Lesser General Public License -// along with the gossamer library. If not, see . - -package network - -import ( - "sync" - - log "github.com/ChainSafe/log15" -) - -// gossip submodule -type gossip struct { - logger log.Logger - seen *sync.Map -} - -// newGossip creates a new gossip message tracker -func newGossip() *gossip { - return &gossip{ - logger: logger.New("module", "gossip"), - seen: &sync.Map{}, - } -} - -// hasSeen broadcasts messages that have not been seen -func (g *gossip) hasSeen(msg NotificationsMessage) bool { //nolint - // check if message has not been seen - if seen, ok := g.seen.Load(msg.Hash()); !ok || !seen.(bool) { - // set message to has been seen - g.seen.Store(msg.Hash(), true) - return false - } - - return true -} diff --git a/dot/network/gossip_test.go b/dot/network/gossip_test.go deleted file mode 100644 index 160236acf5..0000000000 --- a/dot/network/gossip_test.go +++ /dev/null @@ -1,125 +0,0 @@ -// Copyright 2019 ChainSafe Systems (ON) Corp. -// This file is part of gossamer. -// -// The gossamer library is free software: you can redistribute it and/or modify -// it under the terms of the GNU Lesser General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// The gossamer library is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Lesser General Public License for more details. -// -// You should have received a copy of the GNU Lesser General Public License -// along with the gossamer library. If not, see . - -package network - -import ( - "testing" - "time" - - "github.com/ChainSafe/gossamer/lib/utils" - - "github.com/stretchr/testify/require" -) - -// test gossip messages to connected peers -func TestGossip(t *testing.T) { - if testing.Short() { - t.Skip("skipping TestGossip; currently, nothing is gossiped") - } - - basePathA := utils.NewTestBasePath(t, "nodeA") - - configA := &Config{ - BasePath: basePathA, - Port: 7001, - NoBootstrap: true, - NoMDNS: true, - } - - nodeA := createTestService(t, configA) - handlerA := newTestStreamHandler(testBlockAnnounceMessageDecoder) - nodeA.host.registerStreamHandler(nodeA.host.protocolID, handlerA.handleStream) - - basePathB := utils.NewTestBasePath(t, "nodeB") - configB := &Config{ - BasePath: basePathB, - Port: 7002, - NoBootstrap: true, - NoMDNS: true, - } - - nodeB := createTestService(t, configB) - handlerB := newTestStreamHandler(testBlockAnnounceMessageDecoder) - nodeB.host.registerStreamHandler(nodeB.host.protocolID, handlerB.handleStream) - - addrInfoA := nodeA.host.addrInfo() - err := nodeB.host.connect(addrInfoA) - // retry connect if "failed to dial" error - if failedToDial(err) { - time.Sleep(TestBackoffTimeout) - err = nodeB.host.connect(addrInfoA) - } - require.NoError(t, err) - - basePathC := utils.NewTestBasePath(t, "nodeC") - configC := &Config{ - BasePath: basePathC, - Port: 7003, - NoBootstrap: true, - NoMDNS: true, - } - - nodeC := createTestService(t, configC) - handlerC := newTestStreamHandler(testBlockAnnounceMessageDecoder) - nodeC.host.registerStreamHandler(nodeC.host.protocolID, handlerC.handleStream) - - err = nodeC.host.connect(addrInfoA) - // retry connect if "failed to dial" error - if failedToDial(err) { - time.Sleep(TestBackoffTimeout) - err = nodeC.host.connect(addrInfoA) - } - require.NoError(t, err) - - addrInfoB := nodeB.host.addrInfo() - err = nodeC.host.connect(addrInfoB) - // retry connect if "failed to dial" error - if failedToDial(err) { - time.Sleep(TestBackoffTimeout) - err = nodeC.host.connect(addrInfoB) - } - require.NoError(t, err) - - _, err = nodeA.host.send(addrInfoB.ID, "", testBlockAnnounceMessage) - require.NoError(t, err) - - time.Sleep(TestMessageTimeout) - - if hasSeenB, ok := nodeB.gossip.seen.Load(testBlockAnnounceMessage.Hash()); !ok || hasSeenB.(bool) == false { - t.Error( - "node B did not receive block request message from node A", - "\nreceived:", hasSeenB, - "\nexpected:", true, - ) - } - - if hasSeenC, ok := nodeC.gossip.seen.Load(testBlockAnnounceMessage.Hash()); !ok || hasSeenC.(bool) == false { - t.Error( - "node C did not receive block request message from node B", - "\nreceived:", hasSeenC, - "\nexpected:", true, - ) - } - - if hasSeenA, ok := nodeA.gossip.seen.Load(testBlockAnnounceMessage.Hash()); !ok || hasSeenA.(bool) == false { - t.Error( - "node A did not receive block request message from node C", - "\nreceived:", hasSeenA, - "\nexpected:", true, - ) - } -} diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 6abe22293d..74a0ac5b26 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -256,10 +256,7 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, } for _, data := range msgs { - seen := s.gossip.hasSeen(data.msg) - if !seen { - s.broadcastExcluding(info, data.peer, data.msg) - } + s.broadcastExcluding(info, data.peer, data.msg) // report peer if we get duplicate gossip message. s.host.cm.peerSetHandler.ReportPeer(peerset.ReputationChange{ diff --git a/dot/network/service.go b/dot/network/service.go index 31213d3b9c..cb63b1e25e 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -75,7 +75,6 @@ type Service struct { cfg *Config host *host mdns *mdns - gossip *gossip bufPool *sizedBufferPool streamManager *streamManager @@ -165,7 +164,6 @@ func NewService(cfg *Config) (*Service, error) { cfg: cfg, host: host, mdns: newMDNS(host), - gossip: newGossip(), blockState: cfg.BlockState, transactionHandler: cfg.TransactionHandler, noBootstrap: cfg.NoBootstrap, @@ -179,7 +177,7 @@ func NewService(cfg *Config) (*Service, error) { blockResponseBuf: make([]byte, maxBlockResponseSize), batchSize: 100, } - + network.streamManager = newStreamManager(ctx, network.closeInboundStream) return network, err } diff --git a/dot/network/stream_manager.go b/dot/network/stream_manager.go index b0008cad3b..f86d5a343e 100644 --- a/dot/network/stream_manager.go +++ b/dot/network/stream_manager.go @@ -23,14 +23,14 @@ type streamData struct { type streamManager struct { ctx context.Context streamDataMap *sync.Map //map[string]*streamData - closeFunc inboundStreamCloseFunc + closeFunc inboundStreamCloseFunc } func newStreamManager(ctx context.Context, closeFunc inboundStreamCloseFunc) *streamManager { return &streamManager{ ctx: ctx, streamDataMap: new(sync.Map), - closeFunc: closeFunc, + closeFunc: closeFunc, } } From f072ef3326c090551c0cabda92bdd0b6314b7e11 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 19:43:54 -0500 Subject: [PATCH 23/59] re-add discovery connect code for now --- dot/network/discovery.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dot/network/discovery.go b/dot/network/discovery.go index d6ff4b0cf8..73af4bcff4 100644 --- a/dot/network/discovery.go +++ b/dot/network/discovery.go @@ -210,8 +210,20 @@ func (d *discovery) findPeers(ctx context.Context) { logger.Trace("found new peer via DHT", "peer", peer.ID) + // TODO: this isn't working on the devnet d.h.Peerstore().AddAddrs(peer.ID, peer.Addrs, peerstore.PermanentAddrTTL) d.handler.AddPeer(0, peer.ID) + + // found a peer, try to connect if we need more peers + if len(d.h.Network().Peers()) < d.maxPeers { + err = d.h.Connect(d.ctx, peer) + if err != nil { + logger.Trace("failed to connect to discovered peer", "peer", peer.ID, "err", err) + } + } else { + d.h.Peerstore().AddAddrs(peer.ID, peer.Addrs, peerstore.PermanentAddrTTL) + return + } } } } From 56362e492a56e48f8efb5fe10f82e9db6cdd9508 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 19:47:40 -0500 Subject: [PATCH 24/59] make sendData synchronous for now --- dot/network/notifications.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 74a0ac5b26..c6428ec97b 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -419,7 +419,7 @@ func (s *Service) broadcastExcluding(info *notificationsProtocol, excluding peer continue } - go s.sendData(peer, hs, info, msg) + /*go*/ s.sendData(peer, hs, info, msg) } } From 5a5ee4b8a2b72de6e1863ef32d58f01320acf3b9 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 19:51:39 -0500 Subject: [PATCH 25/59] maybe improve concurrency in connmgr closeHandler --- dot/network/notifications.go | 3 ++- dot/network/service.go | 10 ++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index c6428ec97b..3bd2114a4f 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -419,7 +419,8 @@ func (s *Service) broadcastExcluding(info *notificationsProtocol, excluding peer continue } - /*go*/ s.sendData(peer, hs, info, msg) + /*go*/ + s.sendData(peer, hs, info, msg) } } diff --git a/dot/network/service.go b/dot/network/service.go index cb63b1e25e..bbae1096d0 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -480,22 +480,28 @@ func (s *Service) RegisterNotificationsProtocol( connMgr := s.host.h.ConnManager().(*ConnManager) connMgr.registerCloseHandler(protocolID, func(peerID peer.ID) { - if _, ok := np.getInboundHandshakeData(peerID); ok { + if hsData, ok := np.getInboundHandshakeData(peerID); ok { logger.Trace( "Cleaning up inbound handshake data", "peer", peerID, "protocol", protocolID, ) + + hsData.Lock() np.inboundHandshakeData.Delete(peerID) + hsData.Unlock() } - if _, ok := np.getOutboundHandshakeData(peerID); ok { + if hsData, ok := np.getOutboundHandshakeData(peerID); ok { logger.Trace( "Cleaning up outbound handshake data", "peer", peerID, "protocol", protocolID, ) + + hsData.Lock() np.outboundHandshakeData.Delete(peerID) + hsData.Unlock() } }) From 9584b4e9b3184e14d5c197806e79016093a17489 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 19:55:08 -0500 Subject: [PATCH 26/59] remove cache-ignore of consensus messages --- dot/network/notifications.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 3bd2114a4f..d2a5571569 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -369,8 +369,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // only when they are for already finalised rounds; currently this causes issues // because a vote might be received slightly too early, causing a round mismatch err, // causing grandpa to discard the vote. (#1855) - _, isConsensusMsg := msg.(*ConsensusMessage) - if !added && !isConsensusMsg { + //_, isConsensusMsg := msg.(*ConsensusMessage) + if !added /*&& !isConsensusMsg*/ { return } } From c2fa05e94ca1a67138646bf44029c2679754da54 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 20:08:18 -0500 Subject: [PATCH 27/59] embed mutex in handshakeData --- dot/network/notifications.go | 11 ++++++----- lib/grandpa/vote_message.go | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index d2a5571569..2d62be844b 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -121,7 +121,7 @@ type handshakeData struct { validated bool handshake Handshake stream libp2pnetwork.Stream - *sync.Mutex + sync.Mutex } func newHandshakeData(received, validated bool, stream libp2pnetwork.Stream) handshakeData { @@ -129,7 +129,6 @@ func newHandshakeData(received, validated bool, stream libp2pnetwork.Stream) han received: received, validated: validated, stream: stream, - Mutex: new(sync.Mutex), } } @@ -279,6 +278,10 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc return } + if info == nil { + panic("info is nil!!!!") + } + if info.handshakeValidator == nil { logger.Warn("handshakeValidator is not set for protocol!", "protocol", info.protocolID) return @@ -299,9 +302,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc } if !has || !hsData.received || hsData.stream == nil { - if !has { - hsData = newHandshakeData(false, false, nil) - } else if has && hsData.stream == nil { + if has && hsData.stream == nil { panic(fmt.Sprintf("stream is nil, but it shouldn't be!! %s", info.protocolID)) } diff --git a/lib/grandpa/vote_message.go b/lib/grandpa/vote_message.go index 4ddef6f047..7523f392d9 100644 --- a/lib/grandpa/vote_message.go +++ b/lib/grandpa/vote_message.go @@ -38,14 +38,14 @@ func (s *Service) receiveMessages(ctx context.Context) { for { select { case msg, ok := <-s.in: - if msg == nil || msg.msg == nil { - continue - } - if !ok { return } + if msg == nil || msg.msg == nil { + continue + } + logger.Trace("received vote message", "msg", msg.msg, "from", msg.from) vm := msg.msg From d25196bfa9fb3a6802ec75b5085b6f4bbcd24cd9 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 20:10:49 -0500 Subject: [PATCH 28/59] make sendData async again --- dot/network/notifications.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 2d62be844b..d62d7f53bf 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -420,8 +420,7 @@ func (s *Service) broadcastExcluding(info *notificationsProtocol, excluding peer continue } - /*go*/ - s.sendData(peer, hs, info, msg) + go s.sendData(peer, hs, info, msg) } } From 0319217a4d2615ff8a999900c98d3c5c3da840be Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 20:23:08 -0500 Subject: [PATCH 29/59] maybe fix locking? --- dot/network/service.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/dot/network/service.go b/dot/network/service.go index bbae1096d0..60c54fff36 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -480,28 +480,28 @@ func (s *Service) RegisterNotificationsProtocol( connMgr := s.host.h.ConnManager().(*ConnManager) connMgr.registerCloseHandler(protocolID, func(peerID peer.ID) { - if hsData, ok := np.getInboundHandshakeData(peerID); ok { + if _, ok := np.getInboundHandshakeData(peerID); ok { logger.Trace( "Cleaning up inbound handshake data", "peer", peerID, "protocol", protocolID, ) - hsData.Lock() + //hsData.Lock() np.inboundHandshakeData.Delete(peerID) - hsData.Unlock() + //hsData.Unlock() } - if hsData, ok := np.getOutboundHandshakeData(peerID); ok { + if _, ok := np.getOutboundHandshakeData(peerID); ok { logger.Trace( "Cleaning up outbound handshake data", "peer", peerID, "protocol", protocolID, ) - hsData.Lock() + //hsData.Lock() np.outboundHandshakeData.Delete(peerID) - hsData.Unlock() + //hsData.Unlock() } }) From 5a66fff02effe8580cd39863173a4cfd395ad0f9 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 20:29:10 -0500 Subject: [PATCH 30/59] change messageCache TTL to 5s --- dot/network/message_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/network/message_cache.go b/dot/network/message_cache.go index 53565001a1..5c1ef01f6b 100644 --- a/dot/network/message_cache.go +++ b/dot/network/message_cache.go @@ -10,7 +10,7 @@ import ( ) // msgCacheTTL is default duration a key-value will be stored in messageCache. -var msgCacheTTL = 5 * time.Minute +var msgCacheTTL = 5 * time.Second // messageCache is used to detect duplicated messages per peer. type messageCache struct { From 5ec3aa03d8ef2fdd7e679911ca19edb4d8965a1a Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 20:32:54 -0500 Subject: [PATCH 31/59] make hsData lock ptr again --- dot/network/notifications.go | 11 ++++++----- dot/network/stream_manager_test.go | 6 ++++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index d62d7f53bf..4028f109a0 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -117,11 +117,11 @@ func (n *notificationsProtocol) getOutboundHandshakeData(pid peer.ID) (handshake } type handshakeData struct { - received bool - validated bool - handshake Handshake - stream libp2pnetwork.Stream - sync.Mutex + received bool + validated bool + handshake Handshake + stream libp2pnetwork.Stream + *sync.Mutex // this needs to be a pointer, otherwise a new mutex will be created every time hsData is stored/loaded } func newHandshakeData(received, validated bool, stream libp2pnetwork.Stream) handshakeData { @@ -129,6 +129,7 @@ func newHandshakeData(received, validated bool, stream libp2pnetwork.Stream) han received: received, validated: validated, stream: stream, + Mutex: new(sync.Mutex), } } diff --git a/dot/network/stream_manager_test.go b/dot/network/stream_manager_test.go index 92fd6e60ed..f4143acc0d 100644 --- a/dot/network/stream_manager_test.go +++ b/dot/network/stream_manager_test.go @@ -15,6 +15,8 @@ import ( "github.com/stretchr/testify/require" ) +func testCloseFunc(stream network.Stream) {} + func setupStreamManagerTest(t *testing.T) (context.Context, []libp2phost.Host, []*streamManager) { ctx, cancel := context.WithCancel(context.Background()) @@ -24,8 +26,8 @@ func setupStreamManagerTest(t *testing.T) (context.Context, []libp2phost.Host, [ cancel() }) - smA := newStreamManager(ctx) - smB := newStreamManager(ctx) + smA := newStreamManager(ctx, testCloseFunc) + smB := newStreamManager(ctx, testCloseFunc) portA := 7001 portB := 7002 From e30a39972fb33594014cc363708e2d43015a1b56 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 20:49:07 -0500 Subject: [PATCH 32/59] update network service readStream to always assume inbound --- dot/network/notifications.go | 12 ++++++------ dot/network/service.go | 12 +----------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 4028f109a0..8d0b5cf064 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -19,14 +19,14 @@ package network import ( "errors" "fmt" - "io" + //"io" "reflect" "sync" "time" "github.com/ChainSafe/gossamer/dot/peerset" - "github.com/libp2p/go-libp2p-core/mux" + //"github.com/libp2p/go-libp2p-core/mux" libp2pnetwork "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" "github.com/libp2p/go-libp2p-core/protocol" @@ -385,10 +385,10 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc logger.Debug("failed to send message to peer", "protocol", info.protocolID, "peer", peer, "error", err) // the stream was closed or reset, close it on our end and delete it from our peer's data - if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { - _ = hsData.stream.Close() - info.outboundHandshakeData.Delete(peer) - } + //if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { + _ = hsData.stream.Close() + info.outboundHandshakeData.Delete(peer) + //} return } diff --git a/dot/network/service.go b/dot/network/service.go index 60c54fff36..ecd6482137 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -487,9 +487,7 @@ func (s *Service) RegisterNotificationsProtocol( "protocol", protocolID, ) - //hsData.Lock() np.inboundHandshakeData.Delete(peerID) - //hsData.Unlock() } if _, ok := np.getOutboundHandshakeData(peerID); ok { @@ -499,9 +497,7 @@ func (s *Service) RegisterNotificationsProtocol( "protocol", protocolID, ) - //hsData.Lock() np.outboundHandshakeData.Delete(peerID) - //hsData.Unlock() } }) @@ -512,12 +508,6 @@ func (s *Service) RegisterNotificationsProtocol( s.host.registerStreamHandler(protocolID, func(stream libp2pnetwork.Stream) { logger.Trace("received stream", "sub-protocol", protocolID) - conn := stream.Conn() - if conn == nil { - logger.Error("Failed to get connection from stream") - return - } - s.readStream(stream, decoder, handlerWithValidate) }) @@ -626,7 +616,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder s.streamManager.logMessageReceived(stream.ID()) // decode message based on message type - msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) + msg, err := decoder(msgBytes[:tot], peer, true) // stream is always inbound if it passes through service.readStream if err != nil { logger.Trace("failed to decode message from peer", "id", stream.ID(), "protocol", stream.Protocol(), "err", err) s.closeInboundStream(stream) From 02c15ac5800bef7b6ab430e725893cd67405a28f Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 21:07:46 -0500 Subject: [PATCH 33/59] re-add gossip logic for inbound msg re-gossiping --- dot/network/gossip.go | 49 ++++++++++++++ dot/network/gossip_test.go | 125 +++++++++++++++++++++++++++++++++++ dot/network/notifications.go | 13 ++-- dot/network/service.go | 2 + 4 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 dot/network/gossip.go create mode 100644 dot/network/gossip_test.go diff --git a/dot/network/gossip.go b/dot/network/gossip.go new file mode 100644 index 0000000000..9f306151d4 --- /dev/null +++ b/dot/network/gossip.go @@ -0,0 +1,49 @@ +// Copyright 2019 ChainSafe Systems (ON) Corp. +// This file is part of gossamer. +// +// The gossamer library is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// The gossamer library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more detailg. +// +// You should have received a copy of the GNU Lesser General Public License +// along with the gossamer library. If not, see . + +package network + +import ( + "sync" + + log "github.com/ChainSafe/log15" +) + +// gossip submodule +type gossip struct { + logger log.Logger + seen *sync.Map +} + +// newGossip creates a new gossip message tracker +func newGossip() *gossip { + return &gossip{ + logger: logger.New("module", "gossip"), + seen: &sync.Map{}, + } +} + +// hasSeen broadcasts messages that have not been seen +func (g *gossip) hasSeen(msg NotificationsMessage) bool { //nolint + // check if message has not been seen + if seen, ok := g.seen.Load(msg.Hash()); !ok || !seen.(bool) { + // set message to has been seen + g.seen.Store(msg.Hash(), true) + return false + } + + return true +} diff --git a/dot/network/gossip_test.go b/dot/network/gossip_test.go new file mode 100644 index 0000000000..160236acf5 --- /dev/null +++ b/dot/network/gossip_test.go @@ -0,0 +1,125 @@ +// Copyright 2019 ChainSafe Systems (ON) Corp. +// This file is part of gossamer. +// +// The gossamer library is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// The gossamer library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with the gossamer library. If not, see . + +package network + +import ( + "testing" + "time" + + "github.com/ChainSafe/gossamer/lib/utils" + + "github.com/stretchr/testify/require" +) + +// test gossip messages to connected peers +func TestGossip(t *testing.T) { + if testing.Short() { + t.Skip("skipping TestGossip; currently, nothing is gossiped") + } + + basePathA := utils.NewTestBasePath(t, "nodeA") + + configA := &Config{ + BasePath: basePathA, + Port: 7001, + NoBootstrap: true, + NoMDNS: true, + } + + nodeA := createTestService(t, configA) + handlerA := newTestStreamHandler(testBlockAnnounceMessageDecoder) + nodeA.host.registerStreamHandler(nodeA.host.protocolID, handlerA.handleStream) + + basePathB := utils.NewTestBasePath(t, "nodeB") + configB := &Config{ + BasePath: basePathB, + Port: 7002, + NoBootstrap: true, + NoMDNS: true, + } + + nodeB := createTestService(t, configB) + handlerB := newTestStreamHandler(testBlockAnnounceMessageDecoder) + nodeB.host.registerStreamHandler(nodeB.host.protocolID, handlerB.handleStream) + + addrInfoA := nodeA.host.addrInfo() + err := nodeB.host.connect(addrInfoA) + // retry connect if "failed to dial" error + if failedToDial(err) { + time.Sleep(TestBackoffTimeout) + err = nodeB.host.connect(addrInfoA) + } + require.NoError(t, err) + + basePathC := utils.NewTestBasePath(t, "nodeC") + configC := &Config{ + BasePath: basePathC, + Port: 7003, + NoBootstrap: true, + NoMDNS: true, + } + + nodeC := createTestService(t, configC) + handlerC := newTestStreamHandler(testBlockAnnounceMessageDecoder) + nodeC.host.registerStreamHandler(nodeC.host.protocolID, handlerC.handleStream) + + err = nodeC.host.connect(addrInfoA) + // retry connect if "failed to dial" error + if failedToDial(err) { + time.Sleep(TestBackoffTimeout) + err = nodeC.host.connect(addrInfoA) + } + require.NoError(t, err) + + addrInfoB := nodeB.host.addrInfo() + err = nodeC.host.connect(addrInfoB) + // retry connect if "failed to dial" error + if failedToDial(err) { + time.Sleep(TestBackoffTimeout) + err = nodeC.host.connect(addrInfoB) + } + require.NoError(t, err) + + _, err = nodeA.host.send(addrInfoB.ID, "", testBlockAnnounceMessage) + require.NoError(t, err) + + time.Sleep(TestMessageTimeout) + + if hasSeenB, ok := nodeB.gossip.seen.Load(testBlockAnnounceMessage.Hash()); !ok || hasSeenB.(bool) == false { + t.Error( + "node B did not receive block request message from node A", + "\nreceived:", hasSeenB, + "\nexpected:", true, + ) + } + + if hasSeenC, ok := nodeC.gossip.seen.Load(testBlockAnnounceMessage.Hash()); !ok || hasSeenC.(bool) == false { + t.Error( + "node C did not receive block request message from node B", + "\nreceived:", hasSeenC, + "\nexpected:", true, + ) + } + + if hasSeenA, ok := nodeA.gossip.seen.Load(testBlockAnnounceMessage.Hash()); !ok || hasSeenA.(bool) == false { + t.Error( + "node A did not receive block request message from node C", + "\nreceived:", hasSeenA, + "\nexpected:", true, + ) + } +} diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 8d0b5cf064..80d882e6b9 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -256,7 +256,10 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, } for _, data := range msgs { - s.broadcastExcluding(info, data.peer, data.msg) + seen := s.gossip.hasSeen(data.msg) + if !seen { + s.broadcastExcluding(info, data.peer, data.msg) + } // report peer if we get duplicate gossip message. s.host.cm.peerSetHandler.ReportPeer(peerset.ReputationChange{ @@ -311,8 +314,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc stream, err := s.host.send(peer, info.protocolID, hs) if err != nil { logger.Trace("failed to send message to peer", "peer", peer, "error", err) - _ = stream.Close() - info.outboundHandshakeData.Delete(peer) + //_ = stream.Close() + //info.outboundHandshakeData.Delete(peer) return } @@ -337,8 +340,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc hsTimer.Stop() if hsResponse.err != nil { logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) - _ = stream.Close() - info.outboundHandshakeData.Delete(peer) + //_ = stream.Close() + //info.outboundHandshakeData.Delete(peer) return } diff --git a/dot/network/service.go b/dot/network/service.go index ecd6482137..942182857a 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -75,6 +75,7 @@ type Service struct { cfg *Config host *host mdns *mdns + gossip *gossip bufPool *sizedBufferPool streamManager *streamManager @@ -164,6 +165,7 @@ func NewService(cfg *Config) (*Service, error) { cfg: cfg, host: host, mdns: newMDNS(host), + gossip: newGossip(), blockState: cfg.BlockState, transactionHandler: cfg.TransactionHandler, noBootstrap: cfg.NoBootstrap, From 83beda8d09d09d7e1798eb4a720cd678511cb0b3 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 21:15:24 -0500 Subject: [PATCH 34/59] reset streams instead of closing --- dot/network/notifications.go | 10 +++++----- dot/network/service.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 80d882e6b9..3fa4b13b98 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -314,7 +314,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc stream, err := s.host.send(peer, info.protocolID, hs) if err != nil { logger.Trace("failed to send message to peer", "peer", peer, "error", err) - //_ = stream.Close() + //_ = stream.Reset() //info.outboundHandshakeData.Delete(peer) return } @@ -333,14 +333,14 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc }, peer) logger.Trace("handshake timeout reached", "protocol", info.protocolID, "peer", peer) - _ = stream.Close() + _ = stream.Reset() info.outboundHandshakeData.Delete(peer) return case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): hsTimer.Stop() if hsResponse.err != nil { logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) - //_ = stream.Close() + //_ = stream.Reset() //info.outboundHandshakeData.Delete(peer) return } @@ -353,7 +353,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) hsData.validated = false hsData.stream = nil - _ = stream.Close() + _ = stream.Reset() info.outboundHandshakeData.Store(peer, hsData) return } @@ -389,7 +389,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // the stream was closed or reset, close it on our end and delete it from our peer's data //if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { - _ = hsData.stream.Close() + _ = hsData.stream.Reset() info.outboundHandshakeData.Delete(peer) //} diff --git a/dot/network/service.go b/dot/network/service.go index 942182857a..90c3d79c87 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -656,7 +656,7 @@ func (s *Service) closeInboundStream(stream libp2pnetwork.Stream) { prtl.inboundHandshakeData.Delete(stream.Conn().RemotePeer()) } - _ = stream.Close() + _ = stream.Reset() } func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error { From 958c0c1659476225f40e9c99c14cf201b8e54f0a Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 21:26:51 -0500 Subject: [PATCH 35/59] comment out cleanupStreams --- dot/network/notifications.go | 12 ++++++------ dot/network/stream_manager.go | 2 ++ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 3fa4b13b98..3a1d31d05a 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -340,8 +340,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc hsTimer.Stop() if hsResponse.err != nil { logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) - //_ = stream.Reset() - //info.outboundHandshakeData.Delete(peer) + _ = stream.Reset() + info.outboundHandshakeData.Delete(peer) return } @@ -388,10 +388,10 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc logger.Debug("failed to send message to peer", "protocol", info.protocolID, "peer", peer, "error", err) // the stream was closed or reset, close it on our end and delete it from our peer's data - //if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { - _ = hsData.stream.Reset() - info.outboundHandshakeData.Delete(peer) - //} + if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { + _ = hsData.stream.Reset() + info.outboundHandshakeData.Delete(peer) + } return } diff --git a/dot/network/stream_manager.go b/dot/network/stream_manager.go index f86d5a343e..451a1fc221 100644 --- a/dot/network/stream_manager.go +++ b/dot/network/stream_manager.go @@ -51,6 +51,8 @@ func (sm *streamManager) start() { } func (sm *streamManager) cleanupStreams() { + return + sm.streamDataMap.Range(func(id, data interface{}) bool { sdata := data.(*streamData) lastReceived := sdata.lastReceivedMessage From 7c81966a7cc52b3790c9328aac16e48e024cf78d Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 21:27:41 -0500 Subject: [PATCH 36/59] fix --- dot/network/notifications.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 3a1d31d05a..4c44e13ffa 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -19,14 +19,14 @@ package network import ( "errors" "fmt" - //"io" + "io" "reflect" "sync" "time" "github.com/ChainSafe/gossamer/dot/peerset" - //"github.com/libp2p/go-libp2p-core/mux" + "github.com/libp2p/go-libp2p-core/mux" libp2pnetwork "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" "github.com/libp2p/go-libp2p-core/protocol" From 4a8732ed8b04b08fc7c01563b461108bc024e5b7 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 22:47:59 -0500 Subject: [PATCH 37/59] fix stream close handler bug, was deleting both hsDatas instead of 1 --- dot/network/connmgr.go | 10 ++++++---- dot/network/message_cache.go | 2 +- dot/network/notifications.go | 17 ++++------------- dot/network/service.go | 23 +++++++++++++---------- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/dot/network/connmgr.go b/dot/network/connmgr.go index d4b747b525..ad7b04c77e 100644 --- a/dot/network/connmgr.go +++ b/dot/network/connmgr.go @@ -31,6 +31,8 @@ import ( "github.com/ChainSafe/gossamer/dot/peerset" ) +type streamCloseHandler func(peerID peer.ID, inbound bool) + // ConnManager implements connmgr.ConnManager type ConnManager struct { sync.Mutex @@ -39,7 +41,7 @@ type ConnManager struct { disconnectHandler func(peer.ID) // closeHandlerMap contains close handler corresponding to a protocol. - closeHandlerMap map[protocol.ID]func(peerID peer.ID) + closeHandlerMap map[protocol.ID]streamCloseHandler // protectedPeers contains a list of peers that are protected from pruning // when we reach the maximum numbers of peers. @@ -60,7 +62,7 @@ func newConnManager(min, max int, peerSetCfg *peerset.ConfigSet) (*ConnManager, return &ConnManager{ min: min, max: max, - closeHandlerMap: make(map[protocol.ID]func(peerID peer.ID)), + closeHandlerMap: make(map[protocol.ID]streamCloseHandler), protectedPeers: new(sync.Map), persistentPeers: new(sync.Map), peerSetHandler: psh, @@ -210,7 +212,7 @@ func (cm *ConnManager) OpenedStream(_ network.Network, s network.Stream) { ) } -func (cm *ConnManager) registerCloseHandler(protocolID protocol.ID, cb func(id peer.ID)) { +func (cm *ConnManager) registerCloseHandler(protocolID protocol.ID, cb streamCloseHandler) { cm.closeHandlerMap[protocolID] = cb } @@ -225,7 +227,7 @@ func (cm *ConnManager) ClosedStream(_ network.Network, s network.Stream) { cm.Lock() defer cm.Unlock() if closeCB, ok := cm.closeHandlerMap[s.Protocol()]; ok { - closeCB(s.Conn().RemotePeer()) + closeCB(s.Conn().RemotePeer(), isInbound(s)) } } diff --git a/dot/network/message_cache.go b/dot/network/message_cache.go index 5c1ef01f6b..53565001a1 100644 --- a/dot/network/message_cache.go +++ b/dot/network/message_cache.go @@ -10,7 +10,7 @@ import ( ) // msgCacheTTL is default duration a key-value will be stored in messageCache. -var msgCacheTTL = 5 * time.Second +var msgCacheTTL = 5 * time.Minute // messageCache is used to detect duplicated messages per peer. type messageCache struct { diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 4c44e13ffa..aa7557090a 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -18,7 +18,6 @@ package network import ( "errors" - "fmt" "io" "reflect" "sync" @@ -282,10 +281,6 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc return } - if info == nil { - panic("info is nil!!!!") - } - if info.handshakeValidator == nil { logger.Warn("handshakeValidator is not set for protocol!", "protocol", info.protocolID) return @@ -296,9 +291,6 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc hsData = newHandshakeData(false, false, nil) } - hsData.Lock() - defer hsData.Unlock() - if has && hsData.received && !hsData.validated { // peer has sent us an invalid handshake in the past, ignore logger.Warn("peer sent us invalid handshake before, ignoring...", "peer", peer, "protocol", info.protocolID) @@ -306,9 +298,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc } if !has || !hsData.received || hsData.stream == nil { - if has && hsData.stream == nil { - panic(fmt.Sprintf("stream is nil, but it shouldn't be!! %s", info.protocolID)) - } + hsData.Lock() + defer hsData.Unlock() logger.Trace("sending outbound handshake", "protocol", info.protocolID, "peer", peer, "message", hs) stream, err := s.host.send(peer, info.protocolID, hs) @@ -374,8 +365,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // only when they are for already finalised rounds; currently this causes issues // because a vote might be received slightly too early, causing a round mismatch err, // causing grandpa to discard the vote. (#1855) - //_, isConsensusMsg := msg.(*ConsensusMessage) - if !added /*&& !isConsensusMsg*/ { + _, isConsensusMsg := msg.(*ConsensusMessage) + if !added && !isConsensusMsg { return } } diff --git a/dot/network/service.go b/dot/network/service.go index 90c3d79c87..2ed701975c 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -481,16 +481,19 @@ func (s *Service) RegisterNotificationsProtocol( s.notificationsProtocols[messageID] = np connMgr := s.host.h.ConnManager().(*ConnManager) - connMgr.registerCloseHandler(protocolID, func(peerID peer.ID) { - if _, ok := np.getInboundHandshakeData(peerID); ok { - logger.Trace( - "Cleaning up inbound handshake data", - "peer", peerID, - "protocol", protocolID, - ) - - np.inboundHandshakeData.Delete(peerID) - } + connMgr.registerCloseHandler(protocolID, func(peerID peer.ID, inbound bool) { + if inbound { + if _, ok := np.getInboundHandshakeData(peerID); ok { + logger.Trace( + "Cleaning up inbound handshake data", + "peer", peerID, + "protocol", protocolID, + ) + + np.inboundHandshakeData.Delete(peerID) + } + return + } if _, ok := np.getOutboundHandshakeData(peerID); ok { logger.Trace( From dfc5e2ce0eeef901c00a150114bdddbe483b3ec9 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 22:57:27 -0500 Subject: [PATCH 38/59] cleanup --- dot/network/notifications.go | 9 +++----- dot/network/service.go | 33 ++++++------------------------ dot/network/stream_manager.go | 10 ++------- dot/network/stream_manager_test.go | 6 ++---- dot/network/utils.go | 4 ++++ 5 files changed, 17 insertions(+), 45 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index aa7557090a..dd41dcc978 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -282,7 +282,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc } if info.handshakeValidator == nil { - logger.Warn("handshakeValidator is not set for protocol!", "protocol", info.protocolID) + logger.Error("handshakeValidator is not set for protocol!", "protocol", info.protocolID) return } @@ -305,8 +305,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc stream, err := s.host.send(peer, info.protocolID, hs) if err != nil { logger.Trace("failed to send message to peer", "peer", peer, "error", err) - //_ = stream.Reset() - //info.outboundHandshakeData.Delete(peer) + _ = stream.Reset() return } @@ -325,14 +324,12 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc logger.Trace("handshake timeout reached", "protocol", info.protocolID, "peer", peer) _ = stream.Reset() - info.outboundHandshakeData.Delete(peer) return case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): hsTimer.Stop() if hsResponse.err != nil { logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) _ = stream.Reset() - info.outboundHandshakeData.Delete(peer) return } @@ -381,7 +378,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // the stream was closed or reset, close it on our end and delete it from our peer's data if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { _ = hsData.stream.Reset() - info.outboundHandshakeData.Delete(peer) + //info.outboundHandshakeData.Delete(peer) } return diff --git a/dot/network/service.go b/dot/network/service.go index 2ed701975c..51cee9bc5c 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -180,7 +180,7 @@ func NewService(cfg *Config) (*Service, error) { batchSize: 100, } - network.streamManager = newStreamManager(ctx, network.closeInboundStream) + network.streamManager = newStreamManager(ctx) return network, err } @@ -493,7 +493,7 @@ func (s *Service) RegisterNotificationsProtocol( np.inboundHandshakeData.Delete(peerID) } return - } + } if _, ok := np.getOutboundHandshakeData(peerID); ok { logger.Trace( @@ -599,10 +599,6 @@ func (s *Service) decodeLightMessage(in []byte, peer peer.ID, _ bool) (Message, return msg, err } -func isInbound(stream libp2pnetwork.Stream) bool { - return stream.Stat().Direction == libp2pnetwork.DirInbound -} - func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder, handler messageHandler) { s.streamManager.logNewStream(stream) @@ -614,17 +610,17 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder tot, err := readStream(stream, msgBytes[:]) if err != nil { logger.Trace("failed to read from stream", "id", stream.ID(), "peer", stream.Conn().RemotePeer(), "protocol", stream.Protocol(), "error", err) - s.closeInboundStream(stream) + _ = stream.Reset() return } s.streamManager.logMessageReceived(stream.ID()) // decode message based on message type - msg, err := decoder(msgBytes[:tot], peer, true) // stream is always inbound if it passes through service.readStream + msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) // stream shoukd always be inbound if it passes through service.readStream if err != nil { logger.Trace("failed to decode message from peer", "id", stream.ID(), "protocol", stream.Protocol(), "err", err) - s.closeInboundStream(stream) + _ = stream.Reset() return } @@ -637,7 +633,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder if err = handler(stream, msg); err != nil { logger.Trace("failed to handle message from stream", "id", stream.ID(), "message", msg, "error", err) - s.closeInboundStream(stream) + _ = stream.Reset() return } @@ -645,23 +641,6 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder } } -func (s *Service) closeInboundStream(stream libp2pnetwork.Stream) { - protocolID := stream.Protocol() - - s.notificationsMu.Lock() - defer s.notificationsMu.Unlock() - - for _, prtl := range s.notificationsProtocols { - if prtl.protocolID != protocolID { - continue - } - - prtl.inboundHandshakeData.Delete(stream.Conn().RemotePeer()) - } - - _ = stream.Reset() -} - func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error { defer func() { _ = stream.Close() diff --git a/dot/network/stream_manager.go b/dot/network/stream_manager.go index 451a1fc221..d55a127ceb 100644 --- a/dot/network/stream_manager.go +++ b/dot/network/stream_manager.go @@ -10,8 +10,6 @@ import ( var cleanupStreamInterval = time.Minute -type inboundStreamCloseFunc func(stream network.Stream) - type streamData struct { lastReceivedMessage time.Time stream network.Stream @@ -23,14 +21,12 @@ type streamData struct { type streamManager struct { ctx context.Context streamDataMap *sync.Map //map[string]*streamData - closeFunc inboundStreamCloseFunc } -func newStreamManager(ctx context.Context, closeFunc inboundStreamCloseFunc) *streamManager { +func newStreamManager(ctx context.Context) *streamManager { return &streamManager{ ctx: ctx, streamDataMap: new(sync.Map), - closeFunc: closeFunc, } } @@ -51,15 +47,13 @@ func (sm *streamManager) start() { } func (sm *streamManager) cleanupStreams() { - return - sm.streamDataMap.Range(func(id, data interface{}) bool { sdata := data.(*streamData) lastReceived := sdata.lastReceivedMessage stream := sdata.stream if time.Since(lastReceived) > cleanupStreamInterval { - sm.closeFunc(stream) + _ = stream.Reset() sm.streamDataMap.Delete(id) } diff --git a/dot/network/stream_manager_test.go b/dot/network/stream_manager_test.go index f4143acc0d..92fd6e60ed 100644 --- a/dot/network/stream_manager_test.go +++ b/dot/network/stream_manager_test.go @@ -15,8 +15,6 @@ import ( "github.com/stretchr/testify/require" ) -func testCloseFunc(stream network.Stream) {} - func setupStreamManagerTest(t *testing.T) (context.Context, []libp2phost.Host, []*streamManager) { ctx, cancel := context.WithCancel(context.Background()) @@ -26,8 +24,8 @@ func setupStreamManagerTest(t *testing.T) (context.Context, []libp2phost.Host, [ cancel() }) - smA := newStreamManager(ctx, testCloseFunc) - smB := newStreamManager(ctx, testCloseFunc) + smA := newStreamManager(ctx) + smB := newStreamManager(ctx) portA := 7001 portB := 7002 diff --git a/dot/network/utils.go b/dot/network/utils.go index 0636a27681..41117b9d6d 100644 --- a/dot/network/utils.go +++ b/dot/network/utils.go @@ -34,6 +34,10 @@ import ( "github.com/multiformats/go-multiaddr" ) +func isInbound(stream libp2pnetwork.Stream) bool { + return stream.Stat().Direction == libp2pnetwork.DirInbound +} + // stringToAddrInfos converts a single string peer id to AddrInfo func stringToAddrInfo(s string) (peer.AddrInfo, error) { maddr, err := multiaddr.NewMultiaddr(s) From a7de1d170a0515aceb4c08116dee0c3ce2ad1f1c Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 10 Nov 2021 23:18:08 -0500 Subject: [PATCH 39/59] move lock in sendData --- dot/network/notifications.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index dd41dcc978..0c50ad7a3b 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -291,16 +291,16 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc hsData = newHandshakeData(false, false, nil) } + hsData.Lock() + defer hsData.Unlock() + if has && hsData.received && !hsData.validated { // peer has sent us an invalid handshake in the past, ignore logger.Warn("peer sent us invalid handshake before, ignoring...", "peer", peer, "protocol", info.protocolID) return } - if !has || !hsData.received || hsData.stream == nil { - hsData.Lock() - defer hsData.Unlock() - + if !has || !hsData.received { logger.Trace("sending outbound handshake", "protocol", info.protocolID, "peer", peer, "message", hs) stream, err := s.host.send(peer, info.protocolID, hs) if err != nil { From 0fd02303d2a1f443fd7d078bee0b4a2a140ec249 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 11:25:54 -0500 Subject: [PATCH 40/59] change some stuff back --- dot/network/notifications.go | 12 ++++++------ dot/network/service.go | 9 +++++---- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 0c50ad7a3b..0fd8564ed5 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -294,7 +294,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc hsData.Lock() defer hsData.Unlock() - if has && hsData.received && !hsData.validated { + if has /*&& hsData.received*/ && !hsData.validated { // peer has sent us an invalid handshake in the past, ignore logger.Warn("peer sent us invalid handshake before, ignoring...", "peer", peer, "protocol", info.protocolID) return @@ -305,7 +305,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc stream, err := s.host.send(peer, info.protocolID, hs) if err != nil { logger.Trace("failed to send message to peer", "peer", peer, "error", err) - _ = stream.Reset() + //_ = stream.Reset() return } @@ -323,13 +323,13 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc }, peer) logger.Trace("handshake timeout reached", "protocol", info.protocolID, "peer", peer) - _ = stream.Reset() + _ = stream.Close() return case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): hsTimer.Stop() if hsResponse.err != nil { logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) - _ = stream.Reset() + _ = stream.Close() return } @@ -340,8 +340,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc if err = info.handshakeValidator(peer, hs); err != nil { logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) hsData.validated = false - hsData.stream = nil - _ = stream.Reset() + //hsData.stream = nil + //_ = stream.Reset() info.outboundHandshakeData.Store(peer, hsData) return } diff --git a/dot/network/service.go b/dot/network/service.go index 51cee9bc5c..ff470481bf 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -610,7 +610,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder tot, err := readStream(stream, msgBytes[:]) if err != nil { logger.Trace("failed to read from stream", "id", stream.ID(), "peer", stream.Conn().RemotePeer(), "protocol", stream.Protocol(), "error", err) - _ = stream.Reset() + _ = stream.Close() return } @@ -620,8 +620,9 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) // stream shoukd always be inbound if it passes through service.readStream if err != nil { logger.Trace("failed to decode message from peer", "id", stream.ID(), "protocol", stream.Protocol(), "err", err) - _ = stream.Reset() - return + //_ = stream.Reset() + //return + continue } logger.Trace( @@ -633,7 +634,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder if err = handler(stream, msg); err != nil { logger.Trace("failed to handle message from stream", "id", stream.ID(), "message", msg, "error", err) - _ = stream.Reset() + _ = stream.Close() return } From 912a78bc11fcde29700f9b59f3d127d79c6159e2 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 11:59:19 -0500 Subject: [PATCH 41/59] change some more stuff back --- dot/network/notifications.go | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 0fd8564ed5..bcdb4d32d8 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -287,20 +287,32 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc } hsData, has := info.getOutboundHandshakeData(peer) - if !has { - hsData = newHandshakeData(false, false, nil) - } - - hsData.Lock() - defer hsData.Unlock() - - if has /*&& hsData.received*/ && !hsData.validated { + if has && !hsData.validated { // peer has sent us an invalid handshake in the past, ignore - logger.Warn("peer sent us invalid handshake before, ignoring...", "peer", peer, "protocol", info.protocolID) return } - if !has || !hsData.received { + if !has || !hsData.received || hsData.stream == nil { + if !has { + hsData = newHandshakeData(false, false, nil) + } + + hsData.Lock() + defer hsData.Unlock() + // if !has { + // hsData = newHandshakeData(false, false, nil) + // } + + // hsData.Lock() + // defer hsData.Unlock() + + // if has && hsData.received && !hsData.validated { + // // peer has sent us an invalid handshake in the past, ignore + // logger.Warn("peer sent us invalid handshake before, ignoring...", "peer", peer, "protocol", info.protocolID) + // return + // } + + // if !has || !hsData.received { logger.Trace("sending outbound handshake", "protocol", info.protocolID, "peer", peer, "message", hs) stream, err := s.host.send(peer, info.protocolID, hs) if err != nil { @@ -324,12 +336,14 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc logger.Trace("handshake timeout reached", "protocol", info.protocolID, "peer", peer) _ = stream.Close() + info.outboundHandshakeData.Delete(peer) return case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): hsTimer.Stop() if hsResponse.err != nil { logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) _ = stream.Close() + info.outboundHandshakeData.Delete(peer) return } From 8629b43c5304ca37dda3545418566eacf6ade7de Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 12:10:56 -0500 Subject: [PATCH 42/59] change more stuff back again --- dot/network/notifications.go | 2 +- dot/network/service.go | 1 + dot/network/stream_manager.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index bcdb4d32d8..4a811caf2b 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -392,7 +392,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // the stream was closed or reset, close it on our end and delete it from our peer's data if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { _ = hsData.stream.Reset() - //info.outboundHandshakeData.Delete(peer) + info.outboundHandshakeData.Delete(peer) } return diff --git a/dot/network/service.go b/dot/network/service.go index ff470481bf..76e57876cd 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -176,6 +176,7 @@ func NewService(cfg *Config) (*Service, error) { telemetryInterval: cfg.telemetryInterval, closeCh: make(chan struct{}), bufPool: bufPool, + streamManager: newStreamManager(ctx), blockResponseBuf: make([]byte, maxBlockResponseSize), batchSize: 100, } diff --git a/dot/network/stream_manager.go b/dot/network/stream_manager.go index d55a127ceb..6755f5c3da 100644 --- a/dot/network/stream_manager.go +++ b/dot/network/stream_manager.go @@ -53,7 +53,7 @@ func (sm *streamManager) cleanupStreams() { stream := sdata.stream if time.Since(lastReceived) > cleanupStreamInterval { - _ = stream.Reset() + _ = stream.Close() sm.streamDataMap.Delete(id) } From f077036fd54fdc64a77f37b2bbea996e343e0f08 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 12:19:15 -0500 Subject: [PATCH 43/59] change more stuff back again --- lib/grandpa/vote_message.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/grandpa/vote_message.go b/lib/grandpa/vote_message.go index 7523f392d9..4ddef6f047 100644 --- a/lib/grandpa/vote_message.go +++ b/lib/grandpa/vote_message.go @@ -38,14 +38,14 @@ func (s *Service) receiveMessages(ctx context.Context) { for { select { case msg, ok := <-s.in: - if !ok { - return - } - if msg == nil || msg.msg == nil { continue } + if !ok { + return + } + logger.Trace("received vote message", "msg", msg.msg, "from", msg.from) vm := msg.msg From be2fcd8bf5138271201ca0238a6d85087d084fe7 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 12:40:00 -0500 Subject: [PATCH 44/59] close stream on handshake data delete --- dot/network/notifications.go | 8 ++++---- dot/network/service.go | 15 ++++++++++----- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 4a811caf2b..b51c49ce5d 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -221,7 +221,7 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, return nil } - logger.Debug("received message on notifications sub-protocol", "protocol", info.protocolID, + logger.Trace("received message on notifications sub-protocol", "protocol", info.protocolID, "message", msg, "peer", stream.Conn().RemotePeer(), ) @@ -354,8 +354,8 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc if err = info.handshakeValidator(peer, hs); err != nil { logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) hsData.validated = false - //hsData.stream = nil - //_ = stream.Reset() + hsData.stream = nil + _ = stream.Reset() info.outboundHandshakeData.Store(peer, hsData) return } @@ -391,7 +391,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // the stream was closed or reset, close it on our end and delete it from our peer's data if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { - _ = hsData.stream.Reset() + _ = hsData.stream.Close() info.outboundHandshakeData.Delete(peer) } diff --git a/dot/network/service.go b/dot/network/service.go index 76e57876cd..10bfa0c7c2 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -181,7 +181,6 @@ func NewService(cfg *Config) (*Service, error) { batchSize: 100, } - network.streamManager = newStreamManager(ctx) return network, err } @@ -484,26 +483,32 @@ func (s *Service) RegisterNotificationsProtocol( connMgr := s.host.h.ConnManager().(*ConnManager) connMgr.registerCloseHandler(protocolID, func(peerID peer.ID, inbound bool) { if inbound { - if _, ok := np.getInboundHandshakeData(peerID); ok { - logger.Trace( + if hsData, has := np.getInboundHandshakeData(peerID); has { + logger.Debug( "Cleaning up inbound handshake data", "peer", peerID, "protocol", protocolID, ) np.inboundHandshakeData.Delete(peerID) + if has && hsData.stream != nil { + _ = hsData.stream.Reset() + } } return } - if _, ok := np.getOutboundHandshakeData(peerID); ok { - logger.Trace( + if hsData, has := np.getOutboundHandshakeData(peerID); has { + logger.Debug( "Cleaning up outbound handshake data", "peer", peerID, "protocol", protocolID, ) np.outboundHandshakeData.Delete(peerID) + if has && hsData.stream != nil { + _ = hsData.stream.Reset() + } } }) From 7f1f429261550e40a5d81587e1c993c2bbd7de45 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 13:37:26 -0500 Subject: [PATCH 45/59] add closeInboundStream and closeOutboundStream funcs --- dot/network/notifications.go | 32 +++++++++---- dot/network/service.go | 91 +++++++++++++++++++++++------------- dot/types/grandpa.go | 2 +- 3 files changed, 83 insertions(+), 42 deletions(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index b51c49ce5d..4a453d90c0 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -271,6 +271,17 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, } } +func closeOutboundStream(info *notificationsProtocol, peerID peer.ID, stream libp2pnetwork.Stream) { + logger.Debug( + "Cleaning up outbound handshake data", + "peer", peerID, + "protocol", stream.Protocol(), + ) + + info.outboundHandshakeData.Delete(peerID) + _ = stream.Close() +} + func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtocol, msg NotificationsMessage) { if support, err := s.host.supportsProtocol(peer, info.protocolID); err != nil || !support { s.host.cm.peerSetHandler.ReportPeer(peerset.ReputationChange{ @@ -318,6 +329,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc if err != nil { logger.Trace("failed to send message to peer", "peer", peer, "error", err) //_ = stream.Reset() + closeOutboundStream(info, peer, stream) return } @@ -335,15 +347,17 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc }, peer) logger.Trace("handshake timeout reached", "protocol", info.protocolID, "peer", peer) - _ = stream.Close() - info.outboundHandshakeData.Delete(peer) + // _ = stream.Close() + // info.outboundHandshakeData.Delete(peer) + closeOutboundStream(info, peer, stream) return case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): hsTimer.Stop() if hsResponse.err != nil { logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) - _ = stream.Close() - info.outboundHandshakeData.Delete(peer) + // _ = stream.Close() + // info.outboundHandshakeData.Delete(peer) + closeOutboundStream(info, peer, stream) return } @@ -355,8 +369,9 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) hsData.validated = false hsData.stream = nil - _ = stream.Reset() - info.outboundHandshakeData.Store(peer, hsData) + // _ = stream.Reset() + // info.outboundHandshakeData.Store(peer, hsData) + closeOutboundStream(info, peer, stream) return } @@ -391,8 +406,9 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // the stream was closed or reset, close it on our end and delete it from our peer's data if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { - _ = hsData.stream.Close() - info.outboundHandshakeData.Delete(peer) + // _ = hsData.stream.Close() + // info.outboundHandshakeData.Delete(peer) + closeOutboundStream(info, peer, hsData.stream) } return diff --git a/dot/network/service.go b/dot/network/service.go index 10bfa0c7c2..443f39c1f1 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -480,37 +480,37 @@ func (s *Service) RegisterNotificationsProtocol( } s.notificationsProtocols[messageID] = np - connMgr := s.host.h.ConnManager().(*ConnManager) - connMgr.registerCloseHandler(protocolID, func(peerID peer.ID, inbound bool) { - if inbound { - if hsData, has := np.getInboundHandshakeData(peerID); has { - logger.Debug( - "Cleaning up inbound handshake data", - "peer", peerID, - "protocol", protocolID, - ) - - np.inboundHandshakeData.Delete(peerID) - if has && hsData.stream != nil { - _ = hsData.stream.Reset() - } - } - return - } - - if hsData, has := np.getOutboundHandshakeData(peerID); has { - logger.Debug( - "Cleaning up outbound handshake data", - "peer", peerID, - "protocol", protocolID, - ) - - np.outboundHandshakeData.Delete(peerID) - if has && hsData.stream != nil { - _ = hsData.stream.Reset() - } - } - }) + // connMgr := s.host.h.ConnManager().(*ConnManager) + // connMgr.registerCloseHandler(protocolID, func(peerID peer.ID, inbound bool) { + // if inbound { + // if hsData, has := np.getInboundHandshakeData(peerID); has { + // logger.Debug( + // "Cleaning up inbound handshake data", + // "peer", peerID, + // "protocol", protocolID, + // ) + + // np.inboundHandshakeData.Delete(peerID) + // if has && hsData.stream != nil { + // _ = hsData.stream.Reset() + // } + // } + // return + // } + + // if hsData, has := np.getOutboundHandshakeData(peerID); has { + // logger.Debug( + // "Cleaning up outbound handshake data", + // "peer", peerID, + // "protocol", protocolID, + // ) + + // np.outboundHandshakeData.Delete(peerID) + // if has && hsData.stream != nil { + // _ = hsData.stream.Reset() + // } + // } + // }) info := s.notificationsProtocols[messageID] @@ -606,6 +606,7 @@ func (s *Service) decodeLightMessage(in []byte, peer peer.ID, _ bool) (Message, } func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder, handler messageHandler) { + defer s.closeInboundStream(stream) s.streamManager.logNewStream(stream) peer := stream.Conn().RemotePeer() @@ -616,7 +617,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder tot, err := readStream(stream, msgBytes[:]) if err != nil { logger.Trace("failed to read from stream", "id", stream.ID(), "peer", stream.Conn().RemotePeer(), "protocol", stream.Protocol(), "error", err) - _ = stream.Close() + //_ = stream.Close() return } @@ -640,7 +641,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder if err = handler(stream, msg); err != nil { logger.Trace("failed to handle message from stream", "id", stream.ID(), "message", msg, "error", err) - _ = stream.Close() + //_ = stream.Close() return } @@ -648,6 +649,30 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder } } +func (s *Service) closeInboundStream(stream libp2pnetwork.Stream) { + protocolID := stream.Protocol() + peerID := stream.Conn().RemotePeer() + + s.notificationsMu.Lock() + defer s.notificationsMu.Unlock() + + for _, prtl := range s.notificationsProtocols { + if prtl.protocolID != protocolID { + continue + } + + prtl.inboundHandshakeData.Delete(peerID) + } + + logger.Debug( + "Cleaning up inbound handshake data", + "peer", peerID, + "protocol", protocolID, + ) + + _ = stream.Close() +} + func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error { defer func() { _ = stream.Close() diff --git a/dot/types/grandpa.go b/dot/types/grandpa.go index 8f6d83ca19..58f1bd4e9d 100644 --- a/dot/types/grandpa.go +++ b/dot/types/grandpa.go @@ -89,7 +89,7 @@ func (gv *GrandpaVoter) PublicKeyBytes() ed25519.PublicKeyBytes { // String returns a formatted GrandpaVoter string func (gv *GrandpaVoter) String() string { - return fmt.Sprintf("[key=0x%s id=%d]", gv.PublicKeyBytes(), gv.ID) + return fmt.Sprintf("[key=%s id=%d]", gv.PublicKeyBytes(), gv.ID) } // NewGrandpaVotersFromAuthorities returns an array of GrandpaVoters given an array of GrandpaAuthorities From a08a74203eff6627a7027c8ebb2857d882a1777b Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 20:34:40 -0500 Subject: [PATCH 46/59] add sendDataMutexes for each peer per protocol to prevent races --- dot/network/connmgr.go | 12 +- dot/network/notifications.go | 218 ++++++++++++++++++++--------------- dot/network/service.go | 34 +++++- lib/grandpa/grandpa.go | 9 +- 4 files changed, 173 insertions(+), 100 deletions(-) diff --git a/dot/network/connmgr.go b/dot/network/connmgr.go index ad7b04c77e..5edc258ae3 100644 --- a/dot/network/connmgr.go +++ b/dot/network/connmgr.go @@ -38,6 +38,7 @@ type ConnManager struct { sync.Mutex host *host min, max int + connectHandler func(peer.ID) disconnectHandler func(peer.ID) // closeHandlerMap contains close handler corresponding to a protocol. @@ -151,6 +152,10 @@ func (cm *ConnManager) unprotectedPeers(peers []peer.ID) []peer.ID { return unprot } +func (cm *ConnManager) setConnectHandler(handler func(peer.ID)) { + cm.connectHandler = handler +} + // Connected is called when a connection opened func (cm *ConnManager) Connected(n network.Network, c network.Conn) { logger.Trace( @@ -159,6 +164,8 @@ func (cm *ConnManager) Connected(n network.Network, c network.Conn) { "peer", c.RemotePeer(), ) + cm.connectHandler(c.RemotePeer()) + cm.Lock() defer cm.Unlock() @@ -167,6 +174,7 @@ func (cm *ConnManager) Connected(n network.Network, c network.Conn) { return } + // TODO: peer scoring doesn't seem to prevent us from going over the max // if over the max peer count, disconnect from (total_peers - maximum) peers for i := 0; i < over; i++ { unprotPeers := cm.unprotectedPeers(n.Peers()) @@ -216,7 +224,9 @@ func (cm *ConnManager) registerCloseHandler(protocolID protocol.ID, cb streamClo cm.closeHandlerMap[protocolID] = cb } -// ClosedStream is called when a stream closed +// ClosedStream is called when a stream closed by the remote side. +// Only inbound streams pass through this function. +// Outbound streams func (cm *ConnManager) ClosedStream(_ network.Network, s network.Stream) { logger.Trace( "Closed stream", diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 4a453d90c0..f3f5977e54 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -82,12 +82,13 @@ type notificationsProtocol struct { getHandshake HandshakeGetter handshakeDecoder HandshakeDecoder handshakeValidator HandshakeValidator + sendDataMutexes *sync.Map //map[peer.ID]*sync.Mutex inboundHandshakeData *sync.Map //map[peer.ID]*handshakeData outboundHandshakeData *sync.Map //map[peer.ID]*handshakeData } -func (n *notificationsProtocol) getInboundHandshakeData(pid peer.ID) (handshakeData, bool) { +func (n *notificationsProtocol) getInboundHandshakeData(pid peer.ID) (*handshakeData, bool) { var ( data interface{} has bool @@ -95,13 +96,13 @@ func (n *notificationsProtocol) getInboundHandshakeData(pid peer.ID) (handshakeD data, has = n.inboundHandshakeData.Load(pid) if !has { - return handshakeData{}, false + return nil, false } - return data.(handshakeData), true + return data.(*handshakeData), true } -func (n *notificationsProtocol) getOutboundHandshakeData(pid peer.ID) (handshakeData, bool) { +func (n *notificationsProtocol) getOutboundHandshakeData(pid peer.ID) (*handshakeData, bool) { var ( data interface{} has bool @@ -109,10 +110,10 @@ func (n *notificationsProtocol) getOutboundHandshakeData(pid peer.ID) (handshake data, has = n.outboundHandshakeData.Load(pid) if !has { - return handshakeData{}, false + return nil, false } - return data.(handshakeData), true + return data.(*handshakeData), true } type handshakeData struct { @@ -123,8 +124,8 @@ type handshakeData struct { *sync.Mutex // this needs to be a pointer, otherwise a new mutex will be created every time hsData is stored/loaded } -func newHandshakeData(received, validated bool, stream libp2pnetwork.Stream) handshakeData { - return handshakeData{ +func newHandshakeData(received, validated bool, stream libp2pnetwork.Stream) *handshakeData { + return &handshakeData{ received: received, validated: validated, stream: stream, @@ -137,7 +138,7 @@ func createDecoder(info *notificationsProtocol, handshakeDecoder HandshakeDecode // if we don't have handshake data on this peer, or we haven't received the handshake from them already, // assume we are receiving the handshake var ( - hsData handshakeData + hsData *handshakeData has bool ) @@ -297,88 +298,18 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc return } - hsData, has := info.getOutboundHandshakeData(peer) - if has && !hsData.validated { - // peer has sent us an invalid handshake in the past, ignore + stream, err := s.sendHandshake(peer, hs, info) + if err != nil { + logger.Debug("failed to send handshake", "peer", peer, "protocol", info.protocolID, "error", err) return } - if !has || !hsData.received || hsData.stream == nil { - if !has { - hsData = newHandshakeData(false, false, nil) - } - - hsData.Lock() - defer hsData.Unlock() - // if !has { - // hsData = newHandshakeData(false, false, nil) - // } - - // hsData.Lock() - // defer hsData.Unlock() - - // if has && hsData.received && !hsData.validated { - // // peer has sent us an invalid handshake in the past, ignore - // logger.Warn("peer sent us invalid handshake before, ignoring...", "peer", peer, "protocol", info.protocolID) - // return - // } - - // if !has || !hsData.received { - logger.Trace("sending outbound handshake", "protocol", info.protocolID, "peer", peer, "message", hs) - stream, err := s.host.send(peer, info.protocolID, hs) - if err != nil { - logger.Trace("failed to send message to peer", "peer", peer, "error", err) - //_ = stream.Reset() - closeOutboundStream(info, peer, stream) - return - } - - hsData.stream = stream - info.outboundHandshakeData.Store(peer, hsData) - - hsTimer := time.NewTimer(handshakeTimeout) - - var hs Handshake - select { - case <-hsTimer.C: - s.host.cm.peerSetHandler.ReportPeer(peerset.ReputationChange{ - Value: peerset.TimeOutValue, - Reason: peerset.TimeOutReason, - }, peer) - - logger.Trace("handshake timeout reached", "protocol", info.protocolID, "peer", peer) - // _ = stream.Close() - // info.outboundHandshakeData.Delete(peer) - closeOutboundStream(info, peer, stream) - return - case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): - hsTimer.Stop() - if hsResponse.err != nil { - logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) - // _ = stream.Close() - // info.outboundHandshakeData.Delete(peer) - closeOutboundStream(info, peer, stream) - return - } - - hs = hsResponse.hs - hsData.received = true - } - - if err = info.handshakeValidator(peer, hs); err != nil { - logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) - hsData.validated = false - hsData.stream = nil - // _ = stream.Reset() - // info.outboundHandshakeData.Store(peer, hsData) - closeOutboundStream(info, peer, stream) - return - } - - hsData.validated = true - info.outboundHandshakeData.Store(peer, hsData) - logger.Trace("sender: validated handshake", "protocol", info.protocolID, "peer", peer) - } + // if hsData != nil { + // info.outboundHandshakeData.Store(peer, hsData) + // } else { + // info.outboundHandshakeData.Delete(peer) + // return + // } if s.host.messageCache != nil { added, err := s.host.messageCache.put(peer, msg) @@ -400,15 +331,14 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc // we've completed the handshake with the peer, send message directly logger.Debug("sending message", "protocol", info.protocolID, "peer", peer, "message", msg) - err := s.host.writeToStream(hsData.stream, msg) - if err != nil { + if err = s.host.writeToStream(stream, msg); err != nil { logger.Debug("failed to send message to peer", "protocol", info.protocolID, "peer", peer, "error", err) // the stream was closed or reset, close it on our end and delete it from our peer's data if errors.Is(err, io.EOF) || errors.Is(err, mux.ErrReset) { // _ = hsData.stream.Close() // info.outboundHandshakeData.Delete(peer) - closeOutboundStream(info, peer, hsData.stream) + closeOutboundStream(info, peer, stream) } return @@ -421,6 +351,112 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc }, peer) } +func (s *Service) sendHandshake(peer peer.ID, hs Handshake, info *notificationsProtocol) (libp2pnetwork.Stream, error) { + mu, has := info.sendDataMutexes.Load(peer) + if !has || mu == nil { + panic("no sendDataMutex!!") + } + + if _, ok := mu.(*sync.Mutex); !ok { + panic("sendDataMutex is not *sync.Mutex!!!") + } + + mu.(*sync.Mutex).Lock() + defer mu.(*sync.Mutex).Unlock() + + hsData, has := info.getOutboundHandshakeData(peer) + if has && !hsData.validated { + // peer has sent us an invalid handshake in the past, ignore + return nil, errors.New("peer previously sent invalid handshake") + } + + if has && hsData.validated { + return hsData.stream, nil + } + + if !has { + hsData = newHandshakeData(false, false, nil) + } + + // TODO: if !has, then multiple processes could each call this upcoming section, opening multiple streams and + // sending multiple handshakes. need to have a per-peer and per-protocol lock + // hsData.Lock() + // defer hsData.Unlock() + + // // critical section entered, check if other process updated hsData while we were waiting + // hsData, has := info.getOutboundHandshakeData(peer) + // if has { + // return hsData, nil + // } + + // if !has { + // hsData = newHandshakeData(false, false, nil) + // } + + // hsData.Lock() + // defer hsData.Unlock() + + // if has && hsData.received && !hsData.validated { + // // peer has sent us an invalid handshake in the past, ignore + // logger.Warn("peer sent us invalid handshake before, ignoring...", "peer", peer, "protocol", info.protocolID) + // return + // } + + // if !has || !hsData.received { + logger.Trace("sending outbound handshake", "protocol", info.protocolID, "peer", peer, "message", hs) + stream, err := s.host.send(peer, info.protocolID, hs) + if err != nil { + logger.Trace("failed to send message to peer", "peer", peer, "error", err) + // don't need to close the stream here, as it's nil! + return nil, err + } + + hsData.stream = stream + //info.outboundHandshakeData.Store(peer, hsData) + + hsTimer := time.NewTimer(handshakeTimeout) + + var resp Handshake + select { + case <-hsTimer.C: + s.host.cm.peerSetHandler.ReportPeer(peerset.ReputationChange{ + Value: peerset.TimeOutValue, + Reason: peerset.TimeOutReason, + }, peer) + + logger.Trace("handshake timeout reached", "protocol", info.protocolID, "peer", peer) + closeOutboundStream(info, peer, stream) + return nil, errors.New("handshake timeout reached") + case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): + hsTimer.Stop() + if hsResponse.err != nil { + logger.Trace("failed to read handshake", "protocol", info.protocolID, "peer", peer, "error", err) + closeOutboundStream(info, peer, stream) + return nil, hsResponse.err + } + + resp = hsResponse.hs + hsData.received = true + } + + if err = info.handshakeValidator(peer, resp); err != nil { + logger.Trace("failed to validate handshake", "protocol", info.protocolID, "peer", peer, "error", err) + hsData.validated = false + hsData.stream = nil + _ = stream.Reset() + info.outboundHandshakeData.Store(peer, hsData) + // don't delete handshake data, as we want to store that the handshake for this peer was invalid + // and not to exchange messages over this protocol with it + return nil, err + } + + hsData.validated = true + hsData.handshake = resp + info.outboundHandshakeData.Store(peer, hsData) + logger.Trace("sender: validated handshake", "protocol", info.protocolID, "peer", peer) + return hsData.stream, nil +} + // broadcastExcluding sends a message to each connected peer except the given peer, // and peers that have previously sent us the message or who we have already sent the message to. // used for notifications sub-protocols to gossip a message diff --git a/dot/network/service.go b/dot/network/service.go index 443f39c1f1..24c1768d9e 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -245,8 +245,16 @@ func (s *Service) Start() error { } // since this opens block announce streams, it should happen after the protocol is registered + // NOTE: this only handles *incoming* connections s.host.h.Network().SetConnHandler(s.handleConn) + // this handles all new connections (incoming and outgoing) + s.host.cm.setConnectHandler(func(peerID peer.ID) { + for _, prtl := range s.notificationsProtocols { + prtl.sendDataMutexes.Store(peerID, new(sync.Mutex)) + } + }) + // log listening addresses to console for _, addr := range s.host.multiaddrs() { logger.Info("Started listening", "address", addr) @@ -292,6 +300,8 @@ func (s *Service) collectNetworkMetrics() { numOutboundBlockAnnounceStreams := metrics.GetOrRegisterGauge("network/streams/block_announce/outbound", metrics.DefaultRegistry) numInboundGrandpaStreams := metrics.GetOrRegisterGauge("network/streams/grandpa/inbound", metrics.DefaultRegistry) numOutboundGrandpaStreams := metrics.GetOrRegisterGauge("network/streams/grandpa/outbound", metrics.DefaultRegistry) + totalInboundStreams := metrics.GetOrRegisterGauge("network/streams/total/inbound", metrics.DefaultRegistry) + totalOutboundStreams := metrics.GetOrRegisterGauge("network/streams/total/outbound", metrics.DefaultRegistry) peerCount.Update(int64(s.host.peerCount())) totalConn.Update(int64(len(s.host.h.Network().Conns()))) @@ -299,8 +309,10 @@ func (s *Service) collectNetworkMetrics() { numInboundBlockAnnounceStreams.Update(s.getNumStreams(BlockAnnounceMsgType, true)) numOutboundBlockAnnounceStreams.Update(s.getNumStreams(BlockAnnounceMsgType, false)) - numInboundGrandpaStreams.Update(s.getNumStreams(ConsensusMsgType, false)) - numOutboundGrandpaStreams.Update(s.getNumStreams(ConsensusMsgType, true)) + numInboundGrandpaStreams.Update(s.getNumStreams(ConsensusMsgType, true)) + numOutboundGrandpaStreams.Update(s.getNumStreams(ConsensusMsgType, false)) + totalInboundStreams.Update(s.getTotalStreams(true)) + totalOutboundStreams.Update(s.getTotalStreams(false)) num, err := s.blockState.BestBlockNumber() if err != nil { @@ -313,6 +325,18 @@ func (s *Service) collectNetworkMetrics() { } } +func (s *Service) getTotalStreams(inbound bool) int64 { + var count int64 + for _, conn := range s.host.h.Network().Conns() { + for _, stream := range conn.GetStreams() { + if isInbound(stream) && inbound || !isInbound(stream) && !inbound { + count++ + } + } + } + return count +} + func (s *Service) getNumStreams(protocolID byte, inbound bool) int64 { np, has := s.notificationsProtocols[protocolID] if !has { @@ -475,6 +499,7 @@ func (s *Service) RegisterNotificationsProtocol( getHandshake: handshakeGetter, handshakeValidator: handshakeValidator, handshakeDecoder: handshakeDecoder, + sendDataMutexes: new(sync.Map), inboundHandshakeData: new(sync.Map), outboundHandshakeData: new(sync.Map), } @@ -606,6 +631,9 @@ func (s *Service) decodeLightMessage(in []byte, peer peer.ID, _ bool) (Message, } func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder, handler messageHandler) { + // we NEED to close the stream if we ever return from this function, as if we return, + // the stream will never again be read by us, so we need to tell the remote side we aren't + // reading from this stream. defer s.closeInboundStream(stream) s.streamManager.logNewStream(stream) @@ -670,7 +698,7 @@ func (s *Service) closeInboundStream(stream libp2pnetwork.Stream) { "protocol", protocolID, ) - _ = stream.Close() + _ = stream.Reset() } func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error { diff --git a/lib/grandpa/grandpa.go b/lib/grandpa/grandpa.go index 43e3d5af6e..820db960e4 100644 --- a/lib/grandpa/grandpa.go +++ b/lib/grandpa/grandpa.go @@ -175,6 +175,10 @@ func NewService(cfg *Config) (*Service, error) { interval: cfg.Interval, } + if err := s.registerProtocol(); err != nil { + return nil, err + } + s.messageHandler = NewMessageHandler(s, s.blockState) s.tracker = newTracker(s.blockState, s.messageHandler) s.paused.Store(false) @@ -183,11 +187,6 @@ func NewService(cfg *Config) (*Service, error) { // Start begins the GRANDPA finality service func (s *Service) Start() error { - // TODO: determine if we need to send a catch-up request (#1531) - if err := s.registerProtocol(); err != nil { - return err - } - // if we're not an authority, we don't need to worry about the voting process. // the grandpa service is only used to verify incoming block justifications if !s.authority { From 7d093babd64919acb0a57a6d871b203aeabeef96 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 20:40:52 -0500 Subject: [PATCH 47/59] fix --- dot/network/connmgr.go | 4 ++-- dot/network/service.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dot/network/connmgr.go b/dot/network/connmgr.go index 5edc258ae3..b3d75aaa78 100644 --- a/dot/network/connmgr.go +++ b/dot/network/connmgr.go @@ -38,7 +38,7 @@ type ConnManager struct { sync.Mutex host *host min, max int - connectHandler func(peer.ID) + connectHandler func(peer.ID) disconnectHandler func(peer.ID) // closeHandlerMap contains close handler corresponding to a protocol. @@ -226,7 +226,7 @@ func (cm *ConnManager) registerCloseHandler(protocolID protocol.ID, cb streamClo // ClosedStream is called when a stream closed by the remote side. // Only inbound streams pass through this function. -// Outbound streams +// Outbound streams func (cm *ConnManager) ClosedStream(_ network.Network, s network.Stream) { logger.Trace( "Closed stream", diff --git a/dot/network/service.go b/dot/network/service.go index 24c1768d9e..9442cd7668 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -356,7 +356,7 @@ func (s *Service) getNumStreams(protocolID byte, inbound bool) int64 { return true } - if data.(handshakeData).stream != nil { + if data.(*handshakeData).stream != nil { count++ } From 15bccf21946c981786a346539277d4029808def2 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 21:39:05 -0500 Subject: [PATCH 48/59] fix tests --- dot/network/block_announce_test.go | 2 +- dot/network/connmgr.go | 8 +- dot/network/host_test.go | 2 +- dot/network/inbound.go | 72 ++++++++++++++ dot/network/light.go | 69 +++++++++++++ dot/network/notifications.go | 37 ++++--- dot/network/notifications_test.go | 26 ++--- dot/network/service.go | 155 +++-------------------------- dot/network/service_test.go | 2 +- dot/sync/chain_sync_test.go | 2 +- 10 files changed, 200 insertions(+), 175 deletions(-) create mode 100644 dot/network/inbound.go diff --git a/dot/network/block_announce_test.go b/dot/network/block_announce_test.go index bc13558040..d7fe9e6dc2 100644 --- a/dot/network/block_announce_test.go +++ b/dot/network/block_announce_test.go @@ -168,7 +168,7 @@ func TestValidateBlockAnnounceHandshake(t *testing.T) { inboundHandshakeData: new(sync.Map), } testPeerID := peer.ID("noot") - nodeA.notificationsProtocols[BlockAnnounceMsgType].inboundHandshakeData.Store(testPeerID, handshakeData{}) + nodeA.notificationsProtocols[BlockAnnounceMsgType].inboundHandshakeData.Store(testPeerID, &handshakeData{}) err := nodeA.validateBlockAnnounceHandshake(testPeerID, &BlockAnnounceHandshake{ BestBlockNumber: 100, diff --git a/dot/network/connmgr.go b/dot/network/connmgr.go index fb1fcce580..429851b7aa 100644 --- a/dot/network/connmgr.go +++ b/dot/network/connmgr.go @@ -155,7 +155,9 @@ func (cm *ConnManager) Connected(n network.Network, c network.Conn) { logger.Tracef( "Host %s connected to peer %s", n.LocalPeer(), c.RemotePeer()) - cm.connectHandler(c.RemotePeer()) + if cm.connectHandler != nil { + cm.connectHandler(c.RemotePeer()) + } cm.Lock() defer cm.Unlock() @@ -188,6 +190,10 @@ func (cm *ConnManager) Connected(n network.Network, c network.Conn) { } } +func (cm *ConnManager) setDisconnectHandler(handler func(peer.ID)) { + cm.disconnectHandler = handler +} + // Disconnected is called when a connection closed func (cm *ConnManager) Disconnected(_ network.Network, c network.Conn) { logger.Tracef("Host %s disconnected from peer %s", c.LocalPeer(), c.RemotePeer()) diff --git a/dot/network/host_test.go b/dot/network/host_test.go index 800e58b65f..44b3c73c58 100644 --- a/dot/network/host_test.go +++ b/dot/network/host_test.go @@ -326,7 +326,7 @@ func TestStreamCloseMetadataCleanup(t *testing.T) { info := nodeA.notificationsProtocols[BlockAnnounceMsgType] // Set handshake data to received - info.inboundHandshakeData.Store(nodeB.host.id(), handshakeData{ + info.inboundHandshakeData.Store(nodeB.host.id(), &handshakeData{ received: true, validated: true, }) diff --git a/dot/network/inbound.go b/dot/network/inbound.go new file mode 100644 index 0000000000..90c82bb395 --- /dev/null +++ b/dot/network/inbound.go @@ -0,0 +1,72 @@ +package network + +import ( + libp2pnetwork "github.com/libp2p/go-libp2p-core/network" +) + +func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder, handler messageHandler) { + // we NEED to reset the stream if we ever return from this function, as if we return, + // the stream will never again be read by us, so we need to tell the remote side we aren't + // reading from this stream. + defer s.resetInboundStream(stream) + s.streamManager.logNewStream(stream) + + peer := stream.Conn().RemotePeer() + msgBytes := s.bufPool.get() + defer s.bufPool.put(&msgBytes) + + for { + tot, err := readStream(stream, msgBytes[:]) + if err != nil { + logger.Tracef( + "failed to read from stream id %s of peer %s using protocol %s: %s", + stream.ID(), stream.Conn().RemotePeer(), stream.Protocol(), err) + return + } + + s.streamManager.logMessageReceived(stream.ID()) + + // decode message based on message type + msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) // stream shoukd always be inbound if it passes through service.readStream + if err != nil { + logger.Tracef("failed to decode message from stream id %s using protocol %s: %s", + stream.ID(), stream.Protocol(), err) + continue + } + + logger.Tracef( + "host %s received message from peer %s: %s", + s.host.id(), peer, msg.String()) + + if err = handler(stream, msg); err != nil { + logger.Tracef("failed to handle message %s from stream id %s: %s", msg, stream.ID(), err) + return + } + + s.host.bwc.LogRecvMessage(int64(tot)) + } +} + +func (s *Service) resetInboundStream(stream libp2pnetwork.Stream) { + protocolID := stream.Protocol() + peerID := stream.Conn().RemotePeer() + + s.notificationsMu.Lock() + defer s.notificationsMu.Unlock() + + for _, prtl := range s.notificationsProtocols { + if prtl.protocolID != protocolID { + continue + } + + prtl.inboundHandshakeData.Delete(peerID) + } + + logger.Debugf( + "cleaning up inbound handshake data for protocol=%s, peer=%s", + stream.Protocol(), + peerID, + ) + + _ = stream.Reset() +} diff --git a/dot/network/light.go b/dot/network/light.go index 1ee6d774d4..67db2b94ae 100644 --- a/dot/network/light.go +++ b/dot/network/light.go @@ -6,8 +6,77 @@ import ( "github.com/ChainSafe/gossamer/dot/types" "github.com/ChainSafe/gossamer/lib/common" "github.com/ChainSafe/gossamer/pkg/scale" + + libp2pnetwork "github.com/libp2p/go-libp2p-core/network" + "github.com/libp2p/go-libp2p-core/peer" ) +// handleLightStream handles streams with the /light/2 protocol ID +func (s *Service) handleLightStream(stream libp2pnetwork.Stream) { + s.readStream(stream, s.decodeLightMessage, s.handleLightMsg) +} + +func (s *Service) decodeLightMessage(in []byte, peer peer.ID, _ bool) (Message, error) { + s.lightRequestMu.RLock() + defer s.lightRequestMu.RUnlock() + + // check if we are the requester + if _, requested := s.lightRequest[peer]; requested { + // if we are, decode the bytes as a LightResponse + msg := NewLightResponse() + err := msg.Decode(in) + return msg, err + } + + // otherwise, decode bytes as LightRequest + msg := NewLightRequest() + err := msg.Decode(in) + return msg, err +} + +func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error { + defer func() { + _ = stream.Close() + }() + + lr, ok := msg.(*LightRequest) + if !ok { + return nil + } + + resp := NewLightResponse() + var err error + switch { + case lr.RemoteCallRequest != nil: + resp.RemoteCallResponse, err = remoteCallResp(lr.RemoteCallRequest) + case lr.RemoteHeaderRequest != nil: + resp.RemoteHeaderResponse, err = remoteHeaderResp(lr.RemoteHeaderRequest) + case lr.RemoteChangesRequest != nil: + resp.RemoteChangesResponse, err = remoteChangeResp(lr.RemoteChangesRequest) + case lr.RemoteReadRequest != nil: + resp.RemoteReadResponse, err = remoteReadResp(lr.RemoteReadRequest) + case lr.RemoteReadChildRequest != nil: + resp.RemoteReadResponse, err = remoteReadChildResp(lr.RemoteReadChildRequest) + default: + logger.Warn("ignoring LightRequest without request data") + return nil + } + + if err != nil { + logger.Errorf("failed to get the response: %s", err) + return err + } + + // TODO(arijit): Remove once we implement the internal APIs. Added to increase code coverage. (#1856) + logger.Debugf("LightResponse message: %s", resp) + + err = s.host.writeToStream(stream, resp) + if err != nil { + logger.Warnf("failed to send LightResponse message to peer %s: %s", stream.Conn().RemotePeer(), err) + } + return err +} + // Pair is a pair of arbitrary bytes. type Pair struct { First []byte diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 1cdcbd4dfd..fa38ba53e9 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -78,14 +78,25 @@ type handshakeReader struct { } type notificationsProtocol struct { - protocolID protocol.ID - getHandshake HandshakeGetter - handshakeDecoder HandshakeDecoder - handshakeValidator HandshakeValidator - sendDataMutexes *sync.Map //map[peer.ID]*sync.Mutex - - inboundHandshakeData *sync.Map //map[peer.ID]*handshakeData - outboundHandshakeData *sync.Map //map[peer.ID]*handshakeData + protocolID protocol.ID + getHandshake HandshakeGetter + handshakeDecoder HandshakeDecoder + handshakeValidator HandshakeValidator + outboundHandshakeMutexes *sync.Map //map[peer.ID]*sync.Mutex + inboundHandshakeData *sync.Map //map[peer.ID]*handshakeData + outboundHandshakeData *sync.Map //map[peer.ID]*handshakeData +} + +func newNotificationsProtocol(protocolID protocol.ID, handshakeGetter HandshakeGetter, handshakeDecoder HandshakeDecoder, handshakeValidator HandshakeValidator) *notificationsProtocol { + return ¬ificationsProtocol{ + protocolID: protocolID, + getHandshake: handshakeGetter, + handshakeValidator: handshakeValidator, + handshakeDecoder: handshakeDecoder, + outboundHandshakeMutexes: new(sync.Map), + inboundHandshakeData: new(sync.Map), + outboundHandshakeData: new(sync.Map), + } } func (n *notificationsProtocol) getInboundHandshakeData(pid peer.ID) (*handshakeData, bool) { @@ -157,6 +168,7 @@ func createDecoder(info *notificationsProtocol, handshakeDecoder HandshakeDecode } } +// createNotificationsMessageHandler returns a function that is called by the handler of *inbound* streams. func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, messageHandler NotificationsMessageHandler, batchHandler NotificationsMessageBatchHandler) messageHandler { return func(stream libp2pnetwork.Stream, m Message) error { if m == nil || info == nil || info.handshakeValidator == nil || messageHandler == nil { @@ -339,13 +351,10 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc } func (s *Service) sendHandshake(peer peer.ID, hs Handshake, info *notificationsProtocol) (libp2pnetwork.Stream, error) { - mu, has := info.sendDataMutexes.Load(peer) + mu, has := info.outboundHandshakeMutexes.Load(peer) if !has || mu == nil { - panic("no sendDataMutex!!") - } - - if _, ok := mu.(*sync.Mutex); !ok { - panic("sendDataMutex is not *sync.Mutex!!!") + // this should not happen + return nil, errors.New("outboundHandshakeMutex does not exist") } // multiple processes could each call this upcoming section, opening multiple streams and diff --git a/dot/network/notifications_test.go b/dot/network/notifications_test.go index cfb8803bb4..24920ade6e 100644 --- a/dot/network/notifications_test.go +++ b/dot/network/notifications_test.go @@ -17,6 +17,7 @@ package network import ( + "errors" "fmt" "math/big" "reflect" @@ -62,7 +63,7 @@ func TestCreateDecoder_BlockAnnounce(t *testing.T) { // haven't received handshake from peer testPeerID := peer.ID("QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ") - info.inboundHandshakeData.Store(testPeerID, handshakeData{ + info.inboundHandshakeData.Store(testPeerID, &handshakeData{ received: false, }) @@ -147,7 +148,7 @@ func TestCreateNotificationsMessageHandler_BlockAnnounce(t *testing.T) { handler := s.createNotificationsMessageHandler(info, s.handleBlockAnnounceMessage, nil) // set handshake data to received - info.inboundHandshakeData.Store(testPeerID, handshakeData{ + info.inboundHandshakeData.Store(testPeerID, &handshakeData{ received: true, validated: true, }) @@ -263,16 +264,14 @@ func Test_HandshakeTimeout(t *testing.T) { nodeB.noGossip = true // create info and handler - info := ¬ificationsProtocol{ - protocolID: nodeA.host.protocolID + blockAnnounceID, - getHandshake: nodeA.getBlockAnnounceHandshake, - handshakeValidator: nodeA.validateBlockAnnounceHandshake, - inboundHandshakeData: new(sync.Map), - outboundHandshakeData: new(sync.Map), + testHandshakeDecoder := func([]byte) (Handshake, error) { + return nil, errors.New("unimplemented") } + info := newNotificationsProtocol(nodeA.host.protocolID+blockAnnounceID, + nodeA.getBlockAnnounceHandshake, testHandshakeDecoder, nodeA.validateBlockAnnounceHandshake) nodeB.host.h.SetStreamHandler(info.protocolID, func(stream libp2pnetwork.Stream) { - fmt.Println("never respond a handshake message") + fmt.Println("never responded to a handshake message") }) addrInfosB := nodeB.host.addrInfo() @@ -293,13 +292,14 @@ func Test_HandshakeTimeout(t *testing.T) { } nodeA.GossipMessage(testHandshakeMsg) + info.outboundHandshakeMutexes.Store(nodeB.host.id(), new(sync.Mutex)) go nodeA.sendData(nodeB.host.id(), testHandshakeMsg, info, nil) time.Sleep(time.Second) - // Verify that handshake data exists. + // handshake data shouldn't exist, as nodeB hasn't responded yet _, ok := info.getOutboundHandshakeData(nodeB.host.id()) - require.True(t, ok) + require.False(t, ok) // a stream should be open until timeout connAToB := nodeA.host.h.Network().ConnsToPeer(nodeB.host.id()) @@ -309,7 +309,7 @@ func Test_HandshakeTimeout(t *testing.T) { // after the timeout time.Sleep(handshakeTimeout) - // handshake data should be removed + // handshake data shouldn't exist still _, ok = info.getOutboundHandshakeData(nodeB.host.id()) require.False(t, ok) @@ -374,7 +374,7 @@ func TestCreateNotificationsMessageHandler_HandleTransaction(t *testing.T) { handler := s.createNotificationsMessageHandler(info, s.handleTransactionMessage, txnBatchHandler) // set handshake data to received - info.inboundHandshakeData.Store(testPeerID, handshakeData{ + info.inboundHandshakeData.Store(testPeerID, &handshakeData{ received: true, validated: true, }) diff --git a/dot/network/service.go b/dot/network/service.go index 43719470f9..aceb701d1c 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -247,9 +247,19 @@ func (s *Service) Start() error { s.host.h.Network().SetConnHandler(s.handleConn) // this handles all new connections (incoming and outgoing) + // it creates a per-protocol mutex for sending outbound handshakes to the peer s.host.cm.setConnectHandler(func(peerID peer.ID) { for _, prtl := range s.notificationsProtocols { - prtl.sendDataMutexes.Store(peerID, new(sync.Mutex)) + prtl.outboundHandshakeMutexes.Store(peerID, new(sync.Mutex)) + } + }) + + // when a peer gets disconnected, we should clear all handshake data we have for it. + s.host.cm.setDisconnectHandler(func(peerID peer.ID) { + for _, prtl := range s.notificationsProtocols { + prtl.outboundHandshakeMutexes.Delete(peerID) + prtl.inboundHandshakeData.Delete(peerID) + prtl.outboundHandshakeData.Delete(peerID) } }) @@ -493,15 +503,7 @@ func (s *Service) RegisterNotificationsProtocol( return errors.New("notifications protocol with message type already exists") } - np := ¬ificationsProtocol{ - protocolID: protocolID, - getHandshake: handshakeGetter, - handshakeValidator: handshakeValidator, - handshakeDecoder: handshakeDecoder, - sendDataMutexes: new(sync.Map), - inboundHandshakeData: new(sync.Map), - outboundHandshakeData: new(sync.Map), - } + np := newNotificationsProtocol(protocolID, handshakeGetter, handshakeDecoder, handshakeValidator) s.notificationsProtocols[messageID] = np decoder := createDecoder(np, handshakeDecoder, messageDecoder) handlerWithValidate := s.createNotificationsMessageHandler(np, messageHandler, batchHandler) @@ -567,139 +569,6 @@ func (s *Service) SendMessage(to peer.ID, msg NotificationsMessage) error { return errors.New("message not supported by any notifications protocol") } -// handleLightStream handles streams with the /light/2 protocol ID -func (s *Service) handleLightStream(stream libp2pnetwork.Stream) { - s.readStream(stream, s.decodeLightMessage, s.handleLightMsg) -} - -func (s *Service) decodeLightMessage(in []byte, peer peer.ID, _ bool) (Message, error) { - s.lightRequestMu.RLock() - defer s.lightRequestMu.RUnlock() - - // check if we are the requester - if _, requested := s.lightRequest[peer]; requested { - // if we are, decode the bytes as a LightResponse - msg := NewLightResponse() - err := msg.Decode(in) - return msg, err - } - - // otherwise, decode bytes as LightRequest - msg := NewLightRequest() - err := msg.Decode(in) - return msg, err -} - -func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder, handler messageHandler) { - // we NEED to reset the stream if we ever return from this function, as if we return, - // the stream will never again be read by us, so we need to tell the remote side we aren't - // reading from this stream. - defer s.resetInboundStream(stream) - s.streamManager.logNewStream(stream) - - peer := stream.Conn().RemotePeer() - msgBytes := s.bufPool.get() - defer s.bufPool.put(&msgBytes) - - for { - tot, err := readStream(stream, msgBytes[:]) - if err != nil { - logger.Tracef( - "failed to read from stream id %s of peer %s using protocol %s: %s", - stream.ID(), stream.Conn().RemotePeer(), stream.Protocol(), err) - return - } - - s.streamManager.logMessageReceived(stream.ID()) - - // decode message based on message type - msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) // stream shoukd always be inbound if it passes through service.readStream - if err != nil { - logger.Tracef("failed to decode message from stream id %s using protocol %s: %s", - stream.ID(), stream.Protocol(), err) - continue - } - - logger.Tracef( - "host %s received message from peer %s: %s", - s.host.id(), peer, msg.String()) - - if err = handler(stream, msg); err != nil { - logger.Tracef("failed to handle message %s from stream id %s: %s", msg, stream.ID(), err) - return - } - - s.host.bwc.LogRecvMessage(int64(tot)) - } -} - -func (s *Service) resetInboundStream(stream libp2pnetwork.Stream) { - protocolID := stream.Protocol() - peerID := stream.Conn().RemotePeer() - - s.notificationsMu.Lock() - defer s.notificationsMu.Unlock() - - for _, prtl := range s.notificationsProtocols { - if prtl.protocolID != protocolID { - continue - } - - prtl.inboundHandshakeData.Delete(peerID) - } - - logger.Debugf( - "cleaning up inbound handshake data for protocol=%s, peer=%s", - stream.Protocol(), - peerID, - ) - - _ = stream.Reset() -} - -func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error { - defer func() { - _ = stream.Close() - }() - - lr, ok := msg.(*LightRequest) - if !ok { - return nil - } - - resp := NewLightResponse() - var err error - switch { - case lr.RemoteCallRequest != nil: - resp.RemoteCallResponse, err = remoteCallResp(lr.RemoteCallRequest) - case lr.RemoteHeaderRequest != nil: - resp.RemoteHeaderResponse, err = remoteHeaderResp(lr.RemoteHeaderRequest) - case lr.RemoteChangesRequest != nil: - resp.RemoteChangesResponse, err = remoteChangeResp(lr.RemoteChangesRequest) - case lr.RemoteReadRequest != nil: - resp.RemoteReadResponse, err = remoteReadResp(lr.RemoteReadRequest) - case lr.RemoteReadChildRequest != nil: - resp.RemoteReadResponse, err = remoteReadChildResp(lr.RemoteReadChildRequest) - default: - logger.Warn("ignoring LightRequest without request data") - return nil - } - - if err != nil { - logger.Errorf("failed to get the response: %s", err) - return err - } - - // TODO(arijit): Remove once we implement the internal APIs. Added to increase code coverage. (#1856) - logger.Debugf("LightResponse message: %s", resp) - - err = s.host.writeToStream(stream, resp) - if err != nil { - logger.Warnf("failed to send LightResponse message to peer %s: %s", stream.Conn().RemotePeer(), err) - } - return err -} - // Health returns information about host needed for the rpc server func (s *Service) Health() common.Health { return common.Health{ diff --git a/dot/network/service_test.go b/dot/network/service_test.go index 56a5a85ee4..ac24753fe8 100644 --- a/dot/network/service_test.go +++ b/dot/network/service_test.go @@ -224,7 +224,7 @@ func TestBroadcastDuplicateMessage(t *testing.T) { require.NotNil(t, stream) protocol := nodeA.notificationsProtocols[BlockAnnounceMsgType] - protocol.outboundHandshakeData.Store(nodeB.host.id(), handshakeData{ + protocol.outboundHandshakeData.Store(nodeB.host.id(), &handshakeData{ received: true, validated: true, stream: stream, diff --git a/dot/sync/chain_sync_test.go b/dot/sync/chain_sync_test.go index 08d6a86cf7..900ab0f9d3 100644 --- a/dot/sync/chain_sync_test.go +++ b/dot/sync/chain_sync_test.go @@ -467,7 +467,7 @@ func TestValidateBlockData(t *testing.T) { err = cs.validateBlockData(req, &types.BlockData{ Header: &types.Header{}, }, "") - require.Equal(t, errNilBodyInResponse, err) + require.True(t, errors.Is(err, errNilBodyInResponse)) err = cs.validateBlockData(req, &types.BlockData{ Header: &types.Header{}, From bfbca1713a5a604e483cc49576c70a82b7d01151 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 21:48:33 -0500 Subject: [PATCH 49/59] cleanup --- dot/network/connmgr.go | 37 +++++++++---------------------------- dot/network/discovery.go | 4 +++- dot/network/inbound.go | 5 +++-- 3 files changed, 15 insertions(+), 31 deletions(-) diff --git a/dot/network/connmgr.go b/dot/network/connmgr.go index 429851b7aa..8484a8de93 100644 --- a/dot/network/connmgr.go +++ b/dot/network/connmgr.go @@ -25,14 +25,11 @@ import ( "github.com/libp2p/go-libp2p-core/connmgr" "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" - "github.com/libp2p/go-libp2p-core/protocol" ma "github.com/multiformats/go-multiaddr" "github.com/ChainSafe/gossamer/dot/peerset" ) -type streamCloseHandler func(peerID peer.ID, inbound bool) - // ConnManager implements connmgr.ConnManager type ConnManager struct { sync.Mutex @@ -41,9 +38,6 @@ type ConnManager struct { connectHandler func(peer.ID) disconnectHandler func(peer.ID) - // closeHandlerMap contains close handler corresponding to a protocol. - closeHandlerMap map[protocol.ID]streamCloseHandler - // protectedPeers contains a list of peers that are protected from pruning // when we reach the maximum numbers of peers. protectedPeers *sync.Map // map[peer.ID]struct{} @@ -63,7 +57,6 @@ func newConnManager(min, max int, peerSetCfg *peerset.ConfigSet) (*ConnManager, return &ConnManager{ min: min, max: max, - closeHandlerMap: make(map[protocol.ID]streamCloseHandler), protectedPeers: new(sync.Map), persistentPeers: new(sync.Map), peerSetHandler: psh, @@ -84,19 +77,19 @@ func (cm *ConnManager) Notifee() network.Notifiee { return nb } -// TagPeer peer +// TagPeer is unimplemented func (*ConnManager) TagPeer(peer.ID, string, int) {} -// UntagPeer peer +// UntagPeer is unimplemented func (*ConnManager) UntagPeer(peer.ID, string) {} -// UpsertTag peer +// UpsertTag is unimplemented func (*ConnManager) UpsertTag(peer.ID, string, func(int) int) {} -// GetTagInfo peer +// GetTagInfo is unimplemented func (*ConnManager) GetTagInfo(peer.ID) *connmgr.TagInfo { return &connmgr.TagInfo{} } -// TrimOpenConns peer +// TrimOpenConns is unimplemented func (*ConnManager) TrimOpenConns(context.Context) {} // Protect peer will add the given peer to the protectedPeerMap which will @@ -113,7 +106,7 @@ func (cm *ConnManager) Unprotect(id peer.ID, _ string) bool { return wasDeleted } -// Close peer +// Close is unimplemented func (*ConnManager) Close() error { return nil } // IsProtected returns whether the given peer is protected from pruning or not. @@ -167,7 +160,7 @@ func (cm *ConnManager) Connected(n network.Network, c network.Conn) { return } - // TODO: peer scoring doesn't seem to prevent us from going over the max + // TODO: peer scoring doesn't seem to prevent us from going over the max. // if over the max peer count, disconnect from (total_peers - maximum) peers for i := 0; i < over; i++ { unprotPeers := cm.unprotectedPeers(n.Peers()) @@ -204,28 +197,16 @@ func (cm *ConnManager) Disconnected(_ network.Network, c network.Conn) { } } -// OpenedStream is called when a stream opened +// OpenedStream is called when a stream is opened func (cm *ConnManager) OpenedStream(_ network.Network, s network.Stream) { logger.Tracef("Stream opened with peer %s using protocol %s", s.Conn().RemotePeer(), s.Protocol()) } -func (cm *ConnManager) registerCloseHandler(protocolID protocol.ID, cb streamCloseHandler) { - cm.closeHandlerMap[protocolID] = cb -} - -// ClosedStream is called when a stream closed by the remote side. -// Only inbound streams pass through this function. -// Outbound streams +// ClosedStream is called when a stream is closed func (cm *ConnManager) ClosedStream(_ network.Network, s network.Stream) { logger.Tracef("Stream closed with peer %s using protocol %s", s.Conn().RemotePeer(), s.Protocol()) - - cm.Lock() - defer cm.Unlock() - if closeCB, ok := cm.closeHandlerMap[s.Protocol()]; ok { - closeCB(s.Conn().RemotePeer(), isInbound(s)) - } } func (cm *ConnManager) isPersistent(p peer.ID) bool { diff --git a/dot/network/discovery.go b/dot/network/discovery.go index 4ea388d3fb..2c54a2746c 100644 --- a/dot/network/discovery.go +++ b/dot/network/discovery.go @@ -210,7 +210,9 @@ func (d *discovery) findPeers(ctx context.Context) { logger.Tracef("found new peer %s via DHT", peer.ID) - // TODO: this isn't working on the devnet + // TODO: this isn't working on the devnet (#2026) + // can remove the code block below this that directly connects + // once that's fixed d.h.Peerstore().AddAddrs(peer.ID, peer.Addrs, peerstore.PermanentAddrTTL) d.handler.AddPeer(0, peer.ID) diff --git a/dot/network/inbound.go b/dot/network/inbound.go index 90c82bb395..59daeafb80 100644 --- a/dot/network/inbound.go +++ b/dot/network/inbound.go @@ -6,8 +6,8 @@ import ( func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder, handler messageHandler) { // we NEED to reset the stream if we ever return from this function, as if we return, - // the stream will never again be read by us, so we need to tell the remote side we aren't - // reading from this stream. + // the stream will never again be read by us, so we need to tell the remote side we're + // done with this stream, and they should also forget about it. defer s.resetInboundStream(stream) s.streamManager.logNewStream(stream) @@ -60,6 +60,7 @@ func (s *Service) resetInboundStream(stream libp2pnetwork.Stream) { } prtl.inboundHandshakeData.Delete(peerID) + break } logger.Debugf( From 5efe3312b6336277b40c60ff64a005bb63102807 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 21:55:54 -0500 Subject: [PATCH 50/59] move errors to file --- dot/network/errors.go | 14 ++++++++++++++ dot/network/notifications.go | 27 +++++++++++---------------- 2 files changed, 25 insertions(+), 16 deletions(-) create mode 100644 dot/network/errors.go diff --git a/dot/network/errors.go b/dot/network/errors.go new file mode 100644 index 0000000000..95bb75a781 --- /dev/null +++ b/dot/network/errors.go @@ -0,0 +1,14 @@ +package network + +import ( + "errors" +) + +var ( + errCannotValidateHandshake = errors.New("failed to validate handshake") + errInvalidNotificationsMessage = errors.New("message is not NotificationsMessage") + errMessageIsNotHandshake = errors.New("failed to convert message to Handshake") + errMissingHandshakeMutex = errors.New("outboundHandshakeMutex does not exist") + errInvalidHandshakeForPeer = errors.New("peer previously sent invalid handshake") + errHandshakeTimeout = errors.New("handshake timeout reached") +) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index fa38ba53e9..6928212ebc 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -31,13 +31,10 @@ import ( "github.com/libp2p/go-libp2p-core/protocol" ) -var ( - errCannotValidateHandshake = errors.New("failed to validate handshake") - maxHandshakeSize = reflect.TypeOf(BlockAnnounceHandshake{}).Size() -) - const handshakeTimeout = time.Second * 10 +var maxHandshakeSize = reflect.TypeOf(BlockAnnounceHandshake{}).Size() + // Handshake is the interface all handshakes for notifications protocols must implement type Handshake interface { NotificationsMessage @@ -128,11 +125,10 @@ func (n *notificationsProtocol) getOutboundHandshakeData(pid peer.ID) (*handshak } type handshakeData struct { - received bool - validated bool - handshake Handshake - stream libp2pnetwork.Stream - *sync.Mutex // this needs to be a pointer, otherwise a new mutex will be created every time hsData is stored/loaded + received bool + validated bool + handshake Handshake + stream libp2pnetwork.Stream } func newHandshakeData(received, validated bool, stream libp2pnetwork.Stream) *handshakeData { @@ -140,7 +136,6 @@ func newHandshakeData(received, validated bool, stream libp2pnetwork.Stream) *ha received: received, validated: validated, stream: stream, - Mutex: new(sync.Mutex), } } @@ -182,7 +177,7 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, ) if msg, ok = m.(NotificationsMessage); !ok { - return errors.New("message is not NotificationsMessage") + return errInvalidNotificationsMessage } if msg.IsHandshake() { @@ -191,7 +186,7 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, hs, ok := msg.(Handshake) if !ok { - return errors.New("failed to convert message to Handshake") + return errMessageIsNotHandshake } // if we are the receiver and haven't received the handshake already, validate it @@ -354,7 +349,7 @@ func (s *Service) sendHandshake(peer peer.ID, hs Handshake, info *notificationsP mu, has := info.outboundHandshakeMutexes.Load(peer) if !has || mu == nil { // this should not happen - return nil, errors.New("outboundHandshakeMutex does not exist") + return nil, errMissingHandshakeMutex } // multiple processes could each call this upcoming section, opening multiple streams and @@ -365,7 +360,7 @@ func (s *Service) sendHandshake(peer peer.ID, hs Handshake, info *notificationsP hsData, has := info.getOutboundHandshakeData(peer) if has && !hsData.validated { // peer has sent us an invalid handshake in the past, ignore - return nil, errors.New("peer previously sent invalid handshake") + return nil, errInvalidHandshakeForPeer } if has && hsData.validated { @@ -399,7 +394,7 @@ func (s *Service) sendHandshake(peer peer.ID, hs Handshake, info *notificationsP logger.Tracef("handshake timeout reached for peer %s using protocol %s", peer, info.protocolID) closeOutboundStream(info, peer, stream) - return nil, errors.New("handshake timeout reached") + return nil, errHandshakeTimeout case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): hsTimer.Stop() if hsResponse.err != nil { From 0ef20de0f12f01907cc8dbed8a40e6fb90a01688 Mon Sep 17 00:00:00 2001 From: noot Date: Thu, 11 Nov 2021 22:43:56 -0500 Subject: [PATCH 51/59] fix tests --- dot/services_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dot/services_test.go b/dot/services_test.go index 7bd1071114..c371e61732 100644 --- a/dot/services_test.go +++ b/dot/services_test.go @@ -282,7 +282,10 @@ func TestCreateGrandpaService(t *testing.T) { dh, err := createDigestHandler(stateSrvc) require.NoError(t, err) - gs, err := createGRANDPAService(cfg, stateSrvc, dh, ks.Gran, &network.Service{}) + networkSrvc, err := createNetworkService(cfg, stateSrvc) + require.NoError(t, err) + + gs, err := createGRANDPAService(cfg, stateSrvc, dh, ks.Gran, networkSrvc) require.NoError(t, err) require.NotNil(t, gs) } From 426a5faf3bf11ee1fb76afb205f0e09f7aab7843 Mon Sep 17 00:00:00 2001 From: noot Date: Fri, 12 Nov 2021 14:42:20 -0500 Subject: [PATCH 52/59] address comments --- dot/network/connmgr.go | 9 +++++++++ dot/network/discovery.go | 13 ++++++------- dot/network/inbound.go | 4 ++-- dot/network/light.go | 26 ++++++++++++++++---------- dot/network/notifications.go | 26 +++++++++++++------------- dot/sync/chain_sync_test.go | 2 +- 6 files changed, 47 insertions(+), 33 deletions(-) diff --git a/dot/network/connmgr.go b/dot/network/connmgr.go index 8484a8de93..310e6c393c 100644 --- a/dot/network/connmgr.go +++ b/dot/network/connmgr.go @@ -140,6 +140,10 @@ func (cm *ConnManager) unprotectedPeers(peers []peer.ID) []peer.ID { } func (cm *ConnManager) setConnectHandler(handler func(peer.ID)) { + if cm.connectHandler != nil { + return + } + cm.connectHandler = handler } @@ -162,6 +166,7 @@ func (cm *ConnManager) Connected(n network.Network, c network.Conn) { // TODO: peer scoring doesn't seem to prevent us from going over the max. // if over the max peer count, disconnect from (total_peers - maximum) peers + // (#2039) for i := 0; i < over; i++ { unprotPeers := cm.unprotectedPeers(n.Peers()) if len(unprotPeers) == 0 { @@ -184,6 +189,10 @@ func (cm *ConnManager) Connected(n network.Network, c network.Conn) { } func (cm *ConnManager) setDisconnectHandler(handler func(peer.ID)) { + if cm.disconnectHandler != nil { + return + } + cm.disconnectHandler = handler } diff --git a/dot/network/discovery.go b/dot/network/discovery.go index 2c54a2746c..6f23343969 100644 --- a/dot/network/discovery.go +++ b/dot/network/discovery.go @@ -211,21 +211,20 @@ func (d *discovery) findPeers(ctx context.Context) { logger.Tracef("found new peer %s via DHT", peer.ID) // TODO: this isn't working on the devnet (#2026) - // can remove the code block below this that directly connects + // can remove the code block below which directly connects // once that's fixed d.h.Peerstore().AddAddrs(peer.ID, peer.Addrs, peerstore.PermanentAddrTTL) d.handler.AddPeer(0, peer.ID) // found a peer, try to connect if we need more peers - if len(d.h.Network().Peers()) < d.maxPeers { - err = d.h.Connect(d.ctx, peer) - if err != nil { - logger.Tracef("failed to connect to discovered peer %s: %s", peer.ID, err) - } - } else { + if len(d.h.Network().Peers()) >= d.maxPeers { d.h.Peerstore().AddAddrs(peer.ID, peer.Addrs, peerstore.PermanentAddrTTL) return } + + if err = d.h.Connect(d.ctx, peer); err != nil { + logger.Tracef("failed to connect to discovered peer %s: %s", peer.ID, err) + } } } } diff --git a/dot/network/inbound.go b/dot/network/inbound.go index 59daeafb80..dcb87867d8 100644 --- a/dot/network/inbound.go +++ b/dot/network/inbound.go @@ -27,7 +27,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder s.streamManager.logMessageReceived(stream.ID()) // decode message based on message type - msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) // stream shoukd always be inbound if it passes through service.readStream + msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) // stream should always be inbound if it passes through service.readStream if err != nil { logger.Tracef("failed to decode message from stream id %s using protocol %s: %s", stream.ID(), stream.Protocol(), err) @@ -36,7 +36,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder logger.Tracef( "host %s received message from peer %s: %s", - s.host.id(), peer, msg.String()) + s.host.id(), peer, msg) if err = handler(stream, msg); err != nil { logger.Tracef("failed to handle message %s from stream id %s: %s", msg, stream.ID(), err) diff --git a/dot/network/light.go b/dot/network/light.go index 67db2b94ae..e0b12158d5 100644 --- a/dot/network/light.go +++ b/dot/network/light.go @@ -21,20 +21,16 @@ func (s *Service) decodeLightMessage(in []byte, peer peer.ID, _ bool) (Message, defer s.lightRequestMu.RUnlock() // check if we are the requester - if _, requested := s.lightRequest[peer]; requested { + if _, ok := s.lightRequest[peer]; ok { // if we are, decode the bytes as a LightResponse - msg := NewLightResponse() - err := msg.Decode(in) - return msg, err + return newLightResponseFromBytes(in) } // otherwise, decode bytes as LightRequest - msg := NewLightRequest() - err := msg.Decode(in) - return msg, err + return newLightRequestFromBytes(in) } -func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error { +func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) (err error) { defer func() { _ = stream.Close() }() @@ -45,7 +41,6 @@ func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error } resp := NewLightResponse() - var err error switch { case lr.RemoteCallRequest != nil: resp.RemoteCallResponse, err = remoteCallResp(lr.RemoteCallRequest) @@ -63,7 +58,6 @@ func (s *Service) handleLightMsg(stream libp2pnetwork.Stream, msg Message) error } if err != nil { - logger.Errorf("failed to get the response: %s", err) return err } @@ -112,6 +106,12 @@ func NewLightRequest() *LightRequest { } } +func newLightRequestFromBytes(in []byte) (msg *LightRequest, err error) { + msg = NewLightRequest() + err = msg.Decode(in) + return +} + func newRequest() *request { return &request{ RemoteCallRequest: *newRemoteCallRequest(), @@ -188,6 +188,12 @@ func NewLightResponse() *LightResponse { } } +func newLightResponseFromBytes(in []byte) (msg *LightResponse, err error) { + msg = NewLightResponse() + err = msg.Decode(in) + return +} + func newResponse() *response { return &response{ RemoteCallResponse: *newRemoteCallResponse(), diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 6928212ebc..bb0330b7bc 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -289,6 +289,11 @@ func closeOutboundStream(info *notificationsProtocol, peerID peer.ID, stream lib } func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtocol, msg NotificationsMessage) { + if info.handshakeValidator == nil { + logger.Errorf("handshakeValidator is not set for protocol %s", info.protocolID) + return + } + if support, err := s.host.supportsProtocol(peer, info.protocolID); err != nil || !support { s.host.cm.peerSetHandler.ReportPeer(peerset.ReputationChange{ Value: peerset.BadProtocolValue, @@ -298,11 +303,6 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc return } - if info.handshakeValidator == nil { - logger.Errorf("handshakeValidator is not set for protocol %s", info.protocolID) - return - } - stream, err := s.sendHandshake(peer, hs, info) if err != nil { logger.Debugf("failed to send handshake to peer %s on protocol %s: %s", peer, info.protocolID, err) @@ -358,16 +358,13 @@ func (s *Service) sendHandshake(peer peer.ID, hs Handshake, info *notificationsP defer mu.(*sync.Mutex).Unlock() hsData, has := info.getOutboundHandshakeData(peer) - if has && !hsData.validated { + switch { + case has && !hsData.validated: // peer has sent us an invalid handshake in the past, ignore return nil, errInvalidHandshakeForPeer - } - - if has && hsData.validated { + case has && hsData.validated: return hsData.stream, nil - } - - if !has { + case !has: hsData = newHandshakeData(false, false, nil) } @@ -396,7 +393,10 @@ func (s *Service) sendHandshake(peer peer.ID, hs Handshake, info *notificationsP closeOutboundStream(info, peer, stream) return nil, errHandshakeTimeout case hsResponse := <-s.readHandshake(stream, info.handshakeDecoder): - hsTimer.Stop() + if !hsTimer.Stop() { + <-hsTimer.C + } + if hsResponse.err != nil { logger.Tracef("failed to read handshake from peer %s using protocol %s: %s", peer, info.protocolID, err) closeOutboundStream(info, peer, stream) diff --git a/dot/sync/chain_sync_test.go b/dot/sync/chain_sync_test.go index 900ab0f9d3..8a437ed88c 100644 --- a/dot/sync/chain_sync_test.go +++ b/dot/sync/chain_sync_test.go @@ -467,7 +467,7 @@ func TestValidateBlockData(t *testing.T) { err = cs.validateBlockData(req, &types.BlockData{ Header: &types.Header{}, }, "") - require.True(t, errors.Is(err, errNilBodyInResponse)) + require.ErrorIs(t, err, errNilBodyInResponse) err = cs.validateBlockData(req, &types.BlockData{ Header: &types.Header{}, From 122c2a41d2fa2fcae019eb8dfd34401343cb443c Mon Sep 17 00:00:00 2001 From: noot Date: Tue, 16 Nov 2021 12:13:47 -0500 Subject: [PATCH 53/59] address comments --- dot/network/connmgr.go | 25 ++----------------------- dot/network/notifications.go | 4 +--- dot/network/notifications_test.go | 6 +----- dot/network/service.go | 8 ++++---- 4 files changed, 8 insertions(+), 35 deletions(-) diff --git a/dot/network/connmgr.go b/dot/network/connmgr.go index 178e4c0260..9f7e257d73 100644 --- a/dot/network/connmgr.go +++ b/dot/network/connmgr.go @@ -126,22 +126,11 @@ func (cm *ConnManager) unprotectedPeers(peers []peer.ID) []peer.ID { return unprot } -func (cm *ConnManager) setConnectHandler(handler func(peer.ID)) { - if cm.connectHandler != nil { - return - } - - cm.connectHandler = handler -} - // Connected is called when a connection opened func (cm *ConnManager) Connected(n network.Network, c network.Conn) { logger.Tracef( "Host %s connected to peer %s", n.LocalPeer(), c.RemotePeer()) - - if cm.connectHandler != nil { - cm.connectHandler(c.RemotePeer()) - } + cm.connectHandler(c.RemotePeer()) cm.Lock() defer cm.Unlock() @@ -175,22 +164,12 @@ func (cm *ConnManager) Connected(n network.Network, c network.Conn) { } } -func (cm *ConnManager) setDisconnectHandler(handler func(peer.ID)) { - if cm.disconnectHandler != nil { - return - } - - cm.disconnectHandler = handler -} - // Disconnected is called when a connection closed func (cm *ConnManager) Disconnected(_ network.Network, c network.Conn) { logger.Tracef("Host %s disconnected from peer %s", c.LocalPeer(), c.RemotePeer()) cm.Unprotect(c.RemotePeer(), "") - if cm.disconnectHandler != nil { - cm.disconnectHandler(c.RemotePeer()) - } + cm.disconnectHandler(c.RemotePeer()) } // OpenedStream is called when a stream is opened diff --git a/dot/network/notifications.go b/dot/network/notifications.go index cc929408bc..5813445041 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -6,7 +6,7 @@ package network import ( "errors" "io" - "reflect" + //"reflect" "sync" "time" @@ -20,8 +20,6 @@ import ( const handshakeTimeout = time.Second * 10 -var maxHandshakeSize = reflect.TypeOf(BlockAnnounceHandshake{}).Size() - // Handshake is the interface all handshakes for notifications protocols must implement type Handshake interface { NotificationsMessage diff --git a/dot/network/notifications_test.go b/dot/network/notifications_test.go index 5788a4b610..1a5858718c 100644 --- a/dot/network/notifications_test.go +++ b/dot/network/notifications_test.go @@ -22,10 +22,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestHandshake_SizeOf(t *testing.T) { - require.Equal(t, uint32(maxHandshakeSize), uint32(72)) -} - func TestCreateDecoder_BlockAnnounce(t *testing.T) { basePath := utils.NewTestBasePath(t, "nodeA") @@ -258,7 +254,7 @@ func Test_HandshakeTimeout(t *testing.T) { nodeA.getBlockAnnounceHandshake, testHandshakeDecoder, nodeA.validateBlockAnnounceHandshake) nodeB.host.h.SetStreamHandler(info.protocolID, func(stream libp2pnetwork.Stream) { - fmt.Println("never responded to a handshake message") + // should not respond to a handshake message }) addrInfosB := nodeB.host.addrInfo() diff --git a/dot/network/service.go b/dot/network/service.go index e2c44e7b4d..59392c2651 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -234,20 +234,20 @@ func (s *Service) Start() error { // this handles all new connections (incoming and outgoing) // it creates a per-protocol mutex for sending outbound handshakes to the peer - s.host.cm.setConnectHandler(func(peerID peer.ID) { + s.host.cm.connectHandler = func(peerID peer.ID) { for _, prtl := range s.notificationsProtocols { prtl.outboundHandshakeMutexes.Store(peerID, new(sync.Mutex)) } - }) + } // when a peer gets disconnected, we should clear all handshake data we have for it. - s.host.cm.setDisconnectHandler(func(peerID peer.ID) { + s.host.cm.disconnectHandler = func(peerID peer.ID) { for _, prtl := range s.notificationsProtocols { prtl.outboundHandshakeMutexes.Delete(peerID) prtl.inboundHandshakeData.Delete(peerID) prtl.outboundHandshakeData.Delete(peerID) } - }) + } // log listening addresses to console for _, addr := range s.host.multiaddrs() { From b3db52a0a57c6f19a84307b3f3230867fad337af Mon Sep 17 00:00:00 2001 From: noot Date: Tue, 16 Nov 2021 13:07:52 -0500 Subject: [PATCH 54/59] lint --- dot/network/notifications_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/dot/network/notifications_test.go b/dot/network/notifications_test.go index 1a5858718c..0f2dc4b03a 100644 --- a/dot/network/notifications_test.go +++ b/dot/network/notifications_test.go @@ -5,7 +5,6 @@ package network import ( "errors" - "fmt" "math/big" "reflect" "sync" From 7ef53e35e4e708b0d1b020cd98dbd2cf48f54e20 Mon Sep 17 00:00:00 2001 From: noot Date: Tue, 16 Nov 2021 13:14:04 -0500 Subject: [PATCH 55/59] lint --- dot/network/notifications.go | 1 - 1 file changed, 1 deletion(-) diff --git a/dot/network/notifications.go b/dot/network/notifications.go index 5813445041..bf185f35af 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -6,7 +6,6 @@ package network import ( "errors" "io" - //"reflect" "sync" "time" From 99053f9197e9999bdc73543b0f9be4379d86342c Mon Sep 17 00:00:00 2001 From: noot Date: Tue, 16 Nov 2021 18:33:52 -0500 Subject: [PATCH 56/59] address comments --- dot/network/inbound.go | 6 +++--- dot/network/light.go | 16 ++++++++-------- dot/network/notifications.go | 2 +- dot/network/service.go | 8 +++----- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/dot/network/inbound.go b/dot/network/inbound.go index e0227248fe..45bb54bef4 100644 --- a/dot/network/inbound.go +++ b/dot/network/inbound.go @@ -16,7 +16,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder defer s.bufPool.put(msgBytes) for { - tot, err := readStream(stream, msgBytes[:]) + n, err := readStream(stream, msgBytes[:]) if err != nil { logger.Tracef( "failed to read from stream id %s of peer %s using protocol %s: %s", @@ -27,7 +27,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder s.streamManager.logMessageReceived(stream.ID()) // decode message based on message type - msg, err := decoder(msgBytes[:tot], peer, isInbound(stream)) // stream should always be inbound if it passes through service.readStream + msg, err := decoder(msgBytes[:n], peer, isInbound(stream)) // stream should always be inbound if it passes through service.readStream if err != nil { logger.Tracef("failed to decode message from stream id %s using protocol %s: %s", stream.ID(), stream.Protocol(), err) @@ -43,7 +43,7 @@ func (s *Service) readStream(stream libp2pnetwork.Stream, decoder messageDecoder return } - s.host.bwc.LogRecvMessage(int64(tot)) + s.host.bwc.LogRecvMessage(int64(n)) } } diff --git a/dot/network/light.go b/dot/network/light.go index 54468cf4e5..c16e86c5db 100644 --- a/dot/network/light.go +++ b/dot/network/light.go @@ -109,10 +109,10 @@ func NewLightRequest() *LightRequest { } } -func newLightRequestFromBytes(in []byte) (msg *LightRequest, err error) { - msg = NewLightRequest() - err = msg.Decode(in) - return +func newLightRequestFromBytes(in []byte) (*LightRequest, error) { + msg := NewLightRequest() + err := msg.Decode(in) + return msg, err } func newRequest() *request { @@ -191,10 +191,10 @@ func NewLightResponse() *LightResponse { } } -func newLightResponseFromBytes(in []byte) (msg *LightResponse, err error) { - msg = NewLightResponse() - err = msg.Decode(in) - return +func newLightResponseFromBytes(in []byte) (*LightResponse, error) { + msg := NewLightResponse() + err := msg.Decode(in) + return msg, err } func newResponse() *response { diff --git a/dot/network/notifications.go b/dot/network/notifications.go index bf185f35af..b8ac3d02d6 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -331,7 +331,7 @@ func (s *Service) sendData(peer peer.ID, hs Handshake, info *notificationsProtoc func (s *Service) sendHandshake(peer peer.ID, hs Handshake, info *notificationsProtocol) (libp2pnetwork.Stream, error) { mu, has := info.outboundHandshakeMutexes.Load(peer) - if !has || mu == nil { + if !has { // this should not happen return nil, errMissingHandshakeMutex } diff --git a/dot/network/service.go b/dot/network/service.go index 59392c2651..950175fb51 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -320,11 +320,10 @@ func (s *Service) collectNetworkMetrics() { } } -func (s *Service) getTotalStreams(inbound bool) int64 { - var count int64 +func (s *Service) getTotalStreams(inbound bool) (count int64) { for _, conn := range s.host.h.Network().Conns() { for _, stream := range conn.GetStreams() { - if isInbound(stream) && inbound || !isInbound(stream) && !inbound { + if (isInbound(stream) && inbound) || (!isInbound(stream) && !inbound) { count++ } } @@ -332,7 +331,7 @@ func (s *Service) getTotalStreams(inbound bool) int64 { return count } -func (s *Service) getNumStreams(protocolID byte, inbound bool) int64 { +func (s *Service) getNumStreams(protocolID byte, inbound bool) (count int64) { np, has := s.notificationsProtocols[protocolID] if !has { return 0 @@ -345,7 +344,6 @@ func (s *Service) getNumStreams(protocolID byte, inbound bool) int64 { hsData = np.outboundHandshakeData } - var count int64 hsData.Range(func(_, data interface{}) bool { if data == nil { return true From a8e9cfff8288482118c6d6c1a141f3ccd2c50779 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 17 Nov 2021 11:03:36 -0500 Subject: [PATCH 57/59] make license --- dot/network/errors.go | 3 +++ dot/network/inbound.go | 3 +++ dot/network/service.go | 3 ++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/dot/network/errors.go b/dot/network/errors.go index 95bb75a781..1d0028147f 100644 --- a/dot/network/errors.go +++ b/dot/network/errors.go @@ -1,3 +1,6 @@ +// Copyright 2021 ChainSafe Systems (ON) +// SPDX-License-Identifier: LGPL-3.0-only + package network import ( diff --git a/dot/network/inbound.go b/dot/network/inbound.go index 45bb54bef4..154684ed28 100644 --- a/dot/network/inbound.go +++ b/dot/network/inbound.go @@ -1,3 +1,6 @@ +// Copyright 2021 ChainSafe Systems (ON) +// SPDX-License-Identifier: LGPL-3.0-only + package network import ( diff --git a/dot/network/service.go b/dot/network/service.go index 950175fb51..58c0167954 100644 --- a/dot/network/service.go +++ b/dot/network/service.go @@ -323,7 +323,8 @@ func (s *Service) collectNetworkMetrics() { func (s *Service) getTotalStreams(inbound bool) (count int64) { for _, conn := range s.host.h.Network().Conns() { for _, stream := range conn.GetStreams() { - if (isInbound(stream) && inbound) || (!isInbound(stream) && !inbound) { + streamIsInbound := isInbound(stream) + if (streamIsInbound && inbound) || (!streamIsInbound && !inbound) { count++ } } From b0d01a002f4fd6a463e493a157d0c3d773691002 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 17 Nov 2021 17:26:20 -0500 Subject: [PATCH 58/59] address comments --- dot/network/errors.go | 2 +- dot/network/light.go | 12 ++++++------ dot/network/notifications.go | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/dot/network/errors.go b/dot/network/errors.go index 1d0028147f..0d86e3689f 100644 --- a/dot/network/errors.go +++ b/dot/network/errors.go @@ -9,7 +9,7 @@ import ( var ( errCannotValidateHandshake = errors.New("failed to validate handshake") - errInvalidNotificationsMessage = errors.New("message is not NotificationsMessage") + errMessageTypeNotValid = errors.New("message type is not valid") errMessageIsNotHandshake = errors.New("failed to convert message to Handshake") errMissingHandshakeMutex = errors.New("outboundHandshakeMutex does not exist") errInvalidHandshakeForPeer = errors.New("peer previously sent invalid handshake") diff --git a/dot/network/light.go b/dot/network/light.go index c16e86c5db..d782568133 100644 --- a/dot/network/light.go +++ b/dot/network/light.go @@ -109,9 +109,9 @@ func NewLightRequest() *LightRequest { } } -func newLightRequestFromBytes(in []byte) (*LightRequest, error) { - msg := NewLightRequest() - err := msg.Decode(in) +func newLightRequestFromBytes(in []byte) (msg *LightRequest, err error) { + msg = NewLightRequest() + err = msg.Decode(in) return msg, err } @@ -191,9 +191,9 @@ func NewLightResponse() *LightResponse { } } -func newLightResponseFromBytes(in []byte) (*LightResponse, error) { - msg := NewLightResponse() - err := msg.Decode(in) +func newLightResponseFromBytes(in []byte) (msg *LightResponse, err error) { + msg = NewLightResponse() + err = msg.Decode(in) return msg, err } diff --git a/dot/network/notifications.go b/dot/network/notifications.go index b8ac3d02d6..804388d4b0 100644 --- a/dot/network/notifications.go +++ b/dot/network/notifications.go @@ -5,6 +5,7 @@ package network import ( "errors" + "fmt" "io" "sync" "time" @@ -161,7 +162,7 @@ func (s *Service) createNotificationsMessageHandler(info *notificationsProtocol, ) if msg, ok = m.(NotificationsMessage); !ok { - return errInvalidNotificationsMessage + return fmt.Errorf("%w: expected %T but got %T", errMessageTypeNotValid, (NotificationsMessage)(nil), msg) } if msg.IsHandshake() { From b291a9f3deffdf94e98a8ae13919b1f014ac06b7 Mon Sep 17 00:00:00 2001 From: noot Date: Wed, 17 Nov 2021 17:26:50 -0500 Subject: [PATCH 59/59] lint --- dot/network/errors.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/dot/network/errors.go b/dot/network/errors.go index 0d86e3689f..a9c2bd94f3 100644 --- a/dot/network/errors.go +++ b/dot/network/errors.go @@ -8,10 +8,10 @@ import ( ) var ( - errCannotValidateHandshake = errors.New("failed to validate handshake") - errMessageTypeNotValid = errors.New("message type is not valid") - errMessageIsNotHandshake = errors.New("failed to convert message to Handshake") - errMissingHandshakeMutex = errors.New("outboundHandshakeMutex does not exist") - errInvalidHandshakeForPeer = errors.New("peer previously sent invalid handshake") - errHandshakeTimeout = errors.New("handshake timeout reached") + errCannotValidateHandshake = errors.New("failed to validate handshake") + errMessageTypeNotValid = errors.New("message type is not valid") + errMessageIsNotHandshake = errors.New("failed to convert message to Handshake") + errMissingHandshakeMutex = errors.New("outboundHandshakeMutex does not exist") + errInvalidHandshakeForPeer = errors.New("peer previously sent invalid handshake") + errHandshakeTimeout = errors.New("handshake timeout reached") )