From e49d5f89a5ff12faa052d9f33755462e48fe147c Mon Sep 17 00:00:00 2001
From: Konstantin Osipov <kostja@scylladb.com>
Date: Tue, 16 Feb 2021 18:26:12 +0300
Subject: [PATCH] raft: do not account for the same vote twice

While a duplicate vote from the same server is not possible by a
conforming Raft implementation, Raft assumptions on network permit
duplicates.
So, in theory, it is possible that a vote message is delivered
multiple times.

The current voting implementation does reject votes from non-members,
but doesn't check for duplicate votes.

Keep track of who has voted yet, and reject duplicate votes.

A unit test follows.
---
 raft/tracker.cc | 32 +++++++++++++++-----------------
 raft/tracker.hh | 34 +++++++++++++++++++++++-----------
 2 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/raft/tracker.cc b/raft/tracker.cc
index 0958a9173c..18f8974c95 100644
--- a/raft/tracker.cc
+++ b/raft/tracker.cc
@@ -197,29 +197,25 @@ index_t tracker::committed(index_t prev_commit_idx) {
 }
 
 votes::votes(configuration configuration)
-        :_configuration(std::move(configuration))
-        ,_voters(_configuration.current) {
+        :_voters(configuration.current)
+        , _current(configuration.current) {
 
-    if (_configuration.is_joint()) {
-        _voters.insert(_configuration.previous.begin(), _configuration.previous.end());
+    if (configuration.is_joint()) {
+        _previous.emplace(configuration.previous);
+        _voters.insert(configuration.previous.begin(), configuration.previous.end());
     }
 }
 
 void votes::register_vote(server_id from, bool granted) {
-    server_address from_address{from};
     bool registered = false;
 
-    if (_configuration.current.find(from_address) != _configuration.current.end()) {
-        _current.register_vote(granted);
+    if (_current.register_vote(from, granted)) {
         registered = true;
     }
-    if (_configuration.is_joint() &&
-        _configuration.previous.find(from_address) != _configuration.previous.end()) {
-        _previous.register_vote(granted);
+    if (_previous && _previous->register_vote(from, granted)) {
         registered = true;
     }
-    // Should never receive a vote not requested, unless an RPC
-    // bug.
+    // Should never receive a vote not requested, unless an RPC bug.
     if (! registered) {
         seastar::on_internal_error(logger,
             format("Got a vote from unregistered server {} during election", from));
@@ -227,17 +223,17 @@ void votes::register_vote(server_id from, bool granted) {
 }
 
 vote_result votes::tally_votes() const {
-    if (_configuration.is_joint()) {
-        auto previous_result = _previous.tally_votes(_configuration.previous.size());
+    if (_previous) {
+        auto previous_result = _previous->tally_votes();
         if (previous_result != vote_result::WON) {
             return previous_result;
         }
     }
-    return _current.tally_votes(_configuration.current.size());
+    return _current.tally_votes();
 }
 
 std::ostream& operator<<(std::ostream& os, const election_tracker& v) {
-    os << "responded: " << v._responded << ", ";
+    os << "responded: " << v._responded.size() << ", ";
     os << "granted: " << v._granted;
     return os;
 }
@@ -245,7 +241,9 @@ std::ostream& operator<<(std::ostream& os, const election_tracker& v) {
 
 std::ostream& operator<<(std::ostream& os, const votes& v) {
     os << "current: " << v._current << std::endl;
-    os << "previous: " << v._previous << std::endl;
+    if (v._previous) {
+        os << "previous: " << v._previous.value() << std::endl;
+    }
     return os;
 }
 
diff --git a/raft/tracker.hh b/raft/tracker.hh
index 45137d7dce..4133173da6 100644
--- a/raft/tracker.hh
+++ b/raft/tracker.hh
@@ -133,22 +133,35 @@ std::ostream& operator<<(std::ostream& os, const vote_result& v);
 
 // State of election in a single quorum
 class election_tracker {
-    size_t _responded = 0;
+    // All eligible voters
+    std::unordered_set<server_id> _suffrage;
+    // Votes collected
+    std::unordered_set<server_id> _responded;
     size_t _granted = 0;
 public:
-    void register_vote(bool granted) {
-        _responded++;
-        if (granted) {
-            _granted++;
+    election_tracker(const server_address_set& configuration) {
+        for (const auto& a : configuration) {
+            _suffrage.emplace(a.id);
         }
     }
-    vote_result tally_votes(size_t cluster_size) const {
-        auto quorum = cluster_size / 2 + 1;
+
+    bool register_vote(server_id from, bool granted) {
+        if (_suffrage.find(from) == _suffrage.end()) {
+            return false;
+        }
+        if (_responded.emplace(from).second) {
+            // Have not counted this vote yet
+            _granted += static_cast<int>(granted);
+        }
+        return true;
+    }
+    vote_result tally_votes() const {
+        auto quorum = _suffrage.size() / 2 + 1;
         if (_granted >= quorum) {
             return vote_result::WON;
         }
-        assert(_responded <= cluster_size);
-        auto unknown = cluster_size - _responded;
+        assert(_responded.size() <= _suffrage.size());
+        auto unknown = _suffrage.size() - _responded.size();
         return _granted + unknown >= quorum ? vote_result::UNKNOWN : vote_result::LOST;
     }
     friend std::ostream& operator<<(std::ostream& os, const election_tracker& v);
@@ -156,10 +169,9 @@ public:
 
 // Candidate's state specific to election
 class votes {
-    configuration _configuration;
     server_address_set _voters;
     election_tracker _current;
-    election_tracker _previous;
+    std::optional<election_tracker> _previous;
 public:
     votes(configuration configuration);