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

ariard · 2020-10-31T01:18:29Z

This PR extends our functional test coverage of net_processing.cpp, following the overhaul of transaction request.

It extends p2p_ibd_txrelay.py to verify that we don't request transaction during IBD, even lawfully ones from tx-relay peers. It also move network constants/methods in p2p.py to serve generically across the test framework.

DrahtBot · 2020-10-31T09:17:11Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23127 (tests: Use test framework utils where possible by vincenzopalazzo)
#21327 (net_processing: ignore transactions while in IBD by glozow)

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.

mjdietzx · 2020-10-31T21:53:10Z

src/net_processing.cpp

+                //
+                // Orphan transaction is discarded. Note, we don't both to add id to recentRejects
+                // as it's expected to be quickly clean up during IBD.
+                if (ChainstateActive().IsInitialBlockDownload()) return;


Does the scope resolution operator need to be used here like everywhere else? if (::ChainstateActive().IsInitialBlockDownload()) return;

Good point! I'll update once PR got Concept ACK/NACK.

jonatack

Concept ACK. Thanks for adding the no-tx-request-in-IBD test. Verified the no-parent-orphan-in-IBD test fails without commit "p2p: Don't resolve orphans during IBD".

test/functional/test_framework/p2p.py

test/functional/p2p_ibd_txrelay.py

jonatack · 2020-11-01T19:38:28Z

src/net_processing.cpp

+                //
+                // Orphan transaction is discarded. Note, we don't both to add id to recentRejects
+                // as it's expected to be quickly clean up during IBD.
+                if (ChainstateActive().IsInitialBlockDownload()) return;


d65dca4 partial WIP suggestions, though I think it could still be more clearly written

- // To save bandwidth during IBD, stop orphan processing early. With the state of - // our UTXO set far from tip, we might not have yet the unspent output spend by this - // transaction or this transaction is already double-spend but not yet seen block. - // Further, we can't assert accuracy of its feerate. + // If we are in IBD, exit from orphan processing early to save bandwidth. With the + // state of our UTXO set far from the tip, we might not yet have the unspent output + // of this transaction, or the transaction is already double-spent but not yet + // included in a block. We also cannot assert the accuracy of its feerate. // - // Orphan transaction is discarded. Note, we don't both to add id to recentRejects - // as it's expected to be quickly clean up during IBD. - if (ChainstateActive().IsInitialBlockDownload()) return; + // Orphan transaction is discarded. We don't bother to add it to recentRejects, as + // it's expected to be quickly cleaned up during IBD. + if (::ChainstateActive().IsInitialBlockDownload()) return;

"it's expected" is somewhat ambiguous, could you use an explicit subject?

Yes this is the recentRejects filter. I did modify the comment to underscore that during IBD blocks are received fast and thus the filter quickly cleanup in consequence.

maflcko · 2020-11-02T07:33:05Z

How much difference is this going to make in practice? For a node without incoming connections it seems unlikely to have peers that don't support the feefilter message. For a node with incoming connections, minor traffic waste doesn't seem to be a concern.

decryp2kanon · 2020-11-02T15:11:54Z

Concept ACK

- feature_uacomment.py - feature_asmap.py

ariard · 2020-11-03T00:46:16Z

@MarcoFalke

Right, I should have explicit the assumption on sender, that they are non-Bitcoin Core clients. Thus we can't be certain they enforce the regular tx-relay flow inv,getdata,tx but might send directly txn, don't bothering with any feefilter at all nor sending an inv first.

I agree that the waste is likely to be minor in any-case, but note that this PR is aiming to align behavior with current inv-reception code (L2690) as described in PR description. This point has the added benefit to make easier to reason on holistic tx-relay behavior when we review more important p2p stuff, IMHO.

jnewbery · 2020-11-04T12:23:00Z

EDIT: this was for a different version of this PR.

This seems like an extremely verbose PR to modify an unlikely and unimportant edge case. We don't expect to process unconfirmed transactions during IBD for a couple of reasons:

we'll send a feefilter set to MAX_MONEY, telling our peers not to INV transactions to us.
we'll ignore INVs for transactions.

So a peer would need to send us an unsolicited tx ignoring our feefilter for us to even reach this logic. It seems inconsistent to process that transaction as normal, but not do the normal orphan processing logic too.

We could modify the transaction processing logic to ignore all tx messages during IBD, which seems reasonable and at least consistent. I don't think we need a 10 line comment and 100 line test case to justify that - just say that we don't process unconfirmed transactions during IBD.

sdaftuar · 2020-11-04T16:02:53Z

I tend to agree with @jnewbery that just ignoring all unrequested transactions during IBD would make more sense, but I'd go further and suggest we generally ignore all unrequested transactions (perhaps excepting peers with PF_RELAY permission), to mitigate potential CPU DoS.

