Add private peer ID tracking to AddrBook (#1989)

* Add private peer ID tracking to AddrBook

* Remove private peer tracking/blocking from pex

* debug level msg when we fail to add private address

config/config.go

@@ -284,7 +284,6 @@ type P2PConfig struct {
 	Seeds string `mapstructure:"seeds"`
 
 	// Comma separated list of nodes to keep persistent connections to
-	// Do not add private peers to this list if you don't want them advertised
 	PersistentPeers string `mapstructure:"persistent_peers"`
 
 	// UPNP port forwarding

config/toml.go

@@ -152,7 +152,6 @@ external_address = "{{ .P2P.ExternalAddress }}"
 seeds = "{{ .P2P.Seeds }}"
 
 # Comma separated list of nodes to keep persistent connections to
-# Do not add private peers to this list if you don't want them advertised
 persistent_peers = "{{ .P2P.PersistentPeers }}"
 
 # UPNP port forwarding

docs/tendermint-core/configuration.md

@@ -99,7 +99,6 @@ laddr = "tcp://0.0.0.0:26656"
 seeds = ""
 
 # Comma separated list of nodes to keep persistent connections to
-# Do not add private peers to this list if you don't want them advertised
 persistent_peers = ""
 
 # UPNP port forwarding

node/node.go

@@ -322,9 +322,9 @@ func NewNode(config *cfg.Config,
 		// TODO persistent peers ? so we can have their DNS addrs saved
 		pexReactor := pex.NewPEXReactor(addrBook,
 			&pex.PEXReactorConfig{
-				Seeds:          cmn.SplitAndTrim(config.P2P.Seeds, ",", " "),
-				SeedMode:       config.P2P.SeedMode,
-				PrivatePeerIDs: cmn.SplitAndTrim(config.P2P.PrivatePeerIDs, ",", " ")})
+				Seeds:    cmn.SplitAndTrim(config.P2P.Seeds, ",", " "),
+				SeedMode: config.P2P.SeedMode,
+			})
 		pexReactor.SetLogger(p2pLogger)
 		sw.AddReactor("PEX", pexReactor)
 	}
