Merge pull request #1352 from tendermint/1228-require-id

p2p: require all addresses come with an ID no matter what

CHANGELOG.md

@@ -25,16 +25,19 @@ BUG FIXES:
 - Graceful handling/recovery for apps that have non-determinism or fail to halt
 - Graceful handling/recovery for violations of safety, or liveness
 
-## 0.17.2 (TBD)
+## 0.18.0 (TBD)
 
-BUG FIXES:
-- [rpc] fix subscribing using an abci.ResponseDeliverTx tag
+BREAKING:
+- [p2p] require all addresses come with an ID no matter what
 
 IMPROVEMENTS:
 - [p2p] seeds respond with a bias towards good peers
 - [rpc] `/tx` and `/tx_search` responses now include the transaction hash
 - [rpc] include validator power in `/status`
 
+BUG FIXES:
+- [rpc] fix subscribing using an abci.ResponseDeliverTx tag
+
 ## 0.17.1 (March 27th, 2018)
 
 BUG FIXES:

p2p/listener.go

@@ -72,7 +72,7 @@ func NewDefaultListener(protocol string, lAddr string, skipUPNP bool, logger log
 
 	// Determine internal address...
 	var intAddr *NetAddress
-	intAddr, err = NewNetAddressString(lAddr)
+	intAddr, err = NewNetAddressStringWithOptionalID(lAddr)
 	if err != nil {
 		panic(err)
 	}

p2p/netaddress.go

@@ -49,33 +49,45 @@ func NewNetAddress(id ID, addr net.Addr) *NetAddress {
 	}
 	ip := tcpAddr.IP
 	port := uint16(tcpAddr.Port)
-	netAddr := NewNetAddressIPPort(ip, port)
-	netAddr.ID = id
-	return netAddr
+	na := NewNetAddressIPPort(ip, port)
+	na.ID = id
+	return na
 }
 
