test: run mempool_reorg.py even with wallet disabled #21178

pull DariusParvin wants to merge 1 commits into bitcoin:master from DariusParvin:mempool-reorg-to-miniwallet changing 2 files +59 −58
  1. DariusParvin commented at 3:39 AM on February 15, 2021: contributor

    Run mempool_reorg.py test even when the wallet is disabled, as discussed in #20078.

    As part of this PR I created a new method in MiniWallet, create_self_transfer, to return a raw tx (without broadcasting it) and its associated utxo.

  2. DariusParvin force-pushed on Feb 15, 2021
  3. DrahtBot added the label Tests on Feb 15, 2021
  4. DrahtBot commented at 8:44 AM on February 15, 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:

    • #20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)

    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.

  5. DrahtBot cross-referenced this on Feb 15, 2021 from issue test: convert feature_bip68_sequence.py to use MiniWallet by danben
  6. DrahtBot cross-referenced this on Feb 15, 2021 from issue test: Run mempool_accept.py even with wallet disabled by stackman27
  7. DrahtBot cross-referenced this on Feb 15, 2021 from issue test: ensure any failing method in MiniWallet leaves no side-effects by mjdietzx
  8. DrahtBot cross-referenced this on Feb 15, 2021 from issue test: Run mempool_limit.py even with wallet disabled by stackman27
  9. DrahtBot cross-referenced this on Feb 15, 2021 from issue test: Run rpc_generateblock.py even with wallet disabled by nginocchio
  10. in test/functional/test_framework/wallet.py:88 in a96c58aad6 outdated
      84 | +    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None, locktime=0):
      85 | +        """Create and send a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
      86 | +
      87 | +        tx_hex, new_utxo = self.create_self_transfer(fee_rate=fee_rate, from_node=from_node, utxo_to_spend=utxo_to_spend, locktime=locktime)
      88 | +
      89 | +        tx_info = from_node.testmempoolaccept([tx_hex])[0]
    


    glozow commented at 1:41 PM on February 16, 2021:

    this is not needed anymore i believe


    stackman27 commented at 4:11 PM on February 22, 2021:

    I believe tx_info is being reused used in line 92


    stackman27 commented at 4:31 PM on February 22, 2021:

    Also is there a need to recheck testmempoolaccept? Every time we call send_self_transfer it checks the mempool acceptance through create_self_transfer so, can we just return proper values and remove the additional check?


    DariusParvin commented at 5:12 AM on April 5, 2021:

    good catch! I replaced it with testmempoolaccept with FromHex to get the txid and wtxid.

  11. in test/functional/mempool_reorg.py:58 in 2ff5034eb1 outdated
      65 | +        utxo = wallet.get_utxo(txid=coinbase_txids[0])
      66 | +        timelock_tx, _ = wallet.create_self_transfer(
      67 | +            from_node=self.nodes[0],
      68 | +            utxo_to_spend=utxo,
      69 | +            locktime=self.nodes[0].getblockcount() + 2
      70 | +            )
    


    glozow commented at 1:45 PM on February 16, 2021:

    nit

            timelock_tx, _ = wallet.create_self_transfer(
                from_node=self.nodes[0],
                utxo_to_spend=utxo,
                locktime=self.nodes[0].getblockcount() + 2
             )
    
  12. in test/functional/test_framework/wallet.py:136 in 2ff5034eb1 outdated
      59 | -        """Create and send a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
      60 | +    def create_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None, locktime=0):
      61 | +        """Create a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
      62 |          self._utxos = sorted(self._utxos, key=lambda k: k['value'])
      63 |          utxo_to_spend = utxo_to_spend or self._utxos.pop()  # Pick the largest utxo (if none provided) and hope it covers the fee
      64 |          vsize = Decimal(96)
    


    glozow commented at 1:53 PM on February 16, 2021:

    Not sure if this is relevant to this PR or previous one, but I'm surprised the vsize is hard-coded? It should be programmatically obtained or asserted to be correct after it's constructed. There's a get_vsize() for CTransactions in test_framework.messages


    MarcoFalke commented at 3:37 PM on February 16, 2021:

    asserted to be correct

    There is an assertion: assert_equal(tx_info['vsize'], vsize)

  13. glozow commented at 1:56 PM on February 16, 2021: member

    Concept ACK, very nice!

  14. felipsoarez commented at 2:11 PM on February 16, 2021: none

    Concept ACK

  15. in test/functional/mempool_reorg.py:18 in 2ff5034eb1 outdated
      15 | +from test_framework.wallet import MiniWallet
      16 |  
      17 |  class MempoolCoinbaseTest(BitcoinTestFramework):
      18 |      def set_test_params(self):
      19 |          self.num_nodes = 2
      20 | +        self.setup_clean_chain = True
    


    MarcoFalke commented at 4:54 PM on February 16, 2021:

    Generating blocks takes a few seconds in valgrind, so I am thinking if this test may benefit from using miniwallet.scan_blocks (to be introduced in #21200)


    DariusParvin commented at 5:13 AM on April 5, 2021:

    I had a go but I don't think it helps in this case since the pre-mined chain doesn't send the coinbase transaction to self._address. I made it little bit faster though by only mining 100 blocks instead of 200.


    MarcoFalke commented at 7:01 AM on April 20, 2021:

    It should. Specifically the last 25 mature blocks (including block 100). And the last 25 immature blocks (including block 200), but you don't need those.


    MarcoFalke commented at 7:13 AM on April 29, 2021:

    Sorry, there was a bug. If you rebase, the bug should fix itself.


    DariusParvin commented at 1:11 AM on May 17, 2021:

    yes, it worked! I updated the PR to use miniwallet.scan_blocks starting from block 76.

  16. in test/functional/test_framework/wallet.py:89 in 2ff5034eb1 outdated
      85 | +        """Create and send a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
      86 | +
      87 | +        tx_hex, new_utxo = self.create_self_transfer(fee_rate=fee_rate, from_node=from_node, utxo_to_spend=utxo_to_spend, locktime=locktime)
      88 | +
      89 | +        tx_info = from_node.testmempoolaccept([tx_hex])[0]
      90 | +        self._utxos.append(new_utxo)
    


    stackman27 commented at 4:07 PM on February 22, 2021:

    nit: shouldn't the append be after sendrawtx as per this comment 🤔


    DariusParvin commented at 5:13 AM on April 5, 2021:

    switched it around!

  17. in test/functional/test_framework/wallet.py:81 in 2ff5034eb1 outdated
      77 | +        if tx_info['allowed']:
      78 | +            assert_equal(tx_info['vsize'], vsize)
      79 | +            assert_equal(tx_info['fees']['base'], fee)
      80 | +
      81 | +        new_utxo = {'txid': tx_info['txid'], 'vout': 0, 'value': send_value}
      82 | +        return tx_hex,  new_utxo
    


    stackman27 commented at 4:29 PM on February 22, 2021:

    I'm not very sure the intuition behind this but, can we just add everything in one variable and use key/value pairs to retrieve things? Seems cleaner to me and wouldn't have to include dead _ every time I create a tx. For instance instead of doing spend_101_raw, _ = wallet.create_self_transfer(from_node=self.nodes[0], utxo_to_spend=utxo_101) we can simplt do spend_101_raw= wallet.create_self_transfer(from_node=self.nodes[0], utxo_to_spend=utxo_101)['tx_hex']


    DariusParvin commented at 5:13 AM on April 5, 2021:

    done!

  18. stackman27 commented at 4:44 PM on February 22, 2021: contributor

    Concept ACK. mempool_reorg looks really good except, is there a reason why we aren't using self.log.info(...)? I always thought of them to be really helpful while running tests. Therefore, i think it'll be really nice to add the logs :)

  19. fanquake commented at 8:50 AM on April 1, 2021: member

    @DariusParvin are you planning on following up with the review comments here?

  20. DariusParvin commented at 8:58 PM on April 2, 2021: contributor

    @fanquake Yes, thanks for prompting me, I'll push updates this weekend!

  21. DariusParvin force-pushed on Apr 5, 2021
  22. DariusParvin commented at 6:05 AM on April 20, 2021: contributor

    I've addressed the comments, but since there are conflicting changes with #20874, it would probably make sense for that one to get merged first.

  23. DrahtBot cross-referenced this on Apr 22, 2021 from issue test: Run feature_cltv with MiniWallet by MarcoFalke
  24. DrahtBot cross-referenced this on Apr 23, 2021 from issue test: Speed up mempool_spend_coinbase.py by MarcoFalke
  25. DrahtBot added the label Needs rebase on Apr 29, 2021
  26. DariusParvin force-pushed on May 17, 2021
  27. DrahtBot removed the label Needs rebase on May 17, 2021
  28. DariusParvin force-pushed on May 17, 2021
  29. DariusParvin commented at 1:50 AM on May 17, 2021: contributor

    @stackman27 thanks for your review! I added logs instead of comments, as you suggested.

  30. in test/functional/mempool_reorg.py:42 in a856630c27 outdated
      51 | +        # 2. Indirect (coinbase spend in chain, child in mempool) : spend_2 and spend_2_1
      52 | +        # 3. Indirect (coinbase and child both in chain) : spend_3 and spend_3_1
      53 |          # Use invalidatblock to make all of the above coinbase spends invalid (immature coinbase),
      54 |          # and make sure the mempool code behaves correctly.
      55 | -        b = [self.nodes[0].getblockhash(n) for n in range(101, 105)]
      56 | +        b = [self.nodes[0].getblockhash(n) for n in range(76, 80)]
    


    MarcoFalke commented at 7:20 AM on May 17, 2021:
            b = [self.nodes[0].getblockhash(n) for n in range(first_block, first_block+4)]
    

    MarcoFalke commented at 3:39 PM on May 17, 2021:

    Any reason you didn't fix this as well on the latest force push?


    DariusParvin commented at 5:08 PM on May 17, 2021:

    sorry i didn't see this comments, i'll fix it now!


    DariusParvin commented at 5:10 PM on May 17, 2021:

    just pushed it. Thanks for the review

  31. MarcoFalke approved
  32. MarcoFalke commented at 7:26 AM on May 17, 2021: member

    cr ACK a856630c27cd28186d4e39dd1129f0cae8149261

  33. michaelfolkson commented at 11:29 AM on May 17, 2021: contributor

    I've addressed the comments, but since there are conflicting changes with #20874, it would probably make sense for that one to get merged first.

    Is this still waiting on #20874? Given Marco has reviewed this and not #20874 presumably this is closer to merging? 😅

    edit: #20874 needs a rebase so yeah I think this is closer to merging. Obvious Concept ACK too btw.

  34. michaelfolkson commented at 12:27 PM on May 17, 2021: contributor

    The test is still skipped when wallet is disabled. I think you need to delete:

    def skip_test_if_missing_module(self):
            self.skip_if_no_wallet()
    
  35. DariusParvin force-pushed on May 17, 2021
  36. DariusParvin commented at 3:25 PM on May 17, 2021: contributor

    @michaelfolkson oops! I removed those lines. And yes, since #21762 where create_self_transfer was added, this PR is relatively independent of everything else, so it's not waiting on anything. Cheers

  37. DariusParvin force-pushed on May 17, 2021
  38. DariusParvin force-pushed on May 18, 2021
  39. in test/functional/mempool_reorg.py:40 in 65dd482ba8 outdated
      48 | -        # 2. Indirect (coinbase spend in chain, child in mempool) : spend_102 and spend_102_1
      49 | -        # 3. Indirect (coinbase and child both in chain) : spend_103 and spend_103_1
      50 | +        # 1. Direct coinbase spend  :  spend_1
      51 | +        # 2. Indirect (coinbase spend in chain, child in mempool) : spend_2 and spend_2_1
      52 | +        # 3. Indirect (coinbase and child both in chain) : spend_3 and spend_3_1
      53 |          # Use invalidatblock to make all of the above coinbase spends invalid (immature coinbase),
    


    michaelfolkson commented at 11:26 AM on May 27, 2021:

    nit: s/invalidatblock/invalidateblock Might as well fix the typo


    DariusParvin commented at 8:49 PM on May 27, 2021:

    thanks! by the way, should I be squashing commits when making small fixes to the PR or will they get squashed at merge time?


    michaelfolkson commented at 10:17 AM on May 28, 2021:

    On this project we squash commits prior to review and merging. Obviously if there is a reason to have multiple commits then don't squash but in this case I don't think there should be a separate commit for a typo.

  40. michaelfolkson commented at 12:05 PM on May 27, 2021: contributor

    ACK 65dd482ba8c87443e4c27dc28f2027625ca43005

    Reviewed code, test passes (and not skipped) with wallet disabled.

  41. DrahtBot cross-referenced this on May 28, 2021 from issue test: Use COINBASE_MATURITY in functional tests by kiminuo
  42. DariusParvin force-pushed on May 28, 2021
  43. michaelfolkson commented at 5:49 PM on May 29, 2021: contributor

    Re-ACK 163e8881e88a798462a60b5232a6b4ff62e1969a

  44. DrahtBot added the label Needs rebase on May 31, 2021
  45. MarcoFalke commented at 11:32 AM on May 31, 2021: member

    s/disable/disabled/ in commit message 163e8881e88a798462a60b5232a6b4ff62e1969a

  46. DariusParvin force-pushed on May 31, 2021
  47. test: run mempool_reorg.py even with wallet disabled
    - run mempool_reorg.py even when the wallet is not compiled
    - add `locktime` argument to `create_self_transfer` and `send_self_transfer`
    - use more logs instead of comments
    a3f0cbf82d
  48. DariusParvin force-pushed on May 31, 2021
  49. DrahtBot removed the label Needs rebase on May 31, 2021
  50. DariusParvin commented at 5:41 PM on May 31, 2021: contributor

    thanks @MarcoFalke! typo fixed and rebased

  51. MarcoFalke commented at 1:04 PM on June 1, 2021: member

    cr ACK a3f0cbf82ddae2dd83001a9cc3a7948dcfb6fa47

  52. MarcoFalke merged this on Jun 1, 2021
  53. MarcoFalke closed this on Jun 1, 2021

  54. sidhujag referenced this in commit 2b04a4c46a on Jun 1, 2021
  55. shommel cross-referenced this on Apr 22, 2022 from issue test: Convert non-wallet tests to use our python MiniWallet by MarcoFalke
  56. gwillen referenced this in commit f46e4fdf55 on Jun 1, 2022
  57. bitcoin locked this on Aug 16, 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-05-20 06:54 UTC