`.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