test: Convert non-wallet tests to use our python MiniWallet #20078

issue maflcko openend this issue on October 4, 2020
  1. maflcko commented at 2:41 pm on October 4, 2020: member

    It is possible to compile Bitcoin Core without the wallet. Though, many tests require the Bitcoin Core wallet to create and sign transactions. Ideally, every non-wallet test (for example consensus tests) are run even with the Bitcoin Core wallet disabled. This ensures that users running without the wallet can test their Bitcoin Core with the functional test suite.

    Thus, tests that use the Bitcoin Core wallet should be rewritten to use the python MiniWallet.

    This is an open-ended issue. You can find candidate tests via git grep 'self.skip_if_no_wallet()'. Exclude the tests that start with wallet_, as those are tests that are meant to test the Bitcoin Core wallet.

    Refer to #19922 and #19800 as examples on how to approach this issue.

    Edit: See also #20078 (comment)

    Useful skills:

    • Familiarity with the RPC interface
    • Familiarity with our functional test suite
    • Good python3 skills

    Want to work on this issue?

    For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

  2. maflcko added the label good first issue on Oct 4, 2020
  3. maflcko referenced this in commit cd6e193d4c on Oct 13, 2020
  4. dboures commented at 2:47 am on October 14, 2020: none
    Hey, I can give this issue a shot.
  5. maflcko commented at 6:51 am on October 14, 2020: member
    Sure. No need to ask on this one, as it is open ended. There should be enough tests for everyone to convert
  6. maflcko referenced this in commit 80c8a02f1b on Oct 17, 2020
  7. sidhujag referenced this in commit 6436547758 on Oct 18, 2020
  8. laanwj referenced this in commit 04670ef81e on Nov 19, 2020
  9. maflcko referenced this in commit a023094fc4 on Dec 16, 2020
  10. sidhujag referenced this in commit 0432612bae on Dec 17, 2020
  11. maflcko referenced this in commit 1077c93a34 on Dec 21, 2020
  12. stackman27 commented at 9:23 pm on December 29, 2020: contributor
    hi there, does this issue still require work to be done on it?
  13. michaelfolkson commented at 6:43 pm on December 30, 2020: contributor

    @stackman27: As far as I can tell there are around 40 tests from git grep 'self.skip_if_no_wallet()' excluding tests starting with wallet_ (From Marco’s initial description)

    If you look at the above notifications it appears 4 test updates have been merged and 1 test update is open. Marco initially fixed two (p2p_feefilter.py, rpc_txoutproof.py) So there are lots of tests to work on.

    Please link to this issue if you open a PR as it will make it easier to monitor which ones people are working on.

  14. stackman27 commented at 6:01 am on December 31, 2020: contributor
    How would I go about creating and sending tx using miniwallet when the utxo generated doesnot have a key value in the output? Should I just manually append it or is there anything I’m missing? I found this issue while working on mempool_limit.py while creating utxo using create_confirmed_utxos
  15. michaelfolkson commented at 12:47 pm on December 31, 2020: contributor

    @stackman27: The Miniwallet class defines self._address which is a ADDRESS_BCRT1_P2WSH_OP_TRUE from address.py. This OP_TRUE address can be spent with a witness stack of just OP_TRUE so it doesn’t need any keys.

    In mempool_limit.py that should be fine for create_confirmed_utxos. I’m trying to work out if that is a challenge with create_lots_of_big_transactions later… I don’t think it is if I understand what is going on with splicing in "txouts" to each raw transaction

    For more info on OP_TRUE addresses (P2SH rather than P2WSH but may be useful): https://bitcoin.stackexchange.com/questions/72773/how-to-create-a-p2sh-transaction-with-a-scriptsig-of-op-true

  16. michaelfolkson commented at 6:58 pm on January 3, 2021: contributor
    Did you answer your question @stackman27? I think you deleted your latest post asking about value in wallet.py versus amount in mempool_limit.py. I think you can use value for amount unless I’m missing something.
  17. stackman27 commented at 7:04 pm on January 3, 2021: contributor

    Did you answer your question @stackman27? I think you deleted your latest post asking about value in wallet.py versus amount in mempool_limit.py. I think you can use value for amount unless I’m missing something.

    I think so. I just generated utxos using miniwallet.generate rather than create_confirmed_utxos

  18. stackman27 commented at 5:37 am on January 4, 2021: contributor
    @michaelfolkson i’m sorry to bug you so much. But how would i setup create_lots_of_big_transactions without modifying the send_self_transfer? Here’s what my setup looks like (https://ibb.co/gT8ctjW) but it’s not returning the correct mempoolminfee. Any hints?
  19. maflcko referenced this in commit 5082324225 on Jan 8, 2021
  20. practicalswift commented at 12:18 pm on January 15, 2021: contributor
    Concept ACK
  21. DariusParvin commented at 10:38 am on January 16, 2021: contributor
    I’ll have a go at mempool_reorg.py! Just putting it out there as it’ll be my first commit so I’m not sure how long it’ll take.
  22. stackman27 commented at 8:32 pm on January 31, 2021: contributor
    Hi @MarcoFalke is there a way to get information about a tx in miniwallet I tried doing node.gettransaction(txid) but got Invalid or non-wallet transaction id (-5) error?
  23. maflcko commented at 6:36 am on February 1, 2021: member
    miniwallet returns the transaction details when you create a transaction. If you need mempool info about the tx, you can use a mempool rpc. Note that the miniwallet itself doesn’t track the mempool/chain as of now.
  24. stackman27 commented at 4:02 pm on February 8, 2021: contributor

    hi @MarcoFalke, I was refactoring mempool_accept and felt the need to just sign an existing tx also in some parts of the code I only had to create a raw tx. So, I was wondering if it’d be good idea to break down send_self_transfer into create, sign & spend rather than just having to pass boolean send_tx or create_tx into the functional parameter?

    Also, how about the idea of accepting multiple inputs to spend like what’s being done here, I believe currently we can only spend one input utxo, but I think allowing multiple inputs to spend will prevent repetitions

  25. maflcko commented at 4:20 pm on February 8, 2021: member
    Splitting creation and signing doesn’t make too much sense, I think, because signing is just pushing OP_TRUE, which would complicate the interface to having to do manually or in a second step. Splitting up send_tx could make sense if it makes the interface clearer.
  26. danben referenced this in commit e534885b64 on Feb 10, 2021
  27. danben referenced this in commit 03e4524822 on Feb 10, 2021
  28. danben referenced this in commit 473c138d9f on Feb 10, 2021
  29. danben referenced this in commit 0849f805e0 on Feb 10, 2021
  30. danben referenced this in commit e89ee566b7 on Feb 10, 2021
  31. maflcko commented at 8:18 am on February 15, 2021: member
    There a quite a few miniwallet prs open, and review is a bit more important than writing new code. So if everyone who contributed code could help with review of the other open prs, that’d be great. https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+is%3Aopen+miniwallet
  32. danben referenced this in commit 21e12fa121 on Feb 19, 2021
  33. danben referenced this in commit 520e7799b5 on Feb 19, 2021
  34. maflcko referenced this in commit 06d573f053 on May 6, 2021
  35. sidhujag referenced this in commit 87d695985a on May 6, 2021
  36. maflcko referenced this in commit d8ae29ec8f on May 10, 2021
  37. sidhujag referenced this in commit cbcd58b7ad on May 10, 2021
  38. michaelfolkson commented at 10:43 am on May 17, 2021: contributor

    As far as I can tell there are around 40 tests from git grep ‘self.skip_if_no_wallet()’ excluding tests starting with wallet_ (From Marco’s initial description)

    Haven’t looked through to see which of the tests still to be updated to use MiniWallet would benefit from having access to P2PK outputs but #21945 is introducing them.

    Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)

  39. maflcko referenced this in commit 7e83e74e7f on Jun 1, 2021
  40. PastaPastaPasta referenced this in commit cbcc4edc46 on Jun 27, 2021
  41. PastaPastaPasta referenced this in commit eb6ca5b7c2 on Jun 28, 2021
  42. PastaPastaPasta referenced this in commit 52f6913d35 on Jun 29, 2021
  43. PastaPastaPasta referenced this in commit 87745721bb on Jul 1, 2021
  44. PastaPastaPasta referenced this in commit 0bdb90aaa0 on Jul 1, 2021
  45. maflcko referenced this in commit 489beb3984 on Aug 23, 2021
  46. maflcko referenced this in commit 2b264971ad on Sep 14, 2021
  47. maflcko referenced this in commit 33e31f8df9 on Sep 29, 2021
  48. maflcko referenced this in commit 368831371d on Nov 21, 2021
  49. maflcko referenced this in commit d3582f2d3b on Dec 27, 2021
  50. maflcko referenced this in commit 587cbca826 on Dec 28, 2021
  51. maflcko referenced this in commit e00e990606 on Jan 5, 2022
  52. sidhujag referenced this in commit d2bf72862b on Jan 6, 2022
  53. maflcko referenced this in commit 807169e10b on Jan 13, 2022
  54. pg156 commented at 3:44 pm on January 17, 2022: none
    I am working on mempool_updatefromblock.py and will open a PR once ready. It is a modified version of the closed PR #21999.
  55. maflcko referenced this in commit a41976ab77 on Feb 2, 2022
  56. hamzzy commented at 10:01 am on March 6, 2022: none
    I’m working on rpc_signrawtransaction.py and will open a PR once ready.
  57. ayush933 commented at 8:41 am on March 11, 2022: contributor
    I’m working on rpc_createmultisig.py. Not sure how long it will take since it will be my first commit.
  58. maflcko referenced this in commit f94784f5bc on Mar 13, 2022
  59. danielabrozzoni commented at 8:59 pm on March 19, 2022: contributor
    I’m working on rpc_rawtransaction.py, I’ll open a PR once it’s ready :)
  60. maflcko commented at 4:22 pm on March 23, 2022: member
    #21144 might be up for grabs
  61. maflcko commented at 4:23 pm on March 23, 2022: member
    Also, a reminder to help with review of the currently open miniwallet pulls: https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+is%3Aopen+miniwallet+label%3ATests
  62. fanquake referenced this in commit 8234cdaf62 on Mar 24, 2022
  63. maflcko referenced this in commit 864fb89b2f on Mar 24, 2022
  64. michaelfolkson commented at 11:39 am on March 24, 2022: contributor
    Something also to consider for future Miniwallet PRs (as discussed in #24635) is if your test is run twice in the test runner ensure that it is only run once (could be done in same PR) once the dependence on the Core wallet is removed. I’ll try to monitor these and do this if people forget.
  65. sidhujag referenced this in commit 416803d3a8 on Apr 2, 2022
  66. maflcko referenced this in commit ee9af95f09 on Apr 5, 2022
  67. maflcko referenced this in commit cd110cdd0e on Apr 11, 2022
  68. sidhujag referenced this in commit 06add764f5 on Apr 11, 2022
  69. brunoerg commented at 11:27 am on April 17, 2022: contributor
    We could have a list similar to #24783 to track the work. It would be easier to know what have been done and what is missing.
  70. maflcko referenced this in commit 8d3743a365 on Apr 19, 2022
  71. shommel commented at 8:09 pm on April 21, 2022: none

    First timer here: Seems mempool_packages.py is up for grabs. Planning on picking it up!

    Also would be happy to put together the list @brunoerg mentioned in the coming days 👍

  72. Eunoia1729 commented at 3:30 am on April 22, 2022: contributor

    #21144 might be up for grabs

    Picking feature_bip68_sequence.py.

  73. shommel commented at 2:43 pm on April 22, 2022: none

    @MarcoFalke Apologies for ping. As per the comment above, here’s a quick list of past, present, and future MiniWallet-converted tests. Hope it helps!

    List of remaining/non-converted functional tests: [] test/functional/feature_segwit.py [] test/functional/feature_taproot.py

    List of started/finished tests: [x] test/functional/feature_dbcrash.py (in progress here) [x] test/functional/rpc_signrawtransaction.py (in progress here ) [] test/functional/feature_bip68_sequence.py (in progress/shown interest here) [x] test/functional/mempool_updatefromblock.py (in progress #24183) [x] test/functional/mining_prioritisetransaction.py (in progress #24839) [x] test/functional/rpc_rawtransaction.py (in progress here ) [] test/functional/mempool_packages.py (in progress/shown interest here) [x] test/functional/p2p_leak_tx.py (added in #20126) [x] test/functional/mining_getblocktemplate_longpoll.py (added in #20159) [x] test/functional/mempool_expiry.py (added in #20276) [x] test/functional/mempool_spend_coinbase.py (added in #20385) [x] test/functional/mempool_compatibility.py (added in #20688) [x] test/functional/mempool_resurrect.py (added in #20692) [x] test/functional/mempool_reorg.py (added in #21178) [x] test/functional/p2p_blocksonly.py (added in #21867) [x] test/functional/feature_csv_activation.py (added in #21900) [x] test/functional/mempool_limit.py (added in #22543) [x] test/functional/rpc_signmessage.py (added in #22641) [x] test/functional/p2p_filter.py (added in #23079) [x] test/functional/rpc_generateblock.py (added in #23558) [x] test/functional/rpc_scantxoutset.py (added in #23866) [x] test/functional/p2p_compactblocks.py (added in #23873) [x] test/functional/mining_basic.py (added in #23978) [x] test/functional/mempool_accept.py (added in #24035) [x] test/functional/interface_rest.py (added in #24223) [x] test/functional/feature_maxuploadtarget.py (added in #24533) [x] test/functional/mempool_package_onemore.py (added in #24637) [x] test/functional/mempool_unbroadcast.py (added in #24749) [x] test/functional/feature_fee_estimation.py (added in #24817) [x] test/functional/p2p_segwit.py (added in #24896)

  74. danielabrozzoni commented at 6:19 pm on April 22, 2022: contributor

    I would remove example_test.py from the list, I don’t think it should really be “fixed”, it’s only showing how to skip a test if the module is missing :)

    Also, I’m still working on raw_transaction.py (see #20078 (comment)), I’m almost there :)

  75. shommel commented at 6:28 pm on April 22, 2022: none

    I would remove example_test.py from the list, I don’t think it should really be “fixed”, it’s only showing how to skip a test if the module is missing :)

    Fixed @danielabrozzoni. Thanks for the context!

    Also, I’m still working on raw_transaction.py (see #20078 (comment)), I’m almost there :)

    Added to the list, sorry, must’ve missed it. Thanks for pointing it out 👍

  76. michaelfolkson commented at 1:38 pm on May 12, 2022: contributor

    Thanks @shommel for the list! Didn’t realize how much progress had been made, not many left now.

    I was planning to play around with Taproot in Python at some point so I’ll take feature_taproot.py (somewhat surprised it is still there). If someone wants to take it off me feel free though.

  77. maflcko referenced this in commit 269fa667f2 on May 30, 2022
  78. laanwj referenced this in commit 6db7fbd812 on May 31, 2022
  79. maflcko referenced this in commit 9cc010f5a9 on Jun 1, 2022
  80. sidhujag referenced this in commit a7ad5d6dfa on Jun 1, 2022
  81. maflcko referenced this in commit 506d9b25a3 on Jun 13, 2022
  82. kouloumos commented at 10:02 am on June 17, 2022: contributor

    From the description of the issue

    This is an open-ended issue. You can find candidate tests via git grep ‘self.skip_if_no_wallet()’

    I understand why those tests are the obvious candidates for MiniWallet usage, but are those the only tests that could benefit from MiniWallet usage?

    During the process of converting tests to use the MiniWallet, a number of practical methods have been created thus enabling more readable tests by using the MiniWallet class. I am trying to understand if the usage of the MiniWallet to simplify tests that can already run with the Bitcoin Core wallet disabled (e.g. #25379) is something that could be considered beneficial.

  83. pablomartin4btc commented at 8:57 pm on November 22, 2022: member
    Considering segwit has been implemented long time ago… Is it worth it to convert test/functional/feature_segwit.py into MiniWallet?
  84. maflcko commented at 1:36 pm on November 28, 2022: member
    Yes, anything without [x] in #20078 (comment) is up for grabs
  85. maflcko commented at 2:42 pm on November 29, 2022: member
    rpc_psbt looks like a wallet test, so maybe it can be renamed to wallet_psbt.py
  86. miles170 commented at 6:04 am on December 2, 2022: contributor
    Picking test/functional/feature_segwit.py.
  87. maflcko commented at 10:57 am on December 2, 2022: member
    Nice. Pull requests welcome, however I am going to close this issue for now. The remaining tests can not be trivially converted without also extending MiniWallet (I think), so it might be better to open specific threads/issues for each test.
  88. maflcko closed this on Dec 2, 2022
  89. maflcko referenced this in commit 599e941c19 on Jan 16, 2023
  90. vijaydasmp referenced this in commit 1adcc9af0b on Jul 12, 2023
  91. vijaydasmp referenced this in commit a1629eefa0 on Jul 13, 2023
  92. vijaydasmp referenced this in commit 639ff64967 on Aug 4, 2023
  93. vijaydasmp referenced this in commit d98ce3d566 on Aug 5, 2023
  94. vijaydasmp referenced this in commit 52edfa4aaf on Aug 6, 2023
  95. vijaydasmp referenced this in commit bc222e06bb on Aug 6, 2023
  96. vijaydasmp referenced this in commit db8abef1c2 on Aug 6, 2023
  97. vijaydasmp referenced this in commit 6f958ed03e on Aug 6, 2023
  98. bitcoin locked this on Dec 2, 2023


maflcko dboures stackman27 michaelfolkson practicalswift DariusParvin pg156 hamzzy ayush933 danielabrozzoni brunoerg shommel Eunoia1729 kouloumos pablomartin4btc miles170

Labels
good first issue

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