-// NewNetAddressString returns a new NetAddress using the provided
-// address in the form of "ID@IP:Port", where the ID is optional.
+// NewNetAddressString returns a new NetAddress using the provided address in
+// the form of "ID@IP:Port".
 // Also resolves the host if host is not an IP.
 func NewNetAddressString(addr string) (*NetAddress, error) {
-	addr = removeProtocolIfDefined(addr)
+	spl := strings.Split(addr, "@")
+	if len(spl) < 2 {
+		return nil, fmt.Errorf("Address (%s) does not contain ID", 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(addr, "@")
+	spl := strings.Split(addrWithoutProtocol, "@")
 	if len(spl) == 2 {
 		idStr := spl[0]
 		idBytes, err := hex.DecodeString(idStr)
 		if err != nil {
-			return nil, errors.Wrap(err, fmt.Sprintf("Address (%s) contains invalid ID", addr))
+			return nil, errors.Wrapf(err, "Address (%s) contains invalid ID", addrWithoutProtocol)
 		}
 		if len(idBytes) != IDByteLength {
 			return nil, fmt.Errorf("Address (%s) contains ID of invalid length (%d). Should be %d hex-encoded bytes",
-				addr, len(idBytes), IDByteLength)
+				addrWithoutProtocol, len(idBytes), IDByteLength)
 		}
-		id, addr = ID(idStr), spl[1]
+
+		id, addrWithoutProtocol = ID(idStr), spl[1]
 	}
 
-	host, portStr, err := net.SplitHostPort(addr)
+	host, portStr, err := net.SplitHostPort(addrWithoutProtocol)
 	if err != nil {
 		return nil, err
 	}
@@ -120,11 +132,10 @@ func NewNetAddressStrings(addrs []string) ([]*NetAddress, []error) {
 // NewNetAddressIPPort returns a new NetAddress using the provided IP
 // and port number.
 func NewNetAddressIPPort(ip net.IP, port uint16) *NetAddress {
-	na := &NetAddress{
+	return &NetAddress{
 		IP:   ip,
 		Port: port,
 	}
-	return na
 }
 
 // Equals reports whether na and other are the same addresses,

p2p/netaddress_test.go

@@ -9,20 +9,18 @@ import (
 )
 
 func TestNewNetAddress(t *testing.T) {
-	assert, require := assert.New(t), require.New(t)
-
 	tcpAddr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:8080")
-	require.Nil(err)
+	require.Nil(t, err)
 	addr := NewNetAddress("", tcpAddr)
 
-	assert.Equal("127.0.0.1:8080", addr.String())
+	assert.Equal(t, "127.0.0.1:8080", addr.String())
 
-	assert.NotPanics(func() {
+	assert.NotPanics(t, func() {
 		NewNetAddress("", &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8000})
 	}, "Calling NewNetAddress with UDPAddr should not panic in testing")
 }
 
-func TestNewNetAddressString(t *testing.T) {
+func TestNewNetAddressStringWithOptionalID(t *testing.T) {
 	testCases := []struct {
 		addr     string
 		expected string
@@ -57,6 +55,28 @@ func TestNewNetAddressString(t *testing.T) {
 		{" @ ", "", false},
 	}
 
+	for _, tc := range testCases {
+		addr, err := NewNetAddressStringWithOptionalID(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 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 {
@@ -70,23 +90,22 @@ func TestNewNetAddressString(t *testing.T) {
 }
 
 func TestNewNetAddressStrings(t *testing.T) {
-	addrs, errs := NewNetAddressStrings([]string{"127.0.0.1:8080", "127.0.0.2:8080"})
-	assert.Len(t, errs, 0)
+	addrs, errs := NewNetAddressStrings([]string{
+		"127.0.0.1:8080",
+		"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080",
+		"deadbeefdeadbeefdeadbeefdeadbeefdeadbeed@127.0.0.2:8080"})
+	assert.Len(t, errs, 1)
 	assert.Equal(t, 2, len(addrs))
 }
 
 func TestNewNetAddressIPPort(t *testing.T) {
-	assert := assert.New(t)
 	addr := NewNetAddressIPPort(net.ParseIP("127.0.0.1"), 8080)
-
-	assert.Equal("127.0.0.1:8080", addr.String())
+	assert.Equal(t, "127.0.0.1:8080", addr.String())
 }
 
 func TestNetAddressProperties(t *testing.T) {
-	assert, require := assert.New(t), require.New(t)
-
 	// TODO add more test cases
-	tests := []struct {
+	testCases := []struct {
 		addr     string
 		valid    bool
 		local    bool
@@ -96,21 +115,19 @@ func TestNetAddressProperties(t *testing.T) {
 		{"ya.ru:80", true, false, true},
 	}
 
-	for _, t := range tests {
-		addr, err := NewNetAddressString(t.addr)
-		require.Nil(err)
+	for _, tc := range testCases {
+		addr, err := NewNetAddressStringWithOptionalID(tc.addr)
+		require.Nil(t, err)
 
-		assert.Equal(t.valid, addr.Valid())
-		assert.Equal(t.local, addr.Local())
-		assert.Equal(t.routable, addr.Routable())
+		assert.Equal(t, tc.valid, addr.Valid())
+		assert.Equal(t, tc.local, addr.Local())
+		assert.Equal(t, tc.routable, addr.Routable())
 	}
 }
 
 func TestNetAddressReachabilityTo(t *testing.T) {
-	assert, require := assert.New(t), require.New(t)
-
 	// TODO add more test cases
-	tests := []struct {
+	testCases := []struct {
 		addr         string
 		other        string
 		reachability int
@@ -119,13 +136,13 @@ func TestNetAddressReachabilityTo(t *testing.T) {
 		{"ya.ru:80", "127.0.0.1:8080", 1},
 	}
 
-	for _, t := range tests {
-		addr, err := NewNetAddressString(t.addr)
-		require.Nil(err)
+	for _, tc := range testCases {
+		addr, err := NewNetAddressStringWithOptionalID(tc.addr)
+		require.Nil(t, err)
 
-		other, err := NewNetAddressString(t.other)
-		require.Nil(err)
+		other, err := NewNetAddressStringWithOptionalID(tc.other)
+		require.Nil(t, err)
 
-		assert.Equal(t.reachability, addr.ReachabilityTo(other))
+		assert.Equal(t, tc.reachability, addr.ReachabilityTo(other))
 	}
 }