p2p: remove NewNetAddressStringWithOptionalID (#3711)

Fixes #3521

The function NewNetAddressStringWithOptionalID is from a time when peer
IDs were optional. They're not anymore. So this should be renamed to
NewNetAddressString and should ensure the ID is provided.

* update changelog
* use NewNetAddress in transport tests
* use NewNetAddress in TestTransportMultiplexAcceptMultiple

CHANGELOG_PENDING.md

@@ -14,6 +14,7 @@
 - [libs/db] Removed deprecated `LevelDBBackend` const
   * If you have `db_backend` set to `leveldb` in your config file, please
     change it to `goleveldb` or `cleveldb`.
+- [p2p] \#3521 Remove NewNetAddressStringWithOptionalID
 
 * Blockchain Protocol
 

node/node.go

@@ -674,7 +674,7 @@ func (n *Node) OnStart() error {
 	}
 
 	// Start the transport.
-	addr, err := p2p.NewNetAddressStringWithOptionalID(n.config.P2P.ListenAddress)
+	addr, err := p2p.NewNetAddressString(p2p.IDAddressString(n.nodeKey.ID(), n.config.P2P.ListenAddress))
 	if err != nil {
 		return err
 	}

p2p/netaddress.go

@@ -65,36 +65,27 @@ func NewNetAddress(id ID, addr net.Addr) *NetAddress {
 // Also resolves the host if host is not an IP.
 // Errors are of type ErrNetAddressXxx where Xxx is in (NoID, Invalid, Lookup)
 func NewNetAddressString(addr string) (*NetAddress, error) {
-	spl := strings.Split(addr, "@")
-	if len(spl) < 2 {
-		return nil, ErrNetAddressNoID{addr}
-	}
-	return NewNetAddressStringWithOptionalID(addr)
-}
-
-// NewNetAddressStringWithOptionalID returns a new NetAddress using the
-// provided address in the form of "ID@IP:Port", where the ID is optional.
-// Also resolves the host if host is not an IP.
-func NewNetAddressStringWithOptionalID(addr string) (*NetAddress, error) {
 	addrWithoutProtocol := removeProtocolIfDefined(addr)
-
-	var id ID
 	spl := strings.Split(addrWithoutProtocol, "@")
-	if len(spl) == 2 {
-		idStr := spl[0]
-		idBytes, err := hex.DecodeString(idStr)
-		if err != nil {
-			return nil, ErrNetAddressInvalid{addrWithoutProtocol, err}
-		}
-		if len(idBytes) != IDByteLength {
-			return nil, ErrNetAddressInvalid{
-				addrWithoutProtocol,
-				fmt.Errorf("invalid hex length - got %d, expected %d", len(idBytes), IDByteLength)}
-		}
+	if len(spl) != 2 {
+		return nil, ErrNetAddressNoID{addr}
+	}
 
-		id, addrWithoutProtocol = ID(idStr), spl[1]
+	// get ID
+	idStr := spl[0]
+	idBytes, err := hex.DecodeString(idStr)
+	if err != nil {
+		return nil, ErrNetAddressInvalid{addrWithoutProtocol, err}
 	}
+	if len(idBytes) != IDByteLength {
+		return nil, ErrNetAddressInvalid{
+			addrWithoutProtocol,
+			fmt.Errorf("invalid hex length - got %d, expected %d", len(idBytes), IDByteLength)}
+	}
+	var id ID
+	id, addrWithoutProtocol = ID(idStr), spl[1]
 
+	// get host and port
 	host, portStr, err := net.SplitHostPort(addrWithoutProtocol)
 	if err != nil {
 		return nil, ErrNetAddressInvalid{addrWithoutProtocol, err}

p2p/netaddress_test.go

@@ -20,17 +20,23 @@ func TestNewNetAddress(t *testing.T) {
 	}, "Calling NewNetAddress with UDPAddr should not panic in testing")
 }
 
-func TestNewNetAddressStringWithOptionalID(t *testing.T) {
+func TestNewNetAddressString(t *testing.T) {
 	testCases := []struct {
 		name     string
 		addr     string
 		expected string
 		correct  bool
 	}{
-		{"no node id, no protocol", "127.0.0.1:8080", "127.0.0.1:8080", true},
-		{"no node id, tcp input", "tcp://127.0.0.1:8080", "127.0.0.1:8080", true},
-		{"no node id, udp input", "udp://127.0.0.1:8080", "127.0.0.1:8080", true},
-		{"malformed udp input", "udp//127.0.0.1:8080", "", false},
+		{"no node id and no protocol", "127.0.0.1:8080", "", false},
+		{"no node id w/ tcp input", "tcp://127.0.0.1:8080", "", false},
+		{"no node id w/ udp input", "udp://127.0.0.1:8080", "", false},
+
+		{"no protocol", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", true},
+		{"tcp input", "tcp://deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", true},
+		{"udp input", "udp://deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", true},
+		{"malformed tcp input", "tcp//deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "", false},
+		{"malformed udp input", "udp//deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "", false},
+
 		// {"127.0.0:8080", false},
 		{"invalid host", "notahost", "", false},
 		{"invalid port", "127.0.0.1:notapath", "", false},
@@ -41,14 +47,13 @@ func TestNewNetAddressStringWithOptionalID(t *testing.T) {
 		{"too short nodeId", "deadbeef@127.0.0.1:8080", "", false},
 		{"too short, not hex nodeId", "this-isnot-hex@127.0.0.1:8080", "", false},
 		{"not hex nodeId", "xxxxbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "", false},
-		{"correct nodeId", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", true},
 
 		{"too short nodeId w/tcp", "tcp://deadbeef@127.0.0.1:8080", "", false},
 		{"too short notHex nodeId w/tcp", "tcp://this-isnot-hex@127.0.0.1:8080", "", false},
 		{"notHex nodeId w/tcp", "tcp://xxxxbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "", false},
 		{"correct nodeId w/tcp", "tcp://deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", true},
 
-		{"no node id when expected", "tcp://@127.0.0.1:8080", "", false},
+		{"no node id", "tcp://@127.0.0.1:8080", "", false},
 		{"no node id or IP", "tcp://@", "", false},
 		{"tcp no host, w/ port", "tcp://:26656", "", false},
 		{"empty", "", "", false},
@@ -59,7 +64,7 @@ func TestNewNetAddressStringWithOptionalID(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			addr, err := NewNetAddressStringWithOptionalID(tc.addr)
+			addr, err := NewNetAddressString(tc.addr)
 			if tc.correct {
 				if assert.Nil(t, err, tc.addr) {
 					assert.Equal(t, tc.expected, addr.String())
@@ -71,28 +76,6 @@ func TestNewNetAddressStringWithOptionalID(t *testing.T) {
 	}
 }
 
-func TestNewNetAddressString(t *testing.T) {
-	testCases := []struct {
-		addr     string
-		expected string
-		correct  bool
-	}{
-		{"127.0.0.1:8080", "127.0.0.1:8080", false},
-		{"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", true},
-	}
-
-	for _, tc := range testCases {
-		addr, err := NewNetAddressString(tc.addr)
-		if tc.correct {
-			if assert.Nil(t, err, tc.addr) {
-				assert.Equal(t, tc.expected, addr.String())
-			}
-		} else {
-			assert.NotNil(t, err, tc.addr)
-		}
-	}
-}
-
 func TestNewNetAddressStrings(t *testing.T) {
 	addrs, errs := NewNetAddressStrings([]string{
 		"127.0.0.1:8080",
@@ -115,12 +98,12 @@ func TestNetAddressProperties(t *testing.T) {
 		local    bool
 		routable bool
 	}{
-		{"127.0.0.1:8080", true, true, false},
-		{"ya.ru:80", true, false, true},
+		{"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", true, true, false},
+		{"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@ya.ru:80", true, false, true},
 	}
 
 	for _, tc := range testCases {
-		addr, err := NewNetAddressStringWithOptionalID(tc.addr)
+		addr, err := NewNetAddressString(tc.addr)
 		require.Nil(t, err)
 
 		assert.Equal(t, tc.valid, addr.Valid())
@@ -136,15 +119,15 @@ func TestNetAddressReachabilityTo(t *testing.T) {
 		other        string
 		reachability int
 	}{
-		{"127.0.0.1:8080", "127.0.0.1:8081", 0},
-		{"ya.ru:80", "127.0.0.1:8080", 1},
+		{"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8081", 0},
+		{"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@ya.ru:80", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", 1},
 	}
 
 	for _, tc := range testCases {
-		addr, err := NewNetAddressStringWithOptionalID(tc.addr)
+		addr, err := NewNetAddressString(tc.addr)
 		require.Nil(t, err)
 
-		other, err := NewNetAddressStringWithOptionalID(tc.other)
+		other, err := NewNetAddressString(tc.other)
 		require.Nil(t, err)
 
 		assert.Equal(t, tc.reachability, addr.ReachabilityTo(other))

p2p/pex/pex_reactor.go

@@ -345,7 +345,7 @@ func (r *PEXReactor) ReceiveAddrs(addrs []*p2p.NetAddress, src Peer) error {
 		if netAddr == nil {
 			return errors.New("nil address in pexAddrsMessage")
 		}
-		// TODO: extract validating logic from NewNetAddressStringWithOptionalID
+		// TODO: extract validating logic from NewNetAddressString
 		// and put it in netAddr#Valid (#2722)
 		na, err := p2p.NewNetAddressString(netAddr.String())
 		if err != nil {

p2p/transport_test.go

@@ -8,8 +8,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/require"
-
 	"github.com/tendermint/tendermint/crypto/ed25519"
 	"github.com/tendermint/tendermint/p2p/conn"
 )
@@ -39,6 +37,7 @@ func TestTransportMultiplexConnFilter(t *testing.T) {
 			PrivKey: ed25519.GenPrivKey(),
 		},
 	)
+	id := mt.nodeKey.ID()
 
 	MultiplexTransportConnFilters(
 		func(_ ConnSet, _ net.Conn, _ []net.IP) error { return nil },
@@ -48,7 +47,7 @@ func TestTransportMultiplexConnFilter(t *testing.T) {
 		},
 	)(mt)
 
-	addr, err := NewNetAddressStringWithOptionalID("127.0.0.1:0")
+	addr, err := NewNetAddressString(IDAddressString(id, "127.0.0.1:0"))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -60,13 +59,9 @@ func TestTransportMultiplexConnFilter(t *testing.T) {
 	errc := make(chan error)
 
 	go func() {
-		addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
-		if err != nil {
-			errc <- err
-			return
-		}
+		addr := NewNetAddress(id, mt.listener.Addr())
 
-		_, err = addr.Dial()
+		_, err := addr.Dial()
 		if err != nil {
 			errc <- err
 			return
@@ -96,6 +91,7 @@ func TestTransportMultiplexConnFilterTimeout(t *testing.T) {
 			PrivKey: ed25519.GenPrivKey(),
 		},
 	)
+	id := mt.nodeKey.ID()
 
 	MultiplexTransportFilterTimeout(5 * time.Millisecond)(mt)
 	MultiplexTransportConnFilters(
@@ -105,7 +101,7 @@ func TestTransportMultiplexConnFilterTimeout(t *testing.T) {
 		},
 	)(mt)
 
-	addr, err := NewNetAddressStringWithOptionalID("127.0.0.1:0")
+	addr, err := NewNetAddressString(IDAddressString(id, "127.0.0.1:0"))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -117,13 +113,9 @@ func TestTransportMultiplexConnFilterTimeout(t *testing.T) {
 	errc := make(chan error)
 
 	go func() {
-		addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
-		if err != nil {
-			errc <- err
-			return
-		}
+		addr := NewNetAddress(id, mt.listener.Addr())
 
-		_, err = addr.Dial()
+		_, err := addr.Dial()
 		if err != nil {
 			errc <- err
 			return
@@ -144,9 +136,7 @@ func TestTransportMultiplexConnFilterTimeout(t *testing.T) {
 
 func TestTransportMultiplexAcceptMultiple(t *testing.T) {
 	mt := testSetupMultiplexTransport(t)
-	id, addr := mt.nodeKey.ID(), mt.listener.Addr().String()
-	laddr, err := NewNetAddressStringWithOptionalID(IDAddressString(id, addr))
-	require.NoError(t, err)
+	laddr := NewNetAddress(mt.nodeKey.ID(), mt.listener.Addr())
 
 	var (
 		seed     = rand.New(rand.NewSource(time.Now().UnixNano()))
@@ -232,11 +222,7 @@ func TestTransportMultiplexAcceptNonBlocking(t *testing.T) {
 
 	// Simulate slow Peer.
 	go func() {
-		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		if err != nil {
-			errc <- err
-			return
-		}
+		addr := NewNetAddress(mt.nodeKey.ID(), mt.listener.Addr())
 
 		c, err := addr.Dial()
 		if err != nil {
@@ -283,13 +269,9 @@ func TestTransportMultiplexAcceptNonBlocking(t *testing.T) {
 				},
 			)
 		)
-		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		if err != nil {
-			errc <- err
-			return
-		}
+		addr := NewNetAddress(mt.nodeKey.ID(), mt.listener.Addr())
 
-		_, err = dialer.Dial(*addr, peerConfig{})
+		_, err := dialer.Dial(*addr, peerConfig{})
 		if err != nil {
 			errc <- err
 			return
@@ -329,13 +311,9 @@ func TestTransportMultiplexValidateNodeInfo(t *testing.T) {
 			)
 		)
 
-		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		if err != nil {
-			errc <- err
-			return
-		}
+		addr := NewNetAddress(mt.nodeKey.ID(), mt.listener.Addr())
 
-		_, err = dialer.Dial(*addr, peerConfig{})
+		_, err := dialer.Dial(*addr, peerConfig{})
 		if err != nil {
 			errc <- err
 			return
@@ -372,13 +350,9 @@ func TestTransportMultiplexRejectMissmatchID(t *testing.T) {
 				PrivKey: ed25519.GenPrivKey(),
 			},
 		)
-		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		if err != nil {
-			errc <- err
-			return
-		}
+		addr := NewNetAddress(mt.nodeKey.ID(), mt.listener.Addr())
 
-		_, err = dialer.Dial(*addr, peerConfig{})
+		_, err := dialer.Dial(*addr, peerConfig{})
 		if err != nil {
 			errc <- err
 			return
@@ -415,12 +389,9 @@ func TestTransportMultiplexDialRejectWrongID(t *testing.T) {
 	)
 
 	wrongID := PubKeyToID(ed25519.GenPrivKey().PubKey())
-	addr, err := NewNetAddressStringWithOptionalID(IDAddressString(wrongID, mt.listener.Addr().String()))
-	if err != nil {
-		t.Fatalf("invalid address with ID: %v", err)
-	}
+	addr := NewNetAddress(wrongID, mt.listener.Addr())
 
-	_, err = dialer.Dial(*addr, peerConfig{})
+	_, err := dialer.Dial(*addr, peerConfig{})
 	if err != nil {
 		t.Logf("connection failed: %v", err)
 		if err, ok := err.(ErrRejected); ok {
@@ -448,13 +419,9 @@ func TestTransportMultiplexRejectIncompatible(t *testing.T) {
 				},
 			)
 		)
-		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		if err != nil {
-			errc <- err
-			return
-		}
+		addr := NewNetAddress(mt.nodeKey.ID(), mt.listener.Addr())
 
-		_, err = dialer.Dial(*addr, peerConfig{})
+		_, err := dialer.Dial(*addr, peerConfig{})
 		if err != nil {
 			errc <- err
 			return
@@ -479,13 +446,9 @@ func TestTransportMultiplexRejectSelf(t *testing.T) {
 	errc := make(chan error)
 
 	go func() {
-		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		if err != nil {
-			errc <- err
-			return
-		}
+		addr := NewNetAddress(mt.nodeKey.ID(), mt.listener.Addr())
 
-		_, err = mt.Dial(*addr, peerConfig{})
+		_, err := mt.Dial(*addr, peerConfig{})
 		if err != nil {
 			errc <- err
 			return
@@ -609,7 +572,7 @@ func testSetupMultiplexTransport(t *testing.T) *MultiplexTransport {
 		)
 	)
 
-	addr, err := NewNetAddressStringWithOptionalID(IDAddressString(id, "127.0.0.1:0"))
+	addr, err := NewNetAddressString(IDAddressString(id, "127.0.0.1:0"))
 	if err != nil {
 		t.Fatal(err)
 	}