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

pull ariard wants to merge 2 commits into bitcoin:master from ariard:2020-10-ibd-txrelay changing 3 files +70 −39
  1. ariard commented at 1:18 am on October 31, 2020: contributor

    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.

  2. fanquake added the label P2P on Oct 31, 2020
  3. DrahtBot commented at 9:17 am on October 31, 2020: contributor

    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.

  4. in src/net_processing.cpp:3029 in 8afa4fa353 outdated
    3024+                // transaction or this transaction is already double-spend but not yet seen block.
    3025+                // Further, we can't assert accuracy of its feerate.
    3026+                //
    3027+                // Orphan transaction is discarded. Note, we don't both to add id to recentRejects
    3028+                // as it's expected to be quickly clean up during IBD.
    3029+                if (ChainstateActive().IsInitialBlockDownload()) return;
    


    mjdietzx commented at 9:53 pm on October 31, 2020:
    Does the scope resolution operator need to be used here like everywhere else? if (::ChainstateActive().IsInitialBlockDownload()) return;

    ariard commented at 1:31 am on November 1, 2020:
    Good point! I’ll update once PR got Concept ACK/NACK.

    jonatack commented at 7:38 pm on November 1, 2020:

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

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

    “it’s expected” is somewhat ambiguous, could you use an explicit subject?


    ariard commented at 0:33 am on November 3, 2020:
    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.
  5. in test/functional/test_framework/p2p.py:125 in 8afa4fa353 outdated
    115@@ -116,6 +116,9 @@
    116     "signet": b"\x0a\x03\xcf\x40",    # signet
    117 }
    118 
    119+# Constants from net_processing
    120+INBOUND_PEER_TX_DELAY = 2  # seconds
    121+TXID_RELAY_DELAY = 2 # seconds
    


    jonatack commented at 6:12 pm on November 1, 2020:

    78b1044 Could you update the name of INBOUND_PEER_TX_RELAY for clarity and searchability of its equivalent in the net_processing code?

    0TXID_RELAY_DELAY = 2  # seconds
    

    ariard commented at 0:09 am on November 3, 2020:
    Switched to NONPREF_PEER_TX_RELAY.
  6. in test/functional/test_framework/p2p.py:411 in 8afa4fa353 outdated
    405@@ -401,6 +406,11 @@ def on_version(self, message):
    406             self.send_message(msg_sendaddrv2())
    407         self.nServices = message.nServices
    408 
    409+    def on_getdata(self, message):
    410+        for i in message.inv:
    411+            if i.type & MSG_TYPE_MASK == MSG_TX or i.type & MSG_TYPE_MASK == MSG_WTX:
    


    jonatack commented at 6:29 pm on November 1, 2020:

    78b1044

    0            if (i.type & MSG_TYPE_MASK) in {MSG_TX, MSG_WTX}:
    
  7. in test/functional/p2p_ibd_txrelay.py:89 in 8afa4fa353 outdated
    85+
    86+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
    87+        peer.send_message(msg_tx(tx_orphan))
    88+        # As tx-announcing connection is inbound, wait for the inbound delay applied by requester.
    89+        # Also apply the txid delay as orphan parent are requested in the legacy way.
    90+        sleep(INBOUND_PEER_TX_DELAY+TXID_RELAY_DELAY)
    


    jonatack commented at 7:26 pm on November 1, 2020:

    8afa4fa s/parent are/parents are/

    0        sleep(INBOUND_PEER_TX_DELAY + TXID_RELAY_DELAY)
    
  8. in test/functional/p2p_ibd_txrelay.py:26 in 8afa4fa353 outdated
    34+        OP_TRUE,
    35+        OP_0,
    36+)
    37+from test_framework.util import (
    38+        assert_equal,
    39+)
    


    jonatack commented at 7:29 pm on November 1, 2020:
    2ada3103 tabulation is off for all of the added imports, should be 4 spaces
  9. jonatack commented at 7:47 pm on November 1, 2020: member
    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”.
  10. maflcko commented at 7:33 am on November 2, 2020: member
    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.
  11. decryp2kanon commented at 3:11 pm on November 2, 2020: contributor
    Concept ACK
  12. decryp2kanon referenced this in commit 0b75d81939 on Nov 2, 2020
  13. bitcoin deleted a comment on Nov 2, 2020
  14. ariard commented at 0:46 am on November 3, 2020: contributor

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

  15. jnewbery commented at 12:23 pm on November 4, 2020: contributor

    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.

  16. sdaftuar commented at 4:02 pm on November 4, 2020: member
    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.
  17. ariard renamed this:
    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
    on Nov 5, 2020
  18. ariard force-pushed on Nov 5, 2020
  19. ariard commented at 1:43 am on November 5, 2020: contributor

    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.

  20. ariard force-pushed on Nov 5, 2020
  21. ariard commented at 2:01 am on November 5, 2020: contributor

    Note to reviewers, you might verify test_inv_ibd coverage with:

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

    And test_unrequested_tx_ibd:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
     1index c21c9e3ee..32cdba5f2 100644
     2--- a/src/net_processing.cpp
     3+++ b/src/net_processing.cpp
     4@@ -2928,8 +2928,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
     5         // and mitigate potential DoS risks.
     6         if (!pfrom.HasPermission(PF_RELAY) && ::ChainstateActive().IsInitialBlockDownload())
     7         {
     8-            LogPrint(BCLog::NET, "unrequested transaction while in IBD from peer=%d\n", pfrom.GetId());
     9-            return;
    10+            //LogPrint(BCLog::NET, "unrequested transaction while in IBD from peer=%d\n", pfrom.GetId());
    11+            //return;
    12         }
    13 
    14         CTransactionRef ptx;
    
  22. in src/net_processing.cpp:2929 in 51ea531ecf outdated
    2923@@ -2924,6 +2924,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
    2924             return;
    2925         }
    2926 
    2927+        // If we are in IBD, exit from unrequested transaction processing early to save bandwidth
    2928+        // and mitigate potential DoS risks.
    2929+        if (!pfrom.HasPermission(PF_RELAY) && ::ChainstateActive().IsInitialBlockDownload())
    


    maflcko commented at 6:29 am on November 5, 2020:
    0        if (!pfrom.HasPermission(PF_RELAY) && m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
    

    style-nit: Please don’t use the deprecated wrappers. This is going to silently conflict with ongoing work to remove them

    Also clang-format for new code

  23. in test/functional/p2p_ibd_txrelay.py:74 in 51ea531ecf outdated
    70+        script_pubkey = CScript([OP_0, witness_hash])
    71+
    72+        unrequested_tx = CTransaction()
    73+        unrequested_tx.vin.append(CTxIn(outpoint=COutPoint(uint256_from_str(b"23c70ed7c0506e9178fc1a987f40a33946d4ad4c962b5ae3a52546da53af0c5c"),
    74+ 0)))
    75+        unrequested_tx.vout.append(CTxOut(nValue=50 * COIN - 12000, scriptPubKey=script_pubkey))
    


    maflcko commented at 6:34 am on November 5, 2020:
    I don’t think the tx needs to be valid (and it can’t be valid because the utxo set is empty). Just using 0 or default constructors for CTxIn/Out should be sufficient.
  24. in test/functional/p2p_ibd_txrelay.py:70 in 51ea531ecf outdated
    74+ 0)))
    75+        unrequested_tx.vout.append(CTxOut(nValue=50 * COIN - 12000, scriptPubKey=script_pubkey))
    76+        unrequested_tx.calc_sha256()
    77+
    78+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
    79+        peer.send_message(msg_tx(unrequested_tx))
    


    maflcko commented at 6:35 am on November 5, 2020:
    0        peer.send_and_ping(msg_tx(unrequested_tx))
    

    Otherwise you are using the debug.log to synchronize. And the assert_equal(len(self.nodes[0].getrawmempool()), 0) doesn’t actually check anything.


    ariard commented at 0:22 am on November 9, 2020:
    I introduced a sync_with_ping inside the with statement. Test is breaking as expected with new code muted. Let me know if it achieves what you intended.
  25. ariard force-pushed on Nov 9, 2020
  26. ariard commented at 0:23 am on November 9, 2020: contributor
    Thanks @MarcoFalke, updated at 75b45eb.
  27. ariard force-pushed on Nov 9, 2020
  28. in test/functional/p2p_ibd_txrelay.py:9 in 75b45eb0da outdated
    5@@ -6,8 +6,27 @@
    6 
    


    jnewbery commented at 10:23 am on November 10, 2020:
    The docstring in this file could be updated. It’s no longer just testing fee filters.

    ariard commented at 4:56 pm on December 10, 2020:
    Yes updated the docstring, lmk about the formatting by bullet point.
  29. in src/net_processing.cpp:2928 in 75b45eb0da outdated
    2923@@ -2924,6 +2924,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
    2924             return;
    2925         }
    2926 
    2927+        // If we are in IBD, exit from unrequested transaction processing early to save bandwidth
    2928+        // and mitigate potential DoS risks.
    


    jnewbery commented at 10:25 am on November 10, 2020:
    This doesn’t save bandwidth since the tx is already downloaded. I think “Do not process unrequested transactions during IBD” is sufficient as a comment.

    ariard commented at 4:57 pm on December 10, 2020:
    There is a edge case where the transaction is an orphan and thus trigger further download but that’s not worthy to document as bandwidth saving. Updated.
  30. in test/functional/p2p_ibd_txrelay.py:67 in 75b45eb0da outdated
    63+        unrequested_tx.vout.append(CTxOut())
    64+        unrequested_tx.calc_sha256()
    65+
    66+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
    67+        peer.send_message(msg_tx(unrequested_tx))
    68+        with self.nodes[0].assert_debug_log(expected_msgs=['unrequested transaction while in IBD from peer=2'], unexpected_msgs=['from peer=2 was not accepted: scriptpubkey']):
    


    jnewbery commented at 10:29 am on November 10, 2020:
    The unexpected_msgs here is very specific. I understand that you’re trying to check that the tx wasn’t rejected by the mempool, but if the log format ever changes, then the unexpected_msgs string isn’t actually testing anything, and would just be dead code. I’d suggest removing it and relying on the expected_msgs string.

    ariard commented at 5:02 pm on December 10, 2020:

    Thanks for the tip, removed it. I’ve searched for a while for this but I don’t think we have better than checking the logs to test that feeding the node with some input (an unrequested tx) is effectively rejected by the targeted code check (the new check L2928) and not by the mempool checks further down in the code path ?

    I’m still ramping up on our p2p functional test framework :)

  31. jnewbery commented at 10:30 am on November 10, 2020: contributor

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

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

  32. ariard force-pushed on Dec 10, 2020
  33. ariard commented at 5:04 pm on December 10, 2020: contributor
    Thanks @jnewbery for review, sorry for delay and updated at 0cd6088.
  34. maflcko commented at 6:13 pm on December 10, 2020: member

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

    Edit: Or maybe not

  35. ariard commented at 4:05 pm on December 11, 2020: contributor
    @MarcoFalke Ah should I rebase on master to clean out the time out about libfuzzer+valgrind or it’s cirrus-ci’s VM runtime ?
  36. jnewbery commented at 1:19 pm on December 15, 2020: contributor
    @ariard can you try rebasing?
  37. ariard force-pushed on Dec 16, 2020
  38. ariard commented at 4:42 pm on December 16, 2020: contributor
    Rebased at d893b47.
  39. maflcko force-pushed on Dec 16, 2020
  40. DrahtBot added the label Needs rebase on Dec 16, 2020
  41. sdaftuar commented at 7:41 pm on December 16, 2020: member

    Do you have any thoughts on my suggestion from before, about just always ignoring unrequested transactions? #20277 (comment).

    It’s not really clear to me why we’re more concerned about this behavior during IBD than outside of IBD.

  42. jnewbery commented at 8:14 am on December 17, 2020: contributor
    Needs rebase!
  43. ariard commented at 11:09 pm on December 29, 2020: contributor

    @sdaftuar To answer I didn’t propose to stop processing of unrequested transactions even out-of-IBD because I was worried to break bitcoin clients which don’t respect the canonical tx-request sequence and thus smashing out their txn broadcast ?

    If we think such clients are non-existent, extending TxRequestTracker with a bool ExpectedResponse() should be possible.

  44. ariard force-pushed on Dec 29, 2020
  45. ariard commented at 11:52 pm on December 29, 2020: contributor

    Rebased at 886d05d.

    See IRC discussion with concerns about potentially breaking bitcoin software if we go with Suhas suggestion: http://gnusha.org/bitcoin-core-dev/2020-12-29.log (starting at 23:10)

  46. DrahtBot removed the label Needs rebase on Dec 30, 2020
  47. sdaftuar commented at 1:39 pm on January 5, 2021: member

    See IRC discussion with concerns about potentially breaking bitcoin software if we go with Suhas suggestion: http://gnusha.org/bitcoin-core-dev/2020-12-29.log (starting at 23:10)

    From that IRC conversation:

    15:24 < phantomcircuit> im also not sure the core concept of ignoring those transactions makes a lot of sense 15:25 < phantomcircuit> messages from peers are handled round robin so the worst an attacker can do is add latency and make one core run at 100%

    Yes, an attacker can add latency – a substantial amount, for free, by maintaining multiple inbound connections to a listening node and sending expensive to validate transactions that are not accepted to the mempool (eg for policy failures during script validation). The last time I looked at this, it seemed like spending on the order of 1 second (on my modern workstation) to validate a transaction that would ultimately fail mempool acceptance was easily achievable; multiply that by the number of inbound connections we allow and you get a pretty bad state of affairs…

    By requiring an INV/getdata sequence before processing, we would be able to deprioritize announcements from peers that are DoSing us in this way. A first step to that is establishing that processing unrequested transactions is not a required or expected behavior on the network.

    15:42 < sipa> i’m not so much worried about such clients on the public network (we’d know, or at least be able to add monitoring for that to figure out), but there could be software communicating locally with bitcoin core (things like joinmarket, lightning clients, indexers, …), for which we wouldn’t really know 15:43 < sipa> of course, perhaps one generally sets PF_RELAY for those or so

    Agreed that PF_RELAY could be used to bypass what I’m suggesting.

  48. DrahtBot added the label Needs rebase on Jan 11, 2021
  49. test: Make getdata accounting a generic P2PInterface option
    We also move some constants from net_processing in p2p.py
    5b2ab873f7
  50. ariard force-pushed on Feb 8, 2021
  51. DrahtBot removed the label Needs rebase on Feb 8, 2021
  52. ariard force-pushed on Feb 9, 2021
  53. ariard renamed this:
    p2p: Stop processing unrequested transactions during IBD and extend p2p_ibd_txrelay.py coverage
    p2p: Stop processing unrequested transactions and extend p2p_ibd_txrelay.py coverage
    on Feb 9, 2021
  54. ariard force-pushed on Feb 9, 2021
  55. in doc/release-notes.md:70 in 94153eb67f outdated
    65+  Previously, a transaction message was always submitted for processing, even if a
    66+  getdata for this identifier have not been previously issued. Henceforth, such
    67+  unrequested transaction will be rejected without any processing to mitigate about
    68+  DoS risks. Bitcoin clients on the network relying on unannounced transaction broadcast
    69+  should adapt their tx-relay to the compliant tx-request sequence. Peers might be
    70+  authorized to bypass this check with the PF_RELAY permission. (#20277)
    


    maflcko commented at 2:36 pm on February 9, 2021:
    I don’t think light clients can set the flag on the server they connect to. So this will break any such clients that opted to send txs directly. I know that there is no BIP (for obvious reasons) that explains how transaction relay should work, but I’d be cautious breaking a relay mechanism that some software relies on because it has been working in the past. At the very least this breaking change should be communicated to the mailing list.

    ariard commented at 3:46 pm on February 9, 2021:

    I agree on the mailing list communication. By the way, my concern about breaking tx-broadcast capabilities of such clients was one of the reason for all the back-and-forth on this PR. I expect those clients to be connected to at least few different peers and among them you can assume a subset will be prior versions still supporting processing of unrequested transactions. I would say deployment time give them a window of adaptation.

    That said, do you think I should announce on the mailing list now, even before this PR gets merged or only after ?


    sdaftuar commented at 3:48 pm on February 9, 2021:
    Thanks for taking the suggestion – I think it would make sense to mention on the mailing list now that this is a proposed behavior change for Bitcoin Core, so that if others have feedback on whether this is breaking they can speak up.
  56. ariard force-pushed on Feb 10, 2021
  57. ariard marked this as a draft on Feb 10, 2021
  58. ariard commented at 1:16 pm on February 10, 2021: contributor
    See mailing list thread, moving this PR as draft until I gather feedback on this proposal, or at least no one speaks up.
  59. jnewbery commented at 10:38 am on February 16, 2021: contributor
    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.
  60. in test/functional/p2p_tx_download.py:275 in b4622347ae outdated
    270+        with self.nodes[0].assert_debug_log(expected_msgs=['from peer=0 was not accepted: scriptpubkey']):
    271+            peer.sync_with_ping()
    272+            assert_equal(len(self.nodes[0].getrawmempool()), 0)
    273+        peer.peer_disconnect()
    274+        peer.wait_for_disconnect()
    275+
    


    ellemouton commented at 9:51 am on February 17, 2021:

    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.


    ariard commented at 2:23 pm on February 18, 2021:

    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 ?

  61. ariard commented at 5:05 pm on February 17, 2021: contributor
    @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.
  62. ariard force-pushed on Feb 18, 2021
  63. ariard renamed this:
    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
    on Feb 18, 2021
  64. ariard marked this as ready for review on Feb 18, 2021
  65. ariard commented at 2:17 pm on February 18, 2021: contributor
    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
  66. test: add coverage for no tx-request during ibd ec04fa50c7
  67. in test/functional/p2p_ibd_txrelay.py:48 in 8d7d5dc295 outdated
    44+    def test_inv_ibd(self):
    45+        self.log.info("Check that nodes don't request txn while still in IBD")
    46+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
    47+        peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0xffaa)]))
    48+        # As tx-announcing connection is inbound, wait for the inbound delay applied by requester.
    49+        sleep(NONPREF_PEER_TX_DELAY)
    


    maflcko commented at 2:20 pm on February 18, 2021:
    Could speed this up? I think txrequest works with mocktime, too

    ariard commented at 3:17 pm on February 23, 2021:
    Yes I can confirm txrequests works with mocktime. Updated with your suggestion.
  68. ariard force-pushed on Feb 23, 2021
  69. naumenkogs commented at 8:27 am on September 28, 2021: member

    ACK ec04fa50c703e726f2315195892d22c4486bf31a

    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.

  70. in test/functional/test_framework/p2p.py:331 in ec04fa50c7
    326@@ -324,6 +327,9 @@ def __init__(self, support_addrv2=False, wtxidrelay=True):
    327         # If the peer supports wtxid-relay
    328         self.wtxidrelay = wtxidrelay
    329 
    330+        # Getdata counter for tx-relay related tests
    331+        self.tx_getdata_count = 0
    


    jonatack commented at 2:17 pm on September 28, 2021:

    5b2ab873f7c793ea8ce871260b35 With this change can remove these lines?

    0--- a/test/functional/p2p_tx_download.py
    1+++ b/test/functional/p2p_tx_download.py
    2@@ -117,9 +117,6 @@ class TxDownloadTest(BitcoinTestFramework):
    3 
    4         p = self.nodes[0].p2ps[0]
    5 
    6-        with p2p_lock:
    7-            p.tx_getdata_count = 0
    8-
    9         mock_time = int(time.time() + 1)
    
  71. in test/functional/p2p_ibd_txrelay.py:73 in ec04fa50c7
    68@@ -37,6 +69,9 @@ def run_test(self):
    69             assert not node.getblockchaininfo()['initialblockdownload']
    70             self.wait_until(lambda: all(peer['minfeefilter'] == NORMAL_FEE_FILTER for peer in node.getpeerinfo()))
    71 
    72+    def run_test(self):
    73+        self.test_inv_ibd()
    


    jonatack commented at 2:33 pm on September 28, 2021:
    • 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:

     0--- a/test/functional/p2p_ibd_txrelay.py
     1+++ b/test/functional/p2p_ibd_txrelay.py
     2@@ -8,6 +8,7 @@
     3 """ 
     4 from decimal import Decimal
     5+import time
     6 
     7@@ -25,10 +26,10 @@ from test_framework.util import (
     8-import time
     9 
    10 MAX_FEE_FILTER = Decimal(9170997) / COIN
    11 NORMAL_FEE_FILTER = Decimal(100) / COIN
    12+WTXID = 0xffaa
    13 
    14@@ -43,7 +44,7 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
    15         peer = self.nodes[0].add_p2p_connection(P2PInterface())
    16-        peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0xffaa)]))
    17+        peer.send_message(msg_inv([CInv(t=MSG_WTX, h=WTXID)]))
    18
    19@@ -73,5 +74,6 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
    20         self.test_feefilter_ibd()
    21 
    22+
    23 if __name__ == '__main__':
    
    0--- a/test/functional/test_framework/p2p.py
    1+++ b/test/functional/test_framework/p2p.py
    2@@ -122,7 +122,7 @@ MAGIC_BYTES = {
    3 
    4 # Constants from net_processing
    5 NONPREF_PEER_TX_DELAY = 2  # seconds
    6-TXID_RELAY_DELAY = 2 # seconds
    7+TXID_RELAY_DELAY = 2  # seconds
    
  72. jonatack commented at 2:37 pm on September 28, 2021: member
    ACK ec04fa50c703e726f2315195892d22c4486bf31a with some questions and suggestions
  73. DrahtBot commented at 7:32 pm on November 30, 2021: contributor

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

  74. DrahtBot added the label Needs rebase on Nov 30, 2021
  75. DrahtBot commented at 1:07 pm on March 21, 2022: contributor
    • 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.
  76. maflcko added the label Up for grabs on Mar 21, 2022
  77. fanquake commented at 12:27 pm on April 26, 2022: member
    @jnewbery @mzumsande @naumenkogs you might be interested in re-opening / following up with this change?
  78. fanquake closed this on Apr 26, 2022

  79. bitcoin locked this on Apr 26, 2023
  80. maflcko removed the label Up for grabs on Aug 2, 2024
  81. maflcko removed the label Needs rebase on Aug 2, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 07:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me