fix ordered Lua packet timestamp comparison

code/network/multi.cpp

@@ -323,6 +323,9 @@ void multi_level_init()
 	// initialize respawning
 	multi_respawn_init();
 
+	// forget ordered lua packet timestamps from any previous game
+	multi_lua_reset_packet_order();
+
 	// initialize all netgame timestamps
 	multi_reset_timestamps();
 

code/network/multi_lua.cpp

@@ -13,42 +13,44 @@ static SCP_unordered_map<ushort, scripting::api::rpc_h_ref> rpc_map;
 
 enum class lua_net_data_type : uint8_t { NIL, BOOL, NUMBER, STRING8, STRING16, USERDATA, TABLE };
 
-static bool need_toss_packet(ushort target, short packet_source, ushort packetTime, UI_TIMESTAMP localTime) {
-	//Ordering is enforced for packet source and execution targets. Meaning we neither order packets form different sources
-	//(due to incomparibility of timestamps) or to different targets (due to semantic insignificance)
-	static SCP_map<std::pair<ushort, short>, std::pair<ushort, UI_TIMESTAMP>> received_packets;
+//Ordering is enforced for packet source and execution targets. Meaning we neither order packets form different sources
+//(due to incomparibility of timestamps) or to different targets (due to semantic insignificance)
+static SCP_map<std::pair<ushort, short>, std::pair<ushort, UI_TIMESTAMP>> received_packets;
+
+//Once this much local time has passed since the last accepted packet, the 16-bit remote difference may have wrapped and can't be trusted
+constexpr int LUA_PACKET_ORDER_WINDOW_MS = 0x4000;
 
-	//The idea is, that we reject packets which are in the past, unless the last packet we actually got is so far in the past, we're not sure if it might have overflowed
-	//Basically, assume that if packetTime - lastPacketTime < 1000, then it's either delayed over 60 seconds, or not a past packet. If it's larger than that, compare it to
-	//the difference of local timestamps. If the local timestamp is over 2^16, it's also safe, otherwise, allow a 10% delay margin.
+bool multi_lua_packet_is_stale(ushort lastRemote, ushort newRemote, int localDeltaMs) {
+	if (localDeltaMs >= LUA_PACKET_ORDER_WINDOW_MS)
+		return false;
+
+	//Serial number arithmetic: a wrapped difference in the upper half of the range means the new packet was sent before the last one
+	return static_cast<ushort>(newRemote - lastRemote) >= 0x8000U;
+}
 
+static bool need_toss_packet(ushort target, short packet_source, ushort packetTime, UI_TIMESTAMP localTime) {
 	auto& channel = received_packets[{target, packet_source}];
-	std::pair<ushort, UI_TIMESTAMP> newTimestamp = { std::move(packetTime), std::move(localTime) };
 
-	if (!channel.second.isValid()) {
-		//First packet for this channel
-		channel = std::move(newTimestamp);
-		return false;
+	if (channel.second.isValid() && multi_lua_packet_is_stale(channel.first, packetTime, ui_timestamp_get_delta(channel.second, localTime))) {
+		//Since we toss the packet, we DON'T update the received packet index.
+		return true;
 	}
 
-	if (newTimestamp.first - channel.first < 1000) {
-		//The previous packet is less than a second ago. If this were a delayed packet, as a ushort, it'd have to be >60 seconds late, so effectively impossible.
-		channel = std::move(newTimestamp);
-		return false;
-	}
+	channel = { packetTime, localTime };
+	return false;
+}
 
-	if (static_cast<int>(newTimestamp.first - channel.first) * 110 < ui_timestamp_get_delta(channel.second, newTimestamp.second)) {
-		//The previous packet is, at most, 10% older than its age when it arrived. We expect that most delayed packets will happen when sent in quick succession,
-		//so what needs to be guarded here are underflows, which produce very large values but will have very small timestamps. So as long as local and remote timestamp
-		//somewhat agree, the packet's good. In addition, once ui_timestamp_get_delta exceeds 65 seconds, we can't tell anymore due to the remote timestamp being uint16,
-		//but at that point we can just accept the packet as likely new.
-		channel = std::move(newTimestamp);
-		return false;
-	}
+void multi_lua_reset_packet_order() {
+	received_packets.clear();
+}
 
-	//If we're still here, then we've got a packet that has a very high remote time but a very short local time, so likely a packet whose high remote time indicates a
-	//negative remote time. Toss is then. Also, since we toss the packet, we DON'T update the received packet index.
-	return true;
+void multi_lua_forget_player(short player_id) {
+	for (auto it = received_packets.begin(); it != received_packets.end();) {
+		if (it->first.second == player_id)
+			it = received_packets.erase(it);
+		else
+			it++;
+	}
 }
 
 

code/network/multi_lua.h

@@ -17,4 +17,9 @@ void process_lua_packet(ubyte* data, header* hinfo, bool reliable);
 bool send_lua_packet(const luacpp::LuaValue& value, ushort target, lua_net_mode mode, lua_net_reciever reciever);
 
 bool add_rpc(scripting::api::rpc_h_ref ref);
-void clean_rpc_refs();
+void clean_rpc_refs();
+
+//True if an ordered packet stamped newRemote is older than the last accepted packet (stamped lastRemote) on its channel
+bool multi_lua_packet_is_stale(ushort lastRemote, ushort newRemote, int localDeltaMs);
+void multi_lua_reset_packet_order();
+void multi_lua_forget_player(short player_id);

code/network/multiutil.cpp

@@ -63,6 +63,7 @@
 #include "network/multiteamselect.h"
 #include "network/multiui.h"
 #include "network/multi_kick.h"
+#include "network/multi_lua.h"
 #include "network/multi_data.h"
 #include "network/multi_voice.h"
 #include "network/multi_team.h"
@@ -894,6 +895,8 @@ void delete_player(int player_num,int kicked_reason)
 
 	psnet_rel_close_socket(Net_players[player_num].reliable_socket);					// close out the reliable socket
 
+	multi_lua_forget_player(Net_players[player_num].player_id);
+
 	// if this guy was ingame joining, the remove my netgame flag so others may join
 	if ( Net_players[player_num].flags & NETINFO_FLAG_INGAME_JOIN ) {
 		Netgame.flags &= ~NG_FLAG_INGAME_JOINING;

test/src/network/test_multi_lua.cpp

@@ -0,0 +1,58 @@
+
+#include <gtest/gtest.h>
+
+#include "network/multi_lua.h"
+
+TEST(multi_lua, in_order_packet_is_not_stale)
+{
+	EXPECT_FALSE(multi_lua_packet_is_stale(100, 200, 100));
+}
+
+TEST(multi_lua, same_timestamp_is_not_stale)
+{
+	EXPECT_FALSE(multi_lua_packet_is_stale(100, 100, 0));
+}
+
+TEST(multi_lua, reordered_packet_is_stale)
+{
+	EXPECT_TRUE(multi_lua_packet_is_stale(200, 100, 5));
+}
+
+TEST(multi_lua, wrap_forward_is_not_stale)
+{
+	EXPECT_FALSE(multi_lua_packet_is_stale(65000, 10, 546));
+}
+
+TEST(multi_lua, wrap_backward_is_stale)
+{
+	EXPECT_TRUE(multi_lua_packet_is_stale(10, 65000, 5));
+}
+
+TEST(multi_lua, delayed_previous_packet_does_not_toss_newer_packet)
+{
+	// sent 1500ms apart, but the previous packet arrived 200ms late
+	EXPECT_FALSE(multi_lua_packet_is_stale(1000, 2500, 1300));
+}
+
+TEST(multi_lua, long_local_gap_is_not_stale)
+{
+	// a remote gap of 40s looks negative as a 16-bit difference, but the local gap shows the packet is new
+	EXPECT_FALSE(multi_lua_packet_is_stale(0, 40000, 39000));
+}
+
+TEST(multi_lua, short_local_gap_with_negative_remote_gap_is_stale)
+{
+	EXPECT_TRUE(multi_lua_packet_is_stale(0, 40000, 100));
+}
+
+TEST(multi_lua, remote_gap_boundary)
+{
+	EXPECT_FALSE(multi_lua_packet_is_stale(0, 0x7FFF, 0));
+	EXPECT_TRUE(multi_lua_packet_is_stale(0, 0x8000, 0));
+}
+
+TEST(multi_lua, local_window_boundary)
+{
+	EXPECT_TRUE(multi_lua_packet_is_stale(200, 100, 16383));
+	EXPECT_FALSE(multi_lua_packet_is_stale(200, 100, 16384));
+}

test/src/source_groups.cmake

@@ -49,6 +49,10 @@ add_file_folder("model"
     model/test_modelread.cpp
 )
 
+add_file_folder("Network"
+    network/test_multi_lua.cpp
+)
+
 add_file_folder("Options"
     options/test_options_manager.cpp
 )