Merge pull request #86 from caspark/fix-rollback-desync-false-positive

Fix desync detection false positive caused by a rollback

CHANGELOG.md

@@ -9,6 +9,7 @@ In this document, all remarkable changes are listed. Not mentioned are smaller c
 - added `P2PSession::desync_detection()` to read the session's desync detection mode.
 - ggrs no longer panics when trying to send an overly large UDP packet, unless debug assertions are on.
 - fixed: ggrs would panic when trying to send a message over a custom socket implementation if that message exceeded the maximum safe UDP packet size, even though the underlying socket might have totally different applicable thresholds for what messages can be safely delivered.
+- fix a false positive in `P2PSession`'s desync detection; it was possible for a desync to incorrectly be detected when `P2PSession::advance_frame()` would 1. enqueue a checksum-changing rollback, 2. mark a to-be-rolled-back frame as confirmed, and 3. send that newly-confirmed frame's still-incorrect checksum to peers.
 
 ## 0.10.2
 

src/sessions/p2p_session.rs

@@ -271,6 +271,20 @@ impl<T: Config> P2PSession<T> {
             }
         }
 
+        /*
+         *  DESYNC DETECTION
+         */
+        // Collect, send, compare and check the last checksums against the other peers. The timing
+        // of this is important: since the checksum comparison looks at the current confirmed frame,
+        // (and the sync layer will happily mark a frame as confirmed after requesting rollback and
+        // resimulation of it, at which point that frame's new checksum will not be stored yet), we
+        // must examine our checksum state *before* the sync layer is able to mark any frames as
+        // confirmed.
+        if self.desync_detection != DesyncDetection::Off {
+            self.check_checksum_send_interval();
+            self.compare_local_checksums_against_peers();
+        }
+
         // This list of requests will be returned to the user
         let mut requests = Vec::new();
 
@@ -318,15 +332,6 @@ impl<T: Config> P2PSession<T> {
         self.sync_layer
             .set_last_confirmed_frame(confirmed_frame, self.sparse_saving);
 
-        /*
-         *  DESYNC DETECTION
-         */
-        // collect, send, compare and check the last checksums against the other peers
-        if self.desync_detection != DesyncDetection::Off {
-            self.check_checksum_send_interval();
-            self.compare_local_checksums_against_peers();
-        }
-
         /*
          *  WAIT RECOMMENDATION
          */
@@ -939,7 +944,7 @@ impl<T: Config> P2PSession<T> {
                 };
 
                 if frame_to_send <= self.sync_layer.last_confirmed_frame()
-                    && frame_to_send < self.sync_layer.last_saved_frame()
+                    && frame_to_send <= self.sync_layer.last_saved_frame()
                 {
                     let cell = self
                         .sync_layer