Repository: bitcoin/bitcoin
File: src/net.cpp (C++)

Code before:
bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
{
    return FindNode(static_cast<CNetAddr>(addr));
}

bool CConnman::CheckIncomingNonce(uint64_t nonce)
{
    LOCK(m_nodes_mutex);
    for (const CNode* pnode : m_nodes) {
        if (!pnode->fSuccessfullyConnected && !pnode->IsInboundConn() && pnode->GetLocalNonce() == nonce)
            return false;
    }
    return true;
}

/** Get the bind address for a socket as CService. */
static CService GetBindAddress(const Sock& sock)
{
    CService addr_bind;
    struct sockaddr_storage sockaddr_bind;
    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
    if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind, sockaddr_bind_len);
    } else {
        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
    }
    return addr_bind;
}

CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport)
{
    AssertLockNotHeld(m_unused_i2p_sessions_mutex);
    assert(conn_type != ConnectionType::INBOUND);

    if (pszDest == nullptr) {
        if (IsLocal(addrConnect))
            return nullptr;

        // Look for an existing connection
        CNode* pnode = FindNode(static_cast<CService>(addrConnect));
        if (pnode)
        {
            LogPrintf("Failed to open new connection, already connected\n");
            return nullptr;
        }
    }

    LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "trying %s connection %s lastseen=%.1fhrs\n",
        use_v2transport ? "v2" : "v1",
        pszDest ? pszDest : addrConnect.ToStringAddrPort(),
        Ticks<HoursDouble>(pszDest ? 0h : Now<NodeSeconds>() - addrConnect.nTime));

Review comment (bug):
This rename makes obvious that maybe something is wrong in the current code in `master`. Is the intention really to ignore inbound connections when, according to the comment we intent to "Look for an existing connection"?

For example, if `1.1.1.1` connects to `2.2.2.2:8333`, then the TCP connection will look like:
`1.1.1.1:53481 <-> 2.2.2.2:8333` where `53481` is a random port picked by the initiator's OS. Now, if `2.2.2.2` intends to connect to `1.1.1.1:8333` then this check will return false because `1.1.1.1:53481` is not `1.1.1.1:8333` and the code will proceed with connecting.

Is this really the intention? Shouldn't this check ignore the port?

Code after:
    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
    if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind, sockaddr_bind_len);
    } else {
        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
    }
    return addr_bind;
}

CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport)
{
    AssertLockNotHeld(m_unused_i2p_sessions_mutex);
    assert(conn_type != ConnectionType::INBOUND);

    if (pszDest == nullptr) {
        if (IsLocal(addrConnect))
            return nullptr;

        // Look for an existing connection
        if (AlreadyConnectedToAddressPort(addrConnect)) {
            LogInfo("Failed to open new connection to %s, already connected", addrConnect.ToStringAddrPort());
            return nullptr;
        }
    }

    LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "trying %s connection %s lastseen=%.1fhrs\n",
        use_v2transport ? "v2" : "v1",
        pszDest ? pszDest : addrConnect.ToStringAddrPort(),
        Ticks<HoursDouble>(pszDest ? 0h : Now<NodeSeconds>() - addrConnect.nTime));

    // Resolve
    const uint16_t default_port{pszDest != nullptr ? GetDefaultPort(pszDest) :
                                                     m_params.GetDefaultPort()};

    // Collection of addresses to try to connect to: either all dns resolved addresses if a domain name (pszDest) is provided, or addrConnect otherwise.
    std::vector<CAddress> connect_to{};
    if (pszDest) {
        std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
        if (!resolved.empty()) {
            std::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()) {
                    LogDebug(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.
                if (AlreadyConnectedToAddressPort(addrConnect)) {