ariard · 2020-11-05T01:43:47Z

It seems inconsistent to process that transaction as normal, but not do the normal orphan processing logic too.

Thanks to answering my wonder about jumping directly to stop processing unrequested transaction instead of only scoping to orphan. Updated branch with this behavior.

I don't think we need a 10 line comment and 100 line test case to justify that - just say that we don't process unconfirmed transactions during IBD.

I hear you on that, some reviewers might like comments on rational not only on code behaviors, even for edge cases, hard to all agree on. Note that "100 line test case" includes a new test (test_inv_ibd) for old behavior, not processing inv-tx during IBD.

ariard · 2020-11-05T02:01:16Z

Note to reviewers, you might verify test_inv_ibd coverage with:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c21c9e3ee..5def5178e 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2687,7 +2687,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
                     LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId());
                     pfrom.fDisconnect = true;
                     return;
-                } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
+                } else if (!fAlreadyHave) {
                     AddTxAnnouncement(pfrom, gtxid, current_time);
                 }
             } else {

And test_unrequested_tx_ibd:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c21c9e3ee..32cdba5f2 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2928,8 +2928,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
         // and mitigate potential DoS risks.
         if (!pfrom.HasPermission(PF_RELAY) && ::ChainstateActive().IsInitialBlockDownload())
         {
-            LogPrint(BCLog::NET, "unrequested transaction while in IBD from peer=%d\n", pfrom.GetId());
-            return;
+            //LogPrint(BCLog::NET, "unrequested transaction while in IBD from peer=%d\n", pfrom.GetId());
+            //return;
         }
 
         CTransactionRef ptx;

src/net_processing.cpp

test/functional/p2p_ibd_txrelay.py

ariard · 2020-11-09T00:23:53Z

Thanks @MarcoFalke, updated at 75b45eb.

jnewbery

~~Concept ACK. Much prefer this more general approach to the original branch.~~

EDIT: this was for a different version of this PR.

test/functional/p2p_ibd_txrelay.py

src/net_processing.cpp

test/functional/p2p_ibd_txrelay.py

ariard · 2020-12-10T17:04:20Z

Thanks @jnewbery for review, sorry for delay and updated at 0cd6088.

maflcko · 2020-12-10T18:13:38Z

If you want the ci to run on this, you might have to rebase on current master.

Edit: Or maybe not

ariard · 2020-12-11T16:05:42Z

@MarcoFalke Ah should I rebase on master to clean out the time out about libfuzzer+valgrind or it's cirrus-ci's VM runtime ?

jnewbery · 2020-12-15T13:19:01Z

@ariard can you try rebasing?

ariard · 2021-02-10T13:16:25Z

See mailing list thread, moving this PR as draft until I gather feedback on this proposal, or at least no one speaks up.

jnewbery · 2021-02-16T10:38:15Z

As a minor procedural point, I think it would have been better to open a new PR for the current branch. This PR has now been through three significantly different iterations, and it's impossible to tell which versions the review comments/ACKs are referring to.

ellemouton · 2021-02-17T09:51:55Z

test/functional/p2p_tx_download.py

+    def test_unrequested_tx(self):
+        self.log.info("Check that nodes don't process unrequested txn")
+        self.restart_node(0)
+
+        unrequested_tx = CTransaction()
+        unrequested_tx.vin.append(CTxIn())
+
+        unrequested_tx.vout.append(CTxOut())
+        unrequested_tx.calc_sha256()
+
+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
+        peer.send_message(msg_tx(unrequested_tx))
+        with self.nodes[0].assert_debug_log(expected_msgs=['unrequested transaction from peer=0']):
+            peer.sync_with_ping()
+            assert_equal(len(self.nodes[0].getrawmempool()), 0)
+        peer.peer_disconnect()
+        peer.wait_for_disconnect()
+
+        self.log.info("Check that nodes process unrequested txn with relay permission ")
+        self.restart_node(0, extra_args=['-whitelist=relay@127.0.0.1'])
+
+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
+        peer.send_message(msg_tx(unrequested_tx))
+        with self.nodes[0].assert_debug_log(expected_msgs=['from peer=0 was not accepted: scriptpubkey']):
+            peer.sync_with_ping()
+            assert_equal(len(self.nodes[0].getrawmempool()), 0)
+        peer.peer_disconnect()
+        peer.wait_for_disconnect()
+


Seems that assert_equal(len(self.nodes[0].getrawmempool()), 0) isn't really doing anything since the tx being used is always invalid so we don't ever expect it to be accepted to the mempool.

Suggest using a valid tx instead and then testing the changes to the mempool directly (count=0 if tx msg is received before being requested and count=1 if tx msg is received after tx is requested) instead of doing an implicit check via the logs using assert_debug_log.

That's a good suggestion. Initially I was using valid transactions to feed the node and changed it after this comment. I guess the getrawmempool is a leftover. Will remove it on #21224.

I think it's still fine to use invalid transactions as the behavior under check is "Does this transaction reach mempool acceptance ?", which is probed through the assert_debug_log. "Is this transaction mempool-valid ?" is already a distinct checked behavior, of which we're not interested here ?

ariard · 2021-02-17T17:05:24Z

@jnewbery Agree, I've a new branch handling the parent-orphan fetching case and fixing all the tests. Going to open it soon and leave test coverage tx-requester extension here.

ariard · 2021-02-18T14:17:23Z

This PR has been restrained to the lightweight test coverage extension. The halting of unrequested transaction processing has been moved in its own one. See #21224

maflcko · 2021-02-18T14:20:01Z

test/functional/p2p_ibd_txrelay.py

+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
+        peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0xffaa)]))
+        # As tx-announcing connection is inbound, wait for the inbound delay applied by requester.
+        sleep(NONPREF_PEER_TX_DELAY)


