p2p: extract ID validation into a separate func (#3754)

* p2p: extract ID validation into a separate func

- NewNetAddress panics if ID is invalid
- NetAddress#Valid returns an error
- remove ErrAddrBookInvalidAddrNoID

Fixes #2722

* p2p: remove repetitive check in ReceiveAddrs

* fix netaddress test

p2p/netaddress.go

@@ -13,7 +13,7 @@ import (
 	"strings"
 	"time"
 
-	"errors"
+	"github.com/pkg/errors"
 )
 
 // NetAddress defines information about a peer on the network
@@ -40,7 +40,7 @@ func IDAddressString(id ID, protocolHostPort string) string {
 // NewNetAddress returns a new NetAddress using the provided TCP
 // address. When testing, other net.Addr (except TCP) will result in
 // using 0.0.0.0:0. When normal run, other net.Addr (except TCP) will
-// panic.
+// panic. Panics if ID is invalid.
 // TODO: socks proxies?
 func NewNetAddress(id ID, addr net.Addr) *NetAddress {
 	tcpAddr, ok := addr.(*net.TCPAddr)
@@ -53,6 +53,11 @@ func NewNetAddress(id ID, addr net.Addr) *NetAddress {
 			return netAddr
 		}
 	}
+
+	if err := validateID(id); err != nil {
+		panic(fmt.Sprintf("Invalid ID %v: %v (addr: %v)", id, err, addr))
+	}
+
 	ip := tcpAddr.IP
 	port := uint16(tcpAddr.Port)
 	na := NewNetAddressIPPort(ip, port)
@@ -72,18 +77,11 @@ func NewNetAddressString(addr string) (*NetAddress, error) {
 	}
 
 	// get ID
-	idStr := spl[0]
-	idBytes, err := hex.DecodeString(idStr)
-	if err != nil {
+	if err := validateID(ID(spl[0])); 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]
+	id, addrWithoutProtocol = ID(spl[0]), spl[1]
 
 	// get host and port
 	host, portStr, err := net.SplitHostPort(addrWithoutProtocol)
@@ -207,22 +205,28 @@ func (na *NetAddress) DialTimeout(timeout time.Duration) (net.Conn, error) {
 
 // Routable returns true if the address is routable.
 func (na *NetAddress) Routable() bool {
+	if err := na.Valid(); err != nil {
+		return false
+	}
 	// TODO(oga) bitcoind doesn't include RFC3849 here, but should we?
-	return na.Valid() && !(na.RFC1918() || na.RFC3927() || na.RFC4862() ||
+	return !(na.RFC1918() || na.RFC3927() || na.RFC4862() ||
 		na.RFC4193() || na.RFC4843() || na.Local())
 }
 
 // For IPv4 these are either a 0 or all bits set address. For IPv6 a zero
 // address or one that matches the RFC3849 documentation address format.
-func (na *NetAddress) Valid() bool {
-	if string(na.ID) != "" {
-		data, err := hex.DecodeString(string(na.ID))
-		if err != nil || len(data) != IDByteLength {
-			return false
-		}
+func (na *NetAddress) Valid() error {
+	if err := validateID(na.ID); err != nil {
+		return errors.Wrap(err, "invalid ID")
 	}
-	return na.IP != nil && !(na.IP.IsUnspecified() || na.RFC3849() ||
-		na.IP.Equal(net.IPv4bcast))
+
+	if na.IP == nil {
+		return errors.New("no IP")
+	}
+	if na.IP.IsUnspecified() || na.RFC3849() || na.IP.Equal(net.IPv4bcast) {
+		return errors.New("invalid IP")
+	}
+	return nil
 }
 
 // HasID returns true if the address has an ID.
@@ -329,3 +333,17 @@ func removeProtocolIfDefined(addr string) string {
 	return addr
 
 }
+
+func validateID(id ID) error {
+	if len(id) == 0 {
+		return errors.New("no ID")
+	}
+	idBytes, err := hex.DecodeString(string(id))
+	if err != nil {
+		return err
+	}
+	if len(idBytes) != IDByteLength {
+		return fmt.Errorf("invalid hex length - got %d, expected %d", len(idBytes), IDByteLength)
+	}
+	return nil
+}

p2p/netaddress_test.go

@@ -11,9 +11,13 @@ import (
 func TestNewNetAddress(t *testing.T) {
 	tcpAddr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:8080")
 	require.Nil(t, err)
-	addr := NewNetAddress("", tcpAddr)
 
-	assert.Equal(t, "127.0.0.1:8080", addr.String())
+	assert.Panics(t, func() {
+		NewNetAddress("", tcpAddr)
+	})
+
+	addr := NewNetAddress("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", tcpAddr)
+	assert.Equal(t, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", addr.String())
 
 	assert.NotPanics(t, func() {
 		NewNetAddress("", &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8000})
@@ -106,7 +110,12 @@ func TestNetAddressProperties(t *testing.T) {
 		addr, err := NewNetAddressString(tc.addr)
 		require.Nil(t, err)
 
-		assert.Equal(t, tc.valid, addr.Valid())
+		err = addr.Valid()
+		if tc.valid {
+			assert.NoError(t, err)
+		} else {
+			assert.Error(t, err)
+		}
 		assert.Equal(t, tc.local, addr.Local())
 		assert.Equal(t, tc.routable, addr.Routable())
 	}

p2p/pex/addrbook.go

@@ -586,8 +586,8 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error {
 		return ErrAddrBookNilAddr{addr, src}
 	}
 
-	if !addr.HasID() {
-		return ErrAddrBookInvalidAddrNoID{addr}
+	if err := addr.Valid(); err != nil {
+		return ErrAddrBookInvalidAddr{Addr: addr, AddrErr: err}
 	}
 
 	if _, ok := a.privateIDs[addr.ID]; ok {
@@ -607,10 +607,6 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error {
 		return ErrAddrBookNonRoutable{addr}
 	}
 
-	if !addr.Valid() {
-		return ErrAddrBookInvalidAddr{addr}
-	}
-
 	ka := a.addrLookup[addr.ID]
 	if ka != nil {
 		// If its already old and the addr is the same, ignore it.

p2p/pex/errors.go

@@ -56,17 +56,10 @@ func (err ErrAddrBookNilAddr) Error() string {
 }
 
 type ErrAddrBookInvalidAddr struct {
-	Addr *p2p.NetAddress
+	Addr    *p2p.NetAddress
+	AddrErr error
 }
 
 func (err ErrAddrBookInvalidAddr) Error() string {
-	return fmt.Sprintf("Cannot add invalid address %v", err.Addr)
-}
-
-type ErrAddrBookInvalidAddrNoID struct {
-	Addr *p2p.NetAddress
-}
-
-func (err ErrAddrBookInvalidAddrNoID) Error() string {
-	return fmt.Sprintf("Cannot add address with no ID %v", err.Addr)
+	return fmt.Sprintf("Cannot add invalid address %v: %v", err.Addr, err.AddrErr)
 }

p2p/pex/pex_reactor.go

@@ -350,22 +350,8 @@ func (r *PEXReactor) ReceiveAddrs(addrs []*p2p.NetAddress, src Peer) error {
 	}
 
 	for _, netAddr := range addrs {
-		// Validate netAddr. Disconnect from a peer if it sends us invalid data.
-		if netAddr == nil {
-			return errors.New("nil address in pexAddrsMessage")
-		}
-		// TODO: extract validating logic from NewNetAddressString
-		// and put it in netAddr#Valid (#2722)
-		na, err := p2p.NewNetAddressString(netAddr.String())
-		if err != nil {
-			return fmt.Errorf("%s address in pexAddrsMessage is invalid: %v",
-				netAddr.String(),
-				err,
-			)
-		}
-
 		// NOTE: we check netAddr validity and routability in book#AddAddress.
-		err = r.book.AddAddress(na, srcAddr)
+		err = r.book.AddAddress(netAddr, srcAddr)
 		if err != nil {
 			r.logErrAddrBook(err)
 			// XXX: should we be strict about incoming data and disconnect from a