test: p2p: add test for rejected tx request logic (m_recent_rejects filter) #29827

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202404-test-p2p-check_rejected_tx_requests changing 3 files +48 −4
  1. theStack commented at 10:10 am on April 7, 2024: contributor

    Motivated by the discussion in #28970 (https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1553911167), this PR adds test coverage for the logic around the m_recent_rejects filter, in particular that the filter is cleared after a new block comes in: https://github.com/bitcoin/bitcoin/blob/f0794cbd405636a7f528a60f2873050b865cf7e8/src/net_processing.cpp#L2199-L2206

    As expected, the second part of the test fails if the following patch is applied:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 6996af38cb..5cb1090e70 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2202,7 +2202,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid)
 5         // or a double-spend. Reset the rejects filter and give those
 6         // txs a second chance.
 7         hashRecentRejectsChainTip = m_chainman.ActiveChain().Tip()->GetBlockHash();
 8-        m_recent_rejects.reset();
 9+        //m_recent_rejects.reset();
10     }
11 
12     const uint256& hash = gtxid.GetHash();

    I’m still not sure in which file this test fits best, and if there is already test coverage for the first part of the test somewhere. Happy for any suggestions.

  2. DrahtBot commented at 10:10 am on April 7, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, glozow, instagibbs

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Apr 7, 2024
  4. in test/functional/p2p_tx_download.py:246 in e9122b7bc9 outdated 
    242@@ -241,6 +243,30 @@ def test_spurious_notfound(self):
243         self.log.info('Check that spurious notfound is ignored')
244         self.nodes[0].p2ps[0].send_message(msg_notfound(vec=[CInv(MSG_TX, 1)]))
245 
246+    def test_rejected_tx_request_logic(self):
    glozow commented at 6:56 am on April 8, 2024:
    nit: maybe test_rejects_filter_rest
  5. in test/functional/p2p_tx_download.py:263 in e9122b7bc9 outdated 
    258+        peer.sync_with_ping()
259+        assert_equal(peer.tx_getdata_count, 0)
260+
261+        self.log.info('Check that rejection filter is cleared after new block comes in')
262+        self.generate(self.wallet, 1)
263+        with self.nodes[0].assert_debug_log(expected_msgs=[f'got inv: wtx {low_fee_tx["wtxid"]}  new']):
    glozow commented at 7:01 am on April 8, 2024:
    I use assert_debug_log a lot in manual testing, but prefer not to in functional tests - logs aren’t supposed to be a stable interface (this would break if we change it) and they don’t really test the actual behavior we want. In this case, testing that the tx is requested is sufficient to me so we could just get rid of the with.
    theStack commented at 2:47 pm on April 9, 2024:
    Got rid of the “got inv” assert_debug_logs, the mempool reject reason is now checked with testmempoolaccept.
  6. in test/functional/p2p_tx_download.py:251 in e9122b7bc9 outdated 
    242@@ -241,6 +243,30 @@ def test_spurious_notfound(self):
