cli, rpc: add -rpcid option for custom request IDs #35006

I don't see the need to couple rpc and cli here, just always logging the rpcid seems good? Simplifies behaviour and implementation.

The idea was that including the ID if set to the default value would lead to more noise in the logs, without providing any benefit to the log consumer. Just loads and loads of log lines with ID "1". One idea would be to actually generate random IDs, instead of hard coding a value? Could do a dash-less UUID, for example.

would lead to more noise in the logs

It's a debug log, and we're not adding any new lines, just a few characters. Doesn't seem like a big deal to me.

I think it's orthogonal to this PR, but defaulting to something like bitcoin-cli-<version> instead of 1 could be sensible. What benefit would a random ID provide when it's not returned to the user?

I like the idea of changing the default to bitcoin-cli-<version>! But agree on your points wrt. orthogonality and simplicity. Will fix!

nit: would be nice to parameterize DEFAULT_RPC_REQ_ID

Since we're casting the rpcid arg to VSTR, I think it's reasonable to also have the default value be a string, in which case this can be simplified:

<details> <summary>git diff on a2ecbc60e4</summary>

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index b6aa478719..3455aea73b 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -57,6 +57,7 @@ using CliClock = std::chrono::system_clock;
 const TranslateFn G_TRANSLATION_FUN{nullptr};
 
 static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
+static constexpr const char* DEFAULT_RPC_REQ_ID{"1"};
 static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
 static constexpr int DEFAULT_WAIT_CLIENT_TIMEOUT = 0;
 static const bool DEFAULT_NAMED=false;
@@ -100,7 +101,7 @@ static void SetupCliArgs(ArgsManager& argsman)
     SetupChainParamsBaseOptions(argsman);
     argsman.AddArg("-color=<when>", strprintf("Color setting for CLI output (default: %s). Valid values: always, auto (add color codes when standard output is connected to a terminal and OS is not WIN32), never. Only applies to the output of -getinfo.", DEFAULT_COLOR_SETTING), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
     argsman.AddArg("-named", strprintf("Pass named instead of positional arguments (default: %s)", DEFAULT_NAMED), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-rpcid=<id>", "Set a custom JSON-RPC request ID string (default: 1)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION | ArgsManager::DISALLOW_ELISION, OptionsCategory::OPTIONS);
+    argsman.AddArg("-rpcid=<id>", strprintf("Set a custom JSON-RPC request ID string (default: %s)", DEFAULT_RPC_REQ_ID), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION | ArgsManager::DISALLOW_ELISION, OptionsCategory::OPTIONS);
     argsman.AddArg("-rpcclienttimeout=<n>", strprintf("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)", DEFAULT_HTTP_CLIENT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-rpcconnect=<ip>", strprintf("Send commands to node running on <ip> (default: %s)", DEFAULT_RPCCONNECT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-rpccookiefile=<loc>", "Location of the auth cookie. Relative paths will be prefixed by a net-specific datadir location. (default: data dir)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
@@ -788,12 +789,7 @@ struct DefaultRequestHandler : BaseRequestHandler {
         } else {
             params = RPCConvertValues(method, args);
         }
-        UniValue id;
-        if (auto rpcid = gArgs.GetArg("-rpcid")) {
-            id = UniValue(UniValue::VSTR, *rpcid);
-        } else {
-            id = UniValue(1);
-        }
+        UniValue id{UniValue::VSTR, gArgs.GetArg("-rpcid", DEFAULT_RPC_REQ_ID)};
         return JSONRPCRequestObj(method, params, id);
     }
 

</details>

I would really prefer to avoid nesting fmt strings, it's so much harder to read. Just printing an empty "id=" in case of null seems straightforward, consistent and simple.

nit: log lines no longer need a newline at the end, so would be good to clean up while they're touched.

<details> <summary>git diff on 2ad6416eb2</summary>

diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index acfd85b0b0..e1d48eb8ed 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -233,13 +233,13 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
     if (!valMethod.isStr())
         throw JSONRPCError(RPC_INVALID_REQUEST, "Method must be a string");
     strMethod = valMethod.get_str();
-    const std::string log_id{id && !id->isNull() ? strprintf(" id=%s", id->getValStr()) : ""};
+    auto req_id{id.value_or(UniValue{}).getValStr()};
     if (fLogIPs)
-        LogDebug(BCLog::RPC, "ThreadRPCServer method=%s user=%s peeraddr=%s%s\n", SanitizeString(strMethod),
-            this->authUser, this->peerAddr, log_id);
+        LogDebug(BCLog::RPC, "ThreadRPCServer method=%s user=%s peeraddr=%s id=%s", SanitizeString(strMethod),
+            this->authUser, this->peerAddr, req_id);
     else
-        LogDebug(BCLog::RPC, "ThreadRPCServer method=%s user=%s%s\n", SanitizeString(strMethod), this->authUser,
-            log_id);
+        LogDebug(BCLog::RPC, "ThreadRPCServer method=%s user=%s id=%s", SanitizeString(strMethod), this->authUser,
+        req_id);
 
     // Parse params
     const UniValue& valParams{request.find_value("params")};

</details>