test: Add test for wtxid transaction relay #18446

pull fjahr wants to merge 15 commits into bitcoin:master from fjahr:pr18044test changing 18 files +397 −110
  1. fjahr commented at 1:18 AM on March 27, 2020: contributor

    Based on #18044

    This updates the functional testing framework to the p2p protocol version including wtxid transaction relay (70016) and adds a basic test for wtxid based relay.

  2. fanquake added the label Tests on Mar 27, 2020
  3. fanquake requested review from ajtowns on Mar 27, 2020
  4. in test/functional/test_framework/messages.py:34 in ce8488e33f outdated 
      30 | @@ -31,7 +31,7 @@
  31 |  from test_framework.util import hex_str_to_bytes, assert_equal
  32 |  
  33 |  MIN_VERSION_SUPPORTED = 60001
  34 | -MY_VERSION = 70014  # past bip-31 for ping/pong
  35 | +MY_VERSION = 70016  # past bip-31 for ping/pong
    sipa commented at 3:29 AM on March 27, 2020:

    Comment is outdated now.

    fjahr commented at 1:02 AM on April 1, 2020:

    fixed

  5. DrahtBot commented at 6:40 AM on March 27, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19374 (refactor: Drop g_orphan_list global by hebasto)
    • #19364 (net processing: Move orphan reprocessing to a global by jnewbery)
    • #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
    • #19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
    • #19184 (Overhaul transaction request logic by sipa)
    • #19134 (test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until by dboures)
    • #19109 (Only allow getdata of recently announced invs by sipa)
    • #19031 (Implement ADDRv2 support (part of BIP155) by vasild)
    • #17785 (p2p: Unify Send and Receive protocol versions by hebasto)
    • #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

    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.

  6. DrahtBot cross-referenced this on Mar 27, 2020 from issue test: Add basic test for BIP 37 by MarcoFalke
  7. DrahtBot cross-referenced this on Mar 27, 2020 from issue Erlay: bandwidth-efficient transaction relay protocol by naumenkogs
  8. DrahtBot cross-referenced this on Mar 27, 2020 from issue Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli
  9. DrahtBot cross-referenced this on Mar 27, 2020 from issue net_processing: Retry notfounds with more urgency by ajtowns
  10. DrahtBot cross-referenced this on Mar 27, 2020 from issue P2P: Mempool tracks locally submitted transactions to improve wallet privacy by amitiuttarwar
  11. DrahtBot cross-referenced this on Mar 27, 2020 from issue p2p: Unify Send and Receive protocol versions by hebasto
  12. DrahtBot cross-referenced this on Mar 27, 2020 from issue p2p: Stop relaying non-mempool txs, improve tx privacy slightly by MarcoFalke
  13. DrahtBot cross-referenced this on Mar 27, 2020 from issue Serve BIP 157 compact filters by jimpo
  14. ajtowns commented at 12:53 PM on March 30, 2020: contributor

    I think this isn't testing:

    • whether the node switches from sending TX invs to WTX invs when the wtxidrelay message is sent
    • whether we get a wtxidrelay message when we give a 70016 version, but not when we give a lower version
    • in either case, when we announce a TX inv, do we get a TX getdata back, and likewise for WTX?
    • when a tx is sent (not an INV but the actual tx) if it's an orphan, the missing parent's txid is requested as a TX not a WTX

    I've had a go at some of the changes: https://github.com/ajtowns/bitcoin/tree/202003-wtxidrelay-test but haven't made it so it actually sends the wtxidrelay message.

  15. sdaftuar cross-referenced this on Mar 30, 2020 from issue Use wtxid for transaction relay by sdaftuar
  16. DrahtBot added the label Needs rebase on Mar 30, 2020
  17. fjahr force-pushed on Apr 1, 2020
  18. fjahr commented at 1:00 AM on April 1, 2020: contributor

    @ajtowns Thank you, that is very helpful. My intuition was that many of these cases would be tested indirectly by upgrading the test framework but of course, explicit tests are always better and, as I have realized now, I also had not implemented it correctly. Your tests mainly did not pass because I had not implemented feature negotiation correctly. This is done now and I preserved your commit mostly so it is easier for you to review. If the test now does what you intended it to do then I can squash.

    Overview of changes:

    • Rebased
    • Fixed outdated comment
    • Added explicit test for wtxidrelay
    • Fixed wtxidrelay feature negotiation
    • Added @ajtowns code and with some further changes on top
  19. DrahtBot removed the label Needs rebase on Apr 1, 2020
  20. in test/functional/p2p_segwit.py:165 in 3d43eea5c0 outdated 
     161 |  
 162 | +    def on_version(self, message):
 163 | +        if self.wtxidrelay:
 164 | +            self.send_message(msg_wtxidrelay())
 165 | +        self.send_message(msg_verack())
 166 | +        self.nServices = message.nServices
    ajtowns commented at 1:19 PM on April 2, 2020:

    Perhaps if self.wtxidrelay: ... ; super().on_version(self, message) ?

    fjahr commented at 4:51 PM on April 20, 2020:

    done

  21. ajtowns changes_requested
  22. ajtowns commented at 9:24 AM on April 6, 2020: contributor

    I think this needs more changes to match the latest update to #18044; I merged them and added a fixup commit on https://github.com/ajtowns/bitcoin/tree/202003-wtxidrelay-test if you want to take a look. Feel free to squash the fixups anytime btw.

  23. fjahr force-pushed on Apr 20, 2020
  24. DrahtBot added the label Needs rebase on Apr 20, 2020
  25. fjahr force-pushed on Apr 20, 2020
  26. fjahr commented at 5:03 PM on April 20, 2020: contributor

    Thanks @ajtowns ! I pulled in latest changes in #18044 and your suggestions. Then squashed and edited commit messages for easier reviewing. Also left your suggested change ignore non-wtxidrelay compliant invs in for now, pending discussion in #18044 .

  27. DrahtBot removed the label Needs rebase on Apr 20, 2020
  28. fjahr commented at 4:27 PM on April 21, 2020: contributor

    Pushed an update that should make all functional tests use wtxid relay by default (Github doesn't seem to be working properly at the moment, here is the commit: https://github.com/fjahr/bitcoin/commit/becdb68ab37fdcbc1dcf743b19ea8629f52677bb)

  29. DrahtBot cross-referenced this on Apr 25, 2020 from issue refactor: test: replace inv type magic numbers by constants by theStack
  30. DrahtBot cross-referenced this on Apr 29, 2020 from issue [net processing] Drop unknown types in getdata by jnewbery
  31. DrahtBot cross-referenced this on Apr 29, 2020 from issue [doc / test / mempool] unbroadcast follow-ups by amitiuttarwar
  32. DrahtBot added the label Needs rebase on Apr 29, 2020
  33. Add a wtxid-index to the mempool f7bf5c7a94
  34. Add wtxid to mempool unbroadcast tracking fec2cba022
  35. Just pass a hash to AddInventoryKnown
    Since it's only used for transactions, there's no need to pass in an inv type.
    81533be705
  36. Add a wtxid-index to mapRelay 85ac1f56f7
  37. Add wtxid-index to orphan map 911f570485
  38. Add wtxids of confirmed transactions to bloom filter
    This is in preparation for wtxid-based invs (we need to be able to tell whether
we AlreadyHave() a transaction based on either txid or wtxid).

This also double the size of the bloom filter, which is overkill, but still
uses a manageable amount of memory.
    6d55834ab7
  39. Add wtxids to recentRejects instead of txids
    Previously, we only added txids to recentRejects if we were sure that the
transaction couldn't have had the wrong witness (either because the witness was
malleated or stripped).

