Merge pull request #3030 from tendermint/release/v0.27.2

Release/v0.27.2

CHANGELOG.md

@@ -1,5 +1,17 @@
 # Changelog
 
+## v0.27.2
+
+*December 16th, 2018*
+
+### IMPROVEMENTS:
+
+- [node] [\#3025](https://github.com/tendermint/tendermint/issues/3025) Validate NodeInfo addresses on startup.
+
+### BUG FIXES:
+
+- [p2p] [\#3025](https://github.com/tendermint/tendermint/pull/3025) Revert to using defers in addrbook.  Fixes deadlocks in pex and consensus upon invalid ExternalAddr/ListenAddr configuration.
+
 ## v0.27.1
 
 *December 15th, 2018*

CHANGELOG_PENDING.md

@@ -1,4 +1,4 @@
-## v0.27.2
+## v0.27.3
 
 *TBD*
 
@@ -21,3 +21,4 @@ Special thanks to external contributors on this release:
 ### IMPROVEMENTS:
 
 ### BUG FIXES:
+

node/node.go

@@ -348,20 +348,21 @@ func NewNode(config *cfg.Config,
 	indexerService := txindex.NewIndexerService(txIndexer, eventBus)
 	indexerService.SetLogger(logger.With("module", "txindex"))
 
-	var (
-		p2pLogger = logger.With("module", "p2p")
-		nodeInfo  = makeNodeInfo(
-			config,
-			nodeKey.ID(),
-			txIndexer,
-			genDoc.ChainID,
-			p2p.NewProtocolVersion(
-				version.P2PProtocol, // global
-				state.Version.Consensus.Block,
-				state.Version.Consensus.App,
-			),
-		)
+	p2pLogger := logger.With("module", "p2p")
+	nodeInfo, err := makeNodeInfo(
+		config,
+		nodeKey.ID(),
+		txIndexer,
+		genDoc.ChainID,
+		p2p.NewProtocolVersion(
+			version.P2PProtocol, // global
+			state.Version.Consensus.Block,
+			state.Version.Consensus.App,
+		),
 	)
+	if err != nil {
+		return nil, err
+	}
 
 	// Setup Transport.
 	var (
@@ -782,7 +783,7 @@ func makeNodeInfo(
 	txIndexer txindex.TxIndexer,
 	chainID string,
 	protocolVersion p2p.ProtocolVersion,
-) p2p.NodeInfo {
+) (p2p.NodeInfo, error) {
 	txIndexerStatus := "on"
 	if _, ok := txIndexer.(*null.TxIndex); ok {
 		txIndexerStatus = "off"
@@ -817,7 +818,8 @@ func makeNodeInfo(
 
 	nodeInfo.ListenAddr = lAddr
 
-	return nodeInfo
+	err := nodeInfo.Validate()
+	return nodeInfo, err
 }
 
 //------------------------------------------------------------------------------

p2p/netaddress.go

@@ -175,6 +175,9 @@ func (na *NetAddress) Same(other interface{}) bool {
 
 // String representation: <ID>@<IP>:<PORT>
 func (na *NetAddress) String() string {
+	if na == nil {
+		return "<nil-NetAddress>"
+	}
 	if na.str == "" {
 		addrStr := na.DialString()
 		if na.ID != "" {
@@ -186,6 +189,9 @@ func (na *NetAddress) String() string {
 }
 
 func (na *NetAddress) DialString() string {
+	if na == nil {
+		return "<nil-NetAddress>"
+	}
 	return net.JoinHostPort(
 		na.IP.String(),
 		strconv.FormatUint(uint64(na.Port), 10),

p2p/node_info.go

@@ -36,7 +36,7 @@ type nodeInfoAddress interface {
 // nodeInfoTransport validates a nodeInfo and checks
 // our compatibility with it. It's for use in the handshake.
 type nodeInfoTransport interface {
-	ValidateBasic() error
+	Validate() error
 	CompatibleWith(other NodeInfo) error
 }
 
@@ -103,7 +103,7 @@ func (info DefaultNodeInfo) ID() ID {
 	return info.ID_
 }
 
-// ValidateBasic checks the self-reported DefaultNodeInfo is safe.
+// Validate checks the self-reported DefaultNodeInfo is safe.
 // It returns an error if there
 // are too many Channels, if there are any duplicate Channels,
 // if the ListenAddr is malformed, or if the ListenAddr is a host name
@@ -116,7 +116,7 @@ func (info DefaultNodeInfo) ID() ID {
 // International clients could then use punycode (or we could use
 // url-encoding), and we just need to be careful with how we handle that in our
 // clients. (e.g. off by default).
-func (info DefaultNodeInfo) ValidateBasic() error {
+func (info DefaultNodeInfo) Validate() error {
 
 	// ID is already validated.
 

p2p/node_info_test.go

@@ -12,7 +12,7 @@ func TestNodeInfoValidate(t *testing.T) {
 
 	// empty fails
 	ni := DefaultNodeInfo{}
-	assert.Error(t, ni.ValidateBasic())
+	assert.Error(t, ni.Validate())
 
 	channels := make([]byte, maxNumChannels)
 	for i := 0; i < maxNumChannels; i++ {
@@ -68,13 +68,13 @@ func TestNodeInfoValidate(t *testing.T) {
 	// test case passes
 	ni = testNodeInfo(nodeKey.ID(), name).(DefaultNodeInfo)
 	ni.Channels = channels
-	assert.NoError(t, ni.ValidateBasic())
+	assert.NoError(t, ni.Validate())
 
 	for _, tc := range testCases {
 		ni := testNodeInfo(nodeKey.ID(), name).(DefaultNodeInfo)
 		ni.Channels = channels
 		tc.malleateNodeInfo(&ni)
-		err := ni.ValidateBasic()
+		err := ni.Validate()
 		if tc.expectErr {
 			assert.Error(t, err, tc.testName)
 		} else {

p2p/pex/addrbook.go

@@ -162,26 +162,29 @@ func (a *addrBook) FilePath() string {
 
 // AddOurAddress one of our addresses.
 func (a *addrBook) AddOurAddress(addr *p2p.NetAddress) {
-	a.Logger.Info("Add our address to book", "addr", addr)
 	a.mtx.Lock()
+	defer a.mtx.Unlock()
+
+	a.Logger.Info("Add our address to book", "addr", addr)
 	a.ourAddrs[addr.String()] = struct{}{}
-	a.mtx.Unlock()
 }
 
 // OurAddress returns true if it is our address.
 func (a *addrBook) OurAddress(addr *p2p.NetAddress) bool {
 	a.mtx.Lock()
+	defer a.mtx.Unlock()
+
 	_, ok := a.ourAddrs[addr.String()]
-	a.mtx.Unlock()
 	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{}{}
 	}
-	a.mtx.Unlock()
 }
 
 // AddAddress implements AddrBook
@@ -191,6 +194,7 @@ func (a *addrBook) AddPrivateIDs(IDs []string) {
 func (a *addrBook) AddAddress(addr *p2p.NetAddress, src *p2p.NetAddress) error {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
+
 	return a.addAddress(addr, src)
 }
 
@@ -198,6 +202,7 @@ func (a *addrBook) AddAddress(addr *p2p.NetAddress, src *p2p.NetAddress) error {
 func (a *addrBook) RemoveAddress(addr *p2p.NetAddress) {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
+
 	ka := a.addrLookup[addr.ID]
 	if ka == nil {
 		return
@@ -211,14 +216,16 @@ func (a *addrBook) RemoveAddress(addr *p2p.NetAddress) {
 func (a *addrBook) IsGood(addr *p2p.NetAddress) bool {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
+
 	return a.addrLookup[addr.ID].isOld()
 }
 
 // HasAddress returns true if the address is in the book.
 func (a *addrBook) HasAddress(addr *p2p.NetAddress) bool {
 	a.mtx.Lock()
+	defer a.mtx.Unlock()
+
 	ka := a.addrLookup[addr.ID]
-	a.mtx.Unlock()
 	return ka != nil
 }
 
@@ -292,6 +299,7 @@ func (a *addrBook) PickAddress(biasTowardsNewAddrs int) *p2p.NetAddress {
 func (a *addrBook) MarkGood(addr *p2p.NetAddress) {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
+
 	ka := a.addrLookup[addr.ID]
 	if ka == nil {
 		return
@@ -306,6 +314,7 @@ func (a *addrBook) MarkGood(addr *p2p.NetAddress) {
 func (a *addrBook) MarkAttempt(addr *p2p.NetAddress) {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
+
 	ka := a.addrLookup[addr.ID]
 	if ka == nil {
 		return
@@ -461,12 +470,13 @@ ADDRS_LOOP:
 
 // ListOfKnownAddresses returns the new and old addresses.
 func (a *addrBook) ListOfKnownAddresses() []*knownAddress {
-	addrs := []*knownAddress{}
 	a.mtx.Lock()
+	defer a.mtx.Unlock()
+
+	addrs := []*knownAddress{}
 	for _, addr := range a.addrLookup {
 		addrs = append(addrs, addr.copy())
 	}
-	a.mtx.Unlock()
 	return addrs
 }
 
@@ -476,6 +486,7 @@ func (a *addrBook) ListOfKnownAddresses() []*knownAddress {
 func (a *addrBook) Size() int {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
+
 	return a.size()
 }
 

p2p/test_util.go

@@ -24,7 +24,7 @@ type mockNodeInfo struct {
 
 func (ni mockNodeInfo) ID() ID                              { return ni.addr.ID }
 func (ni mockNodeInfo) NetAddress() *NetAddress             { return ni.addr }
-func (ni mockNodeInfo) ValidateBasic() error                { return nil }
+func (ni mockNodeInfo) Validate() error                     { return nil }
 func (ni mockNodeInfo) CompatibleWith(other NodeInfo) error { return nil }
 
 func AddPeerToSwitch(sw *Switch, peer Peer) {

p2p/transport.go

@@ -350,7 +350,7 @@ func (mt *MultiplexTransport) upgrade(
 		}
 	}
 
-	if err := nodeInfo.ValidateBasic(); err != nil {
+	if err := nodeInfo.Validate(); err != nil {
 		return nil, nil, ErrRejected{
 			conn:              c,
 			err:               err,

version/version.go

@@ -18,7 +18,7 @@ const (
 	// TMCoreSemVer is the current version of Tendermint Core.
 	// It's the Semantic Version of the software.
 	// Must be a string because scripts like dist.sh read this file.
-	TMCoreSemVer = "0.27.1"
+	TMCoreSemVer = "0.27.2"
 
 	// ABCISemVer is the semantic version of the ABCI library
 	ABCISemVer  = "0.15.0"