Could speed this up? I think txrequest works with mocktime, too

Yes I can confirm txrequests works with mocktime. Updated with your suggestion.

naumenkogs · 2021-09-28T08:27:02Z

ACK ec04fa5

The test makes sense. I'd say it's gonna be easier to get this merged if you open a separate clean PR. As someone who looked at it for the first time, I found it distracting going through all irrelevant legacy comments.

jonatack

ACK ec04fa5 with some questions and suggestions

jonatack · 2021-09-28T14:17:24Z

test/functional/test_framework/p2p.py

@@ -324,6 +327,9 @@ def __init__(self, support_addrv2=False, wtxidrelay=True):
        # If the peer supports wtxid-relay
        self.wtxidrelay = wtxidrelay

+        # Getdata counter for tx-relay related tests
+        self.tx_getdata_count = 0


5b2ab87 With this change can remove these lines?

--- a/test/functional/p2p_tx_download.py +++ b/test/functional/p2p_tx_download.py @@ -117,9 +117,6 @@ class TxDownloadTest(BitcoinTestFramework): p = self.nodes[0].p2ps[0] - with p2p_lock: - p.tx_getdata_count = 0 - mock_time = int(time.time() + 1)

jonatack · 2021-09-28T14:33:47Z

test/functional/p2p_ibd_txrelay.py

@@ -37,6 +69,9 @@ def run_test(self):
            assert not node.getblockchaininfo()['initialblockdownload']
            self.wait_until(lambda: all(peer['minfeefilter'] == NORMAL_FEE_FILTER for peer in node.getpeerinfo()))

+    def run_test(self):
+        self.test_inv_ibd()


Would it make sense for this new test to be in p2p_tx_download.py where we have similar testing?

Perhaps also test that tx_getdata_count is/can be greater than zero once out of IBD?

A few PEP8 suggestions and (maybe, IIUC) a clarifying constant:

