mempool: Reject same block vote double spends.

Currently, the memory pool policy is to allow votes which double spend
the same ticket (up to a maximum amount) since it is possible that the
same ticket is selected in competing branches and thus the double spend
must be allowed and will resolve itself depending on which chain is
ultimately extended.

A side effect of that is that it also currently accepts votes with
different hashes but otherwise both spend the same ticket and vote on
the same block.  This can happen when there are multiple wallets casting
the vote with different settings such as when one has been upgraded to a
new stake version while the others have not or an improperly configured
wallet where the vote choices have been set on one, but not the others.

Consequently, this modifies the policy to explicitly reject these
duplicate votes while still allowing votes on competing branches.

It also adds tests to ensure the new functionality works as expected.

mempool/mempool.go

@@ -727,6 +727,46 @@ func (mp *TxPool) checkPoolDoubleSpend(tx *dcrutil.Tx, txType stake.TxType) erro
 	return nil
 }
 
+// checkVoteDoubleSpend checks whether or not the passed vote is for a block
+// that already has a vote that spends the same ticket available.  This is
+// necessary because the same ticket might be selected for blocks on candidate
+// side chains and thus a more generic check to merely reject double spends of
+// tickets is not possible.
+//
+// This function MUST be called with the mempool lock held (for reads).
+// This function MUST NOT be called with the votes mutex held.
+func (mp *TxPool) checkVoteDoubleSpend(vote *dcrutil.Tx) error {
+	voteTx := vote.MsgTx()
+	ticketSpent := voteTx.TxIn[1].PreviousOutPoint.Hash
+	hashVotedOn, heightVotedOn := stake.SSGenBlockVotedOn(voteTx)
+	mp.votesMtx.RLock()
+	for _, existingVote := range mp.votes[hashVotedOn] {
+		if existingVote.TicketHash == ticketSpent {
+			// Ensure the vote is still actually in the mempool.  This is needed
+			// because the votes map is not currently kept in sync with the
+			// contents of the pool.
+			//
+			// TODO(decred): Ideally the votes map and mempool would be kept in
+			// sync, which would remove the need for this check, however, there
+			// is currently code outside of mempool that relies on being able to
+			// look up seen votes by block hash, regardless of their current
+			// membership in the pool.
+			if _, exists := mp.pool[existingVote.VoteHash]; !exists {
+				continue
+			}
+
+			mp.votesMtx.RUnlock()
+			str := fmt.Sprintf("vote %v spending ticket %v already votes on "+
+				"block %s (height %d)", vote.Hash(), ticketSpent, hashVotedOn,
+				heightVotedOn)
+			return txRuleError(wire.RejectDuplicate, str)
+		}
+	}
+	mp.votesMtx.RUnlock()
+
+	return nil
+}
+
 // IsRegTxTreeKnownDisapproved returns whether or not the regular tree of the
 // block represented by the provided hash is known to be disapproved according
 // to the votes currently in the memory pool.
