Add functional test test_txid_inv_delay #20171

ariard · 2020-10-16T16:47:46Z

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;

sipa

Concept ACK

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

test/functional/p2p_segwit.py

test/functional/p2p_tx_download.py

ariard · 2020-10-17T15:33:13Z

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

naumenkogs · 2020-10-19T07:11:13Z

test/functional/p2p_tx_download.py

+        self.nodes[0].setmocktime(mock_time + TXID_RELAY_DELAY)
+        peer.wait_until(lambda: peer.tx_getdata_count >= 1, timeout=1)
+        with p2p_lock:
+            assert_equal(peer.tx_getdata_count, 1)


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.

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

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 ?

naumenkogs · 2020-10-19T07:13:16Z

test/functional/p2p_tx_download.py

+        mock_time = int(time.time() + 1)
+        self.nodes[0].setmocktime(mock_time)
+        peer = self.nodes[0].add_p2p_connection(TestP2PConn(wtxidrelay=False))
+        # Add a second wtxid-relay connection otherwise TXID_RELAY_DELAY is waived in


perhaps also worth testing?

Added a test mutation.

naumenkogs · 2020-10-19T07:16:38Z

test/functional/p2p_tx_download.py

+        peer.sync_with_ping()
+        with p2p_lock:
+            assert_equal(peer.tx_getdata_count, 0)
+        self.nodes[0].setmocktime(mock_time + TXID_RELAY_DELAY)


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..

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.

naumenkogs · 2020-10-19T07:16:53Z

Concept ACK

glozow

Concept ACK

glozow · 2020-10-20T15:52:27Z

test/functional/p2p_tx_download.py

+        self.nodes[0].setmocktime(mock_time + TXID_RELAY_DELAY)
+        peer.wait_until(lambda: peer.tx_getdata_count >= 1, timeout=1)
+        with p2p_lock:
+            assert_equal(peer.tx_getdata_count, 1)


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

glozow · 2020-10-20T15:58:55Z

test/functional/p2p_tx_download.py

+        peer.sync_with_ping()
+        with p2p_lock:
+            assert_equal(peer.tx_getdata_count, 0)
+        self.nodes[0].setmocktime(mock_time + TXID_RELAY_DELAY)


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

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

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.

maflcko · 2020-10-21T08:54:17Z

ACK f0fe840

ariard · 2020-10-22T13:37:52Z

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

DrahtBot · 2020-10-28T11:58:37Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Move MIN_VERSION_SUPPORTED to p2p.py #20524 (test: Move MIN_VERSION_SUPPORTED to p2p.py by jnewbery)
test: Extend p2p_ibd_tx_relay.py to verify no-transaction are requested during IBD #20277 (p2p: Stop processing unrequested transactions during IBD and extend p2p_ibd_txrelay.py coverage by ariard)
[tests] Allow outbound & block-relay-only connections in functional tests. #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.

jonatack

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

test/functional/p2p_tx_download.py

test/functional/test_framework/p2p.py

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

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

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

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

ariard · 2020-11-02T23:39:43Z

Thanks @jonatack. I think I took everything at bc4a230

ariard · 2020-12-10T16:25:58Z

@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 · 2020-12-16T17:27:28Z

ACK bc4a230
Thanks for adding a test for this.

bc4a230 Remove redundant p2p lock tacking for tx download functional tests (Antoine Riard) d3b5eac Add mutation for functional test test_preferred_inv (Antoine Riard) 06efb31 Add functional test test_txid_inv_delay (Antoine Riard) a07910a test: Makes wtxidrelay support a generic P2PInterface option (Antoine Riard) Pull request description: This is a simple functional test to increase coverage of bitcoin#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 f14db37..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; ``` ACKs for top commit: laanwj: ACK bc4a230 Tree-SHA512: 150e806bc5289feda94738756ab375c7fdd23c80c12bd417d3112043e26a91a717dc325a01079ebd02a88b90975ead5bd397ec86eb745c7870ebec379a8aa711

DrahtBot added the Tests label Oct 16, 2020

sipa reviewed Oct 16, 2020

View reviewed changes

test/functional/p2p_segwit.py Show resolved Hide resolved

test/functional/p2p_tx_download.py Show resolved Hide resolved

ariard force-pushed the 2020-10-txid-delay-test branch from 637d7a1 to f0fe840 Compare October 17, 2020 15:29

naumenkogs reviewed Oct 19, 2020

View reviewed changes

glozow reviewed Oct 20, 2020

View reviewed changes

ariard force-pushed the 2020-10-txid-delay-test branch from f0fe840 to c55ce67 Compare October 22, 2020 13:32

This was referenced Oct 28, 2020

[tests] Allow outbound & block-relay-only connections in functional tests. #19315

Merged

test: Extend p2p_ibd_tx_relay.py to verify no-transaction are requested during IBD #20277

Closed

jonatack reviewed Nov 1, 2020

View reviewed changes

test/functional/p2p_tx_download.py Outdated Show resolved Hide resolved

test/functional/p2p_tx_download.py Outdated Show resolved Hide resolved

test/functional/test_framework/p2p.py Outdated Show resolved Hide resolved

bitcoin deleted a comment from BARMOLEY007 Nov 1, 2020

Antoine Riard added 4 commits November 2, 2020 18:29

test: Makes wtxidrelay support a generic P2PInterface option

a07910a

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

Add functional test test_txid_inv_delay

06efb31

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

Add mutation for functional test test_preferred_inv

d3b5eac

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

Remove redundant p2p lock tacking for tx download functional tests

bc4a230

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

ariard force-pushed the 2020-10-txid-delay-test branch from c55ce67 to bc4a230 Compare November 2, 2020 23:37

This was referenced Nov 29, 2020

test: Move MIN_VERSION_SUPPORTED to p2p.py #20524

Merged

test: Fix Version Message Deserialization Discrepancy #20411

Closed

laanwj merged commit 5b6f970 into bitcoin:master Dec 16, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Add functional test test_txid_inv_delay #20171

Add functional test test_txid_inv_delay #20171

Uh oh!

Conversation

ariard commented Oct 16, 2020

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariard commented Oct 17, 2020

Uh oh!

naumenkogs Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naumenkogs commented Oct 19, 2020

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 21, 2020

Uh oh!

ariard commented Oct 22, 2020

Uh oh!

DrahtBot commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariard commented Nov 2, 2020

Uh oh!

ariard commented Dec 10, 2020

Uh oh!

laanwj commented Dec 16, 2020

Uh oh!

Uh oh!

naumenkogs Oct 19, 2020 •

edited

Loading

ariard Oct 22, 2020 •

edited

Loading

DrahtBot commented Oct 28, 2020 •

edited

Loading