test: Transaction expiry from mempool #18172

pull 0xB10C wants to merge 1 commits into bitcoin:master from 0xB10C:2020-02-mempool-expiry-test changing 2 files +101 −0
  1. 0xB10C commented at 1:15 PM on February 18, 2020: member

    This adds the functional test mempool_expiry.py covering mempool transaction expiry. Both the default DEFAULT_MEMPOOL_EXPIRY of 336 hours (two weeks, set in #9312) and the user definable mempool expiry via the -mempoolexpiry=<n> command line option are tested. The test checks that descendants of expired transactions are removed as well.

    Notes for reviewers

    • LimitMempoolSize() (which is the only caller of CTxMemPool::Expire()) is only called when a transaction is added to the mempool. In order to test expiry of a transaction-that-should-expire, the mocktime is set and a random transaction is broadcast to trigger LimitMempoolSize(). The transaction-that-should-expire is then checked for expiry. LMK if there is another way, but I don't think there is.
  2. fanquake added the label Tests on Feb 18, 2020
  3. in test/functional/mempool_expiry.py:8 in a7aced8fb6 outdated
       0 | @@ -0,0 +1,100 @@
       1 | +#!/usr/bin/env python3
       2 | +# Copyright (c) 2020 The Bitcoin Core developers
       3 | +# Distributed under the MIT software license, see the accompanying
       4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
       5 | +"""Tests that a mempool transaction expires after a given timeout and that its
       6 | +children are removed as well.
       7 | +
       8 | +Both the default expiry timeout defied by DEFAULT_MEMPOOL_EXPIRY and a user
    


    theStack commented at 1:47 PM on February 18, 2020:

    typo: s/defied/defined

  4. in test/functional/mempool_expiry.py:92 in a7aced8fb6 outdated
      87 | +                      DEFAULT_MEMPOOL_EXPIRY)
      88 | +        self.test_transaction_expiry(DEFAULT_MEMPOOL_EXPIRY)
      89 | +
      90 | +        self.log.info("----")
      91 | +
      92 | +        CUSTOM_MEMPOOL_EXPIRY = 10
    


    theStack commented at 1:50 PM on February 18, 2020:

    Could be moved to the top, right below DEFAULT_MEMPOOL_EXPIRY, to have all expiry constants at one place?

  5. in test/functional/mempool_expiry.py:17 in a7aced8fb6 outdated
      12 | +
      13 | +from datetime import timedelta
      14 | +
      15 | +from test_framework.test_framework import BitcoinTestFramework
      16 | +from test_framework.util import (assert_equal, assert_raises_rpc_error,
      17 | +                                 find_vout_for_address)
    


    theStack commented at 1:55 PM on February 18, 2020:
  6. theStack changes_requested
  7. 0xB10C force-pushed on Feb 18, 2020
  8. 0xB10C commented at 2:20 PM on February 18, 2020: member

    Thank you for the review @theStack! Addressed comments and force pushed.

  9. in test/functional/mempool_expiry.py:58 in 608c572077 outdated
      53 | +        # Let half of the timeout elapse and broadcast the child transaction.
      54 | +        half_expiry_time = entry_time + int(60 * 60 * timeout/2)
      55 | +        node.setmocktime(half_expiry_time)
      56 | +        child_txid = node.sendrawtransaction(child_signed)
      57 | +        self.log.info("Broadcast child transaction after {} hours.".format(
      58 | +            str(timedelta(seconds=(half_expiry_time-entry_time)))))
    


    MarcoFalke commented at 4:26 PM on February 18, 2020:

    I think str( is not needed when using format(


    0xB10C commented at 4:39 PM on February 18, 2020:

    will remove it on the next push

  10. MarcoFalke approved
  11. MarcoFalke commented at 4:26 PM on February 18, 2020: member

    ACK

  12. in test/functional/mempool_expiry.py:94 in 608c572077 outdated
      89 | +
      90 | +        self.log.info("Test default mempool expiry timeout of %d hours." %
      91 | +                      DEFAULT_MEMPOOL_EXPIRY)
      92 | +        self.test_transaction_expiry(DEFAULT_MEMPOOL_EXPIRY)
      93 | +
      94 | +        self.log.info("----")
    


    promag commented at 9:08 PM on February 18, 2020:

    nit, I'd drop this.

  13. in test/functional/mempool_expiry.py:22 in 608c572077 outdated
      17 | +    assert_equal,
      18 | +    assert_raises_rpc_error,
      19 | +    find_vout_for_address,
      20 | +)
      21 | +
      22 | +DEFAULT_MEMPOOL_EXPIRY = 336
    


    promag commented at 9:09 PM on February 18, 2020:

    nit, add comment with unit.

  14. in test/functional/mempool_expiry.py:43 in 608c572077 outdated
      38 | +        # Send a parent transaction that will expire.
      39 | +        parent_address = node.getnewaddress()
      40 | +        parent_txid = node.sendtoaddress(parent_address, 1.0)
      41 | +
      42 | +        # Set the mocktime to the arrival time of the parent transaction.
      43 | +        entry_time = node.getmempoolentry(parent_txid)["time"]
    


    promag commented at 9:10 PM on February 18, 2020:

    nit, could stick to " or ' for strings. My preference is '.

  15. in test/functional/mempool_expiry.py:67 in 608c572077 outdated
      62 | +        nearly_expiry_time = entry_time + 60 * 60 * timeout - 5
      63 | +        node.setmocktime(nearly_expiry_time)
      64 | +        # Expiry of mempool transactions is only checked when a new transaction
      65 | +        # is added to the to the mempool.
      66 | +        node.sendtoaddress(node.getnewaddress(), 1.0)
      67 | +        self.log.info("Test parent tx not expired after {} hours.".format(
    


    promag commented at 9:12 PM on February 18, 2020:

    nit, maybe keep these as comment instead? Not sure if logging this adds value.


    MarcoFalke commented at 9:26 PM on February 18, 2020:

    Logging is preferred over inline comments in the test because logging statements serve as "checkpoints" (to see how far a test run until failure). Also, they help to illustrate the test log better, which would otherwise only contain Bitcoin Core logs.


    promag commented at 9:33 PM on February 18, 2020:

    When a test fails we see trace. I think logging makes sense when it shows some computed values or so, which a trace doesn't help.


    MarcoFalke commented at 9:45 PM on February 18, 2020:

    The trace is one data point that can help debug a test failure. The entire history of the log up to that point is also useful. For example transaction relay (inv-getdata) often takes several seconds, so it is useful to know when a subtest has started and when it ended. If some Bitcoin Core logs from one subtest interleave with the ones of another subtest we can conclude that we are missing a syncronisation call. This is the most common test error we are running into.


    promag commented at 9:47 PM on February 18, 2020:

    Yes makes sense in those cases.

  16. in test/functional/mempool_expiry.py:89 in 608c572077 outdated
      84 | +        self.log.info("Test child tx is evicted as well.")
      85 | +        assert_raises_rpc_error(-5, "Transaction not in mempool",
      86 | +                                node.getmempoolentry, child_txid)
      87 | +
      88 | +    def run_test(self):
      89 | +
    


    promag commented at 9:13 PM on February 18, 2020:

    nit, drop empty line?

  17. 0xB10C force-pushed on Feb 18, 2020
  18. 0xB10C commented at 10:08 PM on February 18, 2020: member

    Thanks @promag and @MarcoFalke! Addressed nit's, but decided to keep #18172 (review) open for now.

  19. add: test that transactions expire from mempool
    This tests that a mempool transaction expires after a given timeout
    and its children are removed as well.
    
    Both the default expiry timeout defied by DEFAULT_MEMPOOL_EXPIRY and
    a user definable expiry timeout via the -mempoolexpiry=<n> command
    line argument (<n> is the timeout in hours) are tested.
    d6d2602a32
  20. 0xB10C force-pushed on Feb 19, 2020
  21. theStack approved
  22. MarcoFalke commented at 3:05 PM on February 19, 2020: member

    ACK d6d2602a32251c1017da88b47c801b7283c66ce3

  23. promag commented at 7:39 AM on February 21, 2020: member

    Code review ACK d6d2602a32251c1017da88b47c801b7283c66ce3.

  24. DrahtBot commented at 5:00 PM on February 21, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--2502f1a698b3751726fa55edcda76cd3-->

    Coverage

    Coverage Change (pull 18172, 8ed789fe4894a474074a55657b83f7d659aa35e0) Reference (master, eb3c6b0912349873113bfd49baf1c505bb48d9cf)
    Lines +0.0202 % 89.9453 %
    Functions +0.0000 % 85.8836 %
    Branches +0.0241 % 51.5363 %

    <sup>Updated at: 2020-02-21T17:00:37.583978.</sup>

  25. MarcoFalke merged this on Feb 21, 2020
  26. MarcoFalke closed this on Feb 21, 2020

  27. 0xB10C deleted the branch on Feb 22, 2020
  28. Fabcien referenced this in commit 7f8780957e on Jan 3, 2021
  29. PastaPastaPasta referenced this in commit 63d922a0d3 on Jun 27, 2021
  30. PastaPastaPasta referenced this in commit 71391cd4c6 on Jun 28, 2021
  31. PastaPastaPasta referenced this in commit 21c3927cc0 on Jun 29, 2021
  32. PastaPastaPasta referenced this in commit abb609ac06 on Jul 1, 2021
  33. PastaPastaPasta referenced this in commit f86ee82d9b on Jul 1, 2021
  34. PastaPastaPasta referenced this in commit 6a0627d073 on Jul 14, 2021
  35. DrahtBot locked this on Feb 15, 2022

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: 2026-04-17 03:14 UTC

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