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:

     0[USER@SERVER functional]$ time ./mempool_updatefromblock.py 
 12021-05-19T21:02:10.809000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_42xs1s0g
 22021-05-19T21:02:11.154000Z TestFramework (INFO): Creating 100 transactions...
 32021-05-19T21:02:15.741000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
 42021-05-19T21:02:15.756000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
 52021-05-19T21:02:20.097000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
 62021-05-19T21:02:20.115000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
 72021-05-19T21:02:24.687000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
 82021-05-19T21:02:24.705000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
 92021-05-19T21:02:28.774000Z TestFramework (INFO): The last batch of 25 transactions has been accepted into the mempool.
102021-05-19T21:02:29.160000Z TestFramework (INFO): All of the recently mined transactions have been re-added into the mempool in 0.3855452537536621 seconds.
112021-05-19T21:02:29.161000Z TestFramework (INFO): Checking descendants/ancestors properties of all of the in-mempool transactions...
122021-05-19T21:02:50.455000Z TestFramework (INFO): Stopping nodes
132021-05-19T21:02:50.508000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_42xs1s0g on exit
142021-05-19T21:02:50.508000Z TestFramework (INFO): Tests successful
15
16real    0m39.895s
17user    0m34.627s
18sys     0m2.474s

    After:

     0[USER@SERVER functional]$ time ./mempool_updatefromblock.py 
 12021-05-19T21:08:17.190000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__3fuskkh
 22021-05-19T21:08:17.493000Z TestFramework (INFO): Creating 100 transactions...
 32021-05-19T21:08:17.634000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
 42021-05-19T21:08:17.686000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
 52021-05-19T21:08:17.771000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
 62021-05-19T21:08:17.820000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
 72021-05-19T21:08:17.896000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
 82021-05-19T21:08:17.943000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
 92021-05-19T21:08:18.009000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
102021-05-19T21:08:18.058000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
112021-05-19T21:08:18.060000Z TestFramework (INFO): Mempool size pre-invalidation: 0
122021-05-19T21:08:18.080000Z TestFramework (INFO): Mempool size post-invalidation: 100
132021-05-19T21:08:18.082000Z TestFramework (INFO): All of the recently mined transactions have been re-added into the mempool in 0.0179746150970459 seconds.
142021-05-19T21:08:18.082000Z TestFramework (INFO): Checking descendants/ancestors properties of all of the in-mempool transactions...
152021-05-19T21:08:29.441000Z TestFramework (INFO): Stopping nodes
162021-05-19T21:08:29.496000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test__3fuskkh on exit
172021-05-19T21:08:29.496000Z TestFramework (INFO): Tests successful
18
19real    0m12.465s
20user    0m5.843s
21sys     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:

    0                .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?

    0                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

    0        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?

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

    0        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 0: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: member

    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):

    0pip install autopep8
1autopep8 --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 
    0pip 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: member 
    0pip 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 
    0pip 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: member

    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: member

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

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

  27. josibake commented at 3:17 pm on July 28, 2021: member

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

    built without wallet and ran test

  28. 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
  29. 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!
  30. 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
  31. reemuru marked this as a draft on Jul 30, 2021
  32. reemuru closed this on Aug 1, 2021
  33. reemuru deleted the branch on Aug 1, 2021
  34. 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?
  35. adamjonas added the label Up for grabs on Aug 19, 2021
  36. 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?
  37. 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.
  38. 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?
  39. MarcoFalke removed the label Up for grabs on Mar 22, 2022
  40. DrahtBot locked this on Mar 22, 2023


reemuru DariusParvin sriramdvt josibake practicalswift DrahtBot MarcoFalke adamjonas pg156

Labels
Tests

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-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me