In preparation for wtxid-based relay, we can observe that txid == wtxid for
transactions that have no witness, and add the wtxid of rejected transactions,
provided the transaction wasn't a witness-stripped one. This means that we now
add more data to the filter (as prior to this commit, any transaction with a
witness that failed to be accepted was being skipped for inclusion in the
filter) but witness malleation should still not interfere with relay of a valid
segwit transaction, because the txid of a segwit transaction would not be added
to the filter after failing validation.

In the future, having wtxids in the recent rejects filter will allow us to
skip downloading the same wtxid multiple times, once our peers use wtxids for
transaction relay.
    444e947145
  40. Add support for tx-relay via wtxid
    This adds a field to CNodeState that tracks whether to relay transactions with
that peer via wtxid, instead of txid. As of this commit the field will always
be false, but in a later commit we will add a way to negotiate turning this on
via p2p messages exchanged with the peer.
    af8e570be4
  41. Add p2p message "wtxidrelay"
    When sent to and received from a given peer, enables using wtxid's for
announcing and fetching transactions with that peer.
    e360aac988
  42. Delay getdata requests from peers using txid-based relay
    Using both txid and wtxid-based relay with peers means that we could sometimes
download the same transaction twice, if announced via two different hashes from
different peers.

Use a heuristic of delaying txid-peer-getdata requests by 2 seconds, if we have
at least one wtxid-based peer.
    bc565ee3bd
  43. Make TX_WITNESS_STRIPPED its own rejection reason
    Previously, TX_WITNESS_MUTATED could be returned during transaction validation
