Add functional test test_txid_inv

ariard commented at 4:47 PM on October 16, 2020: member

This is a simple functional test to increase coverage of #19988, checking that txid announcements from txid-relay peers are delayed by TXID_RELAY_DELAY, assuming we have at least another wtxid-relay peer.

You can verify new test with the following diff :

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f14db379f..2a2805df5 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -773,7 +773,7 @@ void PeerManager::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std
     auto delay = std::chrono::microseconds{0};
     const bool preferred = state->fPreferredDownload;
     if (!preferred) delay += NONPREF_PEER_TX_DELAY;
-    if (!gtxid.IsWtxid() && g_wtxid_relay_peers > 0) delay += TXID_RELAY_DELAY;
+    //if (!gtxid.IsWtxid() && g_wtxid_relay_peers > 0) delay += TXID_RELAY_DELAY;
     const bool overloaded = !node.HasPermission(PF_RELAY) &&
         m_txrequest.CountInFlight(nodeid) >= MAX_PEER_TX_REQUEST_IN_FLIGHT;
     if (overloaded) delay += OVERLOADED_PEER_TX_DELAY;

DrahtBot added the label Tests on Oct 16, 2020

in test/functional/p2p_segwit.py:150 in 637d7a128e outdated

 147 | @@ -148,7 +148,7 @@ def test_witness_block(node, p2p, block, accepted, with_witness=True, reason=Non
 148 |  
 149 |  class TestP2PConn(P2PInterface):
 150 |      def __init__(self, wtxidrelay=False):
 151 | -        super().__init__()
 152 | +        super().__init__(wtxidrelay=wtxidrelay)

sipa commented at 10:40 PM on October 16, 2020:

Can you make the changes to the test framework in a separate commit?

ariard commented at 3:26 PM on October 17, 2020:

Sure, I did it at first before to squash. Split in two.

in test/functional/p2p_tx_download.py:238 in 637d7a128e outdated

 227 | +        self.nodes[0].add_p2p_connection(TestP2PConn(wtxidrelay=True))
 228 | +        peer.send_message(msg_inv([CInv(t=MSG_TX, h=0xff11ff11)]))
 229 | +        peer.sync_with_ping()
 230 | +        with p2p_lock:
 231 | +            assert_equal(peer.tx_getdata_count, 0)
 232 | +        self.nodes[0].setmocktime(mock_time + TXID_RELAY_DELAY)

sipa commented at 10:40 PM on October 16, 2020:

Wouldn't it be better to start mocking before the inv is sent? So that local timing doesn't matter?

ariard commented at 3:27 PM on October 17, 2020:

I think that's already done L223, we set mocktime for nodes 0 before it's even connected to the first p2p connection. Or do you have a more constraining definition of mocktime in mind ?

naumenkogs commented at 7:16 AM on October 19, 2020:

Shouldn't NONPREF_PEER_TX_DELAY also be applied here?

Anyway, I'd first jump in NONPREF_PEER_TX_DELAY to see that it's not enough time passed, and then jump TXID_RELAY_DELAY again to see that enough time passed? I think that's what is supposed to happen..

glozow commented at 3:58 PM on October 20, 2020:

Is this supposed to test that the node waits for the duration of TXID_RELAY_DELAY? Seems like it just confirms that the node doesn't send a getdata immediately, then sends it ~TXID_RELAY_DELAY later? I had imagined making TestP2PConn record the time at which it receives getdatas, then asserting that the time difference from sending the inv is at least TXID_RELAY_DELAY... don't think it could use setmocktime though so idk

ariard commented at 1:35 PM on October 22, 2020:

The positive case was already tested in test_preferred_inv but added a mutation to cover the negative one, i.e applying NONPREF_PEER_TX_DELAY on non-preferred peers.

ariard commented at 1:37 PM on October 22, 2020:

I don't think you can achieve this with current p2p framework, we can write to the mocktime but can we read it from it ? A quick look, I don't find test doing it so not sure that's a feature of our current test framework. Note that was already the way done by legacy (pre-#19988) tx-download tests. But lmk if there is way to do so I don't see

sipa commented at 11:17 PM on October 22, 2020:

Oh, of course!

glozow commented at 4:02 AM on October 23, 2020:

I don't find test doing it so not sure that's a feature of our current test framework

Yeah, I haven't seen an example either. The way I'd do it is just use sleep() instead of setmocktime... 2 seconds isn't too bad when the tests are running in parallel, but fosho not ideal.

sipa commented at 10:40 PM on October 16, 2020: member

Concept ACK

Linter error: test/functional/p2p_segwit.py:14:1: F401 'test_framework.messages.msg_verack' imported but unused

ariard force-pushed on Oct 17, 2020

ariard commented at 3:33 PM on October 17, 2020: member

Thanks, updated at f0fe840. See if this comment needs further modification.

in test/functional/p2p_tx_download.py:235 in f0fe840ace outdated

 230 | +        with p2p_lock:
 231 | +            assert_equal(peer.tx_getdata_count, 0)
 232 | +        self.nodes[0].setmocktime(mock_time + TXID_RELAY_DELAY)
 233 | +        peer.wait_until(lambda: peer.tx_getdata_count >= 1, timeout=1)
 234 | +        with p2p_lock:
 235 | +            assert_equal(peer.tx_getdata_count, 1)

naumenkogs commented at 7:11 AM on October 19, 2020:

wait until right above wouldn't allow this line to happen if tx_getdata_count == 0. So we're checking it's NOT more than 1? Is it a worthy check?... It has nothing to do with delays.

glozow commented at 3:52 PM on October 20, 2020:

yeh, testnode wait_until grabs p2p_lock so this is redundant with L233

ariard commented at 1:34 PM on October 22, 2020:

Right, in fact it sounds other new functional tests added with #19988 have a redundant p2p lock tacking so added a new commit to remove this oversight. Unless @MarcoFalke they serve something else ?

in test/functional/p2p_tx_download.py:225 in f0fe840ace outdated

 220 | +        self.log.info('Check that inv from a txid-relay peers are delayed by {} s'.format(TXID_RELAY_DELAY))
 221 | +        self.restart_node(0, extra_args=['-whitelist=noban@127.0.0.1'])
 222 | +        mock_time = int(time.time() + 1)
 223 | +        self.nodes[0].setmocktime(mock_time)
 224 | +        peer = self.nodes[0].add_p2p_connection(TestP2PConn(wtxidrelay=False))
 225 | +        # Add a second wtxid-relay connection otherwise TXID_RELAY_DELAY is waived in

naumenkogs commented at 7:13 AM on October 19, 2020:

perhaps also worth testing?

ariard commented at 1:34 PM on October 22, 2020:

Added a test mutation.

naumenkogs commented at 7:16 AM on October 19, 2020: member

Concept ACK

glozow commented at 4:06 PM on October 20, 2020: member

Concept ACK

MarcoFalke commented at 8:54 AM on October 21, 2020: member

ACK f0fe840ace0d317dcde48d1f21554f884accbbf9

ariard force-pushed on Oct 22, 2020

ariard commented at 1:37 PM on October 22, 2020: member

Thanks @naumenkogs, @glozow for reviews, updated addressing your comments at c55ce67

DrahtBot commented at 11:58 AM on October 28, 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:

#20524 (test: Move MIN_VERSION_SUPPORTED to p2p.py by jnewbery)
#20277 (p2p: Stop processing unrequested transactions during IBD and extend p2p_ibd_txrelay.py coverage by ariard)
#19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)

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.

in test/functional/p2p_tx_download.py:50 in c55ce67f2c outdated

  46 | @@ -47,6 +47,7 @@ def on_getdata(self, message):
  47 |  OVERLOADED_PEER_DELAY = 2 # seconds
  48 |  MAX_GETDATA_IN_FLIGHT = 100
  49 |  MAX_PEER_TX_ANNOUNCEMENTS = 5000
  50 | +NONPREF_PEER_TX_DELAY=2

jonatack commented at 8:14 PM on November 1, 2020:

NONPREF_PEER_TX_DELAY = 2

in test/functional/p2p_tx_download.py:240 in c55ce67f2c outdated

 242 |          with p2p_lock:
 243 | -            assert_equal(peer.tx_getdata_count, 1)
 244 | +            if glob_wtxid:
 245 | +                    assert_equal(peer.tx_getdata_count, 0)
 246 | +            else:
 247 | +                    assert_equal(peer.tx_getdata_count, 1)

jonatack commented at 8:15 PM on November 1, 2020:

Tabs 4 spaces instead of 8, but you could also simplify here with

-            if glob_wtxid:
-                    assert_equal(peer.tx_getdata_count, 0)
-            else:
-                    assert_equal(peer.tx_getdata_count, 1)
+            assert_equal(peer.tx_getdata_count, 0 if glob_wtxid else 1)

ariard commented at 11:34 PM on November 2, 2020:

Taking it. Less is better.

in test/functional/test_framework/p2p.py:400 in c55ce67f2c outdated

 396 | @@ -394,7 +397,7 @@ def on_verack(self, message):
 397 |  
 398 |      def on_version(self, message):
 399 |          assert message.nVersion >= MIN_VERSION_SUPPORTED, "Version {} received. Test framework only supports versions greater than {}".format(message.nVersion, MIN_VERSION_SUPPORTED)
 400 | -        if message.nVersion >= 70016:
 401 | +        if message.nVersion >= 70016 and self.wtxidrelay :

jonatack commented at 8:16 PM on November 1, 2020:

        if message.nVersion >= 70016 and self.wtxidrelay:

jonatack commented at 8:25 PM on November 1, 2020: member

Approach ACK. Verified the new test does fail per the PR description.

fanquake deleted a comment on Nov 1, 2020

test: Makes wtxidrelay support a generic P2PInterface option

Its usage is extended beyond p2p_segwit.py in next commit.

a07910abcd

Add functional test test_txid_inv_delay

Add a simple functional test to cover TXID_RELAY_DELAY as applied
as a TxRequestTracker parameter in AddTxAnnoucement.

06efb3163c

Add mutation for functional test test_preferred_inv

Add a booelan arg to test_preferred_inv to cover NONPREF_PEER_TX_DELAY
as applied as a TxRequestTracker parameter in AddTxAnnouncement.

d3b5eac9a9

Remove redundant p2p lock tacking for tx download functional tests

New functional test coverage of tx download was added by #19988,
but `with p2p_lock` is redundant for some tests with `wait_until`
test helper, already guaranteeing test lock tacking.

bc4a230087

ariard force-pushed on Nov 2, 2020

ariard commented at 11:39 PM on November 2, 2020: member

Thanks @jonatack. I think I took everything at bc4a230

ariard commented at 4:25 PM on December 10, 2020: member

@naumenkogs @jonatack @glozow @MarcoFalke if you have few minutes to review again this PR, it hasn't changed and I think all previous considerations have been addressed.

laanwj commented at 5:27 PM on December 16, 2020: member

ACK bc4a23008762702ffcd6868bcdb8fe2a732640ba Thanks for adding a test for this.

laanwj merged this on Dec 16, 2020

laanwj closed this on Dec 16, 2020

sidhujag referenced this in commit 74cd6ddc31 on Dec 17, 2020

DrahtBot locked this on Feb 15, 2022

Add functional test test_txid_inv_delay #20171

Conflicts