[MP] Gracefully handle cache confirmation of deleted nodes

It's possible that after sending a cached node reference (e.g. RPC or
static MultiplayerSynchronizer) the reference node is removed from tree
before the remote peer(s) can confirm the referenced path.

To better detect that case, and avoid spamming errors when it happens,
this commit modifies the multiplayer API caching protocol, to send the
received ID instead of the Node path when sending the confirmation
packet.

**This is a breaking change** because it makes the runtime multiplayer
protocol incompatible with previous versions of Godot.

2 files changed, 22 insertions, 19 deletions

diff --git a/modules/multiplayer/scene_cache_interface.cpp b/modules/multiplayer/scene_cache_interface.cpp
index 33b05d4cc2..c08ccbe4cc 100644
--- a/modules/multiplayer/scene_cache_interface.cpp
+++ b/modules/multiplayer/scene_cache_interface.cpp
@@ -52,6 +52,9 @@ void SceneCacheInterface::_remove_node_cache(ObjectID p_oid) {
 	if (!nc) {
 		return;
 	}
+	if (nc->cache_id) {
+		assigned_ids.erase(nc->cache_id);
+	}
 	for (KeyValue<int, int> &E : nc->recv_ids) {
 		PeerInfo *pinfo = peers_info.getptr(E.key);
 		ERR_CONTINUE(!pinfo);
@@ -117,16 +120,12 @@ void SceneCacheInterface::process_simplify_path(int p_from, const uint8_t *p_pac
 	NodeCache &cache = _track(node);
 	cache.recv_ids.insert(p_from, id);
 
-	// Encode path to send ack.
-	CharString pname = String(path).utf8();
-	int len = encode_cstring(pname.get_data(), nullptr);
-
+	// Send ack.
 	Vector<uint8_t> packet;
-
-	packet.resize(1 + 1 + len);
+	packet.resize(1 + 1 + 4);
 	packet.write[0] = SceneMultiplayer::NETWORK_COMMAND_CONFIRM_PATH;
 	packet.write[1] = valid_rpc_checksum;
-	encode_cstring(pname.get_data(), &packet.write[2]);
+	encode_uint32(id, &packet.write[2]);
 
 	Ref<MultiplayerPeer> multiplayer_peer = multiplayer->get_multiplayer_peer();
 	ERR_FAIL_COND(multiplayer_peer.is_null());
@@ -137,26 +136,26 @@ void SceneCacheInterface::process_simplify_path(int p_from, const uint8_t *p_pac
 }
 
 void SceneCacheInterface::process_confirm_path(int p_from, const uint8_t *p_packet, int p_packet_len) {
-	ERR_FAIL_COND_MSG(p_packet_len < 3, "Invalid packet received. Size too small.");
+	ERR_FAIL_COND_MSG(p_packet_len != 6, "Invalid packet received. Size too small.");
 	Node *root_node = SceneTree::get_singleton()->get_root()->get_node(multiplayer->get_root_path());
 	ERR_FAIL_NULL(root_node);
 
 	const bool valid_rpc_checksum = p_packet[1];
+	int id = decode_uint32(&p_packet[2]);
 
-	String paths;
-	paths.parse_utf8((const char *)&p_packet[2], p_packet_len - 2);
-
-	const NodePath path = paths;
+	const ObjectID *oid = assigned_ids.getptr(id);
+	if (oid == nullptr) {
+		return; // May be trying to confirm a node that was removed.
+	}
 
 	if (valid_rpc_checksum == false) {
-		ERR_PRINT("The rpc node checksum failed. Make sure to have the same methods on both nodes. Node path: " + path);
+		const Node *node = Object::cast_to<Node>(ObjectDB::get_instance(*oid));
+		ERR_FAIL_NULL(node); // Bug.
+		ERR_PRINT("The rpc node checksum failed. Make sure to have the same methods on both nodes. Node path: " + node->get_path());
 	}
 
-	Node *node = root_node->get_node(path);
-	ERR_FAIL_NULL(node);
-
-	NodeCache *cache = nodes_cache.getptr(node->get_instance_id());
-	ERR_FAIL_NULL_MSG(cache, "Invalid packet received. Tries to confirm a node which was not requested.");
+	NodeCache *cache = nodes_cache.getptr(*oid);
+	ERR_FAIL_NULL(cache); // Bug.
 
 	bool *confirmed = cache->confirmed_peers.getptr(p_from);
 	ERR_FAIL_NULL_MSG(confirmed, "Invalid packet received. Tries to confirm a node which was not requested.");
@@ -216,6 +215,7 @@ int SceneCacheInterface::make_object_cache(Object *p_obj) {
 	NodeCache &cache = _track(node);
 	if (cache.cache_id == 0) {
 		cache.cache_id = last_send_cache_id++;
+		assigned_ids[cache.cache_id] = p_obj->get_instance_id();
 	}
 	return cache.cache_id;
 }
@@ -227,6 +227,7 @@ bool SceneCacheInterface::send_object_cache(Object *p_obj, int p_peer_id, int &r
 	NodeCache &cache = _track(node);
 	if (cache.cache_id == 0) {
 		cache.cache_id = last_send_cache_id++;
+		assigned_ids[cache.cache_id] = p_obj->get_instance_id();
 	}
 	r_id = cache.cache_id;
 
@@ -289,5 +290,6 @@ void SceneCacheInterface::clear() {
 	}
 	peers_info.clear();
 	nodes_cache.clear();
+	assigned_ids.clear();
 	last_send_cache_id = 1;
 }
diff --git a/modules/multiplayer/scene_cache_interface.h b/modules/multiplayer/scene_cache_interface.h
index ab4a20c078..73d6bde6ef 100644
--- a/modules/multiplayer/scene_cache_interface.h
+++ b/modules/multiplayer/scene_cache_interface.h
@@ -44,7 +44,7 @@ private:
 
 	//path sent caches
 	struct NodeCache {
-		int cache_id;
+		int cache_id = 0;
 		HashMap<int, int> recv_ids; // peer id, remote cache id
 		HashMap<int, bool> confirmed_peers; // peer id, confirmed
 	};
@@ -55,6 +55,7 @@ private:
 	};
 
 	HashMap<ObjectID, NodeCache> nodes_cache;
+	HashMap<int, ObjectID> assigned_ids;
 	HashMap<int, PeerInfo> peers_info;
 	int last_send_cache_id = 1;