for either transactions that had a witness that was non-standard, or for
transactions that had no witness but were invalid due to segwit validation
rules.

However, for txid/wtxid-relay considerations, net_processing distinguishes the
witness stripped case separately, because it affects whether a wtxid should be
able to be added to the reject filter. It is safe to add the wtxid of a
witness-mutated transaction to the filter (as that wtxid shouldn't collide with
the txid, and hence it wouldn't interfere with transaction relay from
txid-relay peers), but it is not safe to add the wtxid (== txid) of a
witness-stripped transaction to the filter, because that would interfere with
relay of another transaction with the same txid (but different wtxid) when
relaying from txid-relay peers.

Also updates the comment explaining this logic, and explaining that we can get
rid of this complexity once there's a sufficient deployment of wtxid-relaying
peers on the network.
    b5e6e5bbc8
  44. Rename AddInventoryKnown() to AddKnownTx() da72e51996
  45. sdaftuar commented at 4:54 PM on June 22, 2020: member

    FYI -- I tried to cherry-pick the test commits here onto the latest version of #18044 but the p2p_segwit.py test is failing for me.

  46. test: Update test framework p2p protocol version to 70016
    This new p2p protocol version allows to use WTXIDs for tx relay.
    3db6c72282
  47. fjahr force-pushed on Jun 26, 2020
  48. fjahr commented at 9:00 PM on June 26, 2020: contributor

    FYI -- I tried to cherry-pick the test commits here onto the latest version of #18044 but the p2p_segwit.py test is failing for me. @sdaftuar Thanks for the heads-up! I rebased with the latest state of #18044 and made several fixes plus squashed to three commits now.

  49. DrahtBot removed the label Needs rebase on Jun 26, 2020
  50. test: Add tests for wtxid tx relay in segwit test
    Also cleans up some doublicate lines in the rest of the test.

co-authored-by: Anthony Towns <aj@erisian.com.au>
    b10e09036a
  51. test: Use wtxid relay generally in functional tests 5d6f913f63
  52. fjahr force-pushed on Jun 26, 2020
  53. DrahtBot cross-referenced this on Jun 26, 2020 from issue refactor: Drop g_orphan_list global by hebasto
  54. DrahtBot cross-referenced this on Jun 26, 2020 from issue net processing: Move orphan reprocessing to a global by jnewbery
  55. DrahtBot cross-referenced this on Jun 27, 2020 from issue [tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar
  56. DrahtBot cross-referenced this on Jun 27, 2020 from issue refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto
  57. DrahtBot cross-referenced this on Jun 27, 2020 from issue Overhaul transaction request logic by sipa
  58. DrahtBot cross-referenced this on Jun 27, 2020 from issue test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until by dboures
  59. DrahtBot cross-referenced this on Jun 27, 2020 from issue Only allow getdata of recently announced invs by sipa
  60. DrahtBot cross-referenced this on Jun 27, 2020 from issue Implement ADDRv2 support (part of BIP155) by vasild
  61. DrahtBot cross-referenced this on Jun 27, 2020 from issue RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr
  62. fjahr commented at 11:19 AM on June 28, 2020: contributor

    These commits were pulled into #18044 so it can be closed.

  63. fjahr closed this on Jun 28, 2020
  64. bitcoin locked this on Feb 15, 2022
Contributors
fjahrsipaDrahtBotajtownssdaftuar
Labels
Tests
Linked (view graph)
#18044 Use wtxid for transaction relay
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-05-20 06:54 UTC