--- a/test/functional/p2p_ibd_txrelay.py +++ b/test/functional/p2p_ibd_txrelay.py @@ -8,6 +8,7 @@ """ from decimal import Decimal +import time @@ -25,10 +26,10 @@ from test_framework.util import ( -import time MAX_FEE_FILTER = Decimal(9170997) / COIN NORMAL_FEE_FILTER = Decimal(100) / COIN +WTXID = 0xffaa @@ -43,7 +44,7 @@ class P2PIBDTxRelayTest(BitcoinTestFramework): peer = self.nodes[0].add_p2p_connection(P2PInterface()) - peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0xffaa)])) + peer.send_message(msg_inv([CInv(t=MSG_WTX, h=WTXID)])) @@ -73,5 +74,6 @@ class P2PIBDTxRelayTest(BitcoinTestFramework): self.test_feefilter_ibd() + if __name__ == '__main__':

--- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -122,7 +122,7 @@ MAGIC_BYTES = { # Constants from net_processing NONPREF_PEER_TX_DELAY = 2 # seconds -TXID_RELAY_DELAY = 2 # seconds +TXID_RELAY_DELAY = 2 # seconds

DrahtBot · 2021-11-30T19:32:35Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2022-03-21T13:07:04Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

fanquake · 2022-04-26T12:27:02Z

@jnewbery @mzumsande @naumenkogs you might be interested in re-opening / following up with this change?

fanquake added the P2P label Oct 31, 2020

This was referenced Oct 31, 2020

Add functional test test_txid_inv_delay #20171

Merged

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

Merged

mjdietzx reviewed Oct 31, 2020

View reviewed changes

jonatack reviewed Nov 1, 2020

View reviewed changes

decryp2kanon referenced this pull request in sugarchain-project/yumekawa Nov 2, 2020

!Fix: During IBD, do not print "Potential stale tip detected..."

0b75d81

- feature_uacomment.py - feature_asmap.py

bitcoin deleted a comment from BARMOLEY007 Nov 2, 2020

ariard changed the title ~~p2p: Do not resolve orphans during IBD and extend p2p_ibd_txrelay.py coverage~~ p2p: Stop processing unrequested transactions during IBD and extend p2p_ibd_txrelay.py coverage Nov 5, 2020

ariard force-pushed the 2020-10-ibd-txrelay branch from 8afa4fa to f6b125a Compare November 5, 2020 01:35

ariard force-pushed the 2020-10-ibd-txrelay branch from f6b125a to 51ea531 Compare November 5, 2020 01:59

maflcko reviewed Nov 5, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

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

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

ariard force-pushed the 2020-10-ibd-txrelay branch from 51ea531 to d6ebe6a Compare November 9, 2020 00:23

ariard force-pushed the 2020-10-ibd-txrelay branch from d6ebe6a to 75b45eb Compare November 9, 2020 23:59

jnewbery reviewed Nov 10, 2020

View reviewed changes

test/functional/p2p_ibd_txrelay.py Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

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

This was referenced Nov 29, 2020

test: Move MIN_VERSION_SUPPORTED to p2p.py #20524

Merged

test: Fix Version Message Deserialization Discrepancy #20411

Closed

ariard force-pushed the 2020-10-ibd-txrelay branch from 75b45eb to 0cd6088 Compare December 10, 2020 16:56

ariard marked this pull request as draft February 10, 2021 13:14

This was referenced Feb 12, 2021

Net Processing: Move RelayTransaction() into PeerManager #21162

Merged

net/net processing: Move tx inventory into net_processing #21160

Merged

ellemouton reviewed Feb 17, 2021

View reviewed changes

ariard force-pushed the 2020-10-ibd-txrelay branch from 35b4168 to 8d7d5dc Compare February 18, 2021 13:48

ariard changed the title ~~p2p: Stop processing unrequested transactions and extend p2p_ibd_txrelay.py coverage~~ test: Extend p2p_ibd_tx_relay.py to verify no-transaction are requested during IBD Feb 18, 2021

ariard marked this pull request as ready for review February 18, 2021 13:56

ariard mentioned this pull request Feb 18, 2021

[p2p] Halt processing of unrequested transactions #21224

Closed

maflcko reviewed Feb 18, 2021

View reviewed changes

test: add coverage for no tx-request during ibd

ec04fa5

ariard force-pushed the 2020-10-ibd-txrelay branch from 8d7d5dc to ec04fa5 Compare February 23, 2021 15:16

DrahtBot mentioned this pull request Mar 1, 2021

net_processing: ignore transactions while in IBD #21327

Merged

ariard mentioned this pull request Aug 17, 2021

validation: mempool validation and submission for packages of 1 child + parents #22674

Merged

DrahtBot mentioned this pull request Sep 15, 2021

scripted-diff: update license URLs to https #22975

Closed

jonatack reviewed Sep 28, 2021

View reviewed changes

DrahtBot mentioned this pull request Nov 5, 2021

tests: Use test framework utils where possible #23127

Closed

DrahtBot added the Needs rebase label Nov 30, 2021

maflcko added the Up for grabs label Mar 21, 2022

fanquake closed this Apr 26, 2022

bitcoin locked and limited conversation to collaborators Apr 26, 2023

maflcko removed Up for grabs Needs rebase labels Aug 2, 2024

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack Nov 1, 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 Nov 2, 2020

Uh oh!

decryp2kanon commented Nov 2, 2020

Uh oh!

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

Uh oh!

Uh oh!

jnewbery commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar commented Nov 4, 2020

Uh oh!

ariard commented Nov 5, 2020

Uh oh!

ariard commented Nov 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jnewbery left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

ariard commented Dec 10, 2020

Uh oh!

maflcko commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented Dec 11, 2020

Uh oh!

jnewbery commented Dec 15, 2020

Uh oh!

ariard commented Feb 10, 2021

Uh oh!

jnewbery commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Feb 17, 2021

Uh oh!

ariard commented Feb 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Oct 31, 2020 •

edited

Loading

DrahtBot commented Oct 31, 2020 •

edited

Loading

jonatack Nov 1, 2020 •

edited

Loading

ariard commented Nov 3, 2020 •

edited

Loading

jnewbery commented Nov 4, 2020 •

edited

Loading

ariard commented Nov 9, 2020 •

edited

Loading

jnewbery left a comment •

edited

Loading

maflcko commented Dec 10, 2020 •

edited

Loading