Don't rollback uncommittable indices on become_follower (#7620)

Co-authored-by: Eddy Ashton <ashton.eddy@gmail.com>
Co-authored-by: Amaury Chamayou <amchamay@microsoft.com>
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>

Author: 3 people

CHANGELOG.md

@@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 - `GET` and `HEAD` `/node/ledger-chunk?since={seqno}` and `/node/ledger-chunk/{chunk_name}` endpoints, gated by the `LedgerChunkDownload` RPC interface operator feature. See [documentation](https://microsoft.github.io/CCF/main/operations/ledger_snapshot.html#download-endpoints) for more detail.
 
+### Fixed
+
+- Only rollback uncommittable indices during become_leader (#7620)
+
 ## [7.0.0-dev9]
 
 [7.0.0-dev9]: https://github.com/microsoft/CCF/releases/tag/ccf-7.0.0-dev9

src/consensus/aft/raft.h

@@ -1161,7 +1161,7 @@ namespace aft
 
         // Reply false if the log doesn't contain an entry at r.prev_idx
         // whose term is r.prev_term. Rejects "future" entries.
-        if (prev_term == 0)
+        if (prev_term == ccf::VIEW_UNKNOWN)
         {
           RAFT_DEBUG_FMT(
             "Recv {} to {} from {} but our log does not yet "
@@ -1212,10 +1212,10 @@ namespace aft
           state->commit_idx);
         return;
       }
-      // Redundant with check on get_term_internal() at line 1149
-      // Which captures this case in every situation, except r.prev_term == 0.
-      // That only happens if r.prev_idx == 0 however, see line 1033,
-      // in which case this path should not be taken either.
+      // This block is redundant - the checks above cover this case, so the code
+      // inside this block should be unreachable. It is retained out of
+      // abundance of caution, in case future rewrites of the above conditions
+      // allow a fallthrough.
       if (r.prev_idx > state->last_idx)
       {
         RAFT_FAIL_FMT(
@@ -1652,6 +1652,7 @@ namespace aft
           std::min(this_match, node->second.sent_idx), node->second.match_idx);
         return;
       }
+
       // max(...) because why would we ever want to go backwards on a success
       // response?!
       node->second.match_idx = std::max(node->second.match_idx, r.last_log_idx);
@@ -2244,11 +2245,6 @@ namespace aft
       restart_election_timeout();
       reset_last_ack_timeouts();
 
-      // Drop anything unsigned here, but retain all signed entries. Only do a
-      // more aggressive rollback, potentially including signatures, when
-      // receiving a conflicting AppendEntries
-      rollback(last_committable_index());
-
       state->leadership_state = ccf::kv::LeadershipState::Follower;
       RAFT_INFO_FMT(
         "Becoming follower {}: {}.{}",

src/consensus/aft/test/driver.cpp

@@ -294,6 +294,11 @@ int main(int argc, char** argv)
         assert(items.size() == 3);
         driver->assert_absent_config(items[1], items[2]);
         break;
+      case shash("assert_last_txid"):
+        assert(items.size() == 3);
+        skip_invariants = true;
+        driver->assert_last_txid(items[1], items[2]);
+        break;
       case shash("replicate_new_configuration"):
         assert(items.size() >= 3);
         items.erase(items.begin());

src/consensus/aft/test/driver.h

@@ -1443,4 +1443,20 @@ class RaftDriver
       }
     }
   }
+
+  void assert_last_txid(
+    const std::string& node_id, const std::string& last_txid_s)
+  {
+    auto idx = _nodes.at(node_id).raft->get_last_idx();
+    auto view = _nodes.at(node_id).raft->get_view(idx);
+    auto last_txid = fmt::format("{}.{}", view, idx);
+    if (last_txid != last_txid_s)
+    {
+      throw std::runtime_error(fmt::format(
+        "Node {} lastTxID is not as expected: {} != {}",
+        node_id,
+        last_txid,
+        last_txid_s));
+    }
+  }
 };

tests/raft_scenarios/follower_rollback_match_index

@@ -0,0 +1,49 @@
+# A repro of the failing scenario where a follower rolls back its ledger by becoming a pre-vote-candidate and then back to a follower (#7618)
+# If this follower had previously acked an append-entries, then the leader's matchIdx for it would be higher than its truncated ledger.
+# This was fixed by no longer rolling back uncommittable entries (no signature) when becoming a follower.
+
+start_node,0
+emit_signature,2
+assert_commit_idx,0,2
+
+swap_nodes,2,in,1,2
+emit_signature,2
+
+loop_until_sync
+
+assert_last_txid,0,2.5
+replicate,2,tx1
+assert_last_txid,0,2.6
+
+# advance matchIdx
+periodic_one,0,10
+dispatch_single,0,2
+dispatch_single,2,0
+
+# Partition 2
+disconnect_node,2
+
+emit_signature,2
+assert_last_txid,0,2.7
+
+periodic_one,0,10
+
+# b2 time out
+periodic_one,2,100
+assert_detail,2,leadership_state,PreVoteCandidate
+assert_last_txid,2,2.6
+
+dispatch_all
+
+# Heal partition
+reconnect_node,2
+
+# leader sends append entries [2.7,2.7) to b2
+# b2 nacks but does not truncate
+periodic_one,0,10
+dispatch_single,0,2
+dispatch_single,2,0
+assert_detail,2,leadership_state,Follower
+assert_last_txid,2,2.6
+
+loop_until_sync

