net_processing: Ignore MSG_WITNESS_TX entries from INV messages #35920

pull ajtowns wants to merge 3 commits into bitcoin:master from ajtowns:202608-inv-msg-witness changing 3 files +28 −25
  1. ajtowns commented at 8:41 PM on August 6, 2026: contributor

    When introducing wtxidrelay we added checks to skip attempts to announce txs with the wrong hash (https://github.com/bitcoin/bitcoin/pull/18044#discussion_r403953004), but didn't similarly ignore MSG_WITNESS_TX entries in announcements, which have never been valid (see BIP-144).

    This PR corrects that oversight, skipping such announcements. It also moves the check to be after the disconnect code, so that attempting to announce txs on fRelay=false doesn't get treated as okay just because you're doing it wrong. As a result this allows deduping the seen_tx_hashes set.

  2. DrahtBot commented at 8:41 PM on August 6, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    External sites

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    ACK fjahr, fametrano
    Concept ACK l0rinc, mzumsande

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36257 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36257.svg"></sub> (qa: assert_equals -> assert_true/assert_false by hodlinator)
    • #35502 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35502.svg"></sub> (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. ajtowns commented at 8:56 PM on August 6, 2026: contributor

    See also #35917 and #35878 (review)

  4. l0rinc commented at 9:02 PM on August 6, 2026: contributor

    Concept ACK, thanks for pushing a fix for #35878 (review).

  5. ajtowns renamed this:
    net_processing: Ignore MSG_TX_WITNESS entries from INV messages
    net_processing: Ignore MSG_WITNESS_TX entries from INV messages
    on Aug 6, 2026
  6. DrahtBot closed this on Aug 7, 2026
  7. DrahtBot reopened this on Aug 7, 2026
  8. in src/net_processing.cpp:4383 in eaaea13cda 
    4381 | +                // This ignores any MSG_WITNESS_TX entries, as that inv
4382 | +                // type is only for use with GETDATA (per BIP 144).
4383 | +                if (peer.m_wtxid_relay) {
4384 | +                    if (!inv.IsMsgWtx()) continue;
4385 | +                } else {
4386 | +                    if (!inv.IsMsgTx()) continue;
    darosior commented at 9:01 PM on August 7, 2026:

    Ignoring MSG_WITNESS_TX for wtxid relay peers seems fine, because i expect wtxid relay codebases to be newer / better maintained, and because BIP 339 explicitly states:

    After a node has received a wtxidrelay message from a peer, the node MUST use the MSG_WTX inv type when announcing transactions to that peer.

    However for txid relay peers, i'm afraid we are in a similar situation to unannounced transaction processing. We have supported it for a decade, and while it Is Incorrect, i'm afraid there may be an unmaintained codebase out there that relies on it. It's a bit more niche than unannounced transaction processing, but still, i think we should have some evidence this isn't being used in the wild before dropping support.

    ajtowns commented at 1:26 AM on August 8, 2026:

    Based on the got inv debug.log lines, my lots-of-peers node received no witness-tx invs in April-July, versus:

    Month block tx wtx
    April 3,210 6,935,321 1,174,811,061
    May 2,092 3,530,619 854,676,397
    June 1,783 13,683,220 1,793,970,323
    July 1,924 10,870,284 2,474,162,561

    Numbers from:

    zstdcat debug.log-202604*.zst | grep got.inv: | cut -d\  -f5 |
  awk '{c[$0]++} END {for (l in c) printf "%8d %s\n", c[l], l}'
    ajtowns commented at 3:19 AM on August 12, 2026:

    For people who do find instances of this, grep got.inv:.witness-tx .bitcoin/debug.log should indicate the txid being sent, as well as whether your peer already had the tx in question ("have" vs "new").

  9. fjahr commented at 1:50 PM on August 11, 2026: contributor

    Concept ACK

  10. 0xB10C commented at 3:02 PM on August 12, 2026: contributor

    As per https://bnoc.xyz/t/any-reachable-node-receiving-tx-announcement-via-msg-witness-tx/159/5: /btcwire:0.5.0/neutrino:0.12.0-beta/ seems to be announcing transactions with InvTypeWitnessTx:

    https://github.com/lightninglabs/neutrino/blob/c96685a023c6946be7d463541b49e0f37f527982/query.go#L951-L969

  12. net_processing: Ignore MSG_WITNESS_TX entries in inv messages
    BIP 144 specifies that MSG_WITNESS_TX is "only for use in getdata",
so if we receive it anyway, ignore it, rather than treating it as a
by-txid announcement.

Also changes the code ordering so that attempting to announce txs on a
connection where tx relay is disabled triggers a disconnect, even if we
would have ignored the announcement for being the wrong inv type.
    2eefc296d4
  13. tests: Check we disconnect on tx INVs on blocks-only conns
    Previously we only checked disconnection occurs on "correct" announcements
(WTX on a wtxidrelay connection); extend that to checking all possible
announcements (still only on a wtxidrelay connection).
    a24f315175
  14. net_processing: Combine seen_txids and seen_wtxids
    After the previous commit only one of these will ever be used
for any particular INV message, so there's no need for two.

Co-Authored-By: Antoine Poinsot <mail@antoinep.com>
    0f9bffe6e1
  15. ajtowns force-pushed on Aug 12, 2026
  16. ajtowns commented at 10:09 PM on August 12, 2026: contributor

    Changed to allow witness-tx in invs for non-wtxidrelay peers. Lame.

    Concept ACK, thanks for pushing a fix for #35878 (comment). If you touch again, please add me as coauthor.

    I didn't copy any code from you, and I'm not even sure what this is in regards to. Nothing here is substantive enough to be worth arguing about authorship.

  17. sedited requested review from darosior on Sep 25, 2026
  18. sedited requested review from fjahr on Sep 25, 2026
  19. in src/net_processing.cpp:4384 in 2eefc296d4 outdated 
    4371 | @@ -4381,7 +4372,17 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4372 |                      pfrom.fDisconnect = true;
4373 |                      return;
4374 |                  }
4375 | -                // MSG_WITNESS_TX is treated as a txid, despite only being specified for getdata.
    fjahr commented at 2:53 PM on September 26, 2026:

    The deleted comment isn't wrong, the code that it referred to still exists below. The difference is that it now only runs for non-wtxidrelay peers. I would keep it around below the new code block or maybe include the information somewhere the new comments.

    ajtowns commented at 12:28 AM on October 1, 2026:

    Everything except MSG_WTX is always a txid, see ToGenTxid() or how MSG_WITNESS_TX is defined in bip 144 ("MSG_WITNESS_TX getdata requests should use the non-witness serialized hash."); I don't think this needs an extra comment

  20. in test/functional/p2p_tx_download.py:381 in 2eefc296d4 outdated 
     377 | @@ -378,7 +378,7 @@ def send_invs_and_read_log(peer, invs):
 378 |              assert_equal(log.count(f"got inv: {first_name} {hash_a:064x}"), 1)
 379 |              assert_equal(log.count(f"got inv: {second_name} {hash_a:064x}"), 0)
 380 |  
 381 | -        self.log.info('Check that txids and wtxids are deduplicated separately')
 382 | +        self.log.info('Check that MSG_WITNESS_TX inv is ignored with wtxid relay')
    fjahr commented at 3:19 PM on September 26, 2026:

    (note to other reviewers: the actual change to the separate deduplication happens in the third commit)

    Based on the redefiniton of the test now only the first of the loop runs seems still relevant. I think I would find it confusing why the test is written this way without the historic background.

             self.log.info('Check that MSG_WITNESS_TX inv is ignored with wtxid relay')
         peer = node.add_p2p_connection(TestP2PConn(wtxidrelay=True))
-        for first_type, second_type, hash_a in [
-            (MSG_WITNESS_TX, MSG_WTX, 0x8899aa),
-            (MSG_WTX, MSG_WITNESS_TX, 0x99aabb),
-        ]:
-            log = send_invs_and_read_log(peer, [
-                CInv(t=first_type, h=hash_a),
-                CInv(t=second_type, h=hash_a),
-            ])
-            assert_equal(log.count(f"got inv: witness-tx {hash_a:064x}"), 0)
-            assert_equal(log.count(f"got inv: wtx {hash_a:064x}"), 1)
+        hash = 0x8899aa
+        log = send_invs_and_read_log(peer, [
+            CInv(t=MSG_WITNESS_TX, h=hash),
+            CInv(t=MSG_WTX, h=hash),
+        ])
+        assert_equal(log.count(f"got inv: witness-tx {hash:064x}"), 0)
+        assert_equal(log.count(f"got inv: wtx {hash:064x}"), 1)
    ajtowns commented at 12:34 AM on October 1, 2026:

    The historic background is readily available via git log: git log -M -C -L 381,392:../test/functional/p2p_tx_download.py I prefer to keep the changes to tests minimal when changing functionality, to avoid accidentally reducing coverage in a way that hides introduced bugs, so I'd rather keep this as-is.

    fjahr commented at 2:37 PM on October 2, 2026:

    I don't think it would occur to me to go back in git history because there is no hint here that there is some historic context that might be helpful. And if I did, looking for the historic context of the test parts that are confusing would let me find the reason because these are the lines that are not being changed here. But not a blocker either.

    ajtowns commented at 2:54 PM on October 3, 2026:

    I just mean if you find a piece of code confusing, looking at the git history should always be one of the first things you consider doing. In this case you'd see the test went from 1 to 0 for witness-tx and the description went from "Check that txids and wtxids are deduplicated separately" to "Check that MSG_WITNESS_TX inv is ignored with wtxid relay" as part of the "net_processing: Ignore MSG_WITNESS_TX entries in inv messages" commit, which you could then trace through to the PR in question if it was still confusing.

  21. in test/functional/p2p_blocksonly.py:119 in a24f315175 outdated 
     119 | -            peer.send_without_ping(msg_inv([CInv(t=MSG_WTX, h=0x12345)]))
 120 | -            peer.wait_for_disconnect()
 121 | -        self.nodes[0].disconnect_p2ps()
 122 | +        h = 0x12345
 123 | +        for msg_type in [MSG_WTX, MSG_TX, MSG_WITNESS_TX]:
 124 | +            h += 1
    fjahr commented at 3:23 PM on September 26, 2026:

    nit: Since each loop run has its own connection this doesn't seem necessary

    ajtowns commented at 12:04 AM on October 1, 2026:

    It's not necessary but if one (or more) of the invs somehow gets a GETDATA response due to a bug, it makes it clearer which one was the trigger.

  22. in test/functional/p2p_blocksonly.py:118 in a24f315175 outdated 
     118 | -        with self.nodes[0].assert_debug_log(["inv sent in violation of protocol, disconnecting peer"]):
 119 | -            peer.send_without_ping(msg_inv([CInv(t=MSG_WTX, h=0x12345)]))
 120 | -            peer.wait_for_disconnect()
 121 | -        self.nodes[0].disconnect_p2ps()
 122 | +        h = 0x12345
 123 | +        for msg_type in [MSG_WTX, MSG_TX, MSG_WITNESS_TX]:
    fjahr commented at 3:36 PM on September 26, 2026:

    nit: Could apply the same loop to the self.log.info('Check that tx invs also violate the protocol') test above in blocksonly_mode_tests.

    instagibbs commented at 8:31 PM on October 5, 2026: 
            for wtxidrelay, msg_type in product([True, False], [MSG_WTX, MSG_TX,
MSG_WITNESS_TX]):
            h += 1
            peer =
self.nodes[0].add_outbound_p2p_connection(P2PInterface(wtxidrelay=wtxidrelay),
p2p_idx=0, connection_type="block-relay-only")
  23. in src/net_processing.cpp:4385 in 0f9bffe6e1 
    4381 | @@ -4383,8 +4382,8 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4382 |                      // BIP 144) for compatibility with neutrino
4383 |                      if (inv.IsMsgWtx()) continue;
4384 |                  }
4385 | -                auto& seen_hashes{inv.IsMsgWtx() ? seen_wtxids : seen_txids};
4386 | -                if (!seen_hashes.insert(inv.hash).second) continue;
4387 | +                // Due to the check above, we're only adding hashes of one particular type here
    fjahr commented at 3:42 PM on September 26, 2026:

    nit: This would be a bit easier to understand for me

                    // Mixing txids and wtxids here is ok because the check above only lets through
                // wtxids (MSG_WTX) from wtxidrelay peers and txids (MSG_TX, MSG_WITNESS_TX) otherwise.
    ajtowns commented at 12:36 AM on October 1, 2026:

    We're not mixing txids and wtxids though. Each peer gets precisely one or the other added to seen_tx_hashes.

    fjahr commented at 2:37 PM on October 2, 2026:

    To me this is still mixing different types in a single data structure, independent of which peer these are coming from. But it's not a blocker for me.

  24. fjahr commented at 3:48 PM on September 26, 2026: contributor

    Code review ACK 0f9bffe6e1bcfdec55a03defc36e7ab282d7ef9d

    Left a bunch of suggestions to improve comments and tests but this is good to merge as is.

  25. DrahtBot requested review from l0rinc on Sep 26, 2026
  26. fametrano commented at 11:37 AM on October 2, 2026: contributor

    tACK 0f9bffe6e1bcfdec55a03defc36e7ab282d7ef9d

    Built and ran p2p_blocksonly.py and p2p_tx_download.py. With net_processing.cpp reverted to the base, both fail. Each of five small breakages I tried, such as moving the wtxidrelay filter back above the disconnect or dropping the dedup, fails at least one of them.

    Nit: the title and description still say MSG_WITNESS_TX invs are ignored; since the neutrino change, that only holds for wtxidrelay peers.

  27. Amrrty commented at 11:49 AM on October 2, 2026: none

    How can it be fixed?

  28. bitcoin blocked a user on Oct 2, 2026
  29. instagibbs commented at 1:11 PM on October 2, 2026: member

    Filed as https://github.com/lightninglabs/neutrino/issues/384

    just noting it was merged a month ago

  30. sedited requested review from instagibbs on Oct 5, 2026
  31. sedited requested review from mzumsande on Oct 6, 2026
  32. instagibbs commented at 10:44 AM on October 6, 2026: member

    0f9bffe6e1bcfdec55a03defc36e7ab282d7ef9d

    LGTM

    I'd happily review a PR that would make MSG_WITNESS_TX and MGS_WTX not sound like the same thing.

    Could we get a test that ensures we GETDATA the neutrino exception, if we don't have one?

  33. mzumsande commented at 5:43 PM on October 6, 2026: contributor

    Concept ACK

    Filed as lightninglabs/neutrino#384

    just noting it was merged a month ago

    What is the plan to deal with this? Merge as is and revisit in a few releases to then ignore MSG_WITNESS_TX also for for non-wtxidrelay peers?

Contributors
ajtownsDrahtBotl0rincdarosiorfjahr0xB10CinstagibbsfametranoAmrrtymzumsande


l0rincdarosior

Linked (view graph)
#18044 Use wtxid for transaction relay#36405 test: look for the wtxid in p2p_blocksonly
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-10-14 23:51 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me