Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

p2p: index TxOrphanage by wtxid, allow entries with same txid #30000

Merged
merged 7 commits into from
May 15, 2024
Merged

p2p: index TxOrphanage by wtxid, allow entries with same txid #30000

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7e475b9
[p2p] don't query orphanage by txid
glozow May 10, 2024
efcc593
[refactor] TxOrphanage::HaveTx only by wtxid
glozow May 10, 2024
c31f148
[refactor] TxOrphanage::EraseTx by wtxid
glozow May 10, 2024
8923edf
[p2p] allow entries with the same txid in TxOrphanage
glozow Apr 29, 2024
6675f64
[unit test] TxOrphanage handling of same-txid-different-witness txns
glozow May 10, 2024
b16da7e
[functional test] attackers sending mutated orphans
glozow Apr 29, 2024
0fb17bf
[log] updates in TxOrphanage
glozow May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2295,7 +2295,20 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside

const uint256& hash = gtxid.GetHash();

if (m_orphanage.HaveTx(gtxid)) return true;
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 7e475b9:

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, 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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've summarized my understanding into the below

Looks correct to me

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 #30000 (comment), but I appreciate that it's mostly orthogonal to this PR.

I think it's really a sign to ban txid-based transaction relay. Just kidding. I agree it's hard to reason about logic dealing with a hash that can be txid, wtxid, or bot. I think it's just one of the common things that trips people in tx relay, just like everybody's mempool fuzzer gets snagged on CPFP carve out, and that for loop in init.cpp that claims reviewer victims every time it's touched.

Renaming AlreadyHaveTx to e.g. ShouldRequestTx or something would probably already clarify quite a bit, too (although also not blocking for this PR either).

Yeah, AlreadyHaveTx should probably have a name that reflects its purpose of "should I bother {requesting, downloading, validating} this tx".

Do note that #30110 makes AlreadyHaveTx a function internal to the TxDownloadImpl, i.e. no longer something that peerman knows about. Funnily, TxDownloadImpl::AlreadyHaveTx might be a fine name when it's in that context. I get that it's confusing, but I'd ask that we defer renaming until after that PR since it would conflict + maybe be less problematic afterwards.


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

Expand Down