tests/raft_scenarios/soft_rollback

@@ -51,30 +51,31 @@ dispatch_all
 replicate,3,saluton mondo
 replicate,3,ah well nevertheless
 state_all
+assert_last_txid,1,3.10
 
 # Node 2 calls an election, advancing to term 3
 periodic_one,2,110
 
-# Node 2's RequestVote reaches Node 1, causing a soft-rollback
+# -- Node 2's RequestVote reaches Node 1, causing a soft-rollback --
+# Node 2's RequestVote reaches Node 1, causing it to advance term but not roll-back
 dispatch_one,2
 
-# Node 1 should retain the committed state!
+# Node 1 should retain the whole state 3.10 it had previously
 state_all
+assert_detail,1,current_view,4
+assert_last_txid,1,3.10
+assert_detail,2,current_view,4
+assert_last_txid,2,2.6
 
 # Node 1 should not vote for node 2!
 dispatch_one,1
 assert_!detail,2,leadership_state,Leader
 
-# Because if they did, then node 2's AEs would break persistence!
-periodic_all,10
-dispatch_all
-state_all
-
 # Now restore comms between 0 and 1, and disconnect 2
 reconnect_node,0
 disconnect_node,2
 
-# Despite not being able to _commit_ the entries it holds, node 1
+# Despite not being able to currently _commit_ the entries it holds, node 1
 # should be able to win an election by advertising them!
 periodic_one,1,110
 dispatch_all

tla/consensus/MCccfraft.cfg

@@ -97,3 +97,4 @@ INVARIANTS
     RetiredCommittedInv
     RetirementCompletedNotInConfigsInv
     RetirementCompletedAreRetirementCompletedInv
+    MatchIndexBoundedByLogInv 

tla/consensus/SIMccfraft.cfg

@@ -111,6 +111,7 @@ INVARIANTS
     RetiredCommittedInv
     RetirementCompletedNotInConfigsInv
     RetirementCompletedAreRetirementCompletedInv
+    MatchIndexBoundedByLogInv 
 
     \* DebugInvLeaderCannotStepDown
     \* DebugInvAnyCommitted

tla/consensus/Traceccfraft.tla

@@ -202,8 +202,9 @@ IsBecomeLeader ==
     /\ Range(logline.msg.state.committable_indices) \subseteq CommittableIndices(logline.msg.state.node_id)
     /\ commitIndex[logline.msg.state.node_id] = logline.msg.state.commit_idx
     /\ leadershipState'[logline.msg.state.node_id] = ToLeadershipState[logline.msg.state.leadership_state]
-    /\ membershipState[logline.msg.state.node_id] \in ToMembershipState[logline.msg.state.membership_state]
-    /\ Len(log[logline.msg.state.node_id]) = logline.msg.state.last_idx
+    \* The log is truncated during BecomeLeader to the last committable index, and so membership state may have also changed
+    /\ membershipState'[logline.msg.state.node_id] \in ToMembershipState[logline.msg.state.membership_state]
+    /\ Len(log'[logline.msg.state.node_id]) = logline.msg.state.last_idx
     /\ (logline.msg.state.pre_vote_enabled => PreVoteEnabled \in preVoteStatus[logline.msg.state.node_id])
 
 IsClientRequest ==

tla/consensus/ccfraft.tla

@@ -1222,16 +1222,9 @@ UpdateTerm(i, j, m) ==
          ![i] = IF @ \in {Leader, Candidate, PreVoteCandidate, None} THEN Follower ELSE @]
     /\ isNewFollower' = [isNewFollower EXCEPT ![i] = TRUE]
     /\ votedFor'       = [votedFor    EXCEPT ![i] = Nil]
-    \* See rollback(last_committable_index()) in raft::become_follower
-    /\ log'            = [log         EXCEPT ![i] = SubSeq(@, 1, LastCommittableIndex(i))]
-    \* Potentially also shorten the configurations if the removed txns contained reconfigurations
-    /\ configurations' = [configurations EXCEPT ![i] = ConfigurationsToIndex(i,Len(log'[i]))]
-    \* If the leader was in the RetirementOrdered state, then its retirement has
-    \* been rolled back as it was unsigned
-    /\ membershipState' = [membershipState EXCEPT ![i] = 
-        IF @ = RetirementOrdered THEN Active ELSE @]
     \* messages is unchanged so m can be processed further.
-    /\ UNCHANGED <<preVoteStatus, messageVars, candidateVars, leaderVars, commitIndex, hasJoined, retirementCompleted>>
+    /\ UNCHANGED <<preVoteStatus, messageVars, candidateVars, leaderVars, logVars, hasJoined, retirementCompleted, membershipState, configurations>>
+
 
 \* Responses with stale terms are ignored.
 DropStaleResponse(i, j, m) ==
@@ -1719,6 +1712,10 @@ NeverCommitEntryPrevTermsProp ==
         \* points to equals the leader's term.
         commitIndex'[i] > commitIndex[i] => log[i][commitIndex'[i]].term = currentTerm'[i] ]_vars
 
+MatchIndexBoundedByLogInv ==
+  \A i,j \in Servers:
+  (leadershipState[i] = Leader /\ currentTerm[i] = currentTerm[j]) => matchIndex[i][j] <= Len(log[j])
+
 ------------------------------------------------------------------------------
 \* Refinement of the more high-level specification abs.tla that abstracts the
 \* asynchronous network and the message passing between nodes.  Instead, any