@@ -449,6 +449,9 @@ func (n *Node) OnStart() error {
 	// Add ourselves to addrbook to prevent dialing ourselves
 	n.addrBook.AddOurAddress(nodeInfo.NetAddress())
 
+	// Add private IDs to addrbook to block those peers being added
+	n.addrBook.AddPrivateIDs(cmn.SplitAndTrim(n.config.P2P.PrivatePeerIDs, ",", " "))
+
 	// Start the RPC server before the P2P server
 	// so we can eg. receive txs for the first block
 	if n.config.RPC.ListenAddress != "" {

p2p/pex/addrbook.go

@@ -13,8 +13,8 @@ import (
 	"time"
 
 	crypto "github.com/tendermint/tendermint/crypto"
-	"github.com/tendermint/tendermint/p2p"
 	cmn "github.com/tendermint/tendermint/libs/common"
+	"github.com/tendermint/tendermint/p2p"
 )
 
 const (
@@ -34,6 +34,8 @@ type AddrBook interface {
 	// Check if it is our address
 	OurAddress(*p2p.NetAddress) bool
 
+	AddPrivateIDs([]string)
+
 	// Add and remove an address
 	AddAddress(addr *p2p.NetAddress, src *p2p.NetAddress) error
 	RemoveAddress(*p2p.NetAddress)
@@ -82,6 +84,7 @@ type addrBook struct {
 	mtx        sync.Mutex
 	rand       *cmn.Rand
 	ourAddrs   map[string]struct{}
+	privateIDs map[p2p.ID]struct{}
 	addrLookup map[p2p.ID]*knownAddress // new & old
 	bucketsOld []map[string]*knownAddress
 	bucketsNew []map[string]*knownAddress
@@ -97,6 +100,7 @@ func NewAddrBook(filePath string, routabilityStrict bool) *addrBook {
 	am := &addrBook{
 		rand:              cmn.NewRand(),
 		ourAddrs:          make(map[string]struct{}),
+		privateIDs:        make(map[p2p.ID]struct{}),
 		addrLookup:        make(map[p2p.ID]*knownAddress),
 		filePath:          filePath,
 		routabilityStrict: routabilityStrict,
@@ -168,6 +172,14 @@ func (a *addrBook) OurAddress(addr *p2p.NetAddress) bool {
 	return ok
 }
 
+func (a *addrBook) AddPrivateIDs(IDs []string) {
+	a.mtx.Lock()
+	defer a.mtx.Unlock()
+	for _, id := range IDs {
+		a.privateIDs[p2p.ID(id)] = struct{}{}
+	}
+}
+
 // AddAddress implements AddrBook
 // Add address to a "new" bucket. If it's already in one, only add it probabilistically.
 // Returns error if the addr is non-routable. Does not add self.
@@ -631,6 +643,10 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error {
 		return ErrAddrBookSelf{addr}
 	}
 
+	if _, ok := a.privateIDs[addr.ID]; ok {
+		return ErrAddrBookPrivate{addr}
+	}
+
 	ka := a.addrLookup[addr.ID]
 	if ka != nil {
 		// If its already old and the addr is the same, ignore it.

p2p/pex/addrbook_test.go

@@ -8,6 +8,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+  
 	cmn "github.com/tendermint/tendermint/libs/common"
 	"github.com/tendermint/tendermint/libs/log"
 	"github.com/tendermint/tendermint/p2p"
@@ -353,3 +355,29 @@ func TestAddrBookHasAddress(t *testing.T) {
 
 	assert.False(t, book.HasAddress(addr))
 }
+
+func TestPrivatePeers(t *testing.T) {
+	fname := createTempFileName("addrbook_test")
+	defer deleteTempFile(fname)
+
+	book := NewAddrBook(fname, true)
+	book.SetLogger(log.TestingLogger())
+
+	addrs := make([]*p2p.NetAddress, 10)
+	for i := 0; i < 10; i++ {
+		addrs[i] = randIPv4Address(t)
+	}
+
+	private := make([]string, 10)
+	for i, addr := range addrs {
+		private[i] = string(addr.ID)
+	}
+	book.AddPrivateIDs(private)
+
+	for _, addr := range addrs {
+		err := book.AddAddress(addr, addr)
+		require.Error(t, err, "AddAddress should have failed with private peer %s", addr)
+		_, ok := err.(ErrAddrBookPrivate)
+		require.True(t, ok, "Wrong error type, wanted ErrAddrBookPrivate, got error: %s", err)
+	}
+}

p2p/pex/errors.go

@@ -22,6 +22,14 @@ func (err ErrAddrBookSelf) Error() string {
 	return fmt.Sprintf("Cannot add ourselves with address %v", err.Addr)
 }
 
+type ErrAddrBookPrivate struct {
+	Addr *p2p.NetAddress
+}
+
+func (err ErrAddrBookPrivate) Error() string {
+	return fmt.Sprintf("Cannot add private peer with address %v", err.Addr)
+}
+
 type ErrAddrBookNilAddr struct {
 	Addr *p2p.NetAddress
 	Src  *p2p.NetAddress

p2p/pex/pex_reactor.go

@@ -91,10 +91,6 @@ type PEXReactorConfig struct {
 	// Seeds is a list of addresses reactor may use
 	// if it can't connect to peers in the addrbook.
 	Seeds []string
-
-	// PrivatePeerIDs is a list of peer IDs, which must not be gossiped to other
-	// peers.
-	PrivatePeerIDs []string
 }
 
 type _attemptsToDial struct {
@@ -173,11 +169,6 @@ func (r *PEXReactor) AddPeer(p Peer) {
 		addr := p.NodeInfo().NetAddress()
 		src := addr
 
-		// ignore private addrs
-		if isAddrPrivate(addr, r.config.PrivatePeerIDs) {
-			return
-		}
-
 		// add to book. dont RequestAddrs right away because
 		// we don't trust inbound as much - let ensurePeersRoutine handle it.
 		err := r.book.AddAddress(addr, src)
@@ -191,7 +182,7 @@ func (r *PEXReactor) logErrAddrBook(err error) {
 		case ErrAddrBookNilAddr:
 			r.Logger.Error("Failed to add new address", "err", err)
 		default:
-			// non-routable, self, full book, etc.
+			// non-routable, self, full book, private, etc.
 			r.Logger.Debug("Failed to add new address", "err", err)
 		}
 	}
@@ -308,14 +299,6 @@ func (r *PEXReactor) ReceiveAddrs(addrs []*p2p.NetAddress, src Peer) error {
 			return cmn.NewError("received nil addr")
 		}
 
-		// ignore private peers
-		// TODO: give private peers to AddrBook so it can enforce this on AddAddress.
-		// We'd then have to check for ErrPrivatePeer on AddAddress here, which is
-		// an error we just ignore (maybe peer is probing us for our private peers :P)
-		if isAddrPrivate(netAddr, r.config.PrivatePeerIDs) {
-			continue
-		}
-
 		err := r.book.AddAddress(netAddr, srcAddr)
 		r.logErrAddrBook(err)
 	}
@@ -647,16 +630,6 @@ func (r *PEXReactor) attemptDisconnects() {
 	}
 }
 
-// isAddrPrivate returns true if addr.ID is a private ID.
-func isAddrPrivate(addr *p2p.NetAddress, privatePeerIDs []string) bool {
-	for _, id := range privatePeerIDs {
-		if string(addr.ID) == id {
-			return true
-		}
-	}
-	return false
-}
-
 //-----------------------------------------------------------------------------
 // Messages
 

p2p/pex/pex_reactor_test.go

@@ -295,7 +295,8 @@ func TestPEXReactorCrawlStatus(t *testing.T) {
 func TestPEXReactorDoesNotAddPrivatePeersToAddrBook(t *testing.T) {
 	peer := p2p.CreateRandomPeer(false)
 
-	pexR, book := createReactor(&PEXReactorConfig{PrivatePeerIDs: []string{string(peer.NodeInfo().ID)}})
+	pexR, book := createReactor(&PEXReactorConfig{})
+	book.AddPrivateIDs([]string{string(peer.NodeInfo().ID)})
 	defer teardownReactor(book)
 
 	// we have to send a request to receive responses