@@ -964,7 +1004,17 @@ func (mp *TxPool) maybeAcceptTransaction(tx *dcrutil.Tx, isNew, rateLimit, allow
 		if err != nil {
 			return nil, err
 		}
+
 	} else if isVote {
+		// Reject votes on blocks that already have a vote that spends the same
+		// ticket available.  This is necessary because the same ticket might be
+		// selected for blocks on candidate side chains and thus a more generic
+		// check to merely reject double spends of tickets is not possible.
+		err := mp.checkVoteDoubleSpend(tx)
+		if err != nil {
+			return nil, err
+		}
+
 		voteAlreadyFound := 0
 		for _, mpTx := range mp.pool {
 			if mpTx.Type == stake.TxTypeSSGen {
@@ -980,6 +1030,7 @@ func (mp *TxPool) maybeAcceptTransaction(tx *dcrutil.Tx, isNew, rateLimit, allow
 				return nil, txRuleError(wire.RejectDuplicate, str)
 			}
 		}
+
 	} else if isRevocation {
 		for _, mpTx := range mp.pool {
 			if mpTx.Type == stake.TxTypeSSRtx {

mempool/mempool_test.go

@@ -1920,3 +1920,89 @@ func TestMaxVoteDoubleSpendRejection(t *testing.T) {
 	}
 	testPoolMembership(tc, vote, false, false)
 }
+
+// TestDuplicateVoteRejection ensures that additional votes on the same block
+// that spend the same ticket are rejected from the pool as expected.
+func TestDuplicateVoteRejection(t *testing.T) {
+	t.Parallel()
+
+	harness, spendableOuts, err := newPoolHarness(&chaincfg.MainNetParams)
+	if err != nil {
+		t.Fatalf("unable to create test pool: %v", err)
+	}
+	tc := &testContext{t, harness}
+
+	// Create a regular transaction from the first spendable output provided by
+	// the harness.
+	tx, err := harness.CreateTx(spendableOuts[0])
+	if err != nil {
+		t.Fatalf("unable to create transaction: %v", err)
+	}
+
+	// Create a ticket purchase transaction spending the outputs of the prior
+	// regular transaction.
+	ticket, err := harness.CreateTicketPurchase(tx, 40000)
+	if err != nil {
+		t.Fatalf("unable to create ticket purchase transaction: %v", err)
+	}
+
+	// Add the ticket outputs as utxos to fake their existence.  Use one after
+	// the stake enabled height for the height of the fake utxos to ensure they
+	// are matured for the votes cast a stake validation height below.
+	harness.chain.SetHeight(harness.chainParams.StakeEnabledHeight + 1)
+	harness.chain.utxos.AddTxOuts(ticket, harness.chain.BestHeight(), 0)
+
+	// Create a vote that votes on a block at stake validation height.
+	harness.chain.SetBestHash(&chainhash.Hash{0x5c, 0xa1, 0xab, 0x1e})
+	harness.chain.SetHeight(harness.chainParams.StakeValidationHeight)
+	vote, err := harness.CreateVote(ticket)
+	if err != nil {
+		t.Fatalf("unable to create vote: %v", err)
+	}
+
+	// Add the vote and ensure it is not in the orphan pool, is in the
+	// transaction pool, and is reported as available.
+	_, err = harness.txPool.ProcessTransaction(vote, false, false, true)
+	if err != nil {
+		t.Fatalf("ProcessTransaction: failed to accept valid vote %v", err)
+	}
+	testPoolMembership(tc, vote, false, true)
+
+	// Create another vote with a different hash that votes on the same block
+	// using the same ticket.
+	dupVote, err := harness.CreateVote(ticket, func(tx *wire.MsgTx) {
+		voteBits := stake.VoteBits{Bits: uint16(0x03), ExtendedBits: nil}
+		voteScript, err := newVoteScript(voteBits)
+		if err != nil {
+			t.Fatalf("failed to create vote script: %v", err)
+		}
+		tx.TxOut[1].PkScript = voteScript
+	})
+	if err != nil {
+		t.Fatalf("unable to create vote: %v", err)
+	}
+
+	// Attempt to add the duplicate vote and ensure it is rejected.  Also,
+	// ensure it is not in the orphan pool, not in the transaction pool, and not
+	// reported as available.
+	_, err = harness.txPool.ProcessTransaction(dupVote, false, false, true)
+	if err == nil {
+		t.Fatalf("ProcessTransaction: accepted duplicate vote with different " +
+			"hash")
+	}
+	testPoolMembership(tc, dupVote, false, false)
+
+	// Remove the original vote from the pool and ensure it is not in the orphan
+	// pool, not in the transaction pool, and not reported as available.
+	harness.txPool.RemoveTransaction(vote, true)
+	testPoolMembership(tc, vote, false, false)
+
+	// Add the duplicate vote which should now be accepted.  Also, ensure it is
+	// not in the orphan pool, is in the transaction pool, and is reported as
+	// available.
+	_, err = harness.txPool.ProcessTransaction(dupVote, false, false, true)
+	if err != nil {
+		t.Fatalf("ProcessTransaction: failed to accept valid vote %v", err)
+	}
+	testPoolMembership(tc, dupVote, false, true)
+}