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

Code before:
    if (m_chainman.ActiveChain().Tip()->GetBlockHash() != hashRecentRejectsChainTip) {
        // If the chain tip has changed previously rejected transactions
        // might be now valid, e.g. due to a nLockTime'd tx becoming valid,
        // or a double-spend. Reset the rejects filter and give those
        // txs a second chance.
        hashRecentRejectsChainTip = m_chainman.ActiveChain().Tip()->GetBlockHash();
        m_recent_rejects.reset();
        m_recent_rejects_reconsiderable.reset();
    }

    const uint256& hash = gtxid.GetHash();

    if (gtxid.IsWtxid()) {
        // Normal query by wtxid.
        if (m_orphanage.HaveTx(gtxid)) return true;
    } else {
        // Never query by txid: it is possible that the transaction in the orphanage has the same
        // txid but a different witness, which would give us a false positive result. If we decided
        // not to request the transaction based on this result, an attacker could prevent us from
        // downloading a transaction by intentionally creating a malleated version of it.
        //
        // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
        // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
        // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
        if (m_orphanage.HaveTx(GenTxid::Wtxid(hash))) return true;
    }

    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true;

    {
        LOCK(m_recent_confirmed_transactions_mutex);
        if (m_recent_confirmed_transactions.contains(hash)) return true;
    }

    return m_recent_rejects.contains(hash) || m_mempool.exists(gtxid);
}

bool PeerManagerImpl::AlreadyHaveBlock(const uint256& block_hash)
{
    return m_chainman.m_blockman.LookupBlockIndex(block_hash) != nullptr;
}

void PeerManagerImpl::SendPings()
{
    LOCK(m_peer_mutex);
    for(auto& it : m_peer_map) it.second->m_ping_queued = true;
}

void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid)
{
    LOCK(m_peer_mutex);

Review comment (refactor):
In 7e475b9648bbee04f5825b922ba0399373eaa5a9:

I think having the `if/else` path in this commit is helpful, since it distinguishes between when we cast the txid and when we don't.

nit: I think this already works pretty well, but some bits (e.g. "guess what the wtxid is") are not super clear imo. I've summarized my understanding into the below, feel free to use what you like.

```
        // Never query by txid: it is possible that an existing transaction in the orphanage has the
        // same txid but a different witness, which would give us a false positive result. If we
        // don't request the transaction based on this result, an attacker could prevent us from
        // downloading a transaction by intentionally creating a malleated version of it.
        //
        // False positive are unsafe and must be avoided.False negatives are acceptable and just
        // lead to increased bandwidth usage.
        //
        // When gtxid is a txid it means when we don't have enough information to query by wtxid,
        // e.g. because the transaction was announced by a non-wtxid-relay peer, or because we're
        // checking the prevouts of a transaction. It doesn't mean that the transaction has no
        // witness data, so we need to distinguish between those cases.
        //
        // If the transaction:
        // 1) does not have witness data, we can safely query the orphanage by casting a txid to a
        //    wtxid because they are equivalent. False positives and false negatives are impossible.
        // 2) has witness data, the txid and wtxid are different by definition, making this query a
        //    no-op. False positives are not possible, but we may have false negatives.
        //
        // So, by querying for a txid cast to a wtxid, we avoid false positives entirely, but we
        // save bandwidth for the case where txid == wtxid.
```

side-note rant: I think the fact that we need such a verbose comment is a pretty strong sign `AlreadyHaveTx` should be refactored, e.g. using `{Wt,T}xid` types as I suggested [here](https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587694422), but I appreciate that it's mostly orthogonal to this PR. Renaming `AlreadyHaveTx` to e.g. `ShouldRequestTx` or something would probably already clarify quite a bit, too (although also not blocking for this PR either).

Code after:
    if (m_chainman.ActiveChain().Tip()->GetBlockHash() != hashRecentRejectsChainTip) {
        // If the chain tip has changed previously rejected transactions
        // might be now valid, e.g. due to a nLockTime'd tx becoming valid,
        // or a double-spend. Reset the rejects filter and give those
        // txs a second chance.
        hashRecentRejectsChainTip = m_chainman.ActiveChain().Tip()->GetBlockHash();
        m_recent_rejects.reset();
        m_recent_rejects_reconsiderable.reset();
    }

    const uint256& hash = gtxid.GetHash();

    if (gtxid.IsWtxid()) {
        // Normal query by wtxid.
        if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;
    } else {
        // Never query by txid: it is possible that the transaction in the orphanage has the same
        // txid but a different witness, which would give us a false positive result. If we decided
        // not to request the transaction based on this result, an attacker could prevent us from
        // downloading a transaction by intentionally creating a malleated version of it.  While
        // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
        // discerning which one that is, so the orphanage can store multiple transactions with the
        // same txid.
        //
        // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
        // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
        // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
        if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;
    }

    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true;

    {
        LOCK(m_recent_confirmed_transactions_mutex);
        if (m_recent_confirmed_transactions.contains(hash)) return true;
    }

    return m_recent_rejects.contains(hash) || m_mempool.exists(gtxid);
}

bool PeerManagerImpl::AlreadyHaveBlock(const uint256& block_hash)
{
    return m_chainman.m_blockman.LookupBlockIndex(block_hash) != nullptr;
}

void PeerManagerImpl::SendPings()
{
    LOCK(m_peer_mutex);
    for(auto& it : m_peer_map) it.second->m_ping_queued = true;
}