test: Add test for wtxid transaction relay #18446
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
I think this isn't testing:
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 |
|
@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:
|
There was a problem hiding this comment.
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.
|
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: fjahr@becdb68) |
Since it's only used for transactions, there's no need to pass in an inv type.
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.
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.
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.
When sent to and received from a given peer, enables using wtxid's for announcing and fetching transactions with that peer.
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.
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.
|
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. |
This new p2p protocol version allows to use WTXIDs for tx relay.
Also cleans up some doublicate lines in the rest of the test. co-authored-by: Anthony Towns <aj@erisian.com.au>
|
These commits were pulled into #18044 so it can be closed. |
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.