Use real bind address in RPC help curl example #15158

domob1812 commented at 10:01 AM on January 13, 2019: contributor

Expose (one of) the real bind addresses for the HTTP server and use that in the RPC help text's curl example. This makes sure that e.g. a differently configured RPC port (or just the RPC port for regtest/testnet) is reported there, so that the example can be used directly as is even if a non-standard configuration is applied or the daemon is not running on mainnet.

fanquake added the label RPC/REST/ZMQ on Jan 13, 2019

laanwj commented at 12:27 PM on January 14, 2019: member

Tend towards NACK, sorry, on one hand this adds a small convenience but—

This adds a forbidden dependency; RPC is supposed to be independent of the backend (HTTP, GUI console, potentially in the future other things), so there is supposed to be no direct dependency of rpcserver.cpp on httpserver.h
99% of users will use the default localhost bind address, and those that don't, likely understand they need to use that different address

The former could be worked around by making httprpc query and register the first bind address. But as said, I'm not sure it's worth it.

Use real bind address in RPC help curl example.

Expose (one of) the real bind addresses for the HTTP server and use that
in the RPC help text's curl example.  This makes sure that e.g. a
differently configured RPC port (or just the RPC port for regtest/testnet)
is reported there, so that the example can be used directly as is even
if a non-standard configuration is applied or the daemon is not mainnet.

6e64d177b7

in src/httpserver.cpp:335 in 66b0f78467 outdated

 330 | @@ -327,6 +331,11 @@ static bool HTTPBindAddresses(struct evhttp* http)
 331 |                  LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
 332 |              }
 333 |              boundSockets.push_back(bind_handle);
 334 | +            if (firstBindAddress.empty()) {
 335 | +                std::ostringstream addr;

practicalswift commented at 1:34 PM on January 14, 2019:

Avoid shadowing addr in outer scope :-)

domob1812 commented at 8:13 AM on January 15, 2019:

Oops, fixed. Good catch!

domob1812 force-pushed on Jan 15, 2019

domob1812 commented at 8:16 AM on January 15, 2019: contributor

@laanwj: No worries, I expected this to be not a clear case. But isn't giving an explicit curl example in the RPC help text a clear dependence on HTTP as backend in the first place? If we do that (as we do now), then I think it is good to make it at least correct.

However, a simpler version could be to just use the default (or configured) RPC port from the chain config instead of the hardcoded 8332. That way, it would at least be a bit more correct, especially for things like running on regtest. That is definitely a lot more relevant than an explicit non-standard bind address.

laanwj commented at 1:52 PM on January 15, 2019: member

But isn't giving an explicit curl example in the RPC help text a clear dependence on HTTP as backend in the first place? If we do that (as we do now), then I think it is good to make it at least correct.

Right, it is a a semantic dependency, not a code dependency. If you want to go all the way to modular design, you'd have a function where the backend (such as httprpc) can register RPC example help generation functions, which are then called by the RPC help generator and in turn appended to the help for a call.

domob1812 commented at 7:01 PM on January 15, 2019: contributor

If you want to go all the way to modular design, you'd have a function where the backend (such as httprpc) can register RPC example help generation functions, which are then called by the RPC help generator and in turn appended to the help for a call.

True, but that seems like overengineering to me. Do you mean to say that you'd prefer that, or is that just a hypothetical example?

So what about using either just the default RPC port from chainparams instead of hardcoding 8332, or perhaps using the rpcport if configured? Neither of that would create a code dependency of the RPC code on the HTTP backend.

laanwj commented at 1:35 PM on January 16, 2019: member

True, but that seems like overengineering to me. Do you mean to say that you'd prefer that, or is that just a hypothetical example?

No, what I'd prefer is to not do this at all, as I said I doubt it is worth it. Sorry if I was unclear.

So what about using either just the default RPC port from chainparams instead of hardcoding 8332, or perhaps using the rpcport if configured? Neither of that would create a code dependency of the RPC code on the HTTP backend.

That would be OK with me, seems straightforward and doesn't add too much code.

domob1812 commented at 4:06 PM on January 16, 2019: contributor

So what about using either just the default RPC port from chainparams instead of hardcoding 8332, or perhaps using the rpcport if configured? Neither of that would create a code dependency of the RPC code on the HTTP backend.

That would be OK with me, seems straightforward and doesn't add too much code.

Sounds good to me. I'll close this PR then and work on one that just uses the default RPC port for the current chain.

domob1812 closed this on Jan 16, 2019

domob1812 deleted the branch on Jan 16, 2019

DrahtBot locked this on Dec 16, 2021