/bitcoin

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

Merged
merged 2 commits into from Jul 15, 2017
+31 −22

Conversation

Reviewers

@laanwj laanwj

@sipa sipa

Assignees
No one assigned
Labels
Scripts and tools
Projects
None yet
Milestone
No milestone
7 participants
@jnewbery @MeshCollider @jonasschnelli @theuni @laanwj @TheBlueMatt @sipa
@jnewbery
Member

jnewbery commented Jul 12, 2017

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
@jnewbery jnewbery [refactor] move SplitHostPort() into utilstrencodings 
This moves SplitHostPort from libbitcoin_common to libbitcoin_util so it
is available to bitcoin-cli.
 fe4faba
@MeshCollider

MeshCollider commented Jul 13, 2017

utACK 7b77805
@jonasschnelli
Member

jonasschnelli commented Jul 13, 2017

utACK 7b77805

jonasschnelli added the Scripts and tools label Jul 13, 2017

src/bitcoin-cli.cpp
- std::string host = GetArg("-rpcconnect", DEFAULT_RPCCONNECT);
+ std::string host;
+ int dummy_port;
+ SplitHostPort(GetArg("-rpcconnect", DEFAULT_RPCCONNECT), dummy_port, host);
@laanwj

laanwj Jul 13, 2017 • edited

Owner

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

jnewbery Jul 13, 2017

Member

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

laanwj Jul 13, 2017

Owner

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

jnewbery Jul 13, 2017

Member

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:

  1. -rpcport if there is one; else
  2. port in -rpcconnect if there is one; else
  3. default port for the chain
@jnewbery jnewbery [utils] allow square brackets for ipv6 addresses in bitcoin-cli 
-rpcconnect can now accept ipv6 addresses with and without square
brackets.
5c64324
@theuni
Member

theuni commented Jul 13, 2017

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 5c64324.
@laanwj
Owner

laanwj commented Jul 14, 2017

utACK, looks good to me now, thanks!
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 14, 2017

utACK 5c64324
src/utilstrencodings.cpp
@@ -91,6 +91,25 @@ std::vector<unsigned char> ParseHex(const std::string& str)
return ParseHex(str.c_str());
}
+void SplitHostPort(std::string in, int &portOut, std::string &hostOut) {
@sipa

sipa Jul 15, 2017

Owner

This really doesn't belong in strencodings.

@jnewbery

jnewbery Jul 15, 2017

Member

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

sipa Jul 15, 2017

Owner

Hmm, ok.

@sipa
Owner

sipa commented Jul 15, 2017

utACK 5c64324

@sipa sipa merged commit 5c64324 into bitcoin:master Jul 15, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sipa sipa added a commit that referenced this pull request Jul 15, 2017

@sipa sipa Merge #10812: [utils] Allow bitcoin-cli's -rpcconnect option to be us… 
…ed with square brackets


5c64324 [utils] allow square brackets for ipv6 addresses in bitcoin-cli (John Newbery)
fe4faba [refactor] move SplitHostPort() into utilstrencodings (John Newbery)

Pull request description:

  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

Tree-SHA512: 249d409f10360c989474283341f458cc97364a56a7d004ae6d5f13d8bffe3a51b5dc2484d42218848e2d42cd9c0b13a1b92e94ea19b209f7e91c875c208d8409
This commit was signed with a verified signature.
@sipa sipa Pieter Wuille
GPG key ID: A636E97631F767E0 Learn about signing commits
c5904e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment