This change exposes the bind address (one of them if multiple) of the HTTP server. This allows us to use a real bind address in the curl example in RPC help texts.
In particular, this now reports the correct port if it was either explicitly changed through -rpcport or is from a different chain than mainnet.
This is a replacement for #15181, based on the feedback from @laanwj there.
This includes some trivial refactoring (static replaced by anonymous namespaces) in places that I changed anyway - I think this would not be worth doing in a separate PR. But of course I'm happy to either remove those changes or submit them in a separate commit / PR if you think that's better.
489 | @@ -478,6 +490,15 @@ void StopHTTPServer() 490 | LogPrint(BCLog::HTTP, "Stopped HTTP server\n"); 491 | } 492 | 493 | +bool GetHTTPBindAddress(std::string& address) 494 | +{ 495 | + if (bindAddress.empty()) { 496 | + return false;
address = bindAddress;
return !address.empty();
Yes, but that would overwrite the passed in address. While it doesn't matter in the current context, I found it cleaner to not affect it at all if false is returned.
This has unnecessary/unrelated changes.
This has unnecessary/unrelated changes.
See my comment above. I feel those changes are an improvement and too insignificant to do on their own, but I'm happy to either split them out or not do them at all.
At least have in a separate commit. If they are an improvement and unrelated here then a separate PR makes more sense.
Use anonymous namespaces in some places instead of the static keyword.
Both result in internal linkage, but anonymous namespaces are a more
modern way to do it.
At least have in a separate commit. If they are an improvement and unrelated here then a separate PR makes more sense.
Agreed, I've split it into two commits for now. As I wrote above, I'm happy to drop the first one or submit in a separate PR if you think that makes more sense.
This is a lot of code for making an example more "real". Generally, I'd prefer if the example was static, since it ends up on static websites (https://bitcoincore.org/en/doc/0.16.0/rpc/rawtransactions/getrawtransaction/ or http://chainquery.com/bitcoin-api/getrawtransaction)
Wouldn't it make more sense to include that information in the getrpcinfo command as suggested by @instagibbs #14982 (comment)?
This is a lot of code for making an example more "real". Generally, I'd prefer if the example was static, since it ends up on static websites (https://bitcoincore.org/en/doc/0.16.0/rpc/rawtransactions/getrawtransaction/ or http://chainquery.com/bitcoin-api/getrawtransaction)
Wouldn't it make more sense to include that information in the getrpcinfo command as suggested by @instagibbs #14982 (comment)?
I don't know. I just noticed that the port can be plain wrong in the static example, for instance if running in regtest mode. (Which is actually something that developers might do and where they might want to use the curl example to access their node!)
@laanwj's last opinion was that he would be happy with that change in the form it is now, at least if I understood him correctly. But since he would also be happy not doing the fix and I don't care too strongly about it (especially with the amount of back and forth this is getting to), I'm happy to just drop this attempt finally.
Agree with @MarcoFalke, the help text is an example.
44 | @@ -43,6 +45,10 @@ def test_categories(self): 45 | 46 | assert_equal(titles, components) 47 | 48 | + def test_exampleBindAddress(self):
Should be snake_case? :-)
Fixed
This change exposes the bind address (one of them if multiple) of the HTTP
server. This allows us to use a real bind address in the "curl" example
in RPC help texts.
In particular, this now reports the correct port if it was either
explicitly changed through -rpcport or is from a different chain than
mainnet.
44 | @@ -43,6 +45,10 @@ def test_categories(self): 45 | 46 | assert_equal(titles, components) 47 | 48 | + def test_exampleBindAddress(self): 49 | + helpText = self.nodes[0].help('getblockchaininfo')
Same here -- should be snake_case? :-)
Fixed
I've fixed the two nits found by @practicalswift. But if the general opinion is that we should just not do this, then feel free to just close the PR.
I've fixed the two nits found by @practicalswift. But if the general opinion is that we should just not do this, then feel free to just close the PR.
Tend to agree. This just keeps dragging on.
But it's better to do that yourself! (than you still have control over it)
I've fixed the two nits found by @practicalswift. But if the general opinion is that we should just not do this, then feel free to just close the PR.
Tend to agree. This just keeps dragging on.
But it's better to do that yourself! (than you still have control over it)
Sounds good, then just let me close it. :)