`.rpc_id(` should not require `"call_local"` to run locally.

[greenfox1505]

Tested versions

Reproduceable in all Godot 4.x version, including current master branch

System information

All

Issue description

Godot should trust the developer when calling rpc_id( on a locally owned node and continue the call locally even when "call_local" isn't part of it's @rpc(. There is clear security problems with trusting external calls. These security issues do not exist in local->local calls and adds confusion. If a developer is calling a function to a particular to a particular id, it should trust that they intended that. By blocking programmer intention to call on a target that just happens to be local owner, it adds confusion.

This diff should fix this and similify some of the SceneRPCInterface::rpcp, I can commit pull request this if this behavior change is agreeable.

diff --git a/modules/multiplayer/scene_rpc_interface.cpp b/modules/multiplayer/scene_rpc_interface.cpp
index 0938d7ef99..dc38586a37 100644
--- a/modules/multiplayer/scene_rpc_interface.cpp
+++ b/modules/multiplayer/scene_rpc_interface.cpp
@@ -477,57 +477,53 @@ Error SceneRPCInterface::rpcp(Object *p_obj, int p_peer_id, const StringName &p_
 	ERR_FAIL_COND_V_MSG(peer->get_connection_status() != MultiplayerPeer::CONNECTION_CONNECTED, ERR_CONNECTION_ERROR, "Trying to call an RPC via a multiplayer peer which is not connected.");
 
 	int caller_id = multiplayer->get_unique_id();
-	bool call_local_native = false;
-	bool call_local_script = false;
 	const RPCConfigCache &config_cache = _get_node_config(node);
 	uint16_t rpc_id = config_cache.ids.has(p_method) ? config_cache.ids[p_method] : UINT16_MAX;
 	ERR_FAIL_COND_V_MSG(rpc_id == UINT16_MAX, ERR_INVALID_PARAMETER,
 			vformat("Unable to get the RPC configuration for the function \"%s\" at path: \"%s\". This happens when the method is missing or not marked for RPCs in the local script.", p_method, node->get_path()));
 	const RPCConfig &config = config_cache.configs[rpc_id];
 
-	ERR_FAIL_COND_V_MSG(p_peer_id == caller_id && !config.call_local, ERR_INVALID_PARAMETER, "RPC '" + p_method + "' on yourself is not allowed by selected mode.");
-
-	if (p_peer_id == 0 || p_peer_id == caller_id || (p_peer_id < 0 && p_peer_id != -caller_id)) {
-		if (rpc_id & (1 << 15)) {
-			call_local_native = config.call_local;
-		} else {
-			call_local_script = config.call_local;
-		}
-	}
+	// ERR_FAIL_COND_V_MSG(p_peer_id == caller_id && !config.call_local, ERR_INVALID_PARAMETER, "RPC '" + p_method + "' on yourself is not allowed by selected mode.");
 
+	WARN_PRINT("hit this line");
 	if (p_peer_id != caller_id) {
 		_send_rpc(node, p_peer_id, rpc_id, config, p_method, p_arg, p_argcount);
 	}
 
-	if (call_local_native) {
-		Callable::CallError ce;
-
-		multiplayer->set_remote_sender_override(multiplayer->get_unique_id());
-		node->callp(p_method, p_arg, p_argcount, ce);
-		multiplayer->set_remote_sender_override(0);
-
-		if (ce.error != Callable::CallError::CALL_OK) {
-			String error = Variant::get_call_error_text(node, p_method, p_arg, p_argcount, ce);
-			error = "rpc() aborted in local call:  - " + error + ".";
-			ERR_PRINT(error);
-			return FAILED;
-		}
-	}
-
-	if (call_local_script) {
-		Callable::CallError ce;
-		ce.error = Callable::CallError::CALL_OK;
-
-		multiplayer->set_remote_sender_override(multiplayer->get_unique_id());
-		node->get_script_instance()->callp(p_method, p_arg, p_argcount, ce);
-		multiplayer->set_remote_sender_override(0);
-
-		if (ce.error != Callable::CallError::CALL_OK) {
-			String error = Variant::get_call_error_text(node, p_method, p_arg, p_argcount, ce);
-			error = "rpc() aborted in script local call:  - " + error + ".";
-			ERR_PRINT(error);
-			return FAILED;
+	// call local:
+	// - if directly called on this peer
+	// - if broadcast to all peers
+	// - if broadcast to all peers except not this peer
+	if (p_peer_id == caller_id || ((p_peer_id == 0 && config.call_local) || (p_peer_id < 0 && p_peer_id != -caller_id))) {
+		if (rpc_id & (1 << 15)) { //call_local_native
+			Callable::CallError ce;
+
+			multiplayer->set_remote_sender_override(multiplayer->get_unique_id());
+			node->callp(p_method, p_arg, p_argcount, ce);
+			multiplayer->set_remote_sender_override(0);
+
+			if (ce.error != Callable::CallError::CALL_OK) {
+				String error = Variant::get_call_error_text(node, p_method, p_arg, p_argcount, ce);
+				error = "rpc() aborted in local call:  - " + error + ".";
+				ERR_PRINT(error);
+				return FAILED;
+			}
+		} else { //call_local_script
+			Callable::CallError ce;
+			ce.error = Callable::CallError::CALL_OK;
+
+			multiplayer->set_remote_sender_override(multiplayer->get_unique_id());
+			node->get_script_instance()->callp(p_method, p_arg, p_argcount, ce);
+			multiplayer->set_remote_sender_override(0);
+
+			if (ce.error != Callable::CallError::CALL_OK) {
+				String error = Variant::get_call_error_text(node, p_method, p_arg, p_argcount, ce);
+				error = "rpc() aborted in script local call:  - " + error + ".";
+				ERR_PRINT(error);
+				return FAILED;
+			}
 		}
 	}
+	
 	return OK;
 }

Steps to reproduce

Calling any rpc_id with the id being the local player causes the rpc to block.

extends Node

func _ready() -> void:
	call_remote.rpc_id(multiplayer.get_unique_id(),"This is being blocked, but shouldn't be.")

@rpc("any_peer","call_remote")
func call_remote(text):
	print(text)

produces the following error:

ERROR: RPC 'echo' on yourself is not allowed by selected mode.
   at: (modules\multiplayer\scene_rpc_interface.cpp:488)

Minimal reproduction project (MRP)

extends Node


func _ready() -> void:
	call_local.rpc("This should always run locally")
	call_remote.rpc("This should never run locally")

	call_local.rpc_id(multiplayer.get_unique_id(),"This should run regardless")
	call_remote.rpc_id(multiplayer.get_unique_id(),"This is being blocked, but shouldn't be.")

@rpc("any_peer","call_local")
func call_local(text):
	print(text)
	
@rpc("any_peer","call_remote")
func call_remote(text):
	print(text)

In any project should be enough to repro this issue.

[AThousandShips]

This should really be a proposal IMO as it is a suggested change in behavior, RPCs are by their very definition remote procedure calls

[greenfox1505]

I promise most people who are making multiplayer games have hit this confusion several times. Anyone who doesn't strictly make dedicated server games will absolutely be using RPCs locally. Being persnikity about "remote" vs local in RPC use isn't really how RPCs are used to make games.

I did consider making it a proposal, but IMHO it's confusing enough to be a bug. At the very least, it needs to be a "harder" error/crash, not just an error message. It's easy to miss. I've personally wasted a lot of time not realizing I needed "call_local".

With networking, security should be the top priority, but when there isn't a security risk, IMHO we should trust the developer. IMHO, this is a bug.

[AThousandShips]

I'm not being "persnikity", please be constructive instead of calling names

I'll leave this open but I'd say this change to intended and documented behavior is absolutely proposal material, it is a far better place to discuss changes of this kind, undesired behaviour isn't necessarily a bug, especially when it's by design and has dedicated features to handle it

[greenfox1505]

Understandable. I can create a proposal if that's necessary. At minimum I think the error message sound be "louder". RPCs called out of proper context should be a show-stopping error in dev builds.

Not to be pedantic (but Imma be pedantic), "persnikity" is an adjective, not a "name". Sorry if I caused offense regardless.

[AThousandShips]

Making general errors stop execution is a dedicated issue, there's a proposal for it somewhere but can't find it right now, so other than crashing or similar we can't make errors in the engine side stop execution, that's an unfortunate limitation

Note also that rpc is just a thin wrapper around rpc_id so having it behave differently on rpc from rpc_id would be a bit complicated

For the pedanticism, to "call names" is to describe someone in derogative ways, though I might be confusing it with general descriptions of using pejorative or derogative phrasing to describe someone's responses, but apology accepted!

[AThousandShips]

To clarify about bugs, generally we consider things bugs if they are:

• Behavior that is either against the intended functionality, or has unintended consequences
• Intended behavior that is practically useless in actual use cases, even if the behavior is not the result of unintended consequences (i.e. a mismatch between expected and actual use cases)

Since in this case you can adjust and use it as intended, with local calling, using configuration, and it works for the general functionality, I would consider this not a bug but a proposed change of features, I hope that makes the distinction clearer

I'd argue that this suggestion will be more likely to be accepted if presented as an improvement suggestion than as a bug, avoiding fighting up-hill over classification

[a-johnston]

I might be misunderstanding but IMHO this would be bad default behavior // call local: [..] // - if broadcast to all peers except not this peer

At least in my project, which is more or less traditional server-client style, state updates are generally not handled the same way on the host as they are when propagated to client peers. I agree that the error message could be improved, potentially explicitly calling out the rpc option to use for the call to be allowed.

[greenfox1505]

I'm not suggesting .rpc( change. If your server is not also a player, this wouldn't change much for you.

In my games, I need certain things to only run on the server. If a player is also the server, and they fire a weapon at a monster owned by the server (who is him), I call a rpc_id(get_multiplayer_authority(),... to make this object run on the owner. This is a pretty common occurrence, make sure this code runs on the owner of this object.

Maybe the real answer is rpc_owner( as a true feature add, ignore "call_local" behaviour and just run this on the owner (if authority is valid).

For me, "call_local" seem to be used most for broadcasts to everyone, including local. Not for 1:1 communication.

[a-johnston]

I understand what you're saying but I'm not sure why it would be better to have an option handled differently between rpc/rpc_id vs requiring the method's rpc annotation be correct for its intended use (although again I agree the error message could be improved to point people towards making this change). I find it beneficial distinguishing between normal method invocations and rpc invocations for a few reasons, so even if this change was made I would not be taking advantage of it. Regardless though, feel free to make a proposal and I can follow up there.