This finishes the work started in #7868. The changed lines count is high, but it’s mostly tests.
CNetAddr/CService/CSubNet lose their string constructors, they must now have lookup operations performed externally. This means that functions/classes that depend on them are no longer dependent on any particular lookup mechanism.
From a high level: New resolvers/parsers may now be used for net operations. libbtcnet will replace what remains of netbase with async versions.
1289- }
1290- else
1291- {
1292- valid = false;
1293- }
1294+ if (ParseInt32(strNetmask, &n)) { // If valid number, assume /24 symtex
Net functionality is no longer needed for CAddress/CAddrman/etc. now that
CNetAddr/CService/CSubNet are dumb storage classes.
105@@ -106,6 +106,7 @@ BITCOIN_CORE_H = \
106 miner.h \
107 net.h \
108 netbase.h \
109+ netaddress.h \
netaddress.h one line up to match the alphabet and the cpp files order below.
202+ CNetAddr localv4;
203+ CNetAddr localv6;
204+ LookupHost("127.0.0.1", localv4, false);
205+ LookupHost("::1", localv6, false);
206+ rpc_allow_subnets.push_back(CSubNet(localv4, 8)); // always allow IPv4 local subnet
207+ rpc_allow_subnets.push_back(CSubNet(localv6)); // always allow IPv6 localhost
208 if (mapMultiArgs.count("-rpcallowip")) {
209 const std::vector<std::string>& vAllow = mapMultiArgs["-rpcallowip"];
210 for (std::string strAllow : vAllow) {
211- CSubNet subnet(strAllow);
212+ CSubNet subnet;
213+ LookupSubNet(strAllow.c_str(), subnet);
Hmm. Why char * instead of std::string in prototypes and c_str() everywhere?
Edit: probably to keep the old/already used semantics?
utACK https://github.com/bitcoin/bitcoin/pull/8128/commits/21ba407a7369a0229b8a8554dee0da63a64e6639
This also helped reducing shadow warnings from the net* codes (e.g. for randomize_credentials: from 309 occurrences in the build log to 42 only).
Also fix up a few small issues:
- Lookup with "badip:port" now sets the port to 0
- Don't allow assert to have side-effects
618@@ -614,7 +619,7 @@ CService HTTPRequest::GetPeer()
619 const char* address = "";
620 uint16_t port = 0;
621 evhttp_connection_get_peer(con, (char**)&address, &port);
622- peer = CService(address, port);
623+ LookupNumeric(address, peer, port);
You ignore the return value of Lookup* almost everywhere. Is this on purpose?
I think the old behavior was to return a service with !IsValid() when the lookup fails. But if that is still the case we could just as well have the API be:
0peer = LookupNumeric(address, port);
Then check for IsValid afterward.
Yes, this is inconsistent. The return value was intended to be a shortcut to avoid having to check for IsValid afterwards, but most places end up forwarding the result elsewhere even if invalid.
I’ll just remove the return.
461@@ -462,7 +462,9 @@ void TorController::auth_cb(TorControlConnection& conn, const TorControlReply& r
462 // Now that we know Tor is running setup the proxy for onion addresses
463 // if -onion isn't set to something else.
464 if (GetArg("-onion", "") == "") {
465- proxyType addrOnion = proxyType(CService("127.0.0.1", 9050), true);
466+ CService resolved;
467+ assert(LookupNumeric("127.0.0.1", resolved, 9050));
Whoops, missed @paveljanik’s batch. @paveljanik Logic for the cstr’s only was that they’re going to be used as pointers for the lookup anyway. That kinda breaks down though since we end up using strings for pre-processing. I can add a string version as well if you’d like.
Will fix up the nits.
@paveljanik Looking at the others, I’d rather not add a string method here. The most logical (imo) choices are:
I think I’d prefer to leave it as-is unless there’s a compelling reason otherwise.
char* here. C string manipulation, because of the inherent buffer overflow risks, should not be used in any new code, but there is no need to refactor this right now in old code, or new code closely modeled after the old code. And these strings are not manipulated anyhow, but directly passed through to libevent/the OS, so nothing would be won by making them std::string.
Labels
P2P