net_processing: process unique tx INVs only #35878

instagibbs commented at 12:17 PM on August 4, 2026: member

There is no reason we should process conflicting advertisements for transactions, as they cannot be both accepted into our mempool.

Avoid processing these and doing spurious work.

Should be no change in observable behavior.

DrahtBot commented at 12:17 PM on August 4, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35878.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	ajtowns, l0rinc, fjahr
Concept ACK	yuvicc
Stale ACK	darosior, mzumsande

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35351 (net: Disallow invalid HeadersSyncState due to lagging clock by hodlinator)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fjahr commented at 3:13 PM on August 4, 2026: contributor

Concept ACK

darosior commented at 5:04 PM on August 4, 2026: member

Makes sense. Concept ACK.

yuvicc commented at 5:23 AM on August 5, 2026: contributor

Concept ACK

Deduplicating identical tx announcements makes sense.

darosior approved

darosior commented at 4:34 PM on August 5, 2026: member

ACK b11d8fc5f761274c1e4aa28b1cf8c40321074c21

DrahtBot requested review from fjahr on Aug 5, 2026

mzumsande commented at 6:48 PM on August 5, 2026: contributor

Code Review ACK b11d8fc5f761274c1e4aa28b1cf8c40321074c21

in src/net_processing.cpp:4342 in b11d8fc5f7

4338 | @@ -4337,6 +4339,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4339 |          }
4340 |  
4341 |          const bool reject_tx_invs{RejectIncomingTxs(pfrom)};
4342 | +        std::unordered_set<uint256, SaltedUint256Hasher> seen_tx_hashes;

l0rinc commented at 9:14 PM on August 5, 2026:

SaltedUint256Hasher is stateful now and it's recreated for most inv messages this way - including ones without a transaction announcement.

Could we keep one salt in PeerManagerImpl and copy it into each short-lived set? (I considered the new SipHash-1-3-UJ here, but CInv::hash is peer-controlled and does not satisfy its jumbo-input assumptions, so I think we should keep SipHash-2-4)

<details><summary>reuse tx INV dedup salt</summary>

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index cbb248b7e5..e94dac73fb 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -844,6 +844,9 @@ private:

     FastRandomContext m_rng GUARDED_BY(NetEventsInterface::g_msgproc_mutex);

+    const SaltedUint256Hasher m_txhash_hasher;
     FeeFilterRounder m_fee_filter_rounder GUARDED_BY(NetEventsInterface::g_msgproc_mutex);

     const CChainParams& m_chainparams;
@@ -4339,7 +4342,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
         }

         const bool reject_tx_invs{RejectIncomingTxs(pfrom)};
-        std::unordered_set<uint256, SaltedUint256Hasher> seen_tx_hashes;
+        std::unordered_set<uint256, SaltedUint256Hasher> seen_tx_hashes(0, m_txhash_hasher);

         LOCK2(cs_main, m_tx_download_mutex);

</details>

instagibbs commented at 1:21 AM on August 6, 2026:

considered this before, but this is pushing me over the edge: I'll make one hasher and reuse it for both filter

in src/net_processing.cpp:4381 in b11d8fc5f7

4377 | @@ -4375,6 +4378,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4378 |                      pfrom.fDisconnect = true;
4379 |                      return;
4380 |                  }
4381 | +                if (!seen_tx_hashes.insert(inv.hash).second) continue;

l0rinc commented at 9:46 PM on August 5, 2026:

Shouldn't we deduplicate txids and wtxids separately to make sure one doesn't prohibit the other? BIP144 says MSG_WITNESS_TX is only for use in getdata, but IsGenTxMsg() includes it, so this inv path currently processes it.

Is it intentional for a preceding MSG_WITNESS_TX to suppress a following MSG_WTX with the same hash before AddTxAnnouncement() sees it? Concretely, this is what I mean:

<details><summary>MSG_WITNESS_TX carries a txid, while MSG_WTX carries a wtxid</summary>

