test: Add test for wtxid transaction relay

fjahr commented at 1:18 am on March 27, 2020: member

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.

fanquake added the label Tests on Mar 27, 2020

fanquake requested review from ajtowns on Mar 27, 2020

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

DrahtBot commented at 6:40 am on March 27, 2020: member

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

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.

ajtowns commented at 12:53 pm on March 30, 2020: member

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.

DrahtBot added the label Needs rebase on Mar 30, 2020

fjahr force-pushed on Apr 1, 2020

fjahr commented at 1:00 am on April 1, 2020: member

@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

DrahtBot removed the label Needs rebase on Apr 1, 2020

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

ajtowns changes_requested

ajtowns commented at 9:24 am on April 6, 2020: member

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.

fjahr force-pushed on Apr 20, 2020

DrahtBot added the label Needs rebase on Apr 20, 2020

fjahr force-pushed on Apr 20, 2020

fjahr commented at 5:03 pm on April 20, 2020: member

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 .

DrahtBot removed the label Needs rebase on Apr 20, 2020

fjahr commented at 4:27 pm on April 21, 2020: member

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)

DrahtBot added the label Needs rebase on Apr 29, 2020

Add a wtxid-index to the mempool f7bf5c7a94

Add wtxid to mempool unbroadcast tracking fec2cba022

Just pass a hash to AddInventoryKnown

Since it's only used for transactions, there's no need to pass in an inv type.

81533be705

Add a wtxid-index to mapRelay 85ac1f56f7

Add wtxid-index to orphan map 911f570485

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

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

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

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

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

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

Rename AddInventoryKnown() to AddKnownTx() da72e51996

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.

test: Update test framework p2p protocol version to 70016

This new p2p protocol version allows to use WTXIDs for tx relay.

3db6c72282

fjahr force-pushed on Jun 26, 2020

fjahr commented at 9:00 pm on June 26, 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. @sdaftuar Thanks for the heads-up! I rebased with the latest state of #18044 and made several fixes plus squashed to three commits now.

DrahtBot removed the label Needs rebase on Jun 26, 2020

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

test: Use wtxid relay generally in functional tests 5d6f913f63

fjahr force-pushed on Jun 26, 2020

fjahr commented at 11:19 am on June 28, 2020: member

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

fjahr closed this on Jun 28, 2020

DrahtBot locked this on Feb 15, 2022

test: Add test for wtxid transaction relay #18446

Conflicts