fix(consensus): stop a previous-height precommit marking the peer's current-height votes

internal/consensus/peer_state.go

@@ -359,20 +359,11 @@ func (ps *PeerState) getVoteBitArray(height int64, round int32, votesType tmprot
 
 		return nil
 	}
-	if ps.PRS.Height == height+1 {
-		if ps.PRS.LastCommitRound == round {
-			switch votesType {
-			case tmproto.PrevoteType:
-				return nil
-
-			case tmproto.PrecommitType:
-				return ps.PRS.Precommits
-			}
-		}
-
-		return nil
-	}
 
+	// Votes of the height the peer has left are not tracked: its commit is
+	// gossiped whole (see GossipCommit), and ps.PRS.Precommits belongs to the
+	// peer's current height, so a late previous-height precommit must not
+	// touch it.
 	return nil
 }
 
@@ -644,12 +635,7 @@ func (ps *PeerState) ApplyNewRoundStepMessage(msg *NewRoundStepMessage) {
 	}
 
 	if psHeight != msg.Height {
-		// Shift Precommits to LastPrecommits.
-		if psHeight+1 == msg.Height && psRound == msg.LastCommitRound {
-			ps.PRS.LastCommitRound = msg.LastCommitRound
-		} else {
-			ps.PRS.LastCommitRound = msg.LastCommitRound
-		}
+		ps.PRS.LastCommitRound = msg.LastCommitRound
 
 		// we'll update the BitArray capacity later
 		ps.PRS.CatchupCommitRound = -1

internal/consensus/peer_state_lastcommit_test.go

@@ -0,0 +1,80 @@
+package consensus
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/dashpay/tenderdash/libs/bits"
+	"github.com/dashpay/tenderdash/libs/log"
+	tmproto "github.com/dashpay/tenderdash/proto/tendermint/types"
+	"github.com/dashpay/tenderdash/types"
+)
+
+// lastCommitPeerState is a peer at height 2, round 0, whose commit for
+// height 1 was at round 0, with validator bit arrays allocated for height 2.
+func lastCommitPeerState(t *testing.T) *PeerState {
+	t.Helper()
+	ps := NewPeerState(log.NewNopLogger(), "testPeerState")
+	ps.PRS.Height, ps.PRS.Round, ps.PRS.LastCommitRound = 2, 0, 0
+	ps.ensureVoteBitArrays(2, 3)
+	return ps
+}
+
+// A precommit of the previous height, received from a peer that has already
+// moved on, must not mark the same validator index as held in the peer's
+// current-height precommits. Otherwise this node's current-height precommit
+// of that validator is never offered to the peer.
+func TestSetHasVoteLastHeightPrecommitDoesNotMarkCurrentHeight(t *testing.T) {
+	ps := lastCommitPeerState(t)
+
+	lateLastCommit := &types.Vote{Height: 1, Round: 0, Type: tmproto.PrecommitType, ValidatorIndex: 1}
+	require.NoError(t, ps.SetHasVote(lateLastCommit))
+
+	require.False(t, ps.PRS.Precommits.GetIndex(1),
+		"a height-1 precommit must not mark the peer as holding the height-2 precommit of that validator")
+
+	ours := bits.NewBitArray(3)
+	ours.SetIndex(1, true)
+	vote, ok := ps.PickVoteToSend(&fixedVoteSet{height: 2, round: 0, msgType: tmproto.PrecommitType, have: ours})
+	require.True(t, ok, "the height-2 precommit of validator 1 must still be offered to the peer")
+	require.EqualValues(t, 1, vote.ValidatorIndex)
+}
+
+// A VoteSetBits answer about the previous height's commit, applied after the
+// peer moved on, must not overwrite the peer's current-height precommit bit
+// array — whether this node has already left that height too (ourVotes nil)
+// or still holds its votes for it.
+func TestApplyVoteSetBitsLastHeightPrecommitsDoNotMarkCurrentHeight(t *testing.T) {
+	peerHas := bits.NewBitArray(3)
+	for i := 0; i < 3; i++ {
+		peerHas.SetIndex(i, true)
+	}
+	for name, ourVotes := range map[string]*bits.BitArray{
+		"we moved on too":              nil,
+		"we still hold height-1 votes": peerHas.Copy(),
+	} {
+		t.Run(name, func(t *testing.T) {
+			ps := lastCommitPeerState(t)
+
+			ps.ApplyVoteSetBitsMessage(&VoteSetBitsMessage{
+				Height: 1, Round: 0, Type: tmproto.PrecommitType, Votes: peerHas,
+			}, ourVotes)
+
+			require.Equal(t, bits.NewBitArray(3), ps.PRS.Precommits,
+				"height-1 commit bits must not be applied to the peer's height-2 precommits")
+		})
+	}
+}
+
+// NewRoundStep moving the peer to the next height still records the round of
+// its commit for the height it left.
+func TestApplyNewRoundStepMessageKeepsLastCommitRound(t *testing.T) {
+	ps := lastCommitPeerState(t)
+	ps.PRS.Round, ps.PRS.LastCommitRound = 3, -1
+
+	ps.ApplyNewRoundStepMessage(&NewRoundStepMessage{Height: 3, Round: 0, Step: 1, LastCommitRound: 3})
+
+	require.EqualValues(t, 3, ps.PRS.Height)
+	require.EqualValues(t, 3, ps.PRS.LastCommitRound)
+}