[utils] Allow bitcoin-cli's -rpcconnect option to be used with square brackets #10812

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:bitcoin_cli_ipv6 changing 7 files +31 −22

jnewbery commented at 10:09 PM on July 12, 2017: member
bitcoin-cli's -rpcconnect can accept ipv6 addresses (as long as the libevent version is new enough), but fails to parse ipv6 with square brackets. This PR makes bitcoin-cli parse ipv6 in square brackets correctly.

bitcoin-cli -rpcconnect=[::1] <command>

should now be equivalent to

bitcoin-cli -rpcconnect=::1 <command>

This is useful so the bitcoin-cli option can now be in the same format as the bitcoind option.

Doesn't include tests. I have a branch that fully tests bitcoin-cli, but that's queued behind several intermediate PRs.
- first commit moves SplitHostPort() from libbitcoin_common into libbitcoin_util
- second commit adds proper ipv6 parsing to bitcoin-cli

[refactor] move SplitHostPort() into utilstrencodings

This moves SplitHostPort from libbitcoin_common to libbitcoin_util so it
is available to bitcoin-cli.

fe4fabaf12

meshcollider commented at 7:09 AM on July 13, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/commit/7b7780592185f04be382065c3b1c9e12cac2449f
jonasschnelli commented at 7:12 AM on July 13, 2017: contributor

utACK 7b7780592185f04be382065c3b1c9e12cac2449f
jonasschnelli added the label Scripts and tools on Jul 13, 2017
in src/bitcoin-cli.cpp:197 in 7b77805921 outdated
```
 191 | @@ -191,7 +192,9 @@ static void http_error_cb(enum evhttp_request_error err, void *ctx)
 192 |  
 193 |  UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 194 |  {
 195 | -    std::string host = GetArg("-rpcconnect", DEFAULT_RPCCONNECT);
 196 | +    std::string host;
 197 | +    int dummy_port;
 198 | +    SplitHostPort(GetArg("-rpcconnect", DEFAULT_RPCCONNECT), dummy_port, host);
```
laanwj commented at 3:34 PM on July 13, 2017:

If you're going to do this anyway, why not allow overriding the port? (e.g. pass port instead of dummy_port, to make it possible to override the rpcport)

After all, the point of bracketed addresses is to allow [ip]:port specifications unambigiously.

jnewbery commented at 5:15 PM on July 13, 2017:

I chose not to allow overriding the port since we already have the -rpcport option, and this is the minimal functional change.

I'm equally happy to make the one-line change so -rpcconnect can accept <address>:<port>. I have zero preference either way and I'm happy to go along with consensus opinion. @theuni has already expressed slight preference for allowing a port number in -rpcconnect.

laanwj commented at 6:08 PM on July 13, 2017:

I'd really prefer to be able to override the port in rpcconnect. FYI we have a similar thing at the server side with bind which can either take a address and port (overriding -port) or just an address (in which case -port is used). If not I'd rather NACK this. It is just confusing to parse an [ip]:port spec if you're going to ignore the port.
jnewbery commented at 9:16 PM on July 13, 2017:

sure - changed to use the port from -rpcconnect if it's specified.

It was a bit more than a one line change because it was a little fiddly to get the precedence correct:

-rpcport if there is one; else

port in -rpcconnect if there is one; else

default port for the chain

[utils] allow square brackets for ipv6 addresses in bitcoin-cli

-rpcconnect can now accept ipv6 addresses with and without square
brackets.

5c643241e5

jnewbery force-pushed on Jul 13, 2017
theuni commented at 9:54 PM on July 13, 2017: member

I agree with @laanwj that this should match how our other ip/port strings are parsed. I was surprised when this didn't work already.

utACK 5c643241e5d98992c0d5487b65e9c7b6d8005d1a.
laanwj commented at 6:49 AM on July 14, 2017: member

utACK, looks good to me now, thanks!
TheBlueMatt commented at 8:53 PM on July 14, 2017: member

utACK 5c643241e5d98992c0d5487b65e9c7b6d8005d1a
in src/utilstrencodings.cpp:94 in 5c643241e5
```
  90 | @@ -91,6 +91,25 @@ std::vector<unsigned char> ParseHex(const std::string& str)
  91 |      return ParseHex(str.c_str());
  92 |  }
  93 |  
  94 | +void SplitHostPort(std::string in, int &portOut, std::string &hostOut) {
```
sipa commented at 12:30 AM on July 15, 2017:

This really doesn't belong in strencodings.

jnewbery commented at 7:34 PM on July 15, 2017:

It's decoding a string into two parts and calls ParseInt32().

Happy to take suggestions of a better place to put this. It needs to be in libbitcoin_util to make it available to bitcoin-cli. bitcoin-cli doesn't link libbitcoin_common.

sipa commented at 8:15 PM on July 15, 2017:

Hmm, ok.
sipa commented at 8:16 PM on July 15, 2017: member

utACK 5c643241e5d98992c0d5487b65e9c7b6d8005d1a
sipa merged this on Jul 15, 2017
sipa closed this on Jul 15, 2017
sipa referenced this in commit c5904e8714 on Jul 15, 2017
PastaPastaPasta referenced this in commit 60f31e0462 on Jul 6, 2019
PastaPastaPasta referenced this in commit d1fde530f7 on Jul 8, 2019
PastaPastaPasta referenced this in commit d437172182 on Jul 9, 2019
PastaPastaPasta referenced this in commit b98e0a3b35 on Jul 11, 2019
PastaPastaPasta referenced this in commit 4a1560993c on Jul 13, 2019
PastaPastaPasta referenced this in commit a836064372 on Jul 13, 2019
PastaPastaPasta referenced this in commit e871c7cd00 on Jul 17, 2019
PastaPastaPasta referenced this in commit 726a9b6db4 on Jul 17, 2019
PastaPastaPasta referenced this in commit 483786a725 on Jul 18, 2019
barrystyle referenced this in commit c424923a0d on Jan 22, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels