Merge pull request #3571 from tendermint/v0.31

V0.31.5

.gitignore

@@ -35,6 +35,7 @@ shunit2
 addrbook.json
 
 */vendor
+.vendor-new/
 */.glide
 .terraform
 terraform.tfstate

CHANGELOG.md

@@ -1,5 +1,28 @@
 # Changelog
 
+## v0.31.5
+
+*April 16th, 2019*
+
+This release fixes a regression from v0.31.4 where, in existing chains that
+were upgraded, `/validators` could return an empty validator set. This is true
+for almost all heights, given the validator set remains the same.
+
+Special thanks to external contributors on this release:
+@brapse, @guagualvcha, @dongsam, @phucc
+
+### IMPROVEMENTS:
+
+- [libs/common] `CMap`: slight optimization in `Keys()` and `Values()` (@phucc)
+- [gitignore] gitignore: add .vendor-new (@dongsam)
+
+### BUG FIXES:
+
+- [state] [\#3537](https://github.com/tendermint/tendermint/pull/3537#issuecomment-482711833)
+  `LoadValidators`: do not return an empty validator set
+- [blockchain] [\#3457](https://github.com/tendermint/tendermint/issues/3457)
+  Fix "peer did not send us anything" in `fast_sync` mode when under high pressure
+
 ## v0.31.4
 
 *April 12th, 2019*
@@ -9,13 +32,14 @@ the address book. This swallowed the peer's self-reported port which is importan
 It brings back `NetAddress()` to `NodeInfo` and uses it instead of `SocketAddr` for adding peers.
 Additionally, it improves response time on the `/validators` or `/status` RPC endpoints.
 As a side-effect it makes these RPC endpoint more difficult to DoS and fixes a performance degradation in `ExecCommitBlock`.
-Also, it contains an [ADR](https://github.com/tendermint/tendermint/pull/3539) that proposes decoupling the 
-responsibility for peer behaviour from the `p2p.Switch` (by @brapse). 
+Also, it contains an [ADR](https://github.com/tendermint/tendermint/pull/3539) that proposes decoupling the
+responsibility for peer behaviour from the `p2p.Switch` (by @brapse).
 
 Special thanks to external contributors on this release:
 @brapse, @guagualvcha, @mydring
 
 ### IMPROVEMENTS:
+
 - [p2p] [\#3463](https://github.com/tendermint/tendermint/pull/3463) Do not log "Can't add peer's address to addrbook" error for a private peer
 - [p2p] [\#3547](https://github.com/tendermint/tendermint/pull/3547) Fix a couple of annoying typos (@mdyring)
 
@@ -43,8 +67,8 @@ panic if the lookup failed.
 
 ### BREAKING CHANGES:
 * Go API
-  - [crypto/secp256k1] [\#3439](https://github.com/tendermint/tendermint/issues/3439) 
-    The `secp256k1.GenPrivKeySecp256k1` function has changed to guarantee that it returns a valid key, which means it 
+  - [crypto/secp256k1] [\#3439](https://github.com/tendermint/tendermint/issues/3439)
+    The `secp256k1.GenPrivKeySecp256k1` function has changed to guarantee that it returns a valid key, which means it
     will return a different private key than in previous versions for the same secret.
 
 ### BUG FIXES:

CHANGELOG_PENDING.md

@@ -1,4 +1,4 @@
-## v0.31.5
+## v0.31.6
 
 **
 

blockchain/reactor.go

@@ -228,32 +228,40 @@ func (bcR *BlockchainReactor) poolRoutine() {
 
 	didProcessCh := make(chan struct{}, 1)
 
+	go func() {
+		for {
+			select {
+			case <-bcR.Quit():
+				return
+			case <-bcR.pool.Quit():
+				return
+			case request := <-bcR.requestsCh:
+				peer := bcR.Switch.Peers().Get(request.PeerID)
+				if peer == nil {
+					continue
+				}
+				msgBytes := cdc.MustMarshalBinaryBare(&bcBlockRequestMessage{request.Height})
+				queued := peer.TrySend(BlockchainChannel, msgBytes)
+				if !queued {
+					bcR.Logger.Debug("Send queue is full, drop block request", "peer", peer.ID(), "height", request.Height)
+				}
+			case err := <-bcR.errorsCh:
+				peer := bcR.Switch.Peers().Get(err.peerID)
+				if peer != nil {
+					bcR.Switch.StopPeerForError(peer, err)
+				}
+
+			case <-statusUpdateTicker.C:
+				// ask for status updates
+				go bcR.BroadcastStatusRequest() // nolint: errcheck
+
+			}
+		}
+	}()
+
 FOR_LOOP:
 	for {
 		select {
-		case request := <-bcR.requestsCh:
-			peer := bcR.Switch.Peers().Get(request.PeerID)
-			if peer == nil {
-				continue FOR_LOOP // Peer has since been disconnected.
-			}
-			msgBytes := cdc.MustMarshalBinaryBare(&bcBlockRequestMessage{request.Height})
-			queued := peer.TrySend(BlockchainChannel, msgBytes)
-			if !queued {
-				// We couldn't make the request, send-queue full.
-				// The pool handles timeouts, just let it go.
-				continue FOR_LOOP
-			}
-
-		case err := <-bcR.errorsCh:
-			peer := bcR.Switch.Peers().Get(err.peerID)
-			if peer != nil {
-				bcR.Switch.StopPeerForError(peer, err)
-			}
-
-		case <-statusUpdateTicker.C:
-			// ask for status updates
-			go bcR.BroadcastStatusRequest() // nolint: errcheck
-
 		case <-switchToConsensusTicker.C:
 			height, numPending, lenRequesters := bcR.pool.GetStatus()
 			outbound, inbound, _ := bcR.Switch.NumPeers()
@@ -262,7 +270,6 @@ FOR_LOOP:
 			if bcR.pool.IsCaughtUp() {
 				bcR.Logger.Info("Time to switch to consensus reactor!", "height", height)
 				bcR.pool.Stop()
-
 				conR, ok := bcR.Switch.Reactor("CONSENSUS").(consensusReactor)
 				if ok {
 					conR.SwitchToConsensus(state, blocksSynced)

docs/architecture/adr-039-peer-behaviour.md

@@ -1,7 +1,8 @@
-# ADR 037: Peer Behaviour Interface
+# ADR 039: Peer Behaviour Interface
 
 ## Changelog
 * 07-03-2019: Initial draft
+* 14-03-2019: Updates from feedback
 
 ## Context
 
@@ -19,36 +20,46 @@ and ties up the reactors in a larger dependency graph when testing.
 
 Introduce a `PeerBehaviour` interface and concrete implementations which
 provide methods for reactors to signal peer behaviour without direct
-coupling `p2p.Switch`.  Introduce a ErrPeer to provide 
-concrete reasons for stopping peers.
+coupling `p2p.Switch`.  Introduce a ErrorBehaviourPeer to provide
+concrete reasons for stopping peers. Introduce GoodBehaviourPeer to provide
+concrete ways in which a peer contributes.
 
 ### Implementation Changes
 
 PeerBehaviour then becomes an interface for signaling peer errors as well
 as for marking peers as `good`.
 
-XXX: It might be better to pass p2p.ID instead of the whole peer but as
-a first draft maintain the underlying implementation as much as
-possible.
-
 ```go
 type PeerBehaviour interface {
-    Errored(peer Peer, reason ErrPeer)
-    MarkPeerAsGood(peer Peer)
+    Behaved(peer Peer, reason GoodBehaviourPeer)
+    Errored(peer Peer, reason ErrorBehaviourPeer)
 }
 ```
 
 Instead of signaling peers to stop with arbitrary reasons:
 `reason interface{}` 
 
-We introduce a concrete error type ErrPeer:
+We introduce a concrete error type ErrorBehaviourPeer:
 ```go
-type ErrPeer int
+type ErrorBehaviourPeer int
 
 const (
-    ErrPeerUnknown = iota
-    ErrPeerBadMessage
-    ErrPeerMessageOutofOrder
+    ErrorBehaviourUnknown = iota
+    ErrorBehaviourBadMessage
+    ErrorBehaviourMessageOutofOrder
+    ...
+)
+```
+
+To provide additional information on the ways a peer contributed, we introduce
+the GoodBehaviourPeer type.
+
+```go
+type GoodBehaviourPeer int
+
+const (
+    GoodBehaviourVote = iota
+    GoodBehaviourBlockPart
     ...
 )
 ```
@@ -60,11 +71,11 @@ type SwitchedPeerBehaviour struct {
     sw *Switch
 }
 
-func (spb *SwitchedPeerBehaviour) Errored(peer Peer, reason ErrPeer) {
+func (spb *SwitchedPeerBehaviour) Errored(peer Peer, reason ErrorBehaviourPeer) {
     spb.sw.StopPeerForError(peer, reason)
 }
 
-func (spb *SwitchedPeerBehaviour) MarkPeerAsGood(peer Peer) {
+func (spb *SwitchedPeerBehaviour) Behaved(peer Peer, reason GoodBehaviourPeer) {
     spb.sw.MarkPeerAsGood(peer)
 }
 
@@ -75,51 +86,54 @@ func NewSwitchedPeerBehaviour(sw *Switch) *SwitchedPeerBehaviour {
 }
 ```
 
-Reactors, which are often difficult to unit test[<sup>2</sup>](#references). could use an implementation which exposes the signals produced by the reactor in
+Reactors, which are often difficult to unit test[<sup>2</sup>](#references) could use an implementation which exposes the signals produced by the reactor in
 manufactured scenarios:
 
 ```go
-type PeerErrors map[Peer][]ErrPeer
-type GoodPeers map[Peer]bool
+type ErrorBehaviours map[Peer][]ErrorBehaviourPeer
+type GoodBehaviours map[Peer][]GoodBehaviourPeer
 
 type StorePeerBehaviour struct {
-    pe PeerErrors
-    gp GoodPeers
+    eb ErrorBehaviours
+    gb GoodBehaviours
 }
 
 func NewStorePeerBehaviour() *StorePeerBehaviour{
     return &StorePeerBehaviour{
-        pe: make(PeerErrors),
-        gp: GoodPeers{},
+        eb: make(ErrorBehaviours),
+        gb: make(GoodBehaviours),
     }
 }
 
-func (spb StorePeerBehaviour) Errored(peer Peer, reason ErrPeer) {
-    if _, ok := spb.pe[peer]; !ok {
-        spb.pe[peer] = []ErrPeer{reason}
+func (spb StorePeerBehaviour) Errored(peer Peer, reason ErrorBehaviourPeer) {
+    if _, ok := spb.eb[peer]; !ok {
+        spb.eb[peer] = []ErrorBehaviours{reason}
     } else {
-        spb.pe[peer] = append(spb.pe[peer], reason)
+        spb.eb[peer] = append(spb.eb[peer], reason)
     }
 }
 
-func (mpb *StorePeerBehaviour) GetPeerErrors() PeerErrors {
-    return mpb.pe
+func (mpb *StorePeerBehaviour) GetErrored() ErrorBehaviours {
+    return mpb.eb
 }
 
-func (spb *StorePeerBehaviour) MarkPeerAsGood(peer Peer) {
-    if _, ok := spb.gp[peer]; !ok {
-        spb.gp[peer] = true
+
+func (spb StorePeerBehaviour) Behaved(peer Peer, reason GoodBehaviourPeer) {
+    if _, ok := spb.gb[peer]; !ok {
+        spb.gb[peer] = []GoodBehaviourPeer{reason}
+    } else {
+        spb.gb[peer] = append(spb.gb[peer], reason)
     }
 }
 
-func (spb *StorePeerBehaviour) GetGoodPeers() GoodPeers {
-    return spb.gp
+func (spb *StorePeerBehaviour) GetBehaved() GoodBehaviours {
+    return spb.gb
 }
 ```
 
 ## Status
 
-Proposed
+Accepted
 
 ## Consequences
 

libs/common/cmap.go

@@ -56,7 +56,7 @@ func (cm *CMap) Clear() {
 func (cm *CMap) Keys() []string {
 	cm.l.Lock()
 
-	keys := []string{}
+	keys := make([]string, 0, len(cm.m))
 	for k := range cm.m {
 		keys = append(keys, k)
 	}
@@ -66,7 +66,7 @@ func (cm *CMap) Keys() []string {
 
 func (cm *CMap) Values() []interface{} {
 	cm.l.Lock()
-	items := []interface{}{}
+	items := make([]interface{}, 0, len(cm.m))
 	for _, v := range cm.m {
 		items = append(items, v)
 	}

libs/common/throttle_timer_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	// make govet noshadow happy...
+
 	asrt "github.com/stretchr/testify/assert"
 )
 

libs/db/mem_batch.go

@@ -1,8 +1,6 @@
 package db
 
-import (
-	"sync"
-)
+import "sync"
 
 type atomicSetDeleter interface {
 	Mutex() *sync.Mutex

state/store.go

@@ -193,7 +193,7 @@ func LoadValidators(db dbm.DB, height int64) (*types.ValidatorSet, error) {
 	if valInfo.ValidatorSet == nil {
 		lastStoredHeight := lastStoredHeightFor(height, valInfo.LastHeightChanged)
 		valInfo2 := loadValidatorsInfo(db, lastStoredHeight)
-		if valInfo2 == nil {
+		if valInfo2 == nil || valInfo2.ValidatorSet == nil {
 			// TODO (melekes): remove the below if condition in the 0.33 major
 			// release and just panic. Old chains might panic otherwise if they
 			// haven't saved validators at intermediate (%valSetCheckpointInterval)
@@ -201,7 +201,7 @@ func LoadValidators(db dbm.DB, height int64) (*types.ValidatorSet, error) {
 			// https://github.com/tendermint/tendermint/issues/3543
 			valInfo2 = loadValidatorsInfo(db, valInfo.LastHeightChanged)
 			lastStoredHeight = valInfo.LastHeightChanged
-			if valInfo2 == nil {
+			if valInfo2 == nil || valInfo2.ValidatorSet == nil {
 				panic(
 					fmt.Sprintf("Couldn't find validators at height %d (height %d was originally requested)",
 						lastStoredHeight,

state/store_test.go

@@ -6,34 +6,50 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	cfg "github.com/tendermint/tendermint/config"
 	dbm "github.com/tendermint/tendermint/libs/db"
 	"github.com/tendermint/tendermint/types"
 )
 
-func TestSaveValidatorsInfo(t *testing.T) {
-	// test we persist validators every valSetCheckpointInterval blocks
+func TestStoreLoadValidators(t *testing.T) {
 	stateDB := dbm.NewMemDB()
 	val, _ := types.RandValidator(true, 10)
 	vals := types.NewValidatorSet([]*types.Validator{val})
 
-	// TODO(melekes): remove in 0.33 release
-	// https://github.com/tendermint/tendermint/issues/3543
+	// 1) LoadValidators loads validators using a height where they were last changed
 	saveValidatorsInfo(stateDB, 1, 1, vals)
 	saveValidatorsInfo(stateDB, 2, 1, vals)
+	loadedVals, err := LoadValidators(stateDB, 2)
+	require.NoError(t, err)
+	assert.NotZero(t, loadedVals.Size())
+
+	// 2) LoadValidators loads validators using a checkpoint height
+
+	// TODO(melekes): REMOVE in 0.33 release
+	// https://github.com/tendermint/tendermint/issues/3543
+	// for releases prior to v0.31.4, it uses last height changed
+	valInfo := &ValidatorsInfo{
+		LastHeightChanged: valSetCheckpointInterval,
+	}
+	stateDB.Set(calcValidatorsKey(valSetCheckpointInterval), valInfo.Bytes())
 	assert.NotPanics(t, func() {
-		_, err := LoadValidators(stateDB, 2)
+		saveValidatorsInfo(stateDB, valSetCheckpointInterval+1, 1, vals)
+		loadedVals, err := LoadValidators(stateDB, valSetCheckpointInterval+1)
 		if err != nil {
-			panic(err)
+			t.Fatal(err)
+		}
+		if loadedVals.Size() == 0 {
+			t.Fatal("Expected validators to be non-empty")
 		}
 	})
-	//ENDREMOVE
+	// ENDREMOVE
 
 	saveValidatorsInfo(stateDB, valSetCheckpointInterval, 1, vals)
 
-	loadedVals, err := LoadValidators(stateDB, valSetCheckpointInterval)
-	assert.NoError(t, err)
+	loadedVals, err = LoadValidators(stateDB, valSetCheckpointInterval)
+	require.NoError(t, err)
 	assert.NotZero(t, loadedVals.Size())
 }
 

version/version.go

@@ -20,7 +20,7 @@ const (
 	// Must be a string because scripts like dist.sh read this file.
 	// XXX: Don't change the name of this variable or you will break
 	// automation :)
-	TMCoreSemVer = "0.31.4"
+	TMCoreSemVer = "0.31.5"
 
 	// ABCISemVer is the semantic version of the ABCI library
 	ABCISemVer  = "0.16.0"