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

Code before:
                return false; // Wasn't response to what we expected, try receiving next packet.
            }
            if (response[0] != NATPMP_VERSION || response[1] != (NATPMP_RESPONSE | NATPMP_OP_MAP_TCP)) {
                LogWarning("natpmp: Response to wrong command\n");
                return false; // Wasn't response to what we expected, try receiving next packet.
            }
            uint16_t internal_port = ReadBE16(response.data() + NATPMP_MAP_RESPONSE_INTERNAL_PORT_OFS);
            if (internal_port != port) {
                LogWarning("natpmp: Response port doesn't match request\n");
                return false; // Wasn't response to what we expected, try receiving next packet.
            }
            return true;
        },
        interrupt);

    if (recv_res) {
        const std::span<uint8_t> response = *recv_res;

        Assume(response.size() >= NATPMP_MAP_RESPONSE_SIZE);
        uint16_t result_code = ReadBE16(response.data() + NATPMP_RESPONSE_HDR_RESULT_OFS);
        if (result_code != NATPMP_RESULT_SUCCESS) {
            if (result_code == NATPMP_RESULT_NOT_AUTHORIZED) {
                static bool warned{false};
                if (!warned) {
                    LogWarning("natpmp: Port mapping failed with result %s\n", NATPMPResultString(result_code));
                    warned = true;
                } else {
                    LogDebug(BCLog::NET, "natpmp: Port mapping failed with result %s\n", NATPMPResultString(result_code));
                }
            } else {
                LogWarning("natpmp: Port mapping failed with result %s\n", NATPMPResultString(result_code));
            }
            if (result_code == NATPMP_RESULT_NO_RESOURCES) {
                return MappingError::NO_RESOURCES;
            }
            return MappingError::PROTOCOL_ERROR;
        }

        uint32_t lifetime_ret = ReadBE32(response.data() + NATPMP_MAP_RESPONSE_LIFETIME_OFS);
        uint16_t external_port = ReadBE16(response.data() + NATPMP_MAP_RESPONSE_EXTERNAL_PORT_OFS);
        return MappingResult(NATPMP_VERSION, CService(internal.sin_addr, port), CService(external_addr, external_port), lifetime_ret);
    } else {
        return MappingError::NETWORK_ERROR;
    }
}

std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonce &nonce, const CNetAddr &gateway, const CNetAddr &bind, uint16_t port, uint32_t lifetime, CThreadInterrupt& interrupt, int num_tries, std::chrono::milliseconds timeout_per_try)
{
    struct sockaddr_storage dest_addr, bind_addr;
    socklen_t dest_addrlen = sizeof(struct sockaddr_storage), bind_addrlen = sizeof(struct sockaddr_storage);


Review comment (bug):
You're modifying a global variable here with no particular assurances that this is only ever called from a single thread (should only be called via `ThreadMapPort`). Is there any reason not to at least declare/name these as the globals they are (`bool g_warned_natpmp_failure`) and add a comment about safety? Perhaps consider making them atomic. 

Code after:
                LogWarning("natpmp: Response too small\n");
                return false; // Wasn't response to what we expected, try receiving next packet.
            }
            if (response[0] != NATPMP_VERSION || response[1] != (NATPMP_RESPONSE | NATPMP_OP_MAP_TCP)) {
                LogWarning("natpmp: Response to wrong command\n");
                return false; // Wasn't response to what we expected, try receiving next packet.
            }
            uint16_t internal_port = ReadBE16(response.data() + NATPMP_MAP_RESPONSE_INTERNAL_PORT_OFS);
            if (internal_port != port) {
                LogWarning("natpmp: Response port doesn't match request\n");
                return false; // Wasn't response to what we expected, try receiving next packet.
            }
            return true;
        },
        interrupt);

    if (recv_res) {
        const std::span<uint8_t> response = *recv_res;

        Assume(response.size() >= NATPMP_MAP_RESPONSE_SIZE);
        uint16_t result_code = ReadBE16(response.data() + NATPMP_RESPONSE_HDR_RESULT_OFS);
        if (result_code != NATPMP_RESULT_SUCCESS) {
            if (result_code == NATPMP_RESULT_NOT_AUTHORIZED) {
                static std::atomic<bool> warned{false};
                if (!warned.exchange(true)) {
                    LogWarning("natpmp: Port mapping failed with result %s\n", NATPMPResultString(result_code));
                } else {
                    LogDebug(BCLog::NET, "natpmp: Port mapping failed with result %s\n", NATPMPResultString(result_code));
                }
            } else {
                LogWarning("natpmp: Port mapping failed with result %s\n", NATPMPResultString(result_code));
            }
            if (result_code == NATPMP_RESULT_NO_RESOURCES) {
                return MappingError::NO_RESOURCES;
            }
            return MappingError::PROTOCOL_ERROR;
        }

        uint32_t lifetime_ret = ReadBE32(response.data() + NATPMP_MAP_RESPONSE_LIFETIME_OFS);
        uint16_t external_port = ReadBE16(response.data() + NATPMP_MAP_RESPONSE_EXTERNAL_PORT_OFS);
        return MappingResult(NATPMP_VERSION, CService(internal.sin_addr, port), CService(external_addr, external_port), lifetime_ret);
    } else {
        return MappingError::NETWORK_ERROR;
    }
}

std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonce &nonce, const CNetAddr &gateway, const CNetAddr &bind, uint16_t port, uint32_t lifetime, CThreadInterrupt& interrupt, int num_tries, std::chrono::milliseconds timeout_per_try)
{
    struct sockaddr_storage dest_addr, bind_addr;
    socklen_t dest_addrlen = sizeof(struct sockaddr_storage), bind_addrlen = sizeof(struct sockaddr_storage);