243         self.log.info('Check that spurious notfound is ignored')
244         self.nodes[0].p2ps[0].send_message(msg_notfound(vec=[CInv(MSG_TX, 1)]))
245 
246+    def test_rejected_tx_request_logic(self):
247+        self.log.info('Check that rejected tx is not requested again')
248+        minrelaytxfee = self.nodes[0].getmempoolinfo()['minrelaytxfee']
249+        peer = self.nodes[0].add_p2p_connection(TestP2PConn())
250+        low_fee_tx = self.wallet.create_self_transfer(fee_rate=Decimal("0.9")*minrelaytxfee)
251+        with self.nodes[0].assert_debug_log(expected_msgs=['was not accepted: min relay fee not met']):
    glozow commented at 8:26 am on April 8, 2024:
    Btw if you wanted to hit the problem in #28970 (review), we would want the error to be “mempool min fee not met” (would need fill_mempool)
    theStack commented at 10:14 am on April 9, 2024:
    Oh good point, my intention was indeed to hit the TX_RECONSIDERABLE error, wasn’t aware that there is another “min relay fee not met” check before (in the future, after #29496 is in, I could simply create a v3 tx, right?). Will change to cause the “mempool min fee not met” error, probably building on #29735.
  7. glozow commented at 8:27 am on April 8, 2024: member
    concept ACK thanks! Maybe p2p_invalid_tx.py would be a good file but no strong opinion
  8. theStack force-pushed on Apr 9, 2024
  9. theStack commented at 3:01 pm on April 9, 2024: contributor
    This PR is based on #29735 now in order to take use of the fill_mempool (see #29827 (review)). Note that I had issues with this helper due to tx_to_be_evicted not being evicted (failed assertion assert tx_to_be_evicted_id not in node.getrawmempool()), as the mempool filling txs spent that newly created coin again; fixed by the first commit.
  10. DrahtBot added the label CI failed on Apr 9, 2024
  11. instagibbs commented at 7:00 am on April 10, 2024: member
    concept ack, will review once parent PR is merged
  12. theStack force-pushed on Apr 11, 2024
  13. in test/functional/p2p_tx_download.py:260 in cdc8283fe1 outdated 
    255+        assert_equal(node.testmempoolaccept([low_fee_tx['hex']])[0]["reject-reason"], "mempool min fee not met")
256+        peer.send_and_ping(msg_tx(low_fee_tx['tx']))
257+        peer.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=int(low_fee_tx['wtxid'], 16))]))
258+        mocktime = int(time.time())
259+        mocktime += MAX_GETDATA_INBOUND_WAIT
260+        node.setmocktime(mocktime)
    glozow commented at 8:24 am on April 15, 2024:
    Use node.bumpmocktime(MAX_GETDATA_INBOUND_WAIT) instead?
    theStack commented at 1:41 pm on April 15, 2024:
    Good idea, done.
  14. in test/functional/test_framework/util.py:530 in cdc8283fe1 outdated 
    523@@ -524,6 +524,10 @@ def fill_mempool(test_framework, node, miniwallet):
524 
525     test_framework.log.debug("Create a mempool tx that will be evicted")
526     tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee)["txid"]
527+    # remove the newly created coin from MiniWallet's UTXO set, in order to avoid it being spent
528+    # in one of the mempool filling txs below (which could bump up its descendant fee-rate and
529+    # hence stop it from being evicted)
530+    miniwallet.get_utxo(txid=tx_to_be_evicted_id, mark_as_spent=True)
    glozow commented at 9:16 am on April 15, 2024:
    Nice catch! I wonder if a more wholistic way to avoid unintentionally creating tx packages is to gather all the (confirmed only) UTXOs at the beginning? e.g. this, but feel free to ignore
    theStack commented at 1:41 pm on April 15, 2024:

    Nice catch! I wonder if a more wholistic way to avoid unintentionally creating tx packages is to gather all the (confirmed only) UTXOs at the beginning? e.g. this, but feel free to ignore

    That seems like a good approach to avoid similar problems popping up in the future 👌 I’ve cherry-picked your commit (with small conflict resolutions to have it introduced before the new test commit).

    Btw, other approaches I’ve tried to fix the problem before:

    • avoid that the to-be-evicted tx enters MiniWallet’s UTXO set in the first place, by creating the tx with a different output script (but MiniWallet.send_to only allows to specify an absolute fee, no fee-rate, meh)
    • modify create_lots_of_big_transactions to always pick a confirmed UTXO (also hacky and kind of unrelated, as that function is not only used in fill_mempool)
    instagibbs commented at 3:14 pm on April 15, 2024:
    I’ll punt to future work(TM) but I was wondering if we could just generate an “ephemeral” miniwallet inside fill_mempool if the resulting utxos from filling the mempool aren’t necessary for the rest of the test?
    theStack commented at 4:29 pm on April 15, 2024:

    I’ll punt to future work(TM) but I was wondering if we could just generate an “ephemeral” miniwallet inside fill_mempool if the resulting utxos from filling the mempool aren’t necessary for the rest of the test?

    Hmm for that purpose I think it would be nice if different MiniWallet instances create different output scripts, to avoid confusion. Currently, MiniWallet instances for the same output type (e.g. the default ADDRESS_OP_TRUE) always result in the same UTXO pool (at least, after they call rescan_utxos()). Will look into this as follow-up.

  15. in test/functional/p2p_tx_download.py:271 in cdc8283fe1 outdated 
    266+        peer.sync_with_ping()
