Fix: when testnet=1 specified, change default RPC port to 18332 #1862

kjj2 commented at 3:54 PM on September 24, 2012: none

Fix: when testnet=1 specified, change default RPC port to 18332

jgarzik commented at 5:15 PM on September 24, 2012: contributor

ACK

in src/protocol.h:None in 767f9a4e8b outdated

  21 | @@ -21,6 +22,10 @@ static inline unsigned short GetDefaultPort(const bool testnet = fTestNet)
  22 |      return testnet ? 18333 : 8333;
  23 |  }
  24 |  
  25 | +static inline unsigned short GetDefaultRPCPort()

gavinandresen commented at 5:23 PM on September 24, 2012:

The RPC port has nothing to do with the p2p protocol, so this doesn't belong in protocol.h.

Should go at the top of bitcoinrpc.cpp, in my humble opinion.

Diapolo commented at 5:53 PM on September 24, 2012:

ACK @gavinandresen

in src/init.cpp:None in 767f9a4e8b outdated

 271 | @@ -272,7 +272,7 @@ bool static Bind(const CService &addr, bool fError = true) {
 272 |  #endif
 273 |          "  -rpcuser=<user>        " + _("Username for JSON-RPC connections") + "\n" +
 274 |          "  -rpcpassword=<pw>      " + _("Password for JSON-RPC connections") + "\n" +
 275 | -        "  -rpcport=<port>        " + _("Listen for JSON-RPC connections on <port> (default: 8332)") + "\n" +
 276 | +        "  -rpcport=<port>        " + _("Listen for JSON-RPC connections on <port> (default: 8332 or Testnet: 18332)") + "\n" +

Diapolo commented at 5:52 PM on September 24, 2012:

testnet should be lowercase

Diapolo commented at 8:17 PM on September 24, 2012: none

Looking good now :).

gavinandresen commented at 8:24 PM on September 24, 2012: contributor

Squash them into one commit and it is perfect.

Fix: when testnet=1 specified, change default ports to 18332 and 18333 b202d43076

kjj2 commented at 8:27 PM on September 24, 2012: none

Done.

in src/bitcoinrpc.cpp:None in b202d43076

1059 | @@ -1055,7 +1060,7 @@ Object CallRPC(const string& strMethod, const Array& params)
1060 |      asio::ssl::stream<asio::ip::tcp::socket> sslStream(io_service, context);
1061 |      SSLIOStreamDevice<asio::ip::tcp> d(sslStream, fUseSSL);
1062 |      iostreams::stream< SSLIOStreamDevice<asio::ip::tcp> > stream(d);
1063 | -    if (!d.connect(GetArg("-rpcconnect", "127.0.0.1"), GetArg("-rpcport", "8332")))
1064 | +    if (!d.connect(GetArg("-rpcconnect", "127.0.0.1"), GetArg("-rpcport", itostr(GetDefaultRPCPort()))))

Diapolo commented at 9:10 PM on September 24, 2012:

itostr() does strprintf("%d", n), which is for signed integers, so should we better use strprintf("%u", GetDefaultRPCPort()) instead, as we have an unsigned short as port number?

kjj2 commented at 11:49 PM on September 24, 2012:

My understanding is that this is well defined. The implicit type conversion is from unsigned short to signed int, which is always safe. (Since the short is unsigned, there is no sign to extend.) The compiler doesn't even blink at this with -Wall, but it does gripe often about the 3 lines in net.h that assign a literal -1 to an unsigned int.

Diapolo commented at 5:03 AM on September 25, 2012:

Alright I'm fine with this patch then, but now I'm interested in which lines give you that signed/unsigned warning, as we are always interested in finding and fixing these :).

kjj2 commented at 5:12 AM on September 25, 2012:

net.h, lines 219, 353, 388. All three lines have nMessageStart = -1; but nMessageStart is unsigned int from line 167. There are other warnings that pop up when I compile, but net.h seems to be in just about everything, so I see them over and over again when I build. Does your compiler not warn on them? Mine is a bit old.

laanwj commented at 8:46 AM on September 25, 2012:

No problem here, it is entirely correct: calling itostr casts the unsigned short to a signed integer (which is larger, so there is never undefined behavior), before feeding it into strprintf as a signed integer (%d).

Diapolo commented at 9:15 AM on September 25, 2012:

@kjj2 OT on net.h: AFAIK -1 for an unsigned var means set all bits, which should be correct. I'm not sure if it is intended, but valid ^^. Oh and my compiler doesn't warn there (GCC 4.7.0).

laanwj commented at 9:42 AM on September 25, 2012:

You could also use ~0 to specify "set all bits". It's less controversial. Though I think (unsigned) -1 is correct. Overflow when converting from signed to unsigned never results in undefined behavior, don't try it the other way, though...