Remove double locking in HasIP

consensus/byzantine_test.go

@@ -27,7 +27,7 @@ func init() {
 // Heal partition and ensure A sees the commit
 func TestByzantine(t *testing.T) {
 	N := 4
-	logger := consensusLogger()
+	logger := consensusLogger().With("test", "byzantine")
 	css := randConsensusNet(N, "consensus_byzantine_test", newMockTickerFunc(false), newCounter)
 
 	// give the byzantine validator a normal ticker

p2p/peer_set.go

@@ -42,11 +42,12 @@ func NewPeerSet() *PeerSet {
 func (ps *PeerSet) Add(peer Peer) error {
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()
+
 	if ps.lookup[peer.ID()] != nil {
 		return ErrSwitchDuplicatePeerID{peer.ID()}
 	}
 
-	if ps.HasIP(peer.RemoteIP()) {
+	if ps.hasIP(peer.RemoteIP()) {
 		return ErrSwitchDuplicatePeerIP{peer.RemoteIP()}
 	}
 
@@ -71,8 +72,14 @@ func (ps *PeerSet) Has(peerKey ID) bool {
 // address.
 func (ps *PeerSet) HasIP(peerIP net.IP) bool {
 	ps.mtx.Lock()
-	ps.mtx.Unlock()
+	defer ps.mtx.Unlock()
 
+	return ps.hasIP(peerIP)
+}
+
+// hasIP does not acquire a lock so it can be used in public methods which
+// already lock.
+func (ps *PeerSet) hasIP(peerIP net.IP) bool {
 	for _, item := range ps.lookup {
 		if item.peer.RemoteIP().Equal(peerIP) {
 			return true

p2p/peer_set_test.go

@@ -2,6 +2,7 @@ package p2p
 
 import (
 	"math/rand"
+	"net"
 	"sync"
 	"testing"
 
@@ -12,23 +13,34 @@ import (
 )
 
 // Returns an empty kvstore peer
-func randPeer() *peer {
+func randPeer(ip net.IP) *peer {
+	if ip == nil {
+		ip = net.IP{127, 0, 0, 1}
+	}
+
 	nodeKey := NodeKey{PrivKey: crypto.GenPrivKeyEd25519()}
-	return &peer{
+	p := &peer{
 		nodeInfo: NodeInfo{
 			ID:         nodeKey.ID(),
 			ListenAddr: cmn.Fmt("%v.%v.%v.%v:46656", rand.Int()%256, rand.Int()%256, rand.Int()%256, rand.Int()%256),
 		},
 	}
+
+	p.ips = []net.IP{
+		ip,
+	}
+
+	return p
 }
 
 func TestPeerSetAddRemoveOne(t *testing.T) {
 	t.Parallel()
+
 	peerSet := NewPeerSet()
 
 	var peerList []Peer
 	for i := 0; i < 5; i++ {
-		p := randPeer()
+		p := randPeer(net.IP{127, 0, 0, byte(i)})
 		if err := peerSet.Add(p); err != nil {
 			t.Error(err)
 		}
@@ -72,7 +84,7 @@ func TestPeerSetAddRemoveMany(t *testing.T) {
 	peers := []Peer{}
 	N := 100
 	for i := 0; i < N; i++ {
-		peer := randPeer()
+		peer := randPeer(net.IP{127, 0, 0, byte(i)})
 		if err := peerSet.Add(peer); err != nil {
 			t.Errorf("Failed to add new peer")
 		}
@@ -96,7 +108,7 @@ func TestPeerSetAddRemoveMany(t *testing.T) {
 func TestPeerSetAddDuplicate(t *testing.T) {
 	t.Parallel()
 	peerSet := NewPeerSet()
-	peer := randPeer()
+	peer := randPeer(nil)
 
 	n := 20
 	errsChan := make(chan error)
@@ -133,10 +145,17 @@ func TestPeerSetAddDuplicate(t *testing.T) {
 	assert.Equal(t, wantNilErrCount, gotNilErrCount, "invalid nil errCount")
 }
 
+func TestPeerSetAddDuplicateIP(t *testing.T) {
+}
+
 func TestPeerSetGet(t *testing.T) {
 	t.Parallel()
-	peerSet := NewPeerSet()
-	peer := randPeer()
+
+	var (
+		peerSet = NewPeerSet()
+		peer    = randPeer(nil)
+	)
+
 	assert.Nil(t, peerSet.Get(peer.ID()), "expecting a nil lookup, before .Add")
 
 	if err := peerSet.Add(peer); err != nil {
@@ -150,8 +169,8 @@ func TestPeerSetGet(t *testing.T) {
 		wg.Add(1)
 		go func(i int) {
 			defer wg.Done()
-			got, want := peerSet.Get(peer.ID()), peer
-			assert.Equal(t, got, want, "#%d: got=%v want=%v", i, got, want)
+			have, want := peerSet.Get(peer.ID()), peer
+			assert.Equal(t, have, want, "%d: have %v, want %v", i, have, want)
 		}(i)
 	}
 	wg.Wait()

p2p/peer_test.go

@@ -1,8 +1,6 @@
 package p2p
 
 import (
-	golog "log"
-	"net"
 	"testing"
 	"time"
 
@@ -14,8 +12,6 @@ import (
 	"github.com/tendermint/tmlibs/log"
 )
 
-const testCh = 0x01
-
 func TestPeerBasic(t *testing.T) {
 	assert, require := assert.New(t), require.New(t)
 
@@ -109,71 +105,3 @@ func createOutboundPeerAndPerformHandshake(addr *NetAddress, config *PeerConfig)
 	p.SetLogger(log.TestingLogger().With("peer", addr))
 	return p, nil
 }
-
-type remotePeer struct {
-	PrivKey crypto.PrivKey
-	Config  *PeerConfig
-	addr    *NetAddress
-	quit    chan struct{}
-}
-
-func (p *remotePeer) Addr() *NetAddress {
-	return p.addr
-}
-
-func (p *remotePeer) ID() ID {
-	return PubKeyToID(p.PrivKey.PubKey())
-}
-
-func (p *remotePeer) Start() {
-	l, e := net.Listen("tcp", "127.0.0.1:0") // any available address
-	if e != nil {
-		golog.Fatalf("net.Listen tcp :0: %+v", e)
-	}
-	p.addr = NewNetAddress(PubKeyToID(p.PrivKey.PubKey()), l.Addr())
-	p.quit = make(chan struct{})
-	go p.accept(l)
-}
-
-func (p *remotePeer) Stop() {
-	close(p.quit)
-}
-
-func (p *remotePeer) accept(l net.Listener) {
-	conns := []net.Conn{}
-
-	for {
-		conn, err := l.Accept()
-		if err != nil {
-			golog.Fatalf("Failed to accept conn: %+v", err)
-		}
-		pc, err := newInboundPeerConn(conn, p.Config, p.PrivKey)
-		if err != nil {
-			golog.Fatalf("Failed to create a peer: %+v", err)
-		}
-		_, err = pc.HandshakeTimeout(NodeInfo{
-			ID:         p.Addr().ID,
-			Moniker:    "remote_peer",
-			Network:    "localhost",
-			Version:    "123.123.123",
-			ListenAddr: l.Addr().String(),
-			Channels:   []byte{testCh},
-		}, 1*time.Second)
-		if err != nil {
-			golog.Fatalf("Failed to perform handshake: %+v", err)
-		}
-
-		conns = append(conns, conn)
-
-		select {
-		case <-p.quit:
-			for _, conn := range conns {
-				if err := conn.Close(); err != nil {
-					golog.Fatal(err)
-				}
-			}
-			return
-		default:
-		}
-	}
-}

p2p/test_util.go

@@ -2,6 +2,7 @@ package p2p
 
 import (
 	"fmt"
+	golog "log"
 	"net"
 	"time"
 
@@ -13,6 +14,8 @@ import (
 	"github.com/tendermint/tendermint/p2p/conn"
 )
 
+const testCh = 0x01
+
 func AddPeerToSwitch(sw *Switch, peer Peer) {
 	sw.peers.Add(peer)
 }
@@ -84,8 +87,9 @@ func Connect2Switches(switches []*Switch, i, j int) {
 	switchJ := switches[j]
 
 	p1 := &remotePeer{
-		Config:  switchJ.peerConfig,
-		PrivKey: switchJ.nodeKey.PrivKey,
+		Config:   switchJ.peerConfig,
+		PrivKey:  switchJ.nodeKey.PrivKey,
+		channels: switchJ.NodeInfo().Channels,
 	}
 	p1.Start()
 
@@ -99,8 +103,9 @@ func Connect2Switches(switches []*Switch, i, j int) {
 	}
 
 	p2 := &remotePeer{
-		Config:  switchI.peerConfig,
-		PrivKey: switchI.nodeKey.PrivKey,
+		Config:   switchI.peerConfig,
+		PrivKey:  switchI.nodeKey.PrivKey,
+		channels: switchI.NodeInfo().Channels,
 	}
 	p2.Start()
 
@@ -183,3 +188,77 @@ func MakeSwitch(cfg *cfg.P2PConfig, i int, network, version string, initSwitch f
 	sw.SetNodeKey(nodeKey)
 	return sw
 }
+
+type remotePeer struct {
+	PrivKey  crypto.PrivKey
+	Config   *PeerConfig
+	addr     *NetAddress
+	quit     chan struct{}
+	channels cmn.HexBytes
+}
+
+func (rp *remotePeer) Addr() *NetAddress {
+	return rp.addr
+}
+
+func (rp *remotePeer) ID() ID {
+	return PubKeyToID(rp.PrivKey.PubKey())
+}
+
+func (rp *remotePeer) Start() {
+	l, e := net.Listen("tcp", "127.0.0.1:0") // any available address
+	if e != nil {
+		golog.Fatalf("net.Listen tcp :0: %+v", e)
+	}
+	rp.addr = NewNetAddress(PubKeyToID(rp.PrivKey.PubKey()), l.Addr())
+	rp.quit = make(chan struct{})
+	if rp.channels == nil {
+		rp.channels = []byte{testCh}
+	}
+	go rp.accept(l)
+}
+
+func (rp *remotePeer) Stop() {
+	close(rp.quit)
+}
+
+func (rp *remotePeer) accept(l net.Listener) {
+	conns := []net.Conn{}
+
+	for {
+		conn, err := l.Accept()
+		if err != nil {
+			golog.Fatalf("Failed to accept conn: %+v", err)
+		}
+
+		pc, err := newInboundPeerConn(conn, rp.Config, rp.PrivKey)
+		if err != nil {
+			golog.Fatalf("Failed to create a peer: %+v", err)
+		}
+
+		_, err = pc.HandshakeTimeout(NodeInfo{
+			ID:         rp.Addr().ID,
+			Moniker:    "remote_peer",
+			Network:    "localhost",
+			Version:    "123.123.123",
+			ListenAddr: l.Addr().String(),
+			Channels:   rp.channels,
+		}, 1*time.Second)
+		if err != nil {
+			golog.Fatalf("Failed to perform handshake: %+v", err)
+		}
+
+		conns = append(conns, conn)
+
+		select {
+		case <-rp.quit:
+			for _, conn := range conns {
+				if err := conn.Close(); err != nil {
+					golog.Fatal(err)
+				}
+			}
+			return
+		default:
+		}
+	}
+}