highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-05-30 17:50:44 +02:00
Code Issues Packages Projects Releases Wiki Activity

net/rpc: Check all resolved addresses in ConnectNode rather than just one

Browse Source 
The current `addnode` rpc command has some edge cases in where it is possible to
connect to the same node twice by combining ip and address requests. This can happen under two situations:

The two commands are run one right after each other, in which case they will be processed
under the same loop in `CConnman::ThreadOpenAddedConnections` without refreshing `vInfo`, so both
will go trough. An example of this would be:

```
bitcoin-cli addnode "localhost:port" add

```

A node is added by IP using `addnode "add"` while the other is added by name using
`addnode "onetry"` with an address that resolves to multiple IPs. In this case, we currently
only check one of the resolved IPs (picked at random), instead of all the resolved ones, meaning
this will only probabilistically fail/succeed. An example of this would be:

```
bitcoin-cli addnode "127.0.0.1:port" add
[...]
bitcoin-cli addnode "localhost:port" onetry
```

Both cases can be fixed by iterating over all resolved addresses in `CConnman::ConnectNode` instead
of picking one at random
This commit is contained in:
Sergi Delgado Segura 2023-07-25 11:50:47 -04:00
parent 97f756b12c
commit 2574b7e177
1 changed files with 18 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
src/net.cpp
View File

@ -464,21 +464,25 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
const uint16_t default_port{pszDest != nullptr ? GetDefaultPort(pszDest) :
m_params.GetDefaultPort()};
if (pszDest) {
const std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
if (!resolved.empty()) {
const CService& rnd{resolved[GetRand(resolved.size())]};
addrConnect = CAddress{MaybeFlipIPv6toCJDNS(rnd), NODE_NONE};
if (!addrConnect.IsValid()) {
LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest);
return nullptr;
}
// It is possible that we already have a connection to the IP/port pszDest resolved to.
// In that case, drop the connection that was just created.
LOCK(m_nodes_mutex);
CNode* pnode = FindNode(static_cast<CService>(addrConnect));
if (pnode) {
LogPrintf("Failed to open new connection, already connected\n");
return nullptr;
Shuffle(resolved.begin(), resolved.end(), FastRandomContext());
// If the connection is made by name, it can be the case that the name resolves to more than one address.
// We don't want to connect any more of them if we are already connected to one
for (const auto& r : resolved) {
addrConnect = CAddress{MaybeFlipIPv6toCJDNS(r), NODE_NONE};
if (!addrConnect.IsValid()) {
LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest);
return nullptr;
}
// It is possible that we already have a connection to the IP/port pszDest resolved to.
// In that case, drop the connection that was just created.
LOCK(m_nodes_mutex);
CNode* pnode = FindNode(static_cast<CService>(addrConnect));
if (pnode) {
LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort());
return nullptr;
}
}
}
}