diff --git a/test/functional/p2p_tx_download.py b/test/functional/p2p_tx_download.py
--- a/test/functional/p2p_tx_download.py	(revision c0355a8b183de8461f29814e992e677fb813417a)
+++ b/test/functional/p2p_tx_download.py	(revision aec4caf0d946e97149009d6280277c159fdc8cb5)
@@ -16,6 +16,7 @@
     CInv,
     MSG_TX,
     MSG_TYPE_MASK,
+    MSG_WITNESS_TX,
     MSG_WTX,
     msg_inv,
     msg_notfound,
@@ -364,6 +365,16 @@
             assert_equal(log.count(inv_a_log), 1)
             assert_equal(log.count(inv_b_log), 0)

+        # MSG_WITNESS_TX carries a txid, while MSG_WTX carries a wtxid.
+        peer = node.add_p2p_connection(TestP2PConn(wtxidrelay=True))
+        hash_a = 0x778899
+        log = send_invs_and_read_log(peer, [
+            CInv(t=MSG_WITNESS_TX, h=hash_a),
+            CInv(t=MSG_WTX, h=hash_a),
+        ])
+        assert_equal(log.count(f"got inv: witness-tx {hash_a:064x}"), 1)
+        assert_equal(log.count(f"got inv: wtx {hash_a:064x}"), 1)  # TODO fails currently
+
     def test_spurious_notfound(self):
         self.log.info('Check that spurious notfound is ignored')
         self.nodes[0].p2ps[0].send_without_ping(msg_notfound(vec=[CInv(MSG_TX, 1)]))

</details>

If so, we could probably fix it by having separate deduplication sets.

instagibbs commented at 1:22 AM on August 6, 2026:

good catch, even if it's a protocol violation I don't want to affect behavior. Added a second filter as you suggest

ajtowns commented at 3:17 AM on August 6, 2026:

I think a followup that does:

        } else if (inv.IsGenTxMsg()) {
            if (reject_tx_invs) { ...; return; }
            if (peer.m_wtxid_relay) {
                if (!inv.IsMsgWtx()) continue;
            } else {
                if (!inv.IsMsgTx()) continue;
            }
            ...

would make sense. (ie, move the ignore-wrong-invs into the GenTxMsg block, disconnect first, and ignore MSG_WITNESS_TX requests) I think not dropping MSG_WITNESS_TX's was just an oversight, ref: #18044 (review)

ajtowns commented at 4:19 AM on August 6, 2026:

Shouldn't we deduplicate txids and wtxids separately to make sure one doesn't prohibit the other?

I don't think this makes sense -- if you have txA with txid X and txB with wtxid X, then either txA=txB and they don't have any witness data and deduping is fine, or you've found a hash collision and bitcoin's broken anyway. Alternatively, if your peer is lying to you advertising tx inv's where it doesn't have the corresponding tx and hoping to trick you into not requesting something that you actually want, they could equally well just ignore your request if you had actually made it, just as they'll have to ignore your request for the tx that doesn't actually exist.

l0rinc commented at 5:21 AM on August 6, 2026:

The relevant context seems to be the orphanage, where "the orphanage can store multiple transactions with the same txid", see https://github.com/bitcoin/bitcoin/blob/1278a5970d5ada0979052a5bad899e896b8ab40b/src/node/txdownloadman_impl.cpp#L129-L135

It's a bit complicated for me, especially since BIP 144's restriction does not seem to be enforced here. If we ignored MSG_WITNESS_TX INVs, the separate sets would probably become unnecessary. Based on the reproducer and the AI explanations, though, it seems to me that either the separation or ignoring those INVs is currently needed for this change.

<details><summary>AI summary of the problem</summary>

The same witnessless transaction is enough for the two announcements to behave differently.

The txid form reaches AlreadyHaveTx(), which finds the witnessless orphan and returns. The wtxid form instead enters the orphan-resolution branch, which treats the peer as if it had announced every missing parent, even if those parent INVs were never sent.

With a shared set, the preceding MSG_WITNESS_TX suppresses that second side effect, so the node loses this peer as a source for the orphan’s missing parents.

The peer could ignore getdata, but that applies to every transaction announcement and does not make these processing paths equivalent.

If the follow-up ignores MSG_WITNESS_TX INVs, the separate sets would indeed become unnecessary.

</details>

instagibbs commented at 11:13 AM on August 6, 2026:

@ajtowns for my part, I wanted to keep the (lack of) behavior changes simple to follow. I had thought that there was a "domain separation" due to the existing filtering of wtid/txid invs from above, but this wasn't strictly true.

Trying to keep this as re-viewable as possible for now. Agreed on followup!

ajtowns commented at 12:23 PM on August 6, 2026:

@instagibbs Yeah, that made so much sense to me it triggered my "doesn't need to be said" filter, oops.

darosior commented at 1:41 PM on August 6, 2026:

I considered that and had the same reaction as AJ. If you get a hit for wtxid == txid, this is a legacy transaction you've already seen so you should drop it.

l0rinc changes_requested

l0rinc commented at 10:41 PM on August 5, 2026: contributor

I wasn't very familiar with this part of the codebase (let me know if my assessment is wrong), but it seems to me the txids and wtxids should be deduped separately.

net_processing: process unique tx INVs only

There is no reason we should process conflicting
advertisements for transactions, as they cannot be both
accepted into our mempool.

Avoid processing these and doing spurious work.

1278a5970d

instagibbs force-pushed on Aug 6, 2026

ajtowns commented at 4:30 AM on August 6, 2026: contributor

ACK 1278a5970d5ada0979052a5bad899e896b8ab40b

Don't think the separation between txids/wtxids is needed, but don't think it's harmful either. Not reseeding the hasher over time or between different peers also seems fine.

DrahtBot requested review from mzumsande on Aug 6, 2026

DrahtBot requested review from darosior on Aug 6, 2026

in src/net_processing.cpp:4387 in 1278a5970d

4380 | @@ -4375,6 +4381,9 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4381 |                      pfrom.fDisconnect = true;
4382 |                      return;
4383 |                  }
4384 | +                // MSG_WITNESS_TX is treated as a txid, despite only being specified for getdata.
4385 | +                auto& seen_hashes{inv.IsMsgWtx() ? seen_wtxids : seen_txids};
4386 | +                if (!seen_hashes.insert(inv.hash).second) continue;
4387 |                  const GenTxid gtxid = ToGenTxid(inv);

l0rinc commented at 5:26 AM on August 6, 2026:

nit: alternatively, to avoid duplicating inv.IsMsgWtx() in ToGenTxid, we could reuse gtxid:

const GenTxid gtxid{ToGenTxid(inv)};
auto& seen_hashes{gtxid.IsWtxid() ? seen_wtxids : seen_txids};
if (!seen_hashes.insert(gtxid.ToUint256()).second) continue;

l0rinc approved

l0rinc commented at 5:35 AM on August 6, 2026: contributor

ACK 1278a5970d5ada0979052a5bad899e896b8ab40b

fjahr commented at 11:08 AM on August 6, 2026: contributor

ACK 1278a5970d5ada0979052a5bad899e896b8ab40b

I agree with @ajtowns that the separation of txid/wtxid was unnecessary, but it's not a blocker for me.

fanquake merged this on Aug 6, 2026

fanquake closed this on Aug 6, 2026

darosior commented at 1:45 PM on August 6, 2026: member

I don't think it's harmful, but it bugs me that we now unnecessarily duplicate our deduplication sets. Would reviewers here be interested in reviewing a small follow-up that restores this to a single set?

fjahr commented at 1:52 PM on August 6, 2026: contributor

I don't think it's harmful, but it bugs me that we now unnecessarily duplicate our deduplication sets. Would reviewers here be interested in reviewing a small follow-up that restores this to a single set?

Sure

darosior commented at 2:30 PM on August 6, 2026: member

Chaser: https://github.com/bitcoin/bitcoin/pull/35917

ajtowns commented at 8:57 PM on August 6, 2026: contributor

Palate-cleanser? #35920