267+        peer.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=int(low_fee_tx['wtxid'], 16))]))
268+        mocktime += MAX_GETDATA_INBOUND_WAIT
269+        node.setmocktime(mocktime)
270+        peer.sync_with_ping()
271+        assert_equal(peer.tx_getdata_count, 1)
    glozow commented at 9:18 am on April 15, 2024:
    peer.wait_for_getdata() ?
    theStack commented at 1:42 pm on April 15, 2024:
    Thanks, done.
  16. glozow commented at 9:19 am on April 15, 2024: member
    CI failure looks related to the lines added here (I haven’t debugged though)
  17. theStack force-pushed on Apr 15, 2024
  18. theStack force-pushed on Apr 15, 2024
  19. in test/functional/rpc_packages.py:428 in 2b7141a15b outdated 
    420@@ -420,7 +421,10 @@ def test_maxburn_submitpackage(self):
421         assert_equal(node.getrawmempool(), [])
422 
423         self.log.info("Submitpackage maxburnamount arg testing")
424-        chained_txns_burn = self.wallet.create_self_transfer_chain(chain_length=2)
425+        chained_txns_burn = self.wallet.create_self_transfer_chain(
426+            chain_length=2,
427+            utxo_to_spend=self.wallet.get_utxo(confirmed_only=True),
    instagibbs commented at 2:52 pm on April 15, 2024:

    is this strictly necessary since the mempool is empty?

    fine to me to add as belt and suspenders either way

    theStack commented at 4:08 pm on April 15, 2024:

    is this strictly necessary since the mempool is empty?

    Good point. Without that change the test fails, but not for the reason I thought it does. I think the problem is that MiniWallet is not aware of restarts between nodes and still holds mempool UTXOs that it tries to spend. Will force-push in a bit with a fix that does a self.wallet.rescan_utxos() after restarts, which should tackle the problem as the root.

    instagibbs commented at 2:00 pm on April 19, 2024:
    micro-nit: this line can be removed safely now :partying_face:
  20. instagibbs approved
  21. instagibbs commented at 3:47 pm on April 15, 2024: member
    ACK 01b17b62cbeb168629d17eb2a54d547628e16759
  22. DrahtBot requested review from glozow on Apr 15, 2024
  23. fixup: get all utxos up front in fill_mempool, discourage wallet mixing 
    Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
    e9dc511a7e
  24. theStack force-pushed on Apr 15, 2024
  25. test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter) 60ca5d5508
  26. theStack force-pushed on Apr 15, 2024
  27. DrahtBot removed the label CI failed on Apr 16, 2024
  28. maflcko commented at 6:59 am on April 18, 2024: member

    ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493 🍳

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493 🍳
3w8iRdk8dg+qdlaW4opc/RAillh5l170CUg7VjSLLc06XOoXzYZuVBpqlMM0f/0oiInwKH5sWWdw6EZbyHp7ECA==
  29. DrahtBot requested review from instagibbs on Apr 18, 2024
  30. glozow commented at 10:59 am on April 19, 2024: member
    code review ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493
  31. instagibbs approved
  32. instagibbs commented at 2:01 pm on April 19, 2024: member

    ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493

    please don’t feel obligated to touch due to nit

  33. Fabcien referenced this in commit 67c0d93982 on Apr 19, 2024
  34. DrahtBot added the label Needs rebase on Apr 19, 2024
  35. DrahtBot commented at 4:39 pm on April 19, 2024: contributor

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

  36. glozow commented at 4:43 pm on April 19, 2024: member
    merged, I think there is a github error https://github.com/bitcoin/bitcoin/commit/67c0d93982ad214f5e0c9509e9dc3d6d792ad97c
  37. glozow closed this on Apr 19, 2024
  38. instagibbs commented at 8:29 pm on April 19, 2024: member
    Seeing a failure of this test in my other PR, though I can’t make heads or tails of the failure: https://cirrus-ci.com/task/6740372997537792
  39. theStack deleted the branch on Apr 19, 2024


theStack DrahtBot glozow instagibbs maflcko

Labels
Tests Needs rebase

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-06-29 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me