test: Run mempool_updatefromblock even with wallet disabled #21999

pull reemuru wants to merge 1 commits into bitcoin:master from reemuru:test changing 1 files +52 −80
  1. reemuru commented at 9:30 PM on May 19, 2021: none

    This is a PR proposed in #20078

    This PR enables one more of the non-wallet functional tests (mempool_updatefromblock.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead.

    Overview:

    • Refactoring and removal of unnecessary imports and arguments to transaction_graph_test()
    • Improve logging
    • Decrease test execution time by ~70% (39s => 12s)

    Before:

    [USER@SERVER functional]$ time ./mempool_updatefromblock.py 
2021-05-19T21:02:10.809000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_42xs1s0g
2021-05-19T21:02:11.154000Z TestFramework (INFO): Creating 100 transactions...
2021-05-19T21:02:15.741000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:02:15.756000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:02:20.097000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:02:20.115000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:02:24.687000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:02:24.705000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:02:28.774000Z TestFramework (INFO): The last batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:02:29.160000Z TestFramework (INFO): All of the recently mined transactions have been re-added into the mempool in 0.3855452537536621 seconds.
2021-05-19T21:02:29.161000Z TestFramework (INFO): Checking descendants/ancestors properties of all of the in-mempool transactions...
2021-05-19T21:02:50.455000Z TestFramework (INFO): Stopping nodes
2021-05-19T21:02:50.508000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_42xs1s0g on exit
2021-05-19T21:02:50.508000Z TestFramework (INFO): Tests successful

real    0m39.895s
user    0m34.627s
sys     0m2.474s

    After:

    [USER@SERVER functional]$ time ./mempool_updatefromblock.py 
2021-05-19T21:08:17.190000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__3fuskkh
2021-05-19T21:08:17.493000Z TestFramework (INFO): Creating 100 transactions...
2021-05-19T21:08:17.634000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:08:17.686000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:08:17.771000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:08:17.820000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:08:17.896000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:08:17.943000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:08:18.009000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:08:18.058000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:08:18.060000Z TestFramework (INFO): Mempool size pre-invalidation: 0
2021-05-19T21:08:18.080000Z TestFramework (INFO): Mempool size post-invalidation: 100
2021-05-19T21:08:18.082000Z TestFramework (INFO): All of the recently mined transactions have been re-added into the mempool in 0.0179746150970459 seconds.
2021-05-19T21:08:18.082000Z TestFramework (INFO): Checking descendants/ancestors properties of all of the in-mempool transactions...
2021-05-19T21:08:29.441000Z TestFramework (INFO): Stopping nodes
2021-05-19T21:08:29.496000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test__3fuskkh on exit
2021-05-19T21:08:29.496000Z TestFramework (INFO): Tests successful

real    0m12.465s
user    0m5.843s
sys     0m0.800s
  2. fanquake added the label Tests on May 19, 2021
  3. in test/functional/mempool_updatefromblock.py:51 in b7c6eea861 outdated 
     102 | +        for i in range(size):
 103 | +            txs.append(self.wallet.send_self_transfer(from_node=node))
 104 | +            tx_size.append(96)
 105 | +            if (i+1) % n_tx_to_mine == 0:
 106 | +                self.log.info('The batch of {} transactions has been accepted into the mempool.'
 107 | +                .format(len(node.getrawmempool())))
    DariusParvin commented at 4:47 PM on May 20, 2021:

    It looks like this should be:

                    .format(n_tx_to_mine)
    reemuru commented at 5:24 PM on May 20, 2021:

    Nice catch! Yes will update.

  4. in test/functional/mempool_updatefromblock.py:55 in b7c6eea861 outdated 
     115 | -                start = time.time()
 116 | -                self.nodes[0].invalidateblock(first_block_hash)
 117 | -                end = time.time()
 118 | -                assert_equal(len(self.nodes[0].getrawmempool()), size)
 119 | -                self.log.info('All of the recently mined transactions have been re-added into the mempool in {} seconds.'.format(end - start))
 120 | +                self.log.info('{}'.format(block_ids))
    DariusParvin commented at 4:51 PM on May 20, 2021:

    is this useful to have in the log?

    reemuru commented at 5:26 PM on May 20, 2021:

    I think I was using this for debugging and set to info by mistake. Should remove this to reduce clutter on the console for sure.

    reemuru commented at 8:39 PM on May 20, 2021:

    Updated with ac8b08b

  5. in test/functional/mempool_updatefromblock.py:56 in b7c6eea861 outdated 
     116 | -                self.nodes[0].invalidateblock(first_block_hash)
 117 | -                end = time.time()
 118 | -                assert_equal(len(self.nodes[0].getrawmempool()), size)
 119 | -                self.log.info('All of the recently mined transactions have been re-added into the mempool in {} seconds.'.format(end - start))
 120 | +                self.log.info('{}'.format(block_ids))
 121 | +                self.log.info('Block height: {}'.format(math.ceil(i / n_tx_to_mine)))
    DariusParvin commented at 4:56 PM on May 20, 2021:

    'Block height' is probably misleading. Should this say something like?

                    self.log.info('Number of recently mined blocks: {}'.format(math.ceil(i / n_tx_to_mine)))

    but also is this useful to have? Without this we won't need to import math

    reemuru commented at 8:40 PM on May 20, 2021:

    Yeah, not sure what I thinking here. Could've just use len(block_ids) as well. Removed in ac8b08b

  6. in test/functional/mempool_updatefromblock.py:41 in a02da6d8c0 outdated 
      92 | -                self.log.info('The batch of {} transactions has been accepted into the mempool.'.format(len(self.nodes[0].getrawmempool())))
  93 | -                block_hash = self.nodes[0].generate(1)[0]
  94 | -                if not first_block_hash:
  95 | -                    first_block_hash = block_hash
  96 | -                assert_equal(len(self.nodes[0].getrawmempool()), 0)
  97 | +        self.wallet.generate(1)
    DariusParvin commented at 5:09 PM on May 20, 2021:

    scan_blocks is slightly quicker than generate, so instead we can use

            self.wallet.scan_blocks(start=76, num=1)

    (76 is the first block where the coinbase is mined to the miniwallet address)

    reemuru commented at 8:41 PM on May 20, 2021:

    Ah, I see. I was wondering how to implement the scan_blocks() but wasn't sure. Thanks! ac8b08b

  7. in test/functional/mempool_updatefromblock.py:58 in a02da6d8c0 outdated 
     101 | +        tx_size = []
 102 | +        for i in range(size):
 103 | +            txs.append(self.wallet.send_self_transfer(from_node=node))
 104 | +            tx_size.append(96)
 105 | +            if (i+1) % n_tx_to_mine == 0:
 106 | +                self.log.info('The batch of {} transactions has been accepted into the mempool.'
    DariusParvin commented at 5:12 PM on May 20, 2021:

    Would it be useful to have this in the line above?

                    assert_equal(len(node.getrawmempool()), n_tx_to_mine)
    reemuru commented at 8:44 PM on May 20, 2021:

    Shoots, I thought I had that but it was just assert for mempool after mining a block. Updated ac8b08b

  8. in test/functional/mempool_updatefromblock.py:71 in a02da6d8c0 outdated 
     132 | +        .format(end - start))
 133 |  
 134 |          self.log.info('Checking descendants/ancestors properties of all of the in-mempool transactions...')
 135 | -        for k, tx in enumerate(tx_id):
 136 | +        k = 0
 137 | +        for tx in txs:
    DariusParvin commented at 5:15 PM on May 20, 2021:

    I'm curious if there's any advantage to incrementing k manually rather than using enumerate? Otherwise enumerate seems more concise.

            for k, tx in enumerate(txs):
    reemuru commented at 8:42 PM on May 20, 2021:

    I checked and there is no significant difference in test execution time. Using enumerate ac8b08b

  9. DariusParvin commented at 5:16 PM on May 20, 2021: contributor

    obvious concept ACK. Great work! I left some suggestions/questions

  10. reemuru commented at 10:39 PM on May 20, 2021: none

    Updated with ac8b08b. Let me know if I missed anything! Also, should I roll theses commits up into one, or will they get squashed at merge time? Forgot to sign the first few. Recent changes:

    • self.wallet.generate(1) to self.wallet.scan_blocks(start=76, num=1)
    • remove blocks mined block_ids from cluttering logs along with unnecessary math import
    • put back enumerate for mempool properties checks
    • added assert before mining block
  11. DariusParvin commented at 12:37 AM on May 21, 2021: contributor

    The changes look good to me! I'm relatively new myself but I think at this stage it would make sense to squash everything into one commit.

  12. reemuru commented at 2:41 AM on May 21, 2021: none

    The changes look good to me! I'm relatively new myself but I think at this stage it would make sense to squash everything into one commit.

    Got it, thank you for your assistance!

  13. in test/functional/mempool_updatefromblock.py:41 in df07919caa outdated 
      33 | @@ -38,86 +34,49 @@ def transaction_graph_test(self, size, n_tx_to_mine=None, start_input_txid='', e
  34 |          More details: https://en.wikipedia.org/wiki/Tournament_(graph_theory)
  35 |          """
    sriramdvt commented at 4:42 PM on July 23, 2021:

    The old implementation of the test explicitly sets the ancestors and descendants of each transaction to form an acyclic tournament. However, wallet.send_self_transfer() only explicitly sets one ancestor for each transaction to form something like a singly linked list of size number of transactions. The acyclic tournament is then formed by the mempool. It would perhaps be a good idea to mention this in the description of transaction_graph_test

    reemuru commented at 6:24 PM on July 25, 2021:

    Updated the comment.

  14. in test/functional/mempool_updatefromblock.py:47 in df07919caa outdated 
      98 | +        txs = []
  99 | +        block_ids = []
 100 | +        tx_size = []
 101 | +        for i in range(size):
 102 | +            txs.append(self.wallet.send_self_transfer(from_node=node))
 103 | +            tx_size.append(96)
    sriramdvt commented at 4:48 PM on July 23, 2021:

    In the earlier implementation of MiniWallet, tx_size had only one fixed value. To account for the varying sizes of transactions in the MiniWallet, it would be a better idea to calculate the transaction size from the transaction returned by send_self_transfer.

  15. in test/functional/mempool_updatefromblock.py:63 in df07919caa outdated 
     123 | +        # Invalidate the first block to send the transactions back to the mempool.
 124 | +        start = time.time()
 125 | +        node.invalidateblock(block_ids[0])
 126 | +        end = time.time()
 127 | +        self.log.info('Mempool size post-invalidation: {}'.format(len(node.getrawmempool())))
 128 | +        assert_equal(len(self.nodes[0].getrawmempool()), size)
    sriramdvt commented at 4:51 PM on July 23, 2021:

    nit: Replace self.nodes[0] with node

  16. sriramdvt changes_requested
  17. sriramdvt commented at 4:57 PM on July 23, 2021: contributor

    tACK df07919caa36d1345590010f5552d9e24db2bbde. The test works properly and checks what it is supposed to check. I strongly recommend making these changes for clarity and to maintain forward compatibility with MiniWallet.

  18. reemuru commented at 6:26 PM on July 25, 2021: none

    tACK df07919. The test works properly and checks what it is supposed to check. I strongly recommend making these changes for clarity and to maintain forward compatibility with MiniWallet.

    Thanks for reviewing! Made some changes based on your suggestions.

  19. josibake commented at 1:27 PM on July 26, 2021: contributor

    Concept ACK

    Thanks for posting the profiling data! Definitely agree with more miniwallet usage, especially if it increases test execution speed. I compiled and ran the tests and everything passed, but I would definitely recommend running flake8 and making the suggested style changes. you can do this automatically with autopep8 (I did it by with the following steps):

    pip install autopep8
autopep8 --in-place mempool_updatefromblock.py

    you can double check the changes by running flake8 mempool_updatefromblock.py. you should only see line length warnings, which are fine to ignore

  20. practicalswift commented at 4:13 PM on July 26, 2021: contributor

    Concept ACK

  21. reemuru commented at 5:33 PM on July 26, 2021: none 
    pip install autopep8

    oh shoots! never used autopep8 before. will use from now on. Thanks for the tip! (^_^) => 0b7dbeb

  22. josibake commented at 10:27 PM on July 26, 2021: contributor 
    pip install autopep8

    oh shoots! never used autopep8 before. will use from now on. Thanks for the tip! (^_^) => 0b7dbeb

    looks great! id suggest squashing into one atomic commit per the contributing guide

  23. reemuru commented at 2:05 AM on July 27, 2021: none 
    pip install autopep8

    oh shoots! never used autopep8 before. will use from now on. Thanks for the tip! (^_^) => 0b7dbeb

    looks great! id suggest squashing into one atomic commit per the contributing guide

    Got it, thanks much!

  24. josibake commented at 11:06 AM on July 27, 2021: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/21999/commits/446c06a6fd8bf205e534be47c02d8088e40dd10f

    built without wallet, ran the test to verify everything is behaving as expected. also code reviewed to verify the logic of the test is unchanged (strictly a refactor)

  25. DrahtBot commented at 8:41 PM on July 27, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    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.

  26. DrahtBot cross-referenced this on Jul 27, 2021 from issue Mempool Update Cut-Through Optimization by JeremyRubin
  27. DrahtBot cross-referenced this on Jul 28, 2021 from issue test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke
  28. in test/functional/mempool_updatefromblock.py:85 in 446c06a6fd outdated 
     153 | -            assert_equal(self.nodes[0].getrawmempool(True)[tx]['descendantcount'], size - k)
 154 | -            assert_equal(self.nodes[0].getrawmempool(True)[tx]['descendantsize'], sum(tx_size[k:size]))
 155 | -            assert_equal(self.nodes[0].getrawmempool(True)[tx]['ancestorcount'], k + 1)
 156 | -            assert_equal(self.nodes[0].getrawmempool(True)[tx]['ancestorsize'], sum(tx_size[0:(k + 1)]))
 157 | +            assert_equal(node.getrawmempool(True)[
 158 | +                         id]['descendantcount'], size - k)
    MarcoFalke commented at 12:21 PM on July 28, 2021:

    please don't commit large style changes in the same commit as refactors/features. This makes review harder because it is not clear what is a refactor/style-change/feature

    https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

    MarcoFalke commented at 12:23 PM on July 28, 2021:

    Also I don't think the change here makes the code easier to read (neither for devs nor for scripts and parsers)

    reemuru commented at 1:51 PM on July 28, 2021:

    Oh shoot, my bad. :sob: Was using some auto-styling tool and must've finger fudged it. Fixed and squashed into 33865a5

    josibake commented at 3:12 PM on July 28, 2021:

    Oh shoot, my bad. sob Was using some auto-styling tool and must've finger fudged it. Fixed and squashed into 33865a5

    my fault, @reemuru , i gave bad advice. @MarcoFalke is correct: linting (style changes) should be in separate PR's from refactors/features

  29. josibake commented at 3:17 PM on July 28, 2021: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/22330/commits/c1c0768b619ab463a00052d0a584fba191eb02a9

    built without wallet and ran test

  30. in test/functional/mempool_updatefromblock.py:21 in 33865a5fba outdated 
      18 |  class MempoolUpdateFromBlockTest(BitcoinTestFramework):
  19 |      def set_test_params(self):
  20 |          self.num_nodes = 1
  21 | -        self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000']]
  22 | +        self.extra_args = [
  23 | +            ['-limitdescendantsize=1000', '-limitancestorsize=1000']]
    MarcoFalke commented at 3:26 PM on July 28, 2021:

    here are still style changes mixed in

  31. in test/functional/mempool_updatefromblock.py:91 in 33865a5fba outdated 
     162 |      def run_test(self):
 163 | -        # Use batch size limited by DEFAULT_ANCESTOR_LIMIT = 25 to not fire "too many unconfirmed parents" error.
 164 | -        self.transaction_graph_test(size=100, n_tx_to_mine=[25, 50, 75])
 165 | +        # Use batch size limited by DEFAULT_ANCESTOR_LIMIT = 25
 166 | +        # to not fire "too many unconfirmed parents" error.
 167 | +        self.transaction_graph_test(size=100, n_tx_to_mine=25)
    MarcoFalke commented at 3:26 PM on July 28, 2021:

    why is this changed?

    reemuru commented at 1:58 PM on July 30, 2021:

    Ah ok, apologies for the noise. Will put this into draft mode since some work is still needed. Thanks for review!

  32. test: Run mempool_updatefromblock even with wallet disabled
    Enables the mempool_updatefromblock non-wallet functional test
to be run even with the Bitcoin Core wallet disabled by using
the new MiniWallet instead.

- Refactoring and removal of unnecessary imports and arguments to
  transaction_graph_test()
- Improve logging
- Decrease test execution time by ~70% (39s => 12s)
    1332dbb526
  33. reemuru marked this as a draft on Jul 30, 2021
  34. reemuru closed this on Aug 1, 2021
  35. reemuru deleted the branch on Aug 1, 2021
  36. adamjonas commented at 11:17 PM on August 5, 2021: member

    @reemuru was there a reason this closed? Would you like to mark it up for grabs?

  37. adamjonas added the label Up for grabs on Aug 19, 2021
  38. pg156 commented at 1:40 PM on January 12, 2022: none

    I will start working on this as it is up for grabs. Please let me know if this is done elsewhere or no longer necessary?

  39. MarcoFalke commented at 1:50 PM on January 12, 2022: member

    You can open the file on the master branch and check if skip_if_no_wallet is still in the file to see if this is still relevant.

  40. pg156 commented at 2:43 AM on January 13, 2022: none

    Thanks @MarcoFalke. I see skip_if_no_wallet is still in https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_updatefromblock.py. I also checked each revision to the file since this PR was created, and don't see any change making this PR irrelevant. Did I miss anything before determining it is useful to work on this?

  41. pg156 cross-referenced this on Jan 17, 2022 from issue test: Convert non-wallet tests to use our python MiniWallet by MarcoFalke
  42. pg156 cross-referenced this on Jan 28, 2022 from issue test: use MiniWallet for mempool_updatefromblock.py by pg156
  43. MarcoFalke removed the label Up for grabs on Mar 22, 2022
  44. bitcoin locked this on Mar 22, 2023
Contributors
reemuruDariusParvinsriramdvtjosibakepracticalswiftDrahtBotMarcoFalkeadamjonaspg156
Labels
Tests
Linked (view graph)
#20078 test: Convert non-wallet tests to use our python MiniWallet#24183 test: use MiniWallet for mempool_updatefromblock.py
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